[PATCH v2 0/3] pwm: lpss: pwm: lpss: Misc. cleanups / improvements

[PATCH v2 0/3] pwm: lpss: pwm: lpss: Misc. cleanups / improvements

[Hans de Goede]

Hi All,

Now that the pwm-lpss / pwm-crc / i915 atomic PWM conversion has landed
in 5.10-rc1 here is a small follow up series with some misc. improvements.

Changes in v2:
- Extend comment to explain why the DPM_FLAG_SMART_SUSPEND is set
- Add Andy's Reviewed-by to all 3 patches

Regards,

Hans

[PATCH v2 1/3] pwm: lpss: Log error from pwm_lpss_is_updating() if the update bit is still set

[Hans de Goede]

pwm_lpss_is_updating() does a sanity check which should never fail.
If the check does actually fail that is worth logging an error,
especially since this means that we will skip making the requested
changes to the PWM settings.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 3444c56b4bed..939de93c157b 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -76,7 +76,12 @@ static int pwm_lpss_wait_for_update(struct pwm_device *pwm)
 
 static inline int pwm_lpss_is_updating(struct pwm_device *pwm)
 {
-	return (pwm_lpss_read(pwm) & PWM_SW_UPDATE) ? -EBUSY : 0;
+	if (pwm_lpss_read(pwm) & PWM_SW_UPDATE) {
+		dev_err(pwm->chip->dev, "PWM_SW_UPDATE is still set, skipping update\n");
+		return -EBUSY;
+	}
+
+	return 0;
 }
 
 static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
-- 
2.28.0

[PATCH v2 2/3] pwm: lpss: Use DPM_FLAG_NO_DIRECT_COMPLETE instead of declaring a prepare handler

[Hans de Goede]

ACPI LPSS devices use direct-complete style suspend/resume handling by
default. We set the DPM_FLAG_SMART_PREPARE and define a prepare handler
to disable this on Cherry Trail devices.

Clean this up a bit by setting the DPM_FLAG_NO_DIRECT_COMPLETE flag for
Cherry Trail devices, instead of defining a prepare handler.

While at it also improve the comment explaining why this is necessary.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pwm/pwm-lpss-platform.c | 35 ++++++++++++++-------------------
 1 file changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index c6502cf7a7af..ac33861edb48 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -58,7 +58,21 @@ static int pwm_lpss_probe_platform(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, lpwm);
 
-	dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_SMART_PREPARE);
+	/*
+	 * On Cherry Trail devices the GFX0._PS0 AML checks if the controller
+	 * is on and if it is not on it turns it on and restores what it
+	 * believes is the correct state to the PWM controller.
+	 * Because of this we must disallow direct-complete, which keeps the
+	 * controller (runtime)suspended on resume, to avoid 2 issues:
+	 * 1. The controller getting turned on without the linux-pm code
+	 *    knowing about this. On devices where the controller is unused
+	 *    this causes it to stay on during the next suspend causing high
+	 *    battery drain (because S0i3 is not reached)
+	 * 2. The state restoring code unexpectedly messing with the controller
+	 */
+	if (info->other_devices_aml_touches_pwm_regs)
+		dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NO_DIRECT_COMPLETE);
+
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
@@ -73,24 +87,6 @@ static int pwm_lpss_remove_platform(struct platform_device *pdev)
 	return pwm_lpss_remove(lpwm);
 }
 
-static int pwm_lpss_prepare(struct device *dev)
-{
-	struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev);
-
-	/*
-	 * If other device's AML code touches the PWM regs on suspend/resume
-	 * force runtime-resume the PWM controller to allow this.
-	 */
-	if (lpwm->info->other_devices_aml_touches_pwm_regs)
-		return 0; /* Force runtime-resume */
-
-	return 1; /* If runtime-suspended leave as is */
-}
-
-static const struct dev_pm_ops pwm_lpss_platform_pm_ops = {
-	.prepare = pwm_lpss_prepare,
-};
-
 static const struct acpi_device_id pwm_lpss_acpi_match[] = {
 	{ "80860F09", (unsigned long)&pwm_lpss_byt_info },
 	{ "80862288", (unsigned long)&pwm_lpss_bsw_info },
@@ -104,7 +100,6 @@ static struct platform_driver pwm_lpss_driver_platform = {
 	.driver = {
 		.name = "pwm-lpss",
 		.acpi_match_table = pwm_lpss_acpi_match,
-		.pm = &pwm_lpss_platform_pm_ops,
 	},
 	.probe = pwm_lpss_probe_platform,
 	.remove = pwm_lpss_remove_platform,
-- 
2.28.0

[PATCH v2 3/3] pwm: lpss: Set DPM_FLAG_SMART_SUSPEND on Cherry Trail devices

[Hans de Goede]

As the comment above the code setting the DPM_FLAG_NO_DIRECT_COMPLETE
flag explains:

    /*
     * On Cherry Trail devices the GFX0._PS0 AML checks if the controller
     * is on and if it is not on it turns it on and restores what it
     * believes is the correct state to the PWM controller.
     * Because of this we must disallow direct-complete, which keeps the
     * controller (runtime)suspended, on resume to avoid 2 issues:
     * 1. The controller getting turned on without the linux-pm code
     *    knowing about this. On devices where the controller is unused
     *    this causes it to stay on during the next suspend causing high
     *    battery drain (because S0i3 is not reached)
     * 2. The state restoring code unexpectedly messing with the controller
     */

The pm-core must not skip resume to avoid the GFX0._PS0 AML code messing
with the PWM controller behind our back. But leaving the controller
runtime-suspended (skipping runtime-resume + normal-suspend) during
suspend is fine. Set the DPM_FLAG_SMART_SUSPEND flag to allow this.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Extend comment to explain why the DPM_FLAG_SMART_SUSPEND is set
---
 drivers/pwm/pwm-lpss-platform.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c
index ac33861edb48..986786be1e49 100644
--- a/drivers/pwm/pwm-lpss-platform.c
+++ b/drivers/pwm/pwm-lpss-platform.c
@@ -69,9 +69,13 @@ static int pwm_lpss_probe_platform(struct platform_device *pdev)
 	 *    this causes it to stay on during the next suspend causing high
 	 *    battery drain (because S0i3 is not reached)
 	 * 2. The state restoring code unexpectedly messing with the controller
+	 *
+	 * Leaving the controller runtime-suspended (skipping runtime-resume +
+	 * normal-suspend) during suspend is fine.
 	 */
 	if (info->other_devices_aml_touches_pwm_regs)
-		dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NO_DIRECT_COMPLETE);
+		dev_pm_set_driver_flags(&pdev->dev, DPM_FLAG_NO_DIRECT_COMPLETE|
+						    DPM_FLAG_SMART_SUSPEND);
 
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
-- 
2.28.0

Re: [PATCH v2 0/3] pwm: lpss: pwm: lpss: Misc. cleanups / improvements

[Thierry Reding]

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

On Mon, Nov 09, 2020 at 11:57:23AM +0100, Hans de Goede wrote:
> Hi All,
> 
> Now that the pwm-lpss / pwm-crc / i915 atomic PWM conversion has landed
> in 5.10-rc1 here is a small follow up series with some misc. improvements.
> 
> Changes in v2:
> - Extend comment to explain why the DPM_FLAG_SMART_SUSPEND is set
> - Add Andy's Reviewed-by to all 3 patches

All three patches applied, thanks.

Thierry

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