Re: [PATCH v8 1/5] dt-bindings: clock: meson: Add audio power domain for S4 soc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Low] dt-bindings: clock: meson: commit message omits s4-audio-clkc
  compatible
- [Low] dt-bindings: clock: meson: power-domains forbidden on AXG and
  SM1 variants
- [Low] dt-bindings: clock: meson: unwieldy per-compatible conditionals
  in allOf
--

commit 3f9534e5b3c2551a42b1642e1cc62923e9975fcc
Author: Jiebing Chen <jiebing.chen@amlogic.com>

> dt-bindings: clock: meson: Add audio power domain for S4 soc
>
> The audio power domain has been found on S4 device.
> It must be enabled prior to audio operations.
>
> Signed-off-by: Jiebing Chen <jiebing.chen@amlogic.com>

[Severity: Low]
Does the commit message omit that this patch introduces the
amlogic,s4-audio-clkc compatible string and its #reset-cells requirement?
The title and changelog only mention adding the audio power domain for S4,
but amlogic,s4-audio-clkc was not present in this binding before.

> diff --git a/Documentation/devicetree/bindings/clock/amlogic,axg-audio-clkc.yaml b/Documentation/devicetree/bindings/clock/amlogic,axg-audio-clkc.yaml
> index fd7982dd4ceab..75ddaf8d80917 100644
> --- a/Documentation/devicetree/bindings/clock/amlogic,axg-audio-clkc.yaml
> +++ b/Documentation/devicetree/bindings/clock/amlogic,axg-audio-clkc.yaml
> @@ -20,6 +20,7 @@ properties:
>      enum:
>        - amlogic,axg-audio-clkc
>        - amlogic,g12a-audio-clkc
> +      - amlogic,s4-audio-clkc
>        - amlogic,sm1-audio-clkc
>  
>    '#clock-cells':
> @@ -100,6 +101,9 @@ properties:
>    resets:
>      description: internal reset line
>  
> +  power-domains:
> +    maxItems: 1
> +
>  required:
>    - compatible
>    - '#clock-cells'
> @@ -115,6 +119,7 @@ allOf:
>            contains:
>              enum:
>                - amlogic,g12a-audio-clkc
> +              - amlogic,s4-audio-clkc
>                - amlogic,sm1-audio-clkc
>      then:
>        required:
> @@ -122,6 +127,18 @@ allOf:
>      else:
>        properties:
>          '#reset-cells': false
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - amlogic,s4-audio-clkc
> +    then:
> +      required:
> +        - power-domains
> +    else:
> +      properties:
> +        power-domains: false

[Severity: Low]
Should power-domains be set to false for the other compatibles here?  Both
AXG and SM1 also have dedicated audio power domains in hardware, so
disallowing power-domains in the else branch causes dtbs_check failures if
amlogic,axg-audio-clkc or amlogic,sm1-audio-clkc nodes reference them:

drivers/pmdomain/amlogic/meson-ee-pwrc.c:axg_pwrc_domains[] {
    ...
	[PWRC_AXG_AUDIO_ID] = MEM_PD("AUDIO", axg_pwrc_mem_audio),
};

drivers/pmdomain/amlogic/meson-ee-pwrc.c:sm1_pwrc_domains[] {
    ...
	[PWRC_SM1_AUDIO_ID] = MEM_PD("AUDIO", sm1_pwrc_mem_audio),
    ...
};

[Severity: Low]
This isn't a bug, but adding another per-compatible if-then-else block to
amlogic,axg-audio-clkc.yaml makes the shared schema harder to maintain,
especially since S4 also diverges from AXG, G12A, and SM1 by not using the
slv_sclk0..9 and slv_lrclk0..9 clock inputs.  Would it be cleaner to split
amlogic,s4-audio-clkc into its own binding schema, similar to
amlogic,s4-peripherals-clkc.yaml and amlogic,s4-pll-clkc.yaml?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-audio_drvier-v8-0-1489241e4c83@amlogic.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic