Hello Jonathan,
On Sun, Sep 27, 2020 at 11:10:44PM +0200, Jonathan Neuschäfer wrote:
> On Fri, Sep 25, 2020 at 08:30:37AM +0200, Uwe Kleine-König wrote:
> > > + if (period > MAX_PERIOD_NS) {
> > > + dev_warn(pwm->dev,
> > > + "Period is not representable in 16 bits after division by %u: %u\n",
> > > + TIME_BASE_NS, period);
> >
> > No error messages in .apply() please; this might spam the kernel log.
> >
> > Also the expectation when a too big period is requested is to configure
> > for the biggest possible period. So just do:
> >
> > if (period > MAX_PERIOD_NS) {
> > period = MAX_PERIOD_NS;
> >
> > if (duty > period)
> > duty = period;
> > }
> >
> > (or something equivalent).
>
> Okay, I'll adjust it.
>
> > > + /*
> > > + * Writing a duty cycle of zone puts the device into a state where
> >
> > What is "zone"? A mixture of zero and one and so approximately 0.5?
>
> Oops, that's a typo. I just meant "zero".
>
> > > + * writing a higher duty cycle doesn't result in the brightness that it
> > > + * usually results in. This can be fixed by cycling the ENABLE register.
> > > + *
> > > + * As a workaround, write ENABLE=0 when the duty cycle is zero.
> > > + */
> > > + 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?
> 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.
> > > +}
> > > +
> > > +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.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
