[PATCH 1/2] i2c: designware: Remove needless pm_runtime_put_noidle() call

[PATCH 1/2] i2c: designware: Remove needless pm_runtime_put_noidle() call

[Jarkko Nikula]

I guess pm_runtime_put_noidle() call in i2c_dw_probe_slave() was copied
by accident from similar master mode adapter registration code. It is
unbalanced due missing pm_runtime_get_noresume() but harmless since it
doesn't decrease dev->power.usage_count below zero.

In theory we can hit similar needless runtime suspend/resume cycle
during slave mode adapter registration that was happening when
registering the master mode adapter. See commit cd998ded5c12 ("i2c:
designware: Prevent runtime suspend during adapter registration").

However, since we are slave, we can consider it as a wrong configuration
if we have other slaves attached under this adapter and can omit the
pm_runtime_get_noresume()/pm_runtime_put_noidle() calls for simplicity.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-slave.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index 64c7c5e337b9..bb8f738cab14 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -382,7 +382,6 @@ int i2c_dw_probe_slave(struct dw_i2c_dev *dev)
 	ret = i2c_add_numbered_adapter(adap);
 	if (ret)
 		dev_err(dev->dev, "failure adding adapter: %d\n", ret);
-	pm_runtime_put_noidle(dev->dev);
 
 	return ret;
 }
-- 
2.14.1

[PATCH 2/2] i2c: designware: Fix runtime PM for I2C slave mode

[Jarkko Nikula]

I2C slave controller must be powered and active all the time when I2C
slave backend is registered in order to let master address and
communicate with us.

Now if the controller is runtime PM capable it will be suspended after
probe and cannot ever respond to the master or generate interrupts.

Fix this by resuming the controller when I2C slave backend is registered
and let it suspend after unregistering.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-slave.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c
index bb8f738cab14..ea9578ab19a1 100644
--- a/drivers/i2c/busses/i2c-designware-slave.c
+++ b/drivers/i2c/busses/i2c-designware-slave.c
@@ -177,6 +177,8 @@ static int i2c_dw_reg_slave(struct i2c_client *slave)
 		return -EBUSY;
 	if (slave->flags & I2C_CLIENT_TEN)
 		return -EAFNOSUPPORT;
+	pm_runtime_get_sync(dev->dev);
+
 	/*
 	 * Set slave address in the IC_SAR register,
 	 * the address to which the DW_apb_i2c responds.
@@ -205,6 +207,7 @@ static int i2c_dw_unreg_slave(struct i2c_client *slave)
 	dev->disable_int(dev);
 	dev->disable(dev);
 	dev->slave = NULL;
+	pm_runtime_put(dev->dev);
 
 	return 0;
 }
-- 
2.14.1

Re: [PATCH 1/2] i2c: designware: Remove needless pm_runtime_put_noidle() call

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]

On Tue, Aug 15, 2017 at 05:34:44PM +0300, Jarkko Nikula wrote:
> I guess pm_runtime_put_noidle() call in i2c_dw_probe_slave() was copied
> by accident from similar master mode adapter registration code. It is
> unbalanced due missing pm_runtime_get_noresume() but harmless since it
> doesn't decrease dev->power.usage_count below zero.
> 
> In theory we can hit similar needless runtime suspend/resume cycle
> during slave mode adapter registration that was happening when
> registering the master mode adapter. See commit cd998ded5c12 ("i2c:
> designware: Prevent runtime suspend during adapter registration").
> 
> However, since we are slave, we can consider it as a wrong configuration
> if we have other slaves attached under this adapter and can omit the
> pm_runtime_get_noresume()/pm_runtime_put_noidle() calls for simplicity.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Applied to for-current, thanks! Not really a bugfix, but 2/2 is, so,
well... BTW, Luis, are you still there? Holiday season?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] i2c: designware: Fix runtime PM for I2C slave mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 604 bytes --]

On Tue, Aug 15, 2017 at 05:34:45PM +0300, Jarkko Nikula wrote:
> I2C slave controller must be powered and active all the time when I2C
> slave backend is registered in order to let master address and
> communicate with us.
> 
> Now if the controller is runtime PM capable it will be suspended after
> probe and cannot ever respond to the master or generate interrupts.
> 
> Fix this by resuming the controller when I2C slave backend is registered
> and let it suspend after unregistering.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Applied to for-current, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] i2c: designware: Remove needless pm_runtime_put_noidle() call

[Luis Oliveira]

On 17-Aug-17 16:57, Wolfram Sang wrote:
> On Tue, Aug 15, 2017 at 05:34:44PM +0300, Jarkko Nikula wrote:
>> I guess pm_runtime_put_noidle() call in i2c_dw_probe_slave() was copied
>> by accident from similar master mode adapter registration code. It is
>> unbalanced due missing pm_runtime_get_noresume() but harmless since it
>> doesn't decrease dev->power.usage_count below zero.
>>
>> In theory we can hit similar needless runtime suspend/resume cycle
>> during slave mode adapter registration that was happening when
>> registering the master mode adapter. See commit cd998ded5c12 ("i2c:
>> designware: Prevent runtime suspend during adapter registration").
>>
>> However, since we are slave, we can consider it as a wrong configuration
>> if we have other slaves attached under this adapter and can omit the
>> pm_runtime_get_noresume()/pm_runtime_put_noidle() calls for simplicity.
>>
>> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> 
> Applied to for-current, thanks! Not really a bugfix, but 2/2 is, so,
> well... BTW, Luis, are you still there? Holiday season?
> 

Hi all,

Sorry I was on holidays yes. I will get my setup ready to test this ASAP.

But it makes sense what Jarkko said in 2/2.
If it helps, for what I can remember I also used i2c-rcar slave mode as
inspiration for this implementation.

Thank you,
Luis