Message-ID: <20200624235858.GB6751@sol>
Date:   Thu, 25 Jun 2020 07:58:58 +0800
From:   Kent Gibson <warthog618@...il.com>
To:     Bartosz Golaszewski <bgolaszewski@...libre.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        linux-gpio <linux-gpio@...r.kernel.org>,
        Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH 13/22] gpio: uapi: define uAPI V2

On Wed, Jun 24, 2020 at 04:36:22PM +0200, Bartosz Golaszewski wrote:
> wt., 23 cze 2020 o 06:02 Kent Gibson <warthog618@...il.com> napisał(a):
> >
[ snip ]
> > +
> > +/*
> > + * Struct padding sizes.
> > + *
> > + * These are sized to pad structs to 64bit boundaries.
> > + * To maintain 32/64bit alignment, any arbitrary change must be even, as
> > + * the pad elements are __u32.
> > + */
> > +#define GPIOLINE_CONFIG_PAD_SIZE               7
> > +#define GPIOLINE_REQUEST_PAD_SIZE              5
> > +#define GPIOLINE_INFO_V2_PAD_SIZE              5
> > +#define GPIOLINE_INFO_CHANGED_V2_PAD_SIZE      5
> > +#define GPIOLINE_EVENT_PAD_SIZE                        2
> > +
> 
> Did someone request such definitions? IMO it's one of those rare
> instances where I'd prefer to see magic numbers used in the structs.
> I'm not sure if we need this indirection.
> 

Not explicitly.  Andy requested documentation on the pad sizes, which
seemed reasonable, and it also seemed reasonable to me to group them in
the one place rather than repeat the doco for each struct.

But having the size of the pad visible in the struct definition is
preferable, as it is only really relevant to that struct.
And everyone wants to keep the uAPI definitions as short possible, so
will drop these in v2.

> > +/**
> > + * struct gpioline_values - Values of GPIO lines
> > + * @bitmap: a bitmap containing the value of the lines, set to 1 for active
> > + * and 0 for inactive.  Note that this is the logical value, which will be
> > + * the opposite of the physical value if GPIOLINE_FLAG_V2_ACTIVE_LOW is
> > + * set in flags.
> > + */
> > +struct gpioline_values {
> > +       __u64 bitmap[GPIOLINES_BITMAP_SIZE];
> > +};
> 
> Ok so now when embedding this structure we get something like
> "config.values.bitmap". This looks fine with the exception that "bits"
> would be even shorter and the information about this field being a
> bitmap is not necessary. I hope this isn't too much bikeshedding. :)
> 

Fair enough - bits it is.

> > +
> > +/**
> > + * struct gpioline_config - Configuration for GPIO lines
> > + * @values: if the direction is GPIOLINE_DIRECTION_OUTPUT, the values that
> > + * the lines will be set to.  This field is write-only and is zeroed when
> > + * returned within struct gpioline_info.
> > + * @flags: flags for the GPIO lines, such as GPIOLINE_FLAG_V2_ACTIVE_LOW,
> > + * GPIOLINE_FLAG_V2_DIRECTION etc, OR:ed together
> > + * @direction: if GPIOLINE_FLAG_V2_DIRECTION is set in flags, the desired
> > + * direction for the requested lines, with a value from enum
> > + * gpioline_direction
> > + * @drive: if GPIOLINE_FLAG_V2_DRIVE is set in flags, the desired drive for
> > + * the requested lines, with a value from enum gpioline_drive
> > + * @bias: if GPIOLINE_FLAG_V2_BIAS is set in flags, the desired bias for
> > + * the requested lines, with a value from enum gpioline_bias
> > + * @edge_detection: if GPIOLINE_FLAG_V2_EDGE_DETECTION is set in flags, the
> > + * desired edge_detection for the requested lines, with a value from enum
> > + * gpioline_edge
> > + * @debounce_period: if GPIOLINE_FLAG_V2_DEBOUNCE is set in flags, the
> > + * desired debounce period for the requested lines, in microseconds
> > + * @padding: reserved for future use and must be zero filled
> > + */
> > +struct gpioline_config {
> > +       struct gpioline_values values;
> > +       __u32 flags;
> > +       /* Note that the following four fields are equivalent to a single u32. */
> > +       __u8 direction;
> > +       __u8 drive;
> > +       __u8 bias;
> > +       __u8 edge_detection;
> 
> Limiting the size of these enum fields doesn't improve performance in
> any measurable way. We already have 4 possible values for events. I'm
> afraid that at some point in the future we'll add support for level
> events or something else etc. and we'll run into a problem because we
> only have 8 bits available. If there are no objections, I'd make it 32
> bits wide.
> 

Agreed - the reduction in size isn't worth the effort.

> For the same reason I was thinking that making flags 64 bits wouldn't
> hurt either.
> 

OK

[ snip ]
> > + * struct gpioline_event - The actual event being pushed to userspace
> > + * @timestamp: best estimate of time of event occurrence, in nanoseconds.
> > + * The timestamp is read from CLOCK_MONOTONIC and is intended to allow the
> > + * accurate measurement of the time between events.  It does not provide
> > + * the wall-clock time.
> > + * @id: event identifier with value from enum gpioline_event_id
> > + * @offset: the offset of the line that triggered the event
> > + * @seqno: the sequence number for this event in the sequence of events for
> > + * all the lines in this line request
> > + * @line_seqno: the sequence number for this event in the sequence of
> > + * events on this particular line
> > + * @padding: reserved for future use
> > + */
> > +struct gpioline_event {
> > +       __u64 timestamp;
> > +       __u32 id;
> > +       __u32 offset;
> > +       __u32 seqno;
> > +       __u32 line_seqno;
> > +       __u32 padding[GPIOLINE_EVENT_PAD_SIZE]; /* for future use */
> 
> Any reason for only having 64 bits of padding? 128 wouldn't change much, right?
> 

No, as I mentioned in the commentary, I'm concerned that I've gone
too small with the padding, so happy to bump that.
And I'm open to suggestions on the other pads as well.

Cheers,
Kent.

Powered by blists - more mailing lists