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.
signature.asc
Description: Digital signature
_______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
