diff mbox series

[v2] gpio-syscon: do not report bogus error

Message ID ZiJkxBU3bYNWOdbK@Z926fQmE5jqhFMgp6
State New
Headers show
Series [v2] gpio-syscon: do not report bogus error | expand

Commit Message

Etienne Buira April 19, 2024, 12:34 p.m. UTC 
Do not issue "can't read the data register offset!" 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 require gpio,syscon-dev setting.

It has been suggested to automatically detect if node has a valid
parent, but that would defeat the purpose of error message, for example
arch/arm/boot/dts/ti/keystone/keystone-k2g.dtsi could then be used
without gpio,syscon-dev, and lead to funny results without error
message.

It has been tried to mandate use of gpio,syscon-dev, but that raised
objection.

So while this patch may be kludgy, it looks the less bad to address
the spurious dev_err call.

v2:
  - changed flag name

Signed-off-by: Etienne Buira <etienne.buira@free.fr>
---
 drivers/gpio/gpio-syscon.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)


base-commit: 4cece764965020c22cff7665b18a012006359095

Comments

Bartosz Golaszewski May 13, 2024, 2:40 p.m. UTC | #1 
On Fri, Apr 19, 2024 at 2:35 PM Etienne Buira <etienne.buira@free.fr> wrote:
>
> Do not issue "can't read the data register offset!" 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 require gpio,syscon-dev setting.
>
> It has been suggested to automatically detect if node has a valid
> parent, but that would defeat the purpose of error message, for example
> arch/arm/boot/dts/ti/keystone/keystone-k2g.dtsi could then be used
> without gpio,syscon-dev, and lead to funny results without error
> message.
>
> It has been tried to mandate use of gpio,syscon-dev, but that raised
> objection.
>
> So while this patch may be kludgy, it looks the less bad to address
> the spurious dev_err call.
>
> v2:
>   - changed flag name
>
> Signed-off-by: Etienne Buira <etienne.buira@free.fr>
> ---

Linus,

Do you have any objections? If not, I'll pick it up after the merge
window. The patch does not look very clean but I cannot find anything
obviously wrong.

Bartosz
Linus Walleij May 13, 2024, 8:08 p.m. UTC | #2 
On Fri, Apr 19, 2024 at 2:35 PM Etienne Buira <etienne.buira@free.fr> wrote:

> It has been suggested to automatically detect if node has a valid
> parent, but that would defeat the purpose of error message, for example
> arch/arm/boot/dts/ti/keystone/keystone-k2g.dtsi could then be used
> without gpio,syscon-dev, and lead to funny results without error
> message.

Now the DTSI does have all these gpio,syscon-dev pointers so
this is just a theoretical problem, right?

The fact that someone can write a DTS to shoot themselves in the
foot doesn't concern me so much, there are many ways to do that
if one wants to.

> It has been tried to mandate use of gpio,syscon-dev, but that raised
> objection.

I don't get it. In the example you mention it is already mandated:
Look in
Documentation/devicetree/bindings/gpio/gpio-dsp-keystone.txt

Required properties:
(...)
- ti,syscon-dev: phandle/offset pair. The phandle to syscon used to
  access device state control registers and the offset of device's specific
  registers within device state control registers range.

Apart from the obvious spelling error (should be gpio,syscon-dev)
it is right there. (The spelling error is a binding bug, patches welcome.)

If this binding is converted to YAML and the property renamed
properly to what is actually used in the device tree then the schema
check will even yell about it.

I don't see what the problem is to let the driver just look for a parent
if gpio,syscon-dev is not set.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-syscon.c b/drivers/gpio/gpio-syscon.c
index 6e1a2581e6ae..4f103ce26019 100644
--- a/drivers/gpio/gpio-syscon.c
+++ b/drivers/gpio/gpio-syscon.c
@@ -13,9 +13,10 @@ 
 #include <linux/regmap.h>
 #include <linux/mfd/syscon.h>
 
-#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_IN		BIT(0)
+#define GPIO_SYSCON_FEAT_OUT		BIT(1)
+#define GPIO_SYSCON_FEAT_DIR		BIT(2)
+#define GPIO_SYSCON_FEAT_USE_PARENT	BIT(3)
 
 /* SYSCON driver is designed to use 32-bit wide registers */
 #define SYSCON_REG_SIZE		(4)
@@ -27,7 +28,9 @@ 
  * @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_USE_PARENT:	gpio,syscon-dev do not
+ *				have to be set, parent regspace will be used.
  * @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 +152,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_USE_PARENT,
 	.bit_count	= 1,
 	.dat_bit_offset = 0x0428 * 8 + 1,
 	.set		= rockchip_gpio_set,
@@ -221,19 +224,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_USE_PARENT)) {
+		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;