| Message ID | 20230113171051.19309-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/2] pinctrl: bcm: bcm2835: Switch to use ->add_pin_ranges() | expand |
Hi Andy, Am 13.01.23 um 18:10 schrieb Andy Shevchenko: > Yeah, while the ->add_pin_ranges() shouldn't be used by DT drivers, > this one requires it to support quite old firmware descriptions that > do not have gpio-ranges property. > > The change allows to clean up GPIO library from OF specifics. > There is no functional change intended. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > v2: fixed compilation issues (LKP), Cc'ed to the author of original code > > Btw, the commit d2b67744fd99 ("pinctrl: bcm2835: implement hook for > missing gpio-ranges") seems problematic in the fist place due to > odd of_node_put() call. I dunno how that part had been tested, or > how it's supposed to work, i.e. where is the counterpart of_node_get(). > Anyway this change drops it for good. The countpart is in of_pinctrl_get(). I was just following the pattern like in other drivers like gpio-rockchip. The original commit has been tested by Florian Fainelli and me. I'm not sure if it's safe to drop it completely. Btw this is not the only platform affected by the gpio-ranges compatibility issue [1]. > Perhaps we can check gpio-ranges property presence inside the GPIO > library, so this ->add_pin_ranges() won't be called at all. I thought this could be very platform specific, so i implemented a hook. But yes my initial hack modified gpiolib-of [2]. [1] - https://patchwork.kernel.org/project/linux-arm-msm/patch/20180412190138.12372-1-chunkeey@gmail.com/ [2] - https://lore.kernel.org/linux-arm-kernel/75266ed1-666a-138b-80f1-ae9a06b7bdf3@i2se.com/ > Also I would like to understand the dance around checking for pin > control device. The original commit lacks of comments in the non-trivial > code. > > drivers/pinctrl/bcm/pinctrl-bcm2835.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > index 7857e612a100..29f278c49103 100644 > --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c > +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c > @@ -358,16 +358,17 @@ static int bcm2835_gpio_direction_output(struct gpio_chip *chip, > return 0; > } > > -static int bcm2835_of_gpio_ranges_fallback(struct gpio_chip *gc, > - struct device_node *np) > +static int bcm2835_add_pin_ranges_fallback(struct gpio_chip *gc) > { > + struct device_node *np = dev_of_node(gc->parent); > struct pinctrl_dev *pctldev = of_pinctrl_get(np); > > - of_node_put(np); > - > if (!pctldev) > return 0; > > + if (of_property_read_bool(np, "gpio-ranges")) > + return 0; > + > gpiochip_add_pin_range(gc, pinctrl_dev_get_devname(pctldev), 0, 0, > gc->ngpio); > > @@ -388,7 +389,7 @@ static const struct gpio_chip bcm2835_gpio_chip = { > .base = -1, > .ngpio = BCM2835_NUM_GPIOS, > .can_sleep = false, > - .of_gpio_ranges_fallback = bcm2835_of_gpio_ranges_fallback, > + .add_pin_ranges = bcm2835_add_pin_ranges_fallback, > }; > > static const struct gpio_chip bcm2711_gpio_chip = { > @@ -405,7 +406,7 @@ static const struct gpio_chip bcm2711_gpio_chip = { > .base = -1, > .ngpio = BCM2711_NUM_GPIOS, > .can_sleep = false, > - .of_gpio_ranges_fallback = bcm2835_of_gpio_ranges_fallback, > + .add_pin_ranges = bcm2835_add_pin_ranges_fallback, > }; > > static void bcm2835_gpio_irq_handle_bank(struct bcm2835_pinctrl *pc,
On Fri, Jan 13, 2023 at 09:13:23PM +0100, Stefan Wahren wrote: > Am 13.01.23 um 18:10 schrieb Andy Shevchenko: ... > > v2: fixed compilation issues (LKP), Cc'ed to the author of original code > > > > Btw, the commit d2b67744fd99 ("pinctrl: bcm2835: implement hook for > > missing gpio-ranges") seems problematic in the fist place due to > > odd of_node_put() call. I dunno how that part had been tested, or > > how it's supposed to work, i.e. where is the counterpart of_node_get(). > > Anyway this change drops it for good. > > The countpart is in of_pinctrl_get(). I was just following the pattern like > in other drivers like gpio-rockchip. The original commit has been tested by > Florian Fainelli and me. I'm not sure if it's safe to drop it completely. Please, elaborate how of_pinctrl_get() increases refcount of the parameter. Maybe I'm looking into a wrong place? > Btw this is not the only platform affected by the gpio-ranges compatibility > issue [1]. This is the only one that uses unnecessary added callback. > > Perhaps we can check gpio-ranges property presence inside the GPIO > > library, so this ->add_pin_ranges() won't be called at all. > > I thought this could be very platform specific, so i implemented a hook. But > yes my initial hack modified gpiolib-of [2]. The point is that possibly documentation of ->add_pin_ranges() should be amended to take care of the cases like this. We don't need two or more hooks to do the same, esp. taking into account that OF specific doesn't cover other cases. > [1] - https://patchwork.kernel.org/project/linux-arm-msm/patch/20180412190138.12372-1-chunkeey@gmail.com/ > > [2] - https://lore.kernel.org/linux-arm-kernel/75266ed1-666a-138b-80f1-ae9a06b7bdf3@i2se.com/ > > > Also I would like to understand the dance around checking for pin > > control device. The original commit lacks of comments in the non-trivial > > code. Any comment on this?
Hi Andy, Am 13.01.23 um 22:31 schrieb Andy Shevchenko: > On Fri, Jan 13, 2023 at 09:13:23PM +0100, Stefan Wahren wrote: >> Am 13.01.23 um 18:10 schrieb Andy Shevchenko: > ... > >>> v2: fixed compilation issues (LKP), Cc'ed to the author of original code >>> >>> Btw, the commit d2b67744fd99 ("pinctrl: bcm2835: implement hook for >>> missing gpio-ranges") seems problematic in the fist place due to >>> odd of_node_put() call. I dunno how that part had been tested, or >>> how it's supposed to work, i.e. where is the counterpart of_node_get(). >>> Anyway this change drops it for good. >> The countpart is in of_pinctrl_get(). I was just following the pattern like >> in other drivers like gpio-rockchip. The original commit has been tested by >> Florian Fainelli and me. I'm not sure if it's safe to drop it completely. > Please, elaborate how of_pinctrl_get() increases refcount of the parameter. > Maybe I'm looking into a wrong place? > >> Btw this is not the only platform affected by the gpio-ranges compatibility >> issue [1]. > This is the only one that uses unnecessary added callback. i didn't have access to any of the other platforms which were also affected. So i thought providing a general solution would be good idea. I wasn't aware of add_pin_ranges(). Since i was annoyed that nobody cared about DTB backward compatibility, i send out a RFC series to fix the GPIO hog regression which breaks the LEDs on the Raspberry Pi. Nobody complained about it. > >>> Perhaps we can check gpio-ranges property presence inside the GPIO >>> library, so this ->add_pin_ranges() won't be called at all. >> I thought this could be very platform specific, so i implemented a hook. But >> yes my initial hack modified gpiolib-of [2]. > The point is that possibly documentation of ->add_pin_ranges() should be > amended to take care of the cases like this. We don't need two or more > hooks to do the same, esp. taking into account that OF specific doesn't > cover other cases. > >> [1] - https://patchwork.kernel.org/project/linux-arm-msm/patch/20180412190138.12372-1-chunkeey@gmail.com/ >> >> [2] - https://lore.kernel.org/linux-arm-kernel/75266ed1-666a-138b-80f1-ae9a06b7bdf3@i2se.com/ >> >>> Also I would like to understand the dance around checking for pin >>> control device. The original commit lacks of comments in the non-trivial >>> code. > Any comment on this? Do you mean the NULL check? of_pinctrl_get() could return NULL, but pinctrl_dev_get_devname() directly access the dev member.
On Sat, Jan 14, 2023 at 12:23:49PM +0100, Stefan Wahren wrote: > Am 13.01.23 um 22:31 schrieb Andy Shevchenko: > > On Fri, Jan 13, 2023 at 09:13:23PM +0100, Stefan Wahren wrote: > > > Am 13.01.23 um 18:10 schrieb Andy Shevchenko: ... > > > > Also I would like to understand the dance around checking for pin > > > > control device. The original commit lacks of comments in the non-trivial > > > > code. > > Any comment on this? > Do you mean the NULL check? of_pinctrl_get() could return NULL, but > pinctrl_dev_get_devname() directly access the dev member. The of_pinctrl_get() call. Why do we need that? AFAIU the code is called after we call devm_pinctrl_register() and hence it can not be NULL. Am I mistaken? P.S. This is out of the scope of the series anyway, I left that untouched. So it is just for me to understand better the code flow.
diff --git a/drivers/pinctrl/bcm/pinctrl-bcm2835.c b/drivers/pinctrl/bcm/pinctrl-bcm2835.c index 7857e612a100..29f278c49103 100644 --- a/drivers/pinctrl/bcm/pinctrl-bcm2835.c +++ b/drivers/pinctrl/bcm/pinctrl-bcm2835.c @@ -358,16 +358,17 @@ static int bcm2835_gpio_direction_output(struct gpio_chip *chip, return 0; } -static int bcm2835_of_gpio_ranges_fallback(struct gpio_chip *gc, - struct device_node *np) +static int bcm2835_add_pin_ranges_fallback(struct gpio_chip *gc) { + struct device_node *np = dev_of_node(gc->parent); struct pinctrl_dev *pctldev = of_pinctrl_get(np); - of_node_put(np); - if (!pctldev) return 0; + if (of_property_read_bool(np, "gpio-ranges")) + return 0; + gpiochip_add_pin_range(gc, pinctrl_dev_get_devname(pctldev), 0, 0, gc->ngpio); @@ -388,7 +389,7 @@ static const struct gpio_chip bcm2835_gpio_chip = { .base = -1, .ngpio = BCM2835_NUM_GPIOS, .can_sleep = false, - .of_gpio_ranges_fallback = bcm2835_of_gpio_ranges_fallback, + .add_pin_ranges = bcm2835_add_pin_ranges_fallback, }; static const struct gpio_chip bcm2711_gpio_chip = { @@ -405,7 +406,7 @@ static const struct gpio_chip bcm2711_gpio_chip = { .base = -1, .ngpio = BCM2711_NUM_GPIOS, .can_sleep = false, - .of_gpio_ranges_fallback = bcm2835_of_gpio_ranges_fallback, + .add_pin_ranges = bcm2835_add_pin_ranges_fallback, }; static void bcm2835_gpio_irq_handle_bank(struct bcm2835_pinctrl *pc,
Yeah, while the ->add_pin_ranges() shouldn't be used by DT drivers, this one requires it to support quite old firmware descriptions that do not have gpio-ranges property. The change allows to clean up GPIO library from OF specifics. There is no functional change intended. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- v2: fixed compilation issues (LKP), Cc'ed to the author of original code Btw, the commit d2b67744fd99 ("pinctrl: bcm2835: implement hook for missing gpio-ranges") seems problematic in the fist place due to odd of_node_put() call. I dunno how that part had been tested, or how it's supposed to work, i.e. where is the counterpart of_node_get(). Anyway this change drops it for good. Perhaps we can check gpio-ranges property presence inside the GPIO library, so this ->add_pin_ranges() won't be called at all. Also I would like to understand the dance around checking for pin control device. The original commit lacks of comments in the non-trivial code. drivers/pinctrl/bcm/pinctrl-bcm2835.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)