[PATCH v1 01/24] spi: mpc512x: prepare clocks before enabling them

[Mark Brown]

On Wed, Jul 17, 2013 at 01:22:29PM +0200, Gerhard Sittig wrote:
> On Mon, Jul 15, 2013 at 21:17 +0100, Mark Brown wrote:
> > On Mon, Jul 15, 2013 at 08:47:30PM +0200, Gerhard Sittig wrote:

> > >   sprintf(name, "psc%d_mclk", master->bus_num);
> > >  	spiclk = clk_get(&master->dev, name);
> > > -	clk_enable(spiclk);
> > > +	clk_prepare_enable(spiclk);
> > >  	mps->mclk = clk_get_rate(spiclk);
> > >  	clk_put(spiclk);

> > This code is *clearly* buggy and should be fixed rather than papered
> > over.  Not only is there no matching put the driver is also dropping the
> > reference to the clock rather than holding on to it while it's in use.

> "This code" refers to the driver's original condition, right?  I
> agree that the driver has been suffering from incomplete clock
> handling since it was born three years ago.  And that this issue
> shall get addressed.  The question is just whether it needs to be
> part of this series which has a different focus.

This is a pretty long e-mail.  It'd probably have taken less time to
fix the issues than to reply to the e-mail...  but anyway.

A big part of the issue with the state of the driver is that there's
obvious clock API abuse going on that isn't corrected here - the main
one is that the sprintf() for the clock name is a fairly clear warning
sign, the driver should be using a fixed value there.  This raises a
warning flag for me about the quality of the common clock API
implementation that's being sent and/or the potential for bugs to be
noticed with the common clock framework.  I'd not expect this code to
actually work, and looking at the rest of the series I don't see how it
does since I can't see what forces the name.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.ozlabs.org/pipermail/devicetree-discuss/attachments/20130717/245057e2/attachment.sig>