[PATCH v5 04/23] clk: starfive: jh71x0: Add generic CRG probe helper

[Changhuang Liang]

Hi, Brain

Thanks for the review.

> Hi Changhuang,
> 
> > The StarFive JHB100 SoC has several clock and reset generators (CRGs)
> > which share the same probe flow: map the registers, register all the
> > clocks described by a table, add the clock provider and register the
> > auxiliary reset device.
> >
> > Add jh71x0_crg_probe() together with struct jh71x0_crg_domain_info so
> > that each CRG driver only needs to provide its clock table, external
> > clock names and reset auxiliary device name. The reset auxiliary
> > device is registered with jh71x0_reset_controller_register().
> >
> > The clock index space of these CRGs is sparse, so also make
> > jh71x0_clk_get() reject indexes that were never registered when the
> > domain info is available.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang at starfivetech.com>
> >
> > diff --git a/drivers/clk/starfive/clk-starfive-jh71x0.c
> > b/drivers/clk/starfive/clk-starfive-jh71x0.c
> > index 2cc71f49ddee..bb7706ede1a6 100644
> > --- a/drivers/clk/starfive/clk-starfive-jh71x0.c
> > +++ b/drivers/clk/starfive/clk-starfive-jh71x0.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/debugfs.h>
> >  #include <linux/device.h>
> >  #include <linux/io.h>
> > +#include <linux/pm_runtime.h>
> 
> Add #include <linux/property.h> for device_get_match_data()
> 
> >  #include <linux/slab.h>
> >  #include <soc/starfive/reset-starfive-jh71x0.h>
> >
> > @@ -334,10 +335,14 @@ struct clk_hw *jh71x0_clk_get(struct
> of_phandle_args *clkspec, void *data)
> >  	struct jh71x0_clk_priv *priv = data;
> >  	unsigned int idx = clkspec->args[0];
> >
> > -	if (idx < priv->num_reg)
> > -		return &priv->reg[idx].hw;
> > +	if (idx >= priv->num_reg)
> > +		return ERR_PTR(-EINVAL);
> >
> > -	return ERR_PTR(-EINVAL);
> > +	/* Index space is sparse: reject holes that were never registered. */
> > +	if (priv->info && !priv->info->clk_data[idx].name)
> > +		return ERR_PTR(-ENOENT);
> > +
> > +	return &priv->reg[idx].hw;
> >  }
> >  EXPORT_SYMBOL_GPL(jh71x0_clk_get);
> >
> > @@ -391,3 +396,87 @@ int jh71x0_reset_controller_register(struct
> jh71x0_clk_priv *priv,
> >  					jh71x0_reset_unregister_adev, adev);  }
> > EXPORT_SYMBOL_GPL(jh71x0_reset_controller_register);
> > +
> > +int jh71x0_crg_probe(struct platform_device *pdev) {
> > +	const struct jh71x0_crg_domain_info *info;
> > +	struct jh71x0_clk_priv *priv;
> > +	unsigned int idx;
> > +	int ret;
> > +
> > +	info = device_get_match_data(&pdev->dev);
> > +	if (!info)
> > +		return -ENODEV;
> > +
> > +	priv = devm_kzalloc(&pdev->dev, struct_size(priv, reg, info->num_clk),
> > +			    GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	spin_lock_init(&priv->rmw_lock);
> > +	priv->info = info;
> > +	priv->num_reg = info->num_clk;
> > +	priv->dev = &pdev->dev;
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	if (info->power_domain) {
> > +		ret = devm_pm_runtime_enable(priv->dev);
> > +		if (ret)
> > +			return dev_err_probe(priv->dev, ret,
> > +					     "failed to enable runtime PM\n");
> > +	}
> > +
> > +	for (idx = 0; idx < info->num_clk; idx++) {
> > +		u32 max = info->clk_data[idx].max;
> > +		struct clk_parent_data parents[4] = {};
> > +		struct clk_init_data init = {
> > +			.name = info->clk_data[idx].name,
> > +			.ops = starfive_jh71x0_clk_ops(max),
> > +			.parent_data = parents,
> > +			.num_parents =
> > +				((max & JH71X0_CLK_MUX_MASK) >>
> JH71X0_CLK_MUX_SHIFT) + 1,
> > +			.flags = info->clk_data[idx].flags,
> > +		};
> > +		struct jh71x0_clk *clk = &priv->reg[idx];
> > +		unsigned int i;
> > +
> > +		if (!init.name)
> > +			continue;
> > +
> > +		if (init.num_parents > ARRAY_SIZE(parents))
> > +			return dev_err_probe(priv->dev, -EINVAL,
> > +					     "clock %s: too many parents (%u > %zu)\n",
> > +					     init.name, init.num_parents,
> ARRAY_SIZE(parents));
> > +
> > +		for (i = 0; i < init.num_parents; i++) {
> > +			unsigned int pidx = info->clk_data[idx].parents[i];
> > +
> > +			if (pidx < info->num_clk) {
> > +				parents[i].hw = &priv->reg[pidx].hw;
> > +			} else {
> > +				if (pidx - info->num_clk >= info->num_ext_clk)
> > +					return -EINVAL;
> 
> What do you think about adding a dev_err_probe() here as well to make it
> easier to troubleshoot any failures here?

OK, I'll add it.

Best Regards,
Changhuang