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

[Bartosz Golaszewski]

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

> > > +
> > > +/* 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).

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

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

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

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

[snip!]

Bartosz