Re: [PATCH v3 01/20] clk: shmobile: r8a7779: Add clocks support

[Wolfram Sang <wsa@the-dreams.de>]

[-- Attachment #1: Type: text/plain, Size: 1316 bytes --]

On Sun, Mar 02, 2014 at 12:35:50PM +0100, Laurent Pinchart wrote:
> Hi Wolfram,
> 
> On Saturday 01 March 2014 14:50:03 Wolfram Sang wrote:
> > > +	cpg = kzalloc(sizeof(*cpg), GFP_KERNEL);
> > > +	clks = kzalloc(CPG_NUM_CLOCKS * sizeof(*clks), GFP_KERNEL);
> > > +	if (cpg == NULL || clks == NULL) {
> > > +		/* We're leaking memory on purpose, there's no point in cleaning
> > > +		 * up as the system won't boot anyway.
> > > +		 */
> > > +		pr_err("%s: failed to allocate cpg\n", __func__);
> > > +		return;
> > > +	}
> > 
> > I have problems with this sloppiness. Writing the comment took probably
> > the same time as writing the proper exit path :) On the drawback side,
> > static code checkers will keep reporting this.
> 
> Then static code checkers should be fixed :-)

How should they know if you 'return'? :) They will know if you BUG().
Which might be the better (read: more descriptive) option which would
save the comment, too.

> > > +	cpg->reg = of_iomap(np, 0);
> > > +	if (WARN_ON(cpg->reg == NULL))
> > > +		return;
> > 
> > if (WARN(!cpg->reg, "can't ioremap CPG\n"))
> > 
> > ?
> 
> We can probably remove this altogether given that the driver doesn't make use 
> of the ioremap'ed memory.

Okay. Was still a good place to advertise WARN() over WARN_ON ;)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]