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/ |