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

Uwe Kleine-König
2020-07-07T07:36:48Z

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

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.

> ###
>
> 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

--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |

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