On Mon, Sep 28, 2020 at 10:35:46AM +0200, Uwe Kleine-König wrote:
> Hello Jonathan,
>
> On Sun, Sep 27, 2020 at 11:10:44PM +0200, Jonathan Neuschäfer wrote:
[...]
> > > > + if (state->enabled && duty != 0) {
> > > > + res = regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> > > > + if (res)
> > > > + return res;
> > > > +
> > > > + /* Disable the auto-off timer */
> > > > + res = regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> > > > + if (res)
> > > > + return res;
> > > > +
> > > > + return regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> > > > + } else {
> > > > + return regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> > > > + }
> > >
> > > This code is wrong for state->enabled = false.
> >
> > Why?
>
> Hm, I wonder the same. Probably I just misunderstood the code, sorry :-\
>
> > > How does the PWM behave when .apply is called? Does it complete the
> > > currently running period? Can it happen that when you switch from say
> > >
> > > .duty_cycle = 900 * TIME_BASE_NS (0x384)
> > > .period = 1800 * TIME_BASE_NS (0x708)
> > >
> > > to
> > >
> > > .duty_cycle = 300 * TIME_BASE_NS (0x12c)
> > > .period = 600 * TIME_BASE_NS (0x258)
> > >
> > > that a period with
> > >
> > > .duty_cycle = 388 * TIME_BASE_NS (0x184)
> > > .period = 1800 * TIME_BASE_NS (0x708)
> > >
> > > (because only NTXEC_REG_PERIOD_HIGH was written when the new period
> > > started) or something similar is emitted?
> >
> > Changes take effect after the low byte is written, so a result like 0x184
> > in the above example should not happen.
> >
> > When the period and duty cycle are both changed, it temporarily results
> > in an inconsistent state:
> >
> > - period = 1800ns, duty cycle = 900ns
> > - period = 600ns, duty cycle = 900ns (!)
> > - period = 600ns, duty cycle = 300ns
>
> Does this always happen, or only if a new cycle starts at an unlucky
> moment?
Just based on thinking about the code, the register writes setting this
intermediate state would always happen, but I don't know if the state
changes are applied in the middle of a running period, or when the new
period starts, because I can't measure the signal in good enough detail
at the moment.
> > The inconsistent state of duty cycle > period is handled gracefully by
> > the EC and it outputs a 100% duty cycle, as far as I can tell.
>
> OK.
>
> > I currently don't have a logic analyzer / oscilloscope to measure
> > whether we get full PWM periods, or some kind of glitch when the new
> > period starts in the middle of the last one.
>
> You can even check this with an LED using something like:
>
> pwm_apply(mypwm, {.enabled = true, .duty_cycle = $big, .period = $big});
> pwm_apply(mypwm, {.enabled = false, ... });
>
> . If the period is completed the LED is on for $big ns, if not the LED
> is on for a timespan that is probably hardly noticable with the human
> eye.
The longest configurable period is about 8ms, so it's not long enough to
see anything. However, after writing enable=0, it can take about a
second for the PWM signal to turn off... this hardware is a bit weird.
> > > > +}
> > > > +
> > > > +static struct pwm_ops ntxec_pwm_ops = {
> > > > + .apply = ntxec_pwm_apply,
> > >
> > > Please implement a .get_state() callback. And enable PWM_DEBUG during
> > > your tests.
> >
> > The device doesn't support reading back the PWM state. What should a
> > driver do in this case?
>
> Document it as a limitation, please.
Okay.
Thanks,
Jonathan Neuschäfer
Attachment:
signature.asc
Description: PGP signature
