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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <bc9074b9-27b5-4a31-ab85-ef7fcc309523@wanadoo.fr>
Date: Thu, 6 Nov 2025 21:25:23 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Michael Turquette <mturquette@...libre.com>,
 Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>,
 Felix Fietkau <nbd@....name>, linux-clk@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/5] clk: en7523: generalize register clocks function

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@...il.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)

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