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(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 7c7f4b8..b15dc20 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -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)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b556e0a..af08c75 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -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
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(-)
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index 63d2292..a80cf28 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -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);
clk_disable_unprepare(i2c->clk);
- i2c->suspended = 0;
return 0;
}
#endif
-static SIMPLE_DEV_PM_OPS(exynos5_i2c_dev_pm_ops, exynos5_i2c_suspend_noirq,
- exynos5_i2c_resume_noirq);
+static SIMPLE_DEV_PM_OPS(exynos5_i2c_dev_pm_ops, exynos5_i2c_resume_noirq);
static struct platform_driver exynos5_i2c_driver = {
.probe = exynos5_i2c_probe,
--
1.9.1
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-eg20t.c | 15 ---------------
1 file changed, 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
index a44ea13..aae0413 100644
--- a/drivers/i2c/busses/i2c-eg20t.c
+++ b/drivers/i2c/busses/i2c-eg20t.c
@@ -162,15 +162,12 @@ struct i2c_algo_pch_data {
* struct adapter_info - This structure holds the adapter information for the
PCH i2c controller
* @pch_data: stores a list of i2c_algo_pch_data
- * @pch_i2c_suspended: specifies whether the system is suspended or not
- * perhaps with more lines and words.
* @ch_num: specifies the number of i2c instance
*
* pch_data has as many elements as maximum I2C channels
*/
struct adapter_info {
struct i2c_algo_pch_data pch_data[PCH_I2C_MAX_DEV];
- bool pch_i2c_suspended;
int ch_num;
};
@@ -677,13 +674,6 @@ static s32 pch_i2c_xfer(struct i2c_adapter *i2c_adap,
if (ret)
return ret;
- if (adap->p_adapter_info->pch_i2c_suspended) {
- mutex_unlock(&pch_mutex);
- return -EBUSY;
- }
-
- pch_dbg(adap, "adap->p_adapter_info->pch_i2c_suspended is %d\n",
- adap->p_adapter_info->pch_i2c_suspended);
/* transfer not completed */
adap->pch_i2c_xfer_in_progress = true;
@@ -786,7 +776,6 @@ static int pch_i2c_probe(struct pci_dev *pdev,
for (i = 0; i < adap_info->ch_num; i++) {
pch_adap = &adap_info->pch_data[i].pch_adapter;
- adap_info->pch_i2c_suspended = false;
adap_info->pch_data[i].p_adapter_info = adap_info;
@@ -862,8 +851,6 @@ static int pch_i2c_suspend(struct pci_dev *pdev, pm_message_t state)
struct adapter_info *adap_info = pci_get_drvdata(pdev);
void __iomem *p = adap_info->pch_data[0].pch_base_address;
- adap_info->pch_i2c_suspended = true;
-
for (i = 0; i < adap_info->ch_num; i++) {
while ((adap_info->pch_data[i].pch_i2c_xfer_in_progress)) {
/* Wait until all channel transfers are completed */
@@ -912,8 +899,6 @@ static int pch_i2c_resume(struct pci_dev *pdev)
for (i = 0; i < adap_info->ch_num; i++)
pch_i2c_init(&adap_info->pch_data[i]);
- adap_info->pch_i2c_suspended = false;
-
return 0;
}
#else
--
1.9.1
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-tegra.c | 19 +------------------
1 file changed, 1 insertion(+), 18 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index f1bb2fc..c279d85 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -172,7 +172,6 @@ struct tegra_i2c_dev {
size_t msg_buf_remaining;
int msg_read;
u32 bus_clk_rate;
- bool is_suspended;
};
static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
@@ -628,9 +627,6 @@ static int tegra_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
int i;
int ret = 0;
- if (i2c_dev->is_suspended)
- return -EBUSY;
-
ret = tegra_i2c_clock_enable(i2c_dev);
if (ret < 0) {
dev_err(i2c_dev->dev, "Clock enable failed %d\n", ret);
@@ -817,17 +813,6 @@ static int tegra_i2c_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_SLEEP
-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;
}
-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);
#define TEGRA_I2C_PM (&tegra_i2c_pm)
#else
#define TEGRA_I2C_PM NULL
--
1.9.1
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-s3c2410.c | 16 ----------------
1 file changed, 16 deletions(-)
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index e828a1d..568b993 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -103,7 +103,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;
@@ -714,9 +713,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);
@@ -1257,16 +1253,6 @@ static int s3c24xx_i2c_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_SLEEP
-static int s3c24xx_i2c_suspend_noirq(struct device *dev)
-{
- struct platform_device *pdev = to_platform_device(dev);
- struct s3c24xx_i2c *i2c = platform_get_drvdata(pdev);
-
- i2c->suspended = 1;
-
- return 0;
-}
-
static int s3c24xx_i2c_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
@@ -1275,7 +1261,6 @@ static int s3c24xx_i2c_resume(struct device *dev)
clk_prepare_enable(i2c->clk);
s3c24xx_i2c_init(i2c);
clk_disable_unprepare(i2c->clk);
- i2c->suspended = 0;
return 0;
}
@@ -1284,7 +1269,6 @@ static int s3c24xx_i2c_resume(struct device *dev)
#ifdef CONFIG_PM
static const struct dev_pm_ops s3c24xx_i2c_dev_pm_ops = {
#ifdef CONFIG_PM_SLEEP
- .suspend_noirq = s3c24xx_i2c_suspend_noirq,
.resume = s3c24xx_i2c_resume,
#endif
};
--
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.
Have fun coding,
Bastian
2014-07-17 16:48 GMT+02:00 Bastian Hecht <[email protected]>:
> 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(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 7c7f4b8..b15dc20 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -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)
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index b556e0a..af08c75 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -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
>
On Thu, Jul 17, 2014 at 04:48:40PM +0200, Bastian Hecht wrote:
[...]
> #ifdef CONFIG_PM_SLEEP
> -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;
> }
>
> -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
On Thu, Jul 17, 2014 at 04:58:17PM +0200, Bastian Hecht wrote:
> 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.
Good to know. I'd like to get some Tested-by tags before applying to
make sure we have no regressions. So, with you being away, let's just
schedule this for 3.18 and we have all the time we need. Thanks for
finishing this code before you left. Have a great month!
Wolfram
2014-07-17 16:59 GMT+02:00 Thierry Reding <[email protected]>:
> On Thu, Jul 17, 2014 at 04:48:40PM +0200, Bastian Hecht wrote:
> [...]
>> #ifdef CONFIG_PM_SLEEP
>> -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;
>> }
>>
>> -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?
Oh yes thanks. I made the same mistake in [PATCH 3/5] too.
Thanks,
Bastian
> Thierry
2014-07-17 16:48 GMT+02:00 Bastian Hecht <[email protected]>:
> 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(-)
>
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> index 63d2292..a80cf28 100644
> --- a/drivers/i2c/busses/i2c-exynos5.c
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -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);
> clk_disable_unprepare(i2c->clk);
> - i2c->suspended = 0;
>
> return 0;
> }
> #endif
>
> -static SIMPLE_DEV_PM_OPS(exynos5_i2c_dev_pm_ops, exynos5_i2c_suspend_noirq,
> - exynos5_i2c_resume_noirq);
> +static SIMPLE_DEV_PM_OPS(exynos5_i2c_dev_pm_ops, exynos5_i2c_resume_noirq);
This should be
+static SIMPLE_DEV_PM_OPS(exynos5_i2c_dev_pm_ops, NULL,
exynos5_i2c_resume_noirq);
And this is v2, not v3.
> static struct platform_driver exynos5_i2c_driver = {
> .probe = exynos5_i2c_probe,
> --
> 1.9.1
>
Hi
On 07/17/2014 05:48 PM, Bastian Hecht wrote:
> 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.
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.
As result, it may cause regression and break PM functionality on some SoCs
if i2c-core will block I2c transfers unconditionally at device's
suspending stage.
May be it will be useful to add just "suspended" field in i2c adapter
structure and provide corresponding accessors.
>
> 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(-)
>
> diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
> index 7c7f4b8..b15dc20 100644
> --- a/drivers/i2c/i2c-core.c
> +++ b/drivers/i2c/i2c-core.c
> @@ -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)
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index b556e0a..af08c75 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -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];
>
Regards,
- grygroii
On Tue, Jul 22, 2014 at 09:46:46PM +0300, Grygorii Strashko wrote:
> 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.
This is definitely not safe. I worked on a PXA platform a while back
where the only way to tell the thing to "sleep" was to send an I2C
message to a microcontroller asking it to remove power.
Plus, as you say, you may also have PMICs that you need to talk to
in order to shut power off to various peripherals (and maybe even
the CPU itself) during the suspend process.
I2C is one of those cases where devices attached to the I2C bus are
themselves responsible for doing their own suspend shutdown at the
appropriate time; it's not the responsibility of the I2C core to
know this kind of system/driver dependent detail.
--
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.