Re: [PATCH v9 4/4] arm64: dts: qcom: sc7280: include sc7280-herobrine-audio-rt5682.dtsi in evoker

[Doug Anderson]

Hi,

On Mon, Oct 31, 2022 at 3:20 AM Sheng-Liang Pan
<sheng-liang.pan@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> Include sc7280-herobrine-audio-rt5682.dtsi in evoker
> as it uses rt5682 codec.
>
> Signed-off-by: Sheng-Liang Pan <sheng-liang.pan@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>
> ---
>
> Changes in v9:
> - new patch for evoker include rt5682.dtsi
>
>  .../boot/dts/qcom/sc7280-herobrine-evoker.dts | 132 ++++++++++++++++++
>  1 file changed, 132 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts
> index dcdd4eecfe670..d54c07ff35f4f 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-evoker.dts

Why are you modifying the "evoker.dts" file and not the "evoker.dtsi"
file? Does the LTE SKU not have rt5682 audio?


> @@ -8,8 +8,140 @@
>  /dts-v1/;
>
>  #include "sc7280-herobrine-evoker.dtsi"
> +#include "sc7280-herobrine-audio-rt5682.dtsi"
> +
>
>  / {
>         model = "Google Evoker";
>         compatible = "google,evoker", "qcom,sc7280";
>  };
> +
> +&sound {

This is sorted incorrectly. "&sound" sorts under "&lpass".

...though looking closer at everything, I suspect that you're going to
need to reorganize things anyway. See below.


> +       model = "sc7280-rt5682-max98360a-3mic";
> +
> +       audio-routing =
> +               "VA DMIC0", "vdd-micb",
> +               "VA DMIC1", "vdd-micb",
> +               "VA DMIC2", "vdd-micb",
> +               "VA DMIC3", "vdd-micb",
> +
> +               "Headphone Jack", "HPOL",
> +               "Headphone Jack", "HPOR";
> +               #address-cells = <1>;
> +               #size-cells = <0>;
> +
> +       dai-link@0 {
> +               link-name = "MAX98360";
> +               reg = <0>;
> +
> +               cpu {
> +                       sound-dai = <&lpass_cpu MI2S_SECONDARY>;
> +               };
> +
> +               codec {
> +                       sound-dai = <&max98360a>;
> +               };
> +       };

The way you have things organized right now, technically the entire
"dai-link@0" here should be removed. Why? You're already getting it
from "sc7280-herobrine-audio-rt5682.dtsi". ...so I would say just to
remove it, but... (see below for more).


> +       dai-link@1 {
> +               link-name = "DisplayPort";
> +               reg = <1>;
> +
> +               cpu {
> +                       sound-dai = <&lpass_cpu LPASS_DP_RX>;
> +               };
> +
> +               codec {
> +                       sound-dai = <&mdss_dp>;
> +               };
> +       };

So dai-link@1 is confusing. You're fully overriding all of the
properties that you inherited by including
"sc7280-herobrine-audio-rt5682.dtsi". It happens to work because you
override _all_ of the properties, but it's a sign that something isn't
quite right. It feels like you are diverging _too much_ from
"sc7280-herobrine-audio-rt5682.dtsi"

My suggestion is that, instead of doing it this way, you:

1. Fully copy "sc7280-herobrine-audio-rt5682.dtsi" to a new file
called "sc7280-herobrine-audio-rt5682-3mic.dtsi".

2. Accept that there will be some duplication between the normal and
the 3mic version. I think there are enough differences that the
duplication is better than the spaghetti of overrides.

3. Try to make it so that diffs between the normal and "3mic" version
are as clean as possible so someone comparing the files can see the
exact differences.


> +       dai-link@2 {
> +               link-name = "ALC5682";
> +               reg = <1>;

Something is wrong with the above node. Your unit address (dai-link@2)
doesn't match your reg (reg = <1>;).