Re: [PATCH 2/2] gpio: rockchip: get pinctrl node from 'gpio-ranges' property

[Heiko Stübner <heiko@xxxxxxxxx>]

Hi Jianqun,

Am Donnerstag, 3. März 2022, 07:22:11 CET schrieb Jianqun Xu:
> The dt nodes for rockchip soc designes as
> 
> 	pinctrl: pinctrl {
> 		gpio {
> 			gpio-ranges = <&pinctrl xxx>;
> 		};
> 	};
> 
> Currently, we get the pinctrl dt node from parent of gpio, this patch
> try to get pinctrl dt node from 'gpio-ranges' property.
> 
> After this patch, the dt nodes possible to be
> 
> 	gpio {
> 		gpio-ranges = <&pinctrl xxx>;
> 	};
> 
> 	pinctrl: pinctrl {
> 
> 	};
> 
> then the gpio driver could register as platform device itself, but not
> populate from pinctrl driver.

The change looks interesting, as it would solve that long-standing
design-issue I "created" back in 2013 ;-) .

Though you need to keep some things in mind:

(1) Such a change should be reflected in the devicetree binding
    as it involves a different form of nodes and introduces.

    Looking at the binding description, using gpio-ranges to map
    to specific pinctrl pins really seems to be a valid use for this.


(2) Keep things backwards compatible.
    Old devicetrees should stay working with new kernel versions

    A common pattern is to try the new approach and if that fails
    try the "deprecated" method, which should be nicely doable
    when looking at the code change below.


Heiko

> Signed-off-by: Jianqun Xu <jay.xu@xxxxxxxxxxxxxx>
> ---
>  drivers/gpio/gpio-rockchip.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-rockchip.c b/drivers/gpio/gpio-rockchip.c
> index 1da0324445cc..46c54dff92db 100644
> --- a/drivers/gpio/gpio-rockchip.c
> +++ b/drivers/gpio/gpio-rockchip.c
> @@ -690,6 +690,9 @@ rockchip_gpio_find_bank(struct pinctrl_dev *pctldev, int id)
>  	int i, found = 0;
>  
>  	info = pinctrl_dev_get_drvdata(pctldev);
> +	if (!info)
> +		return NULL;
> +
>  	bank = info->ctrl->pin_banks;
>  	for (i = 0; i < info->ctrl->nr_banks; i++, bank++) {
>  		if (bank->bank_num == id) {
> @@ -705,15 +708,16 @@ static int rockchip_gpio_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> -	struct device_node *pctlnp = of_get_parent(np);
> +	struct device_node *pctlnp = NULL;
>  	struct pinctrl_dev *pctldev = NULL;
>  	struct rockchip_pin_bank *bank = NULL;
>  	struct rockchip_pin_output_deferred *cfg;
>  	static int gpio;
>  	int id, ret;
>  
> -	if (!np || !pctlnp)
> -		return -ENODEV;
> +	pctlnp = of_parse_phandle(np, "gpio-ranges", 0);
> +	if (!pctlnp)
> +		pctlnp = of_get_parent(np);
>  
>  	pctldev = of_pinctrl_get(pctlnp);
>  	if (!pctldev)
>