From: Arnd Bergmann <arnd@kernel.org> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, Arnd Bergmann <arnd@arndb.de>, Linus Walleij <linus.walleij@linaro.org>, Bartosz Golaszewski <brgl@bgdev.pl>, Geert Uytterhoeven <geert+renesas@glider.be>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2 5/8] gpiolib: shrink further Date: Tue, 9 Nov 2021 12:18:17 +0100 [thread overview] Message-ID: <CAK8P3a0VsDG3af1YkRRb=5bmvZ4zP3Du492hE_jyUWOwnYph_w@mail.gmail.com> (raw) In-Reply-To: <YYpMcKlcZ3JWqp5M@smile.fi.intel.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@kernel.org> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: "open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>, Arnd Bergmann <arnd@arndb.de>, Linus Walleij <linus.walleij@linaro.org>, Bartosz Golaszewski <brgl@bgdev.pl>, Geert Uytterhoeven <geert+renesas@glider.be>, Linux ARM <linux-arm-kernel@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH v2 5/8] gpiolib: shrink further Date: Tue, 9 Nov 2021 12:18:17 +0100 [thread overview] Message-ID: <CAK8P3a0VsDG3af1YkRRb=5bmvZ4zP3Du492hE_jyUWOwnYph_w@mail.gmail.com> (raw) In-Reply-To: <YYpMcKlcZ3JWqp5M@smile.fi.intel.com> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-11-09 11:18 UTC|newest] Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-09 10:01 [PATCH v2 0/8] gpiolib header cleanup Arnd Bergmann 2021-11-09 10:01 ` Arnd Bergmann 2021-11-09 10:02 ` [PATCH v2 1/8] gpiolib: remove irq_to_gpio() definition Arnd Bergmann 2021-11-09 10:02 ` Arnd Bergmann 2021-11-09 10:02 ` [PATCH v2 2/8] gpiolib: remove empty asm/gpio.h files Arnd Bergmann 2021-11-09 10:02 ` Arnd Bergmann 2021-11-09 10:02 ` [PATCH v2 3/8] gpiolib: coldfire: remove custom asm/gpio.h Arnd Bergmann 2021-11-09 10:02 ` Arnd Bergmann 2021-11-09 10:02 ` [PATCH v2 4/8] gpiolib: remove asm-generic/gpio.h Arnd Bergmann 2021-11-09 10:02 ` Arnd Bergmann 2021-11-09 10:19 ` Andy Shevchenko 2021-11-09 10:19 ` Andy Shevchenko 2021-11-09 10:02 ` [PATCH v2 5/8] gpiolib: shrink further Arnd Bergmann 2021-11-09 10:02 ` Arnd Bergmann 2021-11-09 10:24 ` Andy Shevchenko 2021-11-09 10:24 ` Andy Shevchenko 2021-11-09 11:18 ` Arnd Bergmann [this message] 2021-11-09 11:18 ` Arnd Bergmann 2021-11-09 22:17 ` Linus Walleij 2021-11-09 22:17 ` Linus Walleij 2021-11-10 12:39 ` Arnd Bergmann 2021-11-10 12:39 ` Arnd Bergmann 2021-11-09 10:02 ` [PATCH v2 6/8] gpiolib: remove legacy gpio_export Arnd Bergmann 2021-11-09 10:02 ` Arnd Bergmann 2021-11-09 10:30 ` Andy Shevchenko 2021-11-09 10:30 ` Andy Shevchenko 2021-11-09 10:50 ` Arnd Bergmann 2021-11-09 10:50 ` Arnd Bergmann 2021-11-09 20:42 ` Linus Walleij 2021-11-09 20:42 ` Linus Walleij 2021-11-09 22:46 ` Arnd Bergmann 2021-11-09 22:46 ` Arnd Bergmann 2021-11-10 0:03 ` Linus Walleij 2021-11-10 0:03 ` Linus Walleij 2021-11-09 20:33 ` Linus Walleij 2021-11-09 20:33 ` Linus Walleij 2021-11-09 10:02 ` [PATCH v2 7/8] gpiolib: remove gpio_to_chip Arnd Bergmann 2021-11-09 10:02 ` Arnd Bergmann 2021-11-09 10:32 ` Andy Shevchenko 2021-11-09 10:32 ` Andy Shevchenko 2021-11-09 10:54 ` Arnd Bergmann 2021-11-09 10:54 ` Arnd Bergmann 2021-11-09 10:02 ` [PATCH v2 8/8] gpiolib: split linux/gpio/driver.h out of linux/gpio.h Arnd Bergmann 2021-11-09 10:02 ` Arnd Bergmann 2021-11-09 10:34 ` Andy Shevchenko 2021-11-09 10:34 ` Andy Shevchenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAK8P3a0VsDG3af1YkRRb=5bmvZ4zP3Du492hE_jyUWOwnYph_w@mail.gmail.com' \ --to=arnd@kernel.org \ --cc=andriy.shevchenko@linux.intel.com \ --cc=arnd@arndb.de \ --cc=brgl@bgdev.pl \ --cc=geert+renesas@glider.be \ --cc=linus.walleij@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-gpio@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.