[v3,6/6] ASoC: dt-bindings: mediatek,mt7986-afe: add audio afe document

  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
  

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