diff mbox series

[v8,07/17] pwm: lpss: Always update state and set update bit

Message ID 20200830125753.230420-8-hdegoede@redhat.com (mailing list archive)
State New, archived
Headers show
Series acpi/pwm/i915: Convert pwm-crc and i915 driver's PWM code to use the atomic PWM API | expand

Commit Message

Hans de Goede Aug. 30, 2020, 12:57 p.m. UTC
This commit removes a check where we would skip writing the ctrl register
and then setting the update bit in case the ctrl register already contains
the correct values.

In a perfect world skipping the update should be fine in these cases, but
on Cherry Trail devices the AML code in the GFX0 devices' PS0 and PS3
methods messes with the PWM controller.

The "ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase" patch
earlier in this series stops the GFX0._PS0 method from messing with the PWM
controller and on the DSDT-s inspected sofar the _PS3 method only reads
from the PWM controller (and turns it off before we get a change to do so):

    {
        PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
        PSAT |= 0x03
        Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
    }

The PWM controller getting turning off before we do this ourselves is
a bit annoying but not really an issue.

The problem this patch fixes comes from a new variant of the GFX0._PS3 code
messing with the PWM controller found on the Acer One 10 S1003 (1):

    {
        PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
        PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
        PWMT &= 0xFF0000FF
        PWMT |= 0xC0000000
        PWMC = PWMT /* \_SB_.PCI0.GFX0.PWMT */
        PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
        Sleep (0x64)
        PWMB &= 0x3FFFFFFF
        PWMC = PWMB /* \_SB_.PCI0.GFX0.PWMB */
        PSAT |= 0x03
        Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
    }

This "beautiful" piece of code clears the base-unit part of the ctrl-reg,
which effectively disables the controller, and it sets the update flag
to apply this change. Then after this it restores the original ctrl-reg
value, so we do not see it has mucked with the controller.

*But* it does not set the update flag when restoring the original value.
So the check to see if we can skip writing the ctrl register succeeds
but since the update flag was not set, the old base-unit value of 0 is
still in use and the PWM controller is effectively disabled.

IOW this PWM controller poking means that we cannot trust the base-unit /
on-time-div value we read back from the PWM controller since it may not
have been applied/committed. Thus we must always update the ctrl-register
and set the update bit.

1) And once I knew what to look for also in a bunch of other devices
including the popular Lenovo Ideapad Miix 310 and 320 models and
various Medion models.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v8:
- New patch in v8 of this patch-set
---
 drivers/pwm/pwm-lpss.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Comments

Andy Shevchenko Aug. 31, 2020, 8:56 a.m. UTC | #1
On Sun, Aug 30, 2020 at 02:57:43PM +0200, Hans de Goede wrote:
> This commit removes a check where we would skip writing the ctrl register
> and then setting the update bit in case the ctrl register already contains
> the correct values.
> 
> In a perfect world skipping the update should be fine in these cases, but
> on Cherry Trail devices the AML code in the GFX0 devices' PS0 and PS3
> methods messes with the PWM controller.
> 
> The "ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase" patch
> earlier in this series stops the GFX0._PS0 method from messing with the PWM
> controller and on the DSDT-s inspected sofar the _PS3 method only reads
> from the PWM controller (and turns it off before we get a change to do so):
> 
>     {
>         PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>         PSAT |= 0x03
>         Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
>     }
> 
> The PWM controller getting turning off before we do this ourselves is
> a bit annoying but not really an issue.
> 
> The problem this patch fixes comes from a new variant of the GFX0._PS3 code
> messing with the PWM controller found on the Acer One 10 S1003 (1):
> 
>     {
>         PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>         PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>         PWMT &= 0xFF0000FF
>         PWMT |= 0xC0000000
>         PWMC = PWMT /* \_SB_.PCI0.GFX0.PWMT */
>         PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>         Sleep (0x64)
>         PWMB &= 0x3FFFFFFF
>         PWMC = PWMB /* \_SB_.PCI0.GFX0.PWMB */
>         PSAT |= 0x03
>         Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
>     }
> 
> This "beautiful" piece of code clears the base-unit part of the ctrl-reg,
> which effectively disables the controller, and it sets the update flag
> to apply this change. Then after this it restores the original ctrl-reg
> value, so we do not see it has mucked with the controller.
> 
> *But* it does not set the update flag when restoring the original value.
> So the check to see if we can skip writing the ctrl register succeeds
> but since the update flag was not set, the old base-unit value of 0 is
> still in use and the PWM controller is effectively disabled.
> 
> IOW this PWM controller poking means that we cannot trust the base-unit /
> on-time-div value we read back from the PWM controller since it may not
> have been applied/committed. Thus we must always update the ctrl-register
> and set the update bit.
> 
> 1) And once I knew what to look for also in a bunch of other devices
> including the popular Lenovo Ideapad Miix 310 and 320 models and
> various Medion models.

