[RFC/RFT 00/10] i2c: move handling of suspended adapters to the core

[RFC/RFT 00/10] i2c: move handling of suspended adapters to the core

[Wolfram Sang]

Finally, here is the implementation Hans and I agreed on. Plus, all potential
users I could spot already converted. Renesas R-Car driver was added on top.
This series was tested on a Renesas Lager board (R-Car H2). I had to hack some
error cases into the code to verify the workings. I couldn't create an error
case otherwise, this is why further testing with more complex setups is very
welcome.

For my taste, there is still too much boilerplate code for drivers left. Plus,
it is also too easy to put it too early or too late. But I haven't come up with
a better idea yet. And it is time to get this somehow forward.

Please comment, review, test... a branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/core-pm-helpers

Thanks,

   Wolfram

Wolfram Sang (10):
  i2c: add 'is_suspended' flag for i2c adapters
  i2c: reject new transfers when adapters are suspended
  i2c: synquacer: remove unused is_suspended flag
  i2c: brcmstb: use core helper to mark adapter suspended
  i2c: zx2967: use core helper to mark adapter suspended
  i2c: sprd: don't use pdev as variable name for struct device *
  i2c: sprd: use core helper to mark adapter suspended
  i2c: exynos5: use core helper to mark adapter suspended
  i2c: s3c2410: use core helper to mark adapter suspended
  i2c: rcar: add suspend/resume support

 drivers/i2c/busses/i2c-brcmstb.c   | 13 ++-----------
 drivers/i2c/busses/i2c-exynos5.c   | 11 ++---------
 drivers/i2c/busses/i2c-rcar.c      | 25 +++++++++++++++++++++++++
 drivers/i2c/busses/i2c-s3c2410.c   |  8 ++------
 drivers/i2c/busses/i2c-sprd.c      | 34 ++++++++++++----------------------
 drivers/i2c/busses/i2c-synquacer.c |  5 -----
 drivers/i2c/busses/i2c-zx2967.c    |  8 ++------
 drivers/i2c/i2c-core-base.c        |  3 +++
 include/linux/i2c.h                |  9 +++++++++
 9 files changed, 57 insertions(+), 59 deletions(-)

-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC/RFT 00/10] i2c: move handling of suspended adapters to the core

[Wolfram Sang]

Finally, here is the implementation Hans and I agreed on. Plus, all potential
users I could spot already converted. Renesas R-Car driver was added on top.
This series was tested on a Renesas Lager board (R-Car H2). I had to hack some
error cases into the code to verify the workings. I couldn't create an error
case otherwise, this is why further testing with more complex setups is very
welcome.

For my taste, there is still too much boilerplate code for drivers left. Plus,
it is also too easy to put it too early or too late. But I haven't come up with
a better idea yet. And it is time to get this somehow forward.

Please comment, review, test... a branch can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/core-pm-helpers

Thanks,

   Wolfram

Wolfram Sang (10):
  i2c: add 'is_suspended' flag for i2c adapters
  i2c: reject new transfers when adapters are suspended
  i2c: synquacer: remove unused is_suspended flag
  i2c: brcmstb: use core helper to mark adapter suspended
  i2c: zx2967: use core helper to mark adapter suspended
  i2c: sprd: don't use pdev as variable name for struct device *
  i2c: sprd: use core helper to mark adapter suspended
  i2c: exynos5: use core helper to mark adapter suspended
  i2c: s3c2410: use core helper to mark adapter suspended
  i2c: rcar: add suspend/resume support

 drivers/i2c/busses/i2c-brcmstb.c   | 13 ++-----------
 drivers/i2c/busses/i2c-exynos5.c   | 11 ++---------
 drivers/i2c/busses/i2c-rcar.c      | 25 +++++++++++++++++++++++++
 drivers/i2c/busses/i2c-s3c2410.c   |  8 ++------
 drivers/i2c/busses/i2c-sprd.c      | 34 ++++++++++++----------------------
 drivers/i2c/busses/i2c-synquacer.c |  5 -----
 drivers/i2c/busses/i2c-zx2967.c    |  8 ++------
 drivers/i2c/i2c-core-base.c        |  3 +++
 include/linux/i2c.h                |  9 +++++++++
 9 files changed, 57 insertions(+), 59 deletions(-)

-- 
2.11.0

[RFC/RFT 01/10] i2c: add 'is_suspended' flag for i2c adapters

[Wolfram Sang]

Some drivers open code handling of suspended adapters. It should be
handled by the core, though, to ensure generic handling. This patch adds
the flag and an accessor function.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 1 +
 include/linux/i2c.h         | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 28460f6a60cc..9f89e258c8ff 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1232,6 +1232,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (!adap->lock_ops)
 		adap->lock_ops = &i2c_adapter_lock_ops;
 
+	adap->is_suspended = false;
 	rt_mutex_init(&adap->bus_lock);
 	rt_mutex_init(&adap->mux_lock);
 	mutex_init(&adap->userspace_clients_lock);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..9852038ee3a7 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -680,6 +680,7 @@ struct i2c_adapter {
 	int timeout;			/* in jiffies */
 	int retries;
 	struct device dev;		/* the adapter device */
+	unsigned int is_suspended:1;	/* owned by the I2C core */
 
 	int nr;
 	char name[48];
@@ -762,6 +763,14 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
 	adapter->lock_ops->unlock_bus(adapter, flags);
 }
 
+static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap,
+					      bool suspended)
+{
+	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+	adap->is_suspended = suspended;
+	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+}
+
 /*flags for the client struct: */
 #define I2C_CLIENT_PEC		0x04	/* Use Packet Error Checking */
 #define I2C_CLIENT_TEN		0x10	/* we have a ten bit chip address */
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC/RFT 01/10] i2c: add 'is_suspended' flag for i2c adapters

[Wolfram Sang]

Some drivers open code handling of suspended adapters. It should be
handled by the core, though, to ensure generic handling. This patch adds
the flag and an accessor function.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 1 +
 include/linux/i2c.h         | 9 +++++++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 28460f6a60cc..9f89e258c8ff 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1232,6 +1232,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (!adap->lock_ops)
 		adap->lock_ops = &i2c_adapter_lock_ops;
 
+	adap->is_suspended = false;
 	rt_mutex_init(&adap->bus_lock);
 	rt_mutex_init(&adap->mux_lock);
 	mutex_init(&adap->userspace_clients_lock);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..9852038ee3a7 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -680,6 +680,7 @@ struct i2c_adapter {
 	int timeout;			/* in jiffies */
 	int retries;
 	struct device dev;		/* the adapter device */
+	unsigned int is_suspended:1;	/* owned by the I2C core */
 
 	int nr;
 	char name[48];
@@ -762,6 +763,14 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
 	adapter->lock_ops->unlock_bus(adapter, flags);
 }
 
+static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap,
+					      bool suspended)
+{
+	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+	adap->is_suspended = suspended;
+	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+}
+
 /*flags for the client struct: */
 #define I2C_CLIENT_PEC		0x04	/* Use Packet Error Checking */
 #define I2C_CLIENT_TEN		0x10	/* we have a ten bit chip address */
-- 
2.11.0

[RFC/RFT 02/10] i2c: reject new transfers when adapters are suspended

[Wolfram Sang]

Using the new 'is_suspended' flag, we now reject new transfers if the
adapter is already marked suspended.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9f89e258c8ff..5b2078a902f8 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1866,6 +1866,8 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 
 	if (WARN_ON(!msgs || num < 1))
 		return -EINVAL;
+	if (WARN_ON(adap->is_suspended))
+		return -ESHUTDOWN;
 
 	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
 		return -EOPNOTSUPP;
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC/RFT 02/10] i2c: reject new transfers when adapters are suspended

[Wolfram Sang]

Using the new 'is_suspended' flag, we now reject new transfers if the
adapter is already marked suspended.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/i2c-core-base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9f89e258c8ff..5b2078a902f8 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1866,6 +1866,8 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 
 	if (WARN_ON(!msgs || num < 1))
 		return -EINVAL;
+	if (WARN_ON(adap->is_suspended))
+		return -ESHUTDOWN;
 
 	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
 		return -EOPNOTSUPP;
-- 
2.11.0

[RFC/RFT 03/10] i2c: synquacer: remove unused is_suspended flag

[Wolfram Sang]

This flag was defined and checked but never set a value. Remove it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-synquacer.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c
index 2184b7c3580e..d18b0941b71a 100644
--- a/drivers/i2c/busses/i2c-synquacer.c
+++ b/drivers/i2c/busses/i2c-synquacer.c
@@ -144,8 +144,6 @@ struct synquacer_i2c {
 	u32			timeout_ms;
 	enum i2c_state		state;
 	struct i2c_adapter	adapter;
-
-	bool			is_suspended;
 };
 
 static inline int is_lastmsg(struct synquacer_i2c *i2c)
@@ -316,9 +314,6 @@ static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
 	unsigned long timeout;
 	int ret;
 
-	if (i2c->is_suspended)
-		return -EBUSY;
-
 	synquacer_i2c_hw_init(i2c);
 	bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
 	if (bsr & SYNQUACER_I2C_BSR_BB) {
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC/RFT 03/10] i2c: synquacer: remove unused is_suspended flag

[Wolfram Sang]

This flag was defined and checked but never set a value. Remove it.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-synquacer.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c
index 2184b7c3580e..d18b0941b71a 100644
--- a/drivers/i2c/busses/i2c-synquacer.c
+++ b/drivers/i2c/busses/i2c-synquacer.c
@@ -144,8 +144,6 @@ struct synquacer_i2c {
 	u32			timeout_ms;
 	enum i2c_state		state;
 	struct i2c_adapter	adapter;
-
-	bool			is_suspended;
 };
 
 static inline int is_lastmsg(struct synquacer_i2c *i2c)
@@ -316,9 +314,6 @@ static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
 	unsigned long timeout;
 	int ret;
 
