[PATCH v2 0/2] gpio: mmio: Support ngpios property

[PATCH v2 0/2] gpio: mmio: Support ngpios property

[Linus Walleij]

I thought this generic property was already supported by the
generic MMIO bindings and code, but no.

It's a pretty obvious usecase to just use some from 0..n
of a MMIO GPIO bank, so add support for this.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes in v2:
- Collect Rob's ACK on patch 1
- The pdata member is called ngpio not ngpios which shows up if
  I compile the gpio-mmio driver with the *right* flags.
- Link to v1: https://lore.kernel.org/r/20241016-gpio-ngpios-v1-0-f16cf154f715@linaro.org

---
Linus Walleij (2):
      dt-bindings: gpio-mmio: Add ngpios property
      gpio: mmio: Parse ngpios property

 Documentation/devicetree/bindings/gpio/gpio-mmio.yaml | 13 ++++++++++++-
 drivers/gpio/gpio-mmio.c                              |  4 ++++
 2 files changed, 16 insertions(+), 1 deletion(-)
---
base-commit: 9852d85ec9d492ebef56dc5f229416c925758edc
change-id: 20241016-gpio-ngpios-0cad57f0a757

Best regards,
-- 
Linus Walleij <linus.walleij@linaro.org>

[PATCH v2 1/2] dt-bindings: gpio-mmio: Add ngpios property

[Linus Walleij]

This adds the ngpios property to MMIO GPIO. We restrict the
property to 1..63 since there is no point in 0 GPIO lines and
we support up to 64bits wide registers for now.

Acked-by: Rob Herring (Arm) <robh@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/devicetree/bindings/gpio/gpio-mmio.yaml | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/gpio/gpio-mmio.yaml b/Documentation/devicetree/bindings/gpio/gpio-mmio.yaml
index b394e058256e..87e986386f32 100644
--- a/Documentation/devicetree/bindings/gpio/gpio-mmio.yaml
+++ b/Documentation/devicetree/bindings/gpio/gpio-mmio.yaml
@@ -37,7 +37,8 @@ properties:
     description:
       A list of registers in the controller. The width of each register is
       determined by its size. All registers must have the same width. The number
-      of GPIOs is set by the width, with bit 0 corresponding to GPIO 0.
+      of GPIOs is set by the width, with bit 0 corresponding to GPIO 0, unless
+      the ngpios property further restricts the number of used lines.
     items:
       - description:
           Register to READ the value of the GPIO lines. If GPIO line is high,
@@ -74,6 +75,15 @@ properties:
 
   native-endian: true
 
+  ngpios:
+    minimum: 1
+    maximum: 63
+    description:
+      If this property is present the number of usable GPIO lines are restricted
+      to the first 0 .. ngpios lines. This is useful when the GPIO MMIO register
+      has 32 bits for GPIO but only the first 12 are actually connected to
+      real electronics, and then we set ngpios to 12.
+
   no-output:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
@@ -111,6 +121,7 @@ examples:
       compatible = "brcm,bcm6345-gpio";
       reg-names = "dirout", "dat";
       reg = <0xfffe0406 2>, <0xfffe040a 2>;
+      ngpios = <15>;
       native-endian;
       gpio-controller;
       #gpio-cells = <2>;

-- 
2.46.2

[PATCH v2 2/2] gpio: mmio: Parse ngpios property

[Linus Walleij]

