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

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@kwiboo.se Reviewed-by: Kever Yang kever.yang@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