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

[Uwe Kleine-König]

Hello Jonathan,

On Thu, Sep 24, 2020 at 09:24:52PM +0200, Jonathan Neuschäfer wrote:
> The Netronix EC provides a PWM output which is used for the backlight
> on some ebook readers. This patches adds a driver for the PWM output.
> 
> The .get_state callback is not implemented, because the PWM state can't
> be read back from the hardware.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@xxxxxxx>
> ---
> 
> v3:
> - Relicense as GPLv2 or later
> - Add email address to copyright line
> - Remove OF compatible string and don't include linux/of_device.h
> - Fix bogus ?: in return line
> - Don't use a comma after sentinels
> - Avoid ret |= ... pattern
> - Move 8-bit register conversion to ntxec.h
> 
> v2:
> - https://lore.kernel.org/lkml/20200905133230.1014581-6-j.neuschaefer@xxxxxxx/
> - Various grammar and style improvements, as suggested by Uwe Kleine-König,
>   Lee Jones, and Alexandre Belloni
> - Switch to regmap
> - Prefix registers with NTXEC_REG_
> - Add help text to the Kconfig option
> - Use the .apply callback instead of the old API
> - Add a #define for the time base (125ns)
> - Don't change device state in .probe; this avoids multiple problems
> - Rework division and overflow check logic to perform divisions in 32 bits
> - Avoid setting duty cycle to zero, to work around a hardware quirk
> ---
>  drivers/pwm/Kconfig     |   8 ++
>  drivers/pwm/Makefile    |   1 +
>  drivers/pwm/pwm-ntxec.c | 161 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 170 insertions(+)
>  create mode 100644 drivers/pwm/pwm-ntxec.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7dbcf6973d335..530dfda38d65e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -350,6 +350,14 @@ config PWM_MXS
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-mxs.
> 
> +config PWM_NTXEC
> +	tristate "Netronix embedded controller PWM support"
> +	depends on MFD_NTXEC
> +	help
> +	  Say yes here if you want to support the PWM output of the embedded
> +	  controller found in certain e-book readers designed by the ODM
> +	  Netronix.

Is it only me who had to look up what ODM means? If not, maybe spell it
out?

> +
>  config PWM_OMAP_DMTIMER
>  	tristate "OMAP Dual-Mode Timer PWM support"
>  	depends on OF
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 2c2ba0a035577..1cc50dba22d1b 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
>  obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
>  obj-$(CONFIG_PWM_MTK_DISP)	+= pwm-mtk-disp.o
>  obj-$(CONFIG_PWM_MXS)		+= pwm-mxs.o
> +obj-$(CONFIG_PWM_NTXEC)		+= pwm-ntxec.o
>  obj-$(CONFIG_PWM_OMAP_DMTIMER)	+= pwm-omap-dmtimer.o
>  obj-$(CONFIG_PWM_PCA9685)	+= pwm-pca9685.o
>  obj-$(CONFIG_PWM_PXA)		+= pwm-pxa.o
> diff --git a/drivers/pwm/pwm-ntxec.c b/drivers/pwm/pwm-ntxec.c
> new file mode 100644
> index 0000000000000..50da2dc14bb03
> --- /dev/null
> +++ b/drivers/pwm/pwm-ntxec.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * The Netronix embedded controller is a microcontroller found in some
> + * e-book readers designed by the ODM Netronix, Inc. It contains RTC,
> + * battery monitoring, system power management, and PWM functionality.
> + *
> + * This driver implements PWM output.
> + *
> + * Copyright 2020 Jonathan Neuschäfer <j.neuschaefer@xxxxxxx>
> + */
> +
> +#include <linux/mfd/ntxec.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +
> +struct ntxec_pwm {
> +	struct device *dev;
> +	struct ntxec *ec;
> +	struct pwm_chip chip;
> +};
> +
> +static struct ntxec_pwm *pwmchip_to_pwm(struct pwm_chip *chip)
> +{
> +	return container_of(chip, struct ntxec_pwm, chip);
> +}
> +
> +#define NTXEC_REG_AUTO_OFF_HI	0xa1
> +#define NTXEC_REG_AUTO_OFF_LO	0xa2
> +#define NTXEC_REG_ENABLE	0xa3
> +#define NTXEC_REG_PERIOD_LOW	0xa4
> +#define NTXEC_REG_PERIOD_HIGH	0xa5
> +#define NTXEC_REG_DUTY_LOW	0xa6
> +#define NTXEC_REG_DUTY_HIGH	0xa7
> +
> +/*
> + * 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 * 0x10000 - 1)

The maximal configurable period length is 0xffff, so I would have
expected MAX_PERIOD_NS to be 0xffff * TIME_BASE_NS?

> +static int ntxec_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm_dev,
> +			   const struct pwm_state *state)
> +{
> +	struct ntxec_pwm *pwm = pwmchip_to_pwm(pwm_dev->chip);
> +	unsigned int duty = state->duty_cycle;
> +	unsigned int period = state->period;
> +	int res = 0;
> +

I assume your device only supports normal polarity? If so, please check
for it here and point out this limitation in the header (in the format
that is for example used in pwm-sifive.c to make it easy to grep for
that).

> +	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).

> +		return -ERANGE;
> +	}
> +
> +	period /= TIME_BASE_NS;
> +	duty /= TIME_BASE_NS;
> +
> +	res = regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_HIGH, ntxec_reg8(period >> 8));
> +	if (res)
> +		return res;
> +
> +	res = regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_LOW, ntxec_reg8(period));
> +	if (res)
> +		return res;
> +
> +	res = regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_HIGH, ntxec_reg8(duty >> 8));
> +	if (res)
> +		return res;
> +
> +	res = regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_LOW, ntxec_reg8(duty));
> +	if (res)
> +		return res;
> +
> +	/*
> +	 * 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?

> +	 * 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.

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?

> +}
> +
> +static struct pwm_ops ntxec_pwm_ops = {
> +	.apply = ntxec_pwm_apply,

Please implement a .get_state() callback. And enable PWM_DEBUG during
your tests.

> +	.owner = THIS_MODULE,
> +};
> +
> +static int ntxec_pwm_probe(struct platform_device *pdev)
> +{
> +	struct ntxec *ec = dev_get_drvdata(pdev->dev.parent);
> +	struct ntxec_pwm *pwm;

Please don't call this variable pwm. I would expect that a variable with
this name is of type pwm_device. I would have called it "ddata" (and the
type would be named ntxec_pwm_ddata for me); another usual name is "priv".

> +	struct pwm_chip *chip;
> +	int res;
> +
> +	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
> +	if (!pwm)
> +		return -ENOMEM;
> +
> +	pwm->ec = ec;
> +	pwm->dev = &pdev->dev;
> +
> +	chip = &pwm->chip;
> +	chip->dev = &pdev->dev;
> +	chip->ops = &ntxec_pwm_ops;
> +	chip->base = -1;
> +	chip->npwm = 1;
> +
> +	res = pwmchip_add(chip);
> +	if (res < 0)
> +		return res;
> +
> +	platform_set_drvdata(pdev, pwm);

If you do the platform_set_drvdata earlier you can just do

	return pwmchip_add(chip);

> +
> +	return 0;
> +}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature