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

[Stephen Boyd <sboyd@kernel.org>]

Quoting conor.dooley@microchip.com (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@microchip.com (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.