[PATCH v2 1/2] pinctrl: bcm: bcm2835: Switch to use ->add_pin_ranges()

[PATCH v2 1/2] pinctrl: bcm: bcm2835: Switch to use ->add_pin_ranges()

[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.

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(-)

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,
-- 
2.39.0

[PATCH v2 2/2] Revert "gpiolib: of: Introduce hook for missing gpio-ranges"

[Andy Shevchenko]

This reverts commit 3550bba25d5587a701e6edf20e20984d2ee72c78.

No users for this one, revert it for good.
The ->add_pin_ranges() can be used instead.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v2: made it as a pure revert
 drivers/gpio/gpiolib-of.c   |  5 -----
 include/linux/gpio/driver.h | 12 ------------
 2 files changed, 17 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 72d8a3da31e3..266352b1a966 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -980,11 +980,6 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
 	if (!np)
 		return 0;
 
-	if (!of_property_read_bool(np, "gpio-ranges") &&
-	    chip->of_gpio_ranges_fallback) {
-		return chip->of_gpio_ranges_fallback(chip, np);
-	}
-
 	group_names = of_find_property(np, group_names_propname, NULL);
 
 	for (;; index++) {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index ddc7a14a274f..5ab50ba3c309 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -521,18 +521,6 @@ struct gpio_chip {
 	 */
 	int (*of_xlate)(struct gpio_chip *gc,
 			const struct of_phandle_args *gpiospec, u32 *flags);
-
-	/**
-	 * @of_gpio_ranges_fallback:
-	 *
-	 * Optional hook for the case that no gpio-ranges property is defined
-	 * within the device tree node "np" (usually DT before introduction
-	 * of gpio-ranges). So this callback is helpful to provide the
-	 * necessary backward compatibility for the pin ranges.
-	 */
-	int (*of_gpio_ranges_fallback)(struct gpio_chip *gc,
-				       struct device_node *np);
-
 #endif /* CONFIG_OF_GPIO */
 };
 
-- 
2.39.0

Re: [PATCH v2 2/2] Revert "gpiolib: of: Introduce hook for missing gpio-ranges"

[Bartosz Golaszewski]

On Fri, Jan 13, 2023 at 6:14 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> This reverts commit 3550bba25d5587a701e6edf20e20984d2ee72c78.
>
> No users for this one, revert it for good.
> The ->add_pin_ranges() can be used instead.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: made it as a pure revert
>  drivers/gpio/gpiolib-of.c   |  5 -----
>  include/linux/gpio/driver.h | 12 ------------
>  2 files changed, 17 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index 72d8a3da31e3..266352b1a966 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -980,11 +980,6 @@ static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
>         if (!np)
>                 return 0;
>
> -       if (!of_property_read_bool(np, "gpio-ranges") &&
> -           chip->of_gpio_ranges_fallback) {
> -               return chip->of_gpio_ranges_fallback(chip, np);
> -       }
> -
>         group_names = of_find_property(np, group_names_propname, NULL);
>
>         for (;; index++) {
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index ddc7a14a274f..5ab50ba3c309 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -521,18 +521,6 @@ struct gpio_chip {
>          */
>         int (*of_xlate)(struct gpio_chip *gc,
>                         const struct of_phandle_args *gpiospec, u32 *flags);
> -
> -       /**
> -        * @of_gpio_ranges_fallback:
> -        *
> -        * Optional hook for the case that no gpio-ranges property is defined
> -        * within the device tree node "np" (usually DT before introduction
> -        * of gpio-ranges). So this callback is helpful to provide the
> -        * necessary backward compatibility for the pin ranges.
> -        */
> -       int (*of_gpio_ranges_fallback)(struct gpio_chip *gc,
> -                                      struct device_node *np);
> -
>  #endif /* CONFIG_OF_GPIO */
>  };
>
> --
> 2.39.0
>

Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH v2 1/2] pinctrl: bcm: bcm2835: Switch to use ->add_pin_ranges()

[Stefan Wahren]

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,

Re: [PATCH v2 1/2] pinctrl: bcm: bcm2835: Switch to use ->add_pin_ranges()

[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.

> > 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?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] pinctrl: bcm: bcm2835: Switch to use ->add_pin_ranges()

[Stefan Wahren]

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.

Re: [PATCH v2 1/2] pinctrl: bcm: bcm2835: Switch to use ->add_pin_ranges()

[Andy Shevchenko]

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.

-- 
With Best Regards,
Andy Shevchenko