[PATCH 0/2] I2C: OMAP: misc. PM-related cleanups

[PATCH 0/2] I2C: OMAP: misc. PM-related cleanups

[Kevin Hilman]

Do some PM-related cleanup on this driver and make the runtime PM
usage more standard.

Series applies on Andy's I2C cleanup series (for_3.1/i2c-andy-2 branch
in my tree[1]) plus the misc. i2c fixes queued for v3.1-rc
(for_3.1/i2c-fixes branch) and are available in the
for_3.2/i2c-cleanup branch of my tree.

[1] git://git.kernel.org/pub/scm/linux/kernel/git/khilman/linux-omap-pm.git

Kevin Hilman (2):
  I2C: OMAP: remove unneccesary use of pdev
  I2C: OMAP: remove dev->idle, use usage counting provided by runtime
    PM

 drivers/i2c/busses/i2c-omap.c |   76 +++++++++++++++++++++++------------------
 1 files changed, 43 insertions(+), 33 deletions(-)

-- 
1.7.6

[PATCH 1/2] I2C: OMAP: remove unneccesary use of pdev

[Kevin Hilman]

A pointer to the struct device associated with the i2c device is
already kept in the struct omap_i2c_dev, so use omap_i2c_device to
find the pointer to struct device.

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   22 +++++++---------------
 1 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e854be0..12d0cbc 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -267,15 +267,13 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg)
 
 static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 {
-	struct platform_device *pdev;
 	struct omap_i2c_bus_platform_data *pdata;
 
 	WARN_ON(!dev->idle);
 
-	pdev = to_platform_device(dev->dev);
-	pdata = pdev->dev.platform_data;
+	pdata = dev->dev->platform_data;
 
-	pm_runtime_get_sync(&pdev->dev);
+	pm_runtime_get_sync(dev->dev);
 
 	if (pdata->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
@@ -299,14 +297,12 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 
 static void omap_i2c_idle(struct omap_i2c_dev *dev)
 {
-	struct platform_device *pdev;
 	struct omap_i2c_bus_platform_data *pdata;
 	u16 iv;
 
 	WARN_ON(dev->idle);
 
-	pdev = to_platform_device(dev->dev);
-	pdata = pdev->dev.platform_data;
+	pdata = dev->dev->platform_data;
 
 	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
 	if (pdata->rev == OMAP_I2C_IP_VERSION_2)
@@ -324,7 +320,7 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
 	}
 	dev->idle = 1;
 
-	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_put_sync(dev->dev);
 }
 
 static int omap_i2c_init(struct omap_i2c_dev *dev)
@@ -335,11 +331,9 @@ static int omap_i2c_init(struct omap_i2c_dev *dev)
 	unsigned long timeout;
 	unsigned long internal_clk = 0;
 	struct clk *fclk;
-	struct platform_device *pdev;
 	struct omap_i2c_bus_platform_data *pdata;
 
-	pdev = to_platform_device(dev->dev);
-	pdata = pdev->dev.platform_data;
+	pdata = dev->dev->platform_data;
 
 	if (dev->rev >= OMAP_I2C_OMAP1_REV_2) {
 		/* Disable I2C controller before soft reset */
@@ -821,11 +815,9 @@ omap_i2c_isr(int this_irq, void *dev_id)
 	u16 bits;
 	u16 stat, w;
 	int err, count = 0;
-	struct platform_device *pdev;
 	struct omap_i2c_bus_platform_data *pdata;
 
-	pdev = to_platform_device(dev->dev);
-	pdata = pdev->dev.platform_data;
+	pdata = dev->dev->platform_data;
 
 	if (dev->idle)
 		return IRQ_NONE;
@@ -1047,7 +1039,7 @@ omap_i2c_probe(struct platform_device *pdev)
 	else
 		dev->regs = (u8 *)reg_map_ip_v1;
 
-	pm_runtime_enable(&pdev->dev);
+	pm_runtime_enable(dev->dev);
 	omap_i2c_unidle(dev);
 
 	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
-- 
1.7.6

[PATCH 1/2] I2C: OMAP: remove unneccesary use of pdev

[Felipe Balbi]

Hi,

On Wed, Aug 03, 2011 at 11:09:19AM -0700, Kevin Hilman wrote:
> A pointer to the struct device associated with the i2c device is
> already kept in the struct omap_i2c_dev, so use omap_i2c_device to
> find the pointer to struct device.
> 
> Signed-off-by: Kevin Hilman <khilman@ti.com>

Definitely :-)

Reviewed-by: Felipe Balbi <balbi@ti.com>

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110804/7cf6b5c0/attachment.sig>

[PATCH 2/2] I2C: OMAP: remove dev->idle, use usage counting provided by runtime PM

[Kevin Hilman]

Current usage of runtime PM is not quite correct.  The actual
idle/unidle of the I2C hardware should not happen until the runtime PM
callbacks are called.  Therefore, change omap_i2c_[un]idle() functions
to only be called from the runtime PM callbacks (when usage count
transitions to/from zero.)

Also, the runtime PM core does usage counting and replaces
functionality currently managed by the dev->idle flag.  Remove usage
of dev->idle in favor of using runtime PM, and checking status using
pm_runtime_suspended().

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   58 ++++++++++++++++++++++++++--------------
 1 files changed, 38 insertions(+), 20 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 12d0cbc..1b5325b 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -194,7 +194,6 @@ struct omap_i2c_dev {
 						 */
 	u8			rev;
 	unsigned		b_hw:1;		/* bad h/w fixes */
-	unsigned		idle:1;
 	u16			iestate;	/* Saved interrupt register */
 	u16			pscstate;
 	u16			scllstate;
@@ -269,12 +268,8 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 {
 	struct omap_i2c_bus_platform_data *pdata;
 
-	WARN_ON(!dev->idle);
-
 	pdata = dev->dev->platform_data;
 
-	pm_runtime_get_sync(dev->dev);
-
 	if (pdata->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 		omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
@@ -285,7 +280,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 	}
-	dev->idle = 0;
 
 	/*
 	 * Don't write to this register if the IE state is 0 as it can
@@ -300,8 +294,6 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
 	struct omap_i2c_bus_platform_data *pdata;
 	u16 iv;
 
-	WARN_ON(dev->idle);
-
 	pdata = dev->dev->platform_data;
 
 	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
@@ -315,12 +307,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
 	} else {
 		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
 
-		/* Flush posted write before the dev->idle store occurs */
+		/* Flush posted write */
 		omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 	}
-	dev->idle = 1;
-
-	pm_runtime_put_sync(dev->dev);
 }
 
 static int omap_i2c_init(struct omap_i2c_dev *dev)
@@ -644,7 +633,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	int i;
 	int r;
 
-	omap_i2c_unidle(dev);
+	pm_runtime_get_sync(dev->dev);
 
 	r = omap_i2c_wait_for_bb(dev);
 	if (r < 0)
@@ -667,7 +656,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	omap_i2c_wait_for_bb(dev);
 out:
-	omap_i2c_idle(dev);
+	pm_runtime_put_sync(dev->dev);
 	return r;
 }
 