-	if (i2c->is_suspended)
-		return -EBUSY;
-
 	synquacer_i2c_hw_init(i2c);
 	bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
 	if (bsr & SYNQUACER_I2C_BSR_BB) {
-- 
2.11.0

[RFC/RFT 04/10] i2c: brcmstb: use core helper to mark adapter suspended

[Wolfram Sang]

Rejecting transfers should be handled by the core.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-brcmstb.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 826d32049996..05c822e5a3f6 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -170,7 +170,6 @@ struct brcmstb_i2c_dev {
 	struct bsc_regs *bsc_regmap;
 	struct i2c_adapter adapter;
 	struct completion done;
-	bool is_suspended;
 	u32 clk_freq_hz;
 	int data_regsz;
 };
@@ -467,9 +466,6 @@ static int brcmstb_i2c_xfer(struct i2c_adapter *adapter,
 	int xfersz = brcmstb_i2c_get_xfersz(dev);
 	u32 cond, cond_per_msg;
 
-	if (dev->is_suspended)
-		return -EBUSY;
-
 	/* Loop through all messages */
 	for (i = 0; i < num; i++) {
 		pmsg = &msgs[i];
@@ -689,10 +685,7 @@ static int brcmstb_i2c_suspend(struct device *dev)
 {
 	struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
 
-	i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
-	i2c_dev->is_suspended = true;
-	i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
-
+	i2c_mark_adapter_suspended(&i2c_dev->adapter, true);
 	return 0;
 }
 
@@ -700,10 +693,8 @@ static int brcmstb_i2c_resume(struct device *dev)
 {
 	struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
 
-	i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
 	brcmstb_i2c_set_bsc_reg_defaults(i2c_dev);
-	i2c_dev->is_suspended = false;
-	i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
+	i2c_mark_adapter_suspended(&i2c_dev->adapter, false);
 
 	return 0;
 }
-- 
2.11.0

[RFC/RFT 04/10] i2c: brcmstb: use core helper to mark adapter suspended

[Wolfram Sang]

Rejecting transfers should be handled by the core.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-brcmstb.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 826d32049996..05c822e5a3f6 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -170,7 +170,6 @@ struct brcmstb_i2c_dev {
 	struct bsc_regs *bsc_regmap;
 	struct i2c_adapter adapter;
 	struct completion done;
-	bool is_suspended;
 	u32 clk_freq_hz;
 	int data_regsz;
 };
@@ -467,9 +466,6 @@ static int brcmstb_i2c_xfer(struct i2c_adapter *adapter,
 	int xfersz = brcmstb_i2c_get_xfersz(dev);
 	u32 cond, cond_per_msg;
 
-	if (dev->is_suspended)
-		return -EBUSY;
-
 	/* Loop through all messages */
 	for (i = 0; i < num; i++) {
 		pmsg = &msgs[i];
@@ -689,10 +685,7 @@ static int brcmstb_i2c_suspend(struct device *dev)
 {
 	struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
 
-	i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
-	i2c_dev->is_suspended = true;
-	i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
-
+	i2c_mark_adapter_suspended(&i2c_dev->adapter, true);
 	return 0;
 }
 
@@ -700,10 +693,8 @@ static int brcmstb_i2c_resume(struct device *dev)
 {
 	struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
 
-	i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
 	brcmstb_i2c_set_bsc_reg_defaults(i2c_dev);
-	i2c_dev->is_suspended = false;
-	i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
+	i2c_mark_adapter_suspended(&i2c_dev->adapter, false);
 
 	return 0;
 }
-- 
2.11.0

[RFC/RFT 05/10] i2c: zx2967: use core helper to mark adapter suspended

[Wolfram Sang]

Rejecting transfers should be handled by the core. Also, this will
ensure proper locking which was forgotten in this open coded version
and make sure resume mark is set after enabling clocks (not before).

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-zx2967.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-zx2967.c b/drivers/i2c/busses/i2c-zx2967.c
index b8f9e020d80e..9fa48bbe0719 100644
--- a/drivers/i2c/busses/i2c-zx2967.c
+++ b/drivers/i2c/busses/i2c-zx2967.c
@@ -66,7 +66,6 @@ struct zx2967_i2c {
 	int			msg_rd;
 	u8			*cur_trans;
 	u8			access_cnt;
-	bool			is_suspended;
 	int			error;
 };
 
@@ -313,9 +312,6 @@ static int zx2967_i2c_xfer(struct i2c_adapter *adap,
 	int ret;
 	int i;
 
-	if (i2c->is_suspended)
-		return -EBUSY;
-
 	zx2967_set_addr(i2c, msgs->addr);
 
 	for (i = 0; i < num; i++) {
@@ -470,7 +466,7 @@ static int __maybe_unused zx2967_i2c_suspend(struct device *dev)
 {
 	struct zx2967_i2c *i2c = dev_get_drvdata(dev);
 
-	i2c->is_suspended = true;
+	i2c_mark_adapter_suspended(&i2c->adap, true);
 	clk_disable_unprepare(i2c->clk);
 
 	return 0;
@@ -480,8 +476,8 @@ static int __maybe_unused zx2967_i2c_resume(struct device *dev)
 {
 	struct zx2967_i2c *i2c = dev_get_drvdata(dev);
 
-	i2c->is_suspended = false;
 	clk_prepare_enable(i2c->clk);
+	i2c_mark_adapter_suspended(&i2c->adap, false);
 
 	return 0;
 }
-- 
2.11.0

[RFC/RFT 06/10] i2c: sprd: don't use pdev as variable name for struct device *

[Wolfram Sang]

The pointer to a device is usually named 'dev'. These 'pdev' here look
much like copy&paste errors. Fix them to avoid confusion.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sprd.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index a94e724f51dc..e266d8a713d9 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -586,40 +586,40 @@ static int sprd_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int __maybe_unused sprd_i2c_suspend_noirq(struct device *pdev)
+static int __maybe_unused sprd_i2c_suspend_noirq(struct device *dev)
 {
-	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+	struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
 
 	i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
 	i2c_dev->is_suspended = true;
 	i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
 
-	return pm_runtime_force_suspend(pdev);
+	return pm_runtime_force_suspend(dev);
 }
 
-static int __maybe_unused sprd_i2c_resume_noirq(struct device *pdev)
+static int __maybe_unused sprd_i2c_resume_noirq(struct device *dev)
 {
-	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+	struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
 
 	i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
 	i2c_dev->is_suspended = false;
 	i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
 
-	return pm_runtime_force_resume(pdev);
+	return pm_runtime_force_resume(dev);
 }
 
-static int __maybe_unused sprd_i2c_runtime_suspend(struct device *pdev)
+static int __maybe_unused sprd_i2c_runtime_suspend(struct device *dev)
 {
-	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+	struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
 
 	clk_disable_unprepare(i2c_dev->clk);
 
 	return 0;
 }
 
-static int __maybe_unused sprd_i2c_runtime_resume(struct device *pdev)
+static int __maybe_unused sprd_i2c_runtime_resume(struct device *dev)
 {
-	struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+	struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
 	int ret;
 
 	ret = clk_prepare_enable(i2c_dev->clk);
-- 
2.11.0

[RFC/RFT 07/10] i2c: sprd: use core helper to mark adapter suspended

[Wolfram Sang]

Rejecting transfers should be handled by the core.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-sprd.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index e266d8a713d9..acbaf5fd29ee 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -86,7 +86,6 @@ struct sprd_i2c {
 	u32 count;
 	int irq;
 	int err;
-	bool is_suspended;
 };
 
 static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
@@ -284,9 +283,6 @@ static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
 	struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
 	int im, ret;
 
-	if (i2c_dev->is_suspended)
-		return -EBUSY;
-
 	ret = pm_runtime_get_sync(i2c_dev->dev);
 	if (ret < 0)
 		return ret;
@@ -590,10 +586,7 @@ static int __maybe_unused sprd_i2c_suspend_noirq(struct device *dev)
 {
 	struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
 
-	i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
-	i2c_dev->is_suspended = true;
-	i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
-
+	i2c_mark_adapter_suspended(&i2c_dev->adap, true);
 	return pm_runtime_force_suspend(dev);
 }
 
@@ -601,10 +594,7 @@ static int __maybe_unused sprd_i2c_resume_noirq(struct device *dev)
 {
 	struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
 
-	i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
-	i2c_dev->is_suspended = false;
-	i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
-
+	i2c_mark_adapter_suspended(&i2c_dev->adap, false);
 	return pm_runtime_force_resume(dev);
 }
 
-- 
2.11.0

[RFC/RFT 08/10] i2c: exynos5: use core helper to mark adapter suspended

[Wolfram Sang]

Rejecting transfers should be handled by the core. Also, this will
ensure proper locking which was forgotten in this open coded version.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-exynos5.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index c1ce2299a76e..fe2f2031ea36 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -183,7 +183,6 @@ enum i2c_type_exynos {
 
 struct exynos5_i2c {
 	struct i2c_adapter	adap;
-	unsigned int		suspended:1;
 
 	struct i2c_msg		*msg;
 	struct completion	msg_complete;
@@ -715,11 +714,6 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
 	struct exynos5_i2c *i2c = adap->algo_data;
 	int i, ret;
 
-	if (i2c->suspended) {
-		dev_err(i2c->dev, "HS-I2C is not initialized.\n");
-		return -EIO;
-	}
-
 	ret = clk_enable(i2c->clk);
 	if (ret)
 		return ret;
@@ -847,8 +841,7 @@ static int exynos5_i2c_suspend_noirq(struct device *dev)
 {
 	struct exynos5_i2c *i2c = dev_get_drvdata(dev);
 
-	i2c->suspended = 1;
-
+	i2c_mark_adapter_suspended(&i2c->adap, true);
 	clk_unprepare(i2c->clk);
 
 	return 0;
@@ -871,7 +864,7 @@ static int exynos5_i2c_resume_noirq(struct device *dev)
 
 	exynos5_i2c_init(i2c);
 	clk_disable(i2c->clk);
-	i2c->suspended = 0;
+	i2c_mark_adapter_suspended(&i2c->adap, false);
 
 	return 0;
 }
-- 
2.11.0

[RFC/RFT 09/10] i2c: s3c2410: use core helper to mark adapter suspended

[Wolfram Sang]

Rejecting transfers should be handled by the core. Also, this will
ensure proper locking which was forgotten in this open coded version.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-s3c2410.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 2f2e28d60ef5..7c76edb25514 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -104,7 +104,6 @@ enum s3c24xx_i2c_state {
 struct s3c24xx_i2c {
 	wait_queue_head_t	wait;
 	kernel_ulong_t		quirks;
-	unsigned int		suspended:1;
 
 	struct i2c_msg		*msg;
 	unsigned int		msg_num;
@@ -703,9 +702,6 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 	unsigned long timeout;
 	int ret;
 
-	if (i2c->suspended)
-		return -EIO;
-
 	ret = s3c24xx_i2c_set_master(i2c);
 	if (ret != 0) {
 		dev_err(i2c->dev, "cannot get bus (error %d)\n", ret);
@@ -1246,7 +1242,7 @@ static int s3c24xx_i2c_suspend_noirq(struct device *dev)
 {
 	struct s3c24xx_i2c *i2c = dev_get_drvdata(dev);
 
-	i2c->suspended = 1;
+	i2c_mark_adapter_suspended(&i2c->adap, true);
 
 	if (!IS_ERR(i2c->sysreg))
 		regmap_read(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, &i2c->sys_i2c_cfg);
@@ -1267,7 +1263,7 @@ static int s3c24xx_i2c_resume_noirq(struct device *dev)
 		return ret;
 	s3c24xx_i2c_init(i2c);
 	clk_disable(i2c->clk);
-	i2c->suspended = 0;
+	i2c_mark_adapter_suspended(&i2c->adap, false);
 
 	return 0;
 }
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC/RFT 09/10] i2c: s3c2410: use core helper to mark adapter suspended

[Wolfram Sang]

Rejecting transfers should be handled by the core. Also, this will
ensure proper locking which was forgotten in this open coded version.

Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-s3c2410.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 2f2e28d60ef5..7c76edb25514 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -104,7 +104,6 @@ enum s3c24xx_i2c_state {
 struct s3c24xx_i2c {
 	wait_queue_head_t	wait;
 	kernel_ulong_t		quirks;
-	unsigned int		suspended:1;
 
 	struct i2c_msg		*msg;
 	unsigned int		msg_num;
@@ -703,9 +702,6 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
 	unsigned long timeout;
 	int ret;
 
-	if (i2c->suspended)
-		return -EIO;
-
 	ret = s3c24xx_i2c_set_master(i2c);
 	if (ret != 0) {
 		dev_err(i2c->dev, "cannot get bus (error %d)\n", ret);
@@ -1246,7 +1242,7 @@ static int s3c24xx_i2c_suspend_noirq(struct device *dev)
 {
 	struct s3c24xx_i2c *i2c = dev_get_drvdata(dev);
 
-	i2c->suspended = 1;
+	i2c_mark_adapter_suspended(&i2c->adap, true);
 
 	if (!IS_ERR(i2c->sysreg))
 		regmap_read(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, &i2c->sys_i2c_cfg);
@@ -1267,7 +1263,7 @@ static int s3c24xx_i2c_resume_noirq(struct device *dev)
 		return ret;
 	s3c24xx_i2c_init(i2c);
 	clk_disable(i2c->clk);
-	i2c->suspended = 0;
+	i2c_mark_adapter_suspended(&i2c->adap, false);
 
 	return 0;
 }
-- 
2.11.0

[RFC/RFT 10/10] i2c: rcar: add suspend/resume support

[Wolfram Sang]

Because the adapter will be set up before every transaction anyhow, we
just need to mark it as adapted to the I2C core.

Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 254e6219e538..30cff066e0da 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -1017,10 +1017,35 @@ static int rcar_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int rcar_i2c_suspend(struct device *dev)
+{
+	struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
+
+	i2c_mark_adapter_suspended(&priv->adap, true);
+	return 0;
+}
+
+static int rcar_i2c_resume(struct device *dev)
+{
+	struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
+
+	i2c_mark_adapter_suspended(&priv->adap, false);
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(rcar_i2c_pm_ops, rcar_i2c_suspend, rcar_i2c_resume);
+
+#define DEV_PM_OPS (&rcar_i2c_pm_ops)
+#else
+#define DEV_PM_OPS NULL
+#endif /* CONFIG_PM_SLEEP */
+
 static struct platform_driver rcar_i2c_driver = {
 	.driver	= {
 		.name	= "i2c-rcar",
 		.of_match_table = rcar_i2c_dt_ids,
+		.pm	= DEV_PM_OPS,
 	},
 	.probe		= rcar_i2c_probe,
 	.remove		= rcar_i2c_remove,
-- 
2.11.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC/RFT 10/10] i2c: rcar: add suspend/resume support

[Wolfram Sang]

Because the adapter will be set up before every transaction anyhow, we
just need to mark it as adapted to the I2C core.

Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/i2c/busses/i2c-rcar.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 254e6219e538..30cff066e0da 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -1017,10 +1017,35 @@ static int rcar_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int rcar_i2c_suspend(struct device *dev)
+{
+	struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
+
+	i2c_mark_adapter_suspended(&priv->adap, true);
+	return 0;
+}
+
+static int rcar_i2c_resume(struct device *dev)
+{
+	struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
+
+	i2c_mark_adapter_suspended(&priv->adap, false);
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(rcar_i2c_pm_ops, rcar_i2c_suspend, rcar_i2c_resume);
+
+#define DEV_PM_OPS (&rcar_i2c_pm_ops)
+#else
+#define DEV_PM_OPS NULL
+#endif /* CONFIG_PM_SLEEP */
+
 static struct platform_driver rcar_i2c_driver = {
 	.driver	= {
 		.name	= "i2c-rcar",
 		.of_match_table = rcar_i2c_dt_ids,
+		.pm	= DEV_PM_OPS,
 	},
 	.probe		= rcar_i2c_probe,
 	.remove		= rcar_i2c_remove,
-- 
2.11.0

Re: [RFC/RFT 03/10] i2c: synquacer: remove unused is_suspended flag

[Ard Biesheuvel]

On Mon, 10 Dec 2018 at 22:03, Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> This flag was defined and checked but never set a value. Remove it.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  drivers/i2c/busses/i2c-synquacer.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c
> index 2184b7c3580e..d18b0941b71a 100644
> --- a/drivers/i2c/busses/i2c-synquacer.c
> +++ b/drivers/i2c/busses/i2c-synquacer.c
> @@ -144,8 +144,6 @@ struct synquacer_i2c {
>         u32                     timeout_ms;
>         enum i2c_state          state;
>         struct i2c_adapter      adapter;
> -
> -       bool                    is_suspended;
>  };
>
>  static inline int is_lastmsg(struct synquacer_i2c *i2c)
> @@ -316,9 +314,6 @@ static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
>         unsigned long timeout;
>         int ret;
>
> -       if (i2c->is_suspended)
> -               return -EBUSY;
> -
>         synquacer_i2c_hw_init(i2c);
>         bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
>         if (bsr & SYNQUACER_I2C_BSR_BB) {
> --
> 2.11.0
>

Re: [RFC/RFT 10/10] i2c: rcar: add suspend/resume support

[Peter Rosin]

On 2018-12-10 22:03, Wolfram Sang wrote:
> Because the adapter will be set up before every transaction anyhow, we
> just need to mark it as adapted to the I2C core.

mark it as "suspended", not "adapted"?

Cheers,
Peter

> 
> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 254e6219e538..30cff066e0da 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -1017,10 +1017,35 @@ static int rcar_i2c_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int rcar_i2c_suspend(struct device *dev)
> +{
> +	struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
> +
> +	i2c_mark_adapter_suspended(&priv->adap, true);
> +	return 0;
> +}
> +
> +static int rcar_i2c_resume(struct device *dev)
> +{
> +	struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
> +
> +	i2c_mark_adapter_suspended(&priv->adap, false);
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rcar_i2c_pm_ops, rcar_i2c_suspend, rcar_i2c_resume);
> +
> +#define DEV_PM_OPS (&rcar_i2c_pm_ops)
> +#else
> +#define DEV_PM_OPS NULL
> +#endif /* CONFIG_PM_SLEEP */
> +
>  static struct platform_driver rcar_i2c_driver = {
>  	.driver	= {
>  		.name	= "i2c-rcar",
>  		.of_match_table = rcar_i2c_dt_ids,
> +		.pm	= DEV_PM_OPS,
>  	},
>  	.probe		= rcar_i2c_probe,
>  	.remove		= rcar_i2c_remove,
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC/RFT 10/10] i2c: rcar: add suspend/resume support

[Peter Rosin]

On 2018-12-10 22:03, Wolfram Sang wrote:
> Because the adapter will be set up before every transaction anyhow, we
> just need to mark it as adapted to the I2C core.

mark it as "suspended", not "adapted"?

Cheers,
Peter

> 
> Signed-off-by: Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@renesas.com>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/busses/i2c-rcar.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
> index 254e6219e538..30cff066e0da 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -1017,10 +1017,35 @@ static int rcar_i2c_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static int rcar_i2c_suspend(struct device *dev)
> +{
> +	struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
> +
> +	i2c_mark_adapter_suspended(&priv->adap, true);
> +	return 0;
> +}
> +
> +static int rcar_i2c_resume(struct device *dev)
> +{
> +	struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
> +
> +	i2c_mark_adapter_suspended(&priv->adap, false);
> +	return 0;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(rcar_i2c_pm_ops, rcar_i2c_suspend, rcar_i2c_resume);
> +
> +#define DEV_PM_OPS (&rcar_i2c_pm_ops)
> +#else
> +#define DEV_PM_OPS NULL
> +#endif /* CONFIG_PM_SLEEP */
> +
>  static struct platform_driver rcar_i2c_driver = {
>  	.driver	= {
>  		.name	= "i2c-rcar",
>  		.of_match_table = rcar_i2c_dt_ids,
> +		.pm	= DEV_PM_OPS,
>  	},
>  	.probe		= rcar_i2c_probe,
>  	.remove		= rcar_i2c_remove,
> 

Re: [RFC/RFT 01/10] i2c: add 'is_suspended' flag for i2c adapters

[Peter Rosin]

On 2018-12-10 22:02, Wolfram Sang wrote:
> Some drivers open code handling of suspended adapters. It should be
> handled by the core, though, to ensure generic handling. This patch adds
> the flag and an accessor function.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 1 +
>  include/linux/i2c.h         | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 28460f6a60cc..9f89e258c8ff 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1232,6 +1232,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>  	if (!adap->lock_ops)
>  		adap->lock_ops = &i2c_adapter_lock_ops;
>  
> +	adap->is_suspended = false;
>  	rt_mutex_init(&adap->bus_lock);
>  	rt_mutex_init(&adap->mux_lock);
>  	mutex_init(&adap->userspace_clients_lock);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..9852038ee3a7 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -680,6 +680,7 @@ struct i2c_adapter {
>  	int timeout;			/* in jiffies */
>  	int retries;
>  	struct device dev;		/* the adapter device */
> +	unsigned int is_suspended:1;	/* owned by the I2C core */

When more stuff is added to this bit field (which always happens at
some point) updates to all members of the bit field will have to use
the same root-adapter-locking, or we will suffer from RMW-races. So
this feels like an invitation for future disaster. Maybe a comment
about that to remind our future selves? Or perhaps the bit field
should be avoided altogether?

Cheers,
Peter

>  
>  	int nr;
>  	char name[48];
> @@ -762,6 +763,14 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
>  	adapter->lock_ops->unlock_bus(adapter, flags);
>  }
>  
> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap,
> +					      bool suspended)
> +{
> +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +	adap->is_suspended = suspended;
> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +}
> +
>  /*flags for the client struct: */
>  #define I2C_CLIENT_PEC		0x04	/* Use Packet Error Checking */
>  #define I2C_CLIENT_TEN		0x10	/* we have a ten bit chip address */
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC/RFT 01/10] i2c: add 'is_suspended' flag for i2c adapters

[Peter Rosin]

On 2018-12-10 22:02, Wolfram Sang wrote:
> Some drivers open code handling of suspended adapters. It should be
> handled by the core, though, to ensure generic handling. This patch adds
> the flag and an accessor function.
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
>  drivers/i2c/i2c-core-base.c | 1 +
>  include/linux/i2c.h         | 9 +++++++++
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 28460f6a60cc..9f89e258c8ff 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1232,6 +1232,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>  	if (!adap->lock_ops)
>  		adap->lock_ops = &i2c_adapter_lock_ops;
>  
> +	adap->is_suspended = false;
>  	rt_mutex_init(&adap->bus_lock);
>  	rt_mutex_init(&adap->mux_lock);
>  	mutex_init(&adap->userspace_clients_lock);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 65b4eaed1d96..9852038ee3a7 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -680,6 +680,7 @@ struct i2c_adapter {
>  	int timeout;			/* in jiffies */
>  	int retries;
>  	struct device dev;		/* the adapter device */
> +	unsigned int is_suspended:1;	/* owned by the I2C core */

When more stuff is added to this bit field (which always happens at
some point) updates to all members of the bit field will have to use
the same root-adapter-locking, or we will suffer from RMW-races. So
this feels like an invitation for future disaster. Maybe a comment
about that to remind our future selves? Or perhaps the bit field
should be avoided altogether?

Cheers,
Peter

>  
>  	int nr;
>  	char name[48];
> @@ -762,6 +763,14 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
>  	adapter->lock_ops->unlock_bus(adapter, flags);
>  }
>  
> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap,
> +					      bool suspended)
> +{
> +	i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +	adap->is_suspended = suspended;
> +	i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +}
> +
>  /*flags for the client struct: */
>  #define I2C_CLIENT_PEC		0x04	/* Use Packet Error Checking */
>  #define I2C_CLIENT_TEN		0x10	/* we have a ten bit chip address */
> 

Re: [RFC/RFT 05/10] i2c: zx2967: use core helper to mark adapter suspended

[Shawn Guo]

On Mon, Dec 10, 2018 at 10:03:03PM +0100, Wolfram Sang wrote:
> Rejecting transfers should be handled by the core. Also, this will
> ensure proper locking which was forgotten in this open coded version
> and make sure resume mark is set after enabling clocks (not before).
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Acked-by: Shawn Guo <shawnguo@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC/RFT 05/10] i2c: zx2967: use core helper to mark adapter suspended

[Shawn Guo]

On Mon, Dec 10, 2018 at 10:03:03PM +0100, Wolfram Sang wrote:
> Rejecting transfers should be handled by the core. Also, this will
> ensure proper locking which was forgotten in this open coded version
> and make sure resume mark is set after enabling clocks (not before).
> 
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Acked-by: Shawn Guo <shawnguo@kernel.org>

Re: [RFC/RFT 04/10] i2c: brcmstb: use core helper to mark adapter suspended

[Kamal Dasu]

On Mon, Dec 10, 2018 at 4:03 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
>
> Rejecting transfers should be handled by the core.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Reviewed-by: Kamal Dasu <kdasu.kdev@gmail.com>

> ---
>  drivers/i2c/busses/i2c-brcmstb.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
> index 826d32049996..05c822e5a3f6 100644
> --- a/drivers/i2c/busses/i2c-brcmstb.c
> +++ b/drivers/i2c/busses/i2c-brcmstb.c
> @@ -170,7 +170,6 @@ struct brcmstb_i2c_dev {
>         struct bsc_regs *bsc_regmap;
>         struct i2c_adapter adapter;
>         struct completion done;
> -       bool is_suspended;
>         u32 clk_freq_hz;
>         int data_regsz;
>  };
> @@ -467,9 +466,6 @@ static int brcmstb_i2c_xfer(struct i2c_adapter *adapter,
>         int xfersz = brcmstb_i2c_get_xfersz(dev);
>         u32 cond, cond_per_msg;
>
> -       if (dev->is_suspended)
> -               return -EBUSY;
> -
>         /* Loop through all messages */
>         for (i = 0; i < num; i++) {
>                 pmsg = &msgs[i];
> @@ -689,10 +685,7 @@ static int brcmstb_i2c_suspend(struct device *dev)
>  {
>         struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
>
> -       i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
> -       i2c_dev->is_suspended = true;
> -       i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
> -
> +       i2c_mark_adapter_suspended(&i2c_dev->adapter, true);
>         return 0;
>  }
>
> @@ -700,10 +693,8 @@ static int brcmstb_i2c_resume(struct device *dev)
>  {
>         struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
>
> -       i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
>         brcmstb_i2c_set_bsc_reg_defaults(i2c_dev);
> -       i2c_dev->is_suspended = false;
> -       i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
> +       i2c_mark_adapter_suspended(&i2c_dev->adapter, false);
>
>         return 0;
>  }
> --
> 2.11.0
>

Re: [RFC/RFT 00/10] i2c: move handling of suspended adapters to the core

[Hans de Goede]

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

Hi Wolfram,

On 10-12-18 22:02, Wolfram Sang wrote:
> Finally, here is the implementation Hans and I agreed on. Plus, all potential
> users I could spot already converted. Renesas R-Car driver was added on top.
> This series was tested on a Renesas Lager board (R-Car H2). I had to hack some
> error cases into the code to verify the workings. I couldn't create an error
> case otherwise, this is why further testing with more complex setups is very
> welcome.
> 
> For my taste, there is still too much boilerplate code for drivers left. Plus,
> it is also too easy to put it too early or too late. But I haven't come up with
> a better idea yet. And it is time to get this somehow forward.
> 
> Please comment, review, test... a branch can be found here:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/core-pm-helpers

So I've been testing this patch-set on some Intel devices with
an i2c-designware controller, combined with a patch to make the
suspend/resume methods of the controller call i2c_mark_adapter_suspended()

The results were ... interesting:

Take 1:
4.20-rc6 + your branch + i2c-designw-patch (+ some unrelated patches which are in my personal tree).
*i2c transfers no longer work*, because the controller runtime suspends and the pm_runtime_get_sync
to undo this is called from the driver's master_xfer function and the
is_suspended check happens before this.

Take 2:
Kernel from 1 with a patch added (attached) to make the core call
pm_runtime_get_sync from __i2c_transfer() if a flag is set +
i2c-designware changes to set this flag
*i2c transfers still do not work*, because __i2c_transfer is
called with the bus-lock held and the pm_runtime_get_sync calls
the adapters resume callback which calls i2c_mark_adapter_suspended()
which tries to take the bus-lock again -> deadlock

Take 3:
Kernel from 2 with the i2c-designware suspend/callback patches
modified to directly set adapter.is_suspended. To avoid the deadlock.
Ok, this works.  Note I've not tested reverting one of my recent
ACPI suspend ordering patches yet, which should actually trigger the
WARN_ON, I've ran out of steam just getting things to work with
the patches present.

I'm attaching 2 patches as I'm using them in Take 3.

Note that the i2c-sprd driver changes in your patchset also get close
to triggering this problem, but they dodge the bullet because there
are separate system-wide and runtime suspend handlers and only the
system-wide handlers call the new i2c_mark_adapter_suspended() function.

As I explain in the commit message of the first attached patch
simply always using split handlers is not really an option because
of adapter drivers using DPM_FLAG_SMART_PREPARE or
DPM_FLAG_SMART_SUSPEND.

So I'm afraid that this is way messier then I would like it to be,
we could go with my flag solution combined with not protecting
the is_suspended flag under the bus lock. That would make the
WARN_ON slightly racy, so we might fail to trigger the warning
under some rare circumstances, reverting to the old behavior of
continuing with the transfer on a suspended adapter, but I don't
think that is all that bad.

Regards,

Hans



> Wolfram Sang (10):
>    i2c: add 'is_suspended' flag for i2c adapters
>    i2c: reject new transfers when adapters are suspended
>    i2c: synquacer: remove unused is_suspended flag
>    i2c: brcmstb: use core helper to mark adapter suspended
>    i2c: zx2967: use core helper to mark adapter suspended
>    i2c: sprd: don't use pdev as variable name for struct device *
>    i2c: sprd: use core helper to mark adapter suspended
>    i2c: exynos5: use core helper to mark adapter suspended
>    i2c: s3c2410: use core helper to mark adapter suspended
>    i2c: rcar: add suspend/resume support
> 
>   drivers/i2c/busses/i2c-brcmstb.c   | 13 ++-----------
>   drivers/i2c/busses/i2c-exynos5.c   | 11 ++---------
>   drivers/i2c/busses/i2c-rcar.c      | 25 +++++++++++++++++++++++++
>   drivers/i2c/busses/i2c-s3c2410.c   |  8 ++------
>   drivers/i2c/busses/i2c-sprd.c      | 34 ++++++++++++----------------------
>   drivers/i2c/busses/i2c-synquacer.c |  5 -----
>   drivers/i2c/busses/i2c-zx2967.c    |  8 ++------
>   drivers/i2c/i2c-core-base.c        |  3 +++
>   include/linux/i2c.h                |  9 +++++++++
>   9 files changed, 57 insertions(+), 59 deletions(-)
> 

[-- Attachment #2: 0001-i2c-add-I2C_AQ_RUNTIME_PM-adapter-quirk-flag.patch --]
[-- Type: text/x-patch, Size: 3245 bytes --]

From 7781ff0b5033436c1d2740570364b1739017e03d Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 11 Dec 2018 19:19:17 +0100
Subject: [PATCH 1/2] i2c: add I2C_AQ_RUNTIME_PM adapter quirk flag

Add a new I2C_AQ_RUNTIME_PM adapter quirk flag, when this flag is set
then the i2c-core will all pm_runtime_get_sync() before calling an
adapter's master_xfer function and call pm_runtime_mark_last_busy() +
pm_runtime_put_autosuspend() after the master_xfer function.

Moving these runtime pm calls into the core for adapters which use them
is necessary for the WARN_ON(adap->is_suspended) to not trigger when
runtime-suspend is used. Another approach would be to only call
i2c_mark_adapter_suspended() from the adapter's regular suspend/resume
callbacks and not from the runtime-resume ones, but that would circumvent
the check also on system suspend when using DPM_FLAG_SMART_PREPARE or
DPM_FLAG_SMART_SUSPEND.

Note that of the 20 adapter drivers which call pm_runtime_get_sync() from
there master_xfer function, 16 call pm_runtime_put_autosuspend() when done.
i2c-nomadik.c and i2c-sh_mobile.c use pm_runtime_put_sync() and
i2c-tegra.c and i2c-rcar.c use pm_runtime_put(), so these will need to be
modified to use these new flag, or they will need another flag for their
special case.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/i2c-core-base.c | 16 ++++++++++++++--
 include/linux/i2c.h         |  2 ++
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 5b2078a902f8..acff1e4c09d9 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1866,12 +1866,18 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 
 	if (WARN_ON(!msgs || num < 1))
 		return -EINVAL;
-	if (WARN_ON(adap->is_suspended))
-		return -ESHUTDOWN;
 
 	if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
 		return -EOPNOTSUPP;
 
+	if (adap->quirks && (adap->quirks->flags & I2C_AQ_RUNTIME_PM))
+		pm_runtime_get_sync(adap->dev.parent);
+
+	if (WARN_ON(adap->is_suspended)) {
+		ret = -ESHUTDOWN;
+		goto out;
+	}
+
 	/*
 	 * i2c_trace_msg_key gets enabled when tracepoint i2c_transfer gets
 	 * enabled.  This is an efficient way of keeping the for-loop from
@@ -1904,6 +1910,12 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 		trace_i2c_result(adap, num, ret);
 	}
 
+out:
+	if (adap->quirks && (adap->quirks->flags & I2C_AQ_RUNTIME_PM)) {
+		pm_runtime_mark_last_busy(adap->dev.parent);
+		pm_runtime_put_autosuspend(adap->dev.parent);
+	}
+
 	return ret;
 }
 EXPORT_SYMBOL(__i2c_transfer);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 9852038ee3a7..96b9cac6c01e 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -661,6 +661,8 @@ struct i2c_adapter_quirks {
 #define I2C_AQ_NO_ZERO_LEN_READ		BIT(5)
 #define I2C_AQ_NO_ZERO_LEN_WRITE	BIT(6)
 #define I2C_AQ_NO_ZERO_LEN		(I2C_AQ_NO_ZERO_LEN_READ | I2C_AQ_NO_ZERO_LEN_WRITE)
+/* core must call pm_runtime_get_sync / put_autosuspend around master_xfer */
+#define I2C_AQ_RUNTIME_PM		BIT(7)
 
 /*
  * i2c_adapter is the structure used to identify a physical i2c bus along
-- 
2.19.2


[-- Attachment #3: 0002-i2c-designware-Set-is_suspended-flag-when-suspended.patch --]
[-- Type: text/x-patch, Size: 3804 bytes --]

From ba156b9419629a24fd2a82cc34d6d588585f637f Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 11 Dec 2018 15:29:44 +0100
Subject: [PATCH 2/2] i2c: designware: Set is_suspended flag when suspended

Set the is_suspended flag when suspended so that the i2c-core will warn
if we get called while the controller is (system-wide) suspended. Note
this also switches the runtime-pm handling in i2c-designware-master.c
to the new I2C_AQ_RUNTIME_PM flag, as this is required for the
is_suspended flag to work properly in combination with runtime-pm.

On most Intel Bay- and Cherry-Trail systems the PMIC is connected over I2C
and the PMIC is accessed through various means by the _PS0 and _PS3 ACPI
methods (power on / off methods) of various devices.

This leads to suspend/resume ordering problems where a device may be
resumed and get its _PS0 method executed before the I2C controller is
resumed. On Cherry Trail this leads to errors like these:

     i2c_designware 808622C1:06: controller timed out
     ACPI Error: AE_ERROR, Returned by Handler for [UserDefinedRegion]
     ACPI Error: Method parse/execution failed \_SB.P18W._ON, AE_ERROR
     video LNXVIDEO:00: Failed to change power state to D0

But on Bay Trail this caused I2C reads to seem to succeed, but they end
up returning wrong data, which ends up getting written back by the typical
read-modify-write cycle done to turn on various power-resources.

Debugging the problems caused by this silent data corruption is quite
nasty.

This commit properly marks the adapter as suspended until our resume
callback has run, triggering the i2c-core's WARN_ON when i2c_transfer gets
called on a suspended adapter.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-designware-master.c  | 11 ++---------
 drivers/i2c/busses/i2c-designware-platdrv.c |  4 ++++
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c
index 8d1bc44d2530..ebb2b8368f6f 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -424,8 +424,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
 
-	pm_runtime_get_sync(dev->dev);
-
 	reinit_completion(&dev->cmd_complete);
 	dev->msgs = msgs;
 	dev->msgs_num = num;
@@ -439,7 +437,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	ret = i2c_dw_acquire_lock(dev);
 	if (ret)
-		goto done_nolock;
+		return ret;
 
 	ret = i2c_dw_wait_bus_not_busy(dev);
 	if (ret < 0)
@@ -493,11 +491,6 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 done:
 	i2c_dw_release_lock(dev);
-
-done_nolock:
-	pm_runtime_mark_last_busy(dev->dev);
-	pm_runtime_put_autosuspend(dev->dev);
-
 	return ret;
 }
 
@@ -507,7 +500,7 @@ static const struct i2c_algorithm i2c_dw_algo = {
 };
 
 static const struct i2c_adapter_quirks i2c_dw_quirks = {
-	.flags = I2C_AQ_NO_ZERO_LEN,
+	.flags = I2C_AQ_NO_ZERO_LEN | I2C_AQ_RUNTIME_PM,
 };
 
 static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 9eaac3be1f63..e220336f34ae 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -454,6 +454,8 @@ static int dw_i2c_plat_suspend(struct device *dev)
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
+	i_dev->adapter.is_suspended = true;
+
 	if (i_dev->shared_with_punit)
 		return 0;
 
@@ -472,6 +474,8 @@ static int dw_i2c_plat_resume(struct device *dev)
 
 	i_dev->init(i_dev);
 
+	i_dev->adapter.is_suspended = false;
+
 	return 0;
 }
 
-- 
2.19.2

Re: [RFC/RFT 00/10] i2c: move handling of suspended adapters to the core

[Wolfram Sang]

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

Hi Hans,

thanks for testing this series!

> Note that the i2c-sprd driver changes in your patchset also get close
> to triggering this problem, but they dodge the bullet because there
> are separate system-wide and runtime suspend handlers and only the
> system-wide handlers call the new i2c_mark_adapter_suspended() function.

Yes, this what I assumed to be all around - seperate handlers for RPM
and PM. And I also assumed that they could be seperated if they aren't
already. Until I read...

> As I explain in the commit message of the first attached patch
> simply always using split handlers is not really an option because
> of adapter drivers using DPM_FLAG_SMART_PREPARE or
> DPM_FLAG_SMART_SUSPEND.

... this. I don't know what these flags do (and reading SMART in there
gives me more a 'uh-oh' feeling) but if the handlers can't be split, the
issues you were seeing are a consequence, yes.

For me, this sadly spoils the whole concept. The patches you add make
things even more complicated. Not happy about that.

Looking at the open coded version you did for the designware driver, I
wonder now if it is better to just leave it at driver level? Need to
sleep over it, though.

Thanks again,

   Wolfram


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

Re: [RFC/RFT 00/10] i2c: move handling of suspended adapters to the core

[Hans de Goede]

Hi,

On 12-12-18 00:41, Wolfram Sang wrote:
> Hi Hans,
> 
> thanks for testing this series!
> 
>> Note that the i2c-sprd driver changes in your patchset also get close
>> to triggering this problem, but they dodge the bullet because there
>> are separate system-wide and runtime suspend handlers and only the
>> system-wide handlers call the new i2c_mark_adapter_suspended() function.
> 
> Yes, this what I assumed to be all around - seperate handlers for RPM
> and PM. And I also assumed that they could be seperated if they aren't
> already. Until I read...
> 
>> As I explain in the commit message of the first attached patch
>> simply always using split handlers is not really an option because
>> of adapter drivers using DPM_FLAG_SMART_PREPARE or
>> DPM_FLAG_SMART_SUSPEND.
> 
> ... this. I don't know what these flags do (and reading SMART in there
> gives me more a 'uh-oh' feeling)

On x86 the lines between runtime suspend and system-suspend are blurring
with technologies like "connected standby" and in general devices moving
to suspend2idle as system-suspend state, I guess this also applies to
modern smartphone platforms but I'm not following those closely.

What this means on x86 is that the firmware is not doing any suspend
handling anymore and the OS / device-drivers get to do all the suspend.
For many devices this means that if they are runtime-suspended, and there
is no difference between the runtime and system suspend handling at the
driver level, they can be left as is, so during a system suspend they are
not touched at all (and left runtime suspended during system resume).

The "SMART" bit is really not all that smart, SMART_PREPARE means that
the drivers pm prepare callback will return positive non 0 (e.g. 1) to
indicate that the device may not be kept in its runtime suspended
state when transitioning to system-suspend, otoh when the prepare
callback returns 0 and the SMART_PREPARE flag is set then *all* suspend/
resume handling can be skipped during a system suspend.

SMART_SUSPEND means that if the device is runtime-suspended it may be left
as such, this only gets checked if the driver either does not have the
SMART_PREPARE flag, or the prepare callback indicated that skipping the
entire suspend/resume handling is not ok.

So this is not that scary, but it does require driver authors to know
what they are doing...

> but if the handlers can't be split, the
> issues you were seeing are a consequence, yes.
> 
> For me, this sadly spoils the whole concept. The patches you add make
> things even more complicated. Not happy about that.

Agreed, they don't fill me with happiness either.

> Looking at the open coded version you did for the designware driver, I
> wonder now if it is better to just leave it at driver level? Need to
> sleep over it, though.

I myself was thinking in the same direction (leave the entire suspended
check at the driver level).

Regards,

Hans

Re: [RFC/RFT 08/10] i2c: exynos5: use core helper to mark adapter suspended

[Marek Szyprowski]

Hi,

On 2018-12-10 22:03, Wolfram Sang wrote:
> Rejecting transfers should be handled by the core. Also, this will
> ensure proper locking which was forgotten in this open coded version.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Works fine on TM2e and OdroidXU3. The only minor issue is the following
warning:

WARNING: CPU: 4 PID: 28 at drivers/i2c/i2c-core-base.c:1869
__i2c_transfer+0x428/0x5e0
Modules linked in:
CPU: 4 PID: 28 Comm: cpuhp/4 Tainted: G        W        
4.20.0-rc1-00031-ga1c77790d03f #5130
Hardware name: Samsung TM2E board (DT)
pstate: 20000005 (nzCv daif -PAN -UAO)
pc : __i2c_transfer+0x428/0x5e0
lr : i2c_transfer+0x64/0xb8
...
Call trace:
 __i2c_transfer+0x428/0x5e0
 i2c_transfer+0x64/0xb8
 regmap_i2c_read+0x5c/0xa0
 _regmap_raw_read+0xac/0x2a8
 _regmap_bus_read+0x3c/0x70
 _regmap_read+0x64/0x1a0
 regmap_read+0x48/0x70
 regulator_is_enabled_regmap+0x38/0xb8
 _regulator_is_enabled.part.1+0x1c/0x30
 create_regulator+0x264/0x270
 _regulator_get+0x90/0x2b0
 regulator_get_optional+0x10/0x18
 dev_pm_opp_set_regulators+0xb8/0x1e8
 cpufreq_init+0xd0/0x2f8
 cpufreq_online+0xc0/0x660
 cpuhp_cpufreq_online+0xc/0x18
 cpuhp_invoke_callback+0xac/0x7b0
 cpuhp_thread_fun+0xec/0x180
 smpboot_thread_fn+0x1f0/0x290
 kthread+0xfc/0x128
 ret_from_fork+0x10/0x1c
---[ end trace bea5412bc21deb81 ]---

Similar warning was already there ('HS-I2C is not initialized' message,
without backtrace) during system suspend/resume. It looks that cpu freq
is resumed too early, before any device drivers, but this is not an
issue for this patchset.

> ---
>  drivers/i2c/busses/i2c-exynos5.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> index c1ce2299a76e..fe2f2031ea36 100644
> --- a/drivers/i2c/busses/i2c-exynos5.c
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -183,7 +183,6 @@ enum i2c_type_exynos {
>  
>  struct exynos5_i2c {
>  	struct i2c_adapter	adap;
> -	unsigned int		suspended:1;
>  
>  	struct i2c_msg		*msg;
>  	struct completion	msg_complete;
> @@ -715,11 +714,6 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
>  	struct exynos5_i2c *i2c = adap->algo_data;
>  	int i, ret;
>  
> -	if (i2c->suspended) {
> -		dev_err(i2c->dev, "HS-I2C is not initialized.\n");
> -		return -EIO;
> -	}
> -
>  	ret = clk_enable(i2c->clk);
>  	if (ret)
>  		return ret;
> @@ -847,8 +841,7 @@ static int exynos5_i2c_suspend_noirq(struct device *dev)
>  {
>  	struct exynos5_i2c *i2c = dev_get_drvdata(dev);
>  
> -	i2c->suspended = 1;
> -
> +	i2c_mark_adapter_suspended(&i2c->adap, true);
>  	clk_unprepare(i2c->clk);
>  
>  	return 0;
> @@ -871,7 +864,7 @@ static int exynos5_i2c_resume_noirq(struct device *dev)
>  
>  	exynos5_i2c_init(i2c);
>  	clk_disable(i2c->clk);
> -	i2c->suspended = 0;
> +	i2c_mark_adapter_suspended(&i2c->adap, false);
>  
>  	return 0;
>  }

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [RFC/RFT 09/10] i2c: s3c2410: use core helper to mark adapter suspended

[Marek Szyprowski]

Hi,

On 2018-12-10 22:03, Wolfram Sang wrote:
> Rejecting transfers should be handled by the core. Also, this will
> ensure proper locking which was forgotten in this open coded version.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Works fine on Odroid U3.

> ---
>  drivers/i2c/busses/i2c-s3c2410.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index 2f2e28d60ef5..7c76edb25514 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -104,7 +104,6 @@ enum s3c24xx_i2c_state {
>  struct s3c24xx_i2c {
>  	wait_queue_head_t	wait;
>  	kernel_ulong_t		quirks;
> -	unsigned int		suspended:1;
>  
>  	struct i2c_msg		*msg;
>  	unsigned int		msg_num;
> @@ -703,9 +702,6 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
>  	unsigned long timeout;
>  	int ret;
>  
> -	if (i2c->suspended)
> -		return -EIO;
> -
>  	ret = s3c24xx_i2c_set_master(i2c);
>  	if (ret != 0) {
>  		dev_err(i2c->dev, "cannot get bus (error %d)\n", ret);
> @@ -1246,7 +1242,7 @@ static int s3c24xx_i2c_suspend_noirq(struct device *dev)
>  {
>  	struct s3c24xx_i2c *i2c = dev_get_drvdata(dev);
>  
> -	i2c->suspended = 1;
> +	i2c_mark_adapter_suspended(&i2c->adap, true);
>  
>  	if (!IS_ERR(i2c->sysreg))
>  		regmap_read(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, &i2c->sys_i2c_cfg);
> @@ -1267,7 +1263,7 @@ static int s3c24xx_i2c_resume_noirq(struct device *dev)
>  		return ret;
>  	s3c24xx_i2c_init(i2c);
>  	clk_disable(i2c->clk);
> -	i2c->suspended = 0;
> +	i2c_mark_adapter_suspended(&i2c->adap, false);
>  
>  	return 0;
>  }

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [RFC/RFT 08/10] i2c: exynos5: use core helper to mark adapter suspended

[Wolfram Sang]

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


> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks for testing!

> Similar warning was already there ('HS-I2C is not initialized' message,
> without backtrace) during system suspend/resume. It looks that cpu freq
> is resumed too early, before any device drivers, but this is not an
> issue for this patchset.

Yes, this is actually a desired outcome :) Raise awareness if the
suspend/resume ordering has issues.


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

Re: [RFC/RFT 00/10] i2c: move handling of suspended adapters to the core

[Wolfram Sang]

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

Hans, all,

> > ... this. I don't know what these flags do (and reading SMART in there
> > gives me more a 'uh-oh' feeling)
> 
> On x86 the lines between runtime suspend and system-suspend are blurring
> with technologies like "connected standby" and in general devices moving
> to suspend2idle as system-suspend state, I guess this also applies to
> modern smartphone platforms but I'm not following those closely.

I'd guess so, too. I am not aware of any existing mechanism for that at
the moment, though. If somebody does, please enlighten us.

> The "SMART" bit is really not all that smart, SMART_PREPARE means that
> the drivers pm prepare callback will return positive non 0 (e.g. 1) to
> indicate that the device may not be kept in its runtime suspended
> state when transitioning to system-suspend, otoh when the prepare
> callback returns 0 and the SMART_PREPARE flag is set then *all* suspend/
> resume handling can be skipped during a system suspend.

Thanks for the detailed explanation! Much appreciated.

> > Looking at the open coded version you did for the designware driver, I
> > wonder now if it is better to just leave it at driver level? Need to
> > sleep over it, though.
> 
> I myself was thinking in the same direction (leave the entire suspended
> check at the driver level).

So, I was giving it some more thoughts, and my feeling is to still apply
this series with the review comments addressed. Plus, clearly mark the
new 'is_suspended' flag and the helper function as *optional* to allow
for driver specific solutions as well. The then-to-be-added
documentation would state that it is mostly useful for seperated system
suspend and runtime suspend. For more complex situations, custom
solutions are accepted, too. Which means your patch for designware
should be added to the series.

My take is there are enough drivers out there already which can benefit
from this new helper. If it turns out to be useless somewhen in the
future, we can still remove it.

What do you (and all others, of course) think?

Thanks,

   Wolfram

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

Re: [RFC/RFT 00/10] i2c: move handling of suspended adapters to the core

[Hans de Goede]

Hi,

On 18-12-18 21:17, Wolfram Sang wrote:
> Hans, all,
> 
>>> ... this. I don't know what these flags do (and reading SMART in there
>>> gives me more a 'uh-oh' feeling)
>>
>> On x86 the lines between runtime suspend and system-suspend are blurring
>> with technologies like "connected standby" and in general devices moving
>> to suspend2idle as system-suspend state, I guess this also applies to
>> modern smartphone platforms but I'm not following those closely.
> 
> I'd guess so, too. I am not aware of any existing mechanism for that at
> the moment, though. If somebody does, please enlighten us.
> 
>> The "SMART" bit is really not all that smart, SMART_PREPARE means that
>> the drivers pm prepare callback will return positive non 0 (e.g. 1) to
>> indicate that the device may not be kept in its runtime suspended
>> state when transitioning to system-suspend, otoh when the prepare
>> callback returns 0 and the SMART_PREPARE flag is set then *all* suspend/
>> resume handling can be skipped during a system suspend.
> 
> Thanks for the detailed explanation! Much appreciated.
> 
>>> Looking at the open coded version you did for the designware driver, I
>>> wonder now if it is better to just leave it at driver level? Need to
>>> sleep over it, though.
>>
>> I myself was thinking in the same direction (leave the entire suspended
>> check at the driver level).
> 
> So, I was giving it some more thoughts, and my feeling is to still apply
> this series with the review comments addressed. Plus, clearly mark the
> new 'is_suspended' flag and the helper function as *optional* to allow
> for driver specific solutions as well. The then-to-be-added
> documentation would state that it is mostly useful for seperated system
> suspend and runtime suspend. For more complex situations, custom
> solutions are accepted, too. Which means your patch for designware
> should be added to the series.
> 
> My take is there are enough drivers out there already which can benefit
> from this new helper. If it turns out to be useless somewhen in the
> future, we can still remove it.
> 
> What do you (and all others, of course) think?

I've been thinking along the same lines: your series for the drivers
with separate runtime and system suspend handlers, "custom" driver
specific code for troublesome cases like i2c-designware.

Do you want me to send out a new version of my patch for the i2c-designware
code?

Regards,

Hans

Re: [RFC/RFT 00/10] i2c: move handling of suspended adapters to the core

[Wolfram Sang]

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


> I've been thinking along the same lines: your series for the drivers
> with separate runtime and system suspend handlers, "custom" driver
> specific code for troublesome cases like i2c-designware.

Cool, thanks!

> Do you want me to send out a new version of my patch for the i2c-designware
> code?

Yes, please do.


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

Re: [RFC/RFT 01/10] i2c: add 'is_suspended' flag for i2c adapters

[Wolfram Sang]

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


> > +	unsigned int is_suspended:1;	/* owned by the I2C core */
> 
> When more stuff is added to this bit field (which always happens at
> some point) updates to all members of the bit field will have to use
> the same root-adapter-locking, or we will suffer from RMW-races. So
> this feels like an invitation for future disaster. Maybe a comment
> about that to remind our future selves? Or perhaps the bit field
> should be avoided altogether?

Changed to bool. Thanks!


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

Re: [RFC/RFT 10/10] i2c: rcar: add suspend/resume support

[Wolfram Sang]

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


> > Because the adapter will be set up before every transaction anyhow, we
> > just need to mark it as adapted to the I2C core.
> 
> mark it as "suspended", not "adapted"?

Sure thing :D Thanks!


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

Re: [RFC/RFT 01/10] i2c: add 'is_suspended' flag for i2c adapters

[Geert Uytterhoeven]

Hi Wolfram,

On Wed, Dec 19, 2018 at 12:34 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > +   unsigned int is_suspended:1;    /* owned by the I2C core */
> >
> > When more stuff is added to this bit field (which always happens at
> > some point) updates to all members of the bit field will have to use
> > the same root-adapter-locking, or we will suffer from RMW-races. So
> > this feels like an invitation for future disaster. Maybe a comment
> > about that to remind our future selves? Or perhaps the bit field
> > should be avoided altogether?
>
> Changed to bool. Thanks!

Does that help, given bool is smaller than the CPUs word size?
Is it Alpha that cannot do atomic operations on bytes?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC/RFT 01/10] i2c: add 'is_suspended' flag for i2c adapters

[Wolfram Sang]

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

On Wed, Dec 19, 2018 at 10:39:05AM +0100, Geert Uytterhoeven wrote:
> Hi Wolfram,
> 
> On Wed, Dec 19, 2018 at 12:34 AM Wolfram Sang <wsa@the-dreams.de> wrote:
> > > > +   unsigned int is_suspended:1;    /* owned by the I2C core */
> > >
> > > When more stuff is added to this bit field (which always happens at
> > > some point) updates to all members of the bit field will have to use
> > > the same root-adapter-locking, or we will suffer from RMW-races. So
> > > this feels like an invitation for future disaster. Maybe a comment
> > > about that to remind our future selves? Or perhaps the bit field
> > > should be avoided altogether?
> >
> > Changed to bool. Thanks!
> 
> Does that help, given bool is smaller than the CPUs word size?
> Is it Alpha that cannot do atomic operations on bytes?

Yup, I overestimated bools :( I guess good old

unsigned long locked_flags;
#define <PREFIX>_IS_SUSPENDED	0

set_bit(), clear_bit(), and test_bit() is it then.


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