| Message ID | 20200924192455.2484005-5-j.neuschaefer@gmx.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Netronix embedded controller driver for Kobo and Tolino ebook readers | expand |
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@gmx.net> > --- > > 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@gmx.net/ > - 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@gmx.net> > + */ > + > +#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
On Fri, Sep 25, 2020 at 08:30:37AM +0200, Uwe Kleine-König wrote: > Hello Jonathan, [...] > > +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? I'm sure other readers will have the same problem. I'll spell it out. > > +/* > > + * 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? Due to the division rounding down, TIME_BASE_NS * 0x10000 - 1 would be the highest input that results in a representable value after the division, but I'm not sure it otherwise makes sense. > > > +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). I haven't seen any indication that it supports inverted polarity. I'll point it out in the header comment, and add a check. > > > + 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? > 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 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. 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. > > +} > > + > > +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? > > + .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". Ok, I'll rename it. > > + 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); Good idea, I'll do that. Thanks, Jonathan Neuschäfer
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
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
On Sat, Oct 3, 2020 at 2:36 AM Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote: > On Mon, Sep 28, 2020 at 10:35:46AM +0200, Uwe Kleine-König wrote: > > On Sun, Sep 27, 2020 at 11:10:44PM +0200, Jonathan Neuschäfer wrote: ... > > 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. Sounds like you have 1/128 divisor and off/on is done on lower frequency. (We saw PWMs with an additional 7-bit counter, reminds me Crystal Cove PMIC PWM).
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. + 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@gmx.net> + */ + +#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) + +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, 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 + * 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)); + } +} + +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 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 struct platform_driver ntxec_pwm_driver = { + .driver = { + .name = "ntxec-pwm", + }, + .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. 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@gmx.net> --- 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@gmx.net/ - 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 -- 2.28.0