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

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:

quoted

clocks need to get prepared before they can get enabled,
fix the MPC512x PSC SPI master's initialization

quoted

Signed-off-by: Gerhard Sittig <redacted>
---
 drivers/spi/spi-mpc512x-psc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-mpc512x-psc.c b/drivers/spi/spi-mpc512x-psc.c
index 29fce6a..76b20ea 100644
--- a/drivers/spi/spi-mpc512x-psc.c
+++ b/drivers/spi/spi-mpc512x-psc.c

@@ -395,7 +395,7 @@ static int mpc512x_psc_spi_port_config(struct spi_master *master,
 
 	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.

What the above patch addresses is prevention of an immediate and
fatal breakage, when common clock gets used but clocks aren't
prepared before getting enabled.  So I consider it appropriate as
a step in preparation before introducing support for common clock
on the platform.  (It's funny that I had a comment in this spirit
in the commit message, but trimmed it to not be overly verbose.)

What the patch does not address is to fix any other deficiencies
in the driver which might have been lurking there for ages.  This
would be the scope of a different patch or series, as addressing
it here as well would mix orthogonal issues within one series,
and would complicate review and test (and would delay or even
potentially kill the introduction of desirable support for common
infrastructure just because other legacy non-fatal issues aren't
addressed as well in bypassing).

I will look into what I can do to address your concerns about
proper general clock handling in this specific driver.  But I'm
unable to do this for all other drivers as well which I happen to
pass by as I work on platform code (partially due to lack of
knowledge).  In any case I won't be able to test all of these
changes in all subsystems (mostly due to lack of hardware and
test setups).

So I suggest to leave these general cleanup activities for a
separate series.  The current series certainly improves general
platform code, transparently brings new drivers into successful
operation, and keeps the quality of existing code (doesn't break
anything, does even actively prevent breakage).  So it shall be
acceptable despite its not being perfect (like addressing each
and every other aspect which may arise elsewhere).  Where would I
stop then?

Sorry for the lengthy reply, but I guess it's about just one
general aspect which equally applies to other parts of the
series, not just the specific SPI driver which the feedback was
provided for.  And I do appreciate your looking at the
submission.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

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:

quoted

On Mon, Jul 15, 2013 at 08:47:30PM +0200, Gerhard Sittig wrote:

quoted

quoted

  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);

quoted

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.