Re: [PATCH v4 4/4] clk: eyeq: add driver

["Théo Lebrun" <theo.lebrun@bootlin.com>]

On Fri Oct 4, 2024 at 11:20 PM CEST, Christophe JAILLET wrote:
> Le 04/10/2024 à 18:55, Théo Lebrun a écrit :
> > On Fri Oct 4, 2024 at 6:34 PM CEST, Christophe JAILLET wrote:
> >> Le 04/10/2024 à 17:45, Théo Lebrun a écrit :
> >>> +static void eqc_probe_init_plls(struct device *dev, struct eqc_priv *priv)
> >>> +{
> >>> +	const struct eqc_match_data *data = priv->data;
> >>> +	unsigned long mult, div, acc;
> >>> +	const struct eqc_pll *pll;
> >>> +	struct clk_hw *hw;
> >>> +	unsigned int i;
> >>> +	u32 r0, r1;
> >>> +	u64 val;
> >>> +	int ret;
> >>> +
> >>> +	for (i = 0; i < data->pll_count; i++) {
> >>> +		pll = &data->plls[i];
> >>> +
> >>> +		val = readq(priv->base + pll->reg64);
> >>> +		r0 = val;
> >>> +		r1 = val >> 32;
> >>> +
> >>> +		ret = eqc_pll_parse_registers(r0, r1, &mult, &div, &acc);
> >>> +		if (ret) {
> >>> +			dev_warn(dev, "failed parsing state of %s\n", pll->name);
> >>> +			priv->cells->hws[pll->index] = ERR_PTR(ret);
> >>> +			continue;
> >>> +		}
> >>> +
> >>> +		hw = clk_hw_register_fixed_factor_with_accuracy_fwname(dev,
> >>> +				dev->of_node, pll->name, "ref", 0, mult, div, acc);
> >>
> >> Should this be freed somewhere or is it auto-magically freed by a
> >> put_something()?
> >> Maybe devm_action_or_reset()?
> > 
> > This driver does not support being removed. It provides essential PLLs
> > and the system has not chance of working without them.
> > 
> > Almost all instances will be instantiated at of_clk_init() stage by the
> > way (ie before platform bus infrastructure init). Devres isn't a
> > solution in those cases.
>
> eqc_probe_init_plls() and eqc_probe_init_divs() are called from 
> eqc_probe(), which has several devm_ function calls.
>
> Would it make sense to remove these devm_ ?
>
>
> devm_platform_ioremap_resource(),
> devm_kzalloc(),
> devm_of_clk_add_hw_provider(),
> eqc_auxdev_create() which calls devm_add_action_or_reset().
>
> I sent this patch because of these calls.
>
> Either I miss something, either maybe things can be simplified.

You are right, mixing devres and non-devres handled resources was a
mistake. Things have been simplified in revision v5 [0]. It sets
suppress_bind_attrs to true and switches to 100% non-devres calls.

[0]: https://lore.kernel.org/lkml/20241007-mbly-clk-v5-0-e9d8994269cb@bootlin.com/

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com