[v1] arm64: dts: mediatek: mt8186-corsola-voltorb: Merge speaker codec nodes

[PATCH] arm64: dts: mediatek: mt8186-corsola-voltorb: Merge speaker codec nodes

Posted by Chen-Yu Tsai 1 month, 2 weeks ago

The Voltorb device uses a speaker codec different from the original
Corsola device. When the Voltorb device tree was first added, the new
codec was added as a separate node when it should have just replaced the
existing one.

Merge the two nodes. The only differences are the compatible string and
the GPIO line property name. This keeps the device node path for the
speaker codec the same across the MT8186 Chromebook line.

Fixes: 321ad586e607 ("arm64: dts: mediatek: Add MT8186 Voltorb Chromebooks")
Cc: <stable@vger.kernel.org>
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
 .../dts/mediatek/mt8186-corsola-voltorb.dtsi  | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
index 52ec58128d56..fbcd97069df9 100644
--- a/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
@@ -10,12 +10,6 @@
 
 / {
 	chassis-type = "laptop";
-
-	max98360a: max98360a {
-		compatible = "maxim,max98360a";
-		sdmode-gpios = <&pio 150 GPIO_ACTIVE_HIGH>;
-		#sound-dai-cells = <0>;
-	};
 };
 
 &cpu6 {
@@ -59,19 +53,14 @@ &cluster1_opp_15 {
 	opp-hz = /bits/ 64 <2200000000>;
 };
 
-&rt1019p{
-	status = "disabled";
+&rt1019p {
+	compatible = "maxim,max98360a";
+	sdmode-gpios = <&pio 150 GPIO_ACTIVE_HIGH>;
+	/delete-property/ sdb-gpios;
 };
 
 &sound {
 	compatible = "mediatek,mt8186-mt6366-rt5682s-max98360-sound";
-	status = "okay";
-
-	spk-hdmi-playback-dai-link {
-		codec {
-			sound-dai = <&it6505dptx>, <&max98360a>;
-		};
-	};
 };
 
 &spmi {
-- 
2.47.0.rc0.187.ge670bccf7e-goog

Re: [PATCH] arm64: dts: mediatek: mt8186-corsola-voltorb: Merge speaker codec nodes

Posted by AngeloGioacchino Del Regno 1 month, 2 weeks ago

Il 08/10/24 10:21, Chen-Yu Tsai ha scritto:
> The Voltorb device uses a speaker codec different from the original
> Corsola device. When the Voltorb device tree was first added, the new
> codec was added as a separate node when it should have just replaced the
> existing one.
> 
> Merge the two nodes. The only differences are the compatible string and
> the GPIO line property name. This keeps the device node path for the
> speaker codec the same across the MT8186 Chromebook line.

Ok, I agree...

But, at this point, can we rename `rt1019p` to `speaker_codec` instead?

Imo, that makes a bit more sense as a phandle, as it reads generic and it's not
screaming "I'm RT1019P" on dts(i) files where it's actually not.

> 
> Fixes: 321ad586e607 ("arm64: dts: mediatek: Add MT8186 Voltorb Chromebooks")
 > Cc: <stable@vger.kernel.org>

Well, that's not a fix - it's an improvement, so we can avoid this Fixes tag :-)

Cheers,
Angelo

> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
>   .../dts/mediatek/mt8186-corsola-voltorb.dtsi  | 19 ++++---------------
>   1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
> index 52ec58128d56..fbcd97069df9 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
> @@ -10,12 +10,6 @@
>   
>   / {
>   	chassis-type = "laptop";
> -
> -	max98360a: max98360a {
> -		compatible = "maxim,max98360a";
> -		sdmode-gpios = <&pio 150 GPIO_ACTIVE_HIGH>;
> -		#sound-dai-cells = <0>;
> -	};
>   };
>   
>   &cpu6 {
> @@ -59,19 +53,14 @@ &cluster1_opp_15 {
>   	opp-hz = /bits/ 64 <2200000000>;
>   };
>   
> -&rt1019p{
> -	status = "disabled";
> +&rt1019p {
> +	compatible = "maxim,max98360a";
> +	sdmode-gpios = <&pio 150 GPIO_ACTIVE_HIGH>;
> +	/delete-property/ sdb-gpios;
>   };
>   
>   &sound {
>   	compatible = "mediatek,mt8186-mt6366-rt5682s-max98360-sound";
> -	status = "okay";
> -
> -	spk-hdmi-playback-dai-link {
> -		codec {
> -			sound-dai = <&it6505dptx>, <&max98360a>;
> -		};
> -	};
>   };
>   
>   &spmi {

Re: [PATCH] arm64: dts: mediatek: mt8186-corsola-voltorb: Merge speaker codec nodes

Posted by Chen-Yu Tsai 1 month, 2 weeks ago

On Tue, Oct 8, 2024 at 4:51 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 08/10/24 10:21, Chen-Yu Tsai ha scritto:
> > The Voltorb device uses a speaker codec different from the original
> > Corsola device. When the Voltorb device tree was first added, the new
> > codec was added as a separate node when it should have just replaced the
> > existing one.
> >
> > Merge the two nodes. The only differences are the compatible string and
> > the GPIO line property name. This keeps the device node path for the
> > speaker codec the same across the MT8186 Chromebook line.
>
> Ok, I agree...
>
> But, at this point, can we rename `rt1019p` to `speaker_codec` instead?
>
> Imo, that makes a bit more sense as a phandle, as it reads generic and it's not
> screaming "I'm RT1019P" on dts(i) files where it's actually not.

Works for me.

> >
> > Fixes: 321ad586e607 ("arm64: dts: mediatek: Add MT8186 Voltorb Chromebooks")
>  > Cc: <stable@vger.kernel.org>
>
> Well, that's not a fix - it's an improvement, so we can avoid this Fixes tag :-)

I'd like to see it backported though, so we minimize the different DTS files.
Guess I'll add Cc stable instead? Not sure if that works without a Fixes tag.

ChenYu

> Cheers,
> Angelo
>
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> >   .../dts/mediatek/mt8186-corsola-voltorb.dtsi  | 19 ++++---------------
> >   1 file changed, 4 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
> > index 52ec58128d56..fbcd97069df9 100644
> > --- a/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
> > +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
> > @@ -10,12 +10,6 @@
> >
> >   / {
> >       chassis-type = "laptop";
> > -
> > -     max98360a: max98360a {
> > -             compatible = "maxim,max98360a";
> > -             sdmode-gpios = <&pio 150 GPIO_ACTIVE_HIGH>;
> > -             #sound-dai-cells = <0>;
> > -     };
> >   };
> >
> >   &cpu6 {
> > @@ -59,19 +53,14 @@ &cluster1_opp_15 {
> >       opp-hz = /bits/ 64 <2200000000>;
> >   };
> >
> > -&rt1019p{
> > -     status = "disabled";
> > +&rt1019p {
> > +     compatible = "maxim,max98360a";
> > +     sdmode-gpios = <&pio 150 GPIO_ACTIVE_HIGH>;
> > +     /delete-property/ sdb-gpios;
> >   };
> >
> >   &sound {
> >       compatible = "mediatek,mt8186-mt6366-rt5682s-max98360-sound";
> > -     status = "okay";
> > -
> > -     spk-hdmi-playback-dai-link {
> > -             codec {
> > -                     sound-dai = <&it6505dptx>, <&max98360a>;
> > -             };
> > -     };
> >   };
> >
> >   &spmi {
>

Re: [PATCH] arm64: dts: mediatek: mt8186-corsola-voltorb: Merge speaker codec nodes

Posted by AngeloGioacchino Del Regno 1 month, 2 weeks ago

Il 08/10/24 12:14, Chen-Yu Tsai ha scritto:
> On Tue, Oct 8, 2024 at 4:51 PM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 08/10/24 10:21, Chen-Yu Tsai ha scritto:
>>> The Voltorb device uses a speaker codec different from the original
>>> Corsola device. When the Voltorb device tree was first added, the new
>>> codec was added as a separate node when it should have just replaced the
>>> existing one.
>>>
>>> Merge the two nodes. The only differences are the compatible string and
>>> the GPIO line property name. This keeps the device node path for the
>>> speaker codec the same across the MT8186 Chromebook line.
>>
>> Ok, I agree...
>>
>> But, at this point, can we rename `rt1019p` to `speaker_codec` instead?
>>
>> Imo, that makes a bit more sense as a phandle, as it reads generic and it's not
>> screaming "I'm RT1019P" on dts(i) files where it's actually not.
> 
> Works for me.
> 
>>>
>>> Fixes: 321ad586e607 ("arm64: dts: mediatek: Add MT8186 Voltorb Chromebooks")
>>   > Cc: <stable@vger.kernel.org>
>>
>> Well, that's not a fix - it's an improvement, so we can avoid this Fixes tag :-)
> 
> I'd like to see it backported though, so we minimize the different DTS files.
> Guess I'll add Cc stable instead? Not sure if that works without a Fixes tag.
> 

Well, try to send it to the stable mailing list as well...
I fully understand your concern but backporting is for fixes and *not* for
improvements, so I doubt that you'll get that backported.

It's Sasha's call then, anyway.

> ChenYu
> 
>> Cheers,
>> Angelo
>>
>>> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
>>> ---
>>>    .../dts/mediatek/mt8186-corsola-voltorb.dtsi  | 19 ++++---------------
>>>    1 file changed, 4 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi b/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
>>> index 52ec58128d56..fbcd97069df9 100644
>>> --- a/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
>>> +++ b/arch/arm64/boot/dts/mediatek/mt8186-corsola-voltorb.dtsi
>>> @@ -10,12 +10,6 @@
>>>
>>>    / {
>>>        chassis-type = "laptop";
>>> -
>>> -     max98360a: max98360a {
>>> -             compatible = "maxim,max98360a";
>>> -             sdmode-gpios = <&pio 150 GPIO_ACTIVE_HIGH>;
>>> -             #sound-dai-cells = <0>;
>>> -     };
>>>    };
>>>
>>>    &cpu6 {
>>> @@ -59,19 +53,14 @@ &cluster1_opp_15 {
>>>        opp-hz = /bits/ 64 <2200000000>;
>>>    };
>>>
>>> -&rt1019p{
>>> -     status = "disabled";
>>> +&rt1019p {
>>> +     compatible = "maxim,max98360a";
>>> +     sdmode-gpios = <&pio 150 GPIO_ACTIVE_HIGH>;
>>> +     /delete-property/ sdb-gpios;
>>>    };
>>>
>>>    &sound {
>>>        compatible = "mediatek,mt8186-mt6366-rt5682s-max98360-sound";
>>> -     status = "okay";
>>> -
>>> -     spk-hdmi-playback-dai-link {
>>> -             codec {
>>> -                     sound-dai = <&it6505dptx>, <&max98360a>;
>>> -             };
>>> -     };
>>>    };
>>>
>>>    &spmi {
>>