Re: [PATCH 3/4] clk: qcom: Add lpass audio clock controller driver for Glymur

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] clk: qcom: regmap max_register restricts access to WSA3/WSA4 resets
- [Medium] clk: qcom: SC_LPASSCC_GLYMUR missing select CLK_GLYMUR_GCC dependency
- [Medium] clk: qcom: missing NULL check on of_device_get_match_data() result
--

commit a3d72a1f7ec423f2250c7ae64750a7f38d02a929
Author: Srinivas Kandagatla <srinivas.kandagatla@oss.qualcomm.com>

clk: qcom: Add lpass audio clock controller driver for Glymur

This commit adds support for the lpass audio clock controller found on
Glymur based devices. It allows lpass peripheral loader drivers to control
the clocks and bring the subsystems out of reset.

> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index d1fc9bf87126b..8a6c15ab9da7a 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -1177,6 +1177,17 @@ config SC_LPASSCC_8280XP
>  	  Say Y if you want to use the LPASS branch clocks of the LPASS clock
>  	  controller to reset the LPASS subsystem.
>  
> +config SC_LPASSCC_GLYMUR
> +	tristate "GLYMUR Low Power Audio Subsystem (LPASS) Clock Controller"
> +	depends on ARM64 || COMPILE_TEST

[Severity: Medium]
Does this Kconfig entry miss a dependency on the global clock controller?

If this driver is built without CLK_GLYMUR_GCC being enabled, can it lead
to unclocked register accesses and probe failures, since peripheral
controllers depend on GCC for AHB/AXI access?

Additionally, does the naming prefix SC_LPASSCC_GLYMUR diverge from the
standard CLK_GLYMUR_* prefix used by other Glymur clock controllers,
potentially violating cross-config consistency guidelines?

> +	default m if ARCH_QCOM
> +	help
> +	  Support for the LPASS clock controller on Glymur devices.
> +	  Say Y if you want to use the LPASS branch clocks of the LPASS clock
> +	  controller to reset the LPASS subsystem.  Required for the LPASS
> +	  audio macros and the SoundWire compute-mode controller to leave
> +	  reset on Glymur.

[ ... ]

> diff --git a/drivers/clk/qcom/lpasscc-glymur.c b/drivers/clk/qcom/lpasscc-glymur.c
> new file mode 100644
> index 0000000000000..97fc099983b62
> --- /dev/null
> +++ b/drivers/clk/qcom/lpasscc-glymur.c
> @@ -0,0 +1,70 @@

[ ... ]

> +static const struct qcom_reset_map lpass_audiocc_glymur_resets[] = {
> +	[LPASS_AUDIO_SWR_RX_CGCR] = { 0xA0, 1 },
> +	[LPASS_AUDIO_SWR_WSA1_CGCR] = { 0xB0, 1 },
> +	[LPASS_AUDIO_SWR_WSA2_CGCR] = { 0xD8, 1 },
> +	[LPASS_AUDIO_SWR_WSA3_CGCR] = { 0x300C, 1 },
> +	[LPASS_AUDIO_SWR_WSA4_CGCR] = { 0x301C, 1 },
> +};
> +
> +static const struct regmap_config lpass_audiocc_glymur_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.name = "lpass-audio-csr",
> +	.max_register = 0x1000,

[Severity: High]
Does this max_register boundary of 0x1000 prevent access to the WSA3 and
WSA4 resets at offsets 0x300C and 0x301C defined above?

Will the regmap core reject attempts to assert or deassert those resets,
silently failing with -EIO and leaving the dependent audio blocks broken in
an undefined reset state?

> +};

[ ... ]

> +static int lpasscc_glymur_probe(struct platform_device *pdev)
> +{
> +	const struct qcom_cc_desc *desc = of_device_get_match_data(&pdev->dev);
> +

[Severity: Medium]
If the driver is bound manually to a non-DT platform device via sysfs, could
of_device_get_match_data() return NULL here?

If so, when desc is passed to qcom_cc_probe_by_index(), will it cause a
NULL pointer dereference when accessing desc->config?

> +	return qcom_cc_probe_by_index(pdev, 0, desc);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260922184425.56114-1-srinivas.kandagatla@oss.qualcomm.com?part=3