Re: [RFC] gpio: expose pull-up/pull-down line flags to userspace

[Kent Gibson <warthog618@xxxxxxxxx>]

On Sat, Sep 21, 2019 at 12:25:23PM +0200, Drew Fustini wrote:
> Add pull-up/pull-down flags to the gpio line get and
> set ioctl() calls.  Use cases include a push button
> that does not have an external resistor.
> 
> Addition use cases described by Limor Fried (ladyada) of
> Adafruit in this PR for Adafruit_Blinka Python lib:
> https://github.com/adafruit/Adafruit_Blinka/pull/59
> 
> Signed-off-by: Drew Fustini <drew@xxxxxxxx>
> ---
>  drivers/gpio/gpiolib.c    | 12 ++++++++++++
>  include/uapi/linux/gpio.h |  4 ++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index d9074191edef..9da1093cc7f5 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -427,6 +427,8 @@ struct linehandle_state {
>  	(GPIOHANDLE_REQUEST_INPUT | \
>  	GPIOHANDLE_REQUEST_OUTPUT | \
>  	GPIOHANDLE_REQUEST_ACTIVE_LOW | \
> +	GPIOHANDLE_REQUEST_PULL_UP | \
> +	GPIOHANDLE_REQUEST_PULL_DOWN | \
>  	GPIOHANDLE_REQUEST_OPEN_DRAIN | \
>  	GPIOHANDLE_REQUEST_OPEN_SOURCE)
>  
> @@ -598,6 +600,10 @@ static int linehandle_create(struct gpio_device *gdev, void __user *ip)
>  			set_bit(FLAG_OPEN_DRAIN, &desc->flags);
>  		if (lflags & GPIOHANDLE_REQUEST_OPEN_SOURCE)
>  			set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> +		if (lflags & GPIOHANDLE_REQUEST_PULL_DOWN)
> +			set_bit(FLAG_PULL_DOWN, &desc->flags);
> +		if (lflags & GPIOHANDLE_REQUEST_PULL_UP)
> +			set_bit(FLAG_PULL_UP, &desc->flags);
>  
>  		ret = gpiod_set_transitory(desc, false);
>  		if (ret < 0)
> @@ -1102,6 +1108,10 @@ static long gpio_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  		if (test_bit(FLAG_OPEN_SOURCE, &desc->flags))
>  			lineinfo.flags |= (GPIOLINE_FLAG_OPEN_SOURCE |
>  					   GPIOLINE_FLAG_IS_OUT);
> +		if (test_bit(FLAG_PULL_DOWN, &desc->flags))
> +			lineinfo.flags |= GPIOLINE_FLAG_PULL_DOWN;
> +		if (test_bit(FLAG_PULL_UP, &desc->flags))
> +			lineinfo.flags |= GPIOLINE_FLAG_PULL_UP;
>  
>  		if (copy_to_user(ip, &lineinfo, sizeof(lineinfo)))
>  			return -EFAULT;
> @@ -2475,6 +2485,8 @@ static bool gpiod_free_commit(struct gpio_desc *desc)
>  		clear_bit(FLAG_REQUESTED, &desc->flags);
>  		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
>  		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
> +		clear_bit(FLAG_PULL_UP, &desc->flags);
> +		clear_bit(FLAG_PULL_DOWN, &desc->flags);
>  		clear_bit(FLAG_IS_HOGGED, &desc->flags);
>  		ret = true;
>  	}
> diff --git a/include/uapi/linux/gpio.h b/include/uapi/linux/gpio.h
> index 4ebfe0ac6c5b..c2d1f7d908d6 100644
> --- a/include/uapi/linux/gpio.h
> +++ b/include/uapi/linux/gpio.h
> @@ -33,6 +33,8 @@ struct gpiochip_info {
>  #define GPIOLINE_FLAG_ACTIVE_LOW	(1UL << 2)
>  #define GPIOLINE_FLAG_OPEN_DRAIN	(1UL << 3)
>  #define GPIOLINE_FLAG_OPEN_SOURCE	(1UL << 4)
> +#define GPIOLINE_FLAG_PULL_UP	(1UL << 5)
> +#define GPIOLINE_FLAG_PULL_DOWN	(1UL << 6)
>  
>  /**
>   * struct gpioline_info - Information about a certain GPIO line
> @@ -62,6 +64,8 @@ struct gpioline_info {
>  #define GPIOHANDLE_REQUEST_ACTIVE_LOW	(1UL << 2)
>  #define GPIOHANDLE_REQUEST_OPEN_DRAIN	(1UL << 3)
>  #define GPIOHANDLE_REQUEST_OPEN_SOURCE	(1UL << 4)
> +#define GPIOHANDLE_REQUEST_PULL_UP	(1UL << 5)
> +#define GPIOHANDLE_REQUEST_PULL_DOWN	(1UL << 6)
>  
>  /**
>   * struct gpiohandle_request - Information about a GPIO handle request
> -- 
> 2.20.1
> 
Sorry for the late feedback, but I'm still not sure whether this patch
is on track to be applied or not.  I had thought not, at least not yet,
but just in case...

You have added the flags to linehandle_create, but not lineevent_create.
Given that your primary use case is push buttons, isn't the event request
at least as important as the handle request?  Even ignoring your use
case, I'd add them to lineevent_create just to maintain symmetry.

Also, is it valid to set PULL_UP and PULL_DOWN simulaneously?
I would think not, but I could be wrong.
If not then that combination should be rejected with EINVAL.

Cheers,
Kent.