Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL

[Kent Gibson]

On Mon, May 04, 2020 at 12:31:57PM +0200, Bartosz Golaszewski wrote:
> czw., 30 kwi 2020 o 16:58 Kent Gibson <warthog618@xxxxxxxxx> napisaÅ(a):
> >
> > On Thu, Apr 30, 2020 at 01:32:22PM +0000, Bujanda, Hector wrote:
> > > Thanks all for your guidance!
> > >
> > > First saying that this patch request was sent having our platforms in k4.14 in the way of upgrading to k5.4.
> > > In those versions the commit e588bb1eae31be73fbec2b731be986a7c09635a4 "gpio: add new SET_CONFIG ioctl() to gpio chardev" by Kent Gibson was not available.
> > >
> > > I see that you clearly understand the necessity of having a way of configuring debounce from the userspace.
> > > Our platforms make use of hardware debouncing filtering. Up to now we were using the sysfilesystem to let the user handle gpios (including debounce configuration).
> > > We wanted now to get rid of sysfilesystem and start using gpiolib/libgpiod.... but configuring debounce is blocking us.
> > >
> > > Now I clearly see (as pointed by Bartosz Golaszewski) that my suggested GPIO_SET_DEBOUNCE_IOCTL is wrong as it hits the chip file descriptor while 'Modifying any config settings can only happen on lines previously requested too in user-space'.
> > >
> > > I agree with all that a flag is needed to allow configuring debounce to '0' which has always meant disabling it.
> > >
> > > Also agree with 'Kent Gibson' suggestion of  'You might want to add a flag to the GPIOLINE_FLAGs to indicate if debounce is set'.
> > >
> > > I have my doubts if it is compulsory to extend debounce configuration to the gpioevent_requests since the debounce value configured by a user is normally linked to a hardware noise in a line; and that does not change from one gpioevent_requests to another. So I think this configuration would be useful but not compulsory.
> > >
> >
> > Just to clarify on this point, the reason the SET_CONFIG would have to
> > be extended to events is not to alter the debounce on the fly but to set
> > it at all.  Lines are requested as either handles (for outputs or polled inputs)
> > or events (for asynchronous edge events on inputs). We cannot extend
> > either the handle or event request ioctls themselves as there is no provision
> > in their data structures for future expansion.  There is in the
> > SET_CONFIG ioctl - but that doesn't apply to event requests yet...
> >
> 
> Indeed. And as I was thinking about it over the weekend I realized
> that exposing a setter for a config option that's not settable at
> request-time seems wrong. Together with the lineevent structure which
> doesn't work on 64-bit kernel with 32-bit user-space this all makes me
> think we should design v2 of several of the ioctl() calls with more
> care.
> 
> >
> > > I agree with Linus Walleij that 'there is a serious user-facing problem here though, because not all GPIO controllers supports debounce'.
> > > Our platforms have native freescale/NXP gpiochips not supporting hardware debounce and our own gpiochips having hardware debounce.
> > > We have also noticed that 'drivers/input/keyboard/gpio_keys.c contains generic debounce code using kernel timers if the GPIO driver cannot provide debouncing'. That feature is not of our interest (because of having hardware debounce filters) but it would clearly be a very good overall functionality.
> > >
> > > Having said all above, I wonder how you want to proceed.
> > > Our current development in k5.4 and libgpiod1.4.1 is much behind master... what makes collaboration (and reusability) a bit more complex.
> > > Also I see the implementation requires a bigger picture than I initially expected.
> > > So I wonder if you want me to do the initial steps of the development (what I foresee will require some back and forth) or you prefer implementing all pieces.
> > >
> >
> > I totally agree with you on the widening scope.
> >
> > Bart - how do you want to go forward with this?  I'm available to work
> > on it, in part or full.
> >
> 
> Personally I'm super busy with my actual job and adding support for
> line watch ioctl() to libgpiod ATM. I can't really spare any time on
> this. I have some crazy ideas: like storing the debounce time in the
> 16 most significant bits of the flags field but this is just papering
> over bad ABI.
> 
> Ideally we'd have to introduce new versions of gpioevent_request,
> gpioline_request, gpioline_info and gpioevent_data structs - this time
> with enough additional padding and no alignment issues. Then we could
> add the debounce properly.
> 

Agreed - adding setting only via the SET_CONFIG ioctl is a bit of a
hack, and a v2 of the uAPI is required to add it to the requests, and
to add the debounce value to the info.

> This would of course add a lot of cruft to the uAPI code. I'd start by
> moving it out of drivers/gpio/gpiolib.c into a new file:
> drivers/gpio/gpiolib-cdev.c. This way we'd have everything related to
> the character device in one place. It would make it easier to: a) add
> a config option for disabling it entirely and b) add a config option
> to disable the v1 of the ioctl()s.
> 

Ok, that is widening the scope of the change again, but I'm still willing
to have a go at it.  Wrt a v2, I was considering re-organising how the
flag field works, generally using it to indicate the presence of other
fields in the request, rather than trying to encode a capability just in
the flags.  e.g. direction, drive, bias and debounce would each have a
single flag as well as a field, or fields, containing their actual value.

For requests and sets, the flags indicate the capabilities being
explicitly set.  For info it would indicate the fields populated.

This would simplify and clarify what combinations of flags actually make
sense and what is actually being set or left unchanged.

Polarity (ActiveLow) is a bit different, as it is purely a binary and is
always set, one way or the other, so it could remain just a flag.

If we need to add a new capability then we use the next bit in the flags
and some of the padding reserved for future use for the field(s).

Does that make sense?

I am also wondering if we could merge the handle and event requests by
making edge detection just another capability?  Maybe there is something
I'm missing here.  What was the rationale for them being separate?

Cheers,
Kent.

> I also Cc'ed Andy who may have some better ideas.
> 
> Linus: about the software-debounce you mentioned: do you think it
> somehow plugs the hole we identified here?
> 
> Bart