Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200516064507.19058-1-warthog618@gmail.com> <CAMpxmJUbC4qmUGM0Z-6hXsYPRSpEpNM7iXgc7XbMcf_epi0Lig@mail.gmail.com>
 <20200604160006.GA5730@sol>
In-Reply-To: <20200604160006.GA5730@sol>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Fri, 5 Jun 2020 11:53:05 +0200
Message-ID: <CAMRc=MfS1sCTU3vs5Gq_6+Ubt_89HX34mqabtpGbAASo+SfzSw@mail.gmail.com>
Subject: Re: [RFC PATCH] gpio: uapi: v2 proposal
To:     Kent Gibson <warthog618@gmail.com>
Cc:     Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-gpio <linux-gpio@vger.kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

czw., 4 cze 2020 o 18:00 Kent Gibson <warthog618@gmail.com> napisa=C5=82(a)=
:
>

[snip!]

> > > +
> > > +enum gpioline_edge {
> > > +       GPIOLINE_EDGE_NONE              =3D 0,
> > > +       GPIOLINE_EDGE_RISING            =3D 1,
> > > +       GPIOLINE_EDGE_FALLING           =3D 2,
> > > +       GPIOLINE_EDGE_BOTH              =3D 3,
> > > +};
> >
> > I would skip the names of the enum types if we're not reusing them anyw=
here.
> >
>
> 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 us=
ed 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 curren=
t
> > > + * state of a line, when setting the state of lines these should con=
tain
> > > + * 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 userspac=
e
> > > + * @timestamp: best estimate of time of event occurrence, in nanosec=
onds
> > > + * @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