| Message ID | 20240226-audio-i350-v3-0-16bb2c974c55@baylibre.com |
|---|---|
| Headers | show |
| Series | Add audio support for the MediaTek Genio 350-evk board | expand |
On 09/04/2024 15:41, Alexandre Mergnat wrote: > Add MT8365 audio front-end bindings > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > +properties: > + compatible: > + const: mediatek,mt8365-afe-pcm > + > + reg: > + maxItems: 1 > + > + "#sound-dai-cells": > + const: 0 > + > + clocks: > + items: > + - description: 26M clock > + - description: mux for audio clock > + - description: audio i2s0 mck > + - description: audio i2s1 mck > + - description: audio i2s2 mck > + - description: audio i2s3 mck > + - description: engen 1 clock > + - description: engen 2 clock > + - description: audio 1 clock > + - description: audio 2 clock > + - description: mux for i2s0 > + - description: mux for i2s1 > + - description: mux for i2s2 > + - description: mux for i2s3 > + > + clock-names: > + items: > + - const: top_clk26m_clk > + - const: top_audio_sel > + - const: audio_i2s0_m > + - const: audio_i2s1_m > + - const: audio_i2s2_m > + - const: audio_i2s3_m > + - const: engen1 > + - const: engen2 > + - const: aud1 > + - const: aud2 > + - const: i2s0_m_sel > + - const: i2s1_m_sel > + - const: i2s2_m_sel > + - const: i2s3_m_sel > + > + interrupts: > + maxItems: 1 > + > + power-domains: > + maxItems: 1 > + > + mediatek,dmic-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + Indicates how many data pins are used to transmit two channels of PDM > + signal. 1 means two wires, 0 means one wire. Default value is 0. > + enum: > + - 0 # one wire > + - 1 # two wires > + > + mediatek,topckgen: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: The phandle of the mediatek topckgen controller Nothing improved, so again, so something which is not obvious. What is it used for? Why AFE needs topckgen for example? > + > +required: > + - compatible > + - reg > + - clocks > + - clock-names > + - interrupts > + - power-domains > + - mediatek,topckgen > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/clock/mediatek,mt8365-clk.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/power/mediatek,mt8365-power.h> > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + > + afe@11220000 { Did you implement the comment or decided to keep afe? BTW, whatever "consistency" you have in mind, it does not really matter that much for that example. And for sure do not add incorrect code intentionally just to fix it in next patch. Best regards, Krzysztof
On 09/04/2024 15:42, Alexandre Mergnat wrote: > Add MT8365 audio codec bindings to set required > and optional voltage properties between the codec and the board. > The properties are: > - phandle of the requiered power supply. typo > - Setup of microphone bias voltage. > - Setup of the speaker pin pull-down. > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > .../devicetree/bindings/sound/mt6357.yaml | 54 ++++++++++++++++++++++ Filename using compatible syntax, so missing vendor prefix. > 1 file changed, 54 insertions(+) > > diff --git a/Documentation/devicetree/bindings/sound/mt6357.yaml b/Documentation/devicetree/bindings/sound/mt6357.yaml > new file mode 100644 > index 000000000000..381cb71b959f > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/mt6357.yaml > @@ -0,0 +1,54 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/mt6357.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Mediatek MT6357 Codec > + > +maintainers: > + - Alexandre Mergnat <amergnat@baylibre.com> > + > +description: | Do not need '|' unless you need to preserve formatting. > + This is the required and optional voltage properties for this subdevice. > + The communication between MT6357 and SoC is through Mediatek PMIC wrapper. > + For more detail, please visit Mediatek PMIC wrapper documentation. > + Must be a child node of PMIC wrapper. Why? > + > +properties: > + Drop blank line > + 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 > + > + mediatek,vaud28-supply: > + description: 2.8 volt supply phandle for the audio codec Supplies go without vendor prefixes. > + > +required: > + - mediatek,vaud28-supply That's basically no-op schema. I do not understand what you are trying to achieve here. > + > +additionalProperties: false > + > +examples: > + - | > + codec { > + mediatek,micbias0-microvolt = <1900000>; > + mediatek,micbias1-microvolt = <1700000>; > + mediatek,vaud28-supply = <&mt6357_vaud28_reg>; Sorry, this does not work. Change voltage to 1111111 and check the results. Best regards, Krzysztof
On 09/04/2024 15:42, Alexandre Mergnat wrote: > Add audio codec support of MT6357 PMIC. > Update the file header. Why? > > Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> > --- > arch/arm64/boot/dts/mediatek/mt6357.dtsi | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/mediatek/mt6357.dtsi b/arch/arm64/boot/dts/mediatek/mt6357.dtsi > index 3330a03c2f74..ade410851524 100644 > --- a/arch/arm64/boot/dts/mediatek/mt6357.dtsi > +++ b/arch/arm64/boot/dts/mediatek/mt6357.dtsi > @@ -1,7 +1,7 @@ > // SPDX-License-Identifier: (GPL-2.0 OR MIT) > /* > * Copyright (c) 2020 MediaTek Inc. > - * Copyright (c) 2023 BayLibre Inc. > + * Copyright (c) 2024 BayLibre Inc. That's not a reasonable change. The file was published on 2023, wasn't it? If this is not correct, please explain why/how and make it separate patch. > */ > > #include <dt-bindings/input/input.h> > @@ -10,6 +10,9 @@ &pwrap { > mt6357_pmic: pmic { > compatible = "mediatek,mt6357"; > > + mt6357_codec: codec { > + }; There is no single point of having empty nodes. NAK. Best regards, Krzysztof
On 09/04/2024 17:46, Krzysztof Kozlowski wrote: >> + soc { >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + afe@11220000 { > Did you implement the comment or decided to keep afe? > Though it was clear according to [1]: " Audio Front End, this is the same name used for other MTK SoC, to be consistent. Cook a new patch serie to change "afe" by "audio-controller" for all MTK SoC would be great. " I want to keep it and fix it later with ALL other MTK SoC. You didn't answer after that, I though it was ok for you... So if you don't agree with that, just tell me to change it instead of let me think I've the choice. > BTW, whatever "consistency" you have in mind, it does not really matter > that much for that example. And for sure do not add incorrect code > intentionally just to fix it in next patch. I don't get your point. What do you mean by "do not add incorrect code intentionally" please ??? My PoV: I implement my bindings aligned to ALL other MTK SoC, which have been validated by you in the past. I can understand the copied code is wrong, but I've suggested a solution and you didn't NACK it. I fell like you bully me. Are you ok to stop insinuating bad things please and tell me directly if a fix is mandatory and if you are waiting for a specific one, tell me directly to avoid this kind of unpleasant talk and waste of time, I know maintainers are busy persons. Beside that, thanks for your reviews. [1]: https://lore.kernel.org/all/eeb3329b-0558-4237-8343-4e11f65a6a35@baylibre.com/
On 10/04/2024 11:29, Alexandre Mergnat wrote: > > > On 09/04/2024 17:46, Krzysztof Kozlowski wrote: >>> + soc { >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + >>> + afe@11220000 { >> Did you implement the comment or decided to keep afe? >> > > Though it was clear according to [1]: > " > Audio Front End, this is the same name used for other MTK SoC, to be > consistent. > > Cook a new patch serie to change "afe" by "audio-controller" for all MTK > SoC would be great. > " > > I want to keep it and fix it later with ALL other MTK SoC. > You didn't answer after that, I though it was ok for you... Then no, I don't agree. If you add code, which you already plan to fix, it means the code is not correct somehow. Then just add correct code in the beginning. Best regards, Krzysztof
On 09/04/2024 17:55, Krzysztof Kozlowski wrote: >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + codec { >> + mediatek,micbias0-microvolt = <1900000>; >> + mediatek,micbias1-microvolt = <1700000>; >> + mediatek,vaud28-supply = <&mt6357_vaud28_reg>; > Sorry, this does not work. Change voltage to 1111111 and check the results. Actually it's worst ! I've removed the required property (vaud28-supply) but the dt check pass. Same behavior for some other docs like mt6359.yaml The at24.yaml doc works as expected, then I tried compare an find the issue, without success... I've replaced "codec" by "audio-codec", according to [1]. I've tried multiple manner to implement the example code, without success. I'm wondering if what I try to do is the correct way or parse-able by the dt_check. If I drop this file and implement all these new properties into the MFD PMIC documentation directly, I've the expected dt_check result (function to good or wrong parameters) +++ b/Documentation/devicetree/bindings/mfd/mediatek,mt6357.yaml @@ -37,9 +37,32 @@ properties: "#interrupt-cells": const: 2 - codec: + audio-codec: type: object - $ref: /schemas/sound/mt6357.yaml + properties: + vaud28-supply: + description: 2.8 volt supply phandle for the audio codec + + 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 + + required: + - vaud28-supply unevaluatedProperties: false regulators: @@ -88,6 +111,12 @@ examples: interrupt-controller; #interrupt-cells = <2>; + audio-codec { + mediatek,micbias0-microvolt = <1700000>; + mediatek,micbias1-microvolt = <1700000>; + vaud28-supply = <&mt6357_vaud28_reg>; + }; + regulators { Is the implementation above looks good for you ? [1] https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
On 23/04/2024 19:07, Alexandre Mergnat wrote: > > > On 09/04/2024 17:55, Krzysztof Kozlowski wrote: >>> + >>> +additionalProperties: false >>> + >>> +examples: >>> + - | >>> + codec { >>> + mediatek,micbias0-microvolt = <1900000>; >>> + mediatek,micbias1-microvolt = <1700000>; >>> + mediatek,vaud28-supply = <&mt6357_vaud28_reg>; >> Sorry, this does not work. Change voltage to 1111111 and check the results. > > Actually it's worst ! I've removed the required property (vaud28-supply) but the dt check pass. > Same behavior for some other docs like mt6359.yaml Yeah, the schema is not applied. There is nothing selecting it, so this is no-op schema. I don't know what exactly you want to describe, but usually either you miss compatible or this should be just part of parent node. > > The at24.yaml doc works as expected, then I tried compare an find the issue, without success... > > I've replaced "codec" by "audio-codec", according to [1]. > I've tried multiple manner to implement the example code, without success. I'm wondering if what I > try to do is the correct way or parse-able by the dt_check. > > If I drop this file and implement all these new properties into the MFD PMIC documentation directly, > I've the expected dt_check result (function to good or wrong parameters) Yes. Best regards, Krzysztof
This serie aim to add the following audio support for the Genio 350-evk: - Playback - 2ch Headset Jack (Earphone) - 1ch Line-out Jack (Speaker) - 8ch HDMI Tx - Capture - 1ch DMIC (On-board Digital Microphone) - 1ch AMIC (On-board Analogic Microphone) - 1ch Headset Jack (External Analogic Microphone) Of course, HDMI playback need the MT8365 display patches [1] and a DTS change documented in "mediatek,mt8365-mt6357.yaml". Rebase on top of sound/for-next branch and the Angelo's serie "SoC: Cleanup MediaTek soundcard machine drivers" [2] Work branch with all patches [5] Applied patch: - mfd: mt6397-core: register mt6357 sound codec Test passed: - mixer-test log: [3] - pcm-test log: [4] [1]: https://lore.kernel.org/all/20231023-display-support-v1-0-5c860ed5c33b@baylibre.com/ [2]: https://lore.kernel.org/all/20240313110147.1267793-1-angelogioacchino.delregno@collabora.com/ [3]: https://pastebin.com/pc43AVrT [4]: https://pastebin.com/cCtGhDpg [5]: https://gitlab.baylibre.com/baylibre/mediatek/bsp/linux/-/commits/sound/for-next/add-i350-audio-support Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com> --- Changes in v3: - Re-order documentation commit to fix dt_binding_check error. - Remove $ref and add "mediatek," prefix to vaud28-supply property. - Link to v2: https://lore.kernel.org/r/20240226-audio-i350-v2-0-3043d483de0d@baylibre.com Changes in v2: - Documentation fixed: - Remove spurious description. - Change property order to fit with dts coding style rules. - micbias property: use microvolt value instead of index. - mediatek,i2s-shared-clock property removed. - mediatek,dmic-iir-on property removed. - mediatek,dmic-irr-mode property removed. - Change dmic-two-wire-mode => dmic-mode to be aligned with another SoC - Remove the spurious 2nd reg of the afe. - Manage IIR filter feature using audio controls. - Fix audio controls to pass mixer-test and pcm-test. - Refactor some const name according to feedbacks. - Rework the codec to remove spurious driver data. - Use the new common MTK probe functions for AFE PCM and sound card. - Rework pinctrl probe in the soundcard driver. - Remove spurious "const" variables in all files. - Link to v1: https://lore.kernel.org/r/20240226-audio-i350-v1-0-4fa1cea1667f@baylibre.com --- Alexandre Mergnat (16): ASoC: dt-bindings: mediatek,mt8365-afe: Add audio afe document ASoC: dt-bindings: mediatek,mt8365-mt6357: Add audio sound card document ASoC: dt-bindings: mt6357: Add audio codec document dt-bindings: mfd: mediatek: Add codec property for MT6357 PMIC ASoC: mediatek: mt8365: Add common header SoC: mediatek: mt8365: support audio clock control ASoC: mediatek: mt8365: Add I2S DAI support ASoC: mediatek: mt8365: Add ADDA DAI support ASoC: mediatek: mt8365: Add DMIC DAI support ASoC: mediatek: mt8365: Add PCM DAI support ASoC: mediatek: mt8365: Add platform driver ASoC: mediatek: Add MT8365 support arm64: defconfig: enable mt8365 sound arm64: dts: mediatek: add mt6357 audio codec support arm64: dts: mediatek: add afe support for mt8365 SoC arm64: dts: mediatek: add audio support for mt8365-evk Nicolas Belin (2): ASoc: mediatek: mt8365: Add a specific soundcard for EVK ASoC: codecs: add MT6357 support .../devicetree/bindings/mfd/mediatek,mt6357.yaml | 5 + .../bindings/sound/mediatek,mt8365-afe.yaml | 136 ++ .../bindings/sound/mediatek,mt8365-mt6357.yaml | 99 + .../devicetree/bindings/sound/mt6357.yaml | 54 + arch/arm64/boot/dts/mediatek/mt6357.dtsi | 5 +- arch/arm64/boot/dts/mediatek/mt8365-evk.dts | 98 +- arch/arm64/boot/dts/mediatek/mt8365.dtsi | 46 +- arch/arm64/configs/defconfig | 2 + sound/soc/codecs/Kconfig | 7 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/mt6357.c | 1898 ++++++++++++++++ sound/soc/codecs/mt6357.h | 662 ++++++ sound/soc/mediatek/Kconfig | 20 + sound/soc/mediatek/Makefile | 1 + sound/soc/mediatek/mt8365/Makefile | 15 + sound/soc/mediatek/mt8365/mt8365-afe-clk.c | 451 ++++ sound/soc/mediatek/mt8365/mt8365-afe-clk.h | 49 + sound/soc/mediatek/mt8365/mt8365-afe-common.h | 491 +++++ sound/soc/mediatek/mt8365/mt8365-afe-pcm.c | 2275 ++++++++++++++++++++ sound/soc/mediatek/mt8365/mt8365-dai-adda.c | 315 +++ sound/soc/mediatek/mt8365/mt8365-dai-dmic.c | 347 +++ sound/soc/mediatek/mt8365/mt8365-dai-i2s.c | 854 ++++++++ sound/soc/mediatek/mt8365/mt8365-dai-pcm.c | 293 +++ sound/soc/mediatek/mt8365/mt8365-mt6357.c | 348 +++ sound/soc/mediatek/mt8365/mt8365-reg.h | 991 +++++++++ 25 files changed, 9456 insertions(+), 8 deletions(-) --- base-commit: 6a3d4a830e4e9de8e8aefc233d790bef4a5c0037 change-id: 20240226-audio-i350-4e11da088e55 Best regards,