Re: [PATCH v7 09/10] clk: realtek: Add RTD1625-ISO clock controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Missing MODULE_DEVICE_TABLE for the OF match table in the clock driver.
- [Medium] Flexible array overallocation and `__counted_by` mismatch in `hws` array initialization.
- [Low] Typo in MODULE_DESCRIPTION in the reset driver.
--

commit 57dcde089e2dea62cefc3bf2d39edbfbdf75b997
Author: Cheng-Yu Lee <cylee12@realtek.com>
Subject: clk: realtek: Add RTD1625-ISO clock controller driver

Add support for the ISO (Isolation) domain clock controller on the Realtek
RTD1625 SoC. This controller manages clocks in the always-on power domain.
It also includes an auxiliary reset driver instantiated by the core clock
driver to handle the ISO domain resets.

> diff --git a/drivers/clk/realtek/clk-rtd1625-iso.c b/drivers/clk/realtek/clk-rtd1625-iso.c
> new file mode 100644
> index 0000000000000..58c2b147eb019
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-rtd1625-iso.c

[ ... ]

> +static struct clk_hw_onecell_data rtd1625_iso_clk_data = {
> +	.num = RTD1625_ISO_CLK_MAX,
> +	.hws = {

[ ... ]

> +		[RTD1625_ISO_CLK_EN_MISC_VFD]    = &__clk_regmap_gate_hw(&clk_en_misc_vfd),
> +		[RTD1625_ISO_CLK_MAX] = NULL,

Could this cause a bounds mismatch?

By setting .num to RTD1625_ISO_CLK_MAX but initializing the array at the
MAX index, the array size is implicitly expanded to MAX + 1 elements. This
appears to violate the __counted_by(num) contract in struct
clk_hw_onecell_data.

Should this trailing NULL initialization be removed?

> +	},
> +};

[ ... ]

> +static struct clk_hw_onecell_data rtd1625_iso_s_clk_data = {
> +	.num = RTD1625_ISO_S_CLK_MAX,
> +	.hws = {
> +		[RTD1625_ISO_S_CLK_EN_IRDA] = &__clk_regmap_gate_hw(&clk_en_irda),
> +		[RTD1625_ISO_S_CLK_EN_UR10] = &__clk_regmap_gate_hw(&clk_en_ur10),
> +		[RTD1625_ISO_S_CLK_MAX] = NULL,

Is this trailing NULL initialization needed here as well? Like the above,
this makes the array size RTD1625_ISO_S_CLK_MAX + 1 elements, which
doesn't match the .num value.

> +	},
> +};

[ ... ]

> +static const struct of_device_id rtd1625_iso_match[] = {
> +	{.compatible = "realtek,rtd1625-iso-clk", .data = &rtd1625_iso_desc},
> +	{.compatible = "realtek,rtd1625-iso-s-clk", .data = &rtd1625_iso_s_desc},
> +	{ /* sentinel */ }
> +};
> +

Should there be a MODULE_DEVICE_TABLE(of, rtd1625_iso_match) here?

Without it, the module aliases aren't exported to modules.alias, which
might prevent udevd from automatically loading the driver at boot. This
could also cause the auxiliary reset controller to silently fail to probe.

> +static struct platform_driver rtd1625_iso_driver = {

[ ... ]

> diff --git a/drivers/reset/realtek/reset-rtd1625-iso.c b/drivers/reset/realtek/reset-rtd1625-iso.c
> new file mode 100644
> index 0000000000000..0829a5cb0ac11
> --- /dev/null
> +++ b/drivers/reset/realtek/reset-rtd1625-iso.c

[ ... ]

> +module_auxiliary_driver(rtd1625_iso_driver);
> +
> +MODULE_DESCRIPTION("Reatek RTD1625 ISO Reset Controller Driver");

This isn't a bug, but there's a typo in the module description.
"Reatek" should probably be "Realtek".

> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("REALTEK_RESET");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260508111641.3192177-1-eleanor.lin@realtek.com?part=9