Re: [PATCH v3 1/3] dt-bindings: pinctrl: Document Tegra234 pin controllers

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 30/05/2023 15:36, Thierry Reding wrote:
> From: Prathamesh Shete <pshete@xxxxxxxxxx>
> 
> Tegra234 contains two pin controllers. Document their compatible strings
> and describe the list of pins and functions that they provide.
> 
> Signed-off-by: Prathamesh Shete <pshete@xxxxxxxxxx>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
> Changes in v3:
> - split up into multiple files (suggested by Krzysztof)
> - do not permit underscore in pinmux node names
> - reword commit message
> 
>  .../pinctrl/nvidia,tegra234-pinmux-aon.yaml   |  61 ++++++++
>  .../nvidia,tegra234-pinmux-common.yaml        |  65 ++++++++
>  .../pinctrl/nvidia,tegra234-pinmux.yaml       | 141 ++++++++++++++++++
>  3 files changed, 267 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux-aon.yaml
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux-common.yaml
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux-aon.yaml b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux-aon.yaml
> new file mode 100644
> index 000000000000..9d7017a39408
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux-aon.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nvidia,tegra234-pinmux-aon.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +$ref: nvidia,tegra234-pinmux-common.yaml

Keep it before properties:. That's really unexpected order.


> +
> +title: NVIDIA Tegra234 AON Pinmux Controller
> +
> +maintainers:
> +  - Thierry Reding <thierry.reding@xxxxxxxxx>
> +  - Jon Hunter <jonathanh@xxxxxxxxxx>
> +
> +properties:
> +  compatible:
> +    const: nvidia,tegra234-pinmux-aon
> +
> +  reg: true

Drop this one.

> +
> +patternProperties:
> +  "^pinmux(-[a-z0-9-]+)?$":
> +    type: object
> +
> +    # pin groups
> +    additionalProperties:

Why do you need this? This binding looks odd...

> +      properties:
> +        nvidia,pins:

min/maxItems? If variable, put some reasonable numbers.

> +          items:
> +            enum: [ can0_dout_paa0, can0_din_paa1, can1_dout_paa2,
> +                    can1_din_paa3, can0_stb_paa4, can0_en_paa5,
> +                    soc_gpio49_paa6, can0_err_paa7, can1_stb_pbb0,
> +                    can1_en_pbb1, soc_gpio50_pbb2, can1_err_pbb3,
> +                    spi2_sck_pcc0, spi2_miso_pcc1, spi2_mosi_pcc2,
> +                    spi2_cs0_pcc3, touch_clk_pcc4, uart3_tx_pcc5,
> +                    uart3_rx_pcc6, gen2_i2c_scl_pcc7, gen2_i2c_sda_pdd0,
> +                    gen8_i2c_scl_pdd1, gen8_i2c_sda_pdd2,
> +                    sce_error_pee0, vcomp_alert_pee1,
> +                    ao_retention_n_pee2, batt_oc_pee3, power_on_pee4,
> +                    soc_gpio26_pee5, soc_gpio27_pee6, bootv_ctl_n_pee7,
> +                    hdmi_cec_pgg0,
> +                    # drive groups
> +                    drive_touch_clk_pcc4, drive_uart3_rx_pcc6,
> +                    drive_uart3_tx_pcc5, drive_gen8_i2c_sda_pdd2,
> +                    drive_gen8_i2c_scl_pdd1, drive_spi2_mosi_pcc2,
> +                    drive_gen2_i2c_scl_pcc7, drive_spi2_cs0_pcc3,
> +                    drive_gen2_i2c_sda_pdd0, drive_spi2_sck_pcc0,
> +                    drive_spi2_miso_pcc1, drive_can1_dout_paa2,
> +                    drive_can1_din_paa3, drive_can0_dout_paa0,
> +                    drive_can0_din_paa1, drive_can0_stb_paa4,
> +                    drive_can0_en_paa5, drive_soc_gpio49_paa6,
> +                    drive_can0_err_paa7, drive_can1_stb_pbb0,
> +                    drive_can1_en_pbb1, drive_soc_gpio50_pbb2,
> +                    drive_can1_err_pbb3, drive_sce_error_pee0,
> +                    drive_batt_oc_pee3, drive_bootv_ctl_n_pee7,
> +                    drive_power_on_pee4, drive_soc_gpio26_pee5,
> +                    drive_soc_gpio27_pee6, drive_ao_retention_n_pee2,
> +                    drive_vcomp_alert_pee1, drive_hdmi_cec_pgg0 ]
> +
> +additionalProperties: false

unevaluatedProperties: false

> +...
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux-common.yaml b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux-common.yaml
> new file mode 100644
> index 000000000000..a09d050b7d37
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux-common.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nvidia,tegra234-pinmux-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVIDIA Tegra234 Pinmux Controller
> +
> +maintainers:
> +  - Thierry Reding <thierry.reding@xxxxxxxxx>
> +  - Jon Hunter <jonathanh@xxxxxxxxxx>
> +
> +properties:
> +  compatible: true

Drop, won't be needed with additionalProps true.

> +
> +  reg:
> +    items:
> +      - description: pinmux registers
> +
> +patternProperties:
> +  "^pinmux(-[a-z0-9-]+)?$":
> +    type: object
> +    properties:
> +      phandle: true

No clue what's that but if you need it, something is broken. Remove it
and we need to fix the root cause.


> +
> +    # pin groups
> +    additionalProperties:

I still don't get what you want to express here. We usually list the
children with patternProperties for specific pattern. Your approach
could work too, but did you really check it enforces proper type/ref?
That it really works?

> +      $ref: nvidia,tegra-pinmux-common.yaml
> +      unevaluatedProperties: false
> +      properties:
> +        nvidia,function:
> +          enum: [ gp, uartc, i2c8, spi2, i2c2, can1, can0, rsvd0, eth0, eth2,
> +                  eth1, dp, eth3, i2c4, i2c7, i2c9, eqos, pe2, pe1, pe0, pe3,
> +                  pe4, pe5, pe6, pe7, pe8, pe9, pe10, qspi0, qspi1, qpsi,
> +                  sdmmc1, sce, soc, gpio, hdmi, ufs0, spi3, spi1, uartb, uarte,
> +                  usb, extperiph2, extperiph1, i2c3, vi0, i2c5, uarta, uartd,
> +                  i2c1, i2s4, i2s6, aud, spi5, touch, uartj, rsvd1, wdt, tsc,
> +                  dmic3, led, vi0_alt, i2s5, nv, extperiph3, extperiph4, spi4,
> +                  ccla, i2s1, i2s2, i2s3, i2s8, rsvd2, dmic5, dca, displayb,
> +                  displaya, vi1, dcb, dmic1, dmic4, i2s7, dmic2, dspk0, rsvd3,
> +                  tsc_alt, istctrl, vi1_alt, dspk1, igpu ]
> +
> +        nvidia,pins:
> +          description: An array of strings. Each string contains the name
> +            of a pin or group. Valid values for these names are listed
> +            below.

Drop, not needed.

> +
> +        nvidia,pull: true
> +        nvidia,tristate: true
> +        nvidia,schmitt: true
> +        nvidia,enable-input: true
> +        nvidia,open-drain: true
> +        nvidia,lock: true
> +        nvidia,drive-type: true
> +        nvidia,io-hv: true

Drop all these.

> +
> +      required:
> +        - nvidia,pins
> +
> +additionalProperties: false

We keep it "true" for common schema and then the users of this binding
use unevaluatedProperties: false.


> +
> +required:
> +  - compatible
> +  - reg

Keep it before additionalProperites:.

> +...


Best regards,
Krzysztof