Re: [PATCH 02/18] ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document

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

Il 27/02/24 11:23, Alexandre Mergnat ha scritto:
> 
> 
> On 26/02/2024 16:30, AngeloGioacchino Del Regno wrote:
>> Il 26/02/24 15:01, Alexandre Mergnat ha scritto:
>>> Add soundcard bindings for the MT8365 SoC with the MT6357 audio codec.
>>>
>>> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
>>> ---
>>>   .../bindings/sound/mediatek,mt8365-mt6357.yaml     | 127 +++++++++++++++++++++
>>>   1 file changed, 127 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt8365-mt6357.yaml 
>>> b/Documentation/devicetree/bindings/sound/mediatek,mt8365-mt6357.yaml
>>> new file mode 100644
>>> index 000000000000..f469611ec6b6
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/sound/mediatek,mt8365-mt6357.yaml
>>> @@ -0,0 +1,127 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/sound/mediatek,mt8365-mt6357.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Mediatek MT8365 sound card with MT6357 sound codec.
>>> +
>>> +maintainers:
>>> +  - Alexandre Mergnat <amergnat@baylibre.com>
>>> +
>>> +description:
>>> +  This binding describes the MT8365 sound card.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: mediatek,mt8365-mt6357
>>> +
>>> +  mediatek,hp-pull-down:
>>> +    description:
>>> +      Earphone driver positive output stage short to the
>>> +      audio reference ground.
>>> +      Default value is false.
>>> +    type: boolean
>>> +
>>> +  mediatek,micbias0-microvolt:
>>> +    description: |
>>
>> description: Selects MIC Bias 0 output voltage
>>
>>> +      Selects MIC Bias 0 output voltage.
>>> +      [1.7v, 1.8v, 1.9v, 2.0v, 2.1v, 2.5v, 2.6v, 2.7v]
>>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7]
>>
>> No, you don't say 0 1 2 3 4 to a property that says "microvolt", that's simply
>> wrong.
>>
>> mediatek,micbias0-microvolt = <2100000>;
>>
>> ...so you want a binding that says
>> enum: [ 1700000, 1800000, this, that, 2700000]
>>
> 
> Is it correct if I put "description: Selects MIC Bias 0 output voltage index" ?
> 

No, it's still not correct. You have to pass microvolt values.

The driver will then transform the microvolt values to the index and subsequently
write the index value to the hardware registers.

The bindings shall be generic, in the sense that they shall not express hardware
register values... and this is especially true when we have a value that *does*
actually have means to be expressed in common units.

Besides, in the cases in which there's no common units involved, the values most
probably won't be suited for devicetree//bindings, so those would be hardcoded in
the driver as platform data.

This is not the case, so, please keep this property but specify microvolts in the
bindings (and obviously in devicetree).

>>> +
>>> +  mediatek,micbias1-microvolt:
>>> +    description: |
>>> +      Selects MIC Bias 1 output voltage.
>>> +      [1.7v, 1.8v, 1.9v, 2.0v, 2.1v, 2.5v, 2.6v, 2.7v]
>>> +    enum: [0, 1, 2, 3, 4, 5, 6, 7]
>>
>> same here.
>>
>>> +
>>> +  mediatek,platform:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description: The phandle of MT8365 ASoC platform.
>>> +
>>> +  pinctrl-names:
>>> +    minItems: 1
>>> +    items:
>>> +      - const: aud_default
>>> +      - const: aud_dmic
>>> +      - const: aud_miso_off
>>> +      - const: aud_miso_on
>>> +      - const: aud_mosi_off
>>> +      - const: aud_mosi_on
>>> +
>>> +  vaud28-supply:
>>> +    description:
>>> +      2.8 volt supply for the audio codec
>>> +
>>> +patternProperties:
>>> +  "^dai-link-[0-9]+$":
>>> +    type: object
>>> +    description:
>>> +      Container for dai-link level properties and CODEC sub-nodes.
>>> +
>>> +    properties:
>>> +      codec:
>>> +        type: object
>>> +        description: Holds subnode which indicates codec dai.
>>> +
>>> +        properties:
>>> +          sound-dai:
>>> +            maxItems: 1
>>> +            description: phandle of the codec DAI
>>> +
>>> +        additionalProperties: false
>>> +
>>> +      link-name:
>>> +        description:
>>> +          This property corresponds to the name of the BE dai-link to which
>>> +          we are going to update parameters in this node.
>>> +        items:
>>> +          const: 2ND I2S BE
>>> +
>>> +      sound-dai:
>>> +        maxItems: 1
>>> +        description: phandle of the CPU DAI
>>> +
>>> +    additionalProperties: false
>>> +
>>> +    required:
>>> +      - link-name
>>> +      - sound-dai
>>> +
>>> +additionalProperties: false
>>> +
>>> +required:
>>> +  - compatible
>>> +  - mediatek,platform
>>> +  - pinctrl-names
>>> +  - vaud28-supply
>>> +
>>> +examples:
>>> +  - |
>>> +    sound {
>>> +        compatible = "mediatek,mt8365-mt6357";
>>> +        mediatek,platform = <&afe>;
>>
>> Please:
>>
>> https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
> 
> Is it about the wrong pinctrl-names tab alignment ?
> Also, 2ND I2S BE => 2ND_I2S_BE ?
> Otherwise, I don't get it sorry.
> 

...as Krzysztof already clarified, won't repeat :-P

Cheers!

>>
>> Regards,
>> Angelo
>>
>>> +        pinctrl-names = "aud_default",
>>> +            "aud_dmic",
>>> +            "aud_miso_off",
>>> +            "aud_miso_on",
>>> +            "aud_mosi_off",
>>> +            "aud_mosi_on";
>>> +        pinctrl-0 = <&aud_default_pins>;
>>> +        pinctrl-1 = <&aud_dmic_pins>;
>>> +        pinctrl-2 = <&aud_miso_off_pins>;
>>> +        pinctrl-3 = <&aud_miso_on_pins>;
>>> +        pinctrl-4 = <&aud_mosi_off_pins>;
>>> +        pinctrl-5 = <&aud_mosi_on_pins>;
>>> +        vaud28-supply = <&mt6357_vaud28_reg>;
>>> +
>>> +        /* hdmi interface */
>>> +        dai-link-0 {
>>> +            sound-dai = <&afe>;
>>> +            link-name = "2ND I2S BE";
>>> +
>>> +            codec {
>>> +                sound-dai = <&it66121hdmitx>;
>>> +            };
>>> +        };
>>> +    };
>>>
>>
>