i2c: designware: Remove needless pm_runtime_put_noidle() call

PATCH Applied Jarkko Nikula 3 participants 5 messages Aug 15, 2017 – Sep 8, 2017

Subsystems (3)

I2C subsystem host drivers Intel LPSS (Low Power SubSystem) drivers Synopsys DesignWare I2C driver

lore

Share & tools

lore

Jarkko Nikula

author

2017-08-15 14:40 UTC #0 lore raw

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

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 <[email protected]>

---
 drivers/i2c/busses/i2c-designware-slave.c | 1 -
 1 file changed, 1 deletion(-)

drivers/i2c/busses/i2c-designware-slave.c +0 -1

index 64c7c5e337b9..bb8f738cab14 100644
@@ -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;
 }

Signature · 1 line

2.14.1

Jarkko Nikula

author

2017-08-15 14:41 UTC #1 lore raw

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

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 <[email protected]>

---
 drivers/i2c/busses/i2c-designware-slave.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/i2c/busses/i2c-designware-slave.c +3 -0

index bb8f738cab14..ea9578ab19a1 100644
@@ -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;
 }

Signature · 1 line

2.14.1

Wolfram Sang

Applied

2017-08-17 15:57 UTC #2 lore raw

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

Quoted 11 lines · 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 <[email protected]>

Applied to for-current, thanks!

Wolfram Sang

Applied

2017-08-17 15:57 UTC #3 lore raw

In reply to Jarkko Nikula

Quoted 10 lines · 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").

Quoted 5 lines

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 <[email protected]>

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

Luis Oliveira

2017-09-08 10:23 UTC #4 lore raw

Quoted 11 lines · Wolfram Sang

Quoted 10 lines · 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").

Quoted 9 lines

Quoted 5 lines
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 <[email protected]>
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