[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Kevin Wells]

Remove suspend/resume functionality, I2C driver gates clock on
only when an I2C transaction is in progress

Signed-off-by: Kevin Wells <kevin.wells@nxp.com>
---
 drivers/i2c/busses/i2c-pnx.c |   35 +++++++----------------------------
 1 files changed, 7 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-pnx.c b/drivers/i2c/busses/i2c-pnx.c
index cd4805d..c3c5425 100644
--- a/drivers/i2c/busses/i2c-pnx.c
+++ b/drivers/i2c/busses/i2c-pnx.c
@@ -453,6 +453,8 @@ i2c_pnx_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 	dev_dbg(&adap->dev, "%s(): entering: %d messages, stat = %04x.\n",
 		__func__, num, ioread32(I2C_REG_STS(alg_data)));
 
+	clk_enable(alg_data->clk);
+
 	bus_reset_if_active(adap);
 
 	/* Process transactions in a loop. */
@@ -521,6 +523,8 @@ i2c_pnx_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 
 	bus_reset_if_active(adap);
 
+	clk_disable(alg_data->clk);
+
 	/* Cleanup to be sure... */
 	alg_data->mif.buf = NULL;
 	alg_data->mif.len = 0;
@@ -544,31 +548,6 @@ static struct i2c_algorithm pnx_algorithm = {
 	.functionality = i2c_pnx_func,
 };
 
-#ifdef CONFIG_PM
-static int i2c_pnx_controller_suspend(struct platform_device *pdev,
-				      pm_message_t state)
-{
-	struct i2c_pnx_data *i2c_pnx = platform_get_drvdata(pdev);
-	struct i2c_pnx_algo_data *alg_data = i2c_pnx->adapter->algo_data;
-
-	/* FIXME: shouldn't this be clk_disable? */
-	clk_enable(alg_data->clk);
-
-	return 0;
-}
-
-static int i2c_pnx_controller_resume(struct platform_device *pdev)
-{
-	struct i2c_pnx_data *i2c_pnx = platform_get_drvdata(pdev);
-	struct i2c_pnx_algo_data *alg_data = i2c_pnx->adapter->algo_data;
-
-	return clk_enable(alg_data->clk);
-}
-#else
-#define i2c_pnx_controller_suspend	NULL
-#define i2c_pnx_controller_resume	NULL
-#endif
-
 static int __devinit i2c_pnx_probe(struct platform_device *pdev)
 {
 	unsigned long tmp;
@@ -664,6 +643,9 @@ static int __devinit i2c_pnx_probe(struct platform_device *pdev)
 	dev_dbg(&pdev->dev, "%s: Master at %#8x, irq %d.\n",
 	       i2c_pnx->adapter->name, alg_data->base, alg_data->irq);
 
+	/* Disable clock until needed */
+	clk_disable(alg_data->clk);
+
 	return 0;
 
 out_irq:
@@ -690,7 +672,6 @@ static int __devexit i2c_pnx_remove(struct platform_device *pdev)
 
 	free_irq(alg_data->irq, alg_data);
 	i2c_del_adapter(adap);
-	clk_disable(alg_data->clk);
 	iounmap((void *)alg_data->ioaddr);
 	release_mem_region(alg_data->base, I2C_PNX_REGION_SIZE);
 	clk_put(alg_data->clk);
@@ -706,8 +687,6 @@ static struct platform_driver i2c_pnx_driver = {
 	},
 	.probe = i2c_pnx_probe,
 	.remove = __devexit_p(i2c_pnx_remove),
-	.suspend = i2c_pnx_controller_suspend,
-	.resume = i2c_pnx_controller_resume,
 };
 
 static int __init i2c_adap_pnx_init(void)
-- 
1.6.0.6

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Uwe Kleine-König]

Hello,

can you please add something like "i2c-pnx: " to the subject?
(Actually it's a great strategy *not* to put it into the Subject.  This
way it attracts far more attention :-)

> Remove suspend/resume functionality, I2C driver gates clock on
> only when an I2C transaction is in progress
What happens when the machine suspends while a transfer is in progress?
(This might be a problem that already existed before.)  If this is
really a problem the easiest "fix" is to let the suspend callback return
-EBUSY in this case.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Russell King - ARM Linux]

On Thu, Nov 26, 2009 at 10:02:52AM +0100, Uwe Kleine-K?nig wrote:
> Hello,
> 
> can you please add something like "i2c-pnx: " to the subject?
> (Actually it's a great strategy *not* to put it into the Subject.  This
> way it attracts far more attention :-)
> 
> > Remove suspend/resume functionality, I2C driver gates clock on
> > only when an I2C transaction is in progress
> What happens when the machine suspends while a transfer is in progress?
> (This might be a problem that already existed before.)  If this is
> really a problem the easiest "fix" is to let the suspend callback return
> -EBUSY in this case.

If it's a short-lived event (i2c transfers tend to be) the best thing is
to wait for the event to complete before continuing.  Most of the suspend
callbacks (except the noirq versions) are called in process context and
can sleep.

Returning -EBUSY means you abort the suspend entirely and it has to be
started again from the beginning.  Too bad if the suspend request was
because your battery was just ab............

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Kevin Wells]

> Subject: Re: [PATCH 1/1] Remove suspend/resume functionality, add dynamic
> clocking
> 
> Hello,
> 
> can you please add something like "i2c-pnx: " to the subject?
> (Actually it's a great strategy *not* to put it into the Subject.  This
> way it attracts far more attention :-)
> 

Good point!

> > Remove suspend/resume functionality, I2C driver gates clock on
> > only when an I2C transaction is in progress
> What happens when the machine suspends while a transfer is in progress?
> (This might be a problem that already existed before.)  If this is
> really a problem the easiest "fix" is to let the suspend callback return
> -EBUSY in this case.

The suspend callback is now removed. It's actually not needed with this
change. The I2C clocks will turn on prior to a transaction and then turn
off at the completion.

> 
> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                              | Uwe Kleine-K?nig
> |
> Industrial Linux Solutions                    | http://www.pengutronix.de/
> |

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Russell King - ARM Linux]

On Wed, Nov 25, 2009 at 10:23:21PM +0100, Kevin Wells wrote:
> Remove suspend/resume functionality, I2C driver gates clock on
> only when an I2C transaction is in progress

Well, I'm happy with this (we have other drivers which don't do anything
spectacular in their suspend handling as well.)

Can you submit it to the patch system please?

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Kevin Wells]

> Can you submit it to the patch system please?

I'm still new to this and will probably be asking some stupid questions
for a while...I'll try to keep them short!

For the new LPC32XX mach area files, I was going to clean those up and
re-submit, but I have a number of smaller changes and enhancements for
existing drivers that I will try to submit over the next 3 to 6 months.

Besides submitting patches via emails, is there a another (more formal)
method for submitting them?

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Pavel Machek]

Hi!

> > Subject: Re: [PATCH 1/1] Remove suspend/resume functionality, add dynamic
> > clocking
> > 
> > Hello,
> > 
> > can you please add something like "i2c-pnx: " to the subject?
> > (Actually it's a great strategy *not* to put it into the Subject.  This
> > way it attracts far more attention :-)
> > 
> 
> Good point!
> 
> > > Remove suspend/resume functionality, I2C driver gates clock on
> > > only when an I2C transaction is in progress
> > What happens when the machine suspends while a transfer is in progress?
> > (This might be a problem that already existed before.)  If this is
> > really a problem the easiest "fix" is to let the suspend callback return
> > -EBUSY in this case.
> 
> The suspend callback is now removed. It's actually not needed with this
> change. The I2C clocks will turn on prior to a transaction and then turn
> off at the completion.

Are you sure its unneeded? What if someone attempts to suspend the
system when a transaction is running?
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Uwe Kleine-König]

Hi,

On Sun, Dec 06, 2009 at 09:47:31AM +0100, Pavel Machek wrote:
> Hi!
> 
> > > Subject: Re: [PATCH 1/1] Remove suspend/resume functionality, add dynamic
> > > clocking
> > > 
> > > Hello,
> > > 
> > > can you please add something like "i2c-pnx: " to the subject?
> > > (Actually it's a great strategy *not* to put it into the Subject.  This
> > > way it attracts far more attention :-)
> > > 
> > 
> > Good point!
> > 
> > > > Remove suspend/resume functionality, I2C driver gates clock on
> > > > only when an I2C transaction is in progress
> > > What happens when the machine suspends while a transfer is in progress?
> > > (This might be a problem that already existed before.)  If this is
> > > really a problem the easiest "fix" is to let the suspend callback return
> > > -EBUSY in this case.
> > 
> > The suspend callback is now removed. It's actually not needed with this
> > change. The I2C clocks will turn on prior to a transaction and then turn
> > off at the completion.
> 
> Are you sure its unneeded? What if someone attempts to suspend the
> system when a transaction is running?
That's exactly my question.  I think the machine will suspend and the
transaction fail.  So no suspend callback isn't optimal, but maybe OK?!

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Pavel Machek]

On Sun 2009-12-06 22:02:25, Uwe Kleine-K?nig wrote:
> Hi,
> 
> On Sun, Dec 06, 2009 at 09:47:31AM +0100, Pavel Machek wrote:
> > Hi!
> > 
> > > > Subject: Re: [PATCH 1/1] Remove suspend/resume functionality, add dynamic
> > > > clocking
> > > > 
> > > > Hello,
> > > > 
> > > > can you please add something like "i2c-pnx: " to the subject?
> > > > (Actually it's a great strategy *not* to put it into the Subject.  This
> > > > way it attracts far more attention :-)
> > > > 
> > > 
> > > Good point!
> > > 
> > > > > Remove suspend/resume functionality, I2C driver gates clock on
> > > > > only when an I2C transaction is in progress
> > > > What happens when the machine suspends while a transfer is in progress?
> > > > (This might be a problem that already existed before.)  If this is
> > > > really a problem the easiest "fix" is to let the suspend callback return
> > > > -EBUSY in this case.
> > > 
> > > The suspend callback is now removed. It's actually not needed with this
> > > change. The I2C clocks will turn on prior to a transaction and then turn
> > > off at the completion.
> > 
> > Are you sure its unneeded? What if someone attempts to suspend the
> > system when a transaction is running?
> That's exactly my question.  I think the machine will suspend and the
> transaction fail.  So no suspend callback isn't optimal, but maybe OK?!

Having failures just because suspend happened at wrong time is
bad. .suspend() should just wait for end of transaction.

										Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Kevin Wells]

> > Hi,
> >
> > On Sun, Dec 06, 2009 at 09:47:31AM +0100, Pavel Machek wrote:
> > > Hi!
> > >
> > > > > Subject: Re: [PATCH 1/1] Remove suspend/resume functionality, add
> dynamic
> > > > > clocking
> > > > >
> > > > > Hello,
> > > > >
> > > > > can you please add something like "i2c-pnx: " to the subject?
> > > > > (Actually it's a great strategy *not* to put it into the Subject.
> This
> > > > > way it attracts far more attention :-)
> > > > >
> > > >
> > > > Good point!
> > > >
> > > > > > Remove suspend/resume functionality, I2C driver gates clock on
> > > > > > only when an I2C transaction is in progress
> > > > > What happens when the machine suspends while a transfer is in
> progress?
> > > > > (This might be a problem that already existed before.)  If this is
> > > > > really a problem the easiest "fix" is to let the suspend callback
> return
> > > > > -EBUSY in this case.
> > > >
> > > > The suspend callback is now removed. It's actually not needed with
> this
> > > > change. The I2C clocks will turn on prior to a transaction and then
> turn
> > > > off at the completion.
> > >
> > > Are you sure its unneeded? What if someone attempts to suspend the
> > > system when a transaction is running?
> > That's exactly my question.  I think the machine will suspend and the
> > transaction fail.  So no suspend callback isn't optimal, but maybe OK?!
> 
> Having failures just because suspend happened at wrong time is
> bad. .suspend() should just wait for end of transaction.

I'm not sure what situation could actually occur that would suspend the
system in the middle of an I2C transaction. If an I2C transaction was
started and the CPU was suspended, this would appear to be a problem
outside the I2C driver itself. Would other I2C drivers have this similar
problem?

Other drivers that may use the I2C driver during their own suspend
sequence, the USB transceiver shutdown via I2C on this board is a good
example, will pend their own suspend return sequence until the I2C
transactions via I2C are complete. All the I2C driver does now is enable
the clock for the transaction and disable it at completion. This seemed
like the more logical way to go, it saves power when the I2C bus isn't
in use. Since all the suspend/resume functions do for the i2c-pnx
driver is disable and enable clocks, I didn't think the callbacks were
really needed anymore.

The few drivers that I've checked that use I2C in the suspend sequence
all wait for I2C completion prior to returning from suspend. I suppose
it's possible that applications that use i2c-dev may keep chugging away
on I2C as the system is being suspended(?). Maybe.

Would it make sense to add a flag that blocks I2C transcations if the
suspend callback is called (and unblocks them when resume is called)?
It wouldn't fix the root issue, but would prevent the driver from
starting transactions once it's suspend callback is handled just
returning an error status until resume is called?

> 
> 										Pavel
> 
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[PATCH 1/1] Remove suspend/resume functionality, add dynamic clocking

[Pavel Machek]

> > > > Are you sure its unneeded? What if someone attempts to suspend the
> > > > system when a transaction is running?
> > > That's exactly my question.  I think the machine will suspend and the
> > > transaction fail.  So no suspend callback isn't optimal, but maybe OK?!
> > 
> > Having failures just because suspend happened at wrong time is
> > bad. .suspend() should just wait for end of transaction.
> 
> I'm not sure what situation could actually occur that would suspend the
> system in the middle of an I2C transaction. If an I2C transaction was
> started and the CPU was suspended, this would appear to be a problem
> outside the I2C driver itself. Would other I2C drivers have this similar
> problem?

Well, can the i2c transaction sleep? If so, suspend probably can come
in the middle.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html