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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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