Re: [PATCH v1 1/3] dt-bindings: ingenic,aic: Remove unnecessary clocks from schema

[Paul Cercueil <paul@crapouillou.net>]

Hi,

Le ven. 28 oct. 2022 à 11:34:16 +0100, Aidan MacDonald 
<aidanmacdonald.0x0@gmail.com> a écrit :
> The AIC needs only the first two clocks: "aic" is a gate that's used
> for gating the I2S controller when it's suspended, and "i2s" is the
> system clock, from which the bit and frame clocks are derived. Both
> clocks are therefore reasonably part of the AIC and should be passed
> to the OS.
> 
> But the "ext" and "pll half" clocks are a little more questionable.
> It appears these bindings were introduced when the schema was first
> converted to YAML, but weren't present in the original .txt binding.
> They are intended to be the possible parent clocks of "i2s".
> 
> The JZ4770 actually has three parents for its "i2s" clock, named
> "ext", "pll0", and "pll1" in the Linux driver. The JZ4780 has two
> parents but it doesn't have a "pll half" clock, instead it has an
> "i2s_pll" clock which behaves much differently to the actual
> "pll half" clock found on the JZ4740 & JZ4760. And there are other
> Ingenic SoCs that share the JZ4780's clock layout, eg, the X1000.
> 
> Therefore, the bindings aren't really adequate for the JZ4770 and
> a bit misleading for the JZ4780. Either we should fix the bindings,
> or remove them entirely.
> 
> This patch opts to remove the bindings. There is a good case to be
> made that "ext" and "pll half" don't belong here because they aren't
> directly used by the AIC. They are only used to set the parent of
> the "i2s" clock; they have no other effect on the AIC.
> 
> A good way to think of it is in terms of how the AIC constrains
> clocks. The AIC can only generate the bit & frame clocks from the
> system clock in certain ratios. Setting the sample rate effectively
> constrains the frame clock, which, because of the clock dividers
> controlled by the AIC, translates to constraints on the "i2s" clock.
> Nothing in the AIC imposes a direct constraint on the parents of
> the "i2s" clock, and the AIC does not need to enable or disable
> the parents directly, so in principle the AIC doesn't need to be
> aware of the parent clocks at all.
> 
> The choice of parent clock is still important, but the AIC doesn't
> have enough information to apply such constraints itself. The sound
> card does have that information because it knows how the AIC is
> connected to other components. We need to use other DT mechanisms
> to communicate those constraints at the sound card level, instead
> of passing the clocks through to the AIC, and inventing ad-hoc ways
> to plumb the constraints around behind the scenes.
> 
> Signed-off-by: Aidan MacDonald <aidanmacdonald.0x0@gmail.com>

Yes, it makes sense also because from a DT point of view, these clocks 
were redundant information. It's enough to know the i2s clock to also 
know its parents.

Acked-by: Paul Cercueil <paul@crapouillou.net>

Cheers,
-Paul

> ---
>  .../devicetree/bindings/sound/ingenic,aic.yaml         | 10 
> ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/ingenic,aic.yaml 
> b/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
> index d607325f2f15..c4f9b3c2bde5 100644
> --- a/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
> +++ b/Documentation/devicetree/bindings/sound/ingenic,aic.yaml
> @@ -37,15 +37,11 @@ properties:
>      items:
>        - description: AIC clock
>        - description: I2S clock
> -      - description: EXT clock
> -      - description: PLL/2 clock
> 
>    clock-names:
>      items:
>        - const: aic
>        - const: i2s
> -      - const: ext
> -      - const: pll half
> 
>    dmas:
>      items:
> @@ -82,10 +78,8 @@ examples:
>        interrupts = <18>;
> 
>        clocks = <&cgu JZ4740_CLK_AIC>,
> -               <&cgu JZ4740_CLK_I2S>,
> -               <&cgu JZ4740_CLK_EXT>,
> -               <&cgu JZ4740_CLK_PLL_HALF>;
> -      clock-names = "aic", "i2s", "ext", "pll half";
> +               <&cgu JZ4740_CLK_I2S>;
> +      clock-names = "aic", "i2s";
> 
>        dmas = <&dmac 25 0xffffffff>, <&dmac 24 0xffffffff>;
>        dma-names = "rx", "tx";
> --
> 2.38.1
>