@@ -727,7 +716,7 @@ omap_i2c_omap1_isr(int this_irq, void *dev_id)
 	struct omap_i2c_dev *dev = dev_id;
 	u16 iv, w;
 
-	if (dev->idle)
+	if (pm_runtime_suspended(dev->dev))
 		return IRQ_NONE;
 
 	iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
@@ -819,7 +808,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 
 	pdata = dev->dev->platform_data;
 
-	if (dev->idle)
+	if (pm_runtime_suspended(dev->dev))
 		return IRQ_NONE;
 
 	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
@@ -1021,7 +1010,6 @@ omap_i2c_probe(struct platform_device *pdev)
 	}
 
 	dev->speed = speed;
-	dev->idle = 1;
 	dev->dev = &pdev->dev;
 	dev->irq = irq->start;
 	dev->base = ioremap(mem->start, resource_size(mem));
@@ -1040,7 +1028,7 @@ omap_i2c_probe(struct platform_device *pdev)
 		dev->regs = (u8 *)reg_map_ip_v1;
 
 	pm_runtime_enable(dev->dev);
-	omap_i2c_unidle(dev);
+	pm_runtime_get_sync(dev->dev);
 
 	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
 
@@ -1087,7 +1075,7 @@ omap_i2c_probe(struct platform_device *pdev)
 	dev_info(dev->dev, "bus %d rev%d.%d.%d at %d kHz\n", pdev->id,
 		 pdata->rev, dev->rev >> 4, dev->rev & 0xf, dev->speed);
 
