Re: [PATCH v3 2/5] clk: en7523: generalize register clocks function

[Christian Marangi <ansuelsmth@gmail.com>]

On Thu, Nov 06, 2025 at 09:25:23PM +0100, Christophe JAILLET wrote:
> Le 06/11/2025 à 20:59, Christian Marangi a écrit :
> > Generalize register clocks function for Airoha EN7523 and EN7581 clocks
> > driver. The same logic is applied for both clock hence code can be
> > reduced and simplified by putting the base_clocks struct in the soc_data
> > and passing that to a generic register clocks function.
> > 
> > While at it rework some function to return error and use devm variant
> > for clk_hw_regiser.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >   drivers/clk/clk-en7523.c | 148 +++++++++++++++++----------------------
> >   1 file changed, 66 insertions(+), 82 deletions(-)
> 
> ...
> 
> > +static int en75xx_register_clocks(struct device *dev,
> > +				  const struct en_clk_soc_data *soc_data,
> > +				  struct clk_hw_onecell_data *clk_data,
> > +				  struct regmap *map, struct regmap *clk_map)
> > +{
> > +	struct clk_hw *hw;
> > +	u32 rate;
> > +	int i;
> > +
> > +	for (i = 0; i < soc_data->num_clocks - 1; i++) {
> > +		const struct en_clk_desc *desc = &soc_data->base_clks[i];
> > +		u32 val, reg = desc->div_reg ? desc->div_reg : desc->base_reg;
> > +		int err;
> > +
> > +		err = regmap_read(map, desc->base_reg, &val);
> > +		if (err) {
> > +			pr_err("Failed reading fixed clk rate %s: %d\n",
> 
> Would it be better to use dev_err()? (here and in other places)
>

Yes but I wanted to limit the changes. Is it possible to do it later?

> > +			       desc->name, err);
> > +			return err;
> > +		}
> > +		rate = en7523_get_base_rate(desc, val);
> > +
> > +		err = regmap_read(map, reg, &val);
> > +		if (err) {
> > +			pr_err("Failed reading fixed clk div %s: %d\n",
> > +			       desc->name, err);
> > +			return err;
> > +		}
> > +		rate /= en7523_get_div(desc, val);
> > +
> > +		hw = clk_hw_register_fixed_rate(dev, desc->name, NULL, 0, rate);
> 
> I think that the issue was already there before, but should we have a
> corresponding clk_hw_unregister_fixed_rate() somewhere in this driver?
> 
> I've not seen any.
> 
> Or use devm_clk_hw_register_fixed_rate()?
> 

Well yes, I didn't move to devm as it's already planned to move to full
clk with .set_rate and realtime .get_rate. Is it possible to also delay
this in a later series?

(thanks for the review)

> > +		if (IS_ERR(hw)) {
> > +			pr_err("Failed to register clk %s: %ld\n",
> > +			       desc->name, PTR_ERR(hw));
> > +			return PTR_ERR(hw);
> > +		}
> > +
> > +		clk_data->hws[desc->id] = hw;
> > +	}
> > +
> > +	hw = en7523_register_pcie_clk(dev, clk_map);
> > +	if (IS_ERR(hw))
> > +		return PTR_ERR(hw);
> > +
> > +	clk_data->hws[EN7523_CLK_PCIE] = hw;
> > +
> > +	return 0;
> > +}
> > +
> >   static int en7581_pci_is_enabled(struct clk_hw *hw)
> >   {
> >   	struct en_clk_gate *cg = container_of(hw, struct en_clk_gate, hw);
> 
> ...
> 
> CJ

-- 
	Ansuel