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
