Re: [PATCH v9 06/17] pwm: lpss: Make pwm_lpss_apply() not rely on existing hardware state

[Thierry Reding <thierry.reding@gmail.com>]

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

On Thu, Sep 03, 2020 at 12:51:03PM +0200, Hans de Goede wrote:
> Before this commit pwm_lpss_apply() was making 2 assuming
> 2 pre-conditions were met by the existing hardware state:

I think that "making 2" is too much.

> 
> 1. That the base-unit and on-time-div read back from the
> control register are those actually in use, so that it
> can skip setting the update bit if the read-back value
> matches the desired values.
> 
> 2. That the controller is enabled when the cached
> pwm_state.enabled says that the controller is enabled.
> 
> As the long history of fixes for subtle (often suspend/resume)
> lpss-pwm issues shows, this assumptions are not necessary
> always true.
> 
> 1. Specifically is not true on some (*) Cherry Trail devices
> with a nasty GFX0._PS3 method which: a. saves the ctrl reg value.
> b. sets the base-unit to 0 and writes the update bit to apply/commit
> c. restores the original ctrl value without setting the update bit,
> so that the 0 base-unit value is still in use.
> 
> 2. Assumption 2. currently is true, but only because of the code which
> saves/restores the state on suspend/resume. By convention restoring the
> PWM state should be done by the PWM consumer and the presence of this
> code in the pmw-lpss driver is a bug. Therefor the save/restore code will
> be dropped in the next patch in this series, after which this assumption
> also is no longer true.
> 
> This commit changes the pwm_lpss_apply() to make any assumptions about the

Did you mean to say "... to _not_ make any assumptions ..."?

> state the hardware is in. Instead it makes pwm_lpss_apply() always fully
> program the PWM controller, making it much less fragile.
> 
> *) Seen on the Acer One 10 S1003, Lenovo Ideapad Miix 310 and 320 models
> and various Medion models.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pwm/pwm-lpss.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)

Other than the two small nits, this looks much more idiomatic and true
to the atomic API, so:

Acked-by: Thierry Reding <thierry.reding@gmail.com>

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