Re: [PATCH v5 4/7] clk: qcom: videocc: Add video clock controller driver for Eliza

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `video_cc_eliza_desc` structure omits the `.use_rpm = true` flag (or explicit PM runtime enablement), leading to unpowered register access during probe.
- [Low] CLK_ELIZA_VIDEOCC sets 'default m if ARCH_QCOM', which is inconsistent with Kconfig conventions for Eliza clock controllers.
- [Low] CLK_ELIZA_VIDEOCC implements GDSCs but lacks an explicit 'select QCOM_GDSC' dependency.
--

commit 7f851311a4cc00578bd0ab62871977440dcab6e4
Author: Taniya Das <taniya.das@oss.qualcomm.com>

clk: qcom: videocc: Add video clock controller driver for Eliza

Add support for the video clock controller for video clients to be able
to request for videocc clocks on Eliza platform.

> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
[ ... ]
> @@ -45,6 +45,16 @@ config CLK_ELIZA_TCSRCC
>  	  Support for the TCSR clock controller on Eliza devices.
>  	  Say Y if you want to use peripheral devices such as USB/PCIe/UFS.
>  
> +config CLK_ELIZA_VIDEOCC
> +	tristate "Eliza Video Clock Controller"
> +	depends on ARM64 || COMPILE_TEST
> +	select CLK_ELIZA_GCC

[Severity: Low]
Since this driver implements GDSCs (video_cc_mvs0_gdsc and
video_cc_mvs0c_gdsc), shouldn't it explicitly select QCOM_GDSC here?

While it may compile by transitively inheriting the dependency from
CLK_ELIZA_GCC, could this cause Kconfig unmet dependency warnings if the
GCC configuration changes in the future?

> +	default m if ARCH_QCOM

[Severity: Low]
Is this default configuration intentional?

This deviates from Kconfig conventions for the rest of the Eliza clock
controllers (like CLK_ELIZA_DISPCC, CLK_ELIZA_GCC, and CLK_ELIZA_TCSRCC)
which do not set default values and instead rely on defconfig for enablement.

> +	help
> +	  Support for the video clock controller on Eliza devices.
> +	  Say Y if you want to support video devices and functionality such as
> +	  video encode and decode.
> +
[ ... ]
> diff --git a/drivers/clk/qcom/videocc-eliza.c b/drivers/clk/qcom/videocc-eliza.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/clk/qcom/videocc-eliza.c
[ ... ]
> +static const struct qcom_cc_desc video_cc_eliza_desc = {
> +	.config = &video_cc_eliza_regmap_config,
> +	.clks = video_cc_eliza_clocks,
> +	.num_clks = ARRAY_SIZE(video_cc_eliza_clocks),
> +	.resets = video_cc_eliza_resets,
> +	.num_resets = ARRAY_SIZE(video_cc_eliza_resets),
> +	.gdscs = video_cc_eliza_gdscs,
> +	.num_gdscs = ARRAY_SIZE(video_cc_eliza_gdscs),
> +	.driver_data = &video_cc_eliza_driver_data,

[Severity: High]
Will omitting the .use_rpm flag (or explicit PM runtime enablement) cause a
synchronous external abort and hang the system during boot?

Multimedia clock controllers usually require their hardware power domains
(such as MMCX) to be enabled via PM runtime before their registers can be read
or written.

Without .use_rpm = true, qcom_cc_really_probe() skips calling
devm_pm_runtime_enable() and pm_runtime_resume_and_get(), which would cause
the subsequent initialization of PLLs and CBCRs to attempt register access on
unpowered hardware.

> +};
> +
> +static const struct of_device_id video_cc_eliza_match_table[] = {
> +	{ .compatible = "qcom,eliza-videocc" },
> +	{ }
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525-eliza_mm_cc_v2-v5-0-a1d125619a5a@oss.qualcomm.com?part=4