Re: [PATCH 2/4] dt-bindings: pinctrl: Document nuvoton ma35d1 pin control

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 11/10/2023 11:05, Jacky Huang wrote:
> From: Jacky Huang <ychuang3@xxxxxxxxxxx>
> 
> Add the dt-bindings header for nuvoton ma35d1 pinctrl, that gets shared
> between the pin control driver and pin configuration in the dts.
> 
> Add documentation to describe nuvoton ma35d1 pin control and GPIO.
> 
> Signed-off-by: Jacky Huang <ychuang3@xxxxxxxxxxx>
> ---
>  .../pinctrl/nuvoton,ma35d1-pinctrl.yaml       | 180 ++++++++++++++++++
>  include/dt-bindings/pinctrl/ma35d1-pinfunc.h  |  38 ++++
>  2 files changed, 218 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml
>  create mode 100644 include/dt-bindings/pinctrl/ma35d1-pinfunc.h
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml
> new file mode 100644
> index 000000000000..0ddedbad4b78
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nuvoton,ma35d1-pinctrl.yaml
> @@ -0,0 +1,180 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nuvoton,ma35d1-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nuvoton MA35D1 pin control and GPIO
> +
> +maintainers:
> +  - Shan-Chun Hung <schung@xxxxxxxxxxx>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - nuvoton,ma35d1-pinctrl
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 1
> +
> +  nuvoton,sys:
> +    description:
> +      phandle to the syscon node

sys is quite generic. Description explains nothing except duplicating
known information. Drop duplicated info and instead explain to what this
phandle points and how it is going to be used.


> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    items:
> +      maxItems: 1

So just phandle, not phandle-array, unless it is defined like this in
some other binding.

> +
> +  ranges: true
> +
> +allOf:
> +  - $ref: pinctrl.yaml#

allOf: goes after required: block.

> +
> +patternProperties:
> +  "gpio[a-n]@[0-9a-f]+$":

^gpio@[0-9a-f]+$":


> +    type: object
> +    additionalProperties: false
> +    properties:
> +

Drop blank line

> +      gpio-controller: true
> +
> +      '#gpio-cells':
> +        const: 2
> +
> +      reg:
> +        maxItems: 1
> +
> +      clocks:
> +        maxItems: 1
> +
> +      interrupt-controller: true
> +
> +      '#interrupt-cells':
> +        const: 2
> +
> +      interrupts:
> +        description:
> +          The interrupt outputs to sysirq.
> +        maxItems: 1
> +
> +    required:
> +      - reg
> +      - interrupts
> +      - interrupt-controller
> +      - '#interrupt-cells'
> +      - gpio-controller
> +      - '#gpio-cells'

Keep the same order as in list of properties.

> +
> +  "pcfg-[a-z0-9-.]+$":

Why using different naming than other Nuvoton SoCs? You also accept
"foobarpcfg-1", which does not look intentional.


> +    type: object
> +    description:
> +      A pinctrl node should contain at least one subnodes representing the
> +      pinctrl groups available on the machine. Each subnode will list the
> +      pins it needs, and how they should be configured, with regard to muxer
> +      configuration, pullups, drive strength, input enable/disable and input
> +      schmitt.
> +
> +    allOf:
> +      - $ref: pincfg-node.yaml#

missing additional/unevaluatedProperties: false.

> +
> +    properties:
> +      bias-disable: true

Why do you need this and other ones?

> +
> +      bias-pull-down: true
> +
> +      bias-pull-up: true
> +
> +      drive-strength:
> +        minimum: 0

0 mA? Is it really valid? Are you sure you used correct property?


> +        maximum: 7
> +
> +      input-enable: true
> +
> +      input-schmitt-enable: true
> +
> +      power-source:
> +        description:
> +          I/O voltage in millivolt.
> +        enum: [ 1800, 3300 ]

Missing units in property name. power-source also does not really
describe the property.

> +
> +additionalProperties:
> +  type: object
> +  additionalProperties:
> +    type: object

Wait, what? What are you describing here?

> +    properties:
> +      nuvoton,pin:
> +        description:
> +          Each entry consists of 4 parameters and represents the mux and config
> +          setting for one pin.
> +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +        minItems: 1
> +        items:
> +          items:
> +            - minimum: 0x80
> +              maximum: 0xec
> +              description:
> +                The pinctrl register offset in syscon registers.
> +            - minimum: 0
> +              maximum: 30
> +              description:
> +                The bit offset in the pinctrl register.
> +            - minimum: 0
> +              maximum: 15
> +              description:
> +                The multi-function pin value.
> +            - description:
> +                The phandle of a node contains the generic pinconfig options
> +                to use as described in pinctrl-bindings.txt.
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
> +    #include <dt-bindings/pinctrl/ma35d1-pinfunc.h>
> +
> +    pinctrl@40040000 {
> +        compatible = "nuvoton,ma35d1-pinctrl";
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +        nuvoton,sys = <&sys>;
> +        ranges = <0 0x40040000 0xc00>;
> +
> +        gpioa@40040000 {
> +                reg = <0x0 0x40>;
> +                interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> +                clocks = <&clk GPA_GATE>;
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +                interrupt-controller;
> +                #interrupt-cells = <2>;
> +        };
> +
> +        pcfg_default: pcfg-default {
> +                slew-rate = <0>;
> +                input-schmitt-disable;
> +                bias-disable;
> +                power-source = <3300>;
> +                drive-strength = <0>;

Really 0 mA?

Why this is so incomplete?

> +        };
> +    };
> +
> +    pinctrl {> +        uart13 {
> +                pinctrl_uart13: uart13grp {

According to your bindings this does not belong here.

> +                        nuvoton,pins =
> +                                <MA35_SYS_REG_GPH_H 24 2 &pcfg_default>,
> +                                <MA35_SYS_REG_GPH_H 28 2 &pcfg_default>;
> +                };
> +        };
> +    };
> +
> +    serial@407d0000 {

Drop node, not related at all.

> +        compatible = "nuvoton,ma35d1-uart";
> +        reg = <0x407d0000 0x100>;
> +        interrupts = <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&clk UART13_GATE>;
> +        pinctrl-0 = <&pinctrl_uart13>;
> +    };
> diff --git a/include/dt-bindings/pinctrl/ma35d1-pinfunc.h b/include/dt-bindings/pinctrl/ma35d1-pinfunc.h
> new file mode 100644
> index 000000000000..a2609d466dc9
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/ma35d1-pinfunc.h

Filename matching bindings. The same name.

> @@ -0,0 +1,38 @@
> +/* SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) */
> +/*
> + * Copyright (C) 2023 Nuvoton Technologies.
> + */
> +
> +#ifndef __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H
> +#define __DT_BINDINGS_PINCTRL_NUVOTON_MA35D1_H
> +
> +#define MA35_SYS_REG_GPA_L	0x80

Registry addresses are not suitable for bindings. There is also no need
to have REG address in the binding. Drop entire file.

Best regards,
Krzysztof