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

[Christophe JAILLET]

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@xxxxxxxxx>
> ---
>   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)

> +			       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()?

> +		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