diff mbox series

[19/22] ASoC: dt-bindings: mt8192: Document audio-routing and dai-link subnode

Message ID 20240227120939.290143-20-angelogioacchino.delregno@collabora.com
State New
Headers show
Series ASoC: Cleanup MediaTek soundcard machine drivers | expand

Commit Message

AngeloGioacchino Del Regno Feb. 27, 2024, 12:09 p.m. UTC
Document the dai-link subnodes and the audio-routing property, allowing
to describe machine specific audio hardware and links in device tree.

While at it, also deprecate the old properties which were previously
used with the driver's partially hardcoded configuration.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../sound/mt8192-mt6359-rt1015-rt5682.yaml    | 129 ++++++++++++++++--
 1 file changed, 121 insertions(+), 8 deletions(-)

Comments

Rob Herring (Arm) March 4, 2024, 2:23 p.m. UTC | #1
On Tue, Feb 27, 2024 at 01:09:36PM +0100, AngeloGioacchino Del Regno wrote:
> Document the dai-link subnodes and the audio-routing property, allowing
> to describe machine specific audio hardware and links in device tree.
> 
> While at it, also deprecate the old properties which were previously
> used with the driver's partially hardcoded configuration.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../sound/mt8192-mt6359-rt1015-rt5682.yaml    | 129 ++++++++++++++++--
>  1 file changed, 121 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
> index 7e50f5d65c8f..78e221003750 100644
> --- a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
> +++ b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
> @@ -20,6 +20,15 @@ properties:
>        - mediatek,mt8192_mt6359_rt1015p_rt5682
>        - mediatek,mt8192_mt6359_rt1015p_rt5682s
>  
> +  audio-routing:
> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array

Already defined in sound-card-common.yaml. Add a $ref.

> +    description:
> +      A list of the connections between audio components. Each entry is a
> +      pair of strings, the first being the connection's sink, the second
> +      being the connection's source.
> +      Valid names could be the input or output widgets of audio components,
> +      power supplies, MicBias of codec and the software switch.

Generally the names are defined here.

> +
>    mediatek,platform:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description: The phandle of MT8192 ASoC platform.
> @@ -27,10 +36,12 @@ properties:
>    mediatek,hdmi-codec:
>      $ref: /schemas/types.yaml#/definitions/phandle
>      description: The phandle of HDMI codec.
> +    deprecated: true
>  
>    headset-codec:
>      type: object
>      additionalProperties: false
> +    deprecated: true
>  
>      properties:
>        sound-dai:
> @@ -41,6 +52,7 @@ properties:
>    speaker-codecs:
>      type: object
>      additionalProperties: false
> +    deprecated: true
>  
>      properties:
>        sound-dai:
> @@ -51,13 +63,83 @@ properties:
>      required:
>        - sound-dai
>  
> +patternProperties:
> +  ".*-dai-link$":
> +    type: object
> +    description:
> +      Container for dai-link level properties and CODEC sub-nodes.
> +
> +    properties:
> +      link-name:
> +        description: Indicates dai-link name and PCM stream name
> +        items:
> +          enum:
> +            - I2S0
> +            - I2S1
> +            - I2S2
> +            - I2S3
> +            - I2S4
> +            - I2S5
> +            - I2S6
> +            - I2S7
> +            - I2S8
> +            - I2S9
> +            - TDM
> +
> +      codec:
> +        description: Holds subnode which indicates codec dai.
> +        type: object
> +        additionalProperties: false
> +        properties:
> +          sound-dai:
> +            minItems: 1
> +            maxItems: 2
> +        required:
> +          - sound-dai
> +
> +      dai-format:
> +        description: audio format
> +        items:
> +          enum:
> +            - i2s
> +            - right_j
> +            - left_j
> +            - dsp_a
> +            - dsp_b
> +
> +      mediatek,clk-provider:
> +        $ref: /schemas/types.yaml#/definitions/string
> +        description: Indicates dai-link clock master.
> +        items:
> +          enum:
> +            - cpu
> +            - codec
> +
> +    additionalProperties: false

Move this before properties.

> +
> +    required:
> +      - link-name
> +
>  additionalProperties: false
>  
>  required:
>    - compatible
>    - mediatek,platform
> -  - headset-codec
> -  - speaker-codecs
> +
> +allOf:
> +  # Disallow dai-link-xxx nodes if the legacy properties are specified

