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;
Date:   Wed, 13 May 2020 12:33:33 +0800
From:   Kent Gibson <warthog618@gmail.com>
To:     Linus Walleij <linus.walleij@linaro.org>
Cc:     Bartosz Golaszewski <brgl@bgdev.pl>,
        "Bujanda, Hector" <Hector.Bujanda@digi.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Andy Shevchenko <andy.shevchenko@gmail.com>
Subject: Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL
Message-ID: <20200513043333.GA10419@sol>
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>
 <CAMRc=Md5-OgNySDG+XHKow0YSzcZHNtWWPwbmd159fpWL8YAJA@mail.gmail.com>
 <CACRpkdbZPhkzuUvwDnBWTvweBukQRcWx0w=2seQsVBEP8Fv_BA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CACRpkdbZPhkzuUvwDnBWTvweBukQRcWx0w=2seQsVBEP8Fv_BA@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, May 12, 2020 at 07:55:42PM +0200, Linus Walleij wrote:
> On Mon, May 4, 2020 at 12:32 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 

I hope Bart doesn't mind if I jump in here, but I've started working on
this so hopefully I can address most of your points...

> > 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.
> 
> Hm that sounds massive. Is it really that bad?
> 

Agreed - it is massive - we end up replacing the majority of the
existing structs and ioctls.

If we want to be able to set debounce in the request(s), not just in
SET_CONFIG, then we need new requests as there is no room in the
existing.  If we want to be able to report that config in the info then
we need new infos for the same reason.  The info_changed contains an
info so that has to change as well. And the event_data has a 32/64bit
alignment issue so it was already up for replacement.

So it could be worse, but not much.

> > 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.
> 
> Its good to break out for code maintenance no matter what we do
> with it :)
>

It definitely is, and I'll submit a patch soon, that hopefully can be
applied immediately before the next dev window opens, to do just that.

> I would however not make it in any way totally optional, because the
> big win with the character device over the legacy sysfs is to always
> be available.
> 

And if you build it into your kernel, which will be the default, it
still will be.

But maybe there are specific applications that don't need cdev and
would be interested in reducing kernel bloat?

> > Linus: about the software-debounce you mentioned: do you think it
> > somehow plugs the hole we identified here?
> 
> Hm, I don't quite understand what the hole is I guess...
> 

I'll leave this one for Bart - the more I re-read the thread the less
certain I am as well.

I will note that Bart correctly mentioned that the uapi doesn't return
an error if the user requests bias that is not supported by the driver
- gpio_set_bias absorbs the error.

That isn't by intent - it is the way gpiod_direction_input
behaved before I added bias to cdev. It was left that way as I was
unsure on the broader implications of changing it, and wasn't keen on
implementing a cdev specific gpiod_direction_input either.
I'm open to suggestions if you would like to change that.

Cheers,
Kent.