Re: [PATCH v10 05/12] clk: zte: Add Clock registration infrastructure

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The clock driver fails to pass the explicit `of_node` down to the clock registration functions, causing the Common Clock Framework (CCF) to fall back to the child device's `dev->of_node` and breaking parent clock resolution.
--

--- Patch [5]: [PATCH v10 05/12] clk: zte: Add Clock registration infrastructure ---
commit 796448cb70bee510176b9b976cf13443c5e9c07b
Author: Stefan Dösinger <[email protected]>

clk: zte: Add Clock registration infrastructure
    
This patch adds the common clock registration infrastructure for ZTE SoCs.
It prepares for regmap clocks and PLL drivers to be implemented in subsequent
patches. The implementation allows the clock driver to act as an MFD child.

> diff --git a/drivers/clk/zte/clk-zx.c b/drivers/clk/zte/clk-zx.c
> index 0000000000000..aef0f7c753e64 100644
> --- /dev/null
> +++ b/drivers/clk/zte/clk-zx.c
> @@ -0,0 +1,192 @@

[ ... ]

> +static struct clk_hw *zx_clk_register_fixed_div(struct device *dev,
> +						const struct zx_fixed_divider_desc *desc,
> +						struct clk_hw * const *clocks)
> +{
> +	const struct zx_parent_desc *p = &desc->parent;
> +
> +	switch (p->type) {
> +	case ZX_PARENT_FW:
> +		return devm_clk_hw_register_fixed_factor_fwname(dev, NULL, desc->name, p->fw_name,
                                                                     ^^^^

[Severity: High]
Does passing NULL for the fw_node parameter here force the Common Clock
Framework to fall back to dev->of_node?

Since this driver probes as an MFD child, dev->of_node is typically NULL or a
child node without the clock-names property. If CCF looks for the clock-names
property in the wrong device node, it could cause parent clock dependencies
defined via fw_name to fail to resolve, leaving the clocks orphaned.

> +								CLK_SET_RATE_PARENT, 1, desc->div);
> +
> +	case ZX_PARENT_ID:

[ ... ]

> +int zx_clk_common_probe(struct device *dev, struct device_node *of_node,
> +			const struct zx_clk_data *data)
> +{

[ ... ]

> +	for (i = 0; i < data->num_clocks; ++i) {
> +		struct clk_hw *hw;
> +
> +		switch (data->clocks[i].type) {
> +		case ZX_CLOCK_PLL:
> +			hw = zx_clk_register_pll(dev, map, &data->clocks[i].pll, clocks);

[Severity: High]
Should the explicit of_node parameter be passed down to the clock registration
functions inside this loop?

The of_node is correctly accepted by zx_clk_common_probe() and used for
validation, but it is dropped when dispatching to the registration functions.

> +			break;
> +
> +		case ZX_CLOCK_FIXED_DIV:
> +			hw = zx_clk_register_fixed_div(dev, &data->clocks[i].fixed_div, clocks);
> +			break;
> +
> +		case ZX_CLOCK_MUX:

[ ... ]

> diff --git a/drivers/clk/zte/clk-zx.h b/drivers/clk/zte/clk-zx.h
> index 0000000000000..8137185f497f7 100644
> --- /dev/null
> +++ b/drivers/clk/zte/clk-zx.h
> @@ -0,0 +1,137 @@

[ ... ]

> +struct zx_clock {
> +	enum zx_clock_type type;
> +	union {
> +		struct zx_pll_desc pll;
> +		struct zx_fixed_divider_desc fixed_div;
> +		struct zx_mux_desc mux;
> +		struct zx_div_desc div;
> +		struct zx_gate_desc gate;
> +	};
> +};
> +
> +struct clk_hw *zx_clk_register_pll(struct device *dev, struct regmap *regmap,
> +				   const struct zx_pll_desc *desc,
> +				   struct clk_hw * const *clocks);

[Severity: High]
Does this function signature need to include the of_node parameter?

Currently, the of_node parameter is completely omitted here. This guarantees
that future implementations, such as PLL and Mux registration functions, will
also fail to pass the correct node to the Common Clock Framework.

> +struct clk_hw *zx_clk_register_mux(struct device *dev, struct regmap *regmap,
> +				   const struct zx_mux_desc *desc,
> +				   struct clk_hw * const *clocks);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy