Add LPASS LPI pinctrl node required for Audio functionality on sc7280
based platforms.
Signed-off-by: Srinivasa Rao Mandadapu <[email protected]>
Co-developed-by: Venkata Prasad Potturu <[email protected]>
Signed-off-by: Venkata Prasad Potturu <[email protected]>
---
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 {
+ 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";
+ 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 {
+ 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
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 <[email protected]>
>> Co-developed-by: Venkata Prasad Potturu <[email protected]>
>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>> ---
>> 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.
>
>> + 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
>>
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 <[email protected]>
> Co-developed-by: Venkata Prasad Potturu <[email protected]>
> Signed-off-by: Venkata Prasad Potturu <[email protected]>
> ---
> 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?
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.
> + 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?
> + 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.
> + 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
>
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 <[email protected]>
>>> Co-developed-by: Venkata Prasad Potturu <[email protected]>
>>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>>> ---
>>> 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
>>>
On Tue, Apr 12, 2022 at 06:41:25PM +0530, Srinivasa Rao Mandadapu wrote:
>
> 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 <[email protected]>
> > > > Co-developed-by: Venkata Prasad Potturu <[email protected]>
> > > > Signed-off-by: Venkata Prasad Potturu <[email protected]>
> > > > ---
> > > > 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.
It helps to provide some context about the pins which might not be evident
from their short names like 'dmic01' or 'rx_swr'. A nice side effect is that
the pins/groups would grouped automatically together in alphabetic ordering.
In terms of 'redundancy' it is similar to 'qup_' prefix for the I2C/SPI/UART
pins.
> If we add lpass_ to all dmic nodes, some node names are too lengthy.
The longest would be like 'lpass_dmic01_sleep' or 'lpass_rx_swr_sleep', which
doesn't seem outrageous.
In any case it's not super important. If it bothers someone enough later
on they can always send a patch that changes it.
On Tue, Apr 12, 2022 at 06:18:33PM +0530, 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 <[email protected]>
> > > Co-developed-by: Venkata Prasad Potturu <[email protected]>
> > > Signed-off-by: Venkata Prasad Potturu <[email protected]>
> > > ---
> > > 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.
Agreed, these shouldn't be in the tlmm section, but it still seems nice to
separate them from device nodes.
Some sc7180-trogdor devices have a section like this:
/* PINCTRL - modifications to sc7180-trogdor.dtsi */
In any case I don't care too much about the IDP, but this might come up again
for sc7280-herobrine boards.
> 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.
Agreed, _clk isn't appropriate as long as it's a group of pins, that would be
for a label per pin.
> > 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.
Thanks, just wanted to make sure my understanding is correct.
On 4/13/2022 5:29 AM, Matthias Kaehlcke wrote:
Thanks for your time and valuable suggestions Matthias!!!
> On Tue, Apr 12, 2022 at 06:41:25PM +0530, Srinivasa Rao Mandadapu wrote:
>> 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 <[email protected]>
>>>>> Co-developed-by: Venkata Prasad Potturu <[email protected]>
>>>>> Signed-off-by: Venkata Prasad Potturu <[email protected]>
>>>>> ---
>>>>> 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.
> It helps to provide some context about the pins which might not be evident
> from their short names like 'dmic01' or 'rx_swr'. A nice side effect is that
> the pins/groups would grouped automatically together in alphabetic ordering.
>
> In terms of 'redundancy' it is similar to 'qup_' prefix for the I2C/SPI/UART
> pins.
Agree. Will change accordingly. similarly will append lpass_ torx/tx/va
mcro device node names.
>
>> If we add lpass_ to all dmic nodes, some node names are too lengthy.
> The longest would be like 'lpass_dmic01_sleep' or 'lpass_rx_swr_sleep', which
> doesn't seem outrageous.
>
> In any case it's not super important. If it bothers someone enough later
> on they can always send a patch that changes it.
Okay.
On Wed, Apr 13, 2022 at 08:12:20PM +0530, Srinivasa Rao Mandadapu wrote:
>
> On 4/13/2022 5:29 AM, Matthias Kaehlcke wrote:
> Thanks for your time and valuable suggestions Matthias!!!
> > On Tue, Apr 12, 2022 at 06:41:25PM +0530, Srinivasa Rao Mandadapu wrote:
> > > 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 <[email protected]>
> > > > > > Co-developed-by: Venkata Prasad Potturu <[email protected]>
> > > > > > Signed-off-by: Venkata Prasad Potturu <[email protected]>
> > > > > > ---
> > > > > > 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.
> > It helps to provide some context about the pins which might not be evident
> > from their short names like 'dmic01' or 'rx_swr'. A nice side effect is that
> > the pins/groups would grouped automatically together in alphabetic ordering.
> >
> > In terms of 'redundancy' it is similar to 'qup_' prefix for the I2C/SPI/UART
> > pins.
> Agree. Will change accordingly. similarly will append lpass_ torx/tx/va mcro
> device node names.
> >
> > > If we add lpass_ to all dmic nodes, some node names are too lengthy.
> > The longest would be like 'lpass_dmic01_sleep' or 'lpass_rx_swr_sleep', which
> > doesn't seem outrageous.
> >
> > In any case it's not super important. If it bothers someone enough later
> > on they can always send a patch that changes it.
> Okay.
I meant to say whether you change it or not is not super important, it's up
to you :)