Re: [PATCH 4/8] dt-bindings: pinctrl: Add Nuvoton WPCM450

From: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>, "Rob Herring" <robh+dt@kernel.org>,
	"OpenBMC Maillist" <openbmc@lists.ozlabs.org>,
	"Tomer Maimon" <tmaimon77@gmail.com>,
	"Joel Stanley" <joel@jms.id.au>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 4/8] dt-bindings: pinctrl: Add Nuvoton WPCM450
Date: Sun, 13 Jun 2021 11:53:11 +0200	[thread overview]
Message-ID: <YMXVh9FiW9SOWUCV@latitude> (raw)
In-Reply-To: <CACRpkdY6c750Dnh2H_6uEhOHJv-kLd_OpHqnuDKu4DQezZ2C_w@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 2973 bytes --]

On Fri, Jun 04, 2021 at 11:35:48AM +0200, Linus Walleij wrote:
> Hi Jonathan!
> 
> thanks for your patch!

Hi again!

> On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer
> <j.neuschaefer@gmx.net> wrote:
> 
> > +  interrupts: true
> 
> maxitems 4 right?

Yes.

> Make an enum:
> 
> interrupts:
>   - description: what IRQ0 is for
>   - description: what IRQ1 is for
>   - description: what IRQ2 is for
>   - description: what IRQ3 is for
> 
> And describe how these interrupts are used.

Good point.

- IRQ0 is for events (interrupts) from GPIOs  0 to  3.
- IRQ1 is for events (interrupts) from GPIOs  4 to 11.
- IRQ2 is for events (interrupts) from GPIOs 12 to 15.
- IRQ3 is for events (interrupts) from GPIOs 24 to 25.

> Because I am suspicious that they actually correspond to 4 different
> GPIO blocks, which should then be their own nodes.

Unfortunately, It's not that simple. The GPIO ports (as defined by the
groups of registers that do GPIO direction/input/output) are organised
like this:

- GPIO port 0 starts at GPIO   0 and is 16 GPIOs long.
- GPIO port 1 starts at GPIO  16 and is 16 GPIOs long.
- GPIO port 2 starts at GPIO  32 and is 16 GPIOs long.
- GPIO port 3 starts at GPIO  48 and is 16 GPIOs long.
- GPIO port 4 starts at GPIO  64 and is 16 GPIOs long.
- GPIO port 5 starts at GPIO  80 and is 16 GPIOs long.
- GPIO port 6 starts at GPIO  96 and is 18 GPIOs long.
- GPIO port 7 starts at GPIO 114 and is 14 GPIOs long.

(They didn't even make it so that each one has 16 GPIOs...)

As you can see, only a few GPIOs are connected to interrupt logic; most
of them are in port 0, and the remaining two are in port 1.

Forthermore, the GPIO ports don't all have the same set of registers, so
that the register layout of each can't be predicted by the offset of the
first register.

> 
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    pinctrl: pinctrl@b8003000 {
> > +      compatible = "nuvoton,wpcm450-pinctrl";
> > +      reg = <0xb8003000 0x1000>;
> > +      gpio-controller;
> > +      #gpio-cells = <2>;
> > +      interrupts = <2 IRQ_TYPE_LEVEL_HIGH
> > +                    3 IRQ_TYPE_LEVEL_HIGH
> > +                    4 IRQ_TYPE_LEVEL_HIGH
> > +                    5 IRQ_TYPE_LEVEL_HIGH>;
> 
> So these.
> 
> > +      rmii2 {
> > +        groups = "rmii2";
> > +        function = "rmii2";
> > +      };
> > +
> > +      pinctrl_uid: uid {
> > +        pins = "gpio14";
> > +        input-debounce = <1>;
> > +      };
> 
> I challenge you here and encourage you to put a node for each
> GPIO "port":
> 
>   port0: gpio@0 {
>  ....
>   };
>   port1: gpio@1 {
>  ....
>   };

Hmm, well, if the unit addresses simply go from 0 to 7, rather than
encoding offsets, this could work. But it won't help much with the IRQ
problem.

Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]