Despite the above mentioned issue I'm always in favour of not micro-optimizing I/O.
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v8:
> - New patch in v8 of this patch-set
> ---
>  drivers/pwm/pwm-lpss.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
> index 9a7400c6fb6e..20f6b6d6f874 100644
> --- a/drivers/pwm/pwm-lpss.c
> +++ b/drivers/pwm/pwm-lpss.c
> @@ -85,7 +85,7 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
>  	unsigned long long on_time_div;
>  	unsigned long c = lpwm->info->clk_rate, base_unit_range;
>  	unsigned long long base_unit, freq = NSEC_PER_SEC;
> -	u32 orig_ctrl, ctrl;
> +	u32 ctrl;
>  
>  	do_div(freq, period_ns);
>  
> @@ -104,16 +104,14 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
>  	do_div(on_time_div, period_ns);
>  	on_time_div = 255ULL - on_time_div;
>  
> -	orig_ctrl = ctrl = pwm_lpss_read(pwm);
> +	ctrl = pwm_lpss_read(pwm);
>  	ctrl &= ~PWM_ON_TIME_DIV_MASK;
>  	ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT);
>  	ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT;
>  	ctrl |= on_time_div;
>  
> -	if (orig_ctrl != ctrl) {
> -		pwm_lpss_write(pwm, ctrl);
> -		pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
> -	}
> +	pwm_lpss_write(pwm, ctrl);
> +	pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
>  }
>  
>  static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
> -- 
> 2.28.0
>
Thierry Reding Aug. 31, 2020, 11:13 a.m. UTC | #2
On Sun, Aug 30, 2020 at 02:57:43PM +0200, Hans de Goede wrote:
> This commit removes a check where we would skip writing the ctrl register
> and then setting the update bit in case the ctrl register already contains
> the correct values.
> 
> In a perfect world skipping the update should be fine in these cases, but
> on Cherry Trail devices the AML code in the GFX0 devices' PS0 and PS3
> methods messes with the PWM controller.
> 
> The "ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase" patch
> earlier in this series stops the GFX0._PS0 method from messing with the PWM
> controller and on the DSDT-s inspected sofar the _PS3 method only reads
> from the PWM controller (and turns it off before we get a change to do so):
> 
>     {
>         PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>         PSAT |= 0x03
>         Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
>     }
> 
> The PWM controller getting turning off before we do this ourselves is
> a bit annoying but not really an issue.
> 
> The problem this patch fixes comes from a new variant of the GFX0._PS3 code
> messing with the PWM controller found on the Acer One 10 S1003 (1):
> 
>     {
>         PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>         PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>         PWMT &= 0xFF0000FF
>         PWMT |= 0xC0000000
>         PWMC = PWMT /* \_SB_.PCI0.GFX0.PWMT */
>         PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>         Sleep (0x64)
>         PWMB &= 0x3FFFFFFF
>         PWMC = PWMB /* \_SB_.PCI0.GFX0.PWMB */
>         PSAT |= 0x03
>         Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
>     }
> 
> This "beautiful" piece of code clears the base-unit part of the ctrl-reg,
> which effectively disables the controller, and it sets the update flag
> to apply this change. Then after this it restores the original ctrl-reg
> value, so we do not see it has mucked with the controller.
> 
> *But* it does not set the update flag when restoring the original value.
> So the check to see if we can skip writing the ctrl register succeeds
> but since the update flag was not set, the old base-unit value of 0 is
> still in use and the PWM controller is effectively disabled.
> 
> IOW this PWM controller poking means that we cannot trust the base-unit /
> on-time-div value we read back from the PWM controller since it may not
> have been applied/committed. Thus we must always update the ctrl-register
> and set the update bit.

Doesn't this now make patch 6/17 obsolete?

Thierry
Hans de Goede Aug. 31, 2020, 11:26 a.m. UTC | #3
Hi,

