Re: [PATCH v4 0/5] Introduce the Counter character device interface

[William Breathitt Gray]

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 <vilhelm.gray@xxxxxxxxx> wrote:
> > 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.

> > 
> > 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.

> > 
> > 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

Attachment: signature.asc
Description: PGP signature