| Message ID | 20191224120709.18247-3-brgl@bgdev.pl |
|---|---|
| State | New |
| Headers | show |
| Series | gpiolib: add an ioctl() for monitoring line status changes | expand |
On Tue, Dec 24, 2019 at 1:07 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Instead of calling the gpiochip's set_config() callback directly and > checking its existence every time - just add a new routine that performs > this check internally. Call it in gpio_set_config() and > gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop > the check for chip->set() as it's irrelevant to this config option. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi Bartosz, On Tue, Dec 24, 2019 at 1:08 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Instead of calling the gpiochip's set_config() callback directly and > checking its existence every time - just add a new routine that performs > this check internally. Call it in gpio_set_config() and > gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop > the check for chip->set() as it's irrelevant to this config option. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -3042,6 +3042,15 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc); > * rely on gpio_request() having been called beforehand. > */ > > +static int gpio_do_set_config(struct gpio_chip *gc, unsigned int offset, > + enum pin_config_param mode) > +{ > + if (!gc->set_config) > + return -ENOTSUPP; > + > + return gc->set_config(gc, offset, mode); > +} > + > static int gpio_set_config(struct gpio_chip *gc, unsigned int offset, > enum pin_config_param mode) > { > @@ -3060,7 +3069,7 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned int offset, > } > > config = PIN_CONF_PACKED(mode, arg); > - return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP; > + return gpio_do_set_config(gc, offset, mode); These two lines are not equivalent: the new code no longer uses the packed value of mode and arg! Hence this leads to subsequent cleanups in commits e5e42ad224a040f9 ("gpiolib: remove set but not used variable 'config'") and d18fddff061d2796 ("gpiolib: Remove duplicated function gpio_do_set_config()"). However, what was the purpose of the PIN_CONF_PACKED() translation? Why is it no longer needed? Thanks! Gr{oetje,eeting}s, Geert
On Mon, Jan 20, 2020 at 09:44:43AM +0100, Geert Uytterhoeven wrote: > On Tue, Dec 24, 2019 at 1:08 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Instead of calling the gpiochip's set_config() callback directly and > > checking its existence every time - just add a new routine that performs > > this check internally. Call it in gpio_set_config() and > > gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop > > the check for chip->set() as it's irrelevant to this config option. ... > > config = PIN_CONF_PACKED(mode, arg); > > - return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP; > > + return gpio_do_set_config(gc, offset, mode); > > These two lines are not equivalent: the new code no longer uses the > packed value of mode and arg! Good catch! It's a regression (pin control drivers expects arg to be 1 in case it has been called thru GPIO framework to set "default" values in terms of certain driver) and below mentioned commits must be reverted. This one seems has a typo which must be fixed. > Hence this leads to subsequent cleanups in commits e5e42ad224a040f9 > ("gpiolib: remove set but not used variable 'config'") and d18fddff061d2796 > ("gpiolib: Remove duplicated function gpio_do_set_config()"). > However, what was the purpose of the PIN_CONF_PACKED() translation? > Why is it no longer needed?
pon., 20 sty 2020 o 09:44 Geert Uytterhoeven <geert@linux-m68k.org> napisał(a): > > Hi Bartosz, > > On Tue, Dec 24, 2019 at 1:08 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Instead of calling the gpiochip's set_config() callback directly and > > checking its existence every time - just add a new routine that performs > > this check internally. Call it in gpio_set_config() and > > gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop > > the check for chip->set() as it's irrelevant to this config option. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > [snip!] > > These two lines are not equivalent: the new code no longer uses the > packed value of mode and arg! > Hence this leads to subsequent cleanups in commits e5e42ad224a040f9 > ("gpiolib: remove set but not used variable 'config'") and d18fddff061d2796 > ("gpiolib: Remove duplicated function gpio_do_set_config()"). > > However, what was the purpose of the PIN_CONF_PACKED() translation? > Why is it no longer needed? > Thanks for catching this. I was OoO for a couple days. I'll try to get through the mail today and address this as well. Bartosz
On Tue, Dec 24, 2019 at 01:06:58PM +0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Instead of calling the gpiochip's set_config() callback directly and > checking its existence every time - just add a new routine that performs > this check internally. Call it in gpio_set_config() and > gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop > the check for chip->set() as it's irrelevant to this config option. > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> This patch made it into mainline, even though a regression was reported against it by Geert. Please note that it is not just a theoretic problem but _does_ indeed cause regressions. Guenter > --- > drivers/gpio/gpiolib.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c > index e5d101ee9ada..616e431039fc 100644 > --- a/drivers/gpio/gpiolib.c > +++ b/drivers/gpio/gpiolib.c > @@ -3042,6 +3042,15 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc); > * rely on gpio_request() having been called beforehand. > */ > > +static int gpio_do_set_config(struct gpio_chip *gc, unsigned int offset, > + enum pin_config_param mode) > +{ > + if (!gc->set_config) > + return -ENOTSUPP; > + > + return gc->set_config(gc, offset, mode); > +} > + > static int gpio_set_config(struct gpio_chip *gc, unsigned int offset, > enum pin_config_param mode) > { > @@ -3060,7 +3069,7 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned int offset, > } > > config = PIN_CONF_PACKED(mode, arg); > - return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP; > + return gpio_do_set_config(gc, offset, mode); > } > > static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc) > @@ -3294,15 +3303,9 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce) > > VALIDATE_DESC(desc); > chip = desc->gdev->chip; > - if (!chip->set || !chip->set_config) { > - gpiod_dbg(desc, > - "%s: missing set() or set_config() operations\n", > - __func__); > - return -ENOTSUPP; > - } > > config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce); > - return chip->set_config(chip, gpio_chip_hwgpio(desc), config); > + return gpio_do_set_config(chip, gpio_chip_hwgpio(desc), config); > } > EXPORT_SYMBOL_GPL(gpiod_set_debounce); > > @@ -3339,7 +3342,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory) > packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE, > !transitory); > gpio = gpio_chip_hwgpio(desc); > - rc = chip->set_config(chip, gpio, packed); > + rc = gpio_do_set_config(chip, gpio, packed); > if (rc == -ENOTSUPP) { > dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n", > gpio);
sob., 1 lut 2020 o 20:52 Guenter Roeck <linux@roeck-us.net> napisał(a): > > On Tue, Dec 24, 2019 at 01:06:58PM +0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > > > Instead of calling the gpiochip's set_config() callback directly and > > checking its existence every time - just add a new routine that performs > > this check internally. Call it in gpio_set_config() and > > gpiod_set_transitory(). Also call it in gpiod_set_debounce() and drop > > the check for chip->set() as it's irrelevant to this config option. > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > This patch made it into mainline, even though a regression was reported > against it by Geert. Please note that it is not just a theoretic problem > but _does_ indeed cause regressions. > > Guenter > Hi Guenter, I'm sorry for this, I was still largely unavailable for the past two weeks. I'll address it today, this time for real. Best regards, Bartosz Golaszewski
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index e5d101ee9ada..616e431039fc 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -3042,6 +3042,15 @@ EXPORT_SYMBOL_GPL(gpiochip_free_own_desc); * rely on gpio_request() having been called beforehand. */ +static int gpio_do_set_config(struct gpio_chip *gc, unsigned int offset, + enum pin_config_param mode) +{ + if (!gc->set_config) + return -ENOTSUPP; + + return gc->set_config(gc, offset, mode); +} + static int gpio_set_config(struct gpio_chip *gc, unsigned int offset, enum pin_config_param mode) { @@ -3060,7 +3069,7 @@ static int gpio_set_config(struct gpio_chip *gc, unsigned int offset, } config = PIN_CONF_PACKED(mode, arg); - return gc->set_config ? gc->set_config(gc, offset, config) : -ENOTSUPP; + return gpio_do_set_config(gc, offset, mode); } static int gpio_set_bias(struct gpio_chip *chip, struct gpio_desc *desc) @@ -3294,15 +3303,9 @@ int gpiod_set_debounce(struct gpio_desc *desc, unsigned debounce) VALIDATE_DESC(desc); chip = desc->gdev->chip; - if (!chip->set || !chip->set_config) { - gpiod_dbg(desc, - "%s: missing set() or set_config() operations\n", - __func__); - return -ENOTSUPP; - } config = pinconf_to_config_packed(PIN_CONFIG_INPUT_DEBOUNCE, debounce); - return chip->set_config(chip, gpio_chip_hwgpio(desc), config); + return gpio_do_set_config(chip, gpio_chip_hwgpio(desc), config); } EXPORT_SYMBOL_GPL(gpiod_set_debounce); @@ -3339,7 +3342,7 @@ int gpiod_set_transitory(struct gpio_desc *desc, bool transitory) packed = pinconf_to_config_packed(PIN_CONFIG_PERSIST_STATE, !transitory); gpio = gpio_chip_hwgpio(desc); - rc = chip->set_config(chip, gpio, packed); + rc = gpio_do_set_config(chip, gpio, packed); if (rc == -ENOTSUPP) { dev_dbg(&desc->gdev->dev, "Persistence not supported for GPIO %d\n", gpio);