Re: [PATCH v3 04/15] pwm: lpss: Add range limit check for the base_unit register value

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 7/7/20 9:34 AM, Uwe Kleine-König wrote:
On Mon, Jul 06, 2020 at 10:53:08PM +0200, Hans de Goede wrote:
Hi,

Thank you for your review and sorry for the slow reply.

No problem for me, I didn't hold my breath :-)
diff --git a/drivers/pwm/pwm-lpss.c b/drivers/pwm/pwm-lpss.c
index 43b1fc634af1..80d0f9c64f9d 100644
--- a/drivers/pwm/pwm-lpss.c
+++ b/drivers/pwm/pwm-lpss.c
@@ -97,6 +97,9 @@ static void pwm_lpss_prepare(struct pwm_lpss_chip *lpwm, struct pwm_device *pwm,
   	freq *= base_unit_range;
   	base_unit = DIV_ROUND_CLOSEST_ULL(freq, c);

DIV_ROUND_CLOSEST_ULL is most probably wrong, too. But I didn't spend
the time to actually confirm that.

Yes I saw your comment elsewhere that the PWM API defines rounding
in a certain direction, but fixing that falls outside of this patch.

Yeah, sure.

[...]
I hope this helps to explain what is going on a bit.

I will try to make sense of that and reply to the patch directly when I
succeeded.

In case it helps here is the datasheet for the LPSS PWM controller
(somewhat hard to find if you don't know what you are looking for):

https://cdrdv2.intel.com/v1/dl/getcontent/332065
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/atom-z8000-datasheet-vol-2.pdf

The first link contains a description about how the PWM controller works in
section 17.5  "SIO - Pulse Width Modulation (PWM)", the second link contains
all register definitions for the SoC and is not all that interesting other
then for verifying the existing register bits defines.

Regards,

Hans




###

As for the behavior on base_unit==0 in the get_state method,
as mentioned above I wrote that when I did not fully understood
how the controller works.

We really should never encounter this.

But if we do then I think closest to the truth would be:

state->period     = UINT_MAX;
state->duty_cycle = 0;

I'd say state->period = 1 & state->duty_cycle = 0 is a better
representation.

Best regards
Uwe





[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux