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