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