Re: [PATCH v6 6/9] clk: eyeq: Introduce a generic clock type

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Clock probe functions incorrectly return a boolean value instead of a negative error code upon failure.
- [Medium] eqc_probe_divider() drops the device_node parameter, breaking device tree linkage for early dividers.
- [Medium] eqc_fill_parent_data() fails to handle parent clocks that belong to the early clock provider.
--

commit 7d25c6d0cb4fb3419bcfe17efee3ab5acfc0310a
Author: Benoît Monin <benoit.monin@bootlin.com>

clk: eyeq: Introduce a generic clock type

This commit introduces a new generic clock type to support complex
clock interdependencies in the OLB, including probe and unregister
function pointers. It updates the match data structures and adds
probe callbacks for dividers, fixed factors, and PLLs.

> diff --git a/drivers/clk/clk-eyeq.c b/drivers/clk/clk-eyeq.c
> index e4e690a12a238..623b1d3f041df 100644
> --- a/drivers/clk/clk-eyeq.c
> +++ b/drivers/clk/clk-eyeq.c

[ ... ]

> @@ -336,6 +375,101 @@ static void eqc_auxdev_create_optional(struct device *dev, void __iomem *base,
>  	}
>  }
>  
> +static int eqc_fill_parent_data(const struct eqc_clock *clk,
> +				struct clk_hw_onecell_data *cells,
> +				struct clk_parent_data *parent_data)
> +{
> +	int pidx = clk->parent_idx;
> +
> +	memset(parent_data, 0, sizeof(struct clk_parent_data));
> +
> +	if (pidx == PARENT_BY_FWNAME) {
> +		/* lookup the parent clock by its fw_name */
> +		parent_data->index = -1;
> +		parent_data->fw_name = clk->parent_name;
> +	} else if (pidx == PARENT_BY_NAME) {
> +		/* lookup the parent clock by its global name */
> +		parent_data->index = -1;
> +		parent_data->name = clk->parent_name;
> +	} else if (pidx >= 0 && pidx < cells->num && !IS_ERR(cells->hws[pidx])) {
> +		/* get the parent hw directly */
> +		parent_data->hw = cells->hws[pidx];
> +	} else {
> +		return -EINVAL;
> +	}

During the late probe phase, early clocks are marked with ERR_PTR(-EINVAL)
in the cells->hws array. If a late clock depends on an early clock,
!IS_ERR(cells->hws[pidx]) will evaluate to false, and this will return -EINVAL.

Does this prevent late clocks from using early clocks as parents?

Should this assign parent_data->index = pidx when the hardware pointer is an
error, allowing the clock framework to resolve the parent index via the
device tree?

> +
> +	return 0;
> +}
> +
> +static int eqc_probe_divider(struct device *dev, struct device_node *np,
> +			     const struct eqc_clock *clk, void __iomem *base,
> +			     struct clk_hw_onecell_data *cells)
> +{
> +	struct clk_parent_data parent_data;
> +	struct clk_hw *hw;
> +	int ret;
> +
> +	ret = eqc_fill_parent_data(clk, cells, &parent_data);
> +	if (ret)
> +		return ret;
> +
> +	hw = clk_hw_register_divider_table_parent_data(dev, clk->name,
> +			&parent_data, 0, base + clk->div.reg, clk->div.shift, clk->div.width,
> +			clk->div.table ? 0 : CLK_DIVIDER_EVEN_INTEGERS, clk->div.table, NULL);

The clk_hw_register_divider_table_parent_data() macro hardcodes the
device_node (np) argument to NULL when calling __clk_hw_register_divider().

If this is called during early initialization where dev is NULL, does this
cause both dev and np to be NULL? Could this break device tree-based parent
resolution and prevent of_clk_set_defaults() from working?

> +	if (IS_ERR(hw))
> +		return IS_ERR(hw);

Since IS_ERR() evaluates to a boolean, does this return 1 on failure instead
of the actual negative error code? Should this return PTR_ERR(hw) instead?

> +
> +	cells->hws[clk->index] = hw;
> +	return 0;
> +}
> +
> +static int eqc_probe_fixed_factor(struct device *dev, struct device_node *np,
> +				  const struct eqc_clock *clk, void __iomem *base,
> +				  struct clk_hw_onecell_data *cells)
> +{
> +	struct clk_parent_data parent_data;
> +	struct clk_hw *hw;
> +	int ret;
> +
> +	ret = eqc_fill_parent_data(clk, cells, &parent_data);
> +	if (ret)
> +		return ret;
> +
> +	hw = clk_hw_register_fixed_factor_pdata(dev, np, clk->name, &parent_data, 0,
> +						clk->ff.mult, clk->ff.div, 0, 0);
> +	if (IS_ERR(hw))
> +		return IS_ERR(hw);

Similar to the divider probe function above, does this return a boolean 1
instead of the negative error code?

> +
> +	cells->hws[clk->index] = hw;
> +	return 0;
> +}
> +
> +static int eqc_probe_pll_fracg(struct device *dev, struct device_node *np,
> +			       const struct eqc_clock *clk, void __iomem *base,
> +			       struct clk_hw_onecell_data *cells)
> +{
> +	struct clk_parent_data parent_data;
> +	unsigned long mult, div, acc;
> +	struct clk_hw *hw;
> +	int ret;
> +
> +	ret = eqc_pll_parse_fracg(base + clk->pll.reg, &mult, &div, &acc);
> +	if (ret)
> +		return ret;
> +
> +	ret = eqc_fill_parent_data(clk, cells, &parent_data);
> +	if (ret)
> +		return ret;
> +
> +	hw = clk_hw_register_fixed_factor_pdata(dev, np, clk->name, &parent_data, 0, mult,
> +						div, acc, CLK_FIXED_FACTOR_FIXED_ACCURACY);
> +	if (IS_ERR(hw))
> +		return IS_ERR(hw);

Does this also need to return PTR_ERR(hw) instead of IS_ERR(hw) to
propagate the correct error code?

> +
> +	cells->hws[clk->index] = hw;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-clk-eyeq7-v6-0-0540cce18fb2@bootlin.com?part=6