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 <[email protected]> > > > > 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
