Re: [PATCH v2 5/8] gpiolib: shrink further

[Arnd Bergmann <arnd@xxxxxxxxxx>]

On Tue, Nov 9, 2021 at 11:24 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxxxxxxxx> 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