Re: [PATCH v2 2/2] clk: amlogic: Add A9 AO clock controller driver

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] The Kconfig symbol `COMMON_CLK_A9_AO` inappropriately uses `default ARCH_MESON || COMPILE_TEST`. This forces the driver to be enabled by default on all compile-test builds, polluting the default configuration and breaking cross-config consistency.
- [Low] The clock controller driver defines an internal clock as `i2c3` but maps it to `CLKID_AO_SYS_I3C`, indicating a typo where 'i3c' was mistakenly written as 'i2c3'.
--

commit a36917077cf54c71ca4b917e7b2122bd84e2b956
Author: Jian Hu <[email protected]>

clk: amlogic: Add A9 AO clock controller driver

This commit adds the Always-on clock controller driver for the Amlogic A9
SoC family.

> diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> index cf8cf3f9e4ee8..625e6788b9406 100644
> --- a/drivers/clk/meson/Kconfig
> +++ b/drivers/clk/meson/Kconfig
> @@ -132,6 +132,19 @@ config COMMON_CLK_A1_PERIPHERALS
>  	  device, A1 SoC Family. Say Y if you want A1 Peripherals clock
>  	  controller to work.
>  
> +config COMMON_CLK_A9_AO
> +	tristate "Amlogic A9 SoC AO clock controller support"
> +	depends on ARM64
> +	default ARCH_MESON || COMPILE_TEST

[Severity: Low]
Should COMPILE_TEST be used in a default directive here? 

By including it in the default expression, the driver will automatically be
enabled on all compile-test builds, polluting the default configuration. 
Usually, COMPILE_TEST is placed in the 'depends on' clause instead to 
allow build coverage without forcing it on by default.

> +	select COMMON_CLK_MESON_REGMAP
> +	select COMMON_CLK_MESON_CLKC_UTILS

[ ... ]

> diff --git a/drivers/clk/meson/a9-aoclk.c b/drivers/clk/meson/a9-aoclk.c
> new file mode 100644
> index 0000000000000..b7b3ca231a42b
> --- /dev/null
> +++ b/drivers/clk/meson/a9-aoclk.c

[ ... ]

> +/*
> + * A9 integrates a low-power microprocessor (Always-on CPU: AOCPU). Some AO sys
> + * clocks control the AOCPU modules. Mark the AOCPU-related clocks with
> + * CLK_IS_CRITICAL to avoid them being disabled and impacting AOCPU functionality.
> + * AOCPU-related clocks list:
> + * - clktree
> + * - rst_ctrl
> + * - pad
> + * - irq
> + * - pwrctrl
> + * - aocpu
> + * - sram
> + */
> +static A9_AO_PCLK(i2c3,		0,	0);

[Severity: Low]
Is this intended to be an I3C clock rather than I2C? 

Since the driver defines the other I2C controllers alphabetically (i2c_a, 
i2c_b, etc.), and this gets mapped to CLKID_AO_SYS_I3C below, it looks like 
'i2c3' might be a typo for 'i3c'. 

Because of the macro stringification, this typo will cause the Common Clock 
Framework to register the clock with the wrong name.

> +static A9_AO_PCLK(rtc_reg,	1,	0);
> +static A9_AO_PCLK(clktree,	2,	CLK_IS_CRITICAL);

[ ... ]

> +static struct clk_hw *a9_ao_hw_clks[] = {
> +	[CLKID_AO_XTAL_IN]		= &a9_ao_xtal_in.hw,
> +	[CLKID_AO_XTAL]			= &a9_ao_xtal.hw,
> +	[CLKID_AO_SYS]			= &a9_ao_sys.hw,
> +	[CLKID_AO_SYS_I3C]		= &a9_ao_sys_i2c3.hw,

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic