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: <20200419001858.105281-1-hector.bujanda@digi.com>
 <CAMRc=MeHun_WEApEXP59ZszGa2n+wbU9qq3wU1VO9o590rO-Pw@mail.gmail.com>
 <CACRpkdaeXFW5K=Npy2ubWsffc7aepEQ5kSJ2HrkrESjaTy_psQ@mail.gmail.com>
 <CAMRc=MdwSpWkgLTHN+6cOdG7aBAWWYFBC4+tfSNtA2HgX6s_3A@mail.gmail.com>
 <B0E9AFA73AF60B42B6D323E0C4FEB06F01AFAC5A@dor-sms-xch01.digi.com> <20200430145844.GA28588@sol>
In-Reply-To: <20200430145844.GA28588@sol>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Mon, 4 May 2020 12:31:57 +0200
Message-ID: <CAMRc=Md5-OgNySDG+XHKow0YSzcZHNtWWPwbmd159fpWL8YAJA@mail.gmail.com>
Subject: Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL
To:     Kent Gibson <warthog618@gmail.com>
Cc:     "Bujanda, Hector" <Hector.Bujanda@digi.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

czw., 30 kwi 2020 o 16:58 Kent Gibson <warthog618@gmail.com> napisa=C5=82(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 k=
4.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 av=
ailable.
> >
> > I see that you clearly understand the necessity of having a way of conf=
iguring debounce from the userspace.
> > Our platforms make use of hardware debouncing filtering. Up to now we w=
ere using the sysfilesystem to let the user handle gpios (including debounc=
e configuration).
> > We wanted now to get rid of sysfilesystem and start using gpiolib/libgp=
iod.... 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 request=
ed 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 f=
lag to the GPIOLINE_FLAGs to indicate if debounce is set'.
> >
> > I have my doubts if it is compulsory to extend debounce configuration t=
o the gpioevent_requests since the debounce value configured by a user is n=
ormally 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 provi=
sion
> 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 hardwa=
re 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 hardwa=
re debounce filters) but it would clearly be a very good overall functional=
ity.
> >
> > 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 initiall=
y 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.

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.

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