Re: [RFC PATCH] gpio: uapi: v2 proposal

[Kent Gibson]

On Fri, Jun 05, 2020 at 11:53:05AM +0200, Bartosz Golaszewski wrote:
> czw., 4 cze 2020 o 18:00 Kent Gibson <warthog618@xxxxxxxxx> napisaÅ(a):
> >
> 
> [snip!]
> 
> > > > +
> > > > +enum gpioline_edge {
> > > > +       GPIOLINE_EDGE_NONE              = 0,
> > > > +       GPIOLINE_EDGE_RISING            = 1,
> > > > +       GPIOLINE_EDGE_FALLING           = 2,
> > > > +       GPIOLINE_EDGE_BOTH              = 3,
> > > > +};
> > >
> > > I would skip the names of the enum types if we're not reusing them anywhere.
> > >
> >
> > I thought it was useful to name them even if it was just to be able to
> > reference them in the documentation for relevant fields, such as that in
> > struct gpioline_config below, rather than having to either list all
> > possible values or a GPIOLINE_EDGE_* glob.
> >
> > And I'm currently using enum gpioline_edge in my edge detector
> > implementation - is that sufficient?
> >
> 
> The documentation argument is more convincing. :)
> 

I know - but your criteria was reuse... ;-).

> > > > +
> > > > +/* Line flags - V2 */
> > > > +#define GPIOLINE_FLAG_V2_KERNEL                (1UL << 0) /* Line used by the kernel */
> > >
> > > In v1 this flag is also set if the line is used by user-space. Maybe a
> > > simple GPIOLINE_FLAG_V2_USED would be better?
> > >
> >
> > Agreed - the _KERNEL name is confusing.
> > In my latest draft I've already renamed it GPIOLINE_FLAG_V2_BUSY,
> > as EBUSY is what the ioctl returns when you try to request such a line.
> > Does that work for you?
> > I was also considering _IN_USE, and was using _UNAVAILABLE for a while.
> >
> 
> BUSY sounds less precise to me than USED or IN_USE of which both are
> fine (with a preference for the former).
>

OK, USED it shall be.

> [snip!]
> 
> > > > +
> > > > +/**
> > > > + * struct gpioline_values - Values of GPIO lines
> > > > + * @values: when getting the state of lines this contains the current
> > > > + * state of a line, when setting the state of lines these should contain
> > > > + * the desired target state
> > > > + */
> > > > +struct gpioline_values {
> > > > +       __u8 values[GPIOLINES_MAX];
> > >
> > > Same here for bitfield. And maybe reuse this structure in
> > > gpioline_config for default values?
> > >
> >
> > Can do.  What makes me reticent is the extra level of indirection
> > and the stuttering that would cause when referencing them.
> > e.g. config.default_values.values
> > So not sure the gain is worth the pain.
> >
> 
> I'd say yes - consolidation and reuse of data structures is always
> good and normally they are going to be wrapped in some kind of
> low-level user-space library anyway.
> 

Ok, and I've changed the values field name to bitmap, along with the change
to a bitmap type, so the stuttering is gone.

And, as the change to bitmap substantially reduced the size of
gpioline_config, I now embed that in the gpioline_info instead of
duplicating all the other fields.  The values field will be zeroed
when returned within info.

> > And I've renamed "default_values" to just "values" in my latest draft
> > which doesn't help with the stuttering.
> >
> 
> Why though? Aren't these always default values for output?
> 

To me "default" implies a fallback value, and that de-emphasises the
fact that the lines will be immediately set to those values as they
are switched to outputs.
These are the values the outputs will take - the "default" doesn't add
anything.

> [snip!]
> 
> > > > +
> > > > +/**
> > > > + * struct gpioline_event - The actual event being pushed to userspace
> > > > + * @timestamp: best estimate of time of event occurrence, in nanoseconds
> > > > + * @id: event identifier with value from enum gpioline_event_id
> > > > + * @offset: the offset of the line that triggered the event
> > > > + * @padding: reserved for future use
> > > > + */
> > > > +struct gpioline_event {
> > > > +       __u64 timestamp;
> > >
> > > I'd specify in the comment the type of clock used for the timestamp.
> > >
> >
> > Agreed - as this one will be guaranteed to be CLOCK_MONOTONIC.
> >
> > I'm also kicking around the idea of adding sequence numbers to events,
> > one per line and one per handle, so userspace can more easily detect
> > mis-ordering or buffer overflows.  Does that make any sense?
> >
> 
> Hmm, now that you mention it - and in the light of the recent post by
> Ryan Lovelett about polling precision - I think it makes sense to have
> this. Especially since it's very easy to add.
> 

OK.  I was only thinking about the edge events, but you might want it
for your line info events on the chip fd as well?

> > And would it be useful for userspace to be able to influence the size of
> > the event buffer (currently fixed at 16 events per line)?
> >
> 
> Good question. I would prefer to not overdo it though. The event
> request would need to contain the desired kfifo size and we'd only
> allow to set it on request, right?
>

Yeah, it would only be relevant if edge detection was set and, as per
edge detection itself, would only be settable via the request, not
via set_config.  It would only be a suggestion, as the kfifo size gets
rounded up to a power of 2 anyway.  It would be capped - I'm open to
suggestions for a suitable max value.  And the 0 value would mean use
the default - currently 16 per line.

If you want the equivalent for the info watch then I'm not sure where to
hook it in.  It should be at the chip scope, and there isn't any
suitable ioctl to hook it into so it would need a new one - maybe a
set_config for the chip?  But the buffer size would only be settable up
until you add a watch.

Cheers,
Kent.