Quoting conor.dooley@xxxxxxxxxxxxx (2022-01-25 05:40:11)
> > 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 },
> };
Yes this should be sufficient. Make it const though.
>
> 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;
> }
Looks good. Thanks.
