Re: [PATCH v12 08/12] clk: zte: Introduce a driver for zx297520v3 top clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] clk: zte: clock provider registered with NULL of_node breaks DT consumers
--

commit 941de7b2f6e4b79d334d74edd0d2dd9620fabd7e
Author: Stefan Dösinger <stefandoesinger@gmail.com>

clk: zte: Introduce a driver for zx297520v3 top clocks

This commit introduces a new clock driver for the ZTE zx297520v3 SoC top
clocks, which control core devices like PLLs, AHB bus, timers, and USB.
It registers as an MFD child device and provides various clock gates,
dividers, and muxes to the system.

> diff --git a/drivers/clk/zte/Kconfig b/drivers/clk/zte/Kconfig
> index 152c2423b8f02..0ccc6edf3c5b1 100644
> --- a/drivers/clk/zte/Kconfig
> +++ b/drivers/clk/zte/Kconfig
> @@ -15,3 +15,14 @@ config COMMON_CLK_ZTE
>  	  of this.
>  
>  	  Enable this if you are building a kernel for a ZTE designed board.
> +
> +config CLK_ZTE_ZX297520V3
> +	tristate "Clock driver for ZTE zx297520v3 based SoCs"
> +	depends on COMMON_CLK_ZTE && (MFD_ZTE_ZX297520V3_CRM || COMPILE_TEST)

[Severity: Medium]
This is a pre-existing issue, but will enabling this new configuration cause 
linker errors on builds where CONFIG_RATIONAL is not selected elsewhere?

Building the kernel with CONFIG_COMMON_CLK_ZTE=y (which is forced by this new 
config) without CONFIG_RATIONAL causes an undefined reference to 
rational_best_approximation.

The failure occurs in drivers/clk/zte/pll-zx.c:zx29_pll_calc_values():

        rational_best_approximation(vco, parent_rate,
                                    (1 << hweight32(ZX29_PLL_FBDIV_MASK)) - 1,
                                    (1 << hweight32(ZX29_PLL_REFDIV_MASK)) - 1,
                                    &fbdiv, &refdiv);

Should RATIONAL be explicitly selected by this new config, or perhaps added to 
COMMON_CLK_ZTE directly?

> diff --git a/drivers/clk/zte/clk-zx297520v3.c b/drivers/clk/zte/clk-zx297520v3.c
> new file mode 100644
> index 0000000000000..68bfeab84d745
> --- /dev/null
> +++ b/drivers/clk/zte/clk-zx297520v3.c

[ ... ]

> +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]
Since this driver is an MFD child, will registering the clock provider using 
the child's device pointer result in a NULL device tree node?

In clk_zx297520v3_probe, the code correctly passes pdev->dev.parent->of_node 
to zx_clk_common_probe to handle regmap lookups. However, the device pointer 
passed is still pdev->dev:

drivers/clk/zte/clk-zx297520v3.c:clk_zx297520v3_probe()
        return zx_clk_common_probe(&pdev->dev, pdev->dev.parent->of_node,
                                   (const struct zx_clk_data *)id->driver_data);

When zx_clk_common_probe registers the provider, it uses the passed device 
structure which lacks an of_node:

drivers/clk/zte/clk-zx.c:zx_clk_common_probe()
        return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, exports);

Because the MFD child cell doesn't have an explicit device tree node, 
dev->of_node is NULL. Could this cause device tree consumers to fail to find 
and acquire these clocks during boot?

Should pdev->dev.of_node be explicitly assigned to pdev->dev.parent->of_node 
prior to calling zx_clk_common_probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923-zx29clk-v12-0-9c243796b6d2@gmail.com?part=8

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