| Message ID | 20200903112337.4113-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API | expand |
On Thu, Sep 03, 2020 at 01:23:27PM +0200, Hans de Goede wrote: > PWM controller drivers should not restore the PWM state on resume. The > convention is that PWM consumers do this by calling pwm_apply_state(), > so that it can be done at the exact moment when the consumer needs > the state to be stored, avoiding e.g. backlight flickering. > > The only in kernel consumers of the pwm-lpss code, the i915 driver > and the pwm-class sysfs interface code both correctly restore the > state on resume, so there is no need to do this in the pwm-lpss code. > > More-over the removed resume handler is buggy, since it blindly > restores the ctrl-register contents without setting the update > bit, which is necessary to get the controller to actually use/apply > the restored base-unit and on-time-div values. While this is okay Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> the question is do we need to have similar in acpi_lpss.c? For example, static const struct lpss_device_desc byt_pwm_dev_desc = { .flags = LPSS_SAVE_CTX, ^^^^^^^^^^^^^^ .prv_offset = 0x800, .setup = byt_pwm_setup, }; static const struct lpss_device_desc bsw_pwm_dev_desc = { .flags = LPSS_SAVE_CTX | LPSS_NO_D3_DELAY, ^^^^^^^^^^^^^^ .prv_offset = 0x800, .setup = bsw_pwm_setup, }; > Acked-by: Thierry Reding <thierry.reding@gmail.com> > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v9: > - This is a new patch in v9 of this series > --- > drivers/pwm/pwm-lpss-platform.c | 1 - > drivers/pwm/pwm-lpss.c | 24 ------------------------ > drivers/pwm/pwm-lpss.h | 3 --- > 3 files changed, 28 deletions(-) > > diff --git a/drivers/pwm/pwm-lpss-platform.c b/drivers/pwm/pwm-lpss-platform.c > index 48f34d20aecd..c6502cf7a7af 100644 > --- a/drivers/pwm/pwm-lpss-platform.c > +++ b/drivers/pwm/pwm-lpss-platform.c > @@ -89,7 +89,6 @@ static int pwm_lpss_prepare(struct device *dev) > > static const struct dev_pm_ops pwm_lpss_platform_pm_ops = { > .prepare = pwm_lpss_prepare, > - SET_SYSTEM_SLEEP_PM_OPS(pwm_lpss_suspend, pwm_lpss_resume) > }; > > static const struct acpi_device_id pwm_lpss_acpi_match[] = { > diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c > index 9c5c7217c9b6..3444c56b4bed 100644 > --- a/drivers/pwm/pwm-lpss.c > +++ b/drivers/pwm/pwm-lpss.c > @@ -260,30 +260,6 @@ int pwm_lpss_remove(struct pwm_lpss_chip *lpwm) > } > EXPORT_SYMBOL_GPL(pwm_lpss_remove); > > -int pwm_lpss_suspend(struct device *dev) > -{ > - struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev); > - int i; > - > - for (i = 0; i < lpwm->info->npwm; i++) > - lpwm->saved_ctrl[i] = readl(lpwm->regs + i * PWM_SIZE + PWM); > - > - return 0; > -} > -EXPORT_SYMBOL_GPL(pwm_lpss_suspend); > - > -int pwm_lpss_resume(struct device *dev) > -{ > - struct pwm_lpss_chip *lpwm = dev_get_drvdata(dev); > - int i; > - > - for (i = 0; i < lpwm->info->npwm; i++) > - writel(lpwm->saved_ctrl[i], lpwm->regs + i * PWM_SIZE + PWM); > - > - return 0; > -} > -EXPORT_SYMBOL_GPL(pwm_lpss_resume); > - > MODULE_DESCRIPTION("PWM driver for Intel LPSS"); > MODULE_AUTHOR("Mika Westerberg <mika.westerberg@linux.intel.com>"); > MODULE_LICENSE("GPL v2"); > diff --git a/drivers/pwm/pwm-lpss.h b/drivers/pwm/pwm-lpss.h > index 7909fa12fca2..70db7e389d66 100644 > --- a/drivers/pwm/pwm-lpss.h > +++ b/drivers/pwm/pwm-lpss.h > @@ -19,7 +19,6 @@ struct pwm_lpss_chip { > struct pwm_chip chip; > void __iomem *regs; > const struct pwm_lpss_boardinfo *info; > - u32 saved_ctrl[MAX_PWMS]; > }; > > struct pwm_lpss_boardinfo { > @@ -37,7 +36,5 @@ struct pwm_lpss_boardinfo { > struct pwm_lpss_chip *pwm_lpss_probe(struct device *dev, struct resource *r, > const struct pwm_lpss_boardinfo *info); > int pwm_lpss_remove(struct pwm_lpss_chip *lpwm); > -int pwm_lpss_suspend(struct device *dev); > -int pwm_lpss_resume(struct device *dev); > > #endif /* __PWM_LPSS_H */ > -- > 2.28.0 >
On Thu, Sep 03, 2020 at 03:48:16PM +0300, Andy Shevchenko wrote: > On Thu, Sep 03, 2020 at 01:23:27PM +0200, Hans de Goede wrote: > the question is do we need to have similar in acpi_lpss.c? > For example, > static const struct lpss_device_desc byt_pwm_dev_desc = { > .flags = LPSS_SAVE_CTX, > ^^^^^^^^^^^^^^ > .prv_offset = 0x800, > .setup = byt_pwm_setup, > }; > > static const struct lpss_device_desc bsw_pwm_dev_desc = { > .flags = LPSS_SAVE_CTX | LPSS_NO_D3_DELAY, > ^^^^^^^^^^^^^^ > .prv_offset = 0x800, > .setup = bsw_pwm_setup, > }; Okay, it's a private space which has clock and reset gating, so means we still need to handle it.
Hi, On 9/3/20 2:56 PM, Andy Shevchenko wrote: > On Thu, Sep 03, 2020 at 03:48:16PM +0300, Andy Shevchenko wrote: >> On Thu, Sep 03, 2020 at 01:23:27PM +0200, Hans de Goede wrote: > >> the question is do we need to have similar in acpi_lpss.c? >> For example, >> static const struct lpss_device_desc byt_pwm_dev_desc = { >> .flags = LPSS_SAVE_CTX, >> ^^^^^^^^^^^^^^ >> .prv_offset = 0x800, >> .setup = byt_pwm_setup, >> }; >> >> static const struct lpss_device_desc bsw_pwm_dev_desc = { >> .flags = LPSS_SAVE_CTX | LPSS_NO_D3_DELAY, >> ^^^^^^^^^^^^^^ >> .prv_offset = 0x800, >> .setup = bsw_pwm_setup, >> }; > > Okay, it's a private space which has clock and reset gating, so means we still > need to handle it. Right I was about to say the same. As always, thank you for your reviews. Regards, Hans
Hi All, Here is hopefully the last version of this series, as everything seems to be ready for merging this now. The only difference from v9 is correcting some mistakes in the commit-msg of: [PATCH v10 06/17] pwm: lpss: Make pwm_lpss_apply() not rely on hardware state I plan is to push the entire series to drm-intel-next-queued (because of interdependencies) once the series has passed CI. This series has been tested (and re-tested after adding various bug-fixes) extensively. It has been tested on the following devices: -Asus T100TA BYT + CRC-PMIC PWM -Toshiba WT8-A BYT + CRC-PMIC PWM -Thundersoft TS178 BYT + CRC-PMIC PWM, inverse PWM -Asus T100HA CHT + CRC-PMIC PWM -Terra Pad 1061 BYT + LPSS PWM -Trekstor Twin 10.1 BYT + LPSS PWM -Asus T101HA CHT + LPSS PWM -GPD Pocket CHT + LPSS PWM -Acer One S1003 CHT + LPSS PWM Regards, Hans Changelog: Changes in v10: - Fixup the commit msg of: [PATCH v10 06/17] pwm: lpss: Make pwm_lpss_apply() not rely on hardware state Changes in v9: - Replace: [PATCH v8 06/17] pwm: lpss: Use pwm_lpss_restore() when restoring state on resume [PATCH v8 07/17] pwm: lpss: Always update state and set update bit with: [PATCH v9 06/17] pwm: lpss: Make pwm_lpss_apply() not rely on hardware state [PATCH v9 07/17] pwm: lpss: Remove suspend/resume handlers Changes in v8: - Add a new patch dealing with the ACPI/DSDT GFX0._PS3 code poking the PWM controller in unexpected ways on some Cherry Trail devices Changes in v7: - Fix a u64 divide leading to undefined reference to `__udivdi3' errors on 32 bit platforms by casting the divisor to an unsigned long Changes in v6: - Rebase on v5.9-rc1 - Adjust pwm-crc patches for pwm_state.period and .duty_cycle now being u64 Changes in v5: - Dropped the "pwm: lpss: Correct get_state result for base_unit == 0" patch. The base_unit == 0 condition should never happen and sofar it is unclear what the proper behavior / correct values to store in the pwm_state should be when this does happen. Since this patch was added as an extra pwm-lpss fix in v4 of this patch-set and otherwise is orthogonal to the of this patch-set just drop it (again). - "[PATCH 04/16] pwm: lpss: Add range limit check for the base_unit register value" - Use clamp_val(... instead of clam_t(unsigned long long, ... - "[PATCH 05/16] pwm: lpss: Add pwm_lpss_prepare_enable() helper" - This is a new patch in v5 of this patchset - [PATCH 06/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume - Use the new pwm_lpss_prepare_enable() helper Changes in v4: - "[PATCH v4 06/16] pwm: lpss: Correct get_state result for base_unit == 0" - This is a new patch in v4 of this patchset - "[PATCH v4 12/16] pwm: crc: Implement get_state() method" - Use DIV_ROUND_UP when calculating the period and duty_cycle values - "[PATCH v4 16/16] drm/i915: panel: Use atomic PWM API for devs with an external PWM controller" - Add a note to the commit message about the changes in pwm_disable_backlight() - Use the pwm_set/get_relative_duty_cycle() helpers Changes in v3: - "[PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value" - Use base_unit_range - 1 as maximum value for the clamp() - "[PATCH v3 05/15] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume" - This replaces the "pwm: lpss: Set SW_UPDATE bit when enabling the PWM" patch from previous versions of this patch-set, which really was a hack working around the resume issue which this patch fixes properly. - PATCH v3 6 - 11 pwm-crc changes: - Various small changes resulting from the reviews by Andy and Uwe, including some refactoring of the patches to reduce the amount of churn in the patch-set Changes in v2: - Fix coverletter subject - Drop accidentally included debugging patch - "[PATCH v3 02/15] ACPI / LPSS: Save Cherry Trail PWM ctx registers only once ( - Move #define LPSS_SAVE_CTX_ONCE define to group it with LPSS_SAVE_CTX