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@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxx/

[2] - 
https://lore.kernel.org/linux-arm-kernel/75266ed1-666a-138b-80f1-ae9a06b7bdf3@xxxxxxxx/

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