Hello Sean,
On Fri, Oct 18, 2024 at 08:46:28AM +0100, Sean Young wrote:
> > +static int nuvoton_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > + const struct pwm_state *state)
> > +{
> > + struct nuvoton_pwm *nvtpwm;
> > + unsigned int ch = pwm->hwpwm;
> > +
> > + nvtpwm = to_nuvoton_pwm(chip);
> > + if (state->enabled) {
> > + u64 duty_cycles, period_cycles;
> > +
> > + /* Calculate the duty and period cycles */
> > + duty_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
> > + state->duty_cycle, NSEC_PER_SEC);
> > + if (duty_cycles > 0xFFFF)
> > + duty_cycles = 0xFFFF;
> > +
> > + period_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
> > + state->period, NSEC_PER_SEC);
> > + if (period_cycles > 0xFFFF)
> > + period_cycles = 0xFFFF;
>
> If a period is not supported, return -EINVAL - maybe even do a dev_err().
> Same for the duty cycle above. It might make sense to calculate the period
> first, so that the error is more likely to be about the period than the
> duty cycle.
That's a wrong advice. Drivers are supposed to implement the highest
period possible that is not bigger than the requested one. So clamping
the value to 0xFFFF looks right.
However I wonder what happens in hardware if period_cycles == 0. If that
disables the hardware that is something to catch and return an error
for.
> Then again I don't know if all the drivers do this, but at least some of
> them do.
Yeah, and I hesitate to align them because their behaviour might be
relied on. But for new drivers the above rule applies.
(And with the new waveform stuff, consumers can rely on the rounding
rule and even query the resulting waveform before calling the equivalent
of pwm_apply_might_sleep().
> > + chip->ops = &nuvoton_pwm_ops;
>
> I think you can add chip->atomic = true; here
ack.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
