Re: [PATCH 1/2] clk: en7523: convert to full clk_hw implementation for en7581

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] clk: en7523: negative error returned as unsigned rate in en7581_recalc_rate()
--

commit 3b91713eaffa8e4eb9f5cbbb86a0f1a541e129cc
Author: Christian Marangi <ansuelsmth@gmail.com>

clk: en7523: convert to full clk_hw implementation for en7581

This commit converts the Airoha EN7581 clock registration in
drivers/clk/clk-en7523.c from fixed-rate clocks to a full clk_hw
implementation with a dedicated .recalc_rate callback in preparation for
adding .set_rate support.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 24fdec2e6198c..2f203c94f87e9 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c

[ ... ]

> @@ -742,46 +748,76 @@ static int en7523_clk_hw_init(struct platform_device *pdev,
>  				     en7581_rst_ofs);
>  }
>  
> -static void en7581_register_clocks(struct device *dev, struct clk_hw_onecell_data *clk_data,
> -				   struct regmap *map, void __iomem *base)
> +static unsigned long en7581_recalc_rate(struct clk_hw *hw,
> +					unsigned long parent_rate)
>  {
> -	struct clk_hw *hw;
> +	struct en_clk *c = container_of(hw, struct en_clk, hw);
> +	const struct en_clk_desc *desc = c->desc;
> +	struct regmap *map = c->map;
> +	u32 val, reg;
>  	u32 rate;
> +	int err;
> +
> +	err = regmap_read(map, desc->base_reg, &val);
> +	if (err) {
> +		pr_err("Failed reading fixed clk rate %s: %d\n",
> +		       desc->name, err);
> +		return err;
> +	}
> +	rate = en7523_get_base_rate(desc, val);
> +
> +	reg = desc->div_reg ? desc->div_reg : desc->base_reg;
> +	err = regmap_read(map, reg, &val);
> +	if (err) {
> +		pr_err("Failed reading fixed clk div %s: %d\n",
> +		       desc->name, err);
> +		return err;
> +	}

[Severity: Medium]
In drivers/clk/clk-en7523.c, when either regmap_read() call on desc->base_reg
or reg fails in en7581_recalc_rate(), returning the negative int err from an
unsigned long .recalc_rate callback converts it into a large positive value
instead of 0.

Per struct clk_ops in include/linux/clk-provider.h, .recalc_rate must return 0
if the driver cannot figure out a rate for the clock, as callers such as
__clk_core_init() and clk_recalc() in drivers/clk/clk.c store the returned
unsigned long directly into core->rate.

Should both error paths in en7581_recalc_rate() return 0 instead of err?

> +
> +	return rate / en7523_get_div(desc, val);
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261022205535.688352-1-ansuelsmth@gmail.com?part=1