Hi,
On 9/3/20 12:59 PM, Thierry Reding wrote:
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.
You're right at first the sentence had something about making
2 assumptions, then I added pre-conditions in there for it
to better describe the problem...
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 ..."?
Yes, oops. That is a small but important difference.
I'll do a v10 with your 2 Acked-by's added and both commit msg issues fixed.
Hopefully that will be the last version.
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@xxxxxxxxxx>
---
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@xxxxxxxxx>
Thank you.
Regards,
Hans
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