-	omap_i2c_idle(dev);
+	pm_runtime_put_sync(dev->dev);
 
 	adap = &dev->adapter;
 	i2c_set_adapdata(adap, dev);
@@ -1111,7 +1099,7 @@ err_free_irq:
 	free_irq(dev->irq, dev);
 err_unuse_clocks:
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
-	omap_i2c_idle(dev);
+	pm_runtime_put_sync(dev->dev);
 	iounmap(dev->base);
 err_free_mem:
 	platform_set_drvdata(pdev, NULL);
@@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_RUNTIME
+static int omap_i2c_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+	omap_i2c_idle(_dev);
+
+	return 0;
+}
+
+static int omap_i2c_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+	omap_i2c_unidle(_dev);
+
+	return 0;
+}
+
+static struct dev_pm_ops omap_i2c_pm_ops = {
+	.runtime_suspend = omap_i2c_runtime_suspend,
+	.runtime_resume = omap_i2c_runtime_resume,
+};
+#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
+#else
+#define OMAP_I2C_PM_OPS NULL
+#endif
+
 static struct platform_driver omap_i2c_driver = {
 	.probe		= omap_i2c_probe,
 	.remove		= omap_i2c_remove,
-- 
1.7.6

[PATCH 2/2] I2C: OMAP: remove dev->idle, use usage counting provided by runtime PM

[Felipe Balbi]

Hi,

On Wed, Aug 03, 2011 at 11:09:20AM -0700, Kevin Hilman wrote:
> Current usage of runtime PM is not quite correct.  The actual
> idle/unidle of the I2C hardware should not happen until the runtime PM
> callbacks are called.  Therefore, change omap_i2c_[un]idle() functions
> to only be called from the runtime PM callbacks (when usage count
> transitions to/from zero.)
> 
> Also, the runtime PM core does usage counting and replaces
> functionality currently managed by the dev->idle flag.  Remove usage
> of dev->idle in favor of using runtime PM, and checking status using
> pm_runtime_suspended().
> 
> Signed-off-by: Kevin Hilman <khilman@ti.com>
> ---
>  drivers/i2c/busses/i2c-omap.c |   58 ++++++++++++++++++++++++++--------------
>  1 files changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index 12d0cbc..1b5325b 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -194,7 +194,6 @@ struct omap_i2c_dev {
>  						 */
>  	u8			rev;
>  	unsigned		b_hw:1;		/* bad h/w fixes */
> -	unsigned		idle:1;
>  	u16			iestate;	/* Saved interrupt register */
>  	u16			pscstate;
>  	u16			scllstate;
> @@ -269,12 +268,8 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
>  {
>  	struct omap_i2c_bus_platform_data *pdata;
>  
> -	WARN_ON(!dev->idle);
> -
>  	pdata = dev->dev->platform_data;
>  
> -	pm_runtime_get_sync(dev->dev);
> -
>  	if (pdata->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
>  		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>  		omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
> @@ -285,7 +280,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
>  		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
>  		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>  	}
> -	dev->idle = 0;
>  
>  	/*
>  	 * Don't write to this register if the IE state is 0 as it can
> @@ -300,8 +294,6 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
>  	struct omap_i2c_bus_platform_data *pdata;
>  	u16 iv;
>  
> -	WARN_ON(dev->idle);
> -
>  	pdata = dev->dev->platform_data;
>  
>  	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
> @@ -315,12 +307,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
>  	} else {
>  		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
>  
> -		/* Flush posted write before the dev->idle store occurs */
> +		/* Flush posted write */
>  		omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>  	}
> -	dev->idle = 1;
> -
> -	pm_runtime_put_sync(dev->dev);
>  }
>  
>  static int omap_i2c_init(struct omap_i2c_dev *dev)
> @@ -644,7 +633,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  	int i;
>  	int r;
>  
> -	omap_i2c_unidle(dev);
> +	pm_runtime_get_sync(dev->dev);
>  
>  	r = omap_i2c_wait_for_bb(dev);
>  	if (r < 0)
> @@ -667,7 +656,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  
>  	omap_i2c_wait_for_bb(dev);
>  out:
> -	omap_i2c_idle(dev);
> +	pm_runtime_put_sync(dev->dev);

I wonder if these pm_runtime_put need to be synchronous ? Could we just
call pm_runtime_put() instead ? Ditto to all other.

> @@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_RUNTIME
> +static int omap_i2c_runtime_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);