xxx-dai-link?

> +  - if:
> +      patternProperties:
> +        ".*-dai-link$": false
> +    then:
> +      required:
> +        - headset-codec
> +        - speaker-codecs
> +    else:
> +      properties:
> +        headset-codec: false
> +        speaker-codecs: false
> +        mediatek,hdmi-codec: false

Allowing both would preserve compatibility. That's not needed? If so, 
say why in the commit msg.

>  
>  examples:
>    - |
> @@ -65,19 +147,50 @@ examples:
>      sound: mt8192-sound {
>          compatible = "mediatek,mt8192_mt6359_rt1015_rt5682";
>          mediatek,platform = <&afe>;
> -        mediatek,hdmi-codec = <&anx_bridge_dp>;
>          pinctrl-names = "aud_clk_mosi_off",
>                          "aud_clk_mosi_on";
>          pinctrl-0 = <&aud_clk_mosi_off>;
>          pinctrl-1 = <&aud_clk_mosi_on>;
>  
> -        headset-codec {
> -            sound-dai = <&rt5682>;
> +        audio-routing =
> +                "Headphone Jack", "HPOL",
> +                "Headphone Jack", "HPOR",
> +                "IN1P", "Headset Mic",
> +                "Speakers", "Speaker";
> +
> +        spk-playback-dai-link {
> +                link-name = "I2S3";
> +                dai-format = "i2s";
> +                mediatek,clk-provider = "cpu";
> +                codec {
> +                        sound-dai = <&rt1015p>;
> +                };
> +        };
> +
> +        hs-playback-dai-link {
> +                link-name = "I2S8";
> +                dai-format = "i2s";
> +                mediatek,clk-provider = "cpu";
> +                codec {
> +                        sound-dai = <&rt5682 0>;
> +                };
> +        };
> +
> +        hs-capture-dai-link {
> +                link-name = "I2S9";
> +                dai-format = "i2s";
> +                mediatek,clk-provider = "cpu";
> +                codec {
> +                        sound-dai = <&rt5682 0>;
> +                };
>          };
>  
> -        speaker-codecs {
> -            sound-dai = <&rt1015_l>,
> -                        <&rt1015_r>;
> +        displayport-dai-link {
> +                link-name = "TDM";
> +                dai-format = "dsp_a";
> +                codec {
> +                        sound-dai = <&anx_bridge_dp>;
> +                };
>          };
>      };
>  
> -- 
> 2.44.0
>
AngeloGioacchino Del Regno March 5, 2024, 11:20 a.m. UTC | #2
Il 04/03/24 15:23, Rob Herring ha scritto:
> On Tue, Feb 27, 2024 at 01:09:36PM +0100, AngeloGioacchino Del Regno wrote:
>> Document the dai-link subnodes and the audio-routing property, allowing
>> to describe machine specific audio hardware and links in device tree.
>>
>> While at it, also deprecate the old properties which were previously
>> used with the driver's partially hardcoded configuration.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   .../sound/mt8192-mt6359-rt1015-rt5682.yaml    | 129 ++++++++++++++++--
>>   1 file changed, 121 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
>> index 7e50f5d65c8f..78e221003750 100644
>> --- a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
>> +++ b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
>> @@ -20,6 +20,15 @@ properties:
>>         - mediatek,mt8192_mt6359_rt1015p_rt5682
>>         - mediatek,mt8192_mt6359_rt1015p_rt5682s
>>   
>> +  audio-routing:
>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> 
> Already defined in sound-card-common.yaml. Add a $ref.
> 

Right. Done for v2.

>> +    description:
>> +      A list of the connections between audio components. Each entry is a
>> +      pair of strings, the first being the connection's sink, the second
>> +      being the connection's source.
>> +      Valid names could be the input or output widgets of audio components,
>> +      power supplies, MicBias of codec and the software switch.
> 
> Generally the names are defined here.
> 

...but those drivers want to support multiple codecs and multiple boards, so
for each board we would maybe have to add (software defined) names in here
which don't always correspond to a HW pin name (but that's not really a problem).

Sure a subset of the names can't change but, on the other hand, some others
can (as in, may be added).

Hence the question:

Is it mandatory to define the names in an enum here, or can that be avoided?
If it is, I can add them no problem.

>> +
>>     mediatek,platform:
>>       $ref: /schemas/types.yaml#/definitions/phandle
>>       description: The phandle of MT8192 ASoC platform.
>> @@ -27,10 +36,12 @@ properties:
>>     mediatek,hdmi-codec:
>>       $ref: /schemas/types.yaml#/definitions/phandle
>>       description: The phandle of HDMI codec.
>> +    deprecated: true
>>   
>>     headset-codec:
>>       type: object
>>       additionalProperties: false
>> +    deprecated: true
>>   
>>       properties:
>>         sound-dai:
>> @@ -41,6 +52,7 @@ properties:
>>     speaker-codecs:
>>       type: object
>>       additionalProperties: false
>> +    deprecated: true
>>   
>>       properties:
>>         sound-dai:
>> @@ -51,13 +63,83 @@ properties:
>>       required:
>>         - sound-dai
>>   
>> +patternProperties:
>> +  ".*-dai-link$":
>> +    type: object
>> +    description:
>> +      Container for dai-link level properties and CODEC sub-nodes.
>> +
>> +    properties:
>> +      link-name:
>> +        description: Indicates dai-link name and PCM stream name
>> +        items:
>> +          enum:
>> +            - I2S0
>> +            - I2S1
>> +            - I2S2
>> +            - I2S3
>> +            - I2S4
>> +            - I2S5
>> +            - I2S6
>> +            - I2S7
>> +            - I2S8
>> +            - I2S9
>> +            - TDM
>> +
>> +      codec:
>> +        description: Holds subnode which indicates codec dai.
>> +        type: object
>> +        additionalProperties: false
>> +        properties:
>> +          sound-dai:
>> +            minItems: 1
>> +            maxItems: 2
>> +        required:
>> +          - sound-dai
>> +
>> +      dai-format:
>> +        description: audio format
>> +        items:
>> +          enum:
>> +            - i2s
>> +            - right_j
>> +            - left_j
>> +            - dsp_a
>> +            - dsp_b
>> +
>> +      mediatek,clk-provider:
>> +        $ref: /schemas/types.yaml#/definitions/string
>> +        description: Indicates dai-link clock master.
>> +        items:
>> +          enum:
>> +            - cpu
>> +            - codec
>> +
>> +    additionalProperties: false
> 
> Move this before properties.
> 

Done for v2.

>> +
>> +    required:
>> +      - link-name
>> +
>>   additionalProperties: false
>>   
>>   required:
>>     - compatible
>>     - mediatek,platform
>> -  - headset-codec
>> -  - speaker-codecs
>> +
>> +allOf:
>> +  # Disallow dai-link-xxx nodes if the legacy properties are specified
> 
> xxx-dai-link?
> 

Oh! Yes, thanks for catching this.

That's what I initially wanted to do, but then I opted for xxx-dai-link and
forgot to update this comment.

Fixed for v2.

>> +  - if:
>> +      patternProperties:
>> +        ".*-dai-link$": false
>> +    then:
>> +      required:
>> +        - headset-codec
>> +        - speaker-codecs
>> +    else:
>> +      properties:
>> +        headset-codec: false
>> +        speaker-codecs: false
>> +        mediatek,hdmi-codec: false
> 
> Allowing both would preserve compatibility. That's not needed? If so,
> say why in the commit msg.
> 

I'm thinking of writing:

"Since describing machine specific audio hardware and links replaces the
now deprecated old logic doing the same in a driver hardcoded fashion,
it is not allowed to have both the old and new properties together."

...but in short - both the old and the new can do exactly the same, but
imo it doesn't make any sense to actually rely on both as:
  1. It's redundant (and one set of them makes the other useless);
  2. I want to avoid confusion (as the other set won't be parsed);
  3. I'm trying to *enforce* consistency as MTK cards have different
     bindings for .. really, no good reason;
  4. I want to see custom stuff disappear completely (and/or as much as
     possible anyway) and use something that is (at least somewhat) common
     between all MTK and non-MTK or anyway as a start at least consistent
     between MTK cards.

In theory, though, speaking of the driver side, there's nothing preventing
you from specifying both audio-routing xxx-dai-link and mediatek,hdmi-codec,
as the drivers' action will be, in short
    if (new_bindings)
      forget_about_old_bindings_use_the_new_ones();
    else
      use_old_hardcoded_stuff(); /* and be sad */


For that, I really don't want to allow both sets of properties - please, please,
tell me that I don't *have to* remove this block :-)

Cheers,
Angelo
Rob Herring (Arm) March 7, 2024, 2:03 p.m. UTC | #3
On Tue, Mar 5, 2024 at 5:20 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 04/03/24 15:23, Rob Herring ha scritto:
> > On Tue, Feb 27, 2024 at 01:09:36PM +0100, AngeloGioacchino Del Regno wrote:
> >> Document the dai-link subnodes and the audio-routing property, allowing
> >> to describe machine specific audio hardware and links in device tree.
> >>
> >> While at it, also deprecate the old properties which were previously
> >> used with the driver's partially hardcoded configuration.
> >>
> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> >> ---
> >>   .../sound/mt8192-mt6359-rt1015-rt5682.yaml    | 129 ++++++++++++++++--
> >>   1 file changed, 121 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
> >> index 7e50f5d65c8f..78e221003750 100644
> >> --- a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
> >> +++ b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
> >> @@ -20,6 +20,15 @@ properties:
> >>         - mediatek,mt8192_mt6359_rt1015p_rt5682
> >>         - mediatek,mt8192_mt6359_rt1015p_rt5682s
> >>
> >> +  audio-routing:
> >> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
> >
> > Already defined in sound-card-common.yaml. Add a $ref.
> >
>
> Right. Done for v2.
>
> >> +    description:
> >> +      A list of the connections between audio components. Each entry is a
> >> +      pair of strings, the first being the connection's sink, the second
> >> +      being the connection's source.
> >> +      Valid names could be the input or output widgets of audio components,
> >> +      power supplies, MicBias of codec and the software switch.
> >
> > Generally the names are defined here.
> >
>
> ...but those drivers want to support multiple codecs and multiple boards, so
> for each board we would maybe have to add (software defined) names in here
> which don't always correspond to a HW pin name (but that's not really a problem).
>
> Sure a subset of the names can't change but, on the other hand, some others
> can (as in, may be added).
>
> Hence the question:
>
> Is it mandatory to define the names in an enum here, or can that be avoided?
> If it is, I can add them no problem.

Does the OS depend on what the names are? As-in if a name was "bar"
and it changed to "baz" in either the DT or the kernel, would that
break things? If yes, then yes, you need them defined here.

>
> >> +
> >>     mediatek,platform:
> >>       $ref: /schemas/types.yaml#/definitions/phandle
> >>       description: The phandle of MT8192 ASoC platform.
> >> @@ -27,10 +36,12 @@ properties:
> >>     mediatek,hdmi-codec:
> >>       $ref: /schemas/types.yaml#/definitions/phandle
> >>       description: The phandle of HDMI codec.
> >> +    deprecated: true
> >>
> >>     headset-codec:
> >>       type: object
> >>       additionalProperties: false
> >> +    deprecated: true
> >>
> >>       properties:
> >>         sound-dai:
> >> @@ -41,6 +52,7 @@ properties:
> >>     speaker-codecs:
> >>       type: object
> >>       additionalProperties: false
> >> +    deprecated: true
> >>
> >>       properties:
> >>         sound-dai:
> >> @@ -51,13 +63,83 @@ properties:
> >>       required:
> >>         - sound-dai
> >>
> >> +patternProperties:
> >> +  ".*-dai-link$":
> >> +    type: object
> >> +    description:
> >> +      Container for dai-link level properties and CODEC sub-nodes.
> >> +
> >> +    properties:
> >> +      link-name:
> >> +        description: Indicates dai-link name and PCM stream name
> >> +        items:
> >> +          enum:
> >> +            - I2S0
> >> +            - I2S1
> >> +            - I2S2
> >> +            - I2S3
> >> +            - I2S4
> >> +            - I2S5
> >> +            - I2S6
> >> +            - I2S7
> >> +            - I2S8
> >> +            - I2S9
> >> +            - TDM
> >> +
> >> +      codec:
> >> +        description: Holds subnode which indicates codec dai.
> >> +        type: object
> >> +        additionalProperties: false
> >> +        properties:
> >> +          sound-dai:
> >> +            minItems: 1
> >> +            maxItems: 2
> >> +        required:
> >> +          - sound-dai
> >> +
> >> +      dai-format:
> >> +        description: audio format
> >> +        items:
> >> +          enum:
> >> +            - i2s
> >> +            - right_j
> >> +            - left_j
> >> +            - dsp_a
> >> +            - dsp_b
> >> +
> >> +      mediatek,clk-provider:
> >> +        $ref: /schemas/types.yaml#/definitions/string
> >> +        description: Indicates dai-link clock master.
> >> +        items:
> >> +          enum:
> >> +            - cpu
> >> +            - codec
> >> +
> >> +    additionalProperties: false
> >
> > Move this before properties.
> >
>
> Done for v2.
>
> >> +
> >> +    required:
> >> +      - link-name
> >> +
> >>   additionalProperties: false
> >>
> >>   required:
> >>     - compatible
> >>     - mediatek,platform
> >> -  - headset-codec
> >> -  - speaker-codecs
> >> +
> >> +allOf:
> >> +  # Disallow dai-link-xxx nodes if the legacy properties are specified
> >
> > xxx-dai-link?
> >
>
> Oh! Yes, thanks for catching this.
>
> That's what I initially wanted to do, but then I opted for xxx-dai-link and
> forgot to update this comment.
>
> Fixed for v2.
>
> >> +  - if:
> >> +      patternProperties:
> >> +        ".*-dai-link$": false
> >> +    then:
> >> +      required:
> >> +        - headset-codec
> >> +        - speaker-codecs
> >> +    else:
> >> +      properties:
> >> +        headset-codec: false
> >> +        speaker-codecs: false
> >> +        mediatek,hdmi-codec: false
> >
> > Allowing both would preserve compatibility. That's not needed? If so,
> > say why in the commit msg.
> >
>
> I'm thinking of writing:
>
> "Since describing machine specific audio hardware and links replaces the
> now deprecated old logic doing the same in a driver hardcoded fashion,
> it is not allowed to have both the old and new properties together."

What happened to that. Instead you just sent a new version with
nothing about this.

> ...but in short - both the old and the new can do exactly the same, but
> imo it doesn't make any sense to actually rely on both as:
>   1. It's redundant (and one set of them makes the other useless);
>   2. I want to avoid confusion (as the other set won't be parsed);
>   3. I'm trying to *enforce* consistency as MTK cards have different
>      bindings for .. really, no good reason;
>   4. I want to see custom stuff disappear completely (and/or as much as
>      possible anyway) and use something that is (at least somewhat) common
>      between all MTK and non-MTK or anyway as a start at least consistent
>      between MTK cards.
>
> In theory, though, speaking of the driver side, there's nothing preventing
> you from specifying both audio-routing xxx-dai-link and mediatek,hdmi-codec,
> as the drivers' action will be, in short
>     if (new_bindings)
>       forget_about_old_bindings_use_the_new_ones();
>     else
>       use_old_hardcoded_stuff(); /* and be sad */

That works for newer kernels with this change, but existing kernels
will only have:

use_old_hardcoded_stuff(); /* and not know it's sad */

If you want to support a new DT and old kernel, you need to populate
both properties.

> For that, I really don't want to allow both sets of properties - please, please,
> tell me that I don't *have to* remove this block :-)

Ultimately it is your decision as Mediatek maintainer, not mine. My
only requirement is the commit message explain why the above
combination is not important for these platforms.

You could leave it, but keep both in the dts files for some time
period. That will cause warnings, but what's a few more. The ABI
doesn't have to be a forever thing. Things evolve and there will be
other reasons to upgrade.

Rob
AngeloGioacchino Del Regno March 7, 2024, 2:26 p.m. UTC | #4
Il 07/03/24 15:03, Rob Herring ha scritto:
> On Tue, Mar 5, 2024 at 5:20 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Il 04/03/24 15:23, Rob Herring ha scritto:
>>> On Tue, Feb 27, 2024 at 01:09:36PM +0100, AngeloGioacchino Del Regno wrote:
>>>> Document the dai-link subnodes and the audio-routing property, allowing
>>>> to describe machine specific audio hardware and links in device tree.
>>>>
>>>> While at it, also deprecate the old properties which were previously
>>>> used with the driver's partially hardcoded configuration.
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>> ---
>>>>    .../sound/mt8192-mt6359-rt1015-rt5682.yaml    | 129 ++++++++++++++++--
>>>>    1 file changed, 121 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
>>>> index 7e50f5d65c8f..78e221003750 100644
>>>> --- a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
>>>> +++ b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
>>>> @@ -20,6 +20,15 @@ properties:
>>>>          - mediatek,mt8192_mt6359_rt1015p_rt5682
>>>>          - mediatek,mt8192_mt6359_rt1015p_rt5682s
>>>>
>>>> +  audio-routing:
>>>> +    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
>>>
>>> Already defined in sound-card-common.yaml. Add a $ref.
>>>
>>
>> Right. Done for v2.
>>
>>>> +    description:
>>>> +      A list of the connections between audio components. Each entry is a
>>>> +      pair of strings, the first being the connection's sink, the second
>>>> +      being the connection's source.
>>>> +      Valid names could be the input or output widgets of audio components,
>>>> +      power supplies, MicBias of codec and the software switch.
>>>
>>> Generally the names are defined here.
>>>
>>
>> ...but those drivers want to support multiple codecs and multiple boards, so
>> for each board we would maybe have to add (software defined) names in here
>> which don't always correspond to a HW pin name (but that's not really a problem).
>>
>> Sure a subset of the names can't change but, on the other hand, some others
>> can (as in, may be added).
>>
>> Hence the question:
>>
>> Is it mandatory to define the names in an enum here, or can that be avoided?
>> If it is, I can add them no problem.
> 
> Does the OS depend on what the names are? As-in if a name was "bar"
> and it changed to "baz" in either the DT or the kernel, would that
> break things? If yes, then yes, you need them defined here.
> 

Yes, I need them defined here, definitely.

>>
>>>> +
>>>>      mediatek,platform:
>>>>        $ref: /schemas/types.yaml#/definitions/phandle
>>>>        description: The phandle of MT8192 ASoC platform.
>>>> @@ -27,10 +36,12 @@ properties:
>>>>      mediatek,hdmi-codec:
>>>>        $ref: /schemas/types.yaml#/definitions/phandle
>>>>        description: The phandle of HDMI codec.
>>>> +    deprecated: true
>>>>
>>>>      headset-codec:
>>>>        type: object
>>>>        additionalProperties: false
>>>> +    deprecated: true
>>>>
>>>>        properties:
>>>>          sound-dai:
>>>> @@ -41,6 +52,7 @@ properties:
>>>>      speaker-codecs:
>>>>        type: object
>>>>        additionalProperties: false
>>>> +    deprecated: true
>>>>
>>>>        properties:
>>>>          sound-dai:
>>>> @@ -51,13 +63,83 @@ properties:
>>>>        required:
>>>>          - sound-dai
>>>>
>>>> +patternProperties:
>>>> +  ".*-dai-link$":
>>>> +    type: object
>>>> +    description:
>>>> +      Container for dai-link level properties and CODEC sub-nodes.
>>>> +
>>>> +    properties:
>>>> +      link-name:
>>>> +        description: Indicates dai-link name and PCM stream name
>>>> +        items:
>>>> +          enum:
>>>> +            - I2S0
>>>> +            - I2S1
>>>> +            - I2S2
>>>> +            - I2S3
>>>> +            - I2S4
>>>> +            - I2S5
>>>> +            - I2S6
>>>> +            - I2S7
>>>> +            - I2S8
>>>> +            - I2S9
>>>> +            - TDM
>>>> +
>>>> +      codec:
>>>> +        description: Holds subnode which indicates codec dai.
>>>> +        type: object
>>>> +        additionalProperties: false
>>>> +        properties:
>>>> +          sound-dai:
>>>> +            minItems: 1
>>>> +            maxItems: 2
>>>> +        required:
>>>> +          - sound-dai
>>>> +
>>>> +      dai-format:
>>>> +        description: audio format
>>>> +        items:
>>>> +          enum:
>>>> +            - i2s
>>>> +            - right_j
>>>> +            - left_j
>>>> +            - dsp_a
>>>> +            - dsp_b
>>>> +
>>>> +      mediatek,clk-provider:
>>>> +        $ref: /schemas/types.yaml#/definitions/string
>>>> +        description: Indicates dai-link clock master.
>>>> +        items:
>>>> +          enum:
>>>> +            - cpu
>>>> +            - codec
>>>> +
>>>> +    additionalProperties: false
>>>
>>> Move this before properties.
>>>
>>
>> Done for v2.
>>
>>>> +
>>>> +    required:
>>>> +      - link-name
>>>> +
>>>>    additionalProperties: false
>>>>
>>>>    required:
>>>>      - compatible
>>>>      - mediatek,platform
>>>> -  - headset-codec
>>>> -  - speaker-codecs
>>>> +
>>>> +allOf:
>>>> +  # Disallow dai-link-xxx nodes if the legacy properties are specified
>>>
>>> xxx-dai-link?
>>>
>>
>> Oh! Yes, thanks for catching this.
>>
>> That's what I initially wanted to do, but then I opted for xxx-dai-link and
>> forgot to update this comment.
>>
>> Fixed for v2.
>>
>>>> +  - if:
>>>> +      patternProperties:
>>>> +        ".*-dai-link$": false
>>>> +    then:
>>>> +      required:
>>>> +        - headset-codec
>>>> +        - speaker-codecs
>>>> +    else:
>>>> +      properties:
>>>> +        headset-codec: false
>>>> +        speaker-codecs: false
>>>> +        mediatek,hdmi-codec: false
>>>
>>> Allowing both would preserve compatibility. That's not needed? If so,
>>> say why in the commit msg.
>>>
>>
>> I'm thinking of writing:
>>
>> "Since describing machine specific audio hardware and links replaces the
>> now deprecated old logic doing the same in a driver hardcoded fashion,
>> it is not allowed to have both the old and new properties together."
> 
> What happened to that. Instead you just sent a new version with
> nothing about this.
> 

The same thing that happened to that card "model" error that shouldn't have
been there because I catched it before sending and fixed, then...

...I have ultimately sent the wrong changeset. My bad.

Anyway - since that's a bigger series, I'll wait for a few days and will
send the v3 with the names added to the audio-routing and this mentioned
in the commit description (so, that's happening next week).

>> ...but in short - both the old and the new can do exactly the same, but
>> imo it doesn't make any sense to actually rely on both as:
>>    1. It's redundant (and one set of them makes the other useless);
>>    2. I want to avoid confusion (as the other set won't be parsed);
>>    3. I'm trying to *enforce* consistency as MTK cards have different
>>       bindings for .. really, no good reason;
>>    4. I want to see custom stuff disappear completely (and/or as much as
>>       possible anyway) and use something that is (at least somewhat) common
>>       between all MTK and non-MTK or anyway as a start at least consistent
>>       between MTK cards.
>>
>> In theory, though, speaking of the driver side, there's nothing preventing
>> you from specifying both audio-routing xxx-dai-link and mediatek,hdmi-codec,
>> as the drivers' action will be, in short
>>      if (new_bindings)
>>        forget_about_old_bindings_use_the_new_ones();
>>      else
>>        use_old_hardcoded_stuff(); /* and be sad */
> 
> That works for newer kernels with this change, but existing kernels
> will only have:
> 
> use_old_hardcoded_stuff(); /* and not know it's sad */
> 
> If you want to support a new DT and old kernel, you need to populate
> both properties.
> 
>> For that, I really don't want to allow both sets of properties - please, please,
>> tell me that I don't *have to* remove this block :-)
> 
> Ultimately it is your decision as Mediatek maintainer, not mine. My
> only requirement is the commit message explain why the above
> combination is not important for these platforms.
> 
> You could leave it, but keep both in the dts files for some time
> period. That will cause warnings, but what's a few more. The ABI
> doesn't have to be a forever thing. Things evolve and there will be
> other reasons to upgrade.
> 

Thanks for explaining.

Cheers,
Angelo
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
index 7e50f5d65c8f..78e221003750 100644
--- a/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
+++ b/Documentation/devicetree/bindings/sound/mt8192-mt6359-rt1015-rt5682.yaml
@@ -20,6 +20,15 @@  properties:
       - mediatek,mt8192_mt6359_rt1015p_rt5682
       - mediatek,mt8192_mt6359_rt1015p_rt5682s
 
+  audio-routing:
+    $ref: /schemas/types.yaml#/definitions/non-unique-string-array
+    description:
+      A list of the connections between audio components. Each entry is a
+      pair of strings, the first being the connection's sink, the second
+      being the connection's source.
+      Valid names could be the input or output widgets of audio components,
+      power supplies, MicBias of codec and the software switch.
+
   mediatek,platform:
     $ref: /schemas/types.yaml#/definitions/phandle
     description: The phandle of MT8192 ASoC platform.
@@ -27,10 +36,12 @@  properties:
   mediatek,hdmi-codec:
     $ref: /schemas/types.yaml#/definitions/phandle
     description: The phandle of HDMI codec.
+    deprecated: true
 
   headset-codec:
     type: object
     additionalProperties: false
+    deprecated: true
 
     properties:
       sound-dai:
@@ -41,6 +52,7 @@  properties:
   speaker-codecs:
     type: object
     additionalProperties: false
+    deprecated: true
 
     properties:
       sound-dai:
@@ -51,13 +63,83 @@  properties:
     required:
       - sound-dai
 
+patternProperties:
+  ".*-dai-link$":
+    type: object
+    description:
+      Container for dai-link level properties and CODEC sub-nodes.
+
+    properties:
+      link-name:
+        description: Indicates dai-link name and PCM stream name
+        items:
+          enum:
+            - I2S0
+            - I2S1
+            - I2S2
+            - I2S3
+            - I2S4
+            - I2S5
+            - I2S6
+            - I2S7
+            - I2S8
+            - I2S9
+            - TDM
+
+      codec:
+        description: Holds subnode which indicates codec dai.
+        type: object
+        additionalProperties: false
+        properties:
+          sound-dai:
+            minItems: 1
+            maxItems: 2
+        required:
+          - sound-dai
+
+      dai-format:
+        description: audio format
+        items:
+          enum:
+            - i2s
+            - right_j
+            - left_j
+            - dsp_a
+            - dsp_b
+
+      mediatek,clk-provider:
+        $ref: /schemas/types.yaml#/definitions/string
+        description: Indicates dai-link clock master.
+        items:
+          enum:
+            - cpu
+            - codec
+
+    additionalProperties: false
+
+    required:
+      - link-name
+
 additionalProperties: false
 
 required:
   - compatible
   - mediatek,platform
-  - headset-codec
-  - speaker-codecs
+
+allOf:
+  # Disallow dai-link-xxx nodes if the legacy properties are specified
+  - if:
+      patternProperties:
+        ".*-dai-link$": false
+    then:
+      required:
+        - headset-codec
+        - speaker-codecs
+    else:
+      properties:
+        headset-codec: false
+        speaker-codecs: false
+        mediatek,hdmi-codec: false
 
 examples:
   - |
@@ -65,19 +147,50 @@  examples:
     sound: mt8192-sound {
         compatible = "mediatek,mt8192_mt6359_rt1015_rt5682";
         mediatek,platform = <&afe>;
-        mediatek,hdmi-codec = <&anx_bridge_dp>;
         pinctrl-names = "aud_clk_mosi_off",
                         "aud_clk_mosi_on";
         pinctrl-0 = <&aud_clk_mosi_off>;
         pinctrl-1 = <&aud_clk_mosi_on>;
 
-        headset-codec {
-            sound-dai = <&rt5682>;
+        audio-routing =
+                "Headphone Jack", "HPOL",
+                "Headphone Jack", "HPOR",
+                "IN1P", "Headset Mic",
+                "Speakers", "Speaker";
+
+        spk-playback-dai-link {
+                link-name = "I2S3";
+                dai-format = "i2s";
+                mediatek,clk-provider = "cpu";
+                codec {
+                        sound-dai = <&rt1015p>;
+                };
+        };
+
+        hs-playback-dai-link {
+                link-name = "I2S8";
+                dai-format = "i2s";
+                mediatek,clk-provider = "cpu";
+                codec {
+                        sound-dai = <&rt5682 0>;
+                };
+        };
+
+        hs-capture-dai-link {
+                link-name = "I2S9";
+                dai-format = "i2s";
+                mediatek,clk-provider = "cpu";
+                codec {
+                        sound-dai = <&rt5682 0>;
+                };
         };
 
-        speaker-codecs {
-            sound-dai = <&rt1015_l>,
-                        <&rt1015_r>;
+        displayport-dai-link {
+                link-name = "TDM";
+                dai-format = "dsp_a";
+                codec {
+                        sound-dai = <&anx_bridge_dp>;
+                };
         };
     };