[1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc

Message ID	20230207115617.12088-1-pshete@nvidia.com
State	Changes Requested, archived
Headers	show Return-Path: <devicetree-owner@vger.kernel.org> Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates 216.228.118.233 as permitted sender) receiver=protection.outlook.com; client-ip=216.228.118.233; helo=mail.nvidia.com; pr=C From: Prathamesh Shete <pshete@nvidia.com> To: <thierry.reding@gmail.com>, <jonathanh@nvidia.com>, <linus.walleij@linaro.org>, <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>, <devicetree@vger.kernel.org>, <linux-tegra@vger.kernel.org>, <linux-gpio@vger.kernel.org> CC: <smangipudi@nvidia.com>, <pshete@nvidia.com> Subject: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc Date: Tue, 7 Feb 2023 17:26:15 +0530 Message-ID: <20230207115617.12088-1-pshete@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk
Series	[1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc \| expand [1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc

Context	Check	Description
robh/checkpatch	success
robh/patch-applied	success
robh/dt-meta-schema	fail	build log

Prathamesh Shete Feb. 7, 2023, 11:56 a.m. UTC

Add DT binding doc for Tegra234 pinmux driver.

Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
---
 .../pinctrl/nvidia,tegra234-pinmux.yaml       | 232 ++++++++++++++++++
 1 file changed, 232 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml

Rob Herring Feb. 7, 2023, 2:37 p.m. UTC | #1

On Tue, 07 Feb 2023 17:26:15 +0530, Prathamesh Shete wrote:
> Add DT binding doc for Tegra234 pinmux driver.
> 
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  .../pinctrl/nvidia,tegra234-pinmux.yaml       | 232 ++++++++++++++++++
>  1 file changed, 232 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.example.dtb: pinmux@2430000: pinmux-pex-rst-c5-out:pex_rst:nvidia,pins:0: 'pex_l5_rst_n_pgg1' is not one of ['dap6_sclk_pa0', 'dap6_dout_pa1', 'dap6_din_pa2', 'dap6_fs_pa3', 'dap4_sclk_pa4', 'dap4_dout_pa5', 'dap4_din_pa6', 'dap4_fs_pa7', 'soc_gpio08_pb0', 'qspi0_sck_pc0', 'qspi0_cs_n_pc1', 'qspi0_io0_pc2', 'qspi0_io1_pc3', 'qspi0_io2_pc4', 'qspi0_io3_pc5', 'qspi1_sck_pc6', 'qspi1_cs_n_pc7', 'qspi1_io0_pd0', 'qspi1_io1_pd1', 'qspi1_io2_pd2', 'qspi1_io3_pd3', 'eqos_txc_pe0', 'eqos_td0_pe1', 'eqos_td1_pe2', 'eqos_td2_pe3', 'eqos_td3_pe4', 'eqos_tx_ctl_pe5', 'eqos_rd0_pe6', 'eqos_rd1_pe7', 'eqos_rd2_pf0', 'eqos_rd3_pf1', 'eqos_rx_ctl_pf2', 'eqos_rxc_pf3', 'eqos_sma_mdio_pf4', 'eqos_sma_mdc_pf5', 'soc_gpio13_pg0', 'soc_gpio14_pg1', 'soc_gpio15_pg2', 'soc_gpio16_pg3', 'soc_gpio17_pg4', 'soc_gpio18_pg5', 'soc_gpio19_pg6', 'soc_gpio20_pg7', 'soc_gpio21_ph0', 'soc_gpio22_ph1', 'soc_gpio06_p
 h2', 'uart4_tx_ph3', 'uart4_rx_ph4', 'uart4_rts_ph5', 'uart4_cts_ph6', 'soc_gpio41_ph7', 'soc_gpio42_pi0', 'soc_gpio43_pi1', 'soc_gpio44_pi2', 'gen1_i2c_scl_pi3', 'gen1_i2c_sda_pi4', 'cpu_pwr_req_pi5', 'soc_gpio07_pi6', 'sdmmc1_clk_pj0', 'sdmmc1_cmd_pj1', 'sdmmc1_dat0_pj2', 'sdmmc1_dat1_pj3', 'sdmmc1_dat2_pj4', 'sdmmc1_dat3_pj5', 'pex_l0_clkreq_n_pk0', 'pex_l0_rst_n_pk1', 'pex_l1_clkreq_n_pk2', 'pex_l1_rst_n_pk3', 'pex_l2_clkreq_n_pk4', 'pex_l2_rst_n_pk5', 'pex_l3_clkreq_n_pk6', 'pex_l3_rst_n_pk7', 'pex_l4_clkreq_n_pl0', 'pex_l4_rst_n_pl1', 'pex_wake_n_pl2', 'soc_gpio34_pl3', 'dp_aux_ch0_hpd_pm0', 'dp_aux_ch1_hpd_pm1', 'dp_aux_ch2_hpd_pm2', 'dp_aux_ch3_hpd_pm3', 'soc_gpio55_pm4', 'soc_gpio36_pm5', 'soc_gpio53_pm6', 'soc_gpio38_pm7', 'dp_aux_ch3_n_pn0', 'soc_gpio39_pn1', 'soc_gpio40_pn2', 'dp_aux_ch1_p_pn3', 'dp_aux_ch1_n_pn4', 'dp_aux_ch2_p_pn5', 'dp_aux_ch2_n_pn6', 'dp_aux_ch3_p_pn7', 'extperiph1_clk_pp0', 'extperiph2_clk_pp1', 'cam_i2c_scl_pp2', 'cam_i2c_sda_pp3', 'soc_gpio23_pp4'
 , 'soc_gpio24_pp5', 'soc_gpio25_pp6', 'pwr_i2c_scl_pp7', 'pwr_i2c_sda_pq0', 'soc_gpio28_pq1', 'soc_gpio29_pq2', 'soc_gpio30_pq3', 'soc_gpio31_pq4', 'soc_gpio32_pq5', 'soc_gpio33_pq6', 'soc_gpio35_pq7', 'soc_gpio37_pr0', 'soc_gpio56_pr1', 'uart1_tx_pr2', 'uart1_rx_pr3', 'uart1_rts_pr4', 'uart1_cts_pr5', 'soc_gpio61_pw0', 'soc_gpio62_pw1', 'gpu_pwr_req_px0', 'cv_pwr_req_px1', 'gp_pwm2_px2', 'gp_pwm3_px3', 'uart2_tx_px4', 'uart2_rx_px5', 'uart2_rts_px6', 'uart2_cts_px7', 'spi3_sck_py0', 'spi3_miso_py1', 'spi3_mosi_py2', 'spi3_cs0_py3', 'spi3_cs1_py4', 'uart5_tx_py5', 'uart5_rx_py6', 'uart5_rts_py7', 'uart5_cts_pz0', 'usb_vbus_en0_pz1', 'usb_vbus_en1_pz2', 'spi1_sck_pz3', 'spi1_miso_pz4', 'spi1_mosi_pz5', 'spi1_cs0_pz6', 'spi1_cs1_pz7', 'spi5_sck_pac0', 'spi5_miso_pac1', 'spi5_mosi_pac2', 'spi5_cs0_pac3', 'soc_gpio57_pac4', 'soc_gpio58_pac5', 'soc_gpio59_pac6', 'soc_gpio60_pac7', 'soc_gpio45_pad0', 'soc_gpio46_pad1', 'soc_gpio47_pad2', 'soc_gpio48_pad3', 'ufs0_ref_clk_pae0', 'ufs0_rst_n
 _pae1', 'pex_l5_clkreq_n_paf0', 'pex_l5_rst_n_paf1', 'pex_l6_clkreq_n_paf2', 'pex_l6_rst_n_paf3', 'pex_l7_clkreq_n_pag0', 'pex_l7_rst_n_pag1', 'pex_l8_clkreq_n_pag2', 'pex_l8_rst_n_pag3', 'pex_l9_clkreq_n_pag4', 'pex_l9_rst_n_pag5', 'pex_l10_clkreq_n_pag6', 'pex_l10_rst_n_pag7', 'sdmmc1_comp', 'eqos_comp', 'qspi_comp', 'drive_soc_gpio08_pb0', 'drive_soc_gpio36_pm5', 'drive_soc_gpio53_pm6', 'drive_soc_gpio55_pm4', 'drive_soc_gpio38_pm7', 'drive_soc_gpio39_pn1', 'drive_soc_gpio40_pn2', 'drive_dp_aux_ch0_hpd_pm0', 'drive_dp_aux_ch1_hpd_pm1', 'drive_dp_aux_ch2_hpd_pm2', 'drive_dp_aux_ch3_hpd_pm3', 'drive_dp_aux_ch1_p_pn3', 'drive_dp_aux_ch1_n_pn4', 'drive_dp_aux_ch2_p_pn5', 'drive_dp_aux_ch2_n_pn6', 'drive_dp_aux_ch3_p_pn7', 'drive_dp_aux_ch3_n_pn0', 'drive_pex_l2_clkreq_n_pk4', 'drive_pex_wake_n_pl2', 'drive_pex_l1_clkreq_n_pk2', 'drive_pex_l1_rst_n_pk3', 'drive_pex_l0_clkreq_n_pk0', 'drive_pex_l0_rst_n_pk1', 'drive_pex_l2_rst_n_pk5', 'drive_pex_l3_clkreq_n_pk6', 'drive_pex_l3_rst_n_pk
 7', 'drive_pex_l4_clkreq_n_pl0', 'drive_pex_l4_rst_n_pl1', 'drive_soc_gpio34_pl3', 'drive_pex_l5_clkreq_n_paf0', 'drive_pex_l5_rst_n_paf1', 'drive_pex_l6_clkreq_n_paf2', 'drive_pex_l6_rst_n_paf3', 'drive_pex_l10_clkreq_n_pag6', 'drive_pex_l10_rst_n_pag7', 'drive_pex_l7_clkreq_n_pag0', 'drive_pex_l7_rst_n_pag1', 'drive_pex_l8_clkreq_n_pag2', 'drive_pex_l8_rst_n_pag3', 'drive_pex_l9_clkreq_n_pag4', 'drive_pex_l9_rst_n_pag5', 'drive_sdmmc1_clk_pj0', 'drive_sdmmc1_cmd_pj1', 'drive_sdmmc1_dat3_pj5', 'drive_sdmmc1_dat2_pj4', 'drive_sdmmc1_dat1_pj3', 'drive_sdmmc1_dat0_pj2']
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230207115617.12088-1-pshete@nvidia.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Krzysztof Kozlowski Feb. 7, 2023, 3:33 p.m. UTC | #2

On 07/02/2023 12:56, Prathamesh Shete wrote:
> Add DT binding doc for Tegra234 pinmux driver.

Subject: drop second/last, redundant "DT binding doc". The "dt-bindings"
prefix is already stating that these are bindings. Which basically means
your subject is quite empty...

> 
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  .../pinctrl/nvidia,tegra234-pinmux.yaml       | 232 ++++++++++++++++++
>  1 file changed, 232 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> new file mode 100644
> index 000000000000..56b8d364c605
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> @@ -0,0 +1,232 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nvidia,tegra234-pinmux.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:
> +    enum:
> +      - nvidia,tegra234-pinmux
> +      - nvidia,tegra234-pinmux-aon
> +
> +  reg:
> +    items:
> +      - description: pinmux registers
> +
> +patternProperties:
> +  "^pinmux(-[a-z0-9-_]+)?$":

Underscore is not a valid character

> +    type: object
> +    properties:
> +      phandle: true
> +
> +    # pin groups
> +    additionalProperties:
> +      $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,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
> +
> +      required:
> +        - nvidia,pins
> +
> +additionalProperties: false
> +
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: nvidia,tegra234-pinmux
> +    then:
> +      patternProperties:
> +        "^pinmux(-[a-z0-9-_]+)?$":

Underscore is not a valid character

> +          type: object
> +          additionalProperties:
> +            properties:
> +              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.

Define properties in top level, which points to the complexity of your
if-else, thus probably this should be split into two bindings. Dunno,
your other bindings repeat this pattern :(

> +
> +                items:
> +                  enum: [ dap6_sclk_pa0, dap6_dout_pa1, dap6_din_pa2,
> +                          dap6_fs_pa3, dap4_sclk_pa4, dap4_dout_pa5,
> +                          dap4_din_pa6, dap4_fs_pa7, soc_gpio08_pb0,
> +                          qspi0_sck_pc0, qspi0_cs_n_pc1,
> +                          qspi0_io0_pc2, qspi0_io1_pc3, qspi0_io2_pc4,
> +                          qspi0_io3_pc5, qspi1_sck_pc6, qspi1_cs_n_pc7,
> +                          qspi1_io0_pd0, qspi1_io1_pd1, qspi1_io2_pd2,
> +                          qspi1_io3_pd3, eqos_txc_pe0, eqos_td0_pe1,
> +                          eqos_td1_pe2, eqos_td2_pe3, eqos_td3_pe4,
> +                          eqos_tx_ctl_pe5, eqos_rd0_pe6, eqos_rd1_pe7,
> +                          eqos_rd2_pf0, eqos_rd3_pf1, eqos_rx_ctl_pf2,
> +                          eqos_rxc_pf3, eqos_sma_mdio_pf4, eqos_sma_mdc_pf5,
> +                          soc_gpio13_pg0, soc_gpio14_pg1, soc_gpio15_pg2,
> +                          soc_gpio16_pg3, soc_gpio17_pg4, soc_gpio18_pg5,
> +                          soc_gpio19_pg6, soc_gpio20_pg7, soc_gpio21_ph0,
> +                          soc_gpio22_ph1, soc_gpio06_ph2, uart4_tx_ph3,
> +                          uart4_rx_ph4, uart4_rts_ph5, uart4_cts_ph6,
> +                          soc_gpio41_ph7, soc_gpio42_pi0, soc_gpio43_pi1,
> +                          soc_gpio44_pi2, gen1_i2c_scl_pi3, gen1_i2c_sda_pi4,
> +                          cpu_pwr_req_pi5, soc_gpio07_pi6,
> +                          sdmmc1_clk_pj0, sdmmc1_cmd_pj1, sdmmc1_dat0_pj2,
> +                          sdmmc1_dat1_pj3, sdmmc1_dat2_pj4, sdmmc1_dat3_pj5,
> +                          pex_l0_clkreq_n_pk0, pex_l0_rst_n_pk1,
> +                          pex_l1_clkreq_n_pk2, pex_l1_rst_n_pk3,
> +                          pex_l2_clkreq_n_pk4, pex_l2_rst_n_pk5,
> +                          pex_l3_clkreq_n_pk6, pex_l3_rst_n_pk7,
> +                          pex_l4_clkreq_n_pl0, pex_l4_rst_n_pl1,
> +                          pex_wake_n_pl2, soc_gpio34_pl3, dp_aux_ch0_hpd_pm0,
> +                          dp_aux_ch1_hpd_pm1, dp_aux_ch2_hpd_pm2,
> +                          dp_aux_ch3_hpd_pm3, soc_gpio55_pm4, soc_gpio36_pm5,
> +                          soc_gpio53_pm6, soc_gpio38_pm7, dp_aux_ch3_n_pn0,
> +                          soc_gpio39_pn1, soc_gpio40_pn2, dp_aux_ch1_p_pn3,
> +                          dp_aux_ch1_n_pn4, dp_aux_ch2_p_pn5, dp_aux_ch2_n_pn6,
> +                          dp_aux_ch3_p_pn7, extperiph1_clk_pp0,
> +                          extperiph2_clk_pp1, cam_i2c_scl_pp2, cam_i2c_sda_pp3,
> +                          soc_gpio23_pp4, soc_gpio24_pp5, soc_gpio25_pp6,
> +                          pwr_i2c_scl_pp7, pwr_i2c_sda_pq0, soc_gpio28_pq1,
> +                          soc_gpio29_pq2, soc_gpio30_pq3, soc_gpio31_pq4,
> +                          soc_gpio32_pq5, soc_gpio33_pq6, soc_gpio35_pq7,
> +                          soc_gpio37_pr0, soc_gpio56_pr1, uart1_tx_pr2,
> +                          uart1_rx_pr3, uart1_rts_pr4, uart1_cts_pr5,
> +                          soc_gpio61_pw0, soc_gpio62_pw1, gpu_pwr_req_px0, cv_pwr_req_px1,
> +                          gp_pwm2_px2, gp_pwm3_px3, uart2_tx_px4, uart2_rx_px5,
> +                          uart2_rts_px6, uart2_cts_px7, spi3_sck_py0,
> +                          spi3_miso_py1, spi3_mosi_py2, spi3_cs0_py3,
> +                          spi3_cs1_py4, uart5_tx_py5, uart5_rx_py6,
> +                          uart5_rts_py7, uart5_cts_pz0, usb_vbus_en0_pz1,
> +                          usb_vbus_en1_pz2, spi1_sck_pz3, spi1_miso_pz4,
> +                          spi1_mosi_pz5, spi1_cs0_pz6, spi1_cs1_pz7,
> +                          spi5_sck_pac0, spi5_miso_pac1, spi5_mosi_pac2,
> +                          spi5_cs0_pac3, soc_gpio57_pac4, soc_gpio58_pac5,
> +                          soc_gpio59_pac6, soc_gpio60_pac7, soc_gpio45_pad0,
> +                          soc_gpio46_pad1, soc_gpio47_pad2, soc_gpio48_pad3,
> +                          ufs0_ref_clk_pae0, ufs0_rst_n_pae1,
> +                          pex_l5_clkreq_n_paf0, pex_l5_rst_n_paf1,
> +                          pex_l6_clkreq_n_paf2, pex_l6_rst_n_paf3,
> +                          pex_l7_clkreq_n_pag0, pex_l7_rst_n_pag1,
> +                          pex_l8_clkreq_n_pag2, pex_l8_rst_n_pag3,
> +                          pex_l9_clkreq_n_pag4, pex_l9_rst_n_pag5,
> +                          pex_l10_clkreq_n_pag6, pex_l10_rst_n_pag7,
> +                          sdmmc1_comp, eqos_comp, qspi_comp,
> +                          # drive groups
> +                          drive_soc_gpio08_pb0, drive_soc_gpio36_pm5,
> +                          drive_soc_gpio53_pm6, drive_soc_gpio55_pm4,
> +                          drive_soc_gpio38_pm7, drive_soc_gpio39_pn1,
> +                          drive_soc_gpio40_pn2, drive_dp_aux_ch0_hpd_pm0,
> +                          drive_dp_aux_ch1_hpd_pm1, drive_dp_aux_ch2_hpd_pm2,
> +                          drive_dp_aux_ch3_hpd_pm3, drive_dp_aux_ch1_p_pn3,
> +                          drive_dp_aux_ch1_n_pn4, drive_dp_aux_ch2_p_pn5,
> +                          drive_dp_aux_ch2_n_pn6, drive_dp_aux_ch3_p_pn7,
> +                          drive_dp_aux_ch3_n_pn0, drive_pex_l2_clkreq_n_pk4,
> +                          drive_pex_wake_n_pl2, drive_pex_l1_clkreq_n_pk2,
> +                          drive_pex_l1_rst_n_pk3, drive_pex_l0_clkreq_n_pk0,
> +                          drive_pex_l0_rst_n_pk1, drive_pex_l2_rst_n_pk5,
> +                          drive_pex_l3_clkreq_n_pk6, drive_pex_l3_rst_n_pk7,
> +                          drive_pex_l4_clkreq_n_pl0, drive_pex_l4_rst_n_pl1,
> +                          drive_soc_gpio34_pl3, drive_pex_l5_clkreq_n_paf0,
> +                          drive_pex_l5_rst_n_paf1, drive_pex_l6_clkreq_n_paf2,
> +                          drive_pex_l6_rst_n_paf3, drive_pex_l10_clkreq_n_pag6,
> +                          drive_pex_l10_rst_n_pag7, drive_pex_l7_clkreq_n_pag0,
> +                          drive_pex_l7_rst_n_pag1, drive_pex_l8_clkreq_n_pag2,
> +                          drive_pex_l8_rst_n_pag3, drive_pex_l9_clkreq_n_pag4,
> +                          drive_pex_l9_rst_n_pag5, drive_sdmmc1_clk_pj0,
> +                          drive_sdmmc1_cmd_pj1, drive_sdmmc1_dat3_pj5,
> +                          drive_sdmmc1_dat2_pj4, drive_sdmmc1_dat1_pj3,
> +                          drive_sdmmc1_dat0_pj2 ]
> +
> +  - if:
> +      properties:
> +        compatible:
> +          const: nvidia,tegra234-pinmux-aon
> +    then:
> +      patternProperties:
> +        "^pinmux(-[a-z0-9-_]+)?$":

No underscores

> +          type: object
> +          additionalProperties:
> +            properties:
> +              nvidia,pins:
> +                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 ]
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    #include <dt-bindings/pinctrl/pinctrl-tegra.h>
> +
> +    pinmux@2430000 {
> +        compatible = "nvidia,tegra234-pinmux";
> +        reg = <0x2430000 0x17000>;
> +
> +        pinctrl-names = "pex_rst";
> +        pinctrl-0 = <&pex_rst_c5_out_state>;
> +
> +        pex_rst_c5_out_state: pinmux-pex-rst-c5-out {
> +            pex_rst {

Underscores are not valid in node names.


Best regards,
Krzysztof

Krzysztof Kozlowski Feb. 7, 2023, 3:33 p.m. UTC | #3

On 07/02/2023 12:56, Prathamesh Shete wrote:
> This change adds pinmux node for Tegra234.
> 
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> ---
>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> index eaf05ee9acd1..c91b88bc56d1 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> @@ -701,6 +701,13 @@
>  			interrupt-controller;
>  			#gpio-cells = <2>;
>  			gpio-controller;
> +			gpio-ranges = <&pinmux 0 0 164>;
> +		};
> +
> +		pinmux: pinmux@2430000 {
> +			compatible = "nvidia,tegra234-pinmux";
> +			reg = <0x2430000 0x19100>;
> +			status = "okay";

Why? Anything disabled it?

>  		};
>  
>  		mc: memory-controller@2c00000 {
> @@ -1664,6 +1671,13 @@
>  			interrupt-controller;
>  			#gpio-cells = <2>;
>  			gpio-controller;
> +			gpio-range = <&pinmux_aon 0 0 32>;
> +		};
> +
> +		pinmux_aon: pinmux@c300000 {
> +			compatible = "nvidia,tegra234-pinmux-aon";
> +			reg = <0xc300000 0x4000>;
> +			status = "okay";

Also why?

>  		};
>  
>  		pwm4: pwm@c340000 {

Best regards,
Krzysztof

Thierry Reding Feb. 8, 2023, 11 a.m. UTC | #4

On Tue, Feb 07, 2023 at 04:33:42PM +0100, Krzysztof Kozlowski wrote:
> On 07/02/2023 12:56, Prathamesh Shete wrote:
> > This change adds pinmux node for Tegra234.
> > 
> > Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> > ---
> >  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> > index eaf05ee9acd1..c91b88bc56d1 100644
> > --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> > +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
> > @@ -701,6 +701,13 @@
> >  			interrupt-controller;
> >  			#gpio-cells = <2>;
> >  			gpio-controller;
> > +			gpio-ranges = <&pinmux 0 0 164>;
> > +		};
> > +
> > +		pinmux: pinmux@2430000 {
> > +			compatible = "nvidia,tegra234-pinmux";
> > +			reg = <0x2430000 0x19100>;
> > +			status = "okay";
> 
> Why? Anything disabled it?
> 
> >  		};
> >  
> >  		mc: memory-controller@2c00000 {
> > @@ -1664,6 +1671,13 @@
> >  			interrupt-controller;
> >  			#gpio-cells = <2>;
> >  			gpio-controller;
> > +			gpio-range = <&pinmux_aon 0 0 32>;
> > +		};
> > +
> > +		pinmux_aon: pinmux@c300000 {
> > +			compatible = "nvidia,tegra234-pinmux-aon";
> > +			reg = <0xc300000 0x4000>;
> > +			status = "okay";
> 
> Also why?

These are probably copy-pasted from Tegra194 where these snuck in. I can
drop those when applying. I'll also prepare a patch to drop these from
the tegra194.dtsi.

I wonder if there's a good way to detect these. We'd have to run checks
on the DT source files, so that's a bit difficult. I do have an
experimental script that tries to capture some common pitfalls on
sources but it's quite ugly and slow, but I guess I could add something
like this. But perhaps there are better ways?

Thierry

Thierry Reding Feb. 8, 2023, 11:24 a.m. UTC | #5

On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:
> On 07/02/2023 12:56, Prathamesh Shete wrote:
> > Add DT binding doc for Tegra234 pinmux driver.
> 
> Subject: drop second/last, redundant "DT binding doc". The "dt-bindings"
> prefix is already stating that these are bindings. Which basically means
> your subject is quite empty...
> 
> > 
> > Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> > ---
> >  .../pinctrl/nvidia,tegra234-pinmux.yaml       | 232 ++++++++++++++++++
> >  1 file changed, 232 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> > new file mode 100644
> > index 000000000000..56b8d364c605
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> > @@ -0,0 +1,232 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/nvidia,tegra234-pinmux.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:
> > +    enum:
> > +      - nvidia,tegra234-pinmux
> > +      - nvidia,tegra234-pinmux-aon
> > +
> > +  reg:
> > +    items:
> > +      - description: pinmux registers
> > +
> > +patternProperties:
> > +  "^pinmux(-[a-z0-9-_]+)?$":
> 
> Underscore is not a valid character

This is again the same as for Tegra194. I don't think we use underscores
in any pinmux node names at the moment, so we should be able to remove
that one.

> 
> > +    type: object
> > +    properties:
> > +      phandle: true
> > +
> > +    # pin groups
> > +    additionalProperties:
> > +      $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,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
> > +
> > +      required:
> > +        - nvidia,pins
> > +
> > +additionalProperties: false
> > +
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: nvidia,tegra234-pinmux
> > +    then:
> > +      patternProperties:
> > +        "^pinmux(-[a-z0-9-_]+)?$":
> 
> Underscore is not a valid character
> 
> > +          type: object
> > +          additionalProperties:
> > +            properties:
> > +              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.
> 
> Define properties in top level, which points to the complexity of your
> if-else, thus probably this should be split into two bindings. Dunno,
> your other bindings repeat this pattern :(

The property itself is already defined in the common schema found in
nvidia,tegra-pinmux-common.yaml and we're overriding this here for each
instance since each has its own set of pins.

This was a compromise to avoid too many bindings. Originally I attempted
to roll all Tegra pinctrl bindings into a single dt-schema, but that
turned out truly horrible =) Splitting this into per-SoC bindings is
already causing a lot of duplication in these files, though splitting
off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit with
that already. Splitting this into per-instance bindings would
effectively duplicate everything but the pin array here, so we kind of
settled on this compromise for Tegra194.

We're taking a bit of a shortcut here already, since technically not all
pins support all the functions listed above. On the other hand, fully
accurately describing per-pin functions would make this a total mess, so
again, we use this simplified representation as a compromise.

> 
> > +
> > +                items:
> > +                  enum: [ dap6_sclk_pa0, dap6_dout_pa1, dap6_din_pa2,
> > +                          dap6_fs_pa3, dap4_sclk_pa4, dap4_dout_pa5,
> > +                          dap4_din_pa6, dap4_fs_pa7, soc_gpio08_pb0,
> > +                          qspi0_sck_pc0, qspi0_cs_n_pc1,
> > +                          qspi0_io0_pc2, qspi0_io1_pc3, qspi0_io2_pc4,
> > +                          qspi0_io3_pc5, qspi1_sck_pc6, qspi1_cs_n_pc7,
> > +                          qspi1_io0_pd0, qspi1_io1_pd1, qspi1_io2_pd2,
> > +                          qspi1_io3_pd3, eqos_txc_pe0, eqos_td0_pe1,
> > +                          eqos_td1_pe2, eqos_td2_pe3, eqos_td3_pe4,
> > +                          eqos_tx_ctl_pe5, eqos_rd0_pe6, eqos_rd1_pe7,
> > +                          eqos_rd2_pf0, eqos_rd3_pf1, eqos_rx_ctl_pf2,
> > +                          eqos_rxc_pf3, eqos_sma_mdio_pf4, eqos_sma_mdc_pf5,
> > +                          soc_gpio13_pg0, soc_gpio14_pg1, soc_gpio15_pg2,
> > +                          soc_gpio16_pg3, soc_gpio17_pg4, soc_gpio18_pg5,
> > +                          soc_gpio19_pg6, soc_gpio20_pg7, soc_gpio21_ph0,
> > +                          soc_gpio22_ph1, soc_gpio06_ph2, uart4_tx_ph3,
> > +                          uart4_rx_ph4, uart4_rts_ph5, uart4_cts_ph6,
> > +                          soc_gpio41_ph7, soc_gpio42_pi0, soc_gpio43_pi1,
> > +                          soc_gpio44_pi2, gen1_i2c_scl_pi3, gen1_i2c_sda_pi4,
> > +                          cpu_pwr_req_pi5, soc_gpio07_pi6,
> > +                          sdmmc1_clk_pj0, sdmmc1_cmd_pj1, sdmmc1_dat0_pj2,
> > +                          sdmmc1_dat1_pj3, sdmmc1_dat2_pj4, sdmmc1_dat3_pj5,
> > +                          pex_l0_clkreq_n_pk0, pex_l0_rst_n_pk1,
> > +                          pex_l1_clkreq_n_pk2, pex_l1_rst_n_pk3,
> > +                          pex_l2_clkreq_n_pk4, pex_l2_rst_n_pk5,
> > +                          pex_l3_clkreq_n_pk6, pex_l3_rst_n_pk7,
> > +                          pex_l4_clkreq_n_pl0, pex_l4_rst_n_pl1,
> > +                          pex_wake_n_pl2, soc_gpio34_pl3, dp_aux_ch0_hpd_pm0,
> > +                          dp_aux_ch1_hpd_pm1, dp_aux_ch2_hpd_pm2,
> > +                          dp_aux_ch3_hpd_pm3, soc_gpio55_pm4, soc_gpio36_pm5,
> > +                          soc_gpio53_pm6, soc_gpio38_pm7, dp_aux_ch3_n_pn0,
> > +                          soc_gpio39_pn1, soc_gpio40_pn2, dp_aux_ch1_p_pn3,
> > +                          dp_aux_ch1_n_pn4, dp_aux_ch2_p_pn5, dp_aux_ch2_n_pn6,
> > +                          dp_aux_ch3_p_pn7, extperiph1_clk_pp0,
> > +                          extperiph2_clk_pp1, cam_i2c_scl_pp2, cam_i2c_sda_pp3,
> > +                          soc_gpio23_pp4, soc_gpio24_pp5, soc_gpio25_pp6,
> > +                          pwr_i2c_scl_pp7, pwr_i2c_sda_pq0, soc_gpio28_pq1,
> > +                          soc_gpio29_pq2, soc_gpio30_pq3, soc_gpio31_pq4,
> > +                          soc_gpio32_pq5, soc_gpio33_pq6, soc_gpio35_pq7,
> > +                          soc_gpio37_pr0, soc_gpio56_pr1, uart1_tx_pr2,
> > +                          uart1_rx_pr3, uart1_rts_pr4, uart1_cts_pr5,
> > +                          soc_gpio61_pw0, soc_gpio62_pw1, gpu_pwr_req_px0, cv_pwr_req_px1,
> > +                          gp_pwm2_px2, gp_pwm3_px3, uart2_tx_px4, uart2_rx_px5,
> > +                          uart2_rts_px6, uart2_cts_px7, spi3_sck_py0,
> > +                          spi3_miso_py1, spi3_mosi_py2, spi3_cs0_py3,
> > +                          spi3_cs1_py4, uart5_tx_py5, uart5_rx_py6,
> > +                          uart5_rts_py7, uart5_cts_pz0, usb_vbus_en0_pz1,
> > +                          usb_vbus_en1_pz2, spi1_sck_pz3, spi1_miso_pz4,
> > +                          spi1_mosi_pz5, spi1_cs0_pz6, spi1_cs1_pz7,
> > +                          spi5_sck_pac0, spi5_miso_pac1, spi5_mosi_pac2,
> > +                          spi5_cs0_pac3, soc_gpio57_pac4, soc_gpio58_pac5,
> > +                          soc_gpio59_pac6, soc_gpio60_pac7, soc_gpio45_pad0,
> > +                          soc_gpio46_pad1, soc_gpio47_pad2, soc_gpio48_pad3,
> > +                          ufs0_ref_clk_pae0, ufs0_rst_n_pae1,
> > +                          pex_l5_clkreq_n_paf0, pex_l5_rst_n_paf1,
> > +                          pex_l6_clkreq_n_paf2, pex_l6_rst_n_paf3,
> > +                          pex_l7_clkreq_n_pag0, pex_l7_rst_n_pag1,
> > +                          pex_l8_clkreq_n_pag2, pex_l8_rst_n_pag3,
> > +                          pex_l9_clkreq_n_pag4, pex_l9_rst_n_pag5,
> > +                          pex_l10_clkreq_n_pag6, pex_l10_rst_n_pag7,
> > +                          sdmmc1_comp, eqos_comp, qspi_comp,
> > +                          # drive groups
> > +                          drive_soc_gpio08_pb0, drive_soc_gpio36_pm5,
> > +                          drive_soc_gpio53_pm6, drive_soc_gpio55_pm4,
> > +                          drive_soc_gpio38_pm7, drive_soc_gpio39_pn1,
> > +                          drive_soc_gpio40_pn2, drive_dp_aux_ch0_hpd_pm0,
> > +                          drive_dp_aux_ch1_hpd_pm1, drive_dp_aux_ch2_hpd_pm2,
> > +                          drive_dp_aux_ch3_hpd_pm3, drive_dp_aux_ch1_p_pn3,
> > +                          drive_dp_aux_ch1_n_pn4, drive_dp_aux_ch2_p_pn5,
> > +                          drive_dp_aux_ch2_n_pn6, drive_dp_aux_ch3_p_pn7,
> > +                          drive_dp_aux_ch3_n_pn0, drive_pex_l2_clkreq_n_pk4,
> > +                          drive_pex_wake_n_pl2, drive_pex_l1_clkreq_n_pk2,
> > +                          drive_pex_l1_rst_n_pk3, drive_pex_l0_clkreq_n_pk0,
> > +                          drive_pex_l0_rst_n_pk1, drive_pex_l2_rst_n_pk5,
> > +                          drive_pex_l3_clkreq_n_pk6, drive_pex_l3_rst_n_pk7,
> > +                          drive_pex_l4_clkreq_n_pl0, drive_pex_l4_rst_n_pl1,
> > +                          drive_soc_gpio34_pl3, drive_pex_l5_clkreq_n_paf0,
> > +                          drive_pex_l5_rst_n_paf1, drive_pex_l6_clkreq_n_paf2,
> > +                          drive_pex_l6_rst_n_paf3, drive_pex_l10_clkreq_n_pag6,
> > +                          drive_pex_l10_rst_n_pag7, drive_pex_l7_clkreq_n_pag0,
> > +                          drive_pex_l7_rst_n_pag1, drive_pex_l8_clkreq_n_pag2,
> > +                          drive_pex_l8_rst_n_pag3, drive_pex_l9_clkreq_n_pag4,
> > +                          drive_pex_l9_rst_n_pag5, drive_sdmmc1_clk_pj0,
> > +                          drive_sdmmc1_cmd_pj1, drive_sdmmc1_dat3_pj5,
> > +                          drive_sdmmc1_dat2_pj4, drive_sdmmc1_dat1_pj3,
> > +                          drive_sdmmc1_dat0_pj2 ]
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: nvidia,tegra234-pinmux-aon
> > +    then:
> > +      patternProperties:
> > +        "^pinmux(-[a-z0-9-_]+)?$":
> 
> No underscores
> 
> > +          type: object
> > +          additionalProperties:
> > +            properties:
> > +              nvidia,pins:
> > +                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 ]
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/pinctrl/pinctrl-tegra.h>
> > +
> > +    pinmux@2430000 {
> > +        compatible = "nvidia,tegra234-pinmux";
> > +        reg = <0x2430000 0x17000>;
> > +
> > +        pinctrl-names = "pex_rst";
> > +        pinctrl-0 = <&pex_rst_c5_out_state>;
> > +
> > +        pex_rst_c5_out_state: pinmux-pex-rst-c5-out {
> > +            pex_rst {
> 
> Underscores are not valid in node names.

We have supported underscore in older bindings for historical reasons.
But I suppose for these newer bindings we could disallow them.

Some of the older DTs have a large number of underscores, so I'm not
sure it makes sense to go back and fix those. Maybe something to keep in
mind for when we're done with all the conversions...

Thierry

Thierry Reding Feb. 8, 2023, 11:29 a.m. UTC | #6

On Tue, Feb 07, 2023 at 08:37:51AM -0600, Rob Herring wrote:
> 
> On Tue, 07 Feb 2023 17:26:15 +0530, Prathamesh Shete wrote:
> > Add DT binding doc for Tegra234 pinmux driver.
> > 
> > Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> > ---
> >  .../pinctrl/nvidia,tegra234-pinmux.yaml       | 232 ++++++++++++++++++
> >  1 file changed, 232 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.example.dtb: pinmux@2430000: pinmux-pex-rst-c5-out:pex_rst:nvidia,pins:0: 'pex_l5_rst_n_pgg1' is not one of
[...]

Prathamesh,

Looks like maybe you copied the example from Tegra194. On Tegra234 I
think the corresponding GPIO is AF1, so the full pin name for this is
pex_l5_rst_n_paf1.

Please fix and make sure to run the DT binding check before sending out
v2.

Thierry

Krzysztof Kozlowski Feb. 8, 2023, 11:57 a.m. UTC | #7

On 08/02/2023 12:24, Thierry Reding wrote:
> On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:


>>> +          type: object
>>> +          additionalProperties:
>>> +            properties:
>>> +              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.
>>
>> Define properties in top level, which points to the complexity of your
>> if-else, thus probably this should be split into two bindings. Dunno,
>> your other bindings repeat this pattern :(
> 
> The property itself is already defined in the common schema found in
> nvidia,tegra-pinmux-common.yaml and we're overriding this here for each
> instance since each has its own set of pins.
> 
> This was a compromise to avoid too many bindings. Originally I attempted
> to roll all Tegra pinctrl bindings into a single dt-schema, but that
> turned out truly horrible =) Splitting this into per-SoC bindings is
> already causing a lot of duplication in these files, 

What would be duplicated? Almost eveerything should be coming from
shared binding, so you will have only compatible,
patternProperties(pinmux) and nvidia,pins. And an example. Maybe I miss
something but I would say this would create many but very easy to read
bindings, referencing common pieces.

> though splitting
> off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit with
> that already. Splitting this into per-instance bindings would
> effectively duplicate everything but the pin array here, so we kind of
> settled on this compromise for Tegra194.

OK, but are you sure it is now readable? You have if:then: with
patternProperties: with additionalProperties: with properties: with
nvidia,pins.

> 
> We're taking a bit of a shortcut here already, since technically not all
> pins support all the functions listed above. On the other hand, fully
> accurately describing per-pin functions would make this a total mess, so
> again, we use this simplified representation as a compromise.

That's okay, many platforms do the same way.

(...)

>>> +
>>> +        pex_rst_c5_out_state: pinmux-pex-rst-c5-out {
>>> +            pex_rst {
>>
>> Underscores are not valid in node names.
> 
> We have supported underscore in older bindings for historical reasons.
> But I suppose for these newer bindings we could disallow them.
> 
> Some of the older DTs have a large number of underscores, so I'm not
> sure it makes sense to go back and fix those. Maybe something to keep in
> mind for when we're done with all the conversions...

I understand, up to you. I think that if such older platform is still
supported/maintained/used, then such cleanups are positive. Underscores
are reported by dtc at W=2, so it is not that critical. But many other
nits like generic node names are being enforced by dtschema, thus if you
want to achieve 0-warning state, at some point you will need to address
these. Of course different question is on what tasks you want to spend
your time. :)

Best regards,
Krzysztof

Krzysztof Kozlowski Feb. 8, 2023, 12:01 p.m. UTC | #8

On 08/02/2023 12:00, Thierry Reding wrote:
> On Tue, Feb 07, 2023 at 04:33:42PM +0100, Krzysztof Kozlowski wrote:
>> On 07/02/2023 12:56, Prathamesh Shete wrote:
>>> This change adds pinmux node for Tegra234.
>>>
>>> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
>>> ---
>>>  arch/arm64/boot/dts/nvidia/tegra234.dtsi | 14 ++++++++++++++
>>>  1 file changed, 14 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
>>> index eaf05ee9acd1..c91b88bc56d1 100644
>>> --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
>>> +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
>>> @@ -701,6 +701,13 @@
>>>  			interrupt-controller;
>>>  			#gpio-cells = <2>;
>>>  			gpio-controller;
>>> +			gpio-ranges = <&pinmux 0 0 164>;
>>> +		};
>>> +
>>> +		pinmux: pinmux@2430000 {
>>> +			compatible = "nvidia,tegra234-pinmux";
>>> +			reg = <0x2430000 0x19100>;
>>> +			status = "okay";
>>
>> Why? Anything disabled it?
>>
>>>  		};
>>>  
>>>  		mc: memory-controller@2c00000 {
>>> @@ -1664,6 +1671,13 @@
>>>  			interrupt-controller;
>>>  			#gpio-cells = <2>;
>>>  			gpio-controller;
>>> +			gpio-range = <&pinmux_aon 0 0 32>;
>>> +		};
>>> +
>>> +		pinmux_aon: pinmux@c300000 {
>>> +			compatible = "nvidia,tegra234-pinmux-aon";
>>> +			reg = <0xc300000 0x4000>;
>>> +			status = "okay";
>>
>> Also why?
> 
> These are probably copy-pasted from Tegra194 where these snuck in. I can
> drop those when applying. I'll also prepare a patch to drop these from
> the tegra194.dtsi.
> 
> I wonder if there's a good way to detect these. We'd have to run checks
> on the DT source files, so that's a bit difficult. I do have an
> experimental script that tries to capture some common pitfalls on
> sources but it's quite ugly and slow, but I guess I could add something
> like this. But perhaps there are better ways?

One way to easy spot them is to override always by label, thus every
node defined like above is a new node. However I think we talked about
this and you do not follow this practice, thus there is no way to tell -
is the status reasonable or not.

Automated tools could help here as well - run fdtdump on DTB and look
for status=okay.

Best regards,
Krzysztof

Krzysztof Kozlowski Feb. 8, 2023, 12:06 p.m. UTC | #9

On 08/02/2023 13:01, Krzysztof Kozlowski wrote:
>> I wonder if there's a good way to detect these. We'd have to run checks
>> on the DT source files, so that's a bit difficult. I do have an
>> experimental script that tries to capture some common pitfalls on
>> sources but it's quite ugly and slow, but I guess I could add something
>> like this. But perhaps there are better ways?
> 
> One way to easy spot them is to override always by label, thus every
> node defined like above is a new node. However I think we talked about
> this and you do not follow this practice, thus there is no way to tell -
> is the status reasonable or not.
> 
> Automated tools could help here as well - run fdtdump on DTB and look
> for status=okay.

Eh, obviously it won't work - every node which was disabled in DTSI and
enabled in DTS will have the status=okay...

Best regards,
Krzysztof

Thierry Reding Feb. 8, 2023, 3:46 p.m. UTC | #10

On Wed, Feb 08, 2023 at 01:06:02PM +0100, Krzysztof Kozlowski wrote:
> On 08/02/2023 13:01, Krzysztof Kozlowski wrote:
> >> I wonder if there's a good way to detect these. We'd have to run checks
> >> on the DT source files, so that's a bit difficult. I do have an
> >> experimental script that tries to capture some common pitfalls on
> >> sources but it's quite ugly and slow, but I guess I could add something
> >> like this. But perhaps there are better ways?
> > 
> > One way to easy spot them is to override always by label, thus every
> > node defined like above is a new node. However I think we talked about
> > this and you do not follow this practice, thus there is no way to tell -
> > is the status reasonable or not.
> > 
> > Automated tools could help here as well - run fdtdump on DTB and look
> > for status=okay.
> 
> Eh, obviously it won't work - every node which was disabled in DTSI and
> enabled in DTS will have the status=okay...

Yeah, I was originally thinking along the same lines, but when things
are overridden that check no longer works. I suppose DTC could be taught
to check for this when it merges nodes.

Thierry

Prathamesh Shete March 8, 2023, 11:45 a.m. UTC | #11

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, February 8, 2023 5:28 PM
> To: Thierry Reding <thierry.reding@gmail.com>
> Cc: Prathamesh Shete <pshete@nvidia.com>; Jonathan Hunter
> <jonathanh@nvidia.com>; linus.walleij@linaro.org; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; devicetree@vger.kernel.org; linux-
> tegra@vger.kernel.org; linux-gpio@vger.kernel.org; Suresh Mangipudi
> <smangipudi@nvidia.com>
> Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc
> 
> External email: Use caution opening links or attachments
> 
> 
> On 08/02/2023 12:24, Thierry Reding wrote:
> > On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:
> 
> 
> >>> +          type: object
> >>> +          additionalProperties:
> >>> +            properties:
> >>> +              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.
> >>
> >> Define properties in top level, which points to the complexity of
> >> your if-else, thus probably this should be split into two bindings.
> >> Dunno, your other bindings repeat this pattern :(
> >
> > The property itself is already defined in the common schema found in
> > nvidia,tegra-pinmux-common.yaml and we're overriding this here for
> > each instance since each has its own set of pins.
> >
> > This was a compromise to avoid too many bindings. Originally I
> > attempted to roll all Tegra pinctrl bindings into a single dt-schema,
> > but that turned out truly horrible =) Splitting this into per-SoC
> > bindings is already causing a lot of duplication in these files,
> 
> What would be duplicated? Almost eveerything should be coming from
> shared binding, so you will have only compatible,
> patternProperties(pinmux) and nvidia,pins. And an example. Maybe I miss
> something but I would say this would create many but very easy to read
> bindings, referencing common pieces.
> 
> > though splitting
> > off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit
> > with that already. Splitting this into per-instance bindings would
> > effectively duplicate everything but the pin array here, so we kind of
> > settled on this compromise for Tegra194.
> 
> OK, but are you sure it is now readable? You have if:then: with
> patternProperties: with additionalProperties: with properties: with
> nvidia,pins.
This is inline with the existing bindings and I think this is the compromise that was reached during review when the bindings were submitted,
offer to rework if a better alternative can be found, but that only makes sense if all the other bindings get changed as well, so I think it'd be good if we can merge in the same format as the existing bindings for now and change all of them later on.
Sent a version v2 addressing all other comments.
Thanks
Prathamesh
> 
> >
> > We're taking a bit of a shortcut here already, since technically not
> > all pins support all the functions listed above. On the other hand,
> > fully accurately describing per-pin functions would make this a total
> > mess, so again, we use this simplified representation as a compromise.
> 
> That's okay, many platforms do the same way.
> 
> (...)
> 
> >>> +
> >>> +        pex_rst_c5_out_state: pinmux-pex-rst-c5-out {
> >>> +            pex_rst {
> >>
> >> Underscores are not valid in node names.
> >
> > We have supported underscore in older bindings for historical reasons.
> > But I suppose for these newer bindings we could disallow them.
> >
> > Some of the older DTs have a large number of underscores, so I'm not
> > sure it makes sense to go back and fix those. Maybe something to keep
> > in mind for when we're done with all the conversions...
> 
> I understand, up to you. I think that if such older platform is still
> supported/maintained/used, then such cleanups are positive. Underscores
> are reported by dtc at W=2, so it is not that critical. But many other nits like
> generic node names are being enforced by dtschema, thus if you want to
> achieve 0-warning state, at some point you will need to address these. Of
> course different question is on what tasks you want to spend your time. :)
> 
> Best regards,
> Krzysztof

Krzysztof Kozlowski March 8, 2023, 12:24 p.m. UTC | #12

On 08/03/2023 12:45, Prathamesh Shete wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Wednesday, February 8, 2023 5:28 PM
>> To: Thierry Reding <thierry.reding@gmail.com>
>> Cc: Prathamesh Shete <pshete@nvidia.com>; Jonathan Hunter
>> <jonathanh@nvidia.com>; linus.walleij@linaro.org; robh+dt@kernel.org;
>> krzysztof.kozlowski+dt@linaro.org; devicetree@vger.kernel.org; linux-
>> tegra@vger.kernel.org; linux-gpio@vger.kernel.org; Suresh Mangipudi
>> <smangipudi@nvidia.com>
>> Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc
>>
>> External email: Use caution opening links or attachments
>>
>>
>> On 08/02/2023 12:24, Thierry Reding wrote:
>>> On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:
>>
>>
>>>>> +          type: object
>>>>> +          additionalProperties:
>>>>> +            properties:
>>>>> +              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.
>>>>
>>>> Define properties in top level, which points to the complexity of
>>>> your if-else, thus probably this should be split into two bindings.
>>>> Dunno, your other bindings repeat this pattern :(
>>>
>>> The property itself is already defined in the common schema found in
>>> nvidia,tegra-pinmux-common.yaml and we're overriding this here for
>>> each instance since each has its own set of pins.
>>>
>>> This was a compromise to avoid too many bindings. Originally I
>>> attempted to roll all Tegra pinctrl bindings into a single dt-schema,
>>> but that turned out truly horrible =) Splitting this into per-SoC
>>> bindings is already causing a lot of duplication in these files,
>>
>> What would be duplicated? Almost eveerything should be coming from
>> shared binding, so you will have only compatible,
>> patternProperties(pinmux) and nvidia,pins. And an example. Maybe I miss
>> something but I would say this would create many but very easy to read
>> bindings, referencing common pieces.
>>
>>> though splitting
>>> off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit
>>> with that already. Splitting this into per-instance bindings would
>>> effectively duplicate everything but the pin array here, so we kind of
>>> settled on this compromise for Tegra194.
>>
>> OK, but are you sure it is now readable? You have if:then: with
>> patternProperties: with additionalProperties: with properties: with
>> nvidia,pins.
> This is inline with the existing bindings and I think this is the compromise that was reached during review when the bindings were submitted,

So the code might be totally unreadable, but it is inline with existing
code, thus it should stay unreadable. Great.

> offer to rework if a better alternative can be found, but that only makes sense if all the other bindings get changed as well, so I think it'd be good if we can merge in the same format as the existing bindings for now and change all of them later on.

Cleanup should happen before adding new bindings.



Best regards,
Krzysztof

Thierry Reding March 23, 2023, 2:11 p.m. UTC | #13

On Wed, Mar 08, 2023 at 01:24:04PM +0100, Krzysztof Kozlowski wrote:
> On 08/03/2023 12:45, Prathamesh Shete wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Wednesday, February 8, 2023 5:28 PM
> >> To: Thierry Reding <thierry.reding@gmail.com>
> >> Cc: Prathamesh Shete <pshete@nvidia.com>; Jonathan Hunter
> >> <jonathanh@nvidia.com>; linus.walleij@linaro.org; robh+dt@kernel.org;
> >> krzysztof.kozlowski+dt@linaro.org; devicetree@vger.kernel.org; linux-
> >> tegra@vger.kernel.org; linux-gpio@vger.kernel.org; Suresh Mangipudi
> >> <smangipudi@nvidia.com>
> >> Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc
> >>
> >> External email: Use caution opening links or attachments
> >>
> >>
> >> On 08/02/2023 12:24, Thierry Reding wrote:
> >>> On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:
> >>
> >>
> >>>>> +          type: object
> >>>>> +          additionalProperties:
> >>>>> +            properties:
> >>>>> +              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.
> >>>>
> >>>> Define properties in top level, which points to the complexity of
> >>>> your if-else, thus probably this should be split into two bindings.
> >>>> Dunno, your other bindings repeat this pattern :(
> >>>
> >>> The property itself is already defined in the common schema found in
> >>> nvidia,tegra-pinmux-common.yaml and we're overriding this here for
> >>> each instance since each has its own set of pins.
> >>>
> >>> This was a compromise to avoid too many bindings. Originally I
> >>> attempted to roll all Tegra pinctrl bindings into a single dt-schema,
> >>> but that turned out truly horrible =) Splitting this into per-SoC
> >>> bindings is already causing a lot of duplication in these files,
> >>
> >> What would be duplicated? Almost eveerything should be coming from
> >> shared binding, so you will have only compatible,
> >> patternProperties(pinmux) and nvidia,pins. And an example. Maybe I miss
> >> something but I would say this would create many but very easy to read
> >> bindings, referencing common pieces.
> >>
> >>> though splitting
> >>> off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit
> >>> with that already. Splitting this into per-instance bindings would
> >>> effectively duplicate everything but the pin array here, so we kind of
> >>> settled on this compromise for Tegra194.
> >>
> >> OK, but are you sure it is now readable? You have if:then: with
> >> patternProperties: with additionalProperties: with properties: with
> >> nvidia,pins.
> > This is inline with the existing bindings and I think this is the compromise that was reached during review when the bindings were submitted,
> 
> So the code might be totally unreadable, but it is inline with existing
> code, thus it should stay unreadable. Great.

I'd say this is very subjective. I personally don't find the current
version hard to read, but that's maybe because I wrote it... =)

> > offer to rework if a better alternative can be found, but that only makes sense if all the other bindings get changed as well, so I think it'd be good if we can merge in the same format as the existing bindings for now and change all of them later on.
> 
> Cleanup should happen before adding new bindings.

I don't recall the exact problems that I ran into last time, but I do
remember that pulling out the common bindings to the very top-level was
the main issue.

If I understand correctly what you're saying, the main problem that
makes this hard to read is the if and else constructs for AON/MAIN
variants on Tegra194/Tegra234. These should be quite easy to pull out
into separate bindings. I'll do that first and then see if there's
anything that could be done to further improve things.

Thierry

Krzysztof Kozlowski March 26, 2023, 12:19 p.m. UTC | #14

On 23/03/2023 15:11, Thierry Reding wrote:
> On Wed, Mar 08, 2023 at 01:24:04PM +0100, Krzysztof Kozlowski wrote:
>> On 08/03/2023 12:45, Prathamesh Shete wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>> Sent: Wednesday, February 8, 2023 5:28 PM
>>>> To: Thierry Reding <thierry.reding@gmail.com>
>>>> Cc: Prathamesh Shete <pshete@nvidia.com>; Jonathan Hunter
>>>> <jonathanh@nvidia.com>; linus.walleij@linaro.org; robh+dt@kernel.org;
>>>> krzysztof.kozlowski+dt@linaro.org; devicetree@vger.kernel.org; linux-
>>>> tegra@vger.kernel.org; linux-gpio@vger.kernel.org; Suresh Mangipudi
>>>> <smangipudi@nvidia.com>
>>>> Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc
>>>>
>>>> External email: Use caution opening links or attachments
>>>>
>>>>
>>>> On 08/02/2023 12:24, Thierry Reding wrote:
>>>>> On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:
>>>>
>>>>
>>>>>>> +          type: object
>>>>>>> +          additionalProperties:
>>>>>>> +            properties:
>>>>>>> +              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.
>>>>>>
>>>>>> Define properties in top level, which points to the complexity of
>>>>>> your if-else, thus probably this should be split into two bindings.
>>>>>> Dunno, your other bindings repeat this pattern :(
>>>>>
>>>>> The property itself is already defined in the common schema found in
>>>>> nvidia,tegra-pinmux-common.yaml and we're overriding this here for
>>>>> each instance since each has its own set of pins.
>>>>>
>>>>> This was a compromise to avoid too many bindings. Originally I
>>>>> attempted to roll all Tegra pinctrl bindings into a single dt-schema,
>>>>> but that turned out truly horrible =) Splitting this into per-SoC
>>>>> bindings is already causing a lot of duplication in these files,
>>>>
>>>> What would be duplicated? Almost eveerything should be coming from
>>>> shared binding, so you will have only compatible,
>>>> patternProperties(pinmux) and nvidia,pins. And an example. Maybe I miss
>>>> something but I would say this would create many but very easy to read
>>>> bindings, referencing common pieces.
>>>>
>>>>> though splitting
>>>>> off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit
>>>>> with that already. Splitting this into per-instance bindings would
>>>>> effectively duplicate everything but the pin array here, so we kind of
>>>>> settled on this compromise for Tegra194.
>>>>
>>>> OK, but are you sure it is now readable? You have if:then: with
>>>> patternProperties: with additionalProperties: with properties: with
>>>> nvidia,pins.
>>> This is inline with the existing bindings and I think this is the compromise that was reached during review when the bindings were submitted,
>>
>> So the code might be totally unreadable, but it is inline with existing
>> code, thus it should stay unreadable. Great.
> 
> I'd say this is very subjective. I personally don't find the current
> version hard to read, but that's maybe because I wrote it... =)
> 
>>> offer to rework if a better alternative can be found, but that only makes sense if all the other bindings get changed as well, so I think it'd be good if we can merge in the same format as the existing bindings for now and change all of them later on.
>>
>> Cleanup should happen before adding new bindings.
> 
> I don't recall the exact problems that I ran into last time, but I do
> remember that pulling out the common bindings to the very top-level was
> the main issue.
> 
> If I understand correctly what you're saying, the main problem that
> makes this hard to read is the if and else constructs for AON/MAIN
> variants on Tegra194/Tegra234. These should be quite easy to pull out
> into separate bindings. I'll do that first and then see if there's
> anything that could be done to further improve things.

One problem is allowing characters here which are not allowed. Second
problem is reluctance to change it with argument "existing bindings also
have this problem". It's explanation like "there is already bug like
this, so I am allowed to add similar one".

Now third is that defining properties in allOf is not the style we want
to have, because it does not work with additionalProperties and is
difficult to read. Again using argument "existing code also does like
this" is a very poor argument.

Best regards,
Krzysztof

Thierry Reding March 28, 2023, 12:39 p.m. UTC | #15

On Sun, Mar 26, 2023 at 02:19:45PM +0200, Krzysztof Kozlowski wrote:
> On 23/03/2023 15:11, Thierry Reding wrote:
> > On Wed, Mar 08, 2023 at 01:24:04PM +0100, Krzysztof Kozlowski wrote:
> >> On 08/03/2023 12:45, Prathamesh Shete wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>>> Sent: Wednesday, February 8, 2023 5:28 PM
> >>>> To: Thierry Reding <thierry.reding@gmail.com>
> >>>> Cc: Prathamesh Shete <pshete@nvidia.com>; Jonathan Hunter
> >>>> <jonathanh@nvidia.com>; linus.walleij@linaro.org; robh+dt@kernel.org;
> >>>> krzysztof.kozlowski+dt@linaro.org; devicetree@vger.kernel.org; linux-
> >>>> tegra@vger.kernel.org; linux-gpio@vger.kernel.org; Suresh Mangipudi
> >>>> <smangipudi@nvidia.com>
> >>>> Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc
> >>>>
> >>>> External email: Use caution opening links or attachments
> >>>>
> >>>>
> >>>> On 08/02/2023 12:24, Thierry Reding wrote:
> >>>>> On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:
> >>>>
> >>>>
> >>>>>>> +          type: object
> >>>>>>> +          additionalProperties:
> >>>>>>> +            properties:
> >>>>>>> +              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.
> >>>>>>
> >>>>>> Define properties in top level, which points to the complexity of
> >>>>>> your if-else, thus probably this should be split into two bindings.
> >>>>>> Dunno, your other bindings repeat this pattern :(
> >>>>>
> >>>>> The property itself is already defined in the common schema found in
> >>>>> nvidia,tegra-pinmux-common.yaml and we're overriding this here for
> >>>>> each instance since each has its own set of pins.
> >>>>>
> >>>>> This was a compromise to avoid too many bindings. Originally I
> >>>>> attempted to roll all Tegra pinctrl bindings into a single dt-schema,
> >>>>> but that turned out truly horrible =) Splitting this into per-SoC
> >>>>> bindings is already causing a lot of duplication in these files,
> >>>>
> >>>> What would be duplicated? Almost eveerything should be coming from
> >>>> shared binding, so you will have only compatible,
> >>>> patternProperties(pinmux) and nvidia,pins. And an example. Maybe I miss
> >>>> something but I would say this would create many but very easy to read
> >>>> bindings, referencing common pieces.
> >>>>
> >>>>> though splitting
> >>>>> off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit
> >>>>> with that already. Splitting this into per-instance bindings would
> >>>>> effectively duplicate everything but the pin array here, so we kind of
> >>>>> settled on this compromise for Tegra194.
> >>>>
> >>>> OK, but are you sure it is now readable? You have if:then: with
> >>>> patternProperties: with additionalProperties: with properties: with
> >>>> nvidia,pins.
> >>> This is inline with the existing bindings and I think this is the compromise that was reached during review when the bindings were submitted,
> >>
> >> So the code might be totally unreadable, but it is inline with existing
> >> code, thus it should stay unreadable. Great.
> > 
> > I'd say this is very subjective. I personally don't find the current
> > version hard to read, but that's maybe because I wrote it... =)
> > 
> >>> offer to rework if a better alternative can be found, but that only makes sense if all the other bindings get changed as well, so I think it'd be good if we can merge in the same format as the existing bindings for now and change all of them later on.
> >>
> >> Cleanup should happen before adding new bindings.
> > 
> > I don't recall the exact problems that I ran into last time, but I do
> > remember that pulling out the common bindings to the very top-level was
> > the main issue.
> > 
> > If I understand correctly what you're saying, the main problem that
> > makes this hard to read is the if and else constructs for AON/MAIN
> > variants on Tegra194/Tegra234. These should be quite easy to pull out
> > into separate bindings. I'll do that first and then see if there's
> > anything that could be done to further improve things.
> 
> One problem is allowing characters here which are not allowed. Second
> problem is reluctance to change it with argument "existing bindings also
> have this problem". It's explanation like "there is already bug like
> this, so I am allowed to add similar one".

This is not a bug that we're trying to replicate. We're basing this
binding on a existing bindings that were already reviewed upstream a
long time ago. It uses a shared binding that's in use by these other
bindings, so making any changes to this new binding means either the
other ones need to be changed as well or we can't reuse the existing
shared binding.

> Now third is that defining properties in allOf is not the style we want
> to have, because it does not work with additionalProperties and is
> difficult to read. Again using argument "existing code also does like
> this" is a very poor argument.

As far as I can tell, it does work as expected in this case because
we're not actually adding any *new* properties in the allOf/if branches.
If we were, then yes, we would need to use unevaluatedProperties and
that can get complicated. But again, in this case we're merely
overriding existing properties with more specific values, which means
that both the standard binding applies and then things are narrowed down
by the values defined for each compatible.

Thierry

Krzysztof Kozlowski March 30, 2023, 1:58 p.m. UTC | #16

On 28/03/2023 14:39, Thierry Reding wrote:
> On Sun, Mar 26, 2023 at 02:19:45PM +0200, Krzysztof Kozlowski wrote:
>> On 23/03/2023 15:11, Thierry Reding wrote:
>>> On Wed, Mar 08, 2023 at 01:24:04PM +0100, Krzysztof Kozlowski wrote:
>>>> On 08/03/2023 12:45, Prathamesh Shete wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>>>>> Sent: Wednesday, February 8, 2023 5:28 PM
>>>>>> To: Thierry Reding <thierry.reding@gmail.com>
>>>>>> Cc: Prathamesh Shete <pshete@nvidia.com>; Jonathan Hunter
>>>>>> <jonathanh@nvidia.com>; linus.walleij@linaro.org; robh+dt@kernel.org;
>>>>>> krzysztof.kozlowski+dt@linaro.org; devicetree@vger.kernel.org; linux-
>>>>>> tegra@vger.kernel.org; linux-gpio@vger.kernel.org; Suresh Mangipudi
>>>>>> <smangipudi@nvidia.com>
>>>>>> Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc
>>>>>>
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> On 08/02/2023 12:24, Thierry Reding wrote:
>>>>>>> On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:
>>>>>>
>>>>>>
>>>>>>>>> +          type: object
>>>>>>>>> +          additionalProperties:
>>>>>>>>> +            properties:
>>>>>>>>> +              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.
>>>>>>>>
>>>>>>>> Define properties in top level, which points to the complexity of
>>>>>>>> your if-else, thus probably this should be split into two bindings.
>>>>>>>> Dunno, your other bindings repeat this pattern :(
>>>>>>>
>>>>>>> The property itself is already defined in the common schema found in
>>>>>>> nvidia,tegra-pinmux-common.yaml and we're overriding this here for
>>>>>>> each instance since each has its own set of pins.
>>>>>>>
>>>>>>> This was a compromise to avoid too many bindings. Originally I
>>>>>>> attempted to roll all Tegra pinctrl bindings into a single dt-schema,
>>>>>>> but that turned out truly horrible =) Splitting this into per-SoC
>>>>>>> bindings is already causing a lot of duplication in these files,
>>>>>>
>>>>>> What would be duplicated? Almost eveerything should be coming from
>>>>>> shared binding, so you will have only compatible,
>>>>>> patternProperties(pinmux) and nvidia,pins. And an example. Maybe I miss
>>>>>> something but I would say this would create many but very easy to read
>>>>>> bindings, referencing common pieces.
>>>>>>
>>>>>>> though splitting
>>>>>>> off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit
>>>>>>> with that already. Splitting this into per-instance bindings would
>>>>>>> effectively duplicate everything but the pin array here, so we kind of
>>>>>>> settled on this compromise for Tegra194.
>>>>>>
>>>>>> OK, but are you sure it is now readable? You have if:then: with
>>>>>> patternProperties: with additionalProperties: with properties: with
>>>>>> nvidia,pins.
>>>>> This is inline with the existing bindings and I think this is the compromise that was reached during review when the bindings were submitted,
>>>>
>>>> So the code might be totally unreadable, but it is inline with existing
>>>> code, thus it should stay unreadable. Great.
>>>
>>> I'd say this is very subjective. I personally don't find the current
>>> version hard to read, but that's maybe because I wrote it... =)
>>>
>>>>> offer to rework if a better alternative can be found, but that only makes sense if all the other bindings get changed as well, so I think it'd be good if we can merge in the same format as the existing bindings for now and change all of them later on.
>>>>
>>>> Cleanup should happen before adding new bindings.
>>>
>>> I don't recall the exact problems that I ran into last time, but I do
>>> remember that pulling out the common bindings to the very top-level was
>>> the main issue.
>>>
>>> If I understand correctly what you're saying, the main problem that
>>> makes this hard to read is the if and else constructs for AON/MAIN
>>> variants on Tegra194/Tegra234. These should be quite easy to pull out
>>> into separate bindings. I'll do that first and then see if there's
>>> anything that could be done to further improve things.
>>
>> One problem is allowing characters here which are not allowed. Second
>> problem is reluctance to change it with argument "existing bindings also
>> have this problem". It's explanation like "there is already bug like
>> this, so I am allowed to add similar one".
> 
> This is not a bug that we're trying to replicate. We're basing this
> binding on a existing bindings that were already reviewed upstream a
> long time ago. It uses a shared binding that's in use by these other
> bindings, so making any changes to this new binding means either the
> other ones need to be changed as well or we can't reuse the existing
> shared binding.

Are you sure? I did not see here conflict. The specific device binding
can narrow the pattern defined in common binding.

What's more, where do you see this pattern at all in shared binding?

I am sorry, but this does not fit my arguments at all. This pattern is
clearly wrong and argument to keep duplicating it because other (not
common!) binding also has it is by design invalid.

> 
>> Now third is that defining properties in allOf is not the style we want
>> to have, because it does not work with additionalProperties and is
>> difficult to read. Again using argument "existing code also does like
>> this" is a very poor argument.
> 
> As far as I can tell, it does work as expected in this case because
> we're not actually adding any *new* properties in the allOf/if branches.

The if:else: defines type and additionalProperties, so I am sorry but
this is not a readable solution.

> If we were, then yes, we would need to use unevaluatedProperties and
> that can get complicated. But again, in this case we're merely
> overriding existing properties with more specific values, which means
> that both the standard binding applies and then things are narrowed down
> by the values defined for each compatible.


Best regards,
Krzysztof

Thierry Reding March 30, 2023, 4:32 p.m. UTC | #17

On Thu, Mar 30, 2023 at 03:58:51PM +0200, Krzysztof Kozlowski wrote:
> On 28/03/2023 14:39, Thierry Reding wrote:
> > On Sun, Mar 26, 2023 at 02:19:45PM +0200, Krzysztof Kozlowski wrote:
> >> On 23/03/2023 15:11, Thierry Reding wrote:
> >>> On Wed, Mar 08, 2023 at 01:24:04PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 08/03/2023 12:45, Prathamesh Shete wrote:
> >>>>>
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>>>>> Sent: Wednesday, February 8, 2023 5:28 PM
> >>>>>> To: Thierry Reding <thierry.reding@gmail.com>
> >>>>>> Cc: Prathamesh Shete <pshete@nvidia.com>; Jonathan Hunter
> >>>>>> <jonathanh@nvidia.com>; linus.walleij@linaro.org; robh+dt@kernel.org;
> >>>>>> krzysztof.kozlowski+dt@linaro.org; devicetree@vger.kernel.org; linux-
> >>>>>> tegra@vger.kernel.org; linux-gpio@vger.kernel.org; Suresh Mangipudi
> >>>>>> <smangipudi@nvidia.com>
> >>>>>> Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc
> >>>>>>
> >>>>>> External email: Use caution opening links or attachments
> >>>>>>
> >>>>>>
> >>>>>> On 08/02/2023 12:24, Thierry Reding wrote:
> >>>>>>> On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:
> >>>>>>
> >>>>>>
> >>>>>>>>> +          type: object
> >>>>>>>>> +          additionalProperties:
> >>>>>>>>> +            properties:
> >>>>>>>>> +              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.
> >>>>>>>>
> >>>>>>>> Define properties in top level, which points to the complexity of
> >>>>>>>> your if-else, thus probably this should be split into two bindings.
> >>>>>>>> Dunno, your other bindings repeat this pattern :(
> >>>>>>>
> >>>>>>> The property itself is already defined in the common schema found in
> >>>>>>> nvidia,tegra-pinmux-common.yaml and we're overriding this here for
> >>>>>>> each instance since each has its own set of pins.
> >>>>>>>
> >>>>>>> This was a compromise to avoid too many bindings. Originally I
> >>>>>>> attempted to roll all Tegra pinctrl bindings into a single dt-schema,
> >>>>>>> but that turned out truly horrible =) Splitting this into per-SoC
> >>>>>>> bindings is already causing a lot of duplication in these files,
> >>>>>>
> >>>>>> What would be duplicated? Almost eveerything should be coming from
> >>>>>> shared binding, so you will have only compatible,
> >>>>>> patternProperties(pinmux) and nvidia,pins. And an example. Maybe I miss
> >>>>>> something but I would say this would create many but very easy to read
> >>>>>> bindings, referencing common pieces.
> >>>>>>
> >>>>>>> though splitting
> >>>>>>> off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit
> >>>>>>> with that already. Splitting this into per-instance bindings would
> >>>>>>> effectively duplicate everything but the pin array here, so we kind of
> >>>>>>> settled on this compromise for Tegra194.
> >>>>>>
> >>>>>> OK, but are you sure it is now readable? You have if:then: with
> >>>>>> patternProperties: with additionalProperties: with properties: with
> >>>>>> nvidia,pins.
> >>>>> This is inline with the existing bindings and I think this is the compromise that was reached during review when the bindings were submitted,
> >>>>
> >>>> So the code might be totally unreadable, but it is inline with existing
> >>>> code, thus it should stay unreadable. Great.
> >>>
> >>> I'd say this is very subjective. I personally don't find the current
> >>> version hard to read, but that's maybe because I wrote it... =)
> >>>
> >>>>> offer to rework if a better alternative can be found, but that only makes sense if all the other bindings get changed as well, so I think it'd be good if we can merge in the same format as the existing bindings for now and change all of them later on.
> >>>>
> >>>> Cleanup should happen before adding new bindings.
> >>>
> >>> I don't recall the exact problems that I ran into last time, but I do
> >>> remember that pulling out the common bindings to the very top-level was
> >>> the main issue.
> >>>
> >>> If I understand correctly what you're saying, the main problem that
> >>> makes this hard to read is the if and else constructs for AON/MAIN
> >>> variants on Tegra194/Tegra234. These should be quite easy to pull out
> >>> into separate bindings. I'll do that first and then see if there's
> >>> anything that could be done to further improve things.
> >>
> >> One problem is allowing characters here which are not allowed. Second
> >> problem is reluctance to change it with argument "existing bindings also
> >> have this problem". It's explanation like "there is already bug like
> >> this, so I am allowed to add similar one".
> > 
> > This is not a bug that we're trying to replicate. We're basing this
> > binding on a existing bindings that were already reviewed upstream a
> > long time ago. It uses a shared binding that's in use by these other
> > bindings, so making any changes to this new binding means either the
> > other ones need to be changed as well or we can't reuse the existing
> > shared binding.
> 
> Are you sure? I did not see here conflict. The specific device binding
> can narrow the pattern defined in common binding.
> 
> What's more, where do you see this pattern at all in shared binding?
> 
> I am sorry, but this does not fit my arguments at all. This pattern is
> clearly wrong and argument to keep duplicating it because other (not
> common!) binding also has it is by design invalid.
> 
> > 
> >> Now third is that defining properties in allOf is not the style we want
> >> to have, because it does not work with additionalProperties and is
> >> difficult to read. Again using argument "existing code also does like
> >> this" is a very poor argument.
> > 
> > As far as I can tell, it does work as expected in this case because
> > we're not actually adding any *new* properties in the allOf/if branches.
> 
> The if:else: defines type and additionalProperties, so I am sorry but
> this is not a readable solution.
> 
> > If we were, then yes, we would need to use unevaluatedProperties and
> > that can get complicated. But again, in this case we're merely
> > overriding existing properties with more specific values, which means
> > that both the standard binding applies and then things are narrowed down
> > by the values defined for each compatible.

Okay, so I'm starting to get a bit lost here, so maybe it's time for
another proposal. I've tried splitting this up more so that we avoid
the if/else block. Would you mind taking another look to see if the
patch below is any more readable to you?

Thierry
From 00cb909f6d8732680d65cdb67e0573c8e6dc7b7a Mon Sep 17 00:00:00 2001
From: Prathamesh Shete <pshete@nvidia.com>
Date: Wed, 8 Mar 2023 17:14:30 +0530
Subject: [PATCH] dt-bindings: pinctrl: tegra234

Add DT binding doc for Tegra234 pinmux driver.

Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 .../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
+
+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
+
+patternProperties:
+  "^pinmux(-[a-z0-9-]+)?$":
+    type: object
+
+    # pin groups
+    additionalProperties:
+      properties:
+        nvidia,pins:
+          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
+...
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
+
+  reg:
+    items:
+      - description: pinmux registers
+
+patternProperties:
+  "^pinmux(-[a-z0-9-]+)?$":
+    type: object
+    properties:
+      phandle: true
+
+    # pin groups
+    additionalProperties:
+      $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.
+
+        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
+
+      required:
+        - nvidia,pins
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+...
diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
new file mode 100644
index 000000000000..7f0bf3d75f35
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
@@ -0,0 +1,141 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/nvidia,tegra234-pinmux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+$ref: nvidia,tegra234-pinmux-common.yaml
+
+title: NVIDIA Tegra234 Pinmux Controller
+
+maintainers:
+  - Thierry Reding <thierry.reding@gmail.com>
+  - Jon Hunter <jonathanh@nvidia.com>
+
+properties:
+  compatible:
+    const: nvidia,tegra234-pinmux
+
+  reg: true
+
+patternProperties:
+  "^pinmux(-[a-z0-9-]+)?$":
+    type: object
+
+    # pin groups
+    additionalProperties:
+      properties:
+        nvidia,pins:
+          items:
+            enum: [ dap6_sclk_pa0, dap6_dout_pa1, dap6_din_pa2,
+                    dap6_fs_pa3, dap4_sclk_pa4, dap4_dout_pa5,
+                    dap4_din_pa6, dap4_fs_pa7, soc_gpio08_pb0,
+                    qspi0_sck_pc0, qspi0_cs_n_pc1,
+                    qspi0_io0_pc2, qspi0_io1_pc3, qspi0_io2_pc4,
+                    qspi0_io3_pc5, qspi1_sck_pc6, qspi1_cs_n_pc7,
+                    qspi1_io0_pd0, qspi1_io1_pd1, qspi1_io2_pd2,
+                    qspi1_io3_pd3, eqos_txc_pe0, eqos_td0_pe1,
+                    eqos_td1_pe2, eqos_td2_pe3, eqos_td3_pe4,
+                    eqos_tx_ctl_pe5, eqos_rd0_pe6, eqos_rd1_pe7,
+                    eqos_rd2_pf0, eqos_rd3_pf1, eqos_rx_ctl_pf2,
+                    eqos_rxc_pf3, eqos_sma_mdio_pf4, eqos_sma_mdc_pf5,
+                    soc_gpio13_pg0, soc_gpio14_pg1, soc_gpio15_pg2,
+                    soc_gpio16_pg3, soc_gpio17_pg4, soc_gpio18_pg5,
+                    soc_gpio19_pg6, soc_gpio20_pg7, soc_gpio21_ph0,
+                    soc_gpio22_ph1, soc_gpio06_ph2, uart4_tx_ph3,
+                    uart4_rx_ph4, uart4_rts_ph5, uart4_cts_ph6,
+                    soc_gpio41_ph7, soc_gpio42_pi0, soc_gpio43_pi1,
+                    soc_gpio44_pi2, gen1_i2c_scl_pi3, gen1_i2c_sda_pi4,
+                    cpu_pwr_req_pi5, soc_gpio07_pi6,
+                    sdmmc1_clk_pj0, sdmmc1_cmd_pj1, sdmmc1_dat0_pj2,
+                    sdmmc1_dat1_pj3, sdmmc1_dat2_pj4, sdmmc1_dat3_pj5,
+                    pex_l0_clkreq_n_pk0, pex_l0_rst_n_pk1,
+                    pex_l1_clkreq_n_pk2, pex_l1_rst_n_pk3,
+                    pex_l2_clkreq_n_pk4, pex_l2_rst_n_pk5,
+                    pex_l3_clkreq_n_pk6, pex_l3_rst_n_pk7,
+                    pex_l4_clkreq_n_pl0, pex_l4_rst_n_pl1,
+                    pex_wake_n_pl2, soc_gpio34_pl3, dp_aux_ch0_hpd_pm0,
+                    dp_aux_ch1_hpd_pm1, dp_aux_ch2_hpd_pm2,
+                    dp_aux_ch3_hpd_pm3, soc_gpio55_pm4, soc_gpio36_pm5,
+                    soc_gpio53_pm6, soc_gpio38_pm7, dp_aux_ch3_n_pn0,
+                    soc_gpio39_pn1, soc_gpio40_pn2, dp_aux_ch1_p_pn3,
+                    dp_aux_ch1_n_pn4, dp_aux_ch2_p_pn5, dp_aux_ch2_n_pn6,
+                    dp_aux_ch3_p_pn7, extperiph1_clk_pp0,
+                    extperiph2_clk_pp1, cam_i2c_scl_pp2, cam_i2c_sda_pp3,
+                    soc_gpio23_pp4, soc_gpio24_pp5, soc_gpio25_pp6,
+                    pwr_i2c_scl_pp7, pwr_i2c_sda_pq0, soc_gpio28_pq1,
+                    soc_gpio29_pq2, soc_gpio30_pq3, soc_gpio31_pq4,
+                    soc_gpio32_pq5, soc_gpio33_pq6, soc_gpio35_pq7,
+                    soc_gpio37_pr0, soc_gpio56_pr1, uart1_tx_pr2,
+                    uart1_rx_pr3, uart1_rts_pr4, uart1_cts_pr5,
+                    soc_gpio61_pw0, soc_gpio62_pw1, gpu_pwr_req_px0,
+                    cv_pwr_req_px1, gp_pwm2_px2, gp_pwm3_px3, uart2_tx_px4,
+                    uart2_rx_px5, uart2_rts_px6, uart2_cts_px7, spi3_sck_py0,
+                    spi3_miso_py1, spi3_mosi_py2, spi3_cs0_py3,
+                    spi3_cs1_py4, uart5_tx_py5, uart5_rx_py6,
+                    uart5_rts_py7, uart5_cts_pz0, usb_vbus_en0_pz1,
+                    usb_vbus_en1_pz2, spi1_sck_pz3, spi1_miso_pz4,
+                    spi1_mosi_pz5, spi1_cs0_pz6, spi1_cs1_pz7,
+                    spi5_sck_pac0, spi5_miso_pac1, spi5_mosi_pac2,
+                    spi5_cs0_pac3, soc_gpio57_pac4, soc_gpio58_pac5,
+                    soc_gpio59_pac6, soc_gpio60_pac7, soc_gpio45_pad0,
+                    soc_gpio46_pad1, soc_gpio47_pad2, soc_gpio48_pad3,
+                    ufs0_ref_clk_pae0, ufs0_rst_n_pae1,
+                    pex_l5_clkreq_n_paf0, pex_l5_rst_n_paf1,
+                    pex_l6_clkreq_n_paf2, pex_l6_rst_n_paf3,
+                    pex_l7_clkreq_n_pag0, pex_l7_rst_n_pag1,
+                    pex_l8_clkreq_n_pag2, pex_l8_rst_n_pag3,
+                    pex_l9_clkreq_n_pag4, pex_l9_rst_n_pag5,
+                    pex_l10_clkreq_n_pag6, pex_l10_rst_n_pag7,
+                    sdmmc1_comp, eqos_comp, qspi_comp,
+                    # drive groups
+                    drive_soc_gpio08_pb0, drive_soc_gpio36_pm5,
+                    drive_soc_gpio53_pm6, drive_soc_gpio55_pm4,
+                    drive_soc_gpio38_pm7, drive_soc_gpio39_pn1,
+                    drive_soc_gpio40_pn2, drive_dp_aux_ch0_hpd_pm0,
+                    drive_dp_aux_ch1_hpd_pm1, drive_dp_aux_ch2_hpd_pm2,
+                    drive_dp_aux_ch3_hpd_pm3, drive_dp_aux_ch1_p_pn3,
+                    drive_dp_aux_ch1_n_pn4, drive_dp_aux_ch2_p_pn5,
+                    drive_dp_aux_ch2_n_pn6, drive_dp_aux_ch3_p_pn7,
+                    drive_dp_aux_ch3_n_pn0, drive_pex_l2_clkreq_n_pk4,
+                    drive_pex_wake_n_pl2, drive_pex_l1_clkreq_n_pk2,
+                    drive_pex_l1_rst_n_pk3, drive_pex_l0_clkreq_n_pk0,
+                    drive_pex_l0_rst_n_pk1, drive_pex_l2_rst_n_pk5,
+                    drive_pex_l3_clkreq_n_pk6, drive_pex_l3_rst_n_pk7,
+                    drive_pex_l4_clkreq_n_pl0, drive_pex_l4_rst_n_pl1,
+                    drive_soc_gpio34_pl3, drive_pex_l5_clkreq_n_paf0,
+                    drive_pex_l5_rst_n_paf1, drive_pex_l6_clkreq_n_paf2,
+                    drive_pex_l6_rst_n_paf3, drive_pex_l10_clkreq_n_pag6,
+                    drive_pex_l10_rst_n_pag7, drive_pex_l7_clkreq_n_pag0,
+                    drive_pex_l7_rst_n_pag1, drive_pex_l8_clkreq_n_pag2,
+                    drive_pex_l8_rst_n_pag3, drive_pex_l9_clkreq_n_pag4,
+                    drive_pex_l9_rst_n_pag5, drive_sdmmc1_clk_pj0,
+                    drive_sdmmc1_cmd_pj1, drive_sdmmc1_dat3_pj5,
+                    drive_sdmmc1_dat2_pj4, drive_sdmmc1_dat1_pj3,
+                    drive_sdmmc1_dat0_pj2 ]
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/pinctrl/pinctrl-tegra.h>
+
+    pinmux@2430000 {
+        compatible = "nvidia,tegra234-pinmux";
+        reg = <0x2430000 0x17000>;
+
+        pinctrl-names = "pex_rst";
+        pinctrl-0 = <&pex_rst_c5_out_state>;
+
+        pex_rst_c5_out_state: pinmux-pex-rst-c5-out {
+            pexrst {
+                nvidia,pins = "pex_l5_rst_n_paf1";
+                nvidia,schmitt = <TEGRA_PIN_DISABLE>;
+                nvidia,enable-input = <TEGRA_PIN_DISABLE>;
+                nvidia,io-hv = <TEGRA_PIN_ENABLE>;
+                nvidia,tristate = <TEGRA_PIN_DISABLE>;
+                nvidia,pull = <TEGRA_PIN_PULL_NONE>;
+            };
+        };
+    };
+...

Thierry Reding April 20, 2023, 5:06 p.m. UTC | #18

On Thu, Mar 30, 2023 at 06:32:30PM +0200, Thierry Reding wrote:
> On Thu, Mar 30, 2023 at 03:58:51PM +0200, Krzysztof Kozlowski wrote:
> > On 28/03/2023 14:39, Thierry Reding wrote:
> > > On Sun, Mar 26, 2023 at 02:19:45PM +0200, Krzysztof Kozlowski wrote:
> > >> On 23/03/2023 15:11, Thierry Reding wrote:
> > >>> On Wed, Mar 08, 2023 at 01:24:04PM +0100, Krzysztof Kozlowski wrote:
> > >>>> On 08/03/2023 12:45, Prathamesh Shete wrote:
> > >>>>>
> > >>>>>
> > >>>>>> -----Original Message-----
> > >>>>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > >>>>>> Sent: Wednesday, February 8, 2023 5:28 PM
> > >>>>>> To: Thierry Reding <thierry.reding@gmail.com>
> > >>>>>> Cc: Prathamesh Shete <pshete@nvidia.com>; Jonathan Hunter
> > >>>>>> <jonathanh@nvidia.com>; linus.walleij@linaro.org; robh+dt@kernel.org;
> > >>>>>> krzysztof.kozlowski+dt@linaro.org; devicetree@vger.kernel.org; linux-
> > >>>>>> tegra@vger.kernel.org; linux-gpio@vger.kernel.org; Suresh Mangipudi
> > >>>>>> <smangipudi@nvidia.com>
> > >>>>>> Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc
> > >>>>>>
> > >>>>>> External email: Use caution opening links or attachments
> > >>>>>>
> > >>>>>>
> > >>>>>> On 08/02/2023 12:24, Thierry Reding wrote:
> > >>>>>>> On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:
> > >>>>>>
> > >>>>>>
> > >>>>>>>>> +          type: object
> > >>>>>>>>> +          additionalProperties:
> > >>>>>>>>> +            properties:
> > >>>>>>>>> +              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.
> > >>>>>>>>
> > >>>>>>>> Define properties in top level, which points to the complexity of
> > >>>>>>>> your if-else, thus probably this should be split into two bindings.
> > >>>>>>>> Dunno, your other bindings repeat this pattern :(
> > >>>>>>>
> > >>>>>>> The property itself is already defined in the common schema found in
> > >>>>>>> nvidia,tegra-pinmux-common.yaml and we're overriding this here for
> > >>>>>>> each instance since each has its own set of pins.
> > >>>>>>>
> > >>>>>>> This was a compromise to avoid too many bindings. Originally I
> > >>>>>>> attempted to roll all Tegra pinctrl bindings into a single dt-schema,
> > >>>>>>> but that turned out truly horrible =) Splitting this into per-SoC
> > >>>>>>> bindings is already causing a lot of duplication in these files,
> > >>>>>>
> > >>>>>> What would be duplicated? Almost eveerything should be coming from
> > >>>>>> shared binding, so you will have only compatible,
> > >>>>>> patternProperties(pinmux) and nvidia,pins. And an example. Maybe I miss
> > >>>>>> something but I would say this would create many but very easy to read
> > >>>>>> bindings, referencing common pieces.
> > >>>>>>
> > >>>>>>> though splitting
> > >>>>>>> off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit
> > >>>>>>> with that already. Splitting this into per-instance bindings would
> > >>>>>>> effectively duplicate everything but the pin array here, so we kind of
> > >>>>>>> settled on this compromise for Tegra194.
> > >>>>>>
> > >>>>>> OK, but are you sure it is now readable? You have if:then: with
> > >>>>>> patternProperties: with additionalProperties: with properties: with
> > >>>>>> nvidia,pins.
> > >>>>> This is inline with the existing bindings and I think this is the compromise that was reached during review when the bindings were submitted,
> > >>>>
> > >>>> So the code might be totally unreadable, but it is inline with existing
> > >>>> code, thus it should stay unreadable. Great.
> > >>>
> > >>> I'd say this is very subjective. I personally don't find the current
> > >>> version hard to read, but that's maybe because I wrote it... =)
> > >>>
> > >>>>> offer to rework if a better alternative can be found, but that only makes sense if all the other bindings get changed as well, so I think it'd be good if we can merge in the same format as the existing bindings for now and change all of them later on.
> > >>>>
> > >>>> Cleanup should happen before adding new bindings.
> > >>>
> > >>> I don't recall the exact problems that I ran into last time, but I do
> > >>> remember that pulling out the common bindings to the very top-level was
> > >>> the main issue.
> > >>>
> > >>> If I understand correctly what you're saying, the main problem that
> > >>> makes this hard to read is the if and else constructs for AON/MAIN
> > >>> variants on Tegra194/Tegra234. These should be quite easy to pull out
> > >>> into separate bindings. I'll do that first and then see if there's
> > >>> anything that could be done to further improve things.
> > >>
> > >> One problem is allowing characters here which are not allowed. Second
> > >> problem is reluctance to change it with argument "existing bindings also
> > >> have this problem". It's explanation like "there is already bug like
> > >> this, so I am allowed to add similar one".
> > > 
> > > This is not a bug that we're trying to replicate. We're basing this
> > > binding on a existing bindings that were already reviewed upstream a
> > > long time ago. It uses a shared binding that's in use by these other
> > > bindings, so making any changes to this new binding means either the
> > > other ones need to be changed as well or we can't reuse the existing
> > > shared binding.
> > 
> > Are you sure? I did not see here conflict. The specific device binding
> > can narrow the pattern defined in common binding.
> > 
> > What's more, where do you see this pattern at all in shared binding?
> > 
> > I am sorry, but this does not fit my arguments at all. This pattern is
> > clearly wrong and argument to keep duplicating it because other (not
> > common!) binding also has it is by design invalid.
> > 
> > > 
> > >> Now third is that defining properties in allOf is not the style we want
> > >> to have, because it does not work with additionalProperties and is
> > >> difficult to read. Again using argument "existing code also does like
> > >> this" is a very poor argument.
> > > 
> > > As far as I can tell, it does work as expected in this case because
> > > we're not actually adding any *new* properties in the allOf/if branches.
> > 
> > The if:else: defines type and additionalProperties, so I am sorry but
> > this is not a readable solution.
> > 
> > > If we were, then yes, we would need to use unevaluatedProperties and
> > > that can get complicated. But again, in this case we're merely
> > > overriding existing properties with more specific values, which means
> > > that both the standard binding applies and then things are narrowed down
> > > by the values defined for each compatible.
> 
> Okay, so I'm starting to get a bit lost here, so maybe it's time for
> another proposal. I've tried splitting this up more so that we avoid
> the if/else block. Would you mind taking another look to see if the
> patch below is any more readable to you?
> 
> Thierry

Krzysztof,

any thoughts on this proposal?

Thanks,
Thierry

> From 00cb909f6d8732680d65cdb67e0573c8e6dc7b7a Mon Sep 17 00:00:00 2001
> From: Prathamesh Shete <pshete@nvidia.com>
> Date: Wed, 8 Mar 2023 17:14:30 +0530
> Subject: [PATCH] dt-bindings: pinctrl: tegra234
> 
> Add DT binding doc for Tegra234 pinmux driver.
> 
> Signed-off-by: Prathamesh Shete <pshete@nvidia.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  .../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
> +
> +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
> +
> +patternProperties:
> +  "^pinmux(-[a-z0-9-]+)?$":
> +    type: object
> +
> +    # pin groups
> +    additionalProperties:
> +      properties:
> +        nvidia,pins:
> +          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
> +...
> 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
> +
> +  reg:
> +    items:
> +      - description: pinmux registers
> +
> +patternProperties:
> +  "^pinmux(-[a-z0-9-]+)?$":
> +    type: object
> +    properties:
> +      phandle: true
> +
> +    # pin groups
> +    additionalProperties:
> +      $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.
> +
> +        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
> +
> +      required:
> +        - nvidia,pins
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +...
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> new file mode 100644
> index 000000000000..7f0bf3d75f35
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra234-pinmux.yaml
> @@ -0,0 +1,141 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/nvidia,tegra234-pinmux.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +$ref: nvidia,tegra234-pinmux-common.yaml
> +
> +title: NVIDIA Tegra234 Pinmux Controller
> +
> +maintainers:
> +  - Thierry Reding <thierry.reding@gmail.com>
> +  - Jon Hunter <jonathanh@nvidia.com>
> +
> +properties:
> +  compatible:
> +    const: nvidia,tegra234-pinmux
> +
> +  reg: true
> +
> +patternProperties:
> +  "^pinmux(-[a-z0-9-]+)?$":
> +    type: object
> +
> +    # pin groups
> +    additionalProperties:
> +      properties:
> +        nvidia,pins:
> +          items:
> +            enum: [ dap6_sclk_pa0, dap6_dout_pa1, dap6_din_pa2,
> +                    dap6_fs_pa3, dap4_sclk_pa4, dap4_dout_pa5,
> +                    dap4_din_pa6, dap4_fs_pa7, soc_gpio08_pb0,
> +                    qspi0_sck_pc0, qspi0_cs_n_pc1,
> +                    qspi0_io0_pc2, qspi0_io1_pc3, qspi0_io2_pc4,
> +                    qspi0_io3_pc5, qspi1_sck_pc6, qspi1_cs_n_pc7,
> +                    qspi1_io0_pd0, qspi1_io1_pd1, qspi1_io2_pd2,
> +                    qspi1_io3_pd3, eqos_txc_pe0, eqos_td0_pe1,
> +                    eqos_td1_pe2, eqos_td2_pe3, eqos_td3_pe4,
> +                    eqos_tx_ctl_pe5, eqos_rd0_pe6, eqos_rd1_pe7,
> +                    eqos_rd2_pf0, eqos_rd3_pf1, eqos_rx_ctl_pf2,
> +                    eqos_rxc_pf3, eqos_sma_mdio_pf4, eqos_sma_mdc_pf5,
> +                    soc_gpio13_pg0, soc_gpio14_pg1, soc_gpio15_pg2,
> +                    soc_gpio16_pg3, soc_gpio17_pg4, soc_gpio18_pg5,
> +                    soc_gpio19_pg6, soc_gpio20_pg7, soc_gpio21_ph0,
> +                    soc_gpio22_ph1, soc_gpio06_ph2, uart4_tx_ph3,
> +                    uart4_rx_ph4, uart4_rts_ph5, uart4_cts_ph6,
> +                    soc_gpio41_ph7, soc_gpio42_pi0, soc_gpio43_pi1,
> +                    soc_gpio44_pi2, gen1_i2c_scl_pi3, gen1_i2c_sda_pi4,
> +                    cpu_pwr_req_pi5, soc_gpio07_pi6,
> +                    sdmmc1_clk_pj0, sdmmc1_cmd_pj1, sdmmc1_dat0_pj2,
> +                    sdmmc1_dat1_pj3, sdmmc1_dat2_pj4, sdmmc1_dat3_pj5,
> +                    pex_l0_clkreq_n_pk0, pex_l0_rst_n_pk1,
> +                    pex_l1_clkreq_n_pk2, pex_l1_rst_n_pk3,
> +                    pex_l2_clkreq_n_pk4, pex_l2_rst_n_pk5,
> +                    pex_l3_clkreq_n_pk6, pex_l3_rst_n_pk7,
> +                    pex_l4_clkreq_n_pl0, pex_l4_rst_n_pl1,
> +                    pex_wake_n_pl2, soc_gpio34_pl3, dp_aux_ch0_hpd_pm0,
> +                    dp_aux_ch1_hpd_pm1, dp_aux_ch2_hpd_pm2,
> +                    dp_aux_ch3_hpd_pm3, soc_gpio55_pm4, soc_gpio36_pm5,
> +                    soc_gpio53_pm6, soc_gpio38_pm7, dp_aux_ch3_n_pn0,
> +                    soc_gpio39_pn1, soc_gpio40_pn2, dp_aux_ch1_p_pn3,
> +                    dp_aux_ch1_n_pn4, dp_aux_ch2_p_pn5, dp_aux_ch2_n_pn6,
> +                    dp_aux_ch3_p_pn7, extperiph1_clk_pp0,
> +                    extperiph2_clk_pp1, cam_i2c_scl_pp2, cam_i2c_sda_pp3,
> +                    soc_gpio23_pp4, soc_gpio24_pp5, soc_gpio25_pp6,
> +                    pwr_i2c_scl_pp7, pwr_i2c_sda_pq0, soc_gpio28_pq1,
> +                    soc_gpio29_pq2, soc_gpio30_pq3, soc_gpio31_pq4,
> +                    soc_gpio32_pq5, soc_gpio33_pq6, soc_gpio35_pq7,
> +                    soc_gpio37_pr0, soc_gpio56_pr1, uart1_tx_pr2,
> +                    uart1_rx_pr3, uart1_rts_pr4, uart1_cts_pr5,
> +                    soc_gpio61_pw0, soc_gpio62_pw1, gpu_pwr_req_px0,
> +                    cv_pwr_req_px1, gp_pwm2_px2, gp_pwm3_px3, uart2_tx_px4,
> +                    uart2_rx_px5, uart2_rts_px6, uart2_cts_px7, spi3_sck_py0,
> +                    spi3_miso_py1, spi3_mosi_py2, spi3_cs0_py3,
> +                    spi3_cs1_py4, uart5_tx_py5, uart5_rx_py6,
> +                    uart5_rts_py7, uart5_cts_pz0, usb_vbus_en0_pz1,
> +                    usb_vbus_en1_pz2, spi1_sck_pz3, spi1_miso_pz4,
> +                    spi1_mosi_pz5, spi1_cs0_pz6, spi1_cs1_pz7,
> +                    spi5_sck_pac0, spi5_miso_pac1, spi5_mosi_pac2,
> +                    spi5_cs0_pac3, soc_gpio57_pac4, soc_gpio58_pac5,
> +                    soc_gpio59_pac6, soc_gpio60_pac7, soc_gpio45_pad0,
> +                    soc_gpio46_pad1, soc_gpio47_pad2, soc_gpio48_pad3,
> +                    ufs0_ref_clk_pae0, ufs0_rst_n_pae1,
> +                    pex_l5_clkreq_n_paf0, pex_l5_rst_n_paf1,
> +                    pex_l6_clkreq_n_paf2, pex_l6_rst_n_paf3,
> +                    pex_l7_clkreq_n_pag0, pex_l7_rst_n_pag1,
> +                    pex_l8_clkreq_n_pag2, pex_l8_rst_n_pag3,
> +                    pex_l9_clkreq_n_pag4, pex_l9_rst_n_pag5,
> +                    pex_l10_clkreq_n_pag6, pex_l10_rst_n_pag7,
> +                    sdmmc1_comp, eqos_comp, qspi_comp,
> +                    # drive groups
> +                    drive_soc_gpio08_pb0, drive_soc_gpio36_pm5,
> +                    drive_soc_gpio53_pm6, drive_soc_gpio55_pm4,
> +                    drive_soc_gpio38_pm7, drive_soc_gpio39_pn1,
> +                    drive_soc_gpio40_pn2, drive_dp_aux_ch0_hpd_pm0,
> +                    drive_dp_aux_ch1_hpd_pm1, drive_dp_aux_ch2_hpd_pm2,
> +                    drive_dp_aux_ch3_hpd_pm3, drive_dp_aux_ch1_p_pn3,
> +                    drive_dp_aux_ch1_n_pn4, drive_dp_aux_ch2_p_pn5,
> +                    drive_dp_aux_ch2_n_pn6, drive_dp_aux_ch3_p_pn7,
> +                    drive_dp_aux_ch3_n_pn0, drive_pex_l2_clkreq_n_pk4,
> +                    drive_pex_wake_n_pl2, drive_pex_l1_clkreq_n_pk2,
> +                    drive_pex_l1_rst_n_pk3, drive_pex_l0_clkreq_n_pk0,
> +                    drive_pex_l0_rst_n_pk1, drive_pex_l2_rst_n_pk5,
> +                    drive_pex_l3_clkreq_n_pk6, drive_pex_l3_rst_n_pk7,
> +                    drive_pex_l4_clkreq_n_pl0, drive_pex_l4_rst_n_pl1,
> +                    drive_soc_gpio34_pl3, drive_pex_l5_clkreq_n_paf0,
> +                    drive_pex_l5_rst_n_paf1, drive_pex_l6_clkreq_n_paf2,
> +                    drive_pex_l6_rst_n_paf3, drive_pex_l10_clkreq_n_pag6,
> +                    drive_pex_l10_rst_n_pag7, drive_pex_l7_clkreq_n_pag0,
> +                    drive_pex_l7_rst_n_pag1, drive_pex_l8_clkreq_n_pag2,
> +                    drive_pex_l8_rst_n_pag3, drive_pex_l9_clkreq_n_pag4,
> +                    drive_pex_l9_rst_n_pag5, drive_sdmmc1_clk_pj0,
> +                    drive_sdmmc1_cmd_pj1, drive_sdmmc1_dat3_pj5,
> +                    drive_sdmmc1_dat2_pj4, drive_sdmmc1_dat1_pj3,
> +                    drive_sdmmc1_dat0_pj2 ]
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/pinctrl/pinctrl-tegra.h>
> +
> +    pinmux@2430000 {
> +        compatible = "nvidia,tegra234-pinmux";
> +        reg = <0x2430000 0x17000>;
> +
> +        pinctrl-names = "pex_rst";
> +        pinctrl-0 = <&pex_rst_c5_out_state>;
> +
> +        pex_rst_c5_out_state: pinmux-pex-rst-c5-out {
> +            pexrst {
> +                nvidia,pins = "pex_l5_rst_n_paf1";
> +                nvidia,schmitt = <TEGRA_PIN_DISABLE>;
> +                nvidia,enable-input = <TEGRA_PIN_DISABLE>;
> +                nvidia,io-hv = <TEGRA_PIN_ENABLE>;
> +                nvidia,tristate = <TEGRA_PIN_DISABLE>;
> +                nvidia,pull = <TEGRA_PIN_PULL_NONE>;
> +            };
> +        };
> +    };
> +...
> -- 
> 2.40.0
>

Krzysztof Kozlowski April 20, 2023, 5:16 p.m. UTC | #19

On 20/04/2023 19:06, Thierry Reding wrote:
>>>
>>>> If we were, then yes, we would need to use unevaluatedProperties and
>>>> that can get complicated. But again, in this case we're merely
>>>> overriding existing properties with more specific values, which means
>>>> that both the standard binding applies and then things are narrowed down
>>>> by the values defined for each compatible.
>>
>> Okay, so I'm starting to get a bit lost here, so maybe it's time for
>> another proposal. I've tried splitting this up more so that we avoid
>> the if/else block. Would you mind taking another look to see if the
>> patch below is any more readable to you?
>>
>> Thierry
> 
> Krzysztof,
> 
> any thoughts on this proposal?

I did not check the code thoroughly, but approach, assuming it works,
looks fine.

Best regards,
Krzysztof

[1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc

Checks

Commit Message

Comments

Patch