Date:   Fri, 21 Apr 2023 16:05:22 +0530
From:   Rohit Agarwal <quic_rohiagar@...cinc.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        <agross@...nel.org>, <andersson@...nel.org>,
        <konrad.dybcio@...aro.org>, <linus.walleij@...aro.org>,
        <robh+dt@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <richardcochran@...il.com>, <manivannan.sadhasivam@...aro.org>
CC:     <linux-arm-msm@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <netdev@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] dt-bindings: pinctrl: qcom: Add SDX75 pinctrl
 devicetree compatible


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@...cinc.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@...0000 {
>> +        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
>

Powered by blists - more mailing lists