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>
In-Reply-To: <20200419001858.105281-1-hector.bujanda@digi.com>
From:   Bartosz Golaszewski <brgl@bgdev.pl>
Date:   Wed, 29 Apr 2020 14:06:35 +0200
Message-ID: <CAMRc=MeHun_WEApEXP59ZszGa2n+wbU9qq3wU1VO9o590rO-Pw@mail.gmail.com>
Subject: Re: [PATCH] gpiolib: add GPIO_SET_DEBOUNCE_IOCTL
To:     Hector Bujanda <hector.bujanda@digi.com>
Cc:     Linus Walleij <linus.walleij@linaro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Kent Gibson <warthog618@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

niedz., 19 kwi 2020 o 02:19 Hector Bujanda <hector.bujanda@digi.com> napisa=
=C5=82(a):
>
> This allows calling gpiod_set_debounce function through char device ioctl=
.
>
> Signed-off-by: Hector Bujanda <hector.bujanda@digi.com>
> ---

Hi Hector,

please keep in mind to Cc me on GPIO patches - especially when
touching uAPI. For uAPI you can also Cc Kent Gibson for a second
opinion.

>  drivers/gpio/gpiolib.c    | 12 ++++++++++++
>  include/uapi/linux/gpio.h | 12 ++++++++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 70f0dedca59f..c959c2962f15 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1073,6 +1073,18 @@ static long gpio_ioctl(struct file *filp, unsigned=
 int cmd, unsigned long arg)
>                 if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
>                         return -EFAULT;
>                 return 0;
> +       } else if (cmd =3D=3D GPIO_SET_DEBOUNCE_IOCTL) {
> +               struct gpioline_debounce linedebounce;
> +               struct gpio_desc *desc;
> +
> +               if (copy_from_user(&linedebounce, ip, sizeof(linedebounce=
)))
> +                       return -EFAULT;
> +               if (linedebounce.line_offset >=3D gdev->ngpio)
> +                       return -EINVAL;
> +
> +               desc =3D &gdev->descs[linedebounce.line_offset];
> +
> +               return gpiod_set_debounce(desc, linedebounce.debounce_use=
c);

As Linus pointed out: adding a new ioctl() for this is out of question
- especially if this new ioctl() would be called on the chip file
descriptor. Modifying any config settings can only happen on lines
previously requested too in user-space.

>         } else if (cmd =3D=3D GPIO_GET_LINEHANDLE_IOCTL) {
>                 return linehandle_create(gdev, ip);
>         } else if (cmd =3D=3D GPIO_GET_LINEEVENT_IOCTL) {
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 1bf6e6df084b..4b092990d4c8 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -53,6 +53,17 @@ struct gpioline_info {
>         char consumer[32];
>  };
>
> +/**
> + * struct gpioline_debounce - GPIO line debounce
> + * @line_offset: the local offset on this GPIO device, fill this in when
> + * requesting the line information from the kernel
> + * @debounce_usec: debounce in uSeconds to set for this line
> + */
> +struct gpioline_debounce {
> +       __u32 line_offset;
> +       __u32 debounce_usec;
> +};
> +
>  /* Maximum number of requested handles */
>  #define GPIOHANDLES_MAX 64
>
> @@ -154,5 +165,6 @@ struct gpioevent_data {
>  #define GPIO_GET_LINEINFO_IOCTL _IOWR(0xB4, 0x02, struct gpioline_info)
>  #define GPIO_GET_LINEHANDLE_IOCTL _IOWR(0xB4, 0x03, struct gpiohandle_re=
quest)
>  #define GPIO_GET_LINEEVENT_IOCTL _IOWR(0xB4, 0x04, struct gpioevent_requ=
est)
> +#define GPIO_SET_DEBOUNCE_IOCTL _IOW(0xB4, 0x05, struct gpioline_debounc=
e)
>
>  #endif /* _UAPI_GPIO_H_ */
> --
> 2.17.1
>

I understand the need to set debounce time to make line events
reliable. As I see it: there'll be a couple steps to add this.

First: this information (debounce setting) isn't exported to
user-space in any way yet. While we can't extend the gpioline_info
structure because there's no padding for future use (unfortunately :(
) we should at least have a flag coming after
GPIOHANDLE_REQUEST_BIAS_DISABLE that would indicate to user-space that
the line is debounced at all e.g. GPIOHANDLE_REQUEST_DEBOUNCED.

At the same time as the above: the line state change notifier chain
must be called from gpiod_set_debounce() - in the end: if we export
this information to the user-space, we also need to notify it when it
changes.

Next: the SET_CONFIG ioctl() should be extended to work with lineevent
file descriptors too (of course - not all options would make sense
here and they'd need to be properly filtered).

Finally: we can extend the gpiohandle_config structure with a field
containing the debounce time which would be read by the kernel if the
debounce flag is set in gpiohandle_config.flags.

Does this make sense?

Bart