On Sat, Jun 20, 2020 at 02:17:47PM +0200, Hans de Goede wrote:
When the user requests a high enough period ns value, then the calculations in pwm_lpss_prepare() might result in a base_unit value of 0.
But according to the data-sheet the way the PWM controller works is that each input clock-cycle the base_unit gets added to a N bit counter and that counter overflowing determines the PWM output frequency. Adding 0 to the counter is a no-op. The data-sheet even explicitly states that writing 0 to the base_unit bits will result in the PWM outputting a continuous 0 signal.
When the user requestes a low enough period ns value, then the calculations in pwm_lpss_prepare() might result in a base_unit value which is bigger then base_unit_range - 1. Currently the codes for this deals with this by applying a mask:
base_unit &= (base_unit_range - 1);
But this means that we let the value overflow the range, we throw away the higher bits and store whatever value is left in the lower bits into the register leading to a random output frequency, rather then clamping the output frequency to the highest frequency which the hardware can do.
This commit fixes both issues by clamping the base_unit value to be between 1 and (base_unit_range - 1).
Fixes: 684309e5043e ("pwm: lpss: Avoid potential overflow of base_unit") Signed-off-by: Hans de Goede hdegoede@redhat.com
Changes in v3:
- Change upper limit of clamp to (base_unit_range - 1)
- Add Fixes tag
drivers/pwm/pwm-lpss.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
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.
- /* base_unit must not be 0 and we also want to avoid overflowing it */
- base_unit = clamp_t(unsigned long long, base_unit, 1,
base_unit_range - 1);
.get_state seems to handle base_unit == 0 just fine?! Though this doesn't look right either ...
Best regards Uwe