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

[Brian Masney]

Hi Christian,

> In preparation for support of .set_rate, convert the clock register
> logic from fixed clock implementation to full clk_hw implementation with
> dedicated OPs for Airoha EN7581 SoC.
> 
> This is just a rework and no behaviour change is expected.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@xxxxxxxxx>
> ---
>  drivers/clk/clk-en7523.c | 86 +++++++++++++++++++++++++++++-----------
>  1 file changed, 62 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> index 24fdec2e6198..aab4306ae037 100644
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
> @@ -126,6 +126,12 @@ struct en_clk_gate {
>  	struct clk_hw hw;
>  };
>  
> +struct en_clk {
> +	struct regmap *map;
> +	const struct en_clk_desc *desc;
> +	struct clk_hw hw;
> +};
> +
>  struct en_rst_data {
>  	const u16 *bank_ofs;
>  	const u16 *idx_map;
> @@ -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 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);

Use dev_err() and elsewhere in this driver since a struct dev is
available here.

> +		return 0;
> +	}
> +	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 0;
> +	}
> +
> +	return rate / en7523_get_div(desc, val);
> +}
> +
> +static const struct clk_ops en7581_clk_ops = {
> +	.recalc_rate = en7581_recalc_rate,
> +};
> +
> +static int en7581_register_clocks(struct device *dev, struct clk_hw_onecell_data *clk_data,
> +				  struct regmap *map, void __iomem *base)
>  {
>  	struct clk_hw *hw;
> -	u32 rate;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(en7581_base_clks); i++) {
>  		const struct en_clk_desc *desc = &en7581_base_clks[i];
> -		u32 val, reg = desc->div_reg ? desc->div_reg : desc->base_reg;
> +		struct clk_init_data init = {
> +			.ops = &en7581_clk_ops,
> +		};
> +		struct en_clk *en_clk;
>  		int err;
>  
> -		err = regmap_read(map, desc->base_reg, &val);
> +		en_clk = devm_kzalloc(dev, sizeof(*en_clk), GFP_KERNEL);

devm_kzalloc_obj() is now available in the tree.

> +		if (!en_clk)
> +			return -ENOMEM;
> +
> +		init.name = desc->name;
> +		en_clk->map = map;
> +		en_clk->desc = desc;
> +		en_clk->hw.init = &init;
> +
> +		err = devm_clk_hw_register(dev, &en_clk->hw);
>  		if (err) {
> -			pr_err("Failed reading fixed clk rate %s: %d\n",
> +			pr_err("Failed to register clk %s: %d\n",

Use dev_err_probe()

>  			       desc->name, err);
> -			continue;
> -		}
> -		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);
> -			continue;
> -		}
> -		rate /= en7523_get_div(desc, val);
> -
> -		hw = clk_hw_register_fixed_rate(dev, desc->name, NULL, 0, rate);
> -		if (IS_ERR(hw)) {
> -			pr_err("Failed to register clk %s: %ld\n",
> -			       desc->name, PTR_ERR(hw));
> -			continue;
> +			return err;

Prior to this patch, a registration failure was logged, and the other
clocks were tried. Now the probe fails. This is fine, however the commit
message says there is no behavior change. This change should be called
out in the commit message.

Brian

-- 
Brian Masney <masneyb@xxxxxxxxxx>