| Message ID | 20240226-audio-i350-v6-3-f754ec1a7634@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add audio support for the MediaTek Genio 350-evk board | expand |
On 19/06/2024 16:46, Alexandre Mergnat wrote: > Add the audio codec sub-device. This sub-device is used to set the > optional voltage values according to the hardware. > The properties are: > - Setup of microphone bias voltage. > - Setup of the speaker pin pull-down. > > Also, add the audio power supply property which is dedicated for > the audio codec sub-device. > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > index 37423c2e0fdf..d95307393e75 100644 > --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml > @@ -37,6 +37,32 @@ properties: > "#interrupt-cells": > const: 2 > > + vaud28-supply: > + description: 2.8 volt supply phandle for the audio codec > + > + audio-codec: > + type: object Still not much improved. You do not have any resources there, so these should go to the parent node. Best regards, Krzysztof
On 21/06/2024 17:00, Krzysztof Kozlowski wrote: > On 19/06/2024 16:46, Alexandre Mergnat wrote: >> Add the audio codec sub-device. This sub-device is used to set the >> optional voltage values according to the hardware. >> The properties are: >> - Setup of microphone bias voltage. >> - Setup of the speaker pin pull-down. >> >> Also, add the audio power supply property which is dedicated for >> the audio codec sub-device. >> >> Signed-off-by: Alexandre Mergnat<amergnat@baylibre.com> >> --- >> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >> index 37423c2e0fdf..d95307393e75 100644 >> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >> @@ -37,6 +37,32 @@ properties: >> "#interrupt-cells": >> const: 2 >> >> + vaud28-supply: >> + description: 2.8 volt supply phandle for the audio codec >> + >> + audio-codec: >> + type: object > Still not much improved. You do not have any resources there, so these > should go to the parent node. Hi Krzysztof, vaud28-supply seems to be a mistake that I forward port. In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as power-supply. pwrap { pmic { ... audio-codec { ... Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of PMIC-audio-codec subsystem. Then: - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec substystem, beside mediatek,micbias0-microvolt Does this sound good to you ?
On 25/06/2024 11:23, Alexandre Mergnat wrote: > > > On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >> On 19/06/2024 16:46, Alexandre Mergnat wrote: >>> Add the audio codec sub-device. This sub-device is used to set the >>> optional voltage values according to the hardware. >>> The properties are: >>> - Setup of microphone bias voltage. >>> - Setup of the speaker pin pull-down. >>> >>> Also, add the audio power supply property which is dedicated for >>> the audio codec sub-device. >>> >>> Signed-off-by: Alexandre Mergnat<amergnat@baylibre.com> >>> --- >>> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >>> 1 file changed, 33 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> index 37423c2e0fdf..d95307393e75 100644 >>> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>> @@ -37,6 +37,32 @@ properties: >>> "#interrupt-cells": >>> const: 2 >>> >>> + vaud28-supply: >>> + description: 2.8 volt supply phandle for the audio codec >>> + >>> + audio-codec: >>> + type: object >> Still not much improved. You do not have any resources there, so these >> should go to the parent node. > > Hi Krzysztof, > > vaud28-supply seems to be a mistake that I forward port. > In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" > sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as > power-supply. > > pwrap { > pmic { > ... > audio-codec { > ... > > Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of > PMIC-audio-codec subsystem. Then: > - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a > power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) > - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to > Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec > substystem, beside mediatek,micbias0-microvolt I don't understand why do we talk again about supply. My comment was not under the supply. Best regards, Krzysztof
On 25/06/2024 15:44, Krzysztof Kozlowski wrote: > On 25/06/2024 11:23, Alexandre Mergnat wrote: >> >> >> On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >>> On 19/06/2024 16:46, Alexandre Mergnat wrote: >>>> Add the audio codec sub-device. This sub-device is used to set the >>>> optional voltage values according to the hardware. >>>> The properties are: >>>> - Setup of microphone bias voltage. >>>> - Setup of the speaker pin pull-down. >>>> >>>> Also, add the audio power supply property which is dedicated for >>>> the audio codec sub-device. >>>> >>>> Signed-off-by: Alexandre Mergnat<amergnat@baylibre.com> >>>> --- >>>> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >>>> 1 file changed, 33 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>> index 37423c2e0fdf..d95307393e75 100644 >>>> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>> @@ -37,6 +37,32 @@ properties: >>>> "#interrupt-cells": >>>> const: 2 >>>> >>>> + vaud28-supply: >>>> + description: 2.8 volt supply phandle for the audio codec >>>> + >>>> + audio-codec: >>>> + type: object >>> Still not much improved. You do not have any resources there, so these >>> should go to the parent node. >> >> Hi Krzysztof, >> >> vaud28-supply seems to be a mistake that I forward port. >> In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" >> sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as >> power-supply. >> >> pwrap { >> pmic { >> ... >> audio-codec { >> ... >> >> Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of >> PMIC-audio-codec subsystem. Then: >> - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a >> power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) >> - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to >> Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec >> substystem, beside mediatek,micbias0-microvolt > > I don't understand why do we talk again about supply. My comment was not > under the supply. Because your word are: " And now you should see how odd it looks. Supplies are part of entire chip, not subblock, even if they supply dedicated domain within that chip. That's why I asked to put it in the parent node. " My bad, I forgot to link you the old message in my previous answer [1] [1] https://lore.kernel.org/all/6d21da37-8be7-467c-8878-d57af0b0201b@kernel.org/#t
On 26/06/2024 10:30, Alexandre Mergnat wrote: > > > On 25/06/2024 15:44, Krzysztof Kozlowski wrote: >> On 25/06/2024 11:23, Alexandre Mergnat wrote: >>> >>> >>> On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >>>> On 19/06/2024 16:46, Alexandre Mergnat wrote: >>>>> Add the audio codec sub-device. This sub-device is used to set the >>>>> optional voltage values according to the hardware. >>>>> The properties are: >>>>> - Setup of microphone bias voltage. >>>>> - Setup of the speaker pin pull-down. >>>>> >>>>> Also, add the audio power supply property which is dedicated for >>>>> the audio codec sub-device. >>>>> >>>>> Signed-off-by: Alexandre Mergnat<amergnat@baylibre.com> >>>>> --- >>>>> .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ >>>>> 1 file changed, 33 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> index 37423c2e0fdf..d95307393e75 100644 >>>>> --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml >>>>> @@ -37,6 +37,32 @@ properties: >>>>> "#interrupt-cells": >>>>> const: 2 >>>>> >>>>> + vaud28-supply: >>>>> + description: 2.8 volt supply phandle for the audio codec >>>>> + >>>>> + audio-codec: >>>>> + type: object >>>> Still not much improved. You do not have any resources there, so these >>>> should go to the parent node. >>> >>> Hi Krzysztof, >>> >>> vaud28-supply seems to be a mistake that I forward port. >>> In the V4, AFAII, your feedback [1] suggested me to move the vaud28-supply from the "audio-codec" >>> sub-node to the parent node, which for me is the "pmic" (mfd), because the property is considered as >>> power-supply. >>> >>> pwrap { >>> pmic { >>> ... >>> audio-codec { >>> ... >>> >>> Hardware side, vaud28-supply is the output of PMIC-regulator subsystem, and AVDD28 is the input of >>> PMIC-audio-codec subsystem. Then: >>> - The property name is wrong and must be change to AVDD28, which is a consumer (power input), not a >>> power-supply. => description: 2.8 volt power input for microphones (AU_VIN0, AU_VIN1, AU_VIN2) >>> - IMHO, move this property to the next parent (pwrap) isn't consistent. It should be moved back to >>> Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml (Done in the V4) into audio-codec >>> substystem, beside mediatek,micbias0-microvolt >> >> I don't understand why do we talk again about supply. My comment was not >> under the supply. > > Because your word are: > " > And now you should see how odd it looks. Supplies are part of entire > chip, not subblock, even if they supply dedicated domain within that chip. > > That's why I asked to put it in the parent node. > " > > My bad, I forgot to link you the old message in my previous answer [1] > > [1] https://lore.kernel.org/all/6d21da37-8be7-467c-8878-d57af0b0201b@kernel.org/#t And you implemented this, so why do we talk again about it? It is already solved, isn't it? Since previous version? Best regards, Krzysztof
On 21/06/2024 17:00, Krzysztof Kozlowski wrote: >> + >> + audio-codec: >> + type: object > > Still not much improved. You do not have any resources there, so these > should go to the parent node. Just to clarify: comment is about audio-codec. I meant what's inside audio-codec. "Parent node" -> parent of audio-codec, so the device node. I guess this was inaccurate. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml index 37423c2e0fdf..d95307393e75 100644 --- a/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml @@ -37,6 +37,32 @@ properties: "#interrupt-cells": const: 2 + vaud28-supply: + description: 2.8 volt supply phandle for the audio codec + + audio-codec: + type: object + properties: + mediatek,hp-pull-down: + description: + Earphone driver positive output stage short to + the audio reference ground. + type: boolean + + mediatek,micbias0-microvolt: + description: Selects MIC Bias 0 output voltage. + enum: [1700000, 1800000, 1900000, 2000000, + 2100000, 2500000, 2600000, 2700000] + default: 1700000 + + mediatek,micbias1-microvolt: + description: Selects MIC Bias 1 output voltage. + enum: [1700000, 1800000, 1900000, 2000000, + 2100000, 2500000, 2600000, 2700000] + default: 1700000 + + unevaluatedProperties: false + regulators: type: object $ref: /schemas/regulator/mediatek,mt6357-regulator.yaml @@ -67,6 +93,7 @@ properties: required: - compatible - regulators + - vaud28-supply additionalProperties: false @@ -81,8 +108,14 @@ examples: interrupt-parent = <&pio>; interrupts = <145 IRQ_TYPE_LEVEL_HIGH>; interrupt-controller; + vaud28-supply = <&mt6357_vaud28_reg>; #interrupt-cells = <2>; + audio-codec { + mediatek,micbias0-microvolt = <1700000>; + mediatek,micbias1-microvolt = <1700000>; + }; + regulators { mt6357_vproc_reg: buck-vproc { regulator-name = "vproc";
Add the audio codec sub-device. This sub-device is used to set the optional voltage values according to the hardware. The properties are: - Setup of microphone bias voltage. - Setup of the speaker pin pull-down. Also, add the audio power supply property which is dedicated for the audio codec sub-device. Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> --- .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+)