[v2,5/8] gpiolib: shrink further

From: Arnd Bergmann <arnd@arndb.de>

gpio_set_debounce() only has a single user, which is trivially
converted to gpiod_set_debounce(), while gpio_cansleep() and
devm_gpio_free() have no users at all.

Remove them all to shrink the old gpio interface.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 .../driver-api/driver-model/devres.rst        |  1 -
 Documentation/driver-api/gpio/legacy.rst      |  2 --
 drivers/gpio/gpiolib-devres.c                 | 25 ----------------
 drivers/input/touchscreen/ads7846.c           |  3 +-
 include/linux/gpio.h                          | 29 -------------------
 5 files changed, 2 insertions(+), 58 deletions(-)

On Tue, Nov 09, 2021 at 11:02:04AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> gpio_set_debounce() only has a single user, which is trivially
> converted to gpiod_set_debounce(), while gpio_cansleep() and
> devm_gpio_free() have no users at all.
> 
> Remove them all to shrink the old gpio interface.

A couple of nit-picks below.
In either case,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  .../driver-api/driver-model/devres.rst        |  1 -
>  Documentation/driver-api/gpio/legacy.rst      |  2 --
>  drivers/gpio/gpiolib-devres.c                 | 25 ----------------
>  drivers/input/touchscreen/ads7846.c           |  3 +-
>  include/linux/gpio.h                          | 29 -------------------
>  5 files changed, 2 insertions(+), 58 deletions(-)
> 
> diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> index 148e19381b79..52821478decd 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -277,7 +277,6 @@ GPIO
>    devm_gpiochip_add_data()
>    devm_gpio_request()
>    devm_gpio_request_one()
> -  devm_gpio_free()
>  
>  I2C
>    devm_i2c_new_dummy_device()
> diff --git a/Documentation/driver-api/gpio/legacy.rst b/Documentation/driver-api/gpio/legacy.rst
> index 06c05e2d62c1..eae185f771d7 100644
> --- a/Documentation/driver-api/gpio/legacy.rst
> +++ b/Documentation/driver-api/gpio/legacy.rst
> @@ -238,8 +238,6 @@ setup or driver probe/teardown code, so this is an easy constraint.)::
>          ## 	gpio_free_array()
>  
>                  gpio_free()
> -                gpio_set_debounce()
> -
>  

One more blank line removal?

