diff mbox series

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

Message ID 20241016-gpio-ngpios-v1-1-f16cf154f715@linaro.org
State New
Headers show
Series gpio: mmio: Support ngpios property | expand

Commit Message

Linus Walleij Oct. 16, 2024, 7:21 a.m. UTC
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(-)

Comments

Rob Herring (Arm) Oct. 16, 2024, 3:47 p.m. UTC | #1
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
Linus Walleij Oct. 16, 2024, 6:31 p.m. UTC | #2
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
Rob Herring (Arm) Oct. 16, 2024, 6:58 p.m. UTC | #3
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>
Linus Walleij Oct. 16, 2024, 7:12 p.m. UTC | #4
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
diff mbox series

Patch

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>;