Re: [PATCH 3/6] clk: mstar: MStar/SigmaStar MPLL driver

[Daniel Palmer]

Hi Stephen,

On Sun, 20 Dec 2020 at 13:36, Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index da8fcf147eb1..b758aae17ab8 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -124,3 +124,4 @@ endif
> >  obj-$(CONFIG_ARCH_ZX)                  += zte/
> >  obj-$(CONFIG_ARCH_ZYNQ)                        += zynq/
> >  obj-$(CONFIG_COMMON_CLK_ZYNQMP)         += zynqmp/
> > +obj-$(CONFIG_ARCH_MSTARV7)             += mstar/
>
> This is in the wrong place. It looks to be sorted based on the path
> name, so mstar/ comes much earlier in this file.

Noted. I'll fix this.

> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/of.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/clkdev.h>
>
> Please remove unused includes
>
> > +#include <linux/of_address.h>
> > +#include <linux/module.h>
>
> Isn't it builtin? This include should be removed.

Yes. Sorry it was originally a module and there are some leftovers I
didn't clean up when I changed it to builtin.

> > +#define to_mpll(_hw) container_of(_hw, struct msc313_mpll, clk_hw)
> > +#define to_divider_hw(_mpll, _which) _mpll->clk_data->hws[_which + 1]
>
> I'd rather not have this macro. It's confusing given that to_foo()
> macros are usually a container_of() invocation. Given that it's only
> used in the registration/unregistration loops please inline it and use a
> local variable.
>

Ok I'll rework that.

> > +
> > +static int msc313_mpll_remove(struct platform_device *pdev)
> > +{
> > +       struct msc313_mpll *mpll = platform_get_drvdata(pdev);
> > +       int i;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(output_dividers); i++)
> > +               clk_hw_unregister_fixed_factor(to_divider_hw(mpll, i));
>
> Maybe add a devm_ for this if it doesn't exist.

I did think about adding this. Would I need to do that in a separate
series or would it be ok to roll it into this one?

Thanks,

Daniel