Re: [PATCH v16 1/1] clk: npcm8xx: add clock controller

[Christophe JAILLET]

Le 22/05/2023 à 14:56, Tomer Maimon a écrit :
> Hi Christophe,
>
> Thanks for your comments

[...]

> > > +static struct clk_hw *
> > > +npcm8xx_clk_register_pll(struct device *dev, void __iomem *pllcon,
> > > +                      const char *name, const struct clk_parent_data *parent,
> > > +                      unsigned long flags)
> > > +{
> > > +     struct npcm8xx_clk_pll *pll;
> > > +     struct clk_init_data init = {};
> > > +     int ret;
> > > +
> > > +     pll = kzalloc(sizeof(*pll), GFP_KERNEL);
> >
> > Everything looks devm_()'ed in this driver, except this kzalloc.
> > Except the one below, there is no kfree to free this memory, and no
> > .remove() function.
> Also  clk_hw_register_divider_parent_data doesn't use devm_
> about free the pll, we use it, return at the end of the function.
> about adding remove, we had a dissection about it in V4, since the
> clock is a service driver it shouldn't be removed.
> https://patchwork.kernel.org/project/linux-watchdog/patch/20220621131424.162355-7-tmaimon77@xxxxxxxxx/

LoL.
At least, I'm consistent :).

Just to make it clear, what I mean about kfree() is not to add one here, 
but either:
   - to use devm_kzalloc() here, to avoid a leak, should loading the 
driver fails      OR
   - have some kfree() where needed (at least in the error handling 
path of the probe, if the remove function makes no point)

CJ