Re: [PATCH v11 2/2] clk: qcom: Add LPASS VA CSR heartbeat pulse clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: qcom: build failure due to missing PM_CLK dependency when CONFIG_PM=n
- [Medium] clk: qcom: missing hardware initialization when CONFIG_PM is disabled
- [Low] clk: qcom: unused-function warnings for PM callbacks when CONFIG_PM=n
--

commit a480f5be8e9c721f7e655f8562c6eda38da7c50c
Author: Sarath Ganapathiraju <sarath.ganapathiraju@oss.qualcomm.com>
Subject: clk: qcom: Add LPASS VA CSR heartbeat pulse clock

This commit adds a clock provider driver for the LPASS VA CSR heartbeat pulse 
on Qualcomm SoCs. The driver models the rate generator and restores register 
state during runtime PM resume.

> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index 27d0ab24d50a4..da2e8005d0756 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -2137,6 +2137,20 @@ config CLK_GFM_LPASS_SM8250
>  	  Support for the Glitch Free Mux (GFM) Low power audio
>            subsystem (LPASS) clocks found on SM8250 SoCs.
>  
> +config QCOM_CLK_LPASS_HEARTBEAT_PULSE
> +	tristate "Qualcomm LPASS VA CSR heartbeat pulse clock provider"
> +	depends on COMMON_CLK
> +	depends on ARM64 || COMPILE_TEST

[Severity: High]
Should this Kconfig entry have a dependency on PM or PM_CLK?

When the driver is built under COMPILE_TEST without CONFIG_PM or 
CONFIG_PM_CLK, the lack of dependency exposes the driver to environments where
the pm_clk APIs are undefined, which breaks the build.

> diff --git a/drivers/clk/qcom/lpass-va-csr.c b/drivers/clk/qcom/lpass-va-csr.c
> new file mode 100644
> index 0000000000000..e5fb870aa36de
> --- /dev/null
> +++ b/drivers/clk/qcom/lpass-va-csr.c

[ ... ]

> +static int lpass_va_csr_runtime_resume(struct device *dev)
> +{
> +	struct lpass_va_csr *csr = dev_get_drvdata(dev);
> +	const struct lpass_va_csr_data *data = csr->data;
> +	int ret;
> +
> +	ret = pm_clk_resume(dev);

[Severity: High]
Will this cause a build failure when CONFIG_PM_CLK is not enabled?

Because the Kconfig entry does not depend on PM_CLK, this code can be
compiled when pm_clk_resume expands to NULL. This causes the preprocessor to
expand the line to ret = NULL(dev); during compilation, resulting in a fatal
error for calling a void pointer.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(csr->hb.regmap, LPASS_RATE_GEN_COUNTER_0,
> +			   data->counter_0);

[Severity: Medium]
Will the hardware operate correctly if CONFIG_PM is disabled?

The driver's hardware initialization (programming LPASS_RATE_GEN_COUNTER_0
and DELAY) is executed exclusively inside this lpass_va_csr_runtime_resume()
callback.

If the kernel is built with CONFIG_PM=n, the pm_runtime_resume_and_get()
call in lpass_va_csr_probe() trivially returns 0 without executing this
callback. As a result, the hardware registers will remain uninitialized and
operate with default or undefined values.

Should the driver enforce depends on PM in its Kconfig, or explicitly
initialize the hardware in probe() when PM is not enabled?

[ ... ]

> +static const struct dev_pm_ops lpass_va_csr_pm_ops = {
> +	SET_RUNTIME_PM_OPS(lpass_va_csr_runtime_suspend,
> +			   lpass_va_csr_runtime_resume, NULL)
> +};

[Severity: Low]
Does this code trigger unused-function warnings when CONFIG_PM=n?

The driver defines lpass_va_csr_runtime_suspend() and
lpass_va_csr_runtime_resume() as static functions without the __maybe_unused
annotation. 

When CONFIG_PM is disabled, SET_RUNTIME_PM_OPS evaluates to an empty block,
leaving the static callback functions completely unreferenced. This triggers
-Wunused-function compiler warnings, which can break builds configured
with -Werror. 

Would it be better to use __maybe_unused on the callbacks, or adopt the modern
RUNTIME_PM_OPS() macro instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-master-v11-0-15009e3ecac3@oss.qualcomm.com?part=2