| Message ID | 20200905133230.1014581-6-j.neuschaefer@gmx.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Netronix embedded controller driver for Kobo and Tolino ebook readers | expand |
On Sat, 05 Sep 2020, Andy Shevchenko wrote: > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net> > 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. > > > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > --- > > > > v2: > > - 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 | 160 ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 169 insertions(+) > > create mode 100644 drivers/pwm/pwm-ntxec.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index 7dbcf6973d335..7fd17c6cda95e 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 && OF > > > I don’t see need to reduce test coverage and use of the driver by sticking > with OF. Actually it’s not used. If the device is only known to boot with OF, then it's pointless building it when !OF. If you want to increase test coverage enable COMPILE_TEST instead.
On Tue, Sep 8, 2020 at 11:14 AM Lee Jones <lee.jones@linaro.org> wrote: > On Sat, 05 Sep 2020, Andy Shevchenko wrote: > > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > wrote: ... > > > +config PWM_NTXEC > > > + tristate "Netronix embedded controller PWM support" > > > > > > > > > > > + depends on MFD_NTXEC && OF > > > > > > I don’t see need to reduce test coverage and use of the driver by sticking > > with OF. Actually it’s not used. > > If the device is only known to boot with OF, then it's pointless > building it when !OF. No, it's not. As I pointed out the (compilation) test coverage is better. > If you want to increase test coverage enable > COMPILE_TEST instead. It is one way to achieve that, yes; depends on OF || COMPILE_TEST depends on MFD_NTXEC
On Tue, 08 Sep 2020, Andy Shevchenko wrote: > On Tue, Sep 8, 2020 at 11:14 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Sat, 05 Sep 2020, Andy Shevchenko wrote: > > > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > > wrote: > > ... > > > > > +config PWM_NTXEC > > > > + tristate "Netronix embedded controller PWM support" > > > > > > > > > > > > > > > > + depends on MFD_NTXEC && OF > > > > > > > > > I don’t see need to reduce test coverage and use of the driver by sticking > > > with OF. Actually it’s not used. > > > > If the device is only known to boot with OF, then it's pointless > > building it when !OF. > > No, it's not. As I pointed out the (compilation) test coverage is better. No, it's a waste of disk space. Why would you knowingly compile something you know you can't use? That's the whole point of COMPILE_TEST. Note that when you want real coverage and you use `allyesconfig` and/or `allmodconfig` then CONFIG_OF is also enabled on platforms which support it. > > If you want to increase test coverage enable > > COMPILE_TEST instead. > > It is one way to achieve that, yes; > > depends on OF || COMPILE_TEST > depends on MFD_NTXEC This is better.
On Sat, Sep 05, 2020 at 09:08:49PM +0300, Andy Shevchenko wrote: > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net> > 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. > > > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > --- [...] > > +#include <linux/of_device.h> > > > mod_devicetable.h Okay > > +/* Convert an 8-bit value into the correct format for writing into a > > register */ > > +#define u8_to_reg(x) (((x) & 0xff) << 8) > > > You spread this macro among the drivers w/o explanation what’s going on. I > think there will be better approach. Okay, I'll move it to ntxec.h and expand on the explanation. I think what's missing is the following part: Some registers, such as the battery status register (0x41), are in big-endian, but others only have eight significant bits, which are in the first byte transmitted over I2C (the MSB of the big-endian value). This convenience macro/function converts an 8-bit value to 16-bit for use in the second kind of register. > > + > > + res |= regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_HIGH, > > u8_to_reg(period >> 8)); > > + res |= regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_LOW, > > u8_to_reg(period)); > > + res |= regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_HIGH, > > u8_to_reg(duty >> 8)); > > + res |= regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_LOW, > > u8_to_reg(duty)); > > > These funny res |= is unusual pattern for returned error codes. Moreover > you are shadowing the real ones. Same go the rest drovers. Please fix by > checking each separately. Okay > > + platform_set_drvdata(pdev, pwm); > > + > > + return (res < 0) ? -EIO : 0; > > > What?! That's an editing error, sorry. > > +static const struct of_device_id ntxec_pwm_of_match[] = { > > + { .compatible = "netronix,ntxec-pwm" }, > > > > > > > + { }, > > > No comma. Okay Thanks for the review, Jonathan Neuschäfer
On Tue, Sep 08, 2020 at 10:35:38AM +0100, Lee Jones wrote: > On Tue, 08 Sep 2020, Andy Shevchenko wrote: > > > On Tue, Sep 8, 2020 at 11:14 AM Lee Jones <lee.jones@linaro.org> wrote: > > > On Sat, 05 Sep 2020, Andy Shevchenko wrote: > > > > On Saturday, September 5, 2020, Jonathan Neuschäfer <j.neuschaefer@gmx.net> [...] > > > > > + depends on MFD_NTXEC && OF > > > > > > > > > > > > I don’t see need to reduce test coverage and use of the driver by sticking > > > > with OF. Actually it’s not used. > > > > > > If the device is only known to boot with OF, then it's pointless > > > building it when !OF. > > > > No, it's not. As I pointed out the (compilation) test coverage is better. > > No, it's a waste of disk space. > > Why would you knowingly compile something you know you can't use? > > That's the whole point of COMPILE_TEST. > > Note that when you want real coverage and you use `allyesconfig` > and/or `allmodconfig` then CONFIG_OF is also enabled on platforms > which support it. > > > > If you want to increase test coverage enable > > > COMPILE_TEST instead. > > > > It is one way to achieve that, yes; > > > > depends on OF || COMPILE_TEST > > depends on MFD_NTXEC > > This is better. Ok, I'll go with this variant. Thanks
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 7dbcf6973d335..7fd17c6cda95e 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 && OF + 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. + 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..325ec0e8f1996 --- /dev/null +++ b/drivers/pwm/pwm-ntxec.c @@ -0,0 +1,160 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * 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 + */ + +#include <linux/mfd/ntxec.h> +#include <linux/module.h> +#include <linux/of_device.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 + +/* Convert an 8-bit value into the correct format for writing into a register */ +#define u8_to_reg(x) (((x) & 0xff) << 8) + +/* + * 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) + +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; + + 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); + return -ERANGE; + } + + period /= TIME_BASE_NS; + duty /= TIME_BASE_NS; + + res |= regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_HIGH, u8_to_reg(period >> 8)); + res |= regmap_write(pwm->ec->regmap, NTXEC_REG_PERIOD_LOW, u8_to_reg(period)); + res |= regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_HIGH, u8_to_reg(duty >> 8)); + res |= regmap_write(pwm->ec->regmap, NTXEC_REG_DUTY_LOW, u8_to_reg(duty)); + + if (res < 0) + return -EIO; + + /* + * Writing a duty cycle of zone 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(pwm->ec->regmap, NTXEC_REG_ENABLE, u8_to_reg(1)); + + /* Disable the auto-off timer */ + res |= regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_HI, u8_to_reg(0xff)); + res |= regmap_write(pwm->ec->regmap, NTXEC_REG_AUTO_OFF_LO, u8_to_reg(0xff)); + return res ? -EIO : 0; + } else { + return regmap_write(pwm->ec->regmap, NTXEC_REG_ENABLE, u8_to_reg(0)); + } +} + +static struct pwm_ops ntxec_pwm_ops = { + .apply = ntxec_pwm_apply, + .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; + 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); + + return (res < 0) ? -EIO : 0; +} + +static int ntxec_pwm_remove(struct platform_device *pdev) +{ + struct ntxec_pwm *pwm = platform_get_drvdata(pdev); + struct pwm_chip *chip = &pwm->chip; + + return pwmchip_remove(chip); +} + +static const struct of_device_id ntxec_pwm_of_match[] = { + { .compatible = "netronix,ntxec-pwm" }, + { }, +}; +MODULE_DEVICE_TABLE(of, ntxec_pwm_of_match); + +static struct platform_driver ntxec_pwm_driver = { + .driver = { + .name = "ntxec-pwm", + .of_match_table = ntxec_pwm_of_match, + }, + .probe = ntxec_pwm_probe, + .remove = ntxec_pwm_remove, +}; +module_platform_driver(ntxec_pwm_driver); + +MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>"); +MODULE_DESCRIPTION("PWM driver for Netronix EC"); +MODULE_LICENSE("GPL");
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. Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> --- v2: - 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 | 160 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+) create mode 100644 drivers/pwm/pwm-ntxec.c -- 2.28.0