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

[Alexandre Mergnat <amergnat@baylibre.com>]

On 27/02/2024 13:38, AngeloGioacchino Del Regno wrote:
> 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).

Got it, thx !

> 
>>>> +
>>>> +  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

Yes it is, I will fix that.

> 
> 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>;
>>>> +            };
>>>> +        };
>>>> +    };
>>>>
>>>
>>
> 
> 

-- 
Regards,
Alexandre