Messages in this thread | | | From | Daniel Palmer <> | Date | Sun, 20 Dec 2020 15:42:40 +0900 | Subject | Re: [PATCH 3/6] clk: mstar: MStar/SigmaStar MPLL driver |
| |
Hi Stephen,
On Sun, 20 Dec 2020 at 13:36, Stephen Boyd <sboyd@kernel.org> 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
| |