i2c: designware: switch to suspend_late/resume_early

PATCH In review Rajat Jain 5 participants 15 messages Sep 20, 2017 – Sep 23, 2017

Subsystems (4)

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

lore

Share & tools

lore

Rajat Jain

author

2017-09-20 22:31 UTC #0 lore raw

PATCH 1/2 i2c: designware: switch to suspend_late/resume_early

Ref: https://lkml.org/lkml/2017/9/19/649

The bus controllers should suspend the bus operations only after all of the devices on the bus have suspended their device completely. Since the i2c_client drivers could be talking to their devices in their suspend_late() calls, lets ensure that the bus is alive by that time. Thus moving the controller suspend logic to suspend_late().

Signed-off-by Rajat Jain <[email protected]>

---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/i2c/busses/i2c-designware-platdrv.c +1 -1

index 0e65b97842b4..66dd7f844c40 100644
@@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
 	.prepare = dw_i2c_plat_prepare,
 	.complete = dw_i2c_plat_complete,
-	SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
 	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
 			   dw_i2c_plat_resume,
 			   NULL)

Signature · 1 line

2.14.1.821.g8fa685d3b7-goog

Rajat Jain

author

2017-09-20 22:31 UTC #1 lore raw

PATCH 2/2 mfd: intel-lpss: switch to suspend_late()/resume_early()

Ref: https://lkml.org/lkml/2017/9/19/649

The intel-lpss hosts the designware i2c controller device, which needs to be up and running until all its i2c child devices have suspended (child devices need to be accessible over i2c until suspend_late() has been called for all those devices).

So lets delay the resetting of the controller until suspend_late(). I thought of renaming function to say *_late / *_early but that might cause confusion since the same function is also used for runtime suspend/resume.

Fixes 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")

Signed-off-by Rajat Jain <[email protected]>

---
 drivers/mfd/intel-lpss.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

drivers/mfd/intel-lpss.h +1 -6

index 694116630ffa..865bbeaaf00c 100644
@@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS			\
 	.prepare = intel_lpss_prepare,		\
-	.suspend = intel_lpss_suspend,		\
-	.resume = intel_lpss_resume,		\
-	.freeze = intel_lpss_suspend,		\
-	.thaw = intel_lpss_resume,		\
-	.poweroff = intel_lpss_suspend,		\
-	.restore = intel_lpss_resume,
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
 #endif

Signature · 1 line

2.14.1.821.g8fa685d3b7-goog

Rafael J. Wysocki

2017-09-21 00:27 UTC #2 lore raw

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

Quoted 26 lines · Rajat Jain

Ref: https://lkml.org/lkml/2017/9/19/649
The intel-lpss hosts the designware i2c controller device, which needs to be up and running until all its i2c child devices have suspended (child devices need to be accessible over i2c until suspend_late() has been called for all those devices).
So lets delay the resetting of the controller until suspend_late(). I thought of renaming function to say *_late / *_early but that might cause confusion since the same function is also used for runtime suspend/resume.
Fixes 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/mfd/intel-lpss.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/mfd/intel-lpss.h +0 -0
index 694116630ffa..865bbeaaf00c 100644
@@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS                        \
        .prepare = intel_lpss_prepare,          \

drivers/mfd/intel-lpss.h int intel_lpss_resume(struct device *dev); · line 44

Quoted 10 lines

drivers/mfd/intel-lpss.h +1 -6

-       .suspend = intel_lpss_suspend,          \
-       .resume = intel_lpss_resume,            \
-       .freeze = intel_lpss_suspend,           \
-       .thaw = intel_lpss_resume,              \
-       .poweroff = intel_lpss_suspend,         \
-       .restore = intel_lpss_resume,
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
 #endif

So I sent this exact patch several days ago:
https://patchwork.kernel.org/patch/9939809/

Thanks,
Rafael

Rajat Jain

author

2017-09-21 01:14 UTC #3 lore raw

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

Quoted 32 lines · Rafael J. Wysocki

Quoted 31 lines · Rajat Jain
Ref: https://lkml.org/lkml/2017/9/19/649
The intel-lpss hosts the designware i2c controller device, which needs to be up and running until all its i2c child devices have suspended (child devices need to be accessible over i2c until suspend_late() has been called for all those devices).
So lets delay the resetting of the controller until suspend_late(). I thought of renaming function to say *_late / *_early but that might cause confusion since the same function is also used for runtime suspend/resume.
Fixes 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/mfd/intel-lpss.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/mfd/intel-lpss.h +0 -5
index 694116630ffa..865bbeaaf00c 100644
@@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS                        \
        .prepare = intel_lpss_prepare,          \