On 8/31/20 1:13 PM, Thierry Reding wrote:
> On Sun, Aug 30, 2020 at 02:57:43PM +0200, Hans de Goede wrote:
>> This commit removes a check where we would skip writing the ctrl register
>> and then setting the update bit in case the ctrl register already contains
>> the correct values.
>>
>> In a perfect world skipping the update should be fine in these cases, but
>> on Cherry Trail devices the AML code in the GFX0 devices' PS0 and PS3
>> methods messes with the PWM controller.
>>
>> The "ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase" patch
>> earlier in this series stops the GFX0._PS0 method from messing with the PWM
>> controller and on the DSDT-s inspected sofar the _PS3 method only reads
>> from the PWM controller (and turns it off before we get a change to do so):
>>
>>      {
>>          PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>>          PSAT |= 0x03
>>          Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
>>      }
>>
>> The PWM controller getting turning off before we do this ourselves is
>> a bit annoying but not really an issue.
>>
>> The problem this patch fixes comes from a new variant of the GFX0._PS3 code
>> messing with the PWM controller found on the Acer One 10 S1003 (1):
>>
>>      {
>>          PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>>          PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>>          PWMT &= 0xFF0000FF
>>          PWMT |= 0xC0000000
>>          PWMC = PWMT /* \_SB_.PCI0.GFX0.PWMT */
>>          PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>>          Sleep (0x64)
>>          PWMB &= 0x3FFFFFFF
>>          PWMC = PWMB /* \_SB_.PCI0.GFX0.PWMB */
>>          PSAT |= 0x03
>>          Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
>>      }
>>
>> This "beautiful" piece of code clears the base-unit part of the ctrl-reg,
>> which effectively disables the controller, and it sets the update flag
>> to apply this change. Then after this it restores the original ctrl-reg
>> value, so we do not see it has mucked with the controller.
>>
>> *But* it does not set the update flag when restoring the original value.
>> So the check to see if we can skip writing the ctrl register succeeds
>> but since the update flag was not set, the old base-unit value of 0 is
>> still in use and the PWM controller is effectively disabled.
>>
>> IOW this PWM controller poking means that we cannot trust the base-unit /
>> on-time-div value we read back from the PWM controller since it may not
>> have been applied/committed. Thus we must always update the ctrl-register
>> and set the update bit.
> 
> Doesn't this now make patch 6/17 obsolete?

No, there is no guarantee we will get any changes soon after resume,
so we must restore the state properly on resume, before 5.17
we were just blindly restoring the old ctrl reg state, but
if either the freq-div or the duty-cycle changes, we should
also set the update bit in that case to apply the new freq-div/
duty-cycle.

This actually also helps with that case since patch 6/17 uses
pwm_lpss_prepare and this makes pwm_lpss_prepare set the
update but unconditionally.

Also on resume we most do the set the enable bit vs set
the update bit in the right order, depending on the
generation of the SoC in which the PWM controller is
embedded. 6/17 fixes all this by resume, by treating
resume as a special case of apply() taking all the
steps apply does.

Regards,

Hans
Hans de Goede Aug. 31, 2020, 11:50 a.m. UTC | #4
Hi,

On 8/31/20 10:56 AM, Andy Shevchenko wrote:
> On Sun, Aug 30, 2020 at 02:57:43PM +0200, Hans de Goede wrote:
>> This commit removes a check where we would skip writing the ctrl register
>> and then setting the update bit in case the ctrl register already contains
>> the correct values.
>>
>> In a perfect world skipping the update should be fine in these cases, but
>> on Cherry Trail devices the AML code in the GFX0 devices' PS0 and PS3
>> methods messes with the PWM controller.
>>
>> The "ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase" patch
>> earlier in this series stops the GFX0._PS0 method from messing with the PWM
>> controller and on the DSDT-s inspected sofar the _PS3 method only reads
>> from the PWM controller (and turns it off before we get a change to do so):
>>
>>      {
>>          PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>>          PSAT |= 0x03
>>          Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
>>      }
>>
>> The PWM controller getting turning off before we do this ourselves is
>> a bit annoying but not really an issue.
>>
>> The problem this patch fixes comes from a new variant of the GFX0._PS3 code
>> messing with the PWM controller found on the Acer One 10 S1003 (1):
>>
>>      {
>>          PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>>          PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>>          PWMT &= 0xFF0000FF
>>          PWMT |= 0xC0000000
>>          PWMC = PWMT /* \_SB_.PCI0.GFX0.PWMT */
>>          PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
>>          Sleep (0x64)
>>          PWMB &= 0x3FFFFFFF
>>          PWMC = PWMB /* \_SB_.PCI0.GFX0.PWMB */
>>          PSAT |= 0x03
>>          Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
>>      }
>>
>> This "beautiful" piece of code clears the base-unit part of the ctrl-reg,
>> which effectively disables the controller, and it sets the update flag
>> to apply this change. Then after this it restores the original ctrl-reg
>> value, so we do not see it has mucked with the controller.
>>
>> *But* it does not set the update flag when restoring the original value.
>> So the check to see if we can skip writing the ctrl register succeeds
>> but since the update flag was not set, the old base-unit value of 0 is
>> still in use and the PWM controller is effectively disabled.
>>
>> IOW this PWM controller poking means that we cannot trust the base-unit /
>> on-time-div value we read back from the PWM controller since it may not
>> have been applied/committed. Thus we must always update the ctrl-register
>> and set the update bit.
>>
>> 1) And once I knew what to look for also in a bunch of other devices
>> including the popular Lenovo Ideapad Miix 310 and 320 models and
>> various Medion models.
> 
> Despite the above mentioned issue I'm always in favour of not micro-optimizing I/O.
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thank you.

