Re: [PATCH v16 7/9] clk: realtek: Add RTD1625-CRT clock controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] clk: realtek: FREQ_MNO_MASK omits bit 19 of O divider for
  pll_vodma
- [Medium] clk: realtek: static clk_hw.init cleared without
  suppress_bind_attrs
--

commit 127e3b9f9c4378fa7aaceda027d1d3b761330569
Author: Cheng-Yu Lee <cylee12@realtek.com>

clk: realtek: Add RTD1625-CRT clock controller driver

This commit adds the CRT domain clock controller driver for the Realtek
RTD1625 SoC and registers the associated reset controller on the auxiliary
bus.

> diff --git a/drivers/clk/realtek/clk-rtd1625-crt.c b/drivers/clk/realtek/clk-rtd1625-crt.c
> new file mode 100644
> index 0000000000000..b0cccf5c7af2d
> --- /dev/null
> +++ b/drivers/clk/realtek/clk-rtd1625-crt.c
> @@ -0,0 +1,878 @@
[ ... ]
> +/* PLL VODMA: M (11:4) / N (13:12) / O (19:17) */
> +#define FREQ_MNO_MASK            0x63ff0
> +#define FREQ_MNO(_r, _m, _n, _o) { .rate = (_r), .val = ((_m) << 4) | \
> +				 ((_n) << 12) | ((_o) << 17), }

[Severity: Medium]
Should FREQ_MNO_MASK be 0xe3ff0 instead of 0x63ff0?  0x63ff0 only covers
bits 18:17 for the O divider field, so rtk_clk_regmap_pll_recalc_rate()
masks off bit 19 when reading RTD1625_REG_PLL_VODMA1 for pll_vodma and can
match the wrong entry in pll_vodma_tbl if O >= 4.

[ ... ]
> +static struct platform_driver rtd1625_crt_driver = {
> +	.probe  = rtd1625_crt_probe,
> +	.driver = {
> +		.name = "rtk-rtd1625-crt-clk",
> +		.of_match_table = rtd1625_crt_match,
> +	},
> +};

[Severity: Medium]
Does rtd1625_crt_driver need .suppress_bind_attrs = true?  During probe,
rtk_clk_probe() registers the static clk_hw structures in
rtd1625_crt_hw_data via devm_clk_hw_register() -> __clk_register(), which
clears hw->init to NULL.  Unbinding and rebinding the driver via sysfs (or
re-probing after a late failure in rtk_clk_probe()) will pass those same
static clk_hw structures to __clk_register() with hw->init == NULL and hit
a NULL pointer dereference.  Unbinding also unregisters CLK_IS_CRITICAL
clocks while they remain prepared and enabled, triggering WARN_ON() in
clk_unregister().

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