Message ID | ZhgDCKhcHdwGoJ4Y@Z926fQmE5jqhFMgp6 |
---|---|
State | New |
Headers | show |
Series | [RFC,resend,after,bogus] gpio-syscon: do not report bogus error | expand |
Hi Etienne, thanks for your patch! On Thu, Apr 11, 2024 at 5:35 PM Etienne Buira <etienne.buira@free.fr> wrote: > Do not call dev_err when gpio,syscon-dev is not set albeit unneeded. > gpio-syscon is used with rk3328 chip, but this iomem region is > documented in > Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml and > does not look like to require gpio,syscon-dev setting. > > Signed-off-by: Etienne Buira <etienne.buira@free.fr> > X-Prefers: kind explanations over rotten tomatoes If you look in drivers/gpio/gpio-syscon.c you see this: priv->syscon = syscon_regmap_lookup_by_phandle(np, "gpio,syscon-dev"); if (IS_ERR(priv->syscon) && np->parent) priv->syscon = syscon_node_to_regmap(np->parent); So the driver will attempt to grab the syscon from the parent if it can't be located from a gpio,syscon-dev node. But it's not optional, look in arch/arm64/boot/dts/rockchip/rk3328.dtsi: grf: syscon@ff100000 { compatible = "rockchip,rk3328-grf", "syscon", "simple-mfd"; reg = <0x0 0xff100000 0x0 0x1000>; (...) grf_gpio: gpio { compatible = "rockchip,rk3328-grf-gpio"; gpio-controller; #gpio-cells = <2>; }; So indeed the parent is a sycon, and syscon_node_to_regmap(np->parent) will be used to populate priv->syscon on RK3328. So what you could do insteaf of the kludge is something like: bool has_parent_syscon = false; priv->syscon = syscon_regmap_lookup_by_phandle(np, "gpio,syscon-dev"); if (IS_ERR(priv->syscon) && np->parent) { priv->syscon = syscon_regmap_lookup_by_phandle(np, "gpio,syscon-dev"); has_parent_syscon = true; } if (IS_ERR(priv->syscon)) return PTR_ERR(priv->syscon); Then when you get to the code you disable for the flag instead of: if (!(priv->data->flags & GPIO_SYSCON_FEAT_NODEV)) { (...) instead do: if (!has_parent_syscon) { (...) What do you think about this? Yours, Linus Walleij
On Fri, Apr 12, 2024 at 12:44:34PM +0200, Linus Walleij wrote: > Hi Etienne, > > thanks for your patch! > > On Thu, Apr 11, 2024 at 5:35 PM Etienne Buira <etienne.buira@free.fr> wrote: > > > Do not call dev_err when gpio,syscon-dev is not set albeit unneeded. > > gpio-syscon is used with rk3328 chip, but this iomem region is > > documented in > > Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml and > > does not look like to require gpio,syscon-dev setting. > > > > Signed-off-by: Etienne Buira <etienne.buira@free.fr> > > X-Prefers: kind explanations over rotten tomatoes ../.. > So indeed the parent is a sycon, and syscon_node_to_regmap(np->parent) will > be used to populate priv->syscon on RK3328. ../.. > if (!has_parent_syscon) { > (...) > > What do you think about this? ../.. Hi Linus, Thanks for your review. IIUC, that would prevent calling dev_err() if, for example, gpio,syscon-dev were forgotten from arch/arm/boot/dts/ti/keystone/keystone-k2g.dtsi, dspgpio0 node although it is needed and would result in funny things without notice. Thinking twice about it, a cleaner way looks to add gpio,syscon-dev node to rk3328.dtsi. I'll send the one-liner to relevant people (that would be really easier if there were only one repo, with different branches...). Do you agree? Regards. Note: I'm not subscribed to list, so please To or CC me.
diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c index 6e1a2581e6ae..14c4f224eb07 100644 --- a/drivers/gpio/gpio-syscon.c +++ b/drivers/gpio/gpio-syscon.c @@ -16,6 +16,7 @@ #define GPIO_SYSCON_FEAT_IN BIT(0) #define GPIO_SYSCON_FEAT_OUT BIT(1) #define GPIO_SYSCON_FEAT_DIR BIT(2) +#define GPIO_SYSCON_FEAT_NODEV BIT(3) /* SYSCON driver is designed to use 32-bit wide registers */ #define SYSCON_REG_SIZE (4) @@ -27,7 +28,8 @@ * @flags: Set of GPIO_SYSCON_FEAT_ flags: * GPIO_SYSCON_FEAT_IN: GPIOs supports input, * GPIO_SYSCON_FEAT_OUT: GPIOs supports output, - * GPIO_SYSCON_FEAT_DIR: GPIOs supports switch direction. + * GPIO_SYSCON_FEAT_DIR: GPIOs supports switch direction, + * GPIO_SYSCON_FEAT_NODEV: gpio,syscon-dev do not have to be set. * @bit_count: Number of bits used as GPIOs. * @dat_bit_offset: Offset (in bits) to the first GPIO bit. * @dir_bit_offset: Optional offset (in bits) to the first bit to switch @@ -149,7 +151,7 @@ static void rockchip_gpio_set(struct gpio_chip *chip, unsigned int offset, static const struct syscon_gpio_data rockchip_rk3328_gpio_mute = { /* RK3328 GPIO_MUTE is an output only pin at GRF_SOC_CON10[1] */ - .flags = GPIO_SYSCON_FEAT_OUT, + .flags = GPIO_SYSCON_FEAT_OUT | GPIO_SYSCON_FEAT_NODEV, .bit_count = 1, .dat_bit_offset = 0x0428 * 8 + 1, .set = rockchip_gpio_set, @@ -221,19 +223,21 @@ static int syscon_gpio_probe(struct platform_device *pdev) if (IS_ERR(priv->syscon)) return PTR_ERR(priv->syscon); - ret = of_property_read_u32_index(np, "gpio,syscon-dev", 1, - &priv->dreg_offset); - if (ret) - dev_err(dev, "can't read the data register offset!\n"); + if (!(priv->data->flags & GPIO_SYSCON_FEAT_NODEV)) { + ret = of_property_read_u32_index(np, "gpio,syscon-dev", 1, + &priv->dreg_offset); + if (ret) + dev_err(dev, "can't read the data register offset!\n"); - priv->dreg_offset <<= 3; + priv->dreg_offset <<= 3; - ret = of_property_read_u32_index(np, "gpio,syscon-dev", 2, - &priv->dir_reg_offset); - if (ret) - dev_dbg(dev, "can't read the dir register offset!\n"); + ret = of_property_read_u32_index(np, "gpio,syscon-dev", 2, + &priv->dir_reg_offset); + if (ret) + dev_dbg(dev, "can't read the dir register offset!\n"); - priv->dir_reg_offset <<= 3; + priv->dir_reg_offset <<= 3; + } priv->chip.parent = dev; priv->chip.owner = THIS_MODULE;
Do not call dev_err when gpio,syscon-dev is not set albeit unneeded. gpio-syscon is used with rk3328 chip, but this iomem region is documented in Documentation/devicetree/bindings/gpio/rockchip,rk3328-grf-gpio.yaml and does not look like to require gpio,syscon-dev setting. Signed-off-by: Etienne Buira <etienne.buira@free.fr> X-Prefers: kind explanations over rotten tomatoes --- drivers/gpio/gpio-syscon.c | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) base-commit: 4cece764965020c22cff7665b18a012006359095