Date:   Fri, 26 Jun 2020 22:02:11 +0800
From:   Kent Gibson <warthog618@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>,
        Linus Walleij <linus.walleij@...aro.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>
Subject: Re: [PATCH 13/22] gpio: uapi: define uAPI V2

On Wed, Jun 24, 2020 at 11:40:57PM +0800, Kent Gibson wrote:
> On Wed, Jun 24, 2020 at 05:33:26PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 23, 2020 at 7:04 AM Kent Gibson <warthog618@...il.com> wrote:
> > >
> > > Add a new version of the uAPI to address existing 32/64bit alignment
> > 

[ snip ]
> > 
> > I'm wondering how many lines (in average) the user usually changes at
> > once? One? Two?
> > 
> > Perhaps we need to be better with this, something like single line /
> > multiple lines?
> > 
> > So, having a struct for single line change being embedded multiple
> > times would allow to configure atomically several lines with different
> > requirements.
> > For example you can turn directions of the two lines for some kind of
> > half-duplex bit banging protocol.
> > 
> > I'm not sure about the rest, but to me it seems reasonable to have
> > single vs. multiple separation in some of the structures.
> > 
> 

I think you are right about this - we should be taking the opportunity
to remove the homogeneous config restriction, so we can have a mixture
of lines with different configs in the one request.
e.g. I've had a request to have lines with different active low settings.
Your example requires both inputs and outputs.
And for a recent rotary encoder example I would've liked to be able to
have edge detection on one line, but return a snapshot with the state
of several lines in the edge event.

So what I'm thinking is to replace the config that I had proposed with
something like this:

struct attrib {
	/*
	 * the set of lines from request.offsets that this attrib DOESN'T
	 * apply to.  A zero value means it applies to all lines.
     * A set bit means the line request.offsets[bit] does NOT get this
     * attribute.
	 */
	__u64 mask[GPIOLINES_BITMAP_SIZE];

	/*
	 * an attribute identifier that dictates the which of the union
	 * fields is valid and how it is interpreted.
	 */
	__u32 id;

	union {
		__u64 flags; /* similar to v1 handleflags + eventflags */
		__u32 debounce_period;
		__u64 values[GPIOLINES_BITMAP_SIZE]; /* for id == output */
        /* ... */

		/* future config values go here */

		/*
		 * padding to ensure 32/64-bit alignment of attrib
		 *
		 * This must be the largest sized value.
		 */
		__u32 padding[3];
	};
};

/* config is a stack of attribs associated with requested lines. */
struct config {
	/* the number of populated attribs */
	__u32 num_attribs;

	/* for 32/64-bit alignment */
	__u32 padding;

	struct attrib attribs[MAX_CONFIG_ATTRIBS];
};

The idea is a stack of attributes, each of which can be applied to a
subset of the requested lines, as determined by the attrib.mask.
The config for a given attribute on a given line is determined by
finding the first match while walking down the stack, or falling
back to the sensible default if no match is found.

To reduce the number of attributes required, a number of boolean or
boolean-ish fields can be combined into flags, similar to v1.

I'm guessing a handful of attribs would suffice for the vast majority of
cases, so MAX_CONFIG_ATTRIBS would be in the 5-10 range.
(Are we concerned about struct sizes?)

Adding new config attribs in the future would involve adding a new id
and a corresponding value in the union.  So we can potentially add as
many new config attribs as we like - though the user would still be
limited to MAX_CONFIG_ATTRIBS and may have to trade-off attribs in
their particular application.

Does that make any sense?

Cheers,
Kent.

Powered by blists - more mailing lists