[PATCH v2 06/10] gpio: rockchip: Get pinctrl device from gpio-ranges prop

[Quentin Schulz]

Hi Jonas,

On 8/3/24 12:56 AM, Jonas Karlman wrote:
> Get pinctrl device from gpio-ranges phandle when the property exists,
> fallback to get the first pinctrl device.
> 
> Signed-off-by: Jonas Karlman <jonas at kwiboo.se>
> Reviewed-by: Kever Yang <kever.yang at rock-chips.com>
> ---
> v2: Collect r-b tag
> ---
>   drivers/gpio/rk_gpio.c | 27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/rk_gpio.c b/drivers/gpio/rk_gpio.c
> index 24ba12dd820e..abece6409ae0 100644
> --- a/drivers/gpio/rk_gpio.c
> +++ b/drivers/gpio/rk_gpio.c
> @@ -181,12 +181,6 @@ static int rockchip_gpio_probe(struct udevice *dev)
>   
>   	priv->regs = dev_read_addr_ptr(dev);
>   
> -	if (CONFIG_IS_ENABLED(PINCTRL)) {
> -		ret = uclass_first_device_err(UCLASS_PINCTRL, &priv->pinctrl);
> -		if (ret)
> -			return ret;
> -	}
> -
>   	/*
>   	 * If "gpio-ranges" is present in the devicetree use it to parse
>   	 * the GPIO bank ID, otherwise use the legacy method.
> @@ -194,16 +188,33 @@ static int rockchip_gpio_probe(struct udevice *dev)
>   	ret = ofnode_parse_phandle_with_args(dev_ofnode(dev),
>   					     "gpio-ranges", NULL, 3,
>   					     0, &args);
> -	if (!ret || ret != -ENOENT) {
> +	if (!ret) {
>   		uc_priv->gpio_count = args.args[2];
>   		priv->bank = args.args[1] / ROCKCHIP_GPIOS_PER_BANK;

I assume this is broken if a bank has less than 32 GPIOs, e.g. rk3288 
and px30's GPIO0?

E.g. this would mean if gpio-ranges is proper in the DT (it isn't for 
PX30 today), GPIO1 would be detected as being GPIO0 because GPIO0 is 
only 24 pins?

I assume we could trick this by doing (base+nrpins-1)/32 which would 
make it much less likely to have overlaps (we would need multiple 
smaller banks to offset bank's bases enough).

> -	} else {
> +
> +		if (CONFIG_IS_ENABLED(PINCTRL)) {
> +			ret = uclass_get_device_by_ofnode(UCLASS_PINCTRL,
> +							  args.node,
> +							  &priv->pinctrl);

Could have been two separate commits here:
1) getting pinctrl from property
2) switching the order to save a call to uclass_first_device_err if we 
have a gpio-ranges property

> +			if (ret)
> +				return ret;
> +		}
> +	} else if (ret == -ENOENT || !CONFIG_IS_ENABLED(PINCTRL)) {
>   		uc_priv->gpio_count = ROCKCHIP_GPIOS_PER_BANK;

Oof, another issue here wrt amount of gpios in a bank. Seems like I 
opened another can of worms :/

>   		ret = dev_read_alias_seq(dev, &priv->bank);
>   		if (ret) {
>   			end = strrchr(dev->name, '@');
>   			priv->bank = trailing_strtoln(dev->name, end);
>   		}
> +
> +		if (CONFIG_IS_ENABLED(PINCTRL)) {
> +			ret = uclass_first_device_err(UCLASS_PINCTRL,
> +						      &priv->pinctrl);
> +			if (ret)
> +				return ret;
> +		}
> +	} else {
> +		return ret;
>   	}

I'm getting a bit confused by the change of if conditions here...

Wouldn't it be much simpler to just do:

if (!priv->pinctrl && CONFIG_IS_ENABLED(PINCTRL)) {
     ret = uclass_first_device_err(UCLASS_PINCTRL, &priv->pinctrl);
     if (ret)
         return ret;
}

after the existing if-else and removing the uclass_first_device_err 
currently before it?

Also not sure to understand why we suddenly do not accept any error code 
(we used to accept everything but ENOENT).

Cheers,
Quentin