[PATCH 0/4] Update properties in two ASoC DT bindings

[PATCH 0/4] Update properties in two ASoC DT bindings

[Fei Shao]

Hi maintainers,

This series includes four patches to update two ASoC DT bindings. The
goal is to fix dtbs_check warnings found with DAI links and audio
routing configuration on some MediaTek SoCs (MT8195 and MT8188).

This series doesn't introduce new properties - all are either commonly
described in existing bindings or are available but undocumented.

Patches 1 and 2 focus on mediatek,mt8188-mt6359.yaml, adding two
vendor-specific properties and updating the DAI link pattern to be more
generic.

Patches 3 and 4 focus on maxim,max98390.yaml, referencing
dai-common.yaml and documenting a hidden vendor-specific property.

Regards,
Fei


Fei Shao (4):
  ASoC: dt-bindings: mediatek,mt8188-mt6359: Add adsp and dai-link
    properties
  ASoC: dt-bindings: mediatek,mt8188-mt6359: Update DAI link node
    pattern
  ASoC: dt-bindings: maxim,max98390: Refernce common DAI properties
  ASoC: dt-bindings: maxim,max98390: Document maxim,dsm_param_name
    property

 .../devicetree/bindings/sound/maxim,max98390.yaml  |  9 ++++++++-
 .../bindings/sound/mediatek,mt8188-mt6359.yaml     | 14 ++++++++++++--
 2 files changed, 20 insertions(+), 3 deletions(-)

-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH 1/4] ASoC: dt-bindings: mediatek,mt8188-mt6359: Add adsp and dai-link properties

[Fei Shao]

Add "mediatek,adsp" property for the ADSP handle if ADSP is enabled on
the platform.
Add "mediatek,dai-link" property for the required DAI links to configure
sound card.

Both properties are commonly used in the MediaTek sound card driver.

Signed-off-by: Fei Shao <fshao@chromium.org>
---

 .../bindings/sound/mediatek,mt8188-mt6359.yaml         | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
index f94ad0715e32..701cedfa38d2 100644
--- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
+++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
@@ -29,6 +29,16 @@ properties:
     $ref: /schemas/types.yaml#/definitions/phandle
     description: The phandle of MT8188 ASoC platform.
 
+  mediatek,adsp:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: The phandle of MT8188 ADSP platform.
+
+  mediatek,dai-link:
+    $ref: /schemas/types.yaml#/definitions/string-array
+    description:
+      A list of the desired dai-links in the sound card. Each entry is a
+      name defined in the machine driver.
+
 patternProperties:
   "^dai-link-[0-9]+$":
     type: object
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH 2/4] ASoC: dt-bindings: mediatek,mt8188-mt6359: Update DAI link node pattern

[Fei Shao]

The associated machine driver is not dependent on the format of DAI link
node names. This means we are allowed to use more descriptive names
instead of indices without impacting functionality.

Update the binding to accept arbitrary DAI link names with a "-dai-link"
suffix. This is the common pattern used by the target (MT8188) and other
(MT8195, MT8186 etc.) MediaTek-based Chromebooks.

Signed-off-by: Fei Shao <fshao@chromium.org>
---

 .../devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml     | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
index 701cedfa38d2..2da34b66818f 100644
--- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
+++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
@@ -40,7 +40,7 @@ properties:
       name defined in the machine driver.
 
 patternProperties:
-  "^dai-link-[0-9]+$":
+  ".*-dai-link$":
     type: object
     description:
       Container for dai-link level properties and CODEC sub-nodes.
@@ -112,7 +112,7 @@ examples:
             "Headphone", "Headphone L",
             "Headphone", "Headphone R",
             "AIN1", "Headset Mic";
