Hello Conor,
I found one remaining issue:
On Fri, Apr 21, 2023 at 10:27:09AM +0100, Conor Dooley wrote:
> +static u64 mchp_core_pwm_calc_duty(const struct pwm_state *state, u64 clk_rate,
> + u8 prescale, u8 period_steps)
> +{
> + u64 duty_steps, tmp;
> +
> + /*
> + * Calculate the duty cycle in multiples of the prescaled period:
> + * duty_steps = duty_in_ns / step_in_ns
> + * step_in_ns = (prescale * NSEC_PER_SEC) / clk_rate
> + * The code below is rearranged slightly to only divide once.
> + */
> + tmp = (prescale + 1) * NSEC_PER_SEC;
If prescale > 4 this overflows on 32bit archs, doesn't it?
(I think prescale + 1 is promoted to unsigned int, then the
multiplication is done and only then the range is extended to u64.
> + duty_steps = mul_u64_u64_div_u64(state->duty_cycle, clk_rate, tmp);
> +
> + return duty_steps;
> +}
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Wed, May 17, 2023 at 12:20:30PM +0200, Uwe Kleine-K?nig wrote:
> Hello Conor,
>
> I found one remaining issue:
>
> On Fri, Apr 21, 2023 at 10:27:09AM +0100, Conor Dooley wrote:
> > +static u64 mchp_core_pwm_calc_duty(const struct pwm_state *state, u64 clk_rate,
> > + u8 prescale, u8 period_steps)
> > +{
> > + u64 duty_steps, tmp;
> > +
> > + /*
> > + * Calculate the duty cycle in multiples of the prescaled period:
> > + * duty_steps = duty_in_ns / step_in_ns
> > + * step_in_ns = (prescale * NSEC_PER_SEC) / clk_rate
> > + * The code below is rearranged slightly to only divide once.
> > + */
> > + tmp = (prescale + 1) * NSEC_PER_SEC;
>
> If prescale > 4 this overflows on 32bit archs, doesn't it?
Ooh, I think you are right.
> (I think prescale + 1 is promoted to unsigned int, then the
> multiplication is done and only then the range is extended to u64.
I'll respin with an explicit cast.
Thanks,
Conor.