>  
>  Claiming and Releasing GPIOs
> diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
> index 79da85d17b71..55465ead492f 100644
> --- a/drivers/gpio/gpiolib-devres.c
> +++ b/drivers/gpio/gpiolib-devres.c
> @@ -385,13 +385,6 @@ static void devm_gpio_release(struct device *dev, void *res)
>  	gpio_free(*gpio);
>  }
>  
> -static int devm_gpio_match(struct device *dev, void *res, void *data)
> -{
> -	unsigned *this = res, *gpio = data;
> -
> -	return *this == *gpio;
> -}
> -
>  /**
>   *      devm_gpio_request - request a GPIO for a managed device
>   *      @dev: device to request the GPIO for
> @@ -459,24 +452,6 @@ int devm_gpio_request_one(struct device *dev, unsigned gpio,
>  }
>  EXPORT_SYMBOL_GPL(devm_gpio_request_one);
>  
> -/**
> - *      devm_gpio_free - free a GPIO
> - *      @dev: device to free GPIO for
> - *      @gpio: GPIO to free
> - *
> - *      Except for the extra @dev argument, this function takes the
> - *      same arguments and performs the same function as gpio_free().
> - *      This function instead of gpio_free() should be used to manually
> - *      free GPIOs allocated with devm_gpio_request().
> - */
> -void devm_gpio_free(struct device *dev, unsigned int gpio)
> -{
> -
> -	WARN_ON(devres_release(dev, devm_gpio_release, devm_gpio_match,
> -		&gpio));
> -}
> -EXPORT_SYMBOL_GPL(devm_gpio_free);
> -
>  static void devm_gpio_chip_release(void *data)
>  {
>  	struct gpio_chip *gc = data;
> diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> index a25a77dd9a32..d0664e3b89bb 100644
> --- a/drivers/input/touchscreen/ads7846.c
> +++ b/drivers/input/touchscreen/ads7846.c
> @@ -27,6 +27,7 @@
>  #include <linux/of.h>

>  #include <linux/of_gpio.h>

(1)

>  #include <linux/of_device.h>

> +#include <linux/gpio/consumer.h>

(2)

>  #include <linux/gpio.h>

(3)

Seems too many...

Are you planning to clean up this driver to get rid of (1) and (3) altogether?

(I understand that for current purposes above is a good quick cleanup)

>  #include <linux/spi/spi.h>
>  #include <linux/spi/ads7846.h>
> @@ -1018,7 +1019,7 @@ static int ads7846_setup_pendown(struct spi_device *spi,
>  		ts->gpio_pendown = pdata->gpio_pendown;
>  
>  		if (pdata->gpio_pendown_debounce)
> -			gpio_set_debounce(pdata->gpio_pendown,
> +			gpiod_set_debounce(gpio_to_desc(pdata->gpio_pendown),
>  					  pdata->gpio_pendown_debounce);
>  	} else {
>  		dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
> diff --git a/include/linux/gpio.h b/include/linux/gpio.h
> index c19256f67e02..64cc8f09eba8 100644
> --- a/include/linux/gpio.h
> +++ b/include/linux/gpio.h
> @@ -117,11 +117,6 @@ static inline int gpio_direction_output(unsigned gpio, int value)
>  	return gpiod_direction_output_raw(gpio_to_desc(gpio), value);
>  }
>  
> -static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
> -{
> -	return gpiod_set_debounce(gpio_to_desc(gpio), debounce);
> -}
> -
>  static inline int gpio_get_value_cansleep(unsigned gpio)
>  {
>  	return gpiod_get_raw_value_cansleep(gpio_to_desc(gpio));
> @@ -140,11 +135,6 @@ static inline void gpio_set_value(unsigned gpio, int value)
>  	return gpiod_set_raw_value(gpio_to_desc(gpio), value);
>  }
>  
> -static inline int gpio_cansleep(unsigned gpio)
> -{
> -	return gpiod_cansleep(gpio_to_desc(gpio));
> -}
> -
>  static inline int gpio_to_irq(unsigned gpio)
>  {
>  	return gpiod_to_irq(gpio_to_desc(gpio));
> @@ -181,8 +171,6 @@ struct device;
>  int devm_gpio_request(struct device *dev, unsigned gpio, const char *label);
>  int devm_gpio_request_one(struct device *dev, unsigned gpio,
>  			  unsigned long flags, const char *label);
> -void devm_gpio_free(struct device *dev, unsigned int gpio);
> -
>  #else /* ! CONFIG_GPIOLIB */
>  
>  #include <linux/kernel.h>
> @@ -239,11 +227,6 @@ static inline int gpio_direction_output(unsigned gpio, int value)
>  	return -ENOSYS;
>  }
>  
> -static inline int gpio_set_debounce(unsigned gpio, unsigned debounce)
> -{
> -	return -ENOSYS;
> -}
> -
>  static inline int gpio_get_value(unsigned gpio)
>  {
>  	/* GPIO can never have been requested or set as {in,out}put */
> @@ -257,13 +240,6 @@ static inline void gpio_set_value(unsigned gpio, int value)
>  	WARN_ON(1);
>  }
>  
> -static inline int gpio_cansleep(unsigned gpio)
> -{
> -	/* GPIO can never have been requested or set as {in,out}put */
> -	WARN_ON(1);
> -	return 0;
> -}
> -
>  static inline int gpio_get_value_cansleep(unsigned gpio)
>  {
>  	/* GPIO can never have been requested or set as {in,out}put */
> @@ -319,11 +295,6 @@ static inline int devm_gpio_request_one(struct device *dev, unsigned gpio,
>  	return -EINVAL;
>  }
>  
> -static inline void devm_gpio_free(struct device *dev, unsigned int gpio)
> -{
> -	WARN_ON(1);
> -}
> -
>  #endif /* ! CONFIG_GPIOLIB */
>  
>  #endif /* __LINUX_GPIO_H */
> -- 
> 2.29.2
>

On Tue, Nov 9, 2021 at 11:24 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> > @@ -238,8 +238,6 @@ setup or driver probe/teardown code, so this is an easy constraint.)::
> >          ##   gpio_free_array()
> >
> >                  gpio_free()
> > -                gpio_set_debounce()
> > -
> >
>
> One more blank line removal?

I think two blank lines at the end of a section is the normal style
for this file.
Do you mean I should remove another line, or not remove the third blank
line here?

> > diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
> > index a25a77dd9a32..d0664e3b89bb 100644
> > --- a/drivers/input/touchscreen/ads7846.c
> > +++ b/drivers/input/touchscreen/ads7846.c
> > @@ -27,6 +27,7 @@
> >  #include <linux/of.h>
>
> >  #include <linux/of_gpio.h>
>
> (1)
>
> >  #include <linux/of_device.h>
>
> > +#include <linux/gpio/consumer.h>
>
> (2)
>
> >  #include <linux/gpio.h>
>
> (3)
>
> Seems too many...
>
> Are you planning to clean up this driver to get rid of (1) and (3) altogether?
>
> (I understand that for current purposes above is a good quick cleanup)

Ideally we should only use linux/gpio/consumer.h, which is required for
gpiod_set_debounce(). of_gpio.h is still needed for of_get_named_gpio()
and should be taken out once we change this to gpiod_get(), while
linux/gpio.h is still needed for gpio_is_valid()/gpio_get_value() and should
be removed when those are changed to the gpiod_ versions.

We could do an intermediate patch that converts one half of the
interface, something like

diff --git a/drivers/input/touchscreen/ads7846.c
b/drivers/input/touchscreen/ads7846.c
index d0664e3b89bb..60e6b292ccdf 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -140,7 +140,7 @@ struct ads7846 {
        int                     (*filter)(void *data, int data_idx, int *val);
        void                    *filter_data;
        int                     (*get_pendown_state)(void);
-       int                     gpio_pendown;
+       struct gpio_desc        *gpio_pendown;

        void                    (*wait_for_sync)(void);
 };
@@ -223,7 +223,7 @@ static int get_pendown_state(struct ads7846 *ts)
        if (ts->get_pendown_state)
                return ts->get_pendown_state();

-       return !gpio_get_value(ts->gpio_pendown);
+       return !gpiod_get_value(ts->gpio_pendown);
 }

 static void ads7846_report_pen_up(struct ads7846 *ts)
@@ -1005,6 +1005,11 @@ static int ads7846_setup_pendown(struct spi_device *spi,

        if (pdata->get_pendown_state) {
                ts->get_pendown_state = pdata->get_pendown_state;
+       } else if (ts->gpio_pendown) {
+               if (IS_ERR(ts->gpio_pendown)) {
+                       dev_err(&spi->dev, "missing pendown gpio\n");
+                       return PTR_ERR(ts->gpio_pendown);
+               }
        } else if (gpio_is_valid(pdata->gpio_pendown)) {

                err = devm_gpio_request_one(&spi->dev, pdata->gpio_pendown,
@@ -1016,10 +1016,10 @@ static int ads7846_setup_pendown(struct spi_device *spi,
                        return err;
                }

-               ts->gpio_pendown = pdata->gpio_pendown;
+               ts->gpio_pendown = gpio_to_desc(pdata->gpio_pendown);

                if (pdata->gpio_pendown_debounce)
-                       gpiod_set_debounce(gpio_to_desc(pdata->gpio_pendown),
+                       gpiod_set_debounce(ts->gpio_pendown,
                                          pdata->gpio_pendown_debounce);
        } else {
                dev_err(&spi->dev, "no get_pendown_state nor gpio_pendown?\n");
@@ -1128,7 +1128,7 @@ static const struct of_device_id ads7846_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, ads7846_dt_ids);

-static const struct ads7846_platform_data *ads7846_probe_dt(struct device *dev)
+static const struct ads7846_platform_data *ads7846_probe_dt(struct
ads7846 *ts, struct device *dev)
 {
        struct ads7846_platform_data *pdata;
        struct device_node *node = dev->of_node;
@@ -1193,7 +1193,7 @@ static const struct ads7846_platform_data
*ads7846_probe_dt(struct device *dev)
        pdata->wakeup = of_property_read_bool(node, "wakeup-source") ||
                        of_property_read_bool(node, "linux,wakeup");

-       pdata->gpio_pendown = of_get_named_gpio(dev->of_node,
"pendown-gpio", 0);
+       ts->gpio_pendown = gpiod_get(dev, "pendown-gpio", GPIOD_IN);

        return pdata;
 }
@@ -1267,7 +1267,7 @@ static int ads7846_probe(struct spi_device *spi)

        pdata = dev_get_platdata(dev);
        if (!pdata) {
-               pdata = ads7846_probe_dt(dev);
+               pdata = ads7846_probe_dt(ts, dev);
                if (IS_ERR(pdata))
                        return PTR_ERR(pdata);
        }

while leaving the platform side untouched, but I think Linus' plan was to
remove the gpio settings from all platform data and instead use the gpio
lookup in the board files.

          Arnd

On Tue, Nov 9, 2021 at 12:18 PM Arnd Bergmann <arnd@kernel.org> wrote:

> Ideally we should only use linux/gpio/consumer.h, which is required for
> gpiod_set_debounce(). of_gpio.h is still needed for of_get_named_gpio()
> and should be taken out once we change this to gpiod_get(), while
> linux/gpio.h is still needed for gpio_is_valid()/gpio_get_value() and should
> be removed when those are changed to the gpiod_ versions.
>
> We could do an intermediate patch that converts one half of the
> interface, something like

When I convert stuff I try to go all the way when I can. It can
be a bit daring if no one is there to test changes.

The patch looks good though apart from:

> -               ts->gpio_pendown = pdata->gpio_pendown;
> +               ts->gpio_pendown = gpio_to_desc(pdata->gpio_pendown);

I usually even go into the defined platform data and try to convert
the boardfile to use a descriptor table so this is never needed.
(But, more work.)

Examples:
git log -p --author=Walleij arch/arm/mach-pxa/

> -       pdata->gpio_pendown = of_get_named_gpio(dev->of_node,
> "pendown-gpio", 0);
> +       ts->gpio_pendown = gpiod_get(dev, "pendown-gpio", GPIOD_IN);

Needs to be just gpiod_get(dev, "pendown", GPIOD_IN); the new
API tries the "-gpio[s]" suffixes when going into the device tree.

Yours,
Linus Walleij

On Tue, Nov 9, 2021 at 11:17 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Nov 9, 2021 at 12:18 PM Arnd Bergmann <arnd@kernel.org> wrote:
>
> > Ideally we should only use linux/gpio/consumer.h, which is required for
> > gpiod_set_debounce(). of_gpio.h is still needed for of_get_named_gpio()
> > and should be taken out once we change this to gpiod_get(), while
> > linux/gpio.h is still needed for gpio_is_valid()/gpio_get_value() and should
> > be removed when those are changed to the gpiod_ versions.
> >
> > We could do an intermediate patch that converts one half of the
> > interface, something like
>
> When I convert stuff I try to go all the way when I can. It can
> be a bit daring if no one is there to test changes.
>
> The patch looks good though apart from:
>
> > -               ts->gpio_pendown = pdata->gpio_pendown;
> > +               ts->gpio_pendown = gpio_to_desc(pdata->gpio_pendown);
>
> I usually even go into the defined platform data and try to convert
> the boardfile to use a descriptor table so this is never needed.
> (But, more work.)

Yes, I noticed. I had done some conversions for pxa this way, I should
look in my tree if I should resend those.

My hope would be that by making the steps smaller, it's easier to find
people that are willing and able to help out. From looking at it so far,
I would partition the problem something like:

a) Remove the (now) trivial wrappers around gpiod_*() functions
by using open-coded gpio_to_desc() calls everywhere. This doesn't
improve the code, but it can be trivially scripted and it should help
by making it less practical to put new users in.

b) one driver/subsystem at a time, replace all calls to
{devm_,}gpio_{free,request{,_one}} with a new
struct gpio_desc *gpiod_get_legacy(struct device *dev, int gpio, enum
gpiod_flags flags);
This takes the conversion only half-way, but is much more manageable
for a random contributor or reviewer, and it undoes the ugly bits
added in step a), making it a clear improvement.

c) convert the boardfile/platform_data/of_get_named_gpio side along with
the corresponding s/gpiod_get_legacy/gpiod_get/, which is now a fairly simple
change on the driver side, while the platform side can be reviewed by
the platform
maintainers.

> Examples:
> git log -p --author=Walleij arch/arm/mach-pxa/
>
> > -       pdata->gpio_pendown = of_get_named_gpio(dev->of_node,
> > "pendown-gpio", 0);
> > +       ts->gpio_pendown = gpiod_get(dev, "pendown-gpio", GPIOD_IN);
>
> Needs to be just gpiod_get(dev, "pendown", GPIOD_IN); the new
> API tries the "-gpio[s]" suffixes when going into the device tree.

Ok, got it.

        Arnd