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

[PATCH 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>
---
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 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.

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

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

[Rob Herring]

On Wed, Oct 16, 2024 at 2:21 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> 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.

Why does it need to be restricted? Is something bad going to happen if
for some reason someone tries to control a non-existent GPIO? If there
is, maybe there should be a specific compatible as the h/w is not so
generic.

Rob

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

[Linus Walleij]

On Wed, Oct 16, 2024 at 5:47 PM Rob Herring <robh@kernel.org> wrote:
> On Wed, Oct 16, 2024 at 2:21 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> >
> > 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.
>
> Why does it need to be restricted? Is something bad going to happen if
> for some reason someone tries to control a non-existent GPIO?

Unlikely. But the biggest inconvenience is that non-existing GPIO lines
gets exposed to userspace which causes confusion. It's a bit like
saying you have 32 harddisks on your system just because the register
has 32 bits.

> If there
> is, maybe there should be a specific compatible as the h/w is not so
> generic.

The gpio-mmio is quite generic, it's the most generic GPIO
driver we have.

The ngpios property is also generic, it is in:
Documentation/devicetree/bindings/gpio/gpio.txt
(commit aacaffd1d9a6f8e2c7369d83c21d41c3b53e2edc)

At the time (2015) I just documented the already widespread use
of this property.

It is also reflected in several of the new yaml bindings, a git grep
ngpios will show you.

Yours,
Linus Walleij

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

[Rob Herring]

On Wed, Oct 16, 2024 at 1:32 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Wed, Oct 16, 2024 at 5:47 PM Rob Herring <robh@kernel.org> wrote:
> > On Wed, Oct 16, 2024 at 2:21 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> > >
> > > 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.
> >
> > Why does it need to be restricted? Is something bad going to happen if
> > for some reason someone tries to control a non-existent GPIO?
>
> Unlikely. But the biggest inconvenience is that non-existing GPIO lines
> gets exposed to userspace which causes confusion. It's a bit like
> saying you have 32 harddisks on your system just because the register
> has 32 bits.

I would love to have 32 harddisks.

My analogy is we don't define how many interrupts an interrupt
controller has. That's generally either implicit or you just don't
need to know (other than validating used interrupt numbers). Of course
interrupts aren't exposed to userspace.

This property has shortcomings if you want to prevent exposing
non-existent GPIOs to userspace. You really need a mask because what
if GPIO0 doesn't exist?

> > If there
> > is, maybe there should be a specific compatible as the h/w is not so
> > generic.
>
> The gpio-mmio is quite generic, it's the most generic GPIO
> driver we have.
>
> The ngpios property is also generic, it is in:
> Documentation/devicetree/bindings/gpio/gpio.txt
> (commit aacaffd1d9a6f8e2c7369d83c21d41c3b53e2edc)
>
> At the time (2015) I just documented the already widespread use
> of this property.
>
> It is also reflected in several of the new yaml bindings, a git grep
> ngpios will show you.

Yes, I know. You can also find push back on using it.

Anyway, I did my push back and what's one more user (sigh), so:

Acked-by: Rob Herring (Arm) <robh@kernel.org>

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

[Linus Walleij]

On Wed, Oct 16, 2024 at 8:59 PM Rob Herring <robh@kernel.org> wrote:

> This property has shortcomings if you want to prevent exposing
> non-existent GPIOs to userspace. You really need a mask because what
> if GPIO0 doesn't exist?

That does happen in $OS (DT hat on), a famous OS has
such sparse masks for GPIO chips. They have not become
DT bindings (yet). As it turns out they are mostly used for cases
where firmware/BIOS/secure world want to "steal" some GPIOs
out of the pool and as such has nothing to do with hardware
description.

It has been shown that in practice what HW engineers
do is line up as many IP instances they need and just truncate
the last one, rendering the upper bits unused. They are practical
and least effort-oriented people who have yet to be seen
doing something crazy like e.g. connect every second bit to
an actual pin.

> > It is also reflected in several of the new yaml bindings, a git grep
> > ngpios will show you.
>
> Yes, I know. You can also find push back on using it.
>
> Anyway, I did my push back and what's one more user (sigh), so:
>
> Acked-by: Rob Herring (Arm) <robh@kernel.org>

Thanks Rob, some push back is always good!

Yours,
Linus Walleij

[PATCH 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..9e944c191551 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->ngpios = ngpios;
+
 	if (device_is_big_endian(dev))
 		*flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
 

-- 
2.46.2

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

[kernel test robot]

Hi Linus,

kernel test robot noticed the following build errors:

[auto build test ERROR on 9852d85ec9d492ebef56dc5f229416c925758edc]

url:    https://github.com/intel-lab-lkp/linux/commits/Linus-Walleij/dt-bindings-gpio-mmio-Add-ngpios-property/20241016-152354
base:   9852d85ec9d492ebef56dc5f229416c925758edc
patch link:    https://lore.kernel.org/r/20241016-gpio-ngpios-v1-2-f16cf154f715%40linaro.org
patch subject: [PATCH 2/2] gpio: mmio: Parse ngpios property
config: i386-buildonly-randconfig-001-20241017 (https://download.01.org/0day-ci/archive/20241017/202410170940.c317EO5s-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410170940.c317EO5s-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410170940.c317EO5s-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpio/gpio-mmio.c: In function 'bgpio_parse_fw':
>> drivers/gpio/gpio-mmio.c:709:24: error: 'struct bgpio_pdata' has no member named 'ngpios'; did you mean 'ngpio'?
     709 |                 pdata->ngpios = ngpios;
         |                        ^~~~~~
         |                        ngpio


vim +709 drivers/gpio/gpio-mmio.c

   693	
   694	static struct bgpio_pdata *bgpio_parse_fw(struct device *dev, unsigned long *flags)
   695	{
   696		struct bgpio_pdata *pdata;
   697		u32 ngpios;
   698	
   699		if (!dev_fwnode(dev))
   700			return NULL;
   701	
   702		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
   703		if (!pdata)
   704			return ERR_PTR(-ENOMEM);
   705	
   706		pdata->base = -1;
   707	
   708		if (!device_property_read_u32(dev, "ngpios", &ngpios))
 > 709			pdata->ngpios = ngpios;
   710	
   711		if (device_is_big_endian(dev))
   712			*flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
   713	
   714		if (device_property_read_bool(dev, "no-output"))
   715			*flags |= BGPIOF_NO_OUTPUT;
   716	
   717		return pdata;
   718	}
   719	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Linus,

kernel test robot noticed the following build errors:

[auto build test ERROR on 9852d85ec9d492ebef56dc5f229416c925758edc]

url:    https://github.com/intel-lab-lkp/linux/commits/Linus-Walleij/dt-bindings-gpio-mmio-Add-ngpios-property/20241016-152354
base:   9852d85ec9d492ebef56dc5f229416c925758edc
patch link:    https://lore.kernel.org/r/20241016-gpio-ngpios-v1-2-f16cf154f715%40linaro.org
patch subject: [PATCH 2/2] gpio: mmio: Parse ngpios property
config: i386-buildonly-randconfig-003-20241017 (https://download.01.org/0day-ci/archive/20241017/202410170940.KyJaAkpF-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241017/202410170940.KyJaAkpF-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410170940.KyJaAkpF-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpio/gpio-mmio.c:709:10: error: no member named 'ngpios' in 'struct bgpio_pdata'; did you mean 'ngpio'?
     709 |                 pdata->ngpios = ngpios;
         |                        ^~~~~~
         |                        ngpio
   include/linux/gpio/driver.h:688:6: note: 'ngpio' declared here
     688 |         int ngpio;
         |             ^
   1 error generated.


vim +709 drivers/gpio/gpio-mmio.c

   693	
   694	static struct bgpio_pdata *bgpio_parse_fw(struct device *dev, unsigned long *flags)
   695	{
   696		struct bgpio_pdata *pdata;
   697		u32 ngpios;
   698	
   699		if (!dev_fwnode(dev))
   700			return NULL;
   701	
   702		pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
   703		if (!pdata)
   704			return ERR_PTR(-ENOMEM);
   705	
   706		pdata->base = -1;
   707	
   708		if (!device_property_read_u32(dev, "ngpios", &ngpios))
 > 709			pdata->ngpios = ngpios;
   710	
   711		if (device_is_big_endian(dev))
   712			*flags |= BGPIOF_BIG_ENDIAN_BYTE_ORDER;
   713	
   714		if (device_property_read_bool(dev, "no-output"))
   715			*flags |= BGPIOF_NO_OUTPUT;
   716	
   717		return pdata;
   718	}
   719	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki