Re: [PATCH v9 2/2] clk: microchip: Add driver for Microchip PolarFire SoC

[<conor.dooley@xxxxxxxxxxxxx>]

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Quoting conor.dooley@xxxxxxxxxxxxx (2021-12-16 06:00:22)
> > diff --git a/drivers/clk/microchip/Makefile b/drivers/clk/microchip/Makefile
> > index f34b247e870f..0dce0b12eac4 100644
> > --- a/drivers/clk/microchip/Makefile
> > +++ b/drivers/clk/microchip/Makefile

Snipping the rest, will/have addressed them.

> > +static int mpfs_clk_register_cfgs(struct device *dev, struct mpfs_cfg_hw_clock *cfg_hws,
> > +                                 unsigned int num_clks, struct mpfs_clock_data *data,
> > +                                 struct clk *clk_parent)
> > +{
> > +       struct clk_hw *hw;
> > +       void __iomem *sys_base = data->base;
> > +       unsigned int i, id;
> > +
> > +       for (i = 0; i < num_clks; i++) {
> > +               struct mpfs_cfg_hw_clock *cfg_hw = &cfg_hws[i];
> > +
> > +               cfg_hw->cfg.parent = __clk_get_hw(clk_parent);
> > +               cfg_hw->hw.init = CLK_HW_INIT_HW(cfg_hw->cfg.name, cfg_hw->cfg.parent,
> > +                                                &mpfs_clk_cfg_ops, cfg_hw->cfg.flags);
> > +               hw = mpfs_clk_register_cfg(dev, cfg_hw, sys_base);
> > +               if (IS_ERR(hw)) {
> > +                       dev_err(dev, "failed to register clock %s\n", cfg_hw->cfg.name);
> > +                       goto err_clk;
> > +               }
> > +
> > +               id = cfg_hws[i].cfg.id;
> > +               data->hw_data.hws[id] = hw;
> > +       }
> > +
> > +       return 0;
> > +
> > +err_clk:
> > +       while (i--)
> > +               devm_clk_hw_unregister(dev, data->hw_data.hws[cfg_hws[i].cfg.id]);
> 
> > +       clk_parent = devm_clk_get(dev, NULL);
> 
> Use clk_parent_data instead please.
> 
> > +       if (IS_ERR(clk_parent))
> > +               return PTR_ERR(clk_parent);


Please correct me if I am misinterpreting:
I had the devm_clk_get() in there to pickup the refclk from the device
tree as a result of previous feedback. I have replaced this with the
following, which I have found in several other drivers - does it achieve
the same thing?
If it does, all of the args to CLK_HW_INIT_PARENTS_DATA are now set at
compile time & I will take CLK_HW_INIT_PARENTS_DATA back out of this
function.

static struct clk_parent_data mpfs_cfg_parent[] = {
	{ .index = 0 },
};

static int mpfs_clk_register_cfgs(struct device *dev, struct mpfs_cfg_hw_clock *cfg_hws,
				  unsigned int num_clks, struct mpfs_clock_data *data)
{
	void __iomem *sys_base = data->base;
	unsigned int i, id;
	int ret;

	for (i = 0; i < num_clks; i++) {
		struct mpfs_cfg_hw_clock *cfg_hw = &cfg_hws[i];

		cfg_hw->hw.init = CLK_HW_INIT_PARENTS_DATA(cfg_hw->cfg.name, mpfs_cfg_parent,
						 &mpfs_clk_cfg_ops, cfg_hw->cfg.flags);

		ret = mpfs_clk_register_cfg(dev, cfg_hw, sys_base);
		if (ret) {
			dev_err_probe(dev, ret, "failed to register clock %s\n",
				      cfg_hw->cfg.name);
			return ret;
		}

		id = cfg_hws[i].cfg.id;
		data->hw_data.hws[id] = &cfg_hw->hw;
	}

	return 0;
}