what happened to dev_get_drvdata(dev) ??

> +	omap_i2c_idle(_dev);
> +
> +	return 0;
> +}
> +
> +static int omap_i2c_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);

ditto

> +	omap_i2c_unidle(_dev);
> +
> +	return 0;
> +}
> +
> +static struct dev_pm_ops omap_i2c_pm_ops = {
> +	.runtime_suspend = omap_i2c_runtime_suspend,
> +	.runtime_resume = omap_i2c_runtime_resume,
> +};
> +#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
> +#else
> +#define OMAP_I2C_PM_OPS NULL
> +#endif

OMAP_I2C_PM_OPS isn't used anywhere ??

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110804/fd9aec0c/attachment-0001.sig>

[PATCH 2/2] I2C: OMAP: remove dev->idle, use usage counting provided by runtime PM

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Wed, Aug 03, 2011 at 11:09:20AM -0700, Kevin Hilman wrote:
>> Current usage of runtime PM is not quite correct.  The actual
>> idle/unidle of the I2C hardware should not happen until the runtime PM
>> callbacks are called.  Therefore, change omap_i2c_[un]idle() functions
>> to only be called from the runtime PM callbacks (when usage count
>> transitions to/from zero.)
>> 
>> Also, the runtime PM core does usage counting and replaces
>> functionality currently managed by the dev->idle flag.  Remove usage
>> of dev->idle in favor of using runtime PM, and checking status using
>> pm_runtime_suspended().
>> 
>> Signed-off-by: Kevin Hilman <khilman@ti.com>
>> ---
>>  drivers/i2c/busses/i2c-omap.c |   58 ++++++++++++++++++++++++++--------------
>>  1 files changed, 38 insertions(+), 20 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index 12d0cbc..1b5325b 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -194,7 +194,6 @@ struct omap_i2c_dev {
>>  						 */
>>  	u8			rev;
>>  	unsigned		b_hw:1;		/* bad h/w fixes */
>> -	unsigned		idle:1;
>>  	u16			iestate;	/* Saved interrupt register */
>>  	u16			pscstate;
>>  	u16			scllstate;
>> @@ -269,12 +268,8 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
>>  {
>>  	struct omap_i2c_bus_platform_data *pdata;
>>  
>> -	WARN_ON(!dev->idle);
>> -
>>  	pdata = dev->dev->platform_data;
>>  
>> -	pm_runtime_get_sync(dev->dev);
>> -
>>  	if (pdata->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
>>  		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
>>  		omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
>> @@ -285,7 +280,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
>>  		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
>>  		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
>>  	}
>> -	dev->idle = 0;
>>  
>>  	/*
>>  	 * Don't write to this register if the IE state is 0 as it can
>> @@ -300,8 +294,6 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
>>  	struct omap_i2c_bus_platform_data *pdata;
>>  	u16 iv;
>>  
>> -	WARN_ON(dev->idle);
>> -
>>  	pdata = dev->dev->platform_data;
>>  
>>  	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
>> @@ -315,12 +307,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
>>  	} else {
>>  		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
>>  
>> -		/* Flush posted write before the dev->idle store occurs */
>> +		/* Flush posted write */
>>  		omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
>>  	}
>> -	dev->idle = 1;
>> -
>> -	pm_runtime_put_sync(dev->dev);
>>  }
>>  
>>  static int omap_i2c_init(struct omap_i2c_dev *dev)
>> @@ -644,7 +633,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>>  	int i;
>>  	int r;
>>  
>> -	omap_i2c_unidle(dev);
>> +	pm_runtime_get_sync(dev->dev);
>>  
>>  	r = omap_i2c_wait_for_bb(dev);
>>  	if (r < 0)
>> @@ -667,7 +656,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>>  
>>  	omap_i2c_wait_for_bb(dev);
>>  out:
>> -	omap_i2c_idle(dev);
>> +	pm_runtime_put_sync(dev->dev);
>
> I wonder if these pm_runtime_put need to be synchronous ? Could we just
> call pm_runtime_put() instead ? Ditto to all other.

Yes, will use asynchronous versions.

>> @@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev)
>>  	return 0;
>>  }
>>  
>> +#ifdef CONFIG_PM_RUNTIME
>> +static int omap_i2c_runtime_suspend(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
>
> what happened to dev_get_drvdata(dev) ??
>

Yes, that would work too since:

static inline void *platform_get_drvdata(const struct platform_device *pdev)
{
	return dev_get_drvdata(&pdev->dev);
}

but IMO, readability is better if the driver does platform_set_drvdata()
and then the corresponding platform_get_drvdata()

>> +	omap_i2c_idle(_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static int omap_i2c_runtime_resume(struct device *dev)
>> +{
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
>
> ditto
>
>> +	omap_i2c_unidle(_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct dev_pm_ops omap_i2c_pm_ops = {
>> +	.runtime_suspend = omap_i2c_runtime_suspend,
>> +	.runtime_resume = omap_i2c_runtime_resume,
>> +};
>> +#define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
>> +#else
>> +#define OMAP_I2C_PM_OPS NULL
>> +#endif
>
> OMAP_I2C_PM_OPS isn't used anywhere ??

doh

thanks for catching

Kevin

[PATCH 2/2] I2C: OMAP: remove dev->idle, use usage counting provided by runtime PM

[Felipe Balbi]

Hi,

On Thu, Aug 04, 2011 at 07:53:37AM -0700, Kevin Hilman wrote:
> >> @@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev)
> >>  	return 0;
> >>  }
> >>  
> >> +#ifdef CONFIG_PM_RUNTIME
> >> +static int omap_i2c_runtime_suspend(struct device *dev)
> >> +{
> >> +	struct platform_device *pdev = to_platform_device(dev);
> >> +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
> >
> > what happened to dev_get_drvdata(dev) ??
> >
> 
> Yes, that would work too since:
> 
> static inline void *platform_get_drvdata(const struct platform_device *pdev)
> {
> 	return dev_get_drvdata(&pdev->dev);
> }
> 
> but IMO, readability is better if the driver does platform_set_drvdata()
> and then the corresponding platform_get_drvdata()

fair enough, but if you already have the dev pointer, what's the gain in
doing a container_of() just to go back to the dev pointer again ?

IMHO, there's really no need for that and just calling dev_get_drvdata()
straight is enough. All in all, platform_get_[sg]et_drvdata() are just
wrappers for dev_[sg]et_drvdata(). The whole point, was to avoid
dev_[sg]et_drvdata(&pdev->dev). It's the same with <drivername>_writel()
calls we see on all drivers. Just a wrapper, but you can use
__raw_writel() directly if you wish.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110804/91a06100/attachment.sig>

[PATCH 2/2] I2C: OMAP: remove dev->idle, use usage counting provided by runtime PM

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Thu, Aug 04, 2011 at 07:53:37AM -0700, Kevin Hilman wrote:
>> >> @@ -1140,6 +1128,36 @@ omap_i2c_remove(struct platform_device *pdev)
>> >>  	return 0;
>> >>  }
>> >>  
>> >> +#ifdef CONFIG_PM_RUNTIME
>> >> +static int omap_i2c_runtime_suspend(struct device *dev)
>> >> +{
>> >> +	struct platform_device *pdev = to_platform_device(dev);
>> >> +	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
>> >
>> > what happened to dev_get_drvdata(dev) ??
>> >
>> 
>> Yes, that would work too since:
>> 
>> static inline void *platform_get_drvdata(const struct platform_device *pdev)
>> {
>> 	return dev_get_drvdata(&pdev->dev);
>> }
>> 
>> but IMO, readability is better if the driver does platform_set_drvdata()
>> and then the corresponding platform_get_drvdata()
>
> fair enough, but if you already have the dev pointer, what's the gain in
> doing a container_of() just to go back to the dev pointer again ?

There is no gain, but there is no loss either.  The compiler is smart
enough that the resulting assembly is the same.

> IMHO, there's really no need for that and just calling dev_get_drvdata()
> straight is enough. All in all, platform_get_[sg]et_drvdata() are just
> wrappers for dev_[sg]et_drvdata(). The whole point, was to avoid
> dev_[sg]et_drvdata(&pdev->dev).

Well, whether or not to use dev_[gs]et_* or platform_[gs]et_* is not
relevant to $SUBJECT patch. 

The current driver does platform_set_drvdata(), so I used
platform_get_drvdata() for consistency and readability.  If I were to
use dev_get* then I would want the correpsonding set changed to
dev_set_* also for consistency.

If someone wants to change both the sets and gets to the dev_ versions,
that's fine with me, but it should be separate patch.

Kevin

[PATCH 0/2] I2C: OMAP: clean up runtime PM usage

[Kevin Hilman]

Current usage of runtime PM is not quite correct.  The actual
idle/unidle of the I2C hardware should not happen until the runtime PM
callbacks are called.

Clean up by using runtime PM callbacks to call idle/unidle functions
and also replace usage of dev->idle check by pm_runtime_suspended() check.

Kevin Hilman (2):
  I2C: OMAP: remove unneccesary use of pdev
  I2C: OMAP: remove dev->idle, use usage counting provided by runtime
    PM

 drivers/i2c/busses/i2c-omap.c |   70 +++++++++++++++++++++--------------------
 1 files changed, 36 insertions(+), 34 deletions(-)

-- 
1.7.3.4

[PATCH 2/2] I2C: OMAP: remove dev->idle, use usage counting provided by runtime PM

[Kevin Hilman]

Current usage of runtime PM is not quite correct.  The actual
idle/unidle of the I2C hardware should not happen until the runtime PM
callbacks are called.  Therefore, change omap_i2c_[un]idle() functions
to only be called from the runtime PM callbacks (when usage count
transitions to/from zero.)

Also, the runtime PM core does usage counting and replaces
functionality currently managed by the dev->idle flag.  Remove usage
of dev->idle in favor of using runtime PM, and checking status using
pm_runtime_suspended().

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 drivers/i2c/busses/i2c-omap.c |   52 ++++++++++++++++++++++++----------------
 1 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index 8633488..4e3256f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -194,7 +194,6 @@ struct omap_i2c_dev {
 						 */
 	u8			rev;
 	unsigned		b_hw:1;		/* bad h/w fixes */
-	unsigned		idle:1;
 	u16			iestate;	/* Saved interrupt register */
 	u16			pscstate;
 	u16			scllstate;
@@ -269,12 +268,8 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 {
 	struct omap_i2c_bus_platform_data *pdata;
 
-	WARN_ON(!dev->idle);
-
 	pdata = dev->dev->platform_data;
 
-	pm_runtime_get_sync(dev->dev);
-
 	if (pdata->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
 		omap_i2c_write_reg(dev, OMAP_I2C_PSC_REG, dev->pscstate);
@@ -285,7 +280,6 @@ static void omap_i2c_unidle(struct omap_i2c_dev *dev)
 		omap_i2c_write_reg(dev, OMAP_I2C_WE_REG, dev->westate);
 		omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, OMAP_I2C_CON_EN);
 	}
-	dev->idle = 0;
 
 	/*
 	 * Don't write to this register if the IE state is 0 as it can
@@ -300,8 +294,6 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
 	struct omap_i2c_bus_platform_data *pdata;
 	u16 iv;
 
-	WARN_ON(dev->idle);
-
 	pdata = dev->dev->platform_data;
 
 	dev->iestate = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
@@ -315,12 +307,9 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev)
 	} else {
 		omap_i2c_write_reg(dev, OMAP_I2C_STAT_REG, dev->iestate);
 
-		/* Flush posted write before the dev->idle store occurs */
+		/* Flush posted write */
 		omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG);
 	}
-	dev->idle = 1;
-
-	pm_runtime_put_sync(dev->dev);
 }
 
 static int omap_i2c_init(struct omap_i2c_dev *dev)
@@ -645,7 +634,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	int i;
 	int r;
 
-	omap_i2c_unidle(dev);
+	pm_runtime_get_sync(dev->dev);
 
 	r = omap_i2c_wait_for_bb(dev);
 	if (r < 0)
@@ -668,7 +657,7 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	omap_i2c_wait_for_bb(dev);
 out:
-	omap_i2c_idle(dev);
+	pm_runtime_put_sync(dev->dev);
 	return r;
 }
 
@@ -728,7 +717,7 @@ omap_i2c_omap1_isr(int this_irq, void *dev_id)
 	struct omap_i2c_dev *dev = dev_id;
 	u16 iv, w;
 
-	if (dev->idle)
+	if (pm_runtime_suspended(dev->dev))
 		return IRQ_NONE;
 
 	iv = omap_i2c_read_reg(dev, OMAP_I2C_IV_REG);
@@ -820,7 +809,7 @@ omap_i2c_isr(int this_irq, void *dev_id)
 
 	pdata = dev->dev->platform_data;
 
-	if (dev->idle)
+	if (pm_runtime_suspended(dev->dev))
 		return IRQ_NONE;
 
 	bits = omap_i2c_read_reg(dev, OMAP_I2C_IE_REG);
@@ -1022,7 +1011,6 @@ omap_i2c_probe(struct platform_device *pdev)
 	}
 
 	dev->speed = speed;
-	dev->idle = 1;
 	dev->dev = &pdev->dev;
 	dev->irq = irq->start;
 	dev->base = ioremap(mem->start, resource_size(mem));
@@ -1041,7 +1029,7 @@ omap_i2c_probe(struct platform_device *pdev)
 		dev->regs = (u8 *)reg_map_ip_v1;
 
 	pm_runtime_enable(dev->dev);