-        dai-link-0 {
+        hdmi-dai-link {
             link-name = "ETDM3_OUT_BE";
             dai-format = "i2s";
             mediatek,clk-provider = "cpu";
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH 3/4] ASoC: dt-bindings: maxim,max98390: Refernce common DAI properties

[Fei Shao]

Reference dai-common.yaml schema to support '#sound-dai-cells' and
'sound-name-prefix' properties.

Signed-off-by: Fei Shao <fshao@chromium.org>
---

 Documentation/devicetree/bindings/sound/maxim,max98390.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
index deaa6886c42f..5bd235cf15e6 100644
--- a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
+++ b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
@@ -9,6 +9,9 @@ title: Maxim Integrated MAX98390 Speaker Amplifier with Integrated Dynamic Speak
 maintainers:
   - Steve Lee <steves.lee@maximintegrated.com>
 
+allOf:
+  - $ref: dai-common.yaml#
+
 properties:
   compatible:
     const: maxim,max98390
@@ -36,7 +39,7 @@ required:
   - compatible
   - reg
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |
-- 
2.47.0.163.g1226f6d8fa-goog

[PATCH 4/4] ASoC: dt-bindings: maxim,max98390: Document maxim,dsm_param_name property

[Fei Shao]

Add the missing "maxim,dsm_param_name" property in the binding.
This property specifies the customized DSM parameter binary name.

Signed-off-by: Fei Shao <fshao@chromium.org>
---

 Documentation/devicetree/bindings/sound/maxim,max98390.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
index 5bd235cf15e6..fa4749735070 100644
--- a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
+++ b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
@@ -32,6 +32,10 @@ properties:
     minimum: 1
     maximum: 8388607
 
+  maxim,dsm_param_name:
+    description: The DSM parameter binary name (e.g. dsm_param.bin).
+    $ref: /schemas/types.yaml#/definitions/string
+
   reset-gpios:
     maxItems: 1
 
-- 
2.47.0.163.g1226f6d8fa-goog

Re: [PATCH 1/4] ASoC: dt-bindings: mediatek,mt8188-mt6359: Add adsp and dai-link properties

[Krzysztof Kozlowski]

On Fri, Oct 25, 2024 at 06:44:41PM +0800, Fei Shao wrote:
> Add "mediatek,adsp" property for the ADSP handle if ADSP is enabled on
> the platform.

We see this from the diff.

> Add "mediatek,dai-link" property for the required DAI links to configure
> sound card.

We see this from the diff.

> 
> Both properties are commonly used in the MediaTek sound card driver.

If they are used, why suddenly they are needed? What changed?

> 
> Signed-off-by: Fei Shao <fshao@chromium.org>
> ---
> 
>  .../bindings/sound/mediatek,mt8188-mt6359.yaml         | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> index f94ad0715e32..701cedfa38d2 100644
> --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> @@ -29,6 +29,16 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description: The phandle of MT8188 ASoC platform.
>  
> +  mediatek,adsp:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: The phandle of MT8188 ADSP platform.

And what is the difference between ASoC and ADSP platforms? What are
they used for?

> +
> +  mediatek,dai-link:
> +    $ref: /schemas/types.yaml#/definitions/string-array
> +    description:
> +      A list of the desired dai-links in the sound card. Each entry is a
> +      name defined in the machine driver.

The list is provided below. I don't understand why do you need it. Your
msg is pretty useless - you describe what you do, instead of why.

Best regards,
Krzysztof

Re: [PATCH 3/4] ASoC: dt-bindings: maxim,max98390: Refernce common DAI properties

[Krzysztof Kozlowski]

On Fri, Oct 25, 2024 at 06:44:43PM +0800, Fei Shao wrote:
> Reference dai-common.yaml schema to support '#sound-dai-cells' and
> 'sound-name-prefix' properties.

Why? Is this a DAI?

> 
> Signed-off-by: Fei Shao <fshao@chromium.org>
> ---
> 
>  Documentation/devicetree/bindings/sound/maxim,max98390.yaml | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> index deaa6886c42f..5bd235cf15e6 100644
> --- a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> +++ b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> @@ -9,6 +9,9 @@ title: Maxim Integrated MAX98390 Speaker Amplifier with Integrated Dynamic Speak
>  maintainers:
>    - Steve Lee <steves.lee@maximintegrated.com>
>  
> +allOf:
> +  - $ref: dai-common.yaml#
> +
>  properties:
>    compatible:
>      const: maxim,max98390

Missing dai cells - how many DAIs are there?

Best regards,
Krzysztof

Re: [PATCH 2/4] ASoC: dt-bindings: mediatek,mt8188-mt6359: Update DAI link node pattern

[Krzysztof Kozlowski]

On Fri, Oct 25, 2024 at 06:44:42PM +0800, Fei Shao wrote:
> The associated machine driver is not dependent on the format of DAI link
> node names. This means we are allowed to use more descriptive names
> instead of indices without impacting functionality.
> 
> Update the binding to accept arbitrary DAI link names with a "-dai-link"
> suffix. This is the common pattern used by the target (MT8188) and other
> (MT8195, MT8186 etc.) MediaTek-based Chromebooks.

We do not want arbitrary names. Why for example green-batman-dai-link
should be correct? The existing pattern looks wrong. Please read DT spec
and chapter about recommended names.


> 
> Signed-off-by: Fei Shao <fshao@chromium.org>
> ---
> 
>  .../devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml     | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> index 701cedfa38d2..2da34b66818f 100644
> --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> @@ -40,7 +40,7 @@ properties:
>        name defined in the machine driver.
>  
>  patternProperties:
> -  "^dai-link-[0-9]+$":
> +  ".*-dai-link$":

This breaks existing users.

>      type: object
>      description:
>        Container for dai-link level properties and CODEC sub-nodes.
> @@ -112,7 +112,7 @@ examples:
>              "Headphone", "Headphone L",
>              "Headphone", "Headphone R",
>              "AIN1", "Headset Mic";
> -        dai-link-0 {
> +        hdmi-dai-link {

No. Not really justified.

Best regards,
Krzysztof

Re: [PATCH 4/4] ASoC: dt-bindings: maxim,max98390: Document maxim,dsm_param_name property

[Krzysztof Kozlowski]

On Fri, Oct 25, 2024 at 06:44:44PM +0800, Fei Shao wrote:
> Add the missing "maxim,dsm_param_name" property in the binding.
> This property specifies the customized DSM parameter binary name.
> 
> Signed-off-by: Fei Shao <fshao@chromium.org>
> ---
> 
>  Documentation/devicetree/bindings/sound/maxim,max98390.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> index 5bd235cf15e6..fa4749735070 100644
> --- a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> +++ b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> @@ -32,6 +32,10 @@ properties:
>      minimum: 1
>      maximum: 8388607
>  
> +  maxim,dsm_param_name:
> +    description: The DSM parameter binary name (e.g. dsm_param.bin).
> +    $ref: /schemas/types.yaml#/definitions/string

NAK, you cannot document properties post-factum. It's not a property
coming from 2014.

For me, this is obvious that this is for ACPI and if you want to use it
for DT platforms, go through proper review.

In any case: NAK for this and other ACPI properties.

Best regards,
Krzysztof

Re: [PATCH 1/4] ASoC: dt-bindings: mediatek,mt8188-mt6359: Add adsp and dai-link properties

[Fei Shao]

On Mon, Oct 28, 2024 at 4:54 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Fri, Oct 25, 2024 at 06:44:41PM +0800, Fei Shao wrote:
> > Add "mediatek,adsp" property for the ADSP handle if ADSP is enabled on
> > the platform.
>
> We see this from the diff.
>
> > Add "mediatek,dai-link" property for the required DAI links to configure
> > sound card.
>
> We see this from the diff.
>
> >
> > Both properties are commonly used in the MediaTek sound card driver.
>
> If they are used, why suddenly they are needed? What changed?

Nothing has changed. These should have been added altogether when the
binding was first introduced. This patch is to fill the gaps and fix
dtbs_check warnings, like I mentioned in the cover letter.
I can add a line in the commit message saying it's to fix the warning
in addition to the cover letter, if that's preferred.

>
> >
> > Signed-off-by: Fei Shao <fshao@chromium.org>
> > ---
> >
> >  .../bindings/sound/mediatek,mt8188-mt6359.yaml         | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> > index f94ad0715e32..701cedfa38d2 100644
> > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> > @@ -29,6 +29,16 @@ properties:
> >      $ref: /schemas/types.yaml#/definitions/phandle
> >      description: The phandle of MT8188 ASoC platform.
> >
> > +  mediatek,adsp:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: The phandle of MT8188 ADSP platform.
>
> And what is the difference between ASoC and ADSP platforms? What are
> they used for?

I'm not a MediaTek or audio folks, and I'm afraid that I'm not the
best person to explain the details accurately in front of experts on
the list... I know it's an audio DSP but that explains nothing.
MediaTek didn't provide a meaningful explanation in the tree or
commits, and I want to avoid adding additional but likely misleading
descriptions from someone who doesn't have enough knowledge,
potentially causing even more confusing situations in the future.
Plus, the same changes were accepted as-is in the past, so I assumed
they might be self-explanatory to people who are familiar with the
matter.

>
> > +
> > +  mediatek,dai-link:
> > +    $ref: /schemas/types.yaml#/definitions/string-array
> > +    description:
> > +      A list of the desired dai-links in the sound card. Each entry is a
> > +      name defined in the machine driver.
>
> The list is provided below. I don't understand why do you need it. Your
> msg is pretty useless - you describe what you do, instead of why.

I think this is used to explicitly list the intermediate but hidden
DAIs, but again, there's not much info about them unless MediaTek can
explain more details and why they need a vendor property for this.

Regards,
Fei

>
> Best regards,
> Krzysztof
>

Re: [PATCH 2/4] ASoC: dt-bindings: mediatek,mt8188-mt6359: Update DAI link node pattern

[Fei Shao]

On Mon, Oct 28, 2024 at 4:56 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Fri, Oct 25, 2024 at 06:44:42PM +0800, Fei Shao wrote:
> > The associated machine driver is not dependent on the format of DAI link
> > node names. This means we are allowed to use more descriptive names
> > instead of indices without impacting functionality.
> >
> > Update the binding to accept arbitrary DAI link names with a "-dai-link"
> > suffix. This is the common pattern used by the target (MT8188) and other
> > (MT8195, MT8186 etc.) MediaTek-based Chromebooks.
>
> We do not want arbitrary names. Why for example green-batman-dai-link
> should be correct? The existing pattern looks wrong. Please read DT spec
> and chapter about recommended names.

At first I was thinking about regex like
`^[a-z-]+(-[a-z]+)*-dai-link$` based on the DTS coding style guide,
but your example is not suggesting that.
and the name like "hs-capture-dai-link" was rejected, so it's not just
about enumerating the names either.
Or "^dai-link@[0-9a-f]+$"? But they don't tie to particular register
addresses... or just for some pseudo indices?
I could miss something about DAI links under the sound documentation.
Still trying to figure it out.

>
> >
> > Signed-off-by: Fei Shao <fshao@chromium.org>
> > ---
> >
> >  .../devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml     | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> > index 701cedfa38d2..2da34b66818f 100644
> > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
> > @@ -40,7 +40,7 @@ properties:
> >        name defined in the machine driver.
> >
> >  patternProperties:
> > -  "^dai-link-[0-9]+$":
> > +  ".*-dai-link$":
>
> This breaks existing users.

There's no existing users in upstream, and the only downstream user is
the MT8188 Chromebook DT that I'm trying to upstream, which I can fix.
This binding was upstreamed exclusively for that DT.
Even if we take a step back and assume someone has another DT already
following this pattern (and that would be a surprise), this wouldn't
break anything except a dtbs_check error, which would be trivial if
they don't run that or attempt upstreaming. The driver doesn't use
node names to distinguish DAI links.
I want to fix this pattern before any actual users are in the tree if
possible, but I'm also fine to stick with dai-link-0 to dai-link-10 in
the case that nothing is changed and the DT has to follow the existing
patterns.

Regards,
Fei

>
> >      type: object
> >      description:
> >        Container for dai-link level properties and CODEC sub-nodes.
> > @@ -112,7 +112,7 @@ examples:
> >              "Headphone", "Headphone L",
> >              "Headphone", "Headphone R",
> >              "AIN1", "Headset Mic";
> > -        dai-link-0 {
> > +        hdmi-dai-link {
>
> No. Not really justified.
>
> Best regards,
> Krzysztof
>

Re: [PATCH 3/4] ASoC: dt-bindings: maxim,max98390: Refernce common DAI properties

[Fei Shao]

On Mon, Oct 28, 2024 at 4:55 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Fri, Oct 25, 2024 at 06:44:43PM +0800, Fei Shao wrote:
> > Reference dai-common.yaml schema to support '#sound-dai-cells' and
> > 'sound-name-prefix' properties.
>
> Why? Is this a DAI?

I'll add a line to say this is a DAI and the patch fixes dtbs_check errors.

>
> >
> > Signed-off-by: Fei Shao <fshao@chromium.org>
> > ---
> >
> >  Documentation/devicetree/bindings/sound/maxim,max98390.yaml | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> > index deaa6886c42f..5bd235cf15e6 100644
> > --- a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> > +++ b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> > @@ -9,6 +9,9 @@ title: Maxim Integrated MAX98390 Speaker Amplifier with Integrated Dynamic Speak
> >  maintainers:
> >    - Steve Lee <steves.lee@maximintegrated.com>
> >
> > +allOf:
> > +  - $ref: dai-common.yaml#
> > +
> >  properties:
> >    compatible:
> >      const: maxim,max98390
>
> Missing dai cells - how many DAIs are there?

Acked. Will add
"#sound-dai-cells":
    const 0

>
> Best regards,
> Krzysztof
>

Re: [PATCH 4/4] ASoC: dt-bindings: maxim,max98390: Document maxim,dsm_param_name property

[Fei Shao]

On Mon, Oct 28, 2024 at 4:59 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Fri, Oct 25, 2024 at 06:44:44PM +0800, Fei Shao wrote:
> > Add the missing "maxim,dsm_param_name" property in the binding.
> > This property specifies the customized DSM parameter binary name.
> >
> > Signed-off-by: Fei Shao <fshao@chromium.org>
> > ---
> >
> >  Documentation/devicetree/bindings/sound/maxim,max98390.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> > index 5bd235cf15e6..fa4749735070 100644
> > --- a/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> > +++ b/Documentation/devicetree/bindings/sound/maxim,max98390.yaml
> > @@ -32,6 +32,10 @@ properties:
> >      minimum: 1
> >      maximum: 8388607
> >
> > +  maxim,dsm_param_name:
> > +    description: The DSM parameter binary name (e.g. dsm_param.bin).
> > +    $ref: /schemas/types.yaml#/definitions/string
>
> NAK, you cannot document properties post-factum. It's not a property
> coming from 2014.
>
> For me, this is obvious that this is for ACPI and if you want to use it
> for DT platforms, go through proper review.
>
> In any case: NAK for this and other ACPI properties.

Acknowledged, I didn't know that.
This was directly from a Maxim customer and I guess they didn't remove
it when porting from ACPI or something.
I'll drop this and update DT in another series.

Regards,
Fei

>
> Best regards,
> Krzysztof
>

Re: [PATCH 1/4] ASoC: dt-bindings: mediatek,mt8188-mt6359: Add adsp and dai-link properties

[AngeloGioacchino Del Regno]

Il 28/10/24 12:10, Fei Shao ha scritto:
> On Mon, Oct 28, 2024 at 4:54 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Fri, Oct 25, 2024 at 06:44:41PM +0800, Fei Shao wrote:
>>> Add "mediatek,adsp" property for the ADSP handle if ADSP is enabled on
>>> the platform.
>>
>> We see this from the diff.
>>
>>> Add "mediatek,dai-link" property for the required DAI links to configure
>>> sound card.
>>
>> We see this from the diff.
>>
>>>
>>> Both properties are commonly used in the MediaTek sound card driver.
>>
>> If they are used, why suddenly they are needed? What changed?
> 
> Nothing has changed. These should have been added altogether when the
> binding was first introduced. This patch is to fill the gaps and fix
> dtbs_check warnings, like I mentioned in the cover letter.
> I can add a line in the commit message saying it's to fix the warning
> in addition to the cover letter, if that's preferred.
> 
>>
>>>
>>> Signed-off-by: Fei Shao <fshao@chromium.org>
>>> ---
>>>
>>>   .../bindings/sound/mediatek,mt8188-mt6359.yaml         | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
>>> index f94ad0715e32..701cedfa38d2 100644
>>> --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
>>> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
>>> @@ -29,6 +29,16 @@ properties:
>>>       $ref: /schemas/types.yaml#/definitions/phandle
>>>       description: The phandle of MT8188 ASoC platform.
>>>
>>> +  mediatek,adsp:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: The phandle of MT8188 ADSP platform.
>>
>> And what is the difference between ASoC and ADSP platforms? What are
>> they used for?
> 
> I'm not a MediaTek or audio folks, and I'm afraid that I'm not the
> best person to explain the details accurately in front of experts on
> the list... I know it's an audio DSP but that explains nothing.
> MediaTek didn't provide a meaningful explanation in the tree or
> commits, and I want to avoid adding additional but likely misleading
> descriptions from someone who doesn't have enough knowledge,
> potentially causing even more confusing situations in the future.
> Plus, the same changes were accepted as-is in the past, so I assumed
> they might be self-explanatory to people who are familiar with the
> matter.
> 

The Audio DSP is a Tensilica HiFi-5 DSP, and it's a block that is - hardware
speaking - separated from the rest of the Audio interfaces of the SoC.

The whole sound subsystem can work either with or without the DSP, in the sense
that the DSP itself can remain unpowered and completely unconfigured if its
functionality is not desired - hence, this is a board specific configuration:
if the board wants to use the DSP, we use the DSP - otherwise, we just don't.

Regarding the two "platforms", in short:
"mediatek,platform" -> Audio Front End (AFE)
"mediatek,adsp" -> Audio DSP

Now, you can either link the AFE DAIs to the I2S

As for "mediatek,platform" - that's used to link the Analog Front End (AFE) as
the DAI Link platform (so the path is direct to/from DL/UL DAIs to AFE) or the
ADSP one as the DAI Link platform (so that the path is to/from DL/UL DAIs to
DSP to AFE), but that - of course - still requires an AFE, otherwise you cannot
get the audio out of the speakers or in from the mic anyway.

>>
>>> +
>>> +  mediatek,dai-link:
>>> +    $ref: /schemas/types.yaml#/definitions/string-array
>>> +    description:
>>> +      A list of the desired dai-links in the sound card. Each entry is a
>>> +      name defined in the machine driver.
>>
>> The list is provided below. I don't understand why do you need it. Your
>> msg is pretty useless - you describe what you do, instead of why.
> 
> I think this is used to explicitly list the intermediate but hidden
> DAIs, but again, there's not much info about them unless MediaTek can
> explain more details and why they need a vendor property for this.
> 

Yes, this is used for exactly that... but I believe that we can deprecate this
now that we have support for the "standard" `audio-routing` property and for the
DAI Link nodes (examples that you can find in current device trees are mm-dai-link,
hs-playback-dai-link, or any other subnode of the sound card).

Specifically, those subnodes *do* require a "link-name" property, which *does*
effectively contain the same DAI Link names as the ones that are inside of the
"mediatek,dai-link" property.

On MT8195, you can find both the subnodes and the mediatek,dai-link - yes - but
that was done to retain compatibility of the device tree with old drivers (so,
an unusual case of new device tree on old kernel).

Finally, I believe that we can avoid adding that "mediatek,dai-link" property
to the MT8188 binding, and rely on:
  1. Whatever is provided in struct snd_soc_card for that device; and
  2. Whatever is provided in device trees as dai link subnodes, which would
     restrict N.1 as that's anyway describing card prelinks.

Cheers,
Angelo

> Regards,
> Fei
> 
>>
>> Best regards,
>> Krzysztof
>>

Re: [PATCH 1/4] ASoC: dt-bindings: mediatek,mt8188-mt6359: Add adsp and dai-link properties

[Trevor Wu (吳文良)]

On Mon, 2024-10-28 at 15:25 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Il 28/10/24 12:10, Fei Shao ha scritto:
> > On Mon, Oct 28, 2024 at 4:54 AM Krzysztof Kozlowski <
> > krzk@kernel.org> wrote:
> > > 
> > > On Fri, Oct 25, 2024 at 06:44:41PM +0800, Fei Shao wrote:
> > > > Add "mediatek,adsp" property for the ADSP handle if ADSP is
> > > > enabled on
> > > > the platform.
> > > 
> > > We see this from the diff.
> > > 
> > > > Add "mediatek,dai-link" property for the required DAI links to
> > > > configure
> > > > sound card.
> > > 
> > > We see this from the diff.
> > > 
> > > > 
> > > > Both properties are commonly used in the MediaTek sound card
> > > > driver.
> > > 
> > > If they are used, why suddenly they are needed? What changed?
> > 
> > Nothing has changed. These should have been added altogether when
> > the
> > binding was first introduced. This patch is to fill the gaps and
> > fix
> > dtbs_check warnings, like I mentioned in the cover letter.
> > I can add a line in the commit message saying it's to fix the
> > warning
> > in addition to the cover letter, if that's preferred.
> > 
> > > 
> > > > 
> > > > Signed-off-by: Fei Shao <fshao@chromium.org>
> > > > ---
> > > > 
> > > >   .../bindings/sound/mediatek,mt8188-mt6359.yaml         | 10
> > > > ++++++++++
> > > >   1 file changed, 10 insertions(+)
> > > > 
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > > > mt6359.yaml
> > > > b/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > > > mt6359.yaml
> > > > index f94ad0715e32..701cedfa38d2 100644
> > > > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > > > mt6359.yaml
> > > > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > > > mt6359.yaml
> > > > @@ -29,6 +29,16 @@ properties:
> > > >       $ref: /schemas/types.yaml#/definitions/phandle
> > > >       description: The phandle of MT8188 ASoC platform.
> > > > 
> > > > +  mediatek,adsp:
> > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > +    description: The phandle of MT8188 ADSP platform.
> > > 
> > > And what is the difference between ASoC and ADSP platforms? What
> > > are
> > > they used for?
> > 
> > I'm not a MediaTek or audio folks, and I'm afraid that I'm not the
> > best person to explain the details accurately in front of experts
> > on
> > the list... I know it's an audio DSP but that explains nothing.
> > MediaTek didn't provide a meaningful explanation in the tree or
> > commits, and I want to avoid adding additional but likely
> > misleading
> > descriptions from someone who doesn't have enough knowledge,
> > potentially causing even more confusing situations in the future.
> > Plus, the same changes were accepted as-is in the past, so I
> > assumed
> > they might be self-explanatory to people who are familiar with the
> > matter.
> > 
> 
> The Audio DSP is a Tensilica HiFi-5 DSP, and it's a block that is -
> hardware
> speaking - separated from the rest of the Audio interfaces of the
> SoC.
> 
> The whole sound subsystem can work either with or without the DSP, in
> the sense
> that the DSP itself can remain unpowered and completely unconfigured
> if its
> functionality is not desired - hence, this is a board specific
> configuration:
> if the board wants to use the DSP, we use the DSP - otherwise, we
> just don't.
> 
> Regarding the two "platforms", in short:
> "mediatek,platform" -> Audio Front End (AFE)
> "mediatek,adsp" -> Audio DSP
> 
> Now, you can either link the AFE DAIs to the I2S
> 
> As for "mediatek,platform" - that's used to link the Analog Front End
> (AFE) as
> the DAI Link platform (so the path is direct to/from DL/UL DAIs to
> AFE) or the
> ADSP one as the DAI Link platform (so that the path is to/from DL/UL
> DAIs to
> DSP to AFE), but that - of course - still requires an AFE, otherwise
> you cannot
> get the audio out of the speakers or in from the mic anyway.
> 
> > > 
> > > > +
> > > > +  mediatek,dai-link:
> > > > +    $ref: /schemas/types.yaml#/definitions/string-array
> > > > +    description:
> > > > +      A list of the desired dai-links in the sound card. Each
> > > > entry is a
> > > > +      name defined in the machine driver.
> > > 
> > > The list is provided below. I don't understand why do you need
> > > it. Your
> > > msg is pretty useless - you describe what you do, instead of why.
> > 
> > I think this is used to explicitly list the intermediate but hidden
> > DAIs, but again, there's not much info about them unless MediaTek
> > can
> > explain more details and why they need a vendor property for this.
> > 
> 
> Yes, this is used for exactly that... but I believe that we can
> deprecate this
> now that we have support for the "standard" `audio-routing` property
> and for the
> DAI Link nodes (examples that you can find in current device trees
> are mm-dai-link,
> hs-playback-dai-link, or any other subnode of the sound card).
> 
> Specifically, those subnodes *do* require a "link-name" property,
> which *does*
> effectively contain the same DAI Link names as the ones that are
> inside of the
> "mediatek,dai-link" property.
> 
> On MT8195, you can find both the subnodes and the mediatek,dai-link -
> yes - but
> that was done to retain compatibility of the device tree with old
> drivers (so,
> an unusual case of new device tree on old kernel).
> 
> Finally, I believe that we can avoid adding that "mediatek,dai-link"
> property
> to the MT8188 binding, and rely on:
>   1. Whatever is provided in struct snd_soc_card for that device; and
>   2. Whatever is provided in device trees as dai link subnodes, which
> would
>      restrict N.1 as that's anyway describing card prelinks.
> 

The "mediatek,dai-link" property is utilized to hide the unused dai-
links in the sound card. By hiding the unused FE links, it can save the
necessary memory and prevent conflicts where both the DSP and AP
control the same AFE Memifs.

This concept was first implemented in mt8195 as Mark aimed to avoid the
need for a separate driver for different system configurations.
With the introduction of DSP (SOF), if certain AFE Memifs are already
in use in the DSP route, they should be excluded from the PCM nodes
created for the AFE platform.

At that time, we did not have a better way to handle these scenarios,
so we made use of a vendor-defined property.

It has been a while since I last kept up with the updates in mt8188, so
I'm uncertain if the current mechanism for sound card description is
sufficient for handling such scenarios. If it is, I agree that we can
deprecate such a vendor property from mt8188.

Thanks,
Trevor


> Cheers,
> Angelo
> 
> > Regards,
> > Fei
> > 
> > > 
> > > Best regards,
> > > Krzysztof
> > > 
> 
> 

Re: [PATCH 2/4] ASoC: dt-bindings: mediatek,mt8188-mt6359: Update DAI link node pattern

[Krzysztof Kozlowski]

On 28/10/2024 12:12, Fei Shao wrote:
> On Mon, Oct 28, 2024 at 4:56 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Fri, Oct 25, 2024 at 06:44:42PM +0800, Fei Shao wrote:
>>> The associated machine driver is not dependent on the format of DAI link
>>> node names. This means we are allowed to use more descriptive names
>>> instead of indices without impacting functionality.
>>>
>>> Update the binding to accept arbitrary DAI link names with a "-dai-link"
>>> suffix. This is the common pattern used by the target (MT8188) and other
>>> (MT8195, MT8186 etc.) MediaTek-based Chromebooks.
>>
>> We do not want arbitrary names. Why for example green-batman-dai-link
>> should be correct? The existing pattern looks wrong. Please read DT spec
>> and chapter about recommended names.
> 
> At first I was thinking about regex like
> `^[a-z-]+(-[a-z]+)*-dai-link$` based on the DTS coding style guide,
> but your example is not suggesting that.
> and the name like "hs-capture-dai-link" was rejected, so it's not just
> about enumerating the names either.
> Or "^dai-link@[0-9a-f]+$"? But they don't tie to particular register
> addresses... or just for some pseudo indices?
> I could miss something about DAI links under the sound documentation.
> Still trying to figure it out.

You did not provide any reasoning why this has to be changed and why
usually preferred generic dai-link is not correct. Names are not
descriptive.


> 
>>
>>>
>>> Signed-off-by: Fei Shao <fshao@chromium.org>
>>> ---
>>>
>>>  .../devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml     | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
>>> index 701cedfa38d2..2da34b66818f 100644
>>> --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
>>> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-mt6359.yaml
>>> @@ -40,7 +40,7 @@ properties:
>>>        name defined in the machine driver.
>>>
>>>  patternProperties:
>>> -  "^dai-link-[0-9]+$":
>>> +  ".*-dai-link$":
>>
>> This breaks existing users.
> 
> There's no existing users in upstream, and the only downstream user is
> the MT8188 Chromebook DT that I'm trying to upstream, which I can fix.
> This binding was upstreamed exclusively for that DT.

So you have entire commit msg to explain impact.



Best regards,
Krzysztof

Re: [PATCH 1/4] ASoC: dt-bindings: mediatek,mt8188-mt6359: Add adsp and dai-link properties

[Fei Shao]

On Tue, Oct 29, 2024 at 11:05 AM Trevor Wu (吳文良) <Trevor.Wu@mediatek.com> wrote:
>
> On Mon, 2024-10-28 at 15:25 +0100, AngeloGioacchino Del Regno wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> >
> >
> > Il 28/10/24 12:10, Fei Shao ha scritto:
> > > On Mon, Oct 28, 2024 at 4:54 AM Krzysztof Kozlowski <
> > > krzk@kernel.org> wrote:
> > > >
> > > > On Fri, Oct 25, 2024 at 06:44:41PM +0800, Fei Shao wrote:
> > > > > Add "mediatek,adsp" property for the ADSP handle if ADSP is
> > > > > enabled on
> > > > > the platform.
> > > >
> > > > We see this from the diff.
> > > >
> > > > > Add "mediatek,dai-link" property for the required DAI links to
> > > > > configure
> > > > > sound card.
> > > >
> > > > We see this from the diff.
> > > >
> > > > >
> > > > > Both properties are commonly used in the MediaTek sound card
> > > > > driver.
> > > >
> > > > If they are used, why suddenly they are needed? What changed?
> > >
> > > Nothing has changed. These should have been added altogether when
> > > the
> > > binding was first introduced. This patch is to fill the gaps and
> > > fix
> > > dtbs_check warnings, like I mentioned in the cover letter.
> > > I can add a line in the commit message saying it's to fix the
> > > warning
> > > in addition to the cover letter, if that's preferred.
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Fei Shao <fshao@chromium.org>
> > > > > ---
> > > > >
> > > > >   .../bindings/sound/mediatek,mt8188-mt6359.yaml         | 10
> > > > > ++++++++++
> > > > >   1 file changed, 10 insertions(+)
> > > > >
> > > > > diff --git
> > > > > a/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > > > > mt6359.yaml
> > > > > b/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > > > > mt6359.yaml
> > > > > index f94ad0715e32..701cedfa38d2 100644
> > > > > --- a/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > > > > mt6359.yaml
> > > > > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8188-
> > > > > mt6359.yaml
> > > > > @@ -29,6 +29,16 @@ properties:
> > > > >       $ref: /schemas/types.yaml#/definitions/phandle
> > > > >       description: The phandle of MT8188 ASoC platform.
> > > > >
> > > > > +  mediatek,adsp:
> > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > > +    description: The phandle of MT8188 ADSP platform.
> > > >
> > > > And what is the difference between ASoC and ADSP platforms? What
> > > > are
> > > > they used for?
> > >
> > > I'm not a MediaTek or audio folks, and I'm afraid that I'm not the
> > > best person to explain the details accurately in front of experts
> > > on
> > > the list... I know it's an audio DSP but that explains nothing.
> > > MediaTek didn't provide a meaningful explanation in the tree or
> > > commits, and I want to avoid adding additional but likely
> > > misleading
> > > descriptions from someone who doesn't have enough knowledge,
> > > potentially causing even more confusing situations in the future.
> > > Plus, the same changes were accepted as-is in the past, so I
> > > assumed
> > > they might be self-explanatory to people who are familiar with the
> > > matter.
> > >
> >
> > The Audio DSP is a Tensilica HiFi-5 DSP, and it's a block that is -
> > hardware
> > speaking - separated from the rest of the Audio interfaces of the
> > SoC.
> >
> > The whole sound subsystem can work either with or without the DSP, in
> > the sense
> > that the DSP itself can remain unpowered and completely unconfigured
> > if its
> > functionality is not desired - hence, this is a board specific
> > configuration:
> > if the board wants to use the DSP, we use the DSP - otherwise, we
> > just don't.
> >
> > Regarding the two "platforms", in short:
> > "mediatek,platform" -> Audio Front End (AFE)
> > "mediatek,adsp" -> Audio DSP
> >
> > Now, you can either link the AFE DAIs to the I2S
> >
> > As for "mediatek,platform" - that's used to link the Analog Front End
> > (AFE) as
> > the DAI Link platform (so the path is direct to/from DL/UL DAIs to
> > AFE) or the
> > ADSP one as the DAI Link platform (so that the path is to/from DL/UL
> > DAIs to
> > DSP to AFE), but that - of course - still requires an AFE, otherwise
> > you cannot
> > get the audio out of the speakers or in from the mic anyway.
> >
> > > >
> > > > > +
> > > > > +  mediatek,dai-link:
> > > > > +    $ref: /schemas/types.yaml#/definitions/string-array
> > > > > +    description:
> > > > > +      A list of the desired dai-links in the sound card. Each
> > > > > entry is a
> > > > > +      name defined in the machine driver.
> > > >
> > > > The list is provided below. I don't understand why do you need
> > > > it. Your
> > > > msg is pretty useless - you describe what you do, instead of why.
> > >
> > > I think this is used to explicitly list the intermediate but hidden
> > > DAIs, but again, there's not much info about them unless MediaTek
> > > can
> > > explain more details and why they need a vendor property for this.
> > >
> >
> > Yes, this is used for exactly that... but I believe that we can
> > deprecate this
> > now that we have support for the "standard" `audio-routing` property
> > and for the
> > DAI Link nodes (examples that you can find in current device trees
> > are mm-dai-link,
> > hs-playback-dai-link, or any other subnode of the sound card).
> >
> > Specifically, those subnodes *do* require a "link-name" property,
> > which *does*
> > effectively contain the same DAI Link names as the ones that are
> > inside of the
> > "mediatek,dai-link" property.
> >
> > On MT8195, you can find both the subnodes and the mediatek,dai-link -
> > yes - but
> > that was done to retain compatibility of the device tree with old
> > drivers (so,
> > an unusual case of new device tree on old kernel).
> >
> > Finally, I believe that we can avoid adding that "mediatek,dai-link"
> > property
> > to the MT8188 binding, and rely on:
> >   1. Whatever is provided in struct snd_soc_card for that device; and
> >   2. Whatever is provided in device trees as dai link subnodes, which
> > would
> >      restrict N.1 as that's anyway describing card prelinks.
> >
>
> The "mediatek,dai-link" property is utilized to hide the unused dai-
> links in the sound card. By hiding the unused FE links, it can save the
> necessary memory and prevent conflicts where both the DSP and AP
> control the same AFE Memifs.
>
> This concept was first implemented in mt8195 as Mark aimed to avoid the
> need for a separate driver for different system configurations.
> With the introduction of DSP (SOF), if certain AFE Memifs are already
> in use in the DSP route, they should be excluded from the PCM nodes
> created for the AFE platform.
>
> At that time, we did not have a better way to handle these scenarios,
> so we made use of a vendor-defined property.
>
> It has been a while since I last kept up with the updates in mt8188, so
> I'm uncertain if the current mechanism for sound card description is
> sufficient for handling such scenarios. If it is, I agree that we can
> deprecate such a vendor property from mt8188.
>
> Thanks,
> Trevor
>
>
> > Cheers,
> > Angelo

Many thanks Angelo and Trevor for the information.
The audio-routing property is in place, so I'll check if removing the
vendor property makes any impact, and refresh the series accordingly.

Thanks,
Fei



> >
> > > Regards,
> > > Fei
> > >
> > > >
> > > > Best regards,
> > > > Krzysztof
> > > >
> >
> >