Re: [PATCH v3 4/7] pwm: ntxec: Add driver for PWM function in Netronix EC

[Jonathan Neuschäfer]

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