| Message ID | 20230728090819.18038-7-maso.huang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: mediatek: Add support for MT7986 SoC | expand |
Il 28/07/23 11:08, Maso Huang ha scritto: > Add mt7986 audio afe document. > > Signed-off-by: Maso Huang <maso.huang@mediatek.com> > --- > .../bindings/sound/mediatek,mt7986-afe.yaml | 89 +++++++++++++++++++ > 1 file changed, 89 insertions(+) > create mode 100644 Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml > > diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml > new file mode 100644 > index 000000000000..ebb151c6400f > --- /dev/null > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml > @@ -0,0 +1,89 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/sound/mediatek,mt7986-afe.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: MediaTek AFE PCM controller for MT7986 > + > +maintainers: > + - Maso Huang <maso.huang@mediatek.com> > + > +properties: > + compatible: > + oneOf: > + - const: mediatek,mt7986-afe > + - items: > + - enum: > + - mediatek,mt7981-afe > + - mediatek,mt7988-afe > + - const: mediatek,mt7986-afe > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > + clocks: > + minItems: 5 > + items: > + - description: audio bus clock > + - description: audio 26M clock > + - description: audio intbus clock > + - description: audio hopping clock > + - description: audio pll clock > + - description: mux for pcm_mck > + - description: audio i2s/pcm mck > + > + clock-names: > + minItems: 5 > + items: > + - const: bus_ck > + - const: 26m_ck > + - const: l_ck > + - const: aud_ck > + - const: eg2_ck > + - const: sel > + - const: i2s_m > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - assigned-clocks > + - assigned-clock-parents assigned-clocks and assigned-clock-parents are not a *required* property, as that depends on a number of things and someone *may* want to omit it. Besides, that's related to how the drivers interact with / setup the hardware and not with the hardware itself... you can keep the assigned-clocks and assigned-clock-parents in your examples, but again they're definitely not required properties. After fixing, Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Regards, Angelo > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/clock/mt7986-clk.h> > + > + afe@11210000 { > + compatible = "mediatek,mt7986-afe"; > + reg = <0x11210000 0x9000>; > + interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&infracfg_ao CLK_INFRA_AUD_BUS_CK>, > + <&infracfg_ao CLK_INFRA_AUD_26M_CK>, > + <&infracfg_ao CLK_INFRA_AUD_L_CK>, > + <&infracfg_ao CLK_INFRA_AUD_AUD_CK>, > + <&infracfg_ao CLK_INFRA_AUD_EG2_CK>; > + clock-names = "bus_ck", > + "26m_ck", > + "l_ck", > + "aud_ck", > + "eg2_ck"; > + assigned-clocks = <&topckgen CLK_TOP_A1SYS_SEL>, > + <&topckgen CLK_TOP_AUD_L_SEL>, > + <&topckgen CLK_TOP_A_TUNER_SEL>; > + assigned-clock-parents = <&topckgen CLK_TOP_APLL2_D4>, > + <&apmixedsys CLK_APMIXED_APLL2>, > + <&topckgen CLK_TOP_APLL2_D4>; > + }; > + > +...
On Fri, 2023-07-28 at 12:00 +0200, AngeloGioacchino Del Regno wrote: > Il 28/07/23 11:08, Maso Huang ha scritto: > > Add mt7986 audio afe document. > > > > Signed-off-by: Maso Huang <maso.huang@mediatek.com> > > --- > > .../bindings/sound/mediatek,mt7986-afe.yaml | 89 > > +++++++++++++++++++ > > 1 file changed, 89 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml > > b/Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml > > new file mode 100644 > > index 000000000000..ebb151c6400f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/sound/mediatek,mt7986- > > afe.yaml > > @@ -0,0 +1,89 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: > > https://urldefense.com/v3/__http://devicetree.org/schemas/sound/mediatek,mt7986-afe.yaml*__;Iw!!CTRNKA9wMg0ARbw!nwtJIMuoaT5RiUlFU-0ExLhvWHgz9nyXm0Jxk9RfoaKqIKzS8_JTvdIW9AhOkTIpVjN9Jv3L7Aj4nXzTEeuHK2-Wdq69mo0$ > > > > +$schema: > > https://urldefense.com/v3/__http://devicetree.org/meta-schemas/core.yaml*__;Iw!!CTRNKA9wMg0ARbw!nwtJIMuoaT5RiUlFU-0ExLhvWHgz9nyXm0Jxk9RfoaKqIKzS8_JTvdIW9AhOkTIpVjN9Jv3L7Aj4nXzTEeuHK2-Wo1ebvx8$ > > > > + > > +title: MediaTek AFE PCM controller for MT7986 > > + > > +maintainers: > > + - Maso Huang <maso.huang@mediatek.com> > > + > > +properties: > > + compatible: > > + oneOf: > > + - const: mediatek,mt7986-afe > > + - items: > > + - enum: > > + - mediatek,mt7981-afe > > + - mediatek,mt7988-afe > > + - const: mediatek,mt7986-afe > > + > > + reg: > > + maxItems: 1 > > + > > + interrupts: > > + maxItems: 1 > > + > > + clocks: > > + minItems: 5 > > + items: > > + - description: audio bus clock > > + - description: audio 26M clock > > + - description: audio intbus clock > > + - description: audio hopping clock > > + - description: audio pll clock > > + - description: mux for pcm_mck > > + - description: audio i2s/pcm mck > > + > > + clock-names: > > + minItems: 5 > > + items: > > + - const: bus_ck > > + - const: 26m_ck > > + - const: l_ck > > + - const: aud_ck > > + - const: eg2_ck > > + - const: sel > > + - const: i2s_m > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - clock-names > > + - assigned-clocks > > + - assigned-clock-parents > > assigned-clocks and assigned-clock-parents are not a *required* > property, > as that depends on a number of things and someone *may* want to omit > it. > > Besides, that's related to how the drivers interact with / setup the > hardware > and not with the hardware itself... you can keep the assigned-clocks > and > assigned-clock-parents in your examples, but again they're definitely > not > required properties. > > After fixing, > > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > > Regards, > Angelo > Hi Angelo, Thanks for your review. I'll remove these in v4 patch! Best regards, Maso > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/clock/mt7986-clk.h> > > + > > + afe@11210000 { > > + compatible = "mediatek,mt7986-afe"; > > + reg = <0x11210000 0x9000>; > > + interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&infracfg_ao CLK_INFRA_AUD_BUS_CK>, > > + <&infracfg_ao CLK_INFRA_AUD_26M_CK>, > > + <&infracfg_ao CLK_INFRA_AUD_L_CK>, > > + <&infracfg_ao CLK_INFRA_AUD_AUD_CK>, > > + <&infracfg_ao CLK_INFRA_AUD_EG2_CK>; > > + clock-names = "bus_ck", > > + "26m_ck", > > + "l_ck", > > + "aud_ck", > > + "eg2_ck"; > > + assigned-clocks = <&topckgen CLK_TOP_A1SYS_SEL>, > > + <&topckgen CLK_TOP_AUD_L_SEL>, > > + <&topckgen CLK_TOP_A_TUNER_SEL>; > > + assigned-clock-parents = <&topckgen CLK_TOP_APLL2_D4>, > > + <&apmixedsys CLK_APMIXED_APLL2>, > > + <&topckgen CLK_TOP_APLL2_D4>; > > + }; > > + > > +... > >
On 28/07/2023 11:08, Maso Huang wrote: > Add mt7986 audio afe document. > > Signed-off-by: Maso Huang <maso.huang@mediatek.com> Thank you for your patch. There is something to discuss/improve. > + > +required: > + - compatible > + - reg > + - interrupts > + - clocks > + - clock-names > + - assigned-clocks > + - assigned-clock-parents You should constrain your clocks per variants. I doubt that they are really so flexible/optional on each SoC... or maybe missing clocks are result of unimplemented parts in the driver? But then this should not really affect bindings. Bindings still should require such clocks. Your DTS can always provide a <0>, if needed. > + > +additionalProperties: false Best regards, Krzysztof
On Fri, Jul 28, 2023 at 02:51:26PM +0200, Krzysztof Kozlowski wrote: > On 28/07/2023 11:08, Maso Huang wrote: > > + - assigned-clocks > > + - assigned-clock-parents > You should constrain your clocks per variants. I doubt that they are > really so flexible/optional on each SoC... or maybe missing clocks are > result of unimplemented parts in the driver? But then this should not > really affect bindings. Bindings still should require such clocks. Your > DTS can always provide a <0>, if needed. Depending on what the clocks are some of them might genuinely be optional, it's fairly common for audio devices to have multiple clock inputs and be able to select between them depending on system requirements or to have bidirectional clock pins which may be either a provider or consumer depending on system configuration. No idea how that applies with this specific device.
On Fri, 2023-07-28 at 14:51 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 28/07/2023 11:08, Maso Huang wrote: > > Add mt7986 audio afe document. > > > > Signed-off-by: Maso Huang <maso.huang@mediatek.com> > > Thank you for your patch. There is something to discuss/improve. > > > + > > +required: > > + - compatible > > + - reg > > + - interrupts > > + - clocks > > + - clock-names > > + - assigned-clocks > > + - assigned-clock-parents > > You should constrain your clocks per variants. I doubt that they are > really so flexible/optional on each SoC... or maybe missing clocks > are > result of unimplemented parts in the driver? But then this should not > really affect bindings. Bindings still should require such clocks. > Your > DTS can always provide a <0>, if needed. > > Hi Krzysztof, After internal check, assigned-clocks and assigned-clock-parents are not required on this SoC. Maybe we can just drop these two options? Best regards, Maso > > + > > +additionalProperties: false > > > Best regards, > Krzysztof >
On 01/08/2023 10:25, Maso Huang (黃加竹) wrote: > On Fri, 2023-07-28 at 14:51 +0200, Krzysztof Kozlowski wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> On 28/07/2023 11:08, Maso Huang wrote: >>> Add mt7986 audio afe document. >>> >>> Signed-off-by: Maso Huang <maso.huang@mediatek.com> >> >> Thank you for your patch. There is something to discuss/improve. >> >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - interrupts >>> + - clocks >>> + - clock-names >>> + - assigned-clocks >>> + - assigned-clock-parents >> >> You should constrain your clocks per variants. I doubt that they are >> really so flexible/optional on each SoC... or maybe missing clocks >> are >> result of unimplemented parts in the driver? But then this should not >> really affect bindings. Bindings still should require such clocks. >> Your >> DTS can always provide a <0>, if needed. >> >> > > Hi Krzysztof, > > After internal check, assigned-clocks and assigned-clock-parents are > not required on this SoC. > Maybe we can just drop these two options? It's separate issue, but yes - why requiring them? I wrote about missing constraints for your clocks in the bindings. Best regards, Krzysztof
On Fri, 2023-08-04 at 11:45 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 01/08/2023 10:25, Maso Huang (黃加竹) wrote: > > On Fri, 2023-07-28 at 14:51 +0200, Krzysztof Kozlowski wrote: > >> > >> External email : Please do not click links or open attachments > until > >> you have verified the sender or the content. > >> On 28/07/2023 11:08, Maso Huang wrote: > >>> Add mt7986 audio afe document. > >>> > >>> Signed-off-by: Maso Huang <maso.huang@mediatek.com> > >> > >> Thank you for your patch. There is something to discuss/improve. > >> > >>> + > >>> +required: > >>> + - compatible > >>> + - reg > >>> + - interrupts > >>> + - clocks > >>> + - clock-names > >>> + - assigned-clocks > >>> + - assigned-clock-parents > >> > >> You should constrain your clocks per variants. I doubt that they > are > >> really so flexible/optional on each SoC... or maybe missing clocks > >> are > >> result of unimplemented parts in the driver? But then this should > not > >> really affect bindings. Bindings still should require such clocks. > >> Your > >> DTS can always provide a <0>, if needed. > >> > >> > > > > Hi Krzysztof, > > > > After internal check, assigned-clocks and assigned-clock-parents > are > > not required on this SoC. > > Maybe we can just drop these two options? > > It's separate issue, but yes - why requiring them? > > I wrote about missing constraints for your clocks in the bindings. > > Best regards, > Krzysztof > Hi Krzysztof, OK, I'll remove assigned-clock and assigned-clock-parents. And constrain the clocks for different SoC in the binding in v4 patch. Best regards, Maso
diff --git a/Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml b/Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml new file mode 100644 index 000000000000..ebb151c6400f --- /dev/null +++ b/Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml @@ -0,0 +1,89 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/sound/mediatek,mt7986-afe.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: MediaTek AFE PCM controller for MT7986 + +maintainers: + - Maso Huang <maso.huang@mediatek.com> + +properties: + compatible: + oneOf: + - const: mediatek,mt7986-afe + - items: + - enum: + - mediatek,mt7981-afe + - mediatek,mt7988-afe + - const: mediatek,mt7986-afe + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + + clocks: + minItems: 5 + items: + - description: audio bus clock + - description: audio 26M clock + - description: audio intbus clock + - description: audio hopping clock + - description: audio pll clock + - description: mux for pcm_mck + - description: audio i2s/pcm mck + + clock-names: + minItems: 5 + items: + - const: bus_ck + - const: 26m_ck + - const: l_ck + - const: aud_ck + - const: eg2_ck + - const: sel + - const: i2s_m + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + - assigned-clocks + - assigned-clock-parents + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/clock/mt7986-clk.h> + + afe@11210000 { + compatible = "mediatek,mt7986-afe"; + reg = <0x11210000 0x9000>; + interrupts = <GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&infracfg_ao CLK_INFRA_AUD_BUS_CK>, + <&infracfg_ao CLK_INFRA_AUD_26M_CK>, + <&infracfg_ao CLK_INFRA_AUD_L_CK>, + <&infracfg_ao CLK_INFRA_AUD_AUD_CK>, + <&infracfg_ao CLK_INFRA_AUD_EG2_CK>; + clock-names = "bus_ck", + "26m_ck", + "l_ck", + "aud_ck", + "eg2_ck"; + assigned-clocks = <&topckgen CLK_TOP_A1SYS_SEL>, + <&topckgen CLK_TOP_AUD_L_SEL>, + <&topckgen CLK_TOP_A_TUNER_SEL>; + assigned-clock-parents = <&topckgen CLK_TOP_APLL2_D4>, + <&apmixedsys CLK_APMIXED_APLL2>, + <&topckgen CLK_TOP_APLL2_D4>; + }; + +...
Add mt7986 audio afe document. Signed-off-by: Maso Huang <maso.huang@mediatek.com> --- .../bindings/sound/mediatek,mt7986-afe.yaml | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/mediatek,mt7986-afe.yaml