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

[Krzysztof Kozlowski]

On 21/04/2023 12:35, Rohit Agarwal wrote:
> 
> 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@xxxxxxxxxxx>
>> 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.

Then the pattern for GPIOs should be narrowed and gpio-reserved-ranges
should be (half + 1), so 67.

Best regards,
Krzysztof