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

[Thierry Reding <thierry.reding@gmail.com>]

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

On Wed, May 31, 2023 at 10:45:53AM +0200, Krzysztof Kozlowski wrote:
> On 30/05/2023 15:36, Thierry Reding wrote:
> > From: Prathamesh Shete <pshete@nvidia.com>
> > 
> > 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@nvidia.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > 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@gmail.com>
> > +  - Jon Hunter <jonathanh@nvidia.com>
> > +
> > +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.

There isn't really a reasonable number for maxItems. Most of the pins in
a controller could technically be assigned to the same group. While that
doesn't happen in practice (most of the time there will be a single pin
per group), limiting this isn't correct and would only make this less
flexible. Are there any advantages to arbitrarily restricting maxItems?

> 
> > +          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@gmail.com>
> > +  - Jon Hunter <jonathanh@nvidia.com>
> > +
> > +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.

I'm fairly sure that the validation tools at least used to require a
"properties" property. Or it might have been because phandle would
otherwise be validated by the "additionalProperties" block below. In
any case, running the validation again it doesn't seem like that's
true anymore, so I'll drop this.

> > +
> > +    # 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?

If you look at the description of the common Tegra pinmux bindings in
pinctr/nvidia,tegra-pinmux-common.yaml, there's this comment in the
description:

  The name of each subnode is not important; all subnodes should be
  enumerated and processed purely based on their content.

Using additionalProperties is the best transcription of this because it
is the easiest way to accept any node name. So we purely validate based
on content.

And yes, this was already tested with the bindings from previous chip
iterations, which all use the same construct, and it works fine.

> 
> > +      $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.

I think these were originally included here because the generic pinmux
group schema (in nvidia,tegra-pinmux-common.yaml) lists the superset of
all properties that can go into a group. However, not all chips
generations support all of those properties.

But looking at this again, given that we have unevaluatedProperties in
the above, this shouldn't have any effect anyway.

I wonder now how we can properly validate for this. I suppose another
option would be to have a list of properties that aren't allowed and
mark them as ": false".

Thierry

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

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