On Fri, Jan 17, 2025 at 03:11:29PM +0100, Mathieu Dubois-Briand wrote:
> On Fri Jan 17, 2025 at 10:33 AM CET, Uwe Kleine-König wrote:
> > Hello Mathieu,
> >
> > On Mon, Jan 13, 2025 at 01:42:27PM +0100, mathieu.dubois-briand@xxxxxxxxxxx wrote:
> > > From: Kamel Bouhara <kamel.bouhara@xxxxxxxxxxx>
> ...
> > > +static int max7360_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > + const struct pwm_state *state)
> > > +{
> > > + struct max7360_pwm *max7360_pwm;
> > > + u64 duty_steps;
> > > + int ret;
> > > +
> > > + if (state->polarity != PWM_POLARITY_NORMAL)
> > > + return -EINVAL;
> > > +
> > > + if (state->period != MAX7360_PWM_PERIOD_NS) {
> > > + dev_warn(&chip->dev,
> > > + "unsupported pwm period: %llu, should be %u\n",
> > > + state->period, MAX7360_PWM_PERIOD_NS);
> > > + return -EINVAL;
> >
> > Please don't emit error messages in .apply(). Also a driver is supposed
> > to round down .period, so any value >= MAX7360_PWM_PERIOD_NS should be
> > accepted.
> >
> > Also note that you might want to implement the waveform callbacks
> > instead of .apply() and .get_state() for the more modern abstraction
> > (with slightly different rounding rules).
> >
>
> Sure, I just switched to the waveform callbacks, it was quite
> straightforward.
sounds great. Note that the detail in rounding that is different for
waveforms is that a value that cannot be round down to a valid value
(because it's too small) is round up. This is a bit ugly in the drivers
but simplifies usage considerably. So you never return -EINVAL because
the values don't fit.
> Thanks for the reproduce steps: I saw the bug and fixed it. Also
> MAX7360_PWM_MAX_RES had to be set to 255 and not 256...
A good test (for a driver doing .apply/.get_state) is a sequence of
increasing settings. So something like:
for p in range(1000, 10000):
pwm_apply(period=p, duty_cycle=0, ...)
and also do the same for duty_cycle and also try decreasing series.
Best regards
Uwe
Attachment:
signature.asc
Description: PGP signature
