On Sun, Aug 09, 2020 at 02:48:00PM +0100, Jonathan Cameron wrote:

On Tue, 21 Jul 2020 15:35:46 -0400
William Breathitt Gray [off-list ref] wrote:

quoted

The following are some questions I have about this patchset:

1. Should I support multiple file descriptors for the character device
   in this introduction patchset?

   I intend to add support for multiple file descriptors to the Counter
   character device, but I restricted this patchset to a single file
   descriptor to simplify the code logic for the sake of review. If
   there is enough interest, I can add support for multiple file
   descriptors in the next revision; I anticipate that this should be
   simple to implement through the allocation of a kfifo for each file
   descriptor during the open callback.

What is the use case?  I can conjecture one easily enough, but I'm not
sure how real it actually is.  We've been around this question a few
times in IIO :)

Certainly makes sense to design an interface that would allow you to
add this support later if needed though.

I don't have any particular use case in mind, but I figured it would be
useful. For example, a counter device can have multiple channels with
their own events, but any particular channel might be counting the
signals of an independent device unrelated to the other channels; in
this scenario, two independent user applications might need access to
the same counter device.

Of course, supporting multiple file descriptors is something that can be
added later so perhaps it's best for us to wait until the need arises
with a real-life use case.

quoted

2. Should struct counter_event have a union for different value types,
   or just a value u8 array?

   Currently I expose the event data value via a union containing the
   various possible Counter data types (value_u8 and value_u64). It is
   up to the user to select the right union member for the data they
   received. Would it make sense to return this data in a u8 array
   instead, with the expectation that the user will cast to the
   necessary data type?

Be careful on alignment if you do that. We would need to ensure that the
buffer is suitable aligned for a cast to work as expected.

That's a fair point. It's probably safer to continue with a union which
also has the benefit of making the possible returned types clearer to
see in the code.

quoted

3. How should errors be returned for Counter data reads performed by
   Counter events?

   Counter events are configured with a list of Counter data read
   operations to perform for the user. Any one of those data reads can
   return an error code, but not necessarily all of them. Currently, the
   code exits early when an error code is returned. Should the code
   instead continue on, saving the error code to the struct
   counter_event for userspace to handle?

I'd argue that errors are expected to be rare, so it isn't a problem
to just fault out hard on the first one.

All right, that should help keep the error logic simple too then.

William Breathitt Gray