Re: [PATCH 1/2] dt-bindings: sound: Add Marvell MMP Audio Clock Controller binding

[Stephen Boyd]

Quoting Lubomir Rintel (2020-05-11 12:55:33)
> diff --git a/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml b/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml
> new file mode 100644
> index 000000000000..b86e9fbfa56d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/marvell,mmp2-audio-clock.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/marvell,mmp2-audio-clock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell MMP2 Audio Clock Controller
> +
> +maintainers:
> +  - Lubomir Rintel <lkundrak@xxxxx>
> +
> +description: |
> +  The audio clock controller generates and supplies the clocks to the audio
> +  codec.
> +
> +  Each clock is assigned an identifier and client nodes use this identifier
> +  to specify the clock which they consume.
> +
> +  All these identifiers could be found in <dt-bindings/clock/marvell,mmp2.h>.

Is this right? The patch puts them in mmp2-audio.h

> +
> +properties:
> +  compatible:
> +    enum:
> +      - marvell,mmp2-audio-clock
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Audio subsystem clock
> +      - description: The crystal oscillator clock
> +      - description: First I2S clock
> +      - description: Second I2S clock
> +
> +  clock-names:
> +    items:
> +      - const: audio
> +      - const: vctcxo
> +      - const: i2s0
> +      - const: i2s1
> +
> +  '#clock-cells':
> +    const: 1
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/marvell,mmp2.h>
> +    #include <dt-bindings/power/marvell,mmp2.h>
> +
> +    clocks@d42a0c30 {

clock-controller@d42a0c30

> +      compatible = "marvell,mmp2-audio-clock";
> +      reg = <0xd42a0c30 0x10>;

That is a very specific and tiny region. Presumably this is part of a
larger hardware block and thus shouldn't be described in DT this way.
Instead there should be one clock-controller node and a driver that
controls all the clks that it wants to inside that hardware block.

> +      clock-names = "audio", "vctcxo", "i2s0", "i2s1";
> +      clocks = <&soc_clocks MMP2_CLK_AUDIO>,
> +               <&soc_clocks MMP2_CLK_VCTCXO>,
> +               <&soc_clocks MMP2_CLK_I2S0>,
> +               <&soc_clocks MMP2_CLK_I2S1>;
> +      power-domains = <&soc_clocks MMP2_POWER_DOMAIN_AUDIO>;
> +      #clock-cells = <1>;
> +    };
> diff --git a/include/dt-bindings/clock/marvell,mmp2-audio.h b/include/dt-bindings/clock/marvell,mmp2-audio.h
> new file mode 100644
> index 000000000000..127b48ec0f0a
> --- /dev/null
> +++ b/include/dt-bindings/clock/marvell,mmp2-audio.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-2-Clause) */
> +#ifndef __DT_BINDINGS_CLOCK_MARVELL_MMP2_AUDIO_H
> +#define __DT_BINDINGS_CLOCK_MARVELL_MMP2_AUDIO_H
> +
> +#define MMP2_CLK_AUDIO_SYSCLK          1

Any reason to start at 1 vs. 0?

> +#define MMP2_CLK_AUDIO_SSPA0           2
> +#define MMP2_CLK_AUDIO_SSPA1           3
> +#endif
> -- 
> 2.26.2
>