-	omap_i2c_unidle(dev);
+	pm_runtime_get_sync(dev->dev);
 
 	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
 
@@ -1088,7 +1076,7 @@ omap_i2c_probe(struct platform_device *pdev)
 	dev_info(dev->dev, "bus %d rev%d.%d.%d at %d kHz\n", pdev->id,
 		 pdata->rev, dev->rev >> 4, dev->rev & 0xf, dev->speed);
 
-	omap_i2c_idle(dev);
+	pm_runtime_put_sync(dev->dev);
 
 	adap = &dev->adapter;
 	i2c_set_adapdata(adap, dev);
@@ -1112,7 +1100,7 @@ err_free_irq:
 	free_irq(dev->irq, dev);
 err_unuse_clocks:
 	omap_i2c_write_reg(dev, OMAP_I2C_CON_REG, 0);
-	omap_i2c_idle(dev);
+	pm_runtime_put_sync(dev->dev);
 	iounmap(dev->base);
 err_free_mem:
 	platform_set_drvdata(pdev, NULL);
@@ -1141,7 +1129,27 @@ omap_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_SUSPEND
+#ifdef CONFIG_PM
+static int omap_i2c_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+	omap_i2c_idle(_dev);
+
+	return 0;
+}
+
+static int omap_i2c_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
+
+	omap_i2c_unidle(_dev);
+
+	return 0;
+}
+
 static int omap_i2c_suspend(struct device *dev)
 {
 	if (!pm_runtime_suspended(dev))
@@ -1163,6 +1171,8 @@ static int omap_i2c_resume(struct device *dev)
 static struct dev_pm_ops omap_i2c_pm_ops = {
 	.suspend = omap_i2c_suspend,
 	.resume = omap_i2c_resume,
+	.runtime_suspend = omap_i2c_runtime_suspend,
+	.runtime_resume = omap_i2c_runtime_resume,
 };
 #define OMAP_I2C_PM_OPS (&omap_i2c_pm_ops)
 #else
-- 
1.7.3.4