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

This binding is heavily based on the one for NPCM7xx, because the
hardware is similar. One notable difference is that there are no
sub-nodes for GPIO banks, because the GPIO registers are arranged
differently.

Certain pins support blink patterns in hardware. This is currently not
modelled in the DT binding.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 .../pinctrl/nuvoton,wpcm450-pinctrl.yaml      | 142 ++++++++++++++++++
 1 file changed, 142 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml

--
2.30.2

Hi Jonathan!

thanks for your patch!

On Wed, Jun 2, 2021 at 2:04 PM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> wrote:

> +  interrupts: true

maxitems 4 right?

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. Because I am suspicious that they
actually correspond to 4 different GPIO blocks, which should then be their own
nodes.

> +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 {
 ....
  };


> +    gpio-keys {
> +      compatible = "gpio-keys";
> +      pinctrl-names = "default";
> +      pinctrl-0 = <&pinctrl_uid>;
> +
> +      uid {
> +        label = "UID";
> +        linux,code = <102>;
> +        gpios = <&pinctrl 14 GPIO_ACTIVE_HIGH>;

Would be gpios <&port0 14...>

Yours,
Linus Walleij

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

On Wed, Jun 02, 2021 at 02:03:25PM +0200, Jonathan Neuschäfer wrote:
> This binding is heavily based on the one for NPCM7xx, because the
> hardware is similar. One notable difference is that there are no
> sub-nodes for GPIO banks, because the GPIO registers are arranged
> differently.
> 
> Certain pins support blink patterns in hardware. This is currently not
> modelled in the DT binding.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  .../pinctrl/nuvoton,wpcm450-pinctrl.yaml      | 142 ++++++++++++++++++
>  1 file changed, 142 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
> new file mode 100644
> index 0000000000000..0664fe2b90db6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
> @@ -0,0 +1,142 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nuvoton,wpcm450-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton WPCM450 pin control and GPIO
> +
> +maintainers:
> +  - Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> +
> +properties:
> +  compatible:
> +    const: "nuvoton,wpcm450-pinctrl"

Don't need quotes.

> +
> +  reg:
> +    maxItems: 1
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  interrupts: true
> +
> +patternProperties:
> +  # There are two kinds of subnodes:
> +  # 1. a pinmux node configures pin muxing for a group of pins (e.g. rmii2)
> +  # 2. a pinctrl node configures properties of a single pin
> +  "^.*$":
> +    if:
> +      type: object
> +    then:

Don't do this hack for new bindings. Pick a node name pattern you can 
match on.

> +      allOf:
> +        - $ref: pincfg-node.yaml#
> +        - $ref: pinmux-node.yaml#
> +      properties:
> +        groups:
> +          description:
> +            One or more groups of pins to mux to a certain function
> +          minItems: 1
> +          items:
> +            enum: [ smb3, smb4, smb5, scs1, scs2, scs3, smb0, smb1, smb2, bsp,
> +                    hsp1, hsp2, r1err, r1md, rmii2, r2err, r2md, kbcc, dvo,
> +                    clko, smi, uinc, gspi, mben, xcs2, xcs1, sdio, sspi, fi0,
> +                    fi1, fi2, fi3, fi4, fi5, fi6, fi7, fi8, fi9, fi10, fi11,
> +                    fi12, fi13, fi14, fi15, pwm0, pwm1, pwm2, pwm3, pwm4, pwm5,
> +                    pwm6, pwm7, hg0, hg1, hg2, hg3, hg4, hg5, hg6, hg7 ]
> +        function:
> +          description:
> +            The function that a group of pins is muxed to
> +          enum: [ smb3, smb4, smb5, scs1, scs2, scs3, smb0, smb1, smb2, bsp,
> +                  hsp1, hsp2, r1err, r1md, rmii2, r2err, r2md, kbcc, dvo0,
> +                  dvo1, dvo2, dvo3, dvo4, dvo5, dvo6, dvo7, clko, smi, uinc,
> +                  gspi, mben, xcs2, xcs1, sdio, sspi, fi0, fi1, fi2, fi3, fi4,
> +                  fi5, fi6, fi7, fi8, fi9, fi10, fi11, fi12, fi13, fi14, fi15,
> +                  pwm0, pwm1, pwm2, pwm3, pwm4, pwm5, pwm6, pwm7, hg0, hg1,
> +                  hg2, hg3, hg4, hg5, hg6, hg7 ]
> +
> +        pins:
> +          description:
> +            A list of pins to configure in certain ways, such as enabling
> +            debouncing
> +          minItems: 1
> +          items:
> +            enum: [ gpio0, gpio1, gpio2, gpio3, gpio4, gpio5, gpio6, gpio7,
> +                    gpio8, gpio9, gpio10, gpio11, gpio12, gpio13, gpio14,
> +                    gpio15, gpio16, gpio17, gpio18, gpio19, gpio20, gpio21,
> +                    gpio22, gpio23, gpio24, gpio25, gpio26, gpio27, gpio28,
> +                    gpio29, gpio30, gpio31, gpio32, gpio33, gpio34, gpio35,
> +                    gpio36, gpio37, gpio38, gpio39, gpio40, gpio41, gpio42,
> +                    gpio43, gpio44, gpio45, gpio46, gpio47, gpio48, gpio49,
> +                    gpio50, gpio51, gpio52, gpio53, gpio54, gpio55, gpio56,
> +                    gpio57, gpio58, gpio59, gpio60, gpio61, gpio62, gpio63,
> +                    gpio64, gpio65, gpio66, gpio67, gpio68, gpio69, gpio70,
> +                    gpio71, gpio72, gpio73, gpio74, gpio75, gpio76, gpio77,
> +                    gpio78, gpio79, gpio80, gpio81, gpio82, gpio83, gpio84,
> +                    gpio85, gpio86, gpio87, gpio88, gpio89, gpio90, gpio91,
> +                    gpio92, gpio93, gpio94, gpio95, gpio96, gpio97, gpio98,
> +                    gpio99, gpio100, gpio101, gpio102, gpio103, gpio104,
> +                    gpio105, gpio106, gpio107, gpio108, gpio109, gpio110,
> +                    gpio111, gpio112, gpio113, gpio114, gpio115, gpio116,
> +                    gpio117, gpio118, gpio119, gpio120, gpio121, gpio122,
> +                    gpio123, gpio124, gpio125, gpio126, gpio127 ]
> +
> +        input-debounce: true
> +        phandle: true

Needing this should be fixed now.

> +
> +      dependencies:
> +        groups: [ function ]
> +        function: [ groups ]
> +
> +      additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - '#gpio-cells'
> +
> +additionalProperties: false
> +
> +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>;
> +      rmii2 {
> +        groups = "rmii2";
> +        function = "rmii2";
> +      };
> +
> +      pinctrl_uid: uid {
> +        pins = "gpio14";
> +        input-debounce = <1>;
> +      };
> +    };
> +
> +    gpio-keys {
> +      compatible = "gpio-keys";
> +      pinctrl-names = "default";
> +      pinctrl-0 = <&pinctrl_uid>;
> +
> +      uid {
> +        label = "UID";
> +        linux,code = <102>;
> +        gpios = <&pinctrl 14 GPIO_ACTIVE_HIGH>;
> +      };
> +    };
> --
> 2.30.2

On Tue, Jun 15, 2021 at 05:45:58PM -0600, Rob Herring wrote:
> On Wed, Jun 02, 2021 at 02:03:25PM +0200, Jonathan Neuschäfer wrote:
> > This binding is heavily based on the one for NPCM7xx, because the
> > hardware is similar. One notable difference is that there are no
> > sub-nodes for GPIO banks, because the GPIO registers are arranged
> > differently.
> > 
> > Certain pins support blink patterns in hardware. This is currently not
> > modelled in the DT binding.
> > 
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
[...]
> > +properties:
> > +  compatible:
> > +    const: "nuvoton,wpcm450-pinctrl"
> 
> Don't need quotes.

Ok, I'll remove them.

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 2

and I just noticed the inconsistency in quotes here. I'll fix it.

> > +
> > +  interrupts: true
> > +
> > +patternProperties:
> > +  # There are two kinds of subnodes:
> > +  # 1. a pinmux node configures pin muxing for a group of pins (e.g. rmii2)
> > +  # 2. a pinctrl node configures properties of a single pin
> > +  "^.*$":
> > +    if:
> > +      type: object
> > +    then:
> 
> Don't do this hack for new bindings. Pick a node name pattern you can 
> match on.

Ok.

> 
> > +      allOf:
> > +        - $ref: pincfg-node.yaml#
> > +        - $ref: pinmux-node.yaml#
> > +      properties:
[...]
> > +        phandle: true
> 
> Needing this should be fixed now.

Ok, I'll drop it.



Thanks,
Jonathan Neuschäfer