Re: [PATCH v13 10/13] clk: zte: Introduce a driver for zx297520v3 matrix clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: zx297520v3: orphaned firmware clocks due to NULL dev->of_node
- [Low] clk: zx297520v3: compilation failure due to const initializers
--

commit 1f05d5e802fc7c2765df7aa9b0a1ac54ff100e45
Author: Stefan Dösinger <stefandoesinger@gmail.com>

    clk: zte: Introduce a driver for zx297520v3 matrix clocks

This commit introduces a clock controller driver for the zx297520v3 matrix
clocks, covering high-speed devices like CPU, DMA, RAM, SDIO, and Ethernet.
It defines fixed dividers, muxes, and gates for the matrix clock tree.

> diff --git a/drivers/clk/zte/clk-zx297520v3.c b/drivers/clk/zte/clk-zx297520v3.c
> index 68bfeab84d745..d5f8bc4064bd4 100644
> --- a/drivers/clk/zte/clk-zx297520v3.c
> +++ b/drivers/clk/zte/clk-zx297520v3.c
[ ... ]
> @@ -812,6 +812,528 @@ static const struct zx_clk_data zx297520v3_topclk_data = {
[ ... ]
> +static const struct zx_parent_desc matrix_unk = PARENT_FW("wclk-osc26m");
[ ... ]
> +static const struct zx_clock zx297520v3_matrix_clocks[] = {
[ ... ]
> +	[ZX_CLK_CPU_WCLK]         = GATE("cpu-wclk", PARENT_ID(ZX_CLK_CPU_MUX), 0x24, 1,
> +					 CLK_IS_CRITICAL),
> +	[ZX_CLK_CPU_PCLK]         = GATE("cpu-pclk", clk_main[0], 0x24, 2, CLK_IS_CRITICAL),

[Severity: Low]
Is this code valid C?

The array element clk_main[0] is being used as a static struct initializer.
In C, unlike C++, array elements and const variables are not constant
expressions, which will produce a fatal "initializer element is not constant"
build error.

[ ... ]
> +	[ZX_CLK_PDCFG_WCLK]       = GATE("pdcfg-wclk", matrix_unk, 0x88, 0, CLK_IS_CRITICAL),

[Severity: Low]
Does this initializer also fail to build?

Similar to the above, matrix_unk is a const variable and cannot be used
as a static struct initializer in C.

[ ... ]
> +static const struct zx_clk_data zx297520v3_matrixclk_data = {
> +	.init = zx297520v3_matrix_init,
> +	.clocks = zx297520v3_matrix_clocks,
> +	.num_clocks = ARRAY_SIZE(zx297520v3_matrix_clocks),
> +	.exports = zx297520v3_matrix_exports,
> +	.num_exports = ARRAY_SIZE(zx297520v3_matrix_exports),
> +};
> +
>  static int clk_zx297520v3_probe(struct platform_device *pdev)
>  {
>  	const struct platform_device_id *id = platform_get_device_id(pdev);
>  
>  	if (!id)
>  		return -ENODEV;
>  
>  	return zx_clk_common_probe(&pdev->dev, pdev->dev.parent->of_node,
>  				   (const struct zx_clk_data *)id->driver_data);
>  }

[Severity: High]
Does this code leave all firmware-based clocks orphaned?

The parent's of_node is passed explicitly to zx_clk_common_probe() for
validation and provider registration, but it is never assigned to
pdev->dev.of_node. Since the matrix clock controller is an MFD child device
without an of_compatible, its pdev->dev.of_node will be NULL.

When registering the clocks, the Common Clock Framework relies on dev->of_node
to resolve fw_name parents. For example, in zx_clk_register_fixed_div() in
drivers/clk/zte/clk-zx.c:

	case ZX_PARENT_FW:
		return devm_clk_hw_register_fixed_factor_fwname(dev, NULL,
				desc->name, p->fw_name,
				CLK_SET_RATE_PARENT, 1, desc->div);

Because dev->of_node is NULL, clk_core_fill_parent()'s of_clk_get_by_name()
will instantly return -ENOENT.

Should pdev->dev.of_node be assigned before probing so the CCF can resolve
the clock tree?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927-zx29clk-v13-0-70d8f673386c@gmail.com?part=10

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy