Message-ID: <20201126231931.GE456020@latitude>
Date:   Fri, 27 Nov 2020 00:19:31 +0100
From:   Jonathan Neuschäfer <j.neuschaefer@....net>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Jonathan Neuschäfer <j.neuschaefer@....net>,
        Thierry Reding <thierry.reding@...il.com>,
        linux-kernel@...r.kernel.org,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Heiko Stuebner <heiko@...ech.de>, linux-pwm@...r.kernel.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Fabio Estevam <festevam@...il.com>, linux-rtc@...r.kernel.org,
        Arnd Bergmann <arnd@...db.de>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Daniel Palmer <daniel@...f.com>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Andreas Kemnade <andreas@...nade.info>,
        NXP Linux Team <linux-imx@....com>,
        devicetree@...r.kernel.org, Stephan Gerhold <stephan@...hold.net>,
        allen <allen.chen@....com.tw>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Lubomir Rintel <lkundrak@...sk>,
        Rob Herring <robh+dt@...nel.org>,
        Lee Jones <lee.jones@...aro.org>,
        linux-arm-kernel@...ts.infradead.org,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Mark Brown <broonie@...nel.org>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Heiko Stuebner <heiko.stuebner@...obroma-systems.com>,
        Josua Mayer <josua.mayer@....eu>,
        Shawn Guo <shawnguo@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v4 4/7] pwm: ntxec: Add driver for PWM function in
 Netronix EC

On Tue, Nov 24, 2020 at 09:20:19AM +0100, Uwe Kleine-König wrote:
> Hello,
> 
> On Sun, Nov 22, 2020 at 11:27:36PM +0100, Jonathan Neuschäfer wrote:
[...]
> > +/*
> > + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are
> > + * measured in this unit.
> > + */
> > +#define TIME_BASE_NS 125
> > +
> > +/*
> > + * The maximum input value (in nanoseconds) is determined by the time base and
> > + * the range of the hardware registers that hold the converted value.
> > + * It fits into 32 bits, so we can do our calculations in 32 bits as well.
> > + */
> > +#define MAX_PERIOD_NS (TIME_BASE_NS * 0xffff)
> > +
> > +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> > +			   const struct pwm_state *state)
> > +{
> > +	struct ntxec_pwm *priv = pwmchip_to_priv(pwm_dev->chip);
> > +	unsigned int duty = state->duty_cycle;
> > +	unsigned int period = state->period;
> 
> state->duty_cycle and state->period are u64, so you're losing
> information here. Consider state->duty_cycle = 0x100000001 and
> state->period = 0x200000001.

Oh, good point, I didn't notice the truncation.

The reason I picked unsigned int was to avoid a 64-bit division;
I suppose I can do something like this:

    period = (u32)period / TIME_BASE_NS;
    duty = (u32)duty / TIME_BASE_NS;

> > +	int res = 0;
> > +
> > +	if (state->polarity != PWM_POLARITY_NORMAL)
> > +		return -EINVAL;
> > +
> > +	if (period > MAX_PERIOD_NS) {
> > +		period = MAX_PERIOD_NS;
> > +
> > +		if (duty > period)
> > +			duty = period;
> > +	}
> > +
> > +	period /= TIME_BASE_NS;
> > +	duty /= TIME_BASE_NS;
> > +
> > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> > +	if (res)
> > +		return res;
> 
> I wonder if you can add some logic to the regmap in the mfd driver such
> that ntxec_reg8 isn't necessary for all users.

I think that would involve:

1. adding custom register access functions to the regmap, which decide
   based on the register number whether a register needs 8-bit or 16-bit
   access. So far I have avoided information about registers into the
   main driver, when the registers are only used in the sub-drivers.

or

2. switching the regmap configuration to little endian, which would be
   advantageous for 8-bit registers, inconsequential for 16-bit
   registers that consist of independent high and low halves, and wrong
   for the 16-bit registers 0x41, which reads the battery voltage ADC
   value. It is also different from how the vendor kernel treats 16-bit
   registers.

Perhaps there is another option that I haven't considered yet.

> 
> > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> > +	if (res)
> > +		return res;
> > +
> > +	res = regmap_write(priv->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> > +	if (res)
> > +		return res;
> 
> I think I already asked, but I don't remember the reply: What happens to
> the output between these writes? A comment here about this would be
> suitable.

I will add something like the following:

/*
 * Changes to the period and duty cycle take effect as soon as the
 * corresponding low byte is written, so the hardware may be configured
 * to an inconsistent state after the period is written and before the
 * duty cycle is fully written. If, in such a case, the old duty cycle
 * is longer than the new period, the EC will output 100% for a moment.
 */

> 
> > +
> > +	/*
> > +	 * Writing a duty cycle of zero puts the device into a state where
> > +	 * 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(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(1));
> > +		if (res)
> > +			return res;
> > +
> > +		/* Disable the auto-off timer */
> > +		res = regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_HI, ntxec_reg8(0xff));
> > +		if (res)
> > +			return res;
> > +
> > +		return regmap_write(priv->ec->regmap, NTXEC_REG_AUTO_OFF_LO, ntxec_reg8(0xff));
> > +	} else {
> > +		return regmap_write(priv->ec->regmap, NTXEC_REG_ENABLE, ntxec_reg8(0));
> > +	}
> > +}
> > +
> > +static struct pwm_ops ntxec_pwm_ops = {
> 
> This can be const.

Indeed, I'll change it.

> > +	.apply = ntxec_pwm_apply,
> 
> /*
>  * The current state cannot be read out, so there is no .get_state
>  * callback.
>  */
> 
> Hmm, at least you could provice a .get_state() callback that reports the
> setting that was actually implemented for in the last call to .apply()?

Yes... I see two options:

1. Caching the state in the driver's private struct. I'm not completely
   convinced of the value, given that the information is mostly
   available in the PWM core already (except for the adjustments that
   the driver makes).

2. Writing the adjusted state back into pwm_dev->state (via pwm_set_*).
   This seems a bit dirty.

> @Thierry: Do you have concerns here? Actually it would be more effective
> to have a callback (like .apply()) that modfies its pwm_state
> accordingly. (Some drivers did that in the past, but I changed that to
> get an uniform behaviour in 71523d1812aca61e32e742e87ec064e3d8c615e1.)
> The downside is that people have to understand that concept to properly
> use it. I'm torn about the right approach.

General guidance for such cases when the state can't be read back from
the hardware would be appreciated.


Thanks,
Jonathan Neuschäfer

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists