| Message ID | 20221208033148.21866-1-trevor.wu@mediatek.com |
|---|---|
| Headers | show |
| Series | ASoC: mediatek: Add support for MT8188 SoC | expand |
On 08/12/2022 04:31, Trevor Wu wrote: > Add mt8188 audio afe document. Use subject prefixes matching the subsystem (git log --oneline -- ...). > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> > --- > .../devicetree/bindings/sound/mt8188-afe.yaml | 196 ++++++++++++++++++ > 1 file changed, 196 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/mt8188-afe.yaml > > diff --git a/Documentation/devicetree/bindings/sound/mt8188-afe.yaml b/Documentation/devicetree/bindings/sound/mt8188-afe.yaml > new file mode 100644 > index 000000000000..6ab26494d924 > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/mt8188-afe.yaml This is a friendly reminder during the review process. It seems my previous comments were not fully addressed. Maybe my feedback got lost between the quotes, maybe you just forgot to apply it. Please go back to the previous discussion and either implement all requested changes or keep discussing them. Thank you. Comment was about filename matching compatible, so with vendor prefix. > @@ -0,0 +1,196 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/mt8188-afe.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MediaTek AFE PCM controller for mt8188 > + > +maintainers: > + - Trevor Wu <trevor.wu@mediatek.com> > + > +properties: > + compatible: > + const: mediatek,mt8188-afe > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + resets: > + maxItems: 1 > + > + reset-names: > + const: audiosys > + > + mediatek,topckgen: > + $ref: /schemas/types.yaml#/definitions/phandle > + description: The phandle of the mediatek topckgen controller > + > + power-domains: > + maxItems: 1 > + > + clocks: > + items: > + - description: 26M clock > + - description: audio pll1 clock > + - description: audio pll2 clock > + - description: clock divider for i2si1_mck > + - description: clock divider for i2si2_mck > + - description: clock divider for i2so1_mck > + - description: clock divider for i2so2_mck > + - description: clock divider for dptx_mck > + - description: a1sys hoping clock > + - description: audio intbus clock > + - description: audio hires clock > + - description: audio local bus clock > + - description: mux for dptx_mck > + - description: mux for i2so1_mck > + - description: mux for i2so2_mck > + - description: mux for i2si1_mck > + - description: mux for i2si2_mck > + - description: audio 26m clock > + > + clock-names: > + items: > + - const: clk26m > + - const: apll1_ck > + - const: apll2_ck > + - const: apll12_div0 > + - const: apll12_div1 > + - const: apll12_div2 > + - const: apll12_div3 > + - const: apll12_div9 > + - const: a1sys_hp_sel > + - const: aud_intbus_sel > + - const: audio_h_sel > + - const: audio_local_bus_sel > + - const: dptx_m_sel > + - const: i2so1_m_sel > + - const: i2so2_m_sel > + - const: i2si1_m_sel > + - const: i2si2_m_sel > + - const: adsp_audio_26m > + > +patternProperties: > + "^mediatek,etdm-in[1-2]-chn-disabled$": > + $ref: /schemas/types.yaml#/definitions/uint8-array > + minItems: 1 > + maxItems: 16 > + description: > + By default, all data received from ETDM pins will be outputed to > + memory. etdm in supports disable_out in direct mode(w/o interconn). > + User can specify the channel ID which they hope dropping and then > + the specified channel won't be seen on memory. So we know what are the IDs but it's a mystery what are the values. Especially with unique values - how any of these should case that channel "won't be seen in memory"? > + uniqueItems: true > + items: > + minimum: 0 > + maximum: 15 > + > + "^mediatek,etdm-in[1-2]-mclk-always-on-rate-hz$": > + description: Specify etdm in mclk output rate for always on case. How is it different than assigned-clock-rates? > + > + "^mediatek,etdm-out[1-3]-mclk-always-on-rate-hz$": > + description: Specify etdm out mclk output rate for always on case. > + > + "^mediatek,etdm-in[1-2]-multi-pin-mode$": > + type: boolean > + description: if present, the etdm data mode is I2S. > + > + "^mediatek,etdm-out[1-3]-multi-pin-mode$": > + type: boolean > + description: if present, the etdm data mode is I2S. > + > + "^mediatek,etdm-in[1-2]-cowork-source$": > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: > + etdm modules can share the same external clock pin. Specify > + which etdm clock source is required by this etdm in moudule. typo: module > + enum: > + - 0 # etdm1_in > + - 1 # etdm2_in > + - 2 # etdm1_out I don't get. This suggests that etdm1_out can be clock source of etdm-in1. Or etdm1_in can be clock source of etdm-in1... It does not make sense... > + - 3 # etdm2_out > + > + "^mediatek,etdm-out[1-2]-cowork-source$": > + $ref: /schemas/types.yaml#/definitions/uint32 > + description: | > + etdm modules can share the same external clock pin. Specify > + which etdm clock source is required by this etdm out moudule. > + enum: > + - 0 # etdm1_in > + - 1 # etdm2_in > + - 2 # etdm1_out > + - 3 # etdm2_out > + > +required: > + - compatible > + - reg > + - interrupts > + - resets > + - reset-names > + - mediatek,topckgen > + - power-domains > + - clocks > + - clock-names > + Best regards, Krzysztof
On Fri, 2022-12-09 at 11:15 +0100, Krzysztof Kozlowski wrote: > On 08/12/2022 04:31, Trevor Wu wrote: > > Add mt8188 audio afe document. > > Use subject prefixes matching the subsystem (git log --oneline -- > ...). > > > > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> > > --- > > .../devicetree/bindings/sound/mt8188-afe.yaml | 196 > > ++++++++++++++++++ > > 1 file changed, 196 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/sound/mt8188- > > afe.yaml > > > > diff --git a/Documentation/devicetree/bindings/sound/mt8188- > > afe.yaml b/Documentation/devicetree/bindings/sound/mt8188-afe.yaml > > new file mode 100644 > > index 000000000000..6ab26494d924 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/sound/mt8188-afe.yaml > > > This is a friendly reminder during the review process. > > It seems my previous comments were not fully addressed. Maybe my > feedback got lost between the quotes, maybe you just forgot to apply > it. > Please go back to the previous discussion and either implement all > requested changes or keep discussing them. > > Thank you. > > Comment was about filename matching compatible, so with vendor > prefix. Hi Krzysztof, Thanks for your review first. I aplogyize for my misunderstanding to your comment. I just renamed the file name(mt8188-afe-pcm.yaml -> mt8188-afe.yaml), and I didn't notice vendor prefix should be included. I will correct the name in v4. > > > @@ -0,0 +1,196 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: > > https://urldefense.com/v3/__http://devicetree.org/schemas/sound/mt8188-afe.yaml*__;Iw!!CTRNKA9wMg0ARbw!kiYsaIYR-KZ9KTQq3rOBRr3GeIC2UtnjFMWMKExmd_V7pW75AaEz30DuKv0nifWtMlknW8os7MiJ2pCCVT7ixRMsg7TpXw$ > > > > +$schema: > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!kiYsaIYR-KZ9KTQq3rOBRr3GeIC2UtnjFMWMKExmd_V7pW75AaEz30DuKv0nifWtMlknW8os7MiJ2pCCVT7ixRO-2YqhFw$ > > > > + > > +title: MediaTek AFE PCM controller for mt8188 > > + > > +maintainers: > > + - Trevor Wu <trevor.wu@mediatek.com> > > + > > +properties: > > + compatible: > > + const: mediatek,mt8188-afe > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + resets: > > + maxItems: 1 > > + > > + reset-names: > > + const: audiosys > > + > > + mediatek,topckgen: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: The phandle of the mediatek topckgen controller > > + > > + power-domains: > > + maxItems: 1 > > + > > + clocks: > > + items: > > + - description: 26M clock > > + - description: audio pll1 clock > > + - description: audio pll2 clock > > + - description: clock divider for i2si1_mck > > + - description: clock divider for i2si2_mck > > + - description: clock divider for i2so1_mck > > + - description: clock divider for i2so2_mck > > + - description: clock divider for dptx_mck > > + - description: a1sys hoping clock > > + - description: audio intbus clock > > + - description: audio hires clock > > + - description: audio local bus clock > > + - description: mux for dptx_mck > > + - description: mux for i2so1_mck > > + - description: mux for i2so2_mck > > + - description: mux for i2si1_mck > > + - description: mux for i2si2_mck > > + - description: audio 26m clock > > + > > + clock-names: > > + items: > > + - const: clk26m > > + - const: apll1_ck > > + - const: apll2_ck > > + - const: apll12_div0 > > + - const: apll12_div1 > > + - const: apll12_div2 > > + - const: apll12_div3 > > + - const: apll12_div9 > > + - const: a1sys_hp_sel > > + - const: aud_intbus_sel > > + - const: audio_h_sel > > + - const: audio_local_bus_sel > > + - const: dptx_m_sel > > + - const: i2so1_m_sel > > + - const: i2so2_m_sel > > + - const: i2si1_m_sel > > + - const: i2si2_m_sel > > + - const: adsp_audio_26m > > + > > +patternProperties: > > + "^mediatek,etdm-in[1-2]-chn-disabled$": > > + $ref: /schemas/types.yaml#/definitions/uint8-array > > + minItems: 1 > > + maxItems: 16 > > + description: > > + By default, all data received from ETDM pins will be > > outputed to > > + memory. etdm in supports disable_out in direct mode(w/o > > interconn). > > + User can specify the channel ID which they hope dropping and > > then > > + the specified channel won't be seen on memory. > > So we know what are the IDs but it's a mystery what are the values. > Especially with unique values - how any of these should case that > channel "won't be seen in memory"? > For example, FE can support 14ch, but BE(etdm) can't support 14ch(it can support 16ch). In the case, we can configure 16ch to etdm and make use of the property to disable the last two channels. mediatek,etdm-in1-chn-disabled = /bits/ 8 <0xE 0xF>; > > + uniqueItems: true > > + items: > > + minimum: 0 > > + maximum: 15 > > + > > + "^mediatek,etdm-in[1-2]-mclk-always-on-rate-hz$": > > + description: Specify etdm in mclk output rate for always on > > case. > > How is it different than assigned-clock-rates? > This includes clock enabling at init stage. > > + > > + "^mediatek,etdm-out[1-3]-mclk-always-on-rate-hz$": > > + description: Specify etdm out mclk output rate for always on > > case. > > + > > + "^mediatek,etdm-in[1-2]-multi-pin-mode$": > > + type: boolean > > + description: if present, the etdm data mode is I2S. > > + > > + "^mediatek,etdm-out[1-3]-multi-pin-mode$": > > + type: boolean > > + description: if present, the etdm data mode is I2S. > > + > > + "^mediatek,etdm-in[1-2]-cowork-source$": > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: > > + etdm modules can share the same external clock pin. Specify > > + which etdm clock source is required by this etdm in moudule. > > typo: module > Thanks, I will correct it in v4. > > + enum: > > + - 0 # etdm1_in > > + - 1 # etdm2_in > > + - 2 # etdm1_out > > I don't get. This suggests that etdm1_out can be clock source of > etdm-in1. Or etdm1_in can be clock source of etdm-in1... It does not > make sense... > You are correct. etdm1_in should only choose other etdm as its clock source when user needs this property, so etdm1_in should not be included in the enum items for etdm-in1-cowork-source. I will separate these properties in v4. Thanks, Trevor > > + - 3 # etdm2_out > > + > > + "^mediatek,etdm-out[1-2]-cowork-source$": > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + description: | > > + etdm modules can share the same external clock pin. Specify > > + which etdm clock source is required by this etdm out > > moudule. > > + enum: > > + - 0 # etdm1_in > > + - 1 # etdm2_in > > + - 2 # etdm1_out > > + - 3 # etdm2_out > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - resets > > + - reset-names > > + - mediatek,topckgen > > + - power-domains > > + - clocks > > + - clock-names > > + > > Best regards, > Krzysztof >
On 09/12/2022 11:56, Trevor Wu (吳文良) wrote: >>> + >>> +patternProperties: >>> + "^mediatek,etdm-in[1-2]-chn-disabled$": >>> + $ref: /schemas/types.yaml#/definitions/uint8-array >>> + minItems: 1 >>> + maxItems: 16 >>> + description: >>> + By default, all data received from ETDM pins will be >>> outputed to >>> + memory. etdm in supports disable_out in direct mode(w/o >>> interconn). >>> + User can specify the channel ID which they hope dropping and >>> then >>> + the specified channel won't be seen on memory. >> >> So we know what are the IDs but it's a mystery what are the values. >> Especially with unique values - how any of these should case that >> channel "won't be seen in memory"? >> > For example, > FE can support 14ch, but BE(etdm) can't support 14ch(it can support > 16ch). > In the case, we can configure 16ch to etdm and make use of the property > to disable the last two channels. > > mediatek,etdm-in1-chn-disabled = /bits/ 8 <0xE 0xF>; Your description should explain that this is a list of channel IDs which should be disabled. > > >>> + uniqueItems: true >>> + items: >>> + minimum: 0 >>> + maximum: 15 >>> + >>> + "^mediatek,etdm-in[1-2]-mclk-always-on-rate-hz$": >>> + description: Specify etdm in mclk output rate for always on >>> case. >> >> How is it different than assigned-clock-rates? >> > This includes clock enabling at init stage. assigned-clock-rates are also at init stage. I asked what is different. > >>> + >>> + "^mediatek,etdm-out[1-3]-mclk-always-on-rate-hz$": >>> + description: Specify etdm out mclk output rate for always on >>> case. >>> + >>> + "^mediatek,etdm-in[1-2]-multi-pin-mode$": >>> + type: boolean >>> + description: if present, the etdm data mode is I2S. >>> + >>> + "^mediatek,etdm-out[1-3]-multi-pin-mode$": >>> + type: boolean >>> + description: if present, the etdm data mode is I2S. >>> + >>> + "^mediatek,etdm-in[1-2]-cowork-source$": >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + description: >>> + etdm modules can share the same external clock pin. Specify >>> + which etdm clock source is required by this etdm in moudule. >> >> typo: module >> Best regards, Krzysztof
On Fri, 2022-12-09 at 15:56 +0100, Krzysztof Kozlowski wrote: > On 09/12/2022 11:56, Trevor Wu (吳文良) wrote: > > > > + > > > > +patternProperties: > > > > + "^mediatek,etdm-in[1-2]-chn-disabled$": > > > > + $ref: /schemas/types.yaml#/definitions/uint8-array > > > > + minItems: 1 > > > > + maxItems: 16 > > > > + description: > > > > + By default, all data received from ETDM pins will be > > > > outputed to > > > > + memory. etdm in supports disable_out in direct mode(w/o > > > > interconn). > > > > + User can specify the channel ID which they hope dropping > > > > and > > > > then > > > > + the specified channel won't be seen on memory. > > > > > > So we know what are the IDs but it's a mystery what are the > > > values. > > > Especially with unique values - how any of these should case that > > > channel "won't be seen in memory"? > > > > > > > For example, > > FE can support 14ch, but BE(etdm) can't support 14ch(it can support > > 16ch). > > In the case, we can configure 16ch to etdm and make use of the > > property > > to disable the last two channels. > > > > mediatek,etdm-in1-chn-disabled = /bits/ 8 <0xE 0xF>; > > Your description should explain that this is a list of channel IDs > which > should be disabled. > Hi Krzysztof, Thanks for your suggestion. I'll put it into the description. > > > > > > > > + uniqueItems: true > > > > + items: > > > > + minimum: 0 > > > > + maximum: 15 > > > > + > > > > + "^mediatek,etdm-in[1-2]-mclk-always-on-rate-hz$": > > > > + description: Specify etdm in mclk output rate for always > > > > on > > > > case. > > > > > > How is it different than assigned-clock-rates? > > > > > > > This includes clock enabling at init stage. > > assigned-clock-rates are also at init stage. I asked what is > different. > If the property is used, there are three parts included in dai driver probe function. 1. set clock parent (which APLL) 2. set clock rate (MCLK rate) 3. enable clock (MCLK On) The first two parts can be done by existing dts clock properties, but the last one can't. When MCLK should be enabled at boot time and kept on, this property is used. That's why I say the property is designed for always-on case. Thanks, Trevor. > > > > > > + > > > > + "^mediatek,etdm-out[1-3]-mclk-always-on-rate-hz$": > > > > + description: Specify etdm out mclk output rate for always > > > > on > > > > case. > > > > + > > > > + "^mediatek,etdm-in[1-2]-multi-pin-mode$": > > > > + type: boolean > > > > + description: if present, the etdm data mode is I2S. > > > > + > > > > + "^mediatek,etdm-out[1-3]-multi-pin-mode$": > > > > + type: boolean > > > > + description: if present, the etdm data mode is I2S. > > > > + > > > > + "^mediatek,etdm-in[1-2]-cowork-source$": > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > > + description: > > > > + etdm modules can share the same external clock pin. > > > > Specify > > > > + which etdm clock source is required by this etdm in > > > > moudule. > > > > > > typo: module > > > > > Best regards, > Krzysztof >
On 12/12/2022 03:43, Trevor Wu (吳文良) wrote: >>> >>>>> + uniqueItems: true >>>>> + items: >>>>> + minimum: 0 >>>>> + maximum: 15 >>>>> + >>>>> + "^mediatek,etdm-in[1-2]-mclk-always-on-rate-hz$": >>>>> + description: Specify etdm in mclk output rate for always >>>>> on >>>>> case. >>>> >>>> How is it different than assigned-clock-rates? >>>> >>> >>> This includes clock enabling at init stage. >> >> assigned-clock-rates are also at init stage. I asked what is >> different. >> > > If the property is used, there are three parts included in dai driver > probe function. > > 1. set clock parent (which APLL) > 2. set clock rate (MCLK rate) > 3. enable clock (MCLK On) > > The first two parts can be done by existing dts clock properties, but > the last one can't. > When MCLK should be enabled at boot time and kept on, this property is used. That's why I say the property is designed for always-on case. Heh, so the "always on case" means this property enables clock? How is this even DT property? That's not how clocks should be kept enabled. You need proper clock provider and consumer. Best regards, Krzysztof
On Fri, 2022-12-09 at 11:18 +0100, Krzysztof Kozlowski wrote: > On 08/12/2022 04:31, Trevor Wu wrote: > > Add document for mt8188 board with mt6359. > > > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> > > --- > > .../bindings/sound/mt8188-mt6359.yaml | 60 > > +++++++++++++++++++ > > 1 file changed, 60 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/sound/mt8188- > > mt6359.yaml > > > > diff --git a/Documentation/devicetree/bindings/sound/mt8188- > > mt6359.yaml b/Documentation/devicetree/bindings/sound/mt8188- > > mt6359.yaml > > new file mode 100644 > > index 000000000000..eac1c87b693a > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/sound/mt8188-mt6359.yaml > > Missing vendor prefix. You got comments for it already. Hi Krzysztof, I will correct it in V4. > > > @@ -0,0 +1,60 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: > > https://urldefense.com/v3/__http://devicetree.org/schemas/sound/mt8188-mt6359.yaml*__;Iw!!CTRNKA9wMg0ARbw!nw_OzBIRlXrEP6K_EtxEWAIZzuvnD3Dm_KDuv8gpLRqlnnSfVLksNfKqMmeYiFlEVVy6F2BXwmXB8oQkDPeBuEXMtfLX2w$ > > > > +$schema: > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!nw_OzBIRlXrEP6K_EtxEWAIZzuvnD3Dm_KDuv8gpLRqlnnSfVLksNfKqMmeYiFlEVVy6F2BXwmXB8oQkDPeBuEXodFoCJA$ > > > > + > > +title: MediaTek MT8188 ASoC sound card > > + > > +maintainers: > > + - Trevor Wu <trevor.wu@mediatek.com> > > + > > +properties: > > + compatible: > > + const: mediatek,mt8188-mt6359-evb > > + > > + model: > > + $ref: /schemas/types.yaml#/definitions/string > > + description: User specified audio sound card name > > + > > + audio-routing: > > + $ref: /schemas/types.yaml#/definitions/non-unique-string-array > > + description: > > + A list of the connections between audio components. Each > > entry is a > > + sink/source pair of strings. Valid names could be the input > > or output > > + widgets of audio components, power supplies, MicBias of > > codec and the > > + software switch. > > + > > + mediatek,platform: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: The phandle of MT8188 ASoC platform. > > + > > + mediatek,dptx-codec: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: The phandle of MT8188 Display Port Tx codec node. > > + > > + mediatek,hdmi-codec: > > + $ref: /schemas/types.yaml#/definitions/phandle > > + description: The phandle of MT8188 HDMI codec node. > > Why you do not use DAI links for all these? Are the following examples "DAI links" you mean? hdmi-dai-link { link-name = "HDMI Playback"; cpu { sound-dai = <&q6afedai TERTIARY_MI2S_RX>; }; platform { sound-dai = <&q6routing>; }; codec { sound-dai = <<9611_codec 0>; }; }; or headset-codec { sound-dai = <&rt5682s>; }; As far as I know, only "mediatek,platform" was used at the beginning. Next, hdmi-codec was introduced, and it followed the same style to get phandle. Finally, it became three properties in mt8195, and the implementation of mt8188 was inherited from mt8195. I just glanced over the usage of dai links. As I see it, I will replace the codec parts with dai link structure like the example above first. It seems to be easier to extend more codecs with the same tyle. For platform part, I need more time to check if it is better to keep the original implementation, because we still have to take SOF related implementation into consideration. > > > + > > +additionalProperties: false > > + > > +required: > > + - compatible > > + - mediatek,platform > > + > > +examples: > > + - | > > + > > Drop blank line. > OK, I will fix it in V4. > > + sound: mt8188-sound { > > Drop label, not used. Use generic node names, so just "sound". > OK, I will fix it in V4. Thanks, Trevor > > + compatible = "mediatek,mt8188-mt6359-evb"; > > Best regards, > Krzysztof >
Il 08/12/22 04:31, Trevor Wu ha scritto: > Add header files for register definition and structure. > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 08/12/22 04:31, Trevor Wu ha scritto: > Add mt8188 etdm dai driver support. > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> > --- > I don't add Reviewed-by tag because one new header file is included > in the patch to resolve compiling issue found by kernel test robot. > Additionally, I re-layout the code for better understanding of the > follow-up patch. > --- > sound/soc/mediatek/mt8188/mt8188-dai-etdm.c | 2591 +++++++++++++++++++ > 1 file changed, 2591 insertions(+) > create mode 100644 sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > > diff --git a/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c b/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > new file mode 100644 > index 000000000000..c653fa5e3f85 > --- /dev/null > +++ b/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c ..snip.. > + > +static void mt8188_dai_etdm_parse_of(struct mtk_base_afe *afe) > +{ > + const struct device_node *of_node = afe->dev->of_node; > + struct mt8188_afe_private *afe_priv = afe->platform_priv; > + struct mtk_dai_etdm_priv *etdm_data; > + int i, j; > + char prop[48]; > + u8 disable_chn[MT8188_ETDM_MAX_CHANNELS]; > + int max_chn = MT8188_ETDM_MAX_CHANNELS; > + u32 sel; > + int ret; > + int dai_id; > + unsigned int sync_id; > + struct { > + const char *name; > + const unsigned int sync_id; > + } of_afe_etdms[MT8188_AFE_IO_ETDM_NUM] = { > + {"etdm-in1", ETDM_SYNC_FROM_IN1}, > + {"etdm-in2", ETDM_SYNC_FROM_IN2}, > + {"etdm-out1", ETDM_SYNC_FROM_OUT1}, > + {"etdm-out2", ETDM_SYNC_FROM_OUT2}, > + {"etdm-out3", ETDM_SYNC_FROM_OUT3}, > + }; > + > + for (i = 0; i < MT8188_AFE_IO_ETDM_NUM; i++) { > + dai_id = ETDM_TO_DAI_ID(i); > + etdm_data = afe_priv->dai_priv[dai_id]; > + > + ret = snprintf(prop, sizeof(prop), > + "mediatek,%s-mclk-always-on-rate", > + of_afe_etdms[i].name); > + if (ret < 0) { > + dev_info(afe->dev, "%s snprintf err=%d\n", Is this property optional? If yes, this dev_info() must be a dev_dbg(), otherwise, this must be a dev_err(). Please fix all prints to use the right message level. > + __func__, ret); > + return; Also, is it possible to specify this property only on selected eTDMs? As it is right now, if anyone wants to specify this only on, for example, etdm-out1 and out2, that won't work. In that case, you should replace that return with a `continue`. P.S.: I'm raising this question because you're not forcing "all or nothing" in commit [10/12] where you introduce the bindings for this driver, so I suppose that returning (hence stopping to parse) is a mistake. > + } > + ret = of_property_read_u32(of_node, prop, &sel); > + if (ret == 0) { > + etdm_data->mclk_dir = SND_SOC_CLOCK_OUT; > + if (mtk_dai_etdm_cal_mclk(afe, sel, dai_id)) > + dev_info(afe->dev, "%s unsupported mclk %uHz\n", > + __func__, sel); > + } > + Regards, Angelo
Il 08/12/22 04:31, Trevor Wu ha scritto: > Add mt8188 pcmif dai driver support > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 08/12/22 04:31, Trevor Wu ha scritto: > Add audio clock wrapper and audio tuner control. > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On 12/12/2022 16:34, Trevor Wu (吳文良) wrote: > On Fri, 2022-12-09 at 11:18 +0100, Krzysztof Kozlowski wrote: >> On 08/12/2022 04:31, Trevor Wu wrote: >>> Add document for mt8188 board with mt6359. >>> >>> Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> >>> --- >>> .../bindings/sound/mt8188-mt6359.yaml | 60 >>> +++++++++++++++++++ >>> 1 file changed, 60 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/sound/mt8188- >>> mt6359.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/sound/mt8188- >>> mt6359.yaml b/Documentation/devicetree/bindings/sound/mt8188- >>> mt6359.yaml >>> new file mode 100644 >>> index 000000000000..eac1c87b693a >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/sound/mt8188-mt6359.yaml >> >> Missing vendor prefix. You got comments for it already. > Hi Krzysztof, > > I will correct it in V4. > >> >>> @@ -0,0 +1,60 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: >>> https://urldefense.com/v3/__http://devicetree.org/schemas/sound/mt8188-mt6359.yaml*__;Iw!!CTRNKA9wMg0ARbw!nw_OzBIRlXrEP6K_EtxEWAIZzuvnD3Dm_KDuv8gpLRqlnnSfVLksNfKqMmeYiFlEVVy6F2BXwmXB8oQkDPeBuEXMtfLX2w$ >>> >>> +$schema: >>> https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!nw_OzBIRlXrEP6K_EtxEWAIZzuvnD3Dm_KDuv8gpLRqlnnSfVLksNfKqMmeYiFlEVVy6F2BXwmXB8oQkDPeBuEXodFoCJA$ >>> >>> + >>> +title: MediaTek MT8188 ASoC sound card >>> + >>> +maintainers: >>> + - Trevor Wu <trevor.wu@mediatek.com> >>> + >>> +properties: >>> + compatible: >>> + const: mediatek,mt8188-mt6359-evb >>> + >>> + model: >>> + $ref: /schemas/types.yaml#/definitions/string >>> + description: User specified audio sound card name >>> + >>> + audio-routing: >>> + $ref: /schemas/types.yaml#/definitions/non-unique-string-array >>> + description: >>> + A list of the connections between audio components. Each >>> entry is a >>> + sink/source pair of strings. Valid names could be the input >>> or output >>> + widgets of audio components, power supplies, MicBias of >>> codec and the >>> + software switch. >>> + >>> + mediatek,platform: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: The phandle of MT8188 ASoC platform. >>> + >>> + mediatek,dptx-codec: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: The phandle of MT8188 Display Port Tx codec node. >>> + >>> + mediatek,hdmi-codec: >>> + $ref: /schemas/types.yaml#/definitions/phandle >>> + description: The phandle of MT8188 HDMI codec node. >> >> Why you do not use DAI links for all these? > > Are the following examples "DAI links" you mean? > > hdmi-dai-link { > link-name = "HDMI Playback"; > cpu { > sound-dai = <&q6afedai TERTIARY_MI2S_RX>; > }; > > platform { > sound-dai = <&q6routing>; > }; > > codec { > sound-dai = <<9611_codec 0>; > }; > }; Yes, this one. I think this is preferred. But anyway the point is to use sound-dai-cells, not pure (non-DAI) phandles). > > or > > headset-codec { > sound-dai = <&rt5682s>; > }; > > > As far as I know, only "mediatek,platform" was used at the beginning. > Next, hdmi-codec was introduced, and it followed the same style to get > phandle. Finally, it became three properties in mt8195, and the > implementation of mt8188 was inherited from mt8195. OK, if you share common code it is fine. > > I just glanced over the usage of dai links. > As I see it, I will replace the codec parts with dai link structure > like the example above first. It seems to be easier to extend more > codecs with the same tyle. Would be good. > > For platform part, I need more time to check if it is better to keep > the original implementation, because we still have to take SOF related > implementation into consideration. OK Best regards, Krzysztof
On Tue, 2022-12-13 at 11:45 +0100, AngeloGioacchino Del Regno wrote: > Il 08/12/22 04:31, Trevor Wu ha scritto: > > Add mt8188 etdm dai driver support. > > > > Signed-off-by: Trevor Wu <trevor.wu@mediatek.com> > > --- > > I don't add Reviewed-by tag because one new header file is included > > in the patch to resolve compiling issue found by kernel test robot. > > Additionally, I re-layout the code for better understanding of the > > follow-up patch. > > --- > > sound/soc/mediatek/mt8188/mt8188-dai-etdm.c | 2591 > > +++++++++++++++++++ > > 1 file changed, 2591 insertions(+) > > create mode 100644 sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > > > > diff --git a/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > > b/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > > new file mode 100644 > > index 000000000000..c653fa5e3f85 > > --- /dev/null > > +++ b/sound/soc/mediatek/mt8188/mt8188-dai-etdm.c > > ..snip.. > > > + > > +static void mt8188_dai_etdm_parse_of(struct mtk_base_afe *afe) > > +{ > > + const struct device_node *of_node = afe->dev->of_node; > > + struct mt8188_afe_private *afe_priv = afe->platform_priv; > > + struct mtk_dai_etdm_priv *etdm_data; > > + int i, j; > > + char prop[48]; > > + u8 disable_chn[MT8188_ETDM_MAX_CHANNELS]; > > + int max_chn = MT8188_ETDM_MAX_CHANNELS; > > + u32 sel; > > + int ret; > > + int dai_id; > > + unsigned int sync_id; > > + struct { > > + const char *name; > > + const unsigned int sync_id; > > + } of_afe_etdms[MT8188_AFE_IO_ETDM_NUM] = { > > + {"etdm-in1", ETDM_SYNC_FROM_IN1}, > > + {"etdm-in2", ETDM_SYNC_FROM_IN2}, > > + {"etdm-out1", ETDM_SYNC_FROM_OUT1}, > > + {"etdm-out2", ETDM_SYNC_FROM_OUT2}, > > + {"etdm-out3", ETDM_SYNC_FROM_OUT3}, > > + }; > > + > > + for (i = 0; i < MT8188_AFE_IO_ETDM_NUM; i++) { > > + dai_id = ETDM_TO_DAI_ID(i); > > + etdm_data = afe_priv->dai_priv[dai_id]; > > + > > + ret = snprintf(prop, sizeof(prop), > > + "mediatek,%s-mclk-always-on-rate", > > + of_afe_etdms[i].name); > > + if (ret < 0) { > > + dev_info(afe->dev, "%s snprintf err=%d\n", > > Is this property optional? If yes, this dev_info() must be a > dev_dbg(), > otherwise, this must be a dev_err(). > > Please fix all prints to use the right message level. Hi Angelo, Yes, this property is optional, so I don't use dev_err here. dev_dbg is hard to find the problem. I will make use of dev_err instead of dev_info in V4. > > > + __func__, ret); > > + return; > > Also, is it possible to specify this property only on selected eTDMs? > > As it is right now, if anyone wants to specify this only on, for > example, > etdm-out1 and out2, that won't work. > In that case, you should replace that return with a `continue`. > > P.S.: I'm raising this question because you're not forcing "all or > nothing" > in commit [10/12] where you introduce the bindings for this > driver, > so I suppose that returning (hence stopping to parse) is a > mistake. > It is OK to specify only etdm-out1 and out2 on. This is just an simple error handling for snprintf to silence coverity. Because it shouldn't happen, I don't return an error value to make probe failed. Thanks, Trevor > > + } > > + ret = of_property_read_u32(of_node, prop, &sel); > > + if (ret == 0) { > > + etdm_data->mclk_dir = SND_SOC_CLOCK_OUT; > > + if (mtk_dai_etdm_cal_mclk(afe, sel, dai_id)) > > + dev_info(afe->dev, "%s unsupported mclk > > %uHz\n", > > + __func__, sel); > > + } > > + > > > Regards, > Angelo >
On Mon, 2022-12-12 at 09:40 +0100, Krzysztof Kozlowski wrote: > On 12/12/2022 03:43, Trevor Wu (吳文良) wrote: > > > > > > > > > > + uniqueItems: true > > > > > > + items: > > > > > > + minimum: 0 > > > > > > + maximum: 15 > > > > > > + > > > > > > + "^mediatek,etdm-in[1-2]-mclk-always-on-rate-hz$": > > > > > > + description: Specify etdm in mclk output rate for > > > > > > always > > > > > > on > > > > > > case. > > > > > > > > > > How is it different than assigned-clock-rates? > > > > > > > > > > > > > This includes clock enabling at init stage. > > > > > > assigned-clock-rates are also at init stage. I asked what is > > > different. > > > > > > > If the property is used, there are three parts included in dai > > driver > > probe function. > > > > 1. set clock parent (which APLL) > > 2. set clock rate (MCLK rate) > > 3. enable clock (MCLK On) > > > > The first two parts can be done by existing dts clock properties, > > but > > the last one can't. > > When MCLK should be enabled at boot time and kept on, this property > > is used. That's why I say the property is designed for always-on > > case. > > Heh, so the "always on case" means this property enables clock? How > is > this even DT property? That's not how clocks should be kept enabled. > You > need proper clock provider and consumer. > > Hi Krzysztof, Sorry, I don't know it is not appropriate to notify driver that the clock should be ketp enabled after boot. The original idea is that enabling this clock in the machine driver, but a property to inform machine driver is also required when the machine driver is shared by different codec combination. And it's easier to handle set_rate and set_parent in etdm dai driver, so I put the property here. Do you mean if the clock consumer(audio codec or external DSP) requries the clock, the consumer should enable the clock by itself? Thanks, Trevor
On 13/12/2022 16:06, Trevor Wu (吳文良) wrote: > On Mon, 2022-12-12 at 09:40 +0100, Krzysztof Kozlowski wrote: >> On 12/12/2022 03:43, Trevor Wu (吳文良) wrote: >>>>> >>>>>>> + uniqueItems: true >>>>>>> + items: >>>>>>> + minimum: 0 >>>>>>> + maximum: 15 >>>>>>> + >>>>>>> + "^mediatek,etdm-in[1-2]-mclk-always-on-rate-hz$": >>>>>>> + description: Specify etdm in mclk output rate for >>>>>>> always >>>>>>> on >>>>>>> case. >>>>>> >>>>>> How is it different than assigned-clock-rates? >>>>>> >>>>> >>>>> This includes clock enabling at init stage. >>>> >>>> assigned-clock-rates are also at init stage. I asked what is >>>> different. >>>> >>> >>> If the property is used, there are three parts included in dai >>> driver >>> probe function. >>> >>> 1. set clock parent (which APLL) >>> 2. set clock rate (MCLK rate) >>> 3. enable clock (MCLK On) >>> >>> The first two parts can be done by existing dts clock properties, >>> but >>> the last one can't. >>> When MCLK should be enabled at boot time and kept on, this property >>> is used. That's why I say the property is designed for always-on >>> case. >> >> Heh, so the "always on case" means this property enables clock? How >> is >> this even DT property? That's not how clocks should be kept enabled. >> You >> need proper clock provider and consumer. >> >> > > Hi Krzysztof, > > Sorry, I don't know it is not appropriate to notify driver that the > clock should be ketp enabled after boot. > > The original idea is that enabling this clock in the machine driver, > but a property to inform machine driver is also required when the > machine driver is shared by different codec combination. And it's > easier to handle set_rate and set_parent in etdm dai driver, so I put > the property here. > > Do you mean if the clock consumer(audio codec or external DSP) requries > the clock, the consumer should enable the clock by itself? Yes, your clocks should have consumers and they keep the clock enabled when needed. Certain clocks can be marked as IGNORE or CRITICAL to keep enabled without consumers (or even when consumers disable), but that's still not a DT property. Best regards, Krzysztof
On Wed, 2022-12-14 at 13:02 +0100, Krzysztof Kozlowski wrote: > On 13/12/2022 16:06, Trevor Wu (吳文良) wrote: > > On Mon, 2022-12-12 at 09:40 +0100, Krzysztof Kozlowski wrote: > > > On 12/12/2022 03:43, Trevor Wu (吳文良) wrote: > > > > > > > > > > > > > > + uniqueItems: true > > > > > > > > + items: > > > > > > > > + minimum: 0 > > > > > > > > + maximum: 15 > > > > > > > > + > > > > > > > > + "^mediatek,etdm-in[1-2]-mclk-always-on-rate-hz$": > > > > > > > > + description: Specify etdm in mclk output rate for > > > > > > > > always > > > > > > > > on > > > > > > > > case. > > > > > > > > > > > > > > How is it different than assigned-clock-rates? > > > > > > > > > > > > > > > > > > > This includes clock enabling at init stage. > > > > > > > > > > assigned-clock-rates are also at init stage. I asked what is > > > > > different. > > > > > > > > > > > > > If the property is used, there are three parts included in dai > > > > driver > > > > probe function. > > > > > > > > 1. set clock parent (which APLL) > > > > 2. set clock rate (MCLK rate) > > > > 3. enable clock (MCLK On) > > > > > > > > The first two parts can be done by existing dts clock > > > > properties, > > > > but > > > > the last one can't. > > > > When MCLK should be enabled at boot time and kept on, this > > > > property > > > > is used. That's why I say the property is designed for always- > > > > on > > > > case. > > > > > > Heh, so the "always on case" means this property enables clock? > > > How > > > is > > > this even DT property? That's not how clocks should be kept > > > enabled. > > > You > > > need proper clock provider and consumer. > > > > > > > > > > Hi Krzysztof, > > > > Sorry, I don't know it is not appropriate to notify driver that the > > clock should be ketp enabled after boot. > > > > The original idea is that enabling this clock in the machine > > driver, > > but a property to inform machine driver is also required when the > > machine driver is shared by different codec combination. And it's > > easier to handle set_rate and set_parent in etdm dai driver, so I > > put > > the property here. > > > > Do you mean if the clock consumer(audio codec or external DSP) > > requries > > the clock, the consumer should enable the clock by itself? > > Yes, your clocks should have consumers and they keep the clock > enabled > when needed. Certain clocks can be marked as IGNORE or CRITICAL to > keep > enabled without consumers (or even when consumers disable), but > that's > still not a DT property. > > Hi Krzysztof, Got it. If the implementation is not suggested, I will drop the property in V4 and ask consumer to use existing clock property with clock control API instead when we have such case. Thanks, Trevor >