Re: [PATCH 16/20] clk: berlin: add Synaptics SL261X SoC clocks and plls

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] clk: berlin: missing .disable callbacks in sysmgr clock operations
- [Low] kbuild: Linker error due to missing Kconfig dependency for SL261X_PLL
--

commit 56d97606c2f6bbb88d4a36a5f8b06b807c05523d
Author: Jisheng Zhang <jszhang@kernel.org>

clk: berlin: add Synaptics SL261X SoC clocks and plls

Add support for clocks, plls in Synaptics SL261X SoCs.

> diff --git a/drivers/clk/berlin/Kconfig b/drivers/clk/berlin/Kconfig
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/berlin/Kconfig
[ ... ]
> +config SL261X_CLK
> +	tristate "Synaptics SL261X Clock Driver"
> +	depends on ARCH_BERLIN || COMPILE_TEST
> +	select SL_COMMON_CLK
> +	select SL_SYSMGR_CLK
> +	default n

[Severity: Low]
Does this missing dependency lead to a linker error? The SL261X_CLK config
fails to declare a dependency or select on SL261X_PLL, but the clock driver
unconditionally calls sl261x_pll_register() defined in the PLL driver. If
CONFIG_SL261X_CLK=y but CONFIG_SL261X_PLL=n or m, the build will fail with an
undefined reference.

[ ... ]
> diff --git a/drivers/clk/berlin/sl-sysmgr-clk.c b/drivers/clk/berlin/sl-sysmgr-clk.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/berlin/sl-sysmgr-clk.c
[ ... ]
> +static const struct clk_ops sl_sysmgr_gateclk_ops = {
> +	.enable		= sl_sysmgr_gateclk_enable,
> +	.is_enabled	= sl_sysmgr_gateclk_is_enabled,
> +};

[Severity: Medium]
Will this leave the gate clocks permanently enabled? The ops struct for gate
clocks implements .enable to ungate the clock but omits the .disable callback.
When a consumer driver calls clk_disable_unprepare(), the common clock
framework will silently skip the hardware operation, leaking power.

[ ... ]
> +static const struct clk_ops sl_sysmgr_clk_ops = {
> +	.recalc_rate	= sl_sysmgr_clk_recalc_rate,
> +	.enable		= sl_sysmgr_clk_enable,
> +	.is_enabled	= sl_sysmgr_clk_is_enabled,
> +};

[Severity: Medium]
Does this have the same issue with missing disable operations? The ops struct
for divider clocks implements .enable but also omits the .disable callback.

[ ... ]
> diff --git a/drivers/clk/berlin/sl261x-clks.c b/drivers/clk/berlin/sl261x-clks.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/berlin/sl261x-clks.c
[ ... ]
> +static int sl261x_soc_clk_setup(struct platform_device *pdev)
> +{
[ ... ]
> +	/* sysplls */
> +	ret = sl261x_pll_register(dev, base + SYSPLL0_OFFSET,
> +				  clk_names[SYSPLL0_OUT], clk_names[SYSPLL0_OUTF]);

[Severity: Low]
This is the call to sl261x_pll_register() mentioned above that requires
SL261X_PLL to be selected or depended on in Kconfig to avoid linker errors.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929061417.20733-1-jszhang@kernel.org?part=16