Re: [PATCH v2 1/2] dt-bindings: pinctrl: qcom: Add SDX75 pinctrl devicetree compatible

[Rohit Agarwal <quic_rohiagar@quicinc.com>]

On 4/21/2023 3:38 PM, Krzysztof Kozlowski wrote:
> On 21/04/2023 11:43, Rohit Agarwal wrote:
>> Add device tree binding Documentation details for Qualcomm SDX75
>> pinctrl driver.
>>
>> Signed-off-by: Rohit Agarwal <quic_rohiagar@quicinc.com>
> Thank you for your patch. There is something to discuss/improve.
>
>> +properties:
>> +  compatible:
>> +    const: qcom,sdx75-tlmm
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts: true
>> +  interrupt-controller: true
>> +  "#interrupt-cells": true
>> +  gpio-controller: true
>> +
>> +  gpio-reserved-ranges:
>> +    minItems: 1
>> +    maxItems: 105
>> +
>> +  gpio-line-names:
>> +    maxItems: 133
> If you have 210 GPIOs, then this should be 210.
>
>> +
>> +  "#gpio-cells": true
>> +  gpio-ranges: true
>> +  wakeup-parent: true
>> +
>> +patternProperties:
>> +  "-state$":
>> +    oneOf:
>> +      - $ref: "#/$defs/qcom-sdx75-tlmm-state"
>> +      - patternProperties:
>> +          "-pins$":
>> +            $ref: "#/$defs/qcom-sdx75-tlmm-state"
>> +        additionalProperties: false
>> +
>> +$defs:
>> +  qcom-sdx75-tlmm-state:
>> +    type: object
>> +    description:
>> +      Pinctrl node's client devices use subnodes for desired pin configuration.
>> +      Client device subnodes use below standard properties.
>> +    $ref: qcom,tlmm-common.yaml#/$defs/qcom-tlmm-state
> unevaluatedProperties: false
>> +
>> +    properties:
>> +      pins:
>> +        description:
>> +          List of gpio pins affected by the properties specified in this
>> +          subnode.
>> +        items:
>> +          oneOf:
>> +            - pattern: "^gpio([0-9]|[1-9][0-9]|1[0-9][0-9]|20[0-9])$"
> This says you have 210 GPIOs.
>
>> +            - enum: [ ufs_reset, sdc2_clk, sdc2_cmd, sdc2_data ]
> Keep these four enum values sorted alphabetically.
>
>> +        minItems: 1
>> +        maxItems: 36
>> +
>> +      function:
>> +        description:
>> +          Specify the alternative function to be configured for the specified
>> +          pins.
>> +        enum: [ gpio, eth0_mdc, eth0_mdio, eth1_mdc, eth1_mdio,
>> +                qlink0_wmss_reset, qlink1_wmss_reset, rgmii_rxc, rgmii_rxd0,
>> +                rgmii_rxd1, rgmii_rxd2, rgmii_rxd3,rgmii_rx_ctl, rgmii_txc,
>> +                rgmii_txd0, rgmii_txd1, rgmii_txd2, rgmii_txd3, rgmii_tx_ctl,
>> +                adsp_ext_vfr, atest_char_start, atest_char_status0,
>> +                atest_char_status1, atest_char_status2, atest_char_status3,
>> +                audio_ref_clk, bimc_dte_test0, bimc_dte_test1,
>> +                char_exec_pending, char_exec_release, coex_uart2_rx,
>> +                coex_uart2_tx, coex_uart_rx, coex_uart_tx, cri_trng_rosc,
>> +                cri_trng_rosc0, cri_trng_rosc1, dbg_out_clk, ddr_bist_complete,
>> +                ddr_bist_fail, ddr_bist_start, ddr_bist_stop, ddr_pxi0_test,
>> +                ebi0_wrcdc_dq2, ebi0_wrcdc_dq3, ebi2_a_d, ebi2_lcd_cs,
>> +                ebi2_lcd_reset, ebi2_lcd_te, emac0_mcg_pst0, emac0_mcg_pst1,
>> +                emac0_mcg_pst2, emac0_mcg_pst3, emac0_ptp_aux, emac0_ptp_pps,
>> +                emac1_mcg_pst0, emac1_mcg_pst1, emac1_mcg_pst2, emac1_mcg_pst3,
>> +                emac1_ptp_aux0, emac1_ptp_aux1, emac1_ptp_aux2, emac1_ptp_aux3,
>> +                emac1_ptp_pps0, emac1_ptp_pps1, emac1_ptp_pps2, emac1_ptp_pps3,
>> +                emac_cdc_dtest0, emac_cdc_dtest1, emac_pps_in, ext_dbg_uart,
>> +                gcc_125_clk, gcc_gp1_clk, gcc_gp2_clk, gcc_gp3_clk,
>> +                gcc_plltest_bypassnl, gcc_plltest_resetn, i2s_mclk,
>> +                jitter_bist_ref, ldo_en, ldo_update, m_voc_ext, mgpi_clk_req,
>> +                native0, native1, native2, native3, native_char_start,
>> +                native_tsens_osc, native_tsense_pwm1, nav_dr_sync, nav_gpio_0,
>> +                nav_gpio_1, nav_gpio_2, nav_gpio_3, pa_indicator_1, pci_e_rst,
>> +                pcie0_clkreq_n, pcie1_clkreq_n, pcie2_clkreq_n, pll_bist_sync,
>> +                pll_clk_aux, pll_ref_clk, pri_mi2s_data0, pri_mi2s_data1,
>> +                pri_mi2s_sck, pri_mi2s_ws, prng_rosc_test0, prng_rosc_test1,
>> +                prng_rosc_test2, prng_rosc_test3, qdss_cti_trig0,
>> +                qdss_cti_trig1, qdss_gpio_traceclk, qdss_gpio_tracectl,
>> +                qdss_gpio_tracedata0, qdss_gpio_tracedata1,
>> +                qdss_gpio_tracedata10, qdss_gpio_tracedata11,
>> +                qdss_gpio_tracedata12, qdss_gpio_tracedata13,
>> +                qdss_gpio_tracedata14, qdss_gpio_tracedata15,
>> +                qdss_gpio_tracedata2, qdss_gpio_tracedata3,
>> +                qdss_gpio_tracedata4, qdss_gpio_tracedata5,
>> +                qdss_gpio_tracedata6, qdss_gpio_tracedata7,
>> +                qdss_gpio_tracedata8, qdss_gpio_tracedata9, qlink0_b_en,
>> +                qlink0_b_req, qlink0_l_en, qlink0_l_req, qlink1_l_en,
>> +                qlink1_l_req, qup_se0_l0, qup_se0_l1, qup_se0_l2, qup_se0_l3,
>> +                qup_se1_l2, qup_se1_l3, qup_se2_l0, qup_se2_l1, qup_se2_l2,
>> +                qup_se2_l3, qup_se3_l0, qup_se3_l1, qup_se3_l2, qup_se3_l3,
>> +                qup_se4_l2, qup_se4_l3, qup_se5_l0, qup_se5_l1, qup_se6_l0,
>> +                qup_se6_l1, qup_se6_l2, qup_se6_l3, qup_se7_l0, qup_se7_l1,
>> +                qup_se7_l2, qup_se7_l3, qup_se8_l2, qup_se8_l3, sdc1_tb_trig,
>> +                sdc2_tb_trig, sec_mi2s_data0, sec_mi2s_data1, sec_mi2s_sck,
>> +                sec_mi2s_ws, sgmii_phy_intr0, sgmii_phy_intr1, spmi_coex_clk,
>> +                spmi_coex_data, spmi_vgi_hwevent, tgu_ch0_trigout,
>> +                tri_mi2s_data0, tri_mi2s_data1, tri_mi2s_sck, tri_mi2s_ws,
>> +                uim1_clk, uim1_data, uim1_present, uim1_reset, uim2_clk,
>> +                uim2_data, uim2_present, uim2_reset, usb2phy_ac_en,
>> +                vsense_trigger_mirnat]
>> +
>> +      bias-disable: true
>> +      bias-pull-down: true
>> +      bias-pull-up: true
>> +      drive-strength: true
>> +      input-enable: true
> This is not allowed. Please rebase on pinctrl maintainer tree or next.
Will do this.
>
>> +      output-high: true
>> +      output-low: true
>> +
>> +    required:
>> +      - pins
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    tlmm: pinctrl@f100000 {
>> +        compatible = "qcom,sdx75-tlmm";
>> +        reg = <0x0f100000 0x300000>;
>> +        gpio-controller;
>> +        #gpio-cells = <2>;
>> +        gpio-ranges = <&tlmm 0 0 134>;
> Wrong number of pins. You have 210, right? This should be number of
> GPIOs + optionally UFS reset.
Thanks for reviewing the patch.
Actually it has 133 pins. Ok. Let me update the above property as well.
And just checked there is no ufs reset pin. So it should be removed 
completely.

Thanks,
Rohit.
>
>
> Best regards,
> Krzysztof
>