Re: [PATCH v7 2/2] arm64: dts: qcom: sc7280: add lpass lpi pin controller node

[Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>]

On 4/12/2022 6:18 PM, Srinivasa Rao Mandadapu wrote:
>
> On 4/12/2022 1:02 AM, Matthias Kaehlcke wrote:
> Thanks for your time Matthias!!!
>> On Mon, Apr 11, 2022 at 07:23:04PM +0530, Srinivasa Rao Mandadapu wrote:
>>> Add LPASS LPI pinctrl node required for Audio functionality on sc7280
>>> based platforms.
>>>
>>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
>>> Co-developed-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
>>> Signed-off-by: Venkata Prasad Potturu <quic_potturu@quicinc.com>
>>> ---
>>>   arch/arm64/boot/dts/qcom/sc7280-idp.dtsi |  84 
>>> ++++++++++++++++++++++++
>>>   arch/arm64/boot/dts/qcom/sc7280.dtsi     | 107 
>>> +++++++++++++++++++++++++++++++
>>>   2 files changed, 191 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi 
>>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> index 4ba2274..ea751dc 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>>> @@ -238,6 +238,90 @@
>>>       modem-init;
>>>   };
>>>   +&dmic01 {
>> Shouldn't these nodes be in the PINCTRL section at their respective
>> positions in alphabetical order?
>
> These are not part of tlmm pin control section. These are part of 
> lpass_tlmm section.
>
> In your previous comment you asked to remove &lpass_tlmm. Hence 
> brought out.
>
>>
>> nit: since you are keeping the groups the group names are a bit 
>> generic IMO.
>> e.g. it is fairly obvious that 'dmic01_clk' refers to a clock pin, 
>> however
>> just 'dmic01' is a bit vague. You could consider adding the prefix 
>> 'lpass_'
>> to the group names for more clarity.
> as dmic01 has both clk and data section, I don't think keeping clk is 
> appropriate here.

As these nodes are part of SC7280, i.e. qcom specific chipset, I feel 
lpass_ is redundant.

If we add lpass_ to all dmic nodes, some node names are too lengthy.

>>
>>> +    clk {
>>> +        drive-strength = <8>;
>>> +    };
>>> +};
>>> +
>>> +&dmic01_sleep {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        bias-disable;
>>> +    };
>>> +
>>> +    data {
>>> +        pull-down;
>>> +    };
>>> +};
>>> +
>>> +&dmic23 {
>>> +    clk {
>>> +        drive-strength = <8>;
>>> +    };
>>> +};
>>> +
>>> +&dmic23_sleep {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        bias-disable;
>>> +    };
>>> +
>>> +    data {
>>> +        pull-down;
>>> +    };
>>> +};
>>> +
>>> +&rx_swr {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        slew-rate = <1>;
>>> +        bias-disable;
>>> +    };
>>> +
>>> +    data {
>>> +        drive-strength = <2>;
>>> +        slew-rate = <1>;
>>> +        bias-bus-hold;
>>> +    };
>>> +};
>>> +
>>> +&rx_swr_sleep {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        bias-pull-down;
>>> +    };
>>> +
>>> +    data {
>>> +        drive-strength = <2>;
>>> +        bias-pull-down;
>>> +    };
>>> +};
>>> +
>>> +&tx_swr {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        slew-rate = <1>;
>>> +        bias-disable;
>>> +    };
>>> +
>>> +    data {
>>> +        slew-rate = <1>;
>>> +        bias-bus-hold;
>>> +    };
>>> +};
>>> +
>>> +&tx_swr_sleep {
>>> +    clk {
>>> +        drive-strength = <2>;
>>> +        bias-pull-down;
>>> +    };
>>> +
>>> +    data {
>>> +        bias-bus-hold;
>>> +    };
>>> +};
>>> +
>>>   &pcie1 {
>>>       status = "okay";
>>>       perst-gpio = <&tlmm 2 GPIO_ACTIVE_LOW>;
>>> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi 
>>> b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> index 8099c80..c692420 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
>>> @@ -1987,6 +1987,113 @@
>>>               qcom,bcm-voters = <&apps_bcm_voter>;
>>>           };
>>>   +        lpass_tlmm: pinctrl@33c0000 {
>>> +            compatible = "qcom,sc7280-lpass-lpi-pinctrl";
>>> +            reg = <0 0x033c0000 0x0 0x20000>,
>>> +                <0 0x03550000 0x0 0x10000>;
>>> +            gpio-controller;
>>> +            #gpio-cells = <2>;
>>> +            gpio-ranges = <&lpass_tlmm 0 0 15>;
>>> +
>>> +            #clock-cells = <1>;
>>> +
>>> +            dmic01: dmic01 {
>>> +                clk {
>>> +                    pins = "gpio6";
>>  From the schematics I interpret that the LPASS GPIOs 0-9 are mapped 
>> to the
>> SC7280 GPIOs 144-153. Is that correct?
> Yes. But we refer with GPIOs 0-9 in driver.
>>
>>> +                    function = "dmic1_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio7";
>>> +                    function = "dmic1_data";
>>> +                };
>>> +            };
>>> +
>>> +            dmic01_sleep: dmic01-sleep {
>>> +                clk {
>>> +                    pins = "gpio6";
>>> +                    function = "dmic1_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio7";
>>> +                    function = "dmic1_data";
>>> +                };
>>> +            };
>>> +
>>> +            dmic23: dmic23 {
>>> +                clk {
>>> +                    pins = "gpio8";
>>> +                    function = "dmic2_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio9";
>>> +                    function = "dmic2_data";
>>> +                };
>>> +            };
>>> +
>>> +            dmic23_sleep: dmic23_sleep {
>> s/dmic23_sleep/dmic23-sleep/ for the node name.
> Okay.
>>
>>> +                clk {
>>> +                    pins = "gpio8";
>>> +                    function = "dmic2_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio9";
>>> +                    function = "dmic2_data";
>>> +                };
>>> +            };
>>> +
>>> +            rx_swr: rx-swr {
>>> +                clk {
>>> +                    pins = "gpio3";
>>> +                    function = "swr_rx_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio4", "gpio5";
>>> +                    function = "swr_rx_data";
>>> +                };
>>> +            };
>>> +
>>> +            rx_swr_sleep: rx-swr-sleep {
>>> +                clk {
>>> +                    pins = "gpio3";
>>> +                    function = "swr_rx_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio4", "gpio5";
>>> +                    function = "swr_rx_data";
>>> +                };
>>> +            };
>>> +
>>> +            tx_swr: tx-swr {
>>> +                clk {
>>> +                    pins = "gpio0";
>>> +                    function = "swr_tx_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio1", "gpio2", "gpio14";
>>> +                    function = "swr_tx_data";
>>> +                };
>>> +            };
>>> +
>>> +            tx_swr_sleep: tx-swr-sleep {
>>> +                clk {
>>> +                    pins = "gpio0";
>>> +                    function = "swr_tx_clk";
>>> +                };
>>> +
>>> +                data {
>>> +                    pins = "gpio1", "gpio2", "gpio14";
>>> +                    function = "swr_tx_data";
>>> +                };
>>> +            };
>>> +        };
>>> +
>>>           gpu: gpu@3d00000 {
>>>               compatible = "qcom,adreno-635.0", "qcom,adreno";
>>>               reg = <0 0x03d00000 0 0x40000>,
>>> -- 
>>> 2.7.4
>>>