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
