i2c: Don't start transfers when suspended

We now take care of suspension in the i2c core code. So we can remove this check here.

Signed-off-by Bastian Hecht <[email protected]>

---
same as v1

 drivers/i2c/busses/i2c-exynos5.c | 20 +-------------------
 1 file changed, 1 insertion(+), 19 deletions(-)

drivers/i2c/busses/i2c-exynos5.c +0 -16

index 63d2292..a80cf28 100644
@@ -145,7 +145,6 @@
 
 struct exynos5_i2c {
        struct i2c_adapter      adap;
-       unsigned int            suspended:1;
 
        struct i2c_msg          *msg;
        struct completion       msg_complete;

@@ -610,11 +609,6 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
        struct exynos5_i2c *i2c = adap->algo_data;
        int i = 0, ret = 0, stop = 0;
 
-       if (i2c->suspended) {
-               dev_err(i2c->dev, "HS-I2C is not initialized.\n");
-               return -EIO;
-       }
-
        clk_prepare_enable(i2c->clk);
 
        for (i = 0; i < num; i++, msgs++) {

@@ -757,16 +751,6 @@ static int exynos5_i2c_remove(struct platform_device *pdev)
 }
 
 #ifdef CONFIG_PM_SLEEP
-static int exynos5_i2c_suspend_noirq(struct device *dev)
-{
-       struct platform_device *pdev = to_platform_device(dev);
-       struct exynos5_i2c *i2c = platform_get_drvdata(pdev);
-
-       i2c->suspended = 1;
-
-       return 0;
-}
-
 static int exynos5_i2c_resume_noirq(struct device *dev)
 {
        struct platform_device *pdev = to_platform_device(dev);

@@ -783,14 +767,12 @@ static int exynos5_i2c_resume_noirq(struct device *dev)
 
        exynos5_i2c_init(i2c);

drivers/i2c/busses/i2c-exynos5.c +0 -1

 static struct platform_driver exynos5_i2c_driver = {
        .probe          = exynos5_i2c_probe,
--

1.9.1

#ifdef CONFIG_PM_SLEEP

drivers/i2c/busses/i2c-tegra.c +0 -11

-static int tegra_i2c_suspend(struct device *dev)
-{
-	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
-
-	i2c_lock_adapter(&i2c_dev->adapter);
-	i2c_dev->is_suspended = true;
-	i2c_unlock_adapter(&i2c_dev->adapter);
-
-	return 0;
-}
-
 static int tegra_i2c_resume(struct device *dev)
 {
 	struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);

@@ -842,14 +827,12 @@ static int tegra_i2c_resume(struct device *dev)
 		return ret;
 	}

[...]

Quoted 26 lines

#ifdef CONFIG_PM_SLEEP

drivers/i2c/busses/i2c-tegra.c +0 -13

-static int tegra_i2c_suspend(struct device *dev)
-{
-     struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);
-
-     i2c_lock_adapter(&i2c_dev->adapter);
-     i2c_dev->is_suspended = true;
-     i2c_unlock_adapter(&i2c_dev->adapter);
-
-     return 0;
-}
-
 static int tegra_i2c_resume(struct device *dev)
 {
      struct tegra_i2c_dev *i2c_dev = dev_get_drvdata(dev);

@@ -842,14 +827,12 @@ static int tegra_i2c_resume(struct device *dev)
              return ret;
      }
 
-     i2c_dev->is_suspended = false;
-
      i2c_unlock_adapter(&i2c_dev->adapter);
 
      return 0;
 }
 

drivers/i2c/busses/i2c-tegra.c static int tegra_i2c_resume(struct device *dev) · line 835

Quoted 2 lines

drivers/i2c/busses/i2c-tegra.c +1 -1

-static SIMPLE_DEV_PM_OPS(tegra_i2c_pm, tegra_i2c_suspend, tegra_i2c_resume);
+static SIMPLE_DEV_PM_OPS(tegra_i2c_pm, tegra_i2c_resume);

Shouldn't this be:

        static SIMPLE_DEV_OPS(tegra_i2c_pm, NULL, tegra_i2c_resume);

instead?

Thierry

i2c transfer requests come in very uncontrolled, like from interrupt routines. We might be suspended when this happens. To avoid i2c timeouts caused by powered down busses we check for suspension.

Several bus drivers handle this problem on their own. We can clean things up by moving the protection mechanism into the core.

Signed-off-by Bastian Hecht <[email protected]>

---
changelog v2:

- commit message extended.
- initialization added for adap->suspended
- swapped branch for increased performance

 drivers/i2c/i2c-core.c | 25 ++++++++++++++++++++++++-
 include/linux/i2c.h    |  1 +
 2 files changed, 25 insertions(+), 1 deletion(-)

drivers/i2c/i2c-core.c +24 -1

index 7c7f4b8..b15dc20 100644
@@ -343,6 +343,13 @@ static int i2c_legacy_resume(struct device *dev)
 static int i2c_device_pm_suspend(struct device *dev)
 {
        const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+       struct i2c_adapter *adap = i2c_verify_adapter(dev);
+
+       if (adap) {
+               i2c_lock_adapter(adap);
+               adap->suspended = true;
+               i2c_unlock_adapter(adap);
+       }
 
        if (pm)
                return pm_generic_suspend(dev);

@@ -353,6 +360,13 @@ static int i2c_device_pm_suspend(struct device *dev)
 static int i2c_device_pm_resume(struct device *dev)
 {
        const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+       struct i2c_adapter *adap = i2c_verify_adapter(dev);
+
+       if (adap) {
+               i2c_lock_adapter(adap);
+               adap->suspended = false;
+               i2c_unlock_adapter(adap);
+       }
 
        if (pm)
                return pm_generic_resume(dev);

@@ -1243,6 +1257,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
        dev_set_name(&adap->dev, "i2c-%d", adap->nr);
        adap->dev.bus = &i2c_bus_type;
        adap->dev.type = &i2c_adapter_type;
+       adap->suspended = false;
        res = device_register(&adap->dev);
        if (res)
                goto out_list;

@@ -1819,7 +1834,10 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
                        i2c_lock_adapter(adap);
                }
 
-               ret = __i2c_transfer(adap, msgs, num);
+               if (!adap->suspended)
+                       ret = __i2c_transfer(adap, msgs, num);
+               else
+                       ret = -EIO;
                i2c_unlock_adapter(adap);
 
                return ret;

@@ -2577,6 +2595,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
 
        if (adapter->algo->smbus_xfer) {
                i2c_lock_adapter(adapter);
+               if (adapter->suspended) {
+                       res = -EIO;
+                       goto unlock;
+               }
 
                /* Retry automatically on arbitration loss */
                orig_jiffies = jiffies;

@@ -2590,6 +2612,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
                                       orig_jiffies + adapter->timeout))
                                break;
                }
+unlock:
                i2c_unlock_adapter(adapter);
 
                if (res != -EOPNOTSUPP || !adapter->algo->master_xfer)

include/linux/i2c.h +1 -1

index b556e0a..af08c75 100644
@@ -434,6 +434,7 @@ struct i2c_adapter {
        int timeout;                    /* in jiffies */
        int retries;
        struct device dev;              /* the adapter device */
+       unsigned int suspended:1;
 
        int nr;
        char name[48];
--

1.9.1

I want to add the remark that I can't help further on the patch series in the next month about. So this is free to be taken/ignored/modified without any approval by my side.

i2c transfer requests come in very uncontrolled, like from interrupt routines. We might be suspended when this happens. To avoid i2c timeouts caused by powered down busses we check for suspension.

Several bus drivers handle this problem on their own. We can clean things up by moving the protection mechanism into the core.

Signed-off-by Bastian Hecht <[email protected]>

---
changelog v2:

- commit message extended.
- initialization added for adap->suspended
- swapped branch for increased performance

  drivers/i2c/i2c-core.c | 25 ++++++++++++++++++++++++-
  include/linux/i2c.h    |  1 +
  2 files changed, 25 insertions(+), 1 deletion(-)

drivers/i2c/i2c-core.c +24 -1

index 7c7f4b8..b15dc20 100644
@@ -343,6 +343,13 @@ static int i2c_legacy_resume(struct device *dev)
  static int i2c_device_pm_suspend(struct device *dev)
  {
  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	struct i2c_adapter *adap = i2c_verify_adapter(dev);
+
+	if (adap) {
+		i2c_lock_adapter(adap);
+		adap->suspended = true;
+		i2c_unlock_adapter(adap);
+	}
 
  	if (pm)
  		return pm_generic_suspend(dev);

@@ -353,6 +360,13 @@ static int i2c_device_pm_suspend(struct device *dev)
  static int i2c_device_pm_resume(struct device *dev)
  {
  	const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
+	struct i2c_adapter *adap = i2c_verify_adapter(dev);
+
+	if (adap) {
+		i2c_lock_adapter(adap);
+		adap->suspended = false;
+		i2c_unlock_adapter(adap);
+	}
 
  	if (pm)
  		return pm_generic_resume(dev);

@@ -1243,6 +1257,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
  	dev_set_name(&adap->dev, "i2c-%d", adap->nr);
  	adap->dev.bus = &i2c_bus_type;
  	adap->dev.type = &i2c_adapter_type;
+	adap->suspended = false;
  	res = device_register(&adap->dev);
  	if (res)
  		goto out_list;

@@ -1819,7 +1834,10 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
  			i2c_lock_adapter(adap);
  		}
 
-		ret = __i2c_transfer(adap, msgs, num);
+		if (!adap->suspended)
+			ret = __i2c_transfer(adap, msgs, num);
+		else
+			ret = -EIO;
  		i2c_unlock_adapter(adap);
 
  		return ret;

@@ -2577,6 +2595,10 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
 
  	if (adapter->algo->smbus_xfer) {
  		i2c_lock_adapter(adapter);
+		if (adapter->suspended) {
+			res = -EIO;
+			goto unlock;
+		}
 
  		/* Retry automatically on arbitration loss */
  		orig_jiffies = jiffies;

@@ -2590,6 +2612,7 @@ s32 i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr, unsigned short flags,
  				       orig_jiffies + adapter->timeout))
  				break;
  		}
+unlock:
  		i2c_unlock_adapter(adapter);
 
  		if (res != -EOPNOTSUPP || !adapter->algo->master_xfer)

include/linux/i2c.h +0 -0

index b556e0a..af08c75 100644

I'm not sure, this optimization is safe (
Because, in many cases the access to PMIC IC needs to be allowed till late stages of suspending (at least till suspend_noirq stage or even later). For example, on some OMAP SoC Voltage management code need to use services provided by PMIC IC, which is connected to I2C.

i2c transfer requests come in very uncontrolled, like from interrupt routines. We might be suspended when this happens. To avoid i2c timeouts caused by powered down busses we check for suspension.

Several bus drivers handle this problem on their own. We can clean things up by moving the protection mechanism into the core.

Signed-off-by Bastian Hecht <[email protected]>