Regards,

Hans



> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v8:
>> - New patch in v8 of this patch-set
>> ---
>>   drivers/pwm/pwm-lpss.c | 10 ++++------
>>   1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
>> index 9a7400c6fb6e..20f6b6d6f874 100644
>> --- a/drivers/pwm/pwm-lpss.c
>> +++ b/drivers/pwm/pwm-lpss.c
>> @@ -85,7 +85,7 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
>>   	unsigned long long on_time_div;
>>   	unsigned long c = lpwm->info->clk_rate, base_unit_range;
>>   	unsigned long long base_unit, freq = NSEC_PER_SEC;
>> -	u32 orig_ctrl, ctrl;
>> +	u32 ctrl;
>>   
>>   	do_div(freq, period_ns);
>>   
>> @@ -104,16 +104,14 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
>>   	do_div(on_time_div, period_ns);
>>   	on_time_div = 255ULL - on_time_div;
>>   
>> -	orig_ctrl = ctrl = pwm_lpss_read(pwm);
>> +	ctrl = pwm_lpss_read(pwm);
>>   	ctrl &= ~PWM_ON_TIME_DIV_MASK;
>>   	ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT);
>>   	ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT;
>>   	ctrl |= on_time_div;
>>   
>> -	if (orig_ctrl != ctrl) {
>> -		pwm_lpss_write(pwm, ctrl);
>> -		pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
>> -	}
>> +	pwm_lpss_write(pwm, ctrl);
>> +	pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
>>   }
>>   
>>   static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)
>> -- 
>> 2.28.0
>>
>
Thierry Reding Aug. 31, 2020, 1:31 p.m. UTC | #5
On Mon, Aug 31, 2020 at 01:26:46PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 8/31/20 1:13 PM, Thierry Reding wrote:
> > On Sun, Aug 30, 2020 at 02:57:43PM +0200, Hans de Goede wrote:
> > > This commit removes a check where we would skip writing the ctrl register
> > > and then setting the update bit in case the ctrl register already contains
> > > the correct values.
> > > 
> > > In a perfect world skipping the update should be fine in these cases, but
> > > on Cherry Trail devices the AML code in the GFX0 devices' PS0 and PS3
> > > methods messes with the PWM controller.
> > > 
> > > The "ACPI / LPSS: Resume Cherry Trail PWM controller in no-irq phase" patch
> > > earlier in this series stops the GFX0._PS0 method from messing with the PWM
> > > controller and on the DSDT-s inspected sofar the _PS3 method only reads
> > > from the PWM controller (and turns it off before we get a change to do so):
> > > 
> > >      {
> > >          PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> > >          PSAT |= 0x03
> > >          Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
> > >      }
> > > 
> > > The PWM controller getting turning off before we do this ourselves is
> > > a bit annoying but not really an issue.
> > > 
> > > The problem this patch fixes comes from a new variant of the GFX0._PS3 code
> > > messing with the PWM controller found on the Acer One 10 S1003 (1):
> > > 
> > >      {
> > >          PWMB = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> > >          PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> > >          PWMT &= 0xFF0000FF
> > >          PWMT |= 0xC0000000
> > >          PWMC = PWMT /* \_SB_.PCI0.GFX0.PWMT */
> > >          PWMT = PWMC /* \_SB_.PCI0.GFX0.PWMC */
> > >          Sleep (0x64)
> > >          PWMB &= 0x3FFFFFFF
> > >          PWMC = PWMB /* \_SB_.PCI0.GFX0.PWMB */
> > >          PSAT |= 0x03
> > >          Local0 = PSAT /* \_SB_.PCI0.GFX0.PSAT */
> > >      }
> > > 
> > > This "beautiful" piece of code clears the base-unit part of the ctrl-reg,
> > > which effectively disables the controller, and it sets the update flag
> > > to apply this change. Then after this it restores the original ctrl-reg
> > > value, so we do not see it has mucked with the controller.
> > > 
> > > *But* it does not set the update flag when restoring the original value.
> > > So the check to see if we can skip writing the ctrl register succeeds
> > > but since the update flag was not set, the old base-unit value of 0 is
> > > still in use and the PWM controller is effectively disabled.
> > > 
> > > IOW this PWM controller poking means that we cannot trust the base-unit /
> > > on-time-div value we read back from the PWM controller since it may not
> > > have been applied/committed. Thus we must always update the ctrl-register
> > > and set the update bit.
> > 
> > Doesn't this now make patch 6/17 obsolete?
> 
> No, there is no guarantee we will get any changes soon after resume,
> so we must restore the state properly on resume, before 5.17
> we were just blindly restoring the old ctrl reg state, but
> if either the freq-div or the duty-cycle changes, we should
> also set the update bit in that case to apply the new freq-div/
> duty-cycle.