This makes the MMIO GPIO driver parse the ngpios property from
devices instatiated directly from the device tree so we can
further restrict the number of GPIOs down from the number of
bits on the target register.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-mmio.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpio/gpio-mmio.c b/drivers/gpio/gpio-mmio.c
index d89e78f0ead3..c772f1b6e694 100644
--- a/drivers/gpio/gpio-mmio.c
+++ b/drivers/gpio/gpio-mmio.c
@@ -694,6 +694,7 @@ MODULE_DEVICE_TABLE(of, bgpio_of_match);
 static struct bgpio_pdata *bgpio_parse_fw(struct device *dev, unsigned long *flags)
 {
 	struct bgpio_pdata *pdata;
+	u32 ngpios;
 
 	if (!dev_fwnode(dev))
 		return NULL;
@@ -704,6 +705,9 @@ static struct bgpio_pdata *bgpio_parse_fw(struct device *dev, unsigned long *fla
 
 	pdata->base = -1;
 
+	if (!device_property_read_u32(dev, "ngpios", &ngpios))
+		pdata->ngpio = ngpios;
+
 	if (device_is_big_endian(dev))
 		*flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
 

-- 
2.46.2

Re: [PATCH v2 0/2] gpio: mmio: Support ngpios property

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>


On Thu, 17 Oct 2024 08:46:07 +0200, Linus Walleij wrote:
> I thought this generic property was already supported by the
> generic MMIO bindings and code, but no.
> 
> It's a pretty obvious usecase to just use some from 0..n
> of a MMIO GPIO bank, so add support for this.
> 
> 
> [...]

Applied, thanks!

[1/2] dt-bindings: gpio-mmio: Add ngpios property
      commit: b4c69d471b72aa70766d94a11c31bc4c13f29eac
[2/2] gpio: mmio: Parse ngpios property
      commit: 1ed9f099323ed366291291826a8beb1a5adfc2c8

Best regards,
-- 
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Re: [PATCH v2 0/2] gpio: mmio: Support ngpios property

[Andy Shevchenko]

On Thu, Oct 17, 2024 at 08:46:07AM +0200, Linus Walleij wrote:
> I thought this generic property was already supported by the
> generic MMIO bindings and code, but no.

I have two issues with the second patch here.

First one is why? What the *practical* issue you have? Can you elaborate
on that?

Second one, is there any other way to avoid duplication of the code so
we have one place of the property parsing?

For the background I have to mention this commit:
55b2395e4e92 ("gpio: mmio: handle "ngpios" properly in bgpio_init()")

If there is an issue, it should be fixed rather than adding a shortcut
with not fully known consequences.

TL;DR: on a brief look the second patch should be reverted (or simply
dropped as it's easy to do while it's on top of the branch.

> It's a pretty obvious usecase to just use some from 0..n
> of a MMIO GPIO bank, so add support for this.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 0/2] gpio: mmio: Support ngpios property

[Linus Walleij]

On Fri, Oct 18, 2024 at 1:30 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:


> First one is why? What the *practical* issue you have? Can you elaborate
> on that?

Sure, there are these hardwares that probe directly from the
gpio-mmio driver:
Documentation/devicetree/bindings/gpio/gpio-mmio.yaml
properties:
  compatible:
    enum:
      - brcm,bcm6345-gpio
      - ni,169445-nand-gpio
      - wd,mbl-gpio # Western Digital MyBook Live memory-mapped GPIO controller


The practical issue is (similar to what was responded to Rob
in patch 2/2) that non-existing GPIOs will get exposed to userspace.

For patch 1/2 (adding the DT binding) it would be that without
ngpios we do not model the hardware properly.

The objection "it makes no harm to register GPIO lines
for all bits in the register" can likewise be raised to the
other 28 (if I count correctly) GPIO drivers that use this
property (git grep ngpios drivers/gpio) and I think the train left the
station long ago to object to the property in general, people
don't want to expose non-existing GPIOs to the GPIO
framework.

> Second one, is there any other way to avoid duplication of the code so
> we have one place of the property parsing?
>
> For the background I have to mention this commit:
> 55b2395e4e92 ("gpio: mmio: handle "ngpios" properly in bgpio_init()")

Oh well spotted! I completely missed the fact that we already
added ngpios parsing elsewhere in the driver.

Bartosz, can you please drop patch 2/2?

Patch 1/2 is needed however: it is just documenting the behaviour
that is already implemented.

Yours,
Linus Walleij

Re: [PATCH v2 0/2] gpio: mmio: Support ngpios property

[Bartosz Golaszewski]

On Fri, Oct 18, 2024 at 1:49 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Oh well spotted! I completely missed the fact that we already
> added ngpios parsing elsewhere in the driver.
>
> Bartosz, can you please drop patch 2/2?
>
> Patch 1/2 is needed however: it is just documenting the behaviour
> that is already implemented.
>

Done.

Bart

Re: [PATCH v2 0/2] gpio: mmio: Support ngpios property

[Andy Shevchenko]

On Fri, Oct 18, 2024 at 01:49:40PM +0200, Linus Walleij wrote:
> On Fri, Oct 18, 2024 at 1:30 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> 
> > First one is why? What the *practical* issue you have? Can you elaborate
> > on that?
> 
> Sure, there are these hardwares that probe directly from the
> gpio-mmio driver:
> Documentation/devicetree/bindings/gpio/gpio-mmio.yaml
> properties:
>   compatible:
>     enum:
>       - brcm,bcm6345-gpio
>       - ni,169445-nand-gpio
>       - wd,mbl-gpio # Western Digital MyBook Live memory-mapped GPIO controller
> 
> The practical issue is (similar to what was responded to Rob
> in patch 2/2) that non-existing GPIOs will get exposed to userspace.
> 
> For patch 1/2 (adding the DT binding) it would be that without
> ngpios we do not model the hardware properly.
> 
> The objection "it makes no harm to register GPIO lines
> for all bits in the register" can likewise be raised to the
> other 28 (if I count correctly) GPIO drivers that use this
> property (git grep ngpios drivers/gpio) and I think the train left the
> station long ago to object to the property in general, people
> don't want to expose non-existing GPIOs to the GPIO
> framework.

Sorry that I likely wasn't clear enough. My question was if you really
experienced any bugs in practice. The above is the theory part and
I completely agree with.

> > Second one, is there any other way to avoid duplication of the code so
> > we have one place of the property parsing?
> >
> > For the background I have to mention this commit:
> > 55b2395e4e92 ("gpio: mmio: handle "ngpios" properly in bgpio_init()")
> 
> Oh well spotted! I completely missed the fact that we already
> added ngpios parsing elsewhere in the driver.
> 
> Bartosz, can you please drop patch 2/2?

> Patch 1/2 is needed however: it is just documenting the behaviour
> that is already implemented.

I'm not agianst this, the first patch is the correct advertisement.
My questioning was related solely to the second patch in the series.

-- 
With Best Regards,
Andy Shevchenko