-       .suspend = intel_lpss_suspend,          \
-       .resume = intel_lpss_resume,            \
-       .freeze = intel_lpss_suspend,           \
-       .thaw = intel_lpss_resume,              \
-       .poweroff = intel_lpss_suspend,         \

Quoted 8 lines

drivers/mfd/intel-lpss.h int intel_lpss_resume(struct device *dev); · line 44
Quoted 5 lines
drivers/mfd/intel-lpss.h +1 -1
-       .restore = intel_lpss_resume,
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
 #endif
So I sent this exact patch several days ago:
https://patchwork.kernel.org/patch/9939809/

Sorry, I missed it. Please consider my patch withdrawn / self-nack'ed in favor of Rafael's patch.

Quoted 3 lines

Thanks,
Rafael

Lee Jones

2017-09-21 08:41 UTC #4 lore raw

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

In reply to Rafael J. Wysocki

Quoted 32 lines · Rafael J. Wysocki

Quoted 31 lines · Rajat Jain
Ref: https://lkml.org/lkml/2017/9/19/649
The intel-lpss hosts the designware i2c controller device, which needs to be up and running until all its i2c child devices have suspended (child devices need to be accessible over i2c until suspend_late() has been called for all those devices).
So lets delay the resetting of the controller until suspend_late(). I thought of renaming function to say *_late / *_early but that might cause confusion since the same function is also used for runtime suspend/resume.
Fixes 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/mfd/intel-lpss.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/mfd/intel-lpss.h +0 -5
index 694116630ffa..865bbeaaf00c 100644
@@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS                        \
        .prepare = intel_lpss_prepare,          \
-       .suspend = intel_lpss_suspend,          \
-       .resume = intel_lpss_resume,            \
-       .freeze = intel_lpss_suspend,           \
-       .thaw = intel_lpss_resume,              \
-       .poweroff = intel_lpss_suspend,         \

Quoted 8 lines

drivers/mfd/intel-lpss.h int intel_lpss_resume(struct device *dev); · line 44
Quoted 5 lines
drivers/mfd/intel-lpss.h +1 -1
-       .restore = intel_lpss_resume,
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
 #endif
So I sent this exact patch several days ago:
https://patchwork.kernel.org/patch/9939809/

You did? Any reason you didn't send it to me?

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Rafael J. Wysocki

2017-09-21 14:10 UTC #5 lore raw

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

Quoted 43 lines · CEST Lee Jones

Quoted 32 lines · Rafael J. Wysocki
Quoted 31 lines · Rajat Jain
Ref: https://lkml.org/lkml/2017/9/19/649
The intel-lpss hosts the designware i2c controller device, which needs to be up and running until all its i2c child devices have suspended (child devices need to be accessible over i2c until suspend_late() has been called for all those devices).
So lets delay the resetting of the controller until suspend_late(). I thought of renaming function to say *_late / *_early but that might cause confusion since the same function is also used for runtime suspend/resume.
Fixes 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/mfd/intel-lpss.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/mfd/intel-lpss.h +0 -5
index 694116630ffa..865bbeaaf00c 100644
@@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS                        \
        .prepare = intel_lpss_prepare,          \
-       .suspend = intel_lpss_suspend,          \
-       .resume = intel_lpss_resume,            \
-       .freeze = intel_lpss_suspend,           \
-       .thaw = intel_lpss_resume,              \
-       .poweroff = intel_lpss_suspend,         \
Quoted 8 lines
drivers/mfd/intel-lpss.h int intel_lpss_resume(struct device *dev); · line 44
Quoted 5 lines
drivers/mfd/intel-lpss.h +1 -1
-       .restore = intel_lpss_resume,
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
 #endif
So I sent this exact patch several days ago:
https://patchwork.kernel.org/patch/9939809/

Quoted 1 line

You did?

Yes, I did.

Quoted 1 line

Any reason you didn't send it to me?

Well, Lee Jones <[email protected]> is very much in the CC list of it.

Was I expected to use a different e-mail address for that?

Thanks,
Rafael

Lee Jones

2017-09-21 15:17 UTC #6 lore raw

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

Quoted 46 lines · Rafael J. Wysocki

Quoted 43 lines · CEST Lee Jones
Quoted 32 lines · Rafael J. Wysocki
Quoted 31 lines · Rajat Jain
Ref: https://lkml.org/lkml/2017/9/19/649
The intel-lpss hosts the designware i2c controller device, which needs to be up and running until all its i2c child devices have suspended (child devices need to be accessible over i2c until suspend_late() has been called for all those devices).
So lets delay the resetting of the controller until suspend_late(). I thought of renaming function to say *_late / *_early but that might cause confusion since the same function is also used for runtime suspend/resume.
Fixes 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/mfd/intel-lpss.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/mfd/intel-lpss.h +0 -5
index 694116630ffa..865bbeaaf00c 100644
@@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS                        \
        .prepare = intel_lpss_prepare,          \
-       .suspend = intel_lpss_suspend,          \
-       .resume = intel_lpss_resume,            \
-       .freeze = intel_lpss_suspend,           \
-       .thaw = intel_lpss_resume,              \
-       .poweroff = intel_lpss_suspend,         \
Quoted 8 lines
drivers/mfd/intel-lpss.h int intel_lpss_resume(struct device *dev); · line 44
Quoted 5 lines
drivers/mfd/intel-lpss.h +1 -1
-       .restore = intel_lpss_resume,
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
 #endif
So I sent this exact patch several days ago:
https://patchwork.kernel.org/patch/9939809/
Quoted 1 line
You did?

Quoted 7 lines

Yes, I did.
Quoted 1 line
Any reason you didn't send it to me?
Well, Lee Jones <[email protected]> is very much in the CC list of it.
Was I expected to use a different e-mail address for that?

I don't monitor that one. Better to use the one in MAINTAINERS.

Signature · 4 lines

Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Rafael J. Wysocki

2017-09-21 15:18 UTC #7 lore raw

Re: [PATCH 2/2] mfd: intel-lpss: switch to suspend_late()/resume_early()

Quoted 50 lines · Lee Jones

Quoted 48 lines · Rafael J. Wysocki
Quoted 43 lines · CEST Lee Jones
Quoted 32 lines · Rafael J. Wysocki
Quoted 31 lines · Rajat Jain
Ref: https://lkml.org/lkml/2017/9/19/649
The intel-lpss hosts the designware i2c controller device, which needs to be up and running until all its i2c child devices have suspended (child devices need to be accessible over i2c until suspend_late() has been called for all those devices).
So lets delay the resetting of the controller until suspend_late(). I thought of renaming function to say *_late / *_early but that might cause confusion since the same function is also used for runtime suspend/resume.
Fixes 0b471aaa0e1a ("Put I2C and SPI controllers into reset state on suspend")
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/mfd/intel-lpss.h | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)
drivers/mfd/intel-lpss.h +0 -5
index 694116630ffa..865bbeaaf00c 100644
@@ -38,12 +38,7 @@ int intel_lpss_resume(struct device *dev);
 #ifdef CONFIG_PM_SLEEP
 #define INTEL_LPSS_SLEEP_PM_OPS                        \
        .prepare = intel_lpss_prepare,          \
-       .suspend = intel_lpss_suspend,          \
-       .resume = intel_lpss_resume,            \
-       .freeze = intel_lpss_suspend,           \
-       .thaw = intel_lpss_resume,              \
-       .poweroff = intel_lpss_suspend,         \
Quoted 8 lines
drivers/mfd/intel-lpss.h int intel_lpss_resume(struct device *dev); · line 44
Quoted 5 lines
drivers/mfd/intel-lpss.h +1 -1
-       .restore = intel_lpss_resume,
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(intel_lpss_suspend, intel_lpss_resume)
 #else
 #define INTEL_LPSS_SLEEP_PM_OPS
 #endif
So I sent this exact patch several days ago:
https://patchwork.kernel.org/patch/9939809/
Quoted 1 line
You did?
Yes, I did.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Any reason you didn't send it to me?
Well, Lee Jones <[email protected]> is very much in the CC list of it.
Was I expected to use a different e-mail address for that?
I don't monitor that one. Better to use the one in MAINTAINERS.

OK, I will in the future.

Rajat Jain

2017-09-20 22:42 UTC #8 lore raw

In reply to Rajat Jain

Quoted 11 lines · Rajat Jain

Ref: https://lkml.org/lkml/2017/9/19/649
The bus controllers should suspend the bus operations only after all of the devices on the bus have suspended their device completely. Since the i2c_client drivers could be talking to their devices in their suspend_late() calls, lets ensure that the bus is alive by that time. Thus moving the controller suspend logic to suspend_late().
Signed-off-by Rajat Jain <[email protected]>
---

Forgot to mention, this was needed because we were seeing problems when the ACPI routines (called at resume_early() time) for the i2c client device were trying to access one of the I2C devices, and were failing because the bus was not ready.

Thanks,

Rajat

Quoted 19 lines

 drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/i2c/busses/i2c-designware-platdrv.c +1 -2

index 0e65b97842b4..66dd7f844c40 100644
@@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
        .prepare = dw_i2c_plat_prepare,
        .complete = dw_i2c_plat_complete,
-       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
        SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
                           dw_i2c_plat_resume,
                           NULL)
--

2.14.1.821.g8fa685d3b7-goog

Rafael J. Wysocki

2017-09-21 00:24 UTC #9 lore raw

In reply to Rajat Jain

Quoted 19 lines · Rajat Jain

Ref: https://lkml.org/lkml/2017/9/19/649
The bus controllers should suspend the bus operations only after all of the devices on the bus have suspended their device completely. Since the i2c_client drivers could be talking to their devices in their suspend_late() calls, lets ensure that the bus is alive by that time. Thus moving the controller suspend logic to suspend_late().
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
index 0e65b97842b4..66dd7f844c40 100644

drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_suspend(struct device *dev) · line 474

Quoted 8 lines

drivers/i2c/busses/i2c-designware-platdrv.c +1 -1

@@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
        .prepare = dw_i2c_plat_prepare,
        .complete = dw_i2c_plat_complete,
-       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
        SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
                           dw_i2c_plat_resume,
                           NULL)

No, you can't just do that.

I sent patches to do it properly before my trip to LA last week, it shouldn't be overly difficult to find them in the mailing list archives. I can look them up tomorrow if need be.

Thanks,
Rafael

Rajat Jain

author

2017-09-21 01:13 UTC #10 lore raw

Quoted 31 lines · Rafael J. Wysocki

Quoted 19 lines · Rajat Jain
Ref: https://lkml.org/lkml/2017/9/19/649
The bus controllers should suspend the bus operations only after all of the devices on the bus have suspended their device completely. Since the i2c_client drivers could be talking to their devices in their suspend_late() calls, lets ensure that the bus is alive by that time. Thus moving the controller suspend logic to suspend_late().
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
index 0e65b97842b4..66dd7f844c40 100644
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_suspend(struct device *dev) · line 474
Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c +1 -1
@@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
        .prepare = dw_i2c_plat_prepare,
        .complete = dw_i2c_plat_complete,
-       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
        SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
                           dw_i2c_plat_resume,
                           NULL)
No, you can't just do that.

Quoted 3 lines

I sent patches to do it properly before my trip to LA last week, it shouldn't be overly difficult to find them in the mailing list archives. I can look them up tomorrow if need be.

Thanks, I am guessing you mean this?

https://patchwork.kernel.org/patch/9939807/

Quoted 3 lines

Thanks,
Rafael

Rafael J. Wysocki

2017-09-21 14:11 UTC #11 lore raw

Quoted 32 lines · CEST Rajat Jain

Quoted 31 lines · Rafael J. Wysocki
Quoted 19 lines · Rajat Jain
Ref: https://lkml.org/lkml/2017/9/19/649
The bus controllers should suspend the bus operations only after all of the devices on the bus have suspended their device completely. Since the i2c_client drivers could be talking to their devices in their suspend_late() calls, lets ensure that the bus is alive by that time. Thus moving the controller suspend logic to suspend_late().
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
index 0e65b97842b4..66dd7f844c40 100644
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_suspend(struct device *dev) · line 474
Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c +1 -1
@@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
        .prepare = dw_i2c_plat_prepare,
        .complete = dw_i2c_plat_complete,
-       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
        SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
                           dw_i2c_plat_resume,
                           NULL)
No, you can't just do that.

Quoted 7 lines

Quoted 3 lines
I sent patches to do it properly before my trip to LA last week, it shouldn't be overly difficult to find them in the mailing list archives. I can look them up tomorrow if need be.
Thanks, I am guessing you mean this?
https://patchwork.kernel.org/patch/9939807/

Yes, that's what I mean.

Thanks,
Rafael

Rafael J. Wysocki

2017-09-23 12:55 UTC #12 lore raw

Quoted 37 lines · Rafael J. Wysocki