Hm... I didn't realize the driver was already saving/restoring context
before this. And from a quick look through the subsystem it looks like
I've done a pretty poor job of enforcing the "no context save/restore
from PWM drivers" rule. There are some cases that have had this support
since before we realized that this is problematic, but I think at least
pwm-img is newer than that and should never have had that code either.

> This actually also helps with that case since patch 6/17 uses
> pwm_lpss_prepare and this makes pwm_lpss_prepare set the
> update but unconditionally.
> 
> Also on resume we most do the set the enable bit vs set
> the update bit in the right order, depending on the
> generation of the SoC in which the PWM controller is
> embedded. 6/17 fixes all this by resume, by treating
> resume as a special case of apply() taking all the
> steps apply does.

As I mentioned earlier this works only under the assumption that the
suspend/resume order is correct. And that's possibly true for LPSS. It
won't work in the general case, though, because a backlight could end up
suspending/resuming completely out of sync with the rest of the display
pipeline and that's not something that we want.

I would expect that on i915 you also do have a controlled call sequence
that LPSS is part of, so I would expect that some consumer would
eventually call pwm_apply_state() and then any new settings would get
applied. Yes, that may perhaps be not immediately at the point where the
LPSS resumes, but it should be exactly at the point where the consumer
wants to enable it and therefore the only point where you can expect it
to make sense to enable the PWM.

Anyway, if this really turns out to be the only way to make this work I
can't object to it. But if you do rely on this, perhaps just make a
mental note that this can lead to sequencing problems that you may
potentially run into at some point.

Thierry
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 9a7400c6fb6e..20f6b6d6f874 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -85,7 +85,7 @@  static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
 	unsigned long long on_time_div;
 	unsigned long c = lpwm->info->clk_rate, base_unit_range;
 	unsigned long long base_unit, freq = NSEC_PER_SEC;
-	u32 orig_ctrl, ctrl;
+	u32 ctrl;
 
 	do_div(freq, period_ns);
 
@@ -104,16 +104,14 @@  static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
 	do_div(on_time_div, period_ns);
 	on_time_div = 255ULL - on_time_div;
 
-	orig_ctrl = ctrl = pwm_lpss_read(pwm);
+	ctrl = pwm_lpss_read(pwm);
 	ctrl &= ~PWM_ON_TIME_DIV_MASK;
 	ctrl &= ~((base_unit_range - 1) << PWM_BASE_UNIT_SHIFT);
 	ctrl |= (u32) base_unit << PWM_BASE_UNIT_SHIFT;
 	ctrl |= on_time_div;
 
-	if (orig_ctrl != ctrl) {
-		pwm_lpss_write(pwm, ctrl);
-		pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
-	}
+	pwm_lpss_write(pwm, ctrl);
+	pwm_lpss_write(pwm, ctrl | PWM_SW_UPDATE);
 }
 
 static inline void pwm_lpss_cond_enable(struct pwm_device *pwm, bool cond)