Re: [PATCH v2 1/5] dt-bindings: clock: samsung: Add Exynos9610 CMU bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Clock IDs for CMU_CMGP muxes and dividers overlap, reusing IDs 0 through 6, causing ID collisions.
- [Low] Incorrect header file referenced in the YAML binding description.
--

--- Patch [1]: [PATCH v2 1/5] dt-bindings: clock: samsung: Add Exynos9610 CMU bindings ---
commit 23fcebc1c69a1cfdcd9b3f745d13b2ff694b6b10
Author: Alexandru Chimac <alex@chimac.ro>

dt-bindings: clock: samsung: Add Exynos9610 CMU bindings

This commit adds the device tree binding documentation for the Exynos9610 clock
controller. It defines the compatible strings and properties for various clock
management units, and introduces the corresponding header file with clock IDs.

> diff --git a/Documentation/devicetree/bindings/clock/samsung,exynos9610-cmu.yaml b/Documentation/devicetree/bindings/clock/samsung,exynos9610-cmu.yaml
> new file mode 100644
> index 0000000000000..4406c25545229
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/samsung,exynos9610-cmu.yaml
[ ... ]
> @@ -0,0 +1,335 @@
[ ... ]
> +description: |
> +  Exynos9610 clock controller is comprised of several CMU units, generating
> +  clocks for different domains. Those CMU units are modeled as separate device
> +  tree nodes, and might depend on each other. The root clock in that root tree
> +  is an external clock: OSCCLK (26MHz). This external clock must be defined
> +  as a fixed-rate clock in the device tree.
> +
> +  CMU_TOP is a top-level CMU, where all base clocks are prepared using PLLs and
> +  dividers; all other clocks of function blocks (other CMUs) are usually
> +  derived from CMU_TOP.
> +
> +  Each clock is assigned an identifier and client nodes can use this identifier
> +  to specify the clock which they consume. All clocks available for usage
> +  in clock consumer nodes are defined as preprocessor macros in
> +  'include/dt-bindings/clock/samsung,exynos9610.h'.

[Severity: Low]
This isn't a bug, but there is a typo in the header file name here. Should
it be samsung,exynos9610-cmu.h instead of samsung,exynos9610.h?

> +
> +properties:
> +  compatible:
[ ... ]
> diff --git a/include/dt-bindings/clock/samsung,exynos9610-cmu.h b/include/dt-bindings/clock/samsung,exynos9610-cmu.h
> new file mode 100644
> index 0000000000000..c07093bae253e
> --- /dev/null
> +++ b/include/dt-bindings/clock/samsung,exynos9610-cmu.h
[ ... ]
> @@ -0,0 +1,722 @@
[ ... ]
> +/* CMU_CMGP muxes */
> +#define CLK_MOUT_CLK_CMGP_ADC			0
> +#define CLK_MOUT_CLK_CMGP_I2C			1
> +#define CLK_MOUT_CLK_CMGP_USI00			2
> +#define CLK_MOUT_CLK_CMGP_USI01			3
> +#define CLK_MOUT_CLK_CMGP_USI02			4
> +#define CLK_MOUT_CLK_CMGP_USI03			5
> +#define CLK_MOUT_CLK_CMGP_USI04			6
> +
> +/* CMU_CMGP dividers */
> +#define CLK_DOUT_CLK_CMGP_ADC			0
> +#define CLK_DOUT_CLK_CMGP_I2C			1
> +#define CLK_DOUT_CLK_CMGP_USI00			2
> +#define CLK_DOUT_CLK_CMGP_USI01			3
> +#define CLK_DOUT_CLK_CMGP_USI02			4
> +#define CLK_DOUT_CLK_CMGP_USI03			5
> +#define CLK_DOUT_CLK_CMGP_USI04			6

[Severity: High]
Does this overlap in clock IDs cause a collision within the CMU_CMGP clock
controller domain?

Since both the muxes and dividers share the same IDs (0 through 6), it seems
one clock will overwrite the other in the provider's clock array. Will consumers
requesting a divider receive the corresponding mux instead, breaking device
drivers that depend on CMGP clocks like the ADC, I2C, and USI?

> +
> +/* CMU_CMGP gates */
> +#define CLK_GOUT_CMGP_CMU_PCLK			7

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716-exynos9610-clocks-v2-0-c13ce8ef1861@chimac.ro?part=1