Quoted 36 lines · CEST Rajat Jain
Quoted 34 lines · Rafael J. Wysocki
Quoted 19 lines · Rajat Jain
Ref: https://lkml.org/lkml/2017/9/19/649
The bus controllers should suspend the bus operations only after all of the devices on the bus have suspended their device completely. Since the i2c_client drivers could be talking to their devices in their suspend_late() calls, lets ensure that the bus is alive by that time. Thus moving the controller suspend logic to suspend_late().
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
index 0e65b97842b4..66dd7f844c40 100644
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_suspend(struct device *dev) · line 474
Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c +1 -1
@@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
        .prepare = dw_i2c_plat_prepare,
        .complete = dw_i2c_plat_complete,
-       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
        SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
                           dw_i2c_plat_resume,
                           NULL)
No, you can't just do that.
I sent patches to do it properly before my trip to LA last week, it shouldn't be overly difficult to find them in the mailing list archives. I can look them up tomorrow if need be.

Quoted 5 lines

Quoted 3 lines
Thanks, I am guessing you mean this?
https://patchwork.kernel.org/patch/9939807/
Yes, that's what I mean.

BTW, does this patchset work for you?

Thanks,
Rafael

Rajat Jain

Tested-by

2017-09-23 15:49 UTC #13 lore raw

Quoted 38 lines · Rafael J. Wysocki

Quoted 37 lines · Rafael J. Wysocki
Quoted 36 lines · CEST Rajat Jain
Quoted 34 lines · Rafael J. Wysocki
Quoted 19 lines · Rajat Jain
Ref: https://lkml.org/lkml/2017/9/19/649
The bus controllers should suspend the bus operations only after all of the devices on the bus have suspended their device completely. Since the i2c_client drivers could be talking to their devices in their suspend_late() calls, lets ensure that the bus is alive by that time. Thus moving the controller suspend logic to suspend_late().
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
index 0e65b97842b4..66dd7f844c40 100644
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_suspend(struct device *dev) · line 474
Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c +1 -1
@@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
        .prepare = dw_i2c_plat_prepare,
        .complete = dw_i2c_plat_complete,
-       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
        SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
                           dw_i2c_plat_resume,
                           NULL)
No, you can't just do that.
I sent patches to do it properly before my trip to LA last week, it shouldn't be overly difficult to find them in the mailing list archives. I can look them up tomorrow if need be.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
Thanks, I am guessing you mean this?
https://patchwork.kernel.org/patch/9939807/
Yes, that's what I mean.
BTW, does this patchset work for you?

Yes, I don't see the issue I was seeing earlier with your patch. Please feel free to add

Tested-by Rajat Jain <[email protected]>

Quoted 3 lines

Thanks,
Rafael

Rajat Jain

2017-09-23 16:17 UTC #14 lore raw

Quoted 41 lines · Rajat Jain

Quoted 40 lines · Rafael J. Wysocki
Quoted 39 lines · Rafael J. Wysocki
Quoted 38 lines · CEST Rajat Jain
Quoted 31 lines · Rafael J. Wysocki
Quoted 19 lines · Rajat Jain
Ref: https://lkml.org/lkml/2017/9/19/649
The bus controllers should suspend the bus operations only after all of the devices on the bus have suspended their device completely. Since the i2c_client drivers could be talking to their devices in their suspend_late() calls, lets ensure that the bus is alive by that time. Thus moving the controller suspend logic to suspend_late().
Signed-off-by Rajat Jain <[email protected]>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/i2c/busses/i2c-designware-platdrv.c +0 -0
index 0e65b97842b4..66dd7f844c40 100644
drivers/i2c/busses/i2c-designware-platdrv.c static int dw_i2c_plat_suspend(struct device *dev) · line 474
Quoted 8 lines
drivers/i2c/busses/i2c-designware-platdrv.c +1 -1
@@ -468,7 +468,7 @@ static int dw_i2c_plat_suspend(struct device *dev)
 static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
        .prepare = dw_i2c_plat_prepare,
        .complete = dw_i2c_plat_complete,
-       SET_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
+       SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
        SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend,
                           dw_i2c_plat_resume,
                           NULL)
No, you can't just do that.
Quoted 3 lines
I sent patches to do it properly before my trip to LA last week, it shouldn't be overly difficult to find them in the mailing list archives. I can look them up tomorrow if need be.
Thanks, I am guessing you mean this?

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Quoted 1 line
https://patchwork.kernel.org/patch/9939807/
Yes, that's what I mean.
BTW, does this patchset work for you?
Yes, I don't see the issue I was seeing earlier with your patch. Please feel free to adds,

I think I made my sentence ambiguous. Let me rephrase: Yes, with your patch, I don't see the issue I was seeing earlier (without your patch)

Quoted 6 lines

Tested-by Rajat Jain <[email protected]>
Quoted 3 lines
Thanks,
Rafael