| Message ID | 20200620224222.1312520-5-j.neuschaefer@gmx.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
Hello, On Sun, Jun 21, 2020 at 12:42:17AM +0200, Jonathan Neuschäfer wrote: > The Netronix EC provides a PWM output, which is used for the backlight s/,// > on ebook readers. This patches adds a driver for the PWM output. on *some* ebook readers > +#define NTXEC_UNK_A 0xa1 > +#define NTXEC_UNK_B 0xa2 > +#define NTXEC_ENABLE 0xa3 > +#define NTXEC_PERIOD_LOW 0xa4 > +#define NTXEC_PERIOD_HIGH 0xa5 > +#define NTXEC_DUTY_LOW 0xa6 > +#define NTXEC_DUTY_HIGH 0xa7 > + > +/* > + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are > + * measured in this unit. > + */ > +static int ntxec_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm_dev, > + int duty_ns, int period_ns) > +{ > + struct ntxec_pwm *pwm = pwmchip_to_pwm(chip); > + uint64_t duty = duty_ns; > + uint64_t period = period_ns; As you cannot use values bigger than 8191999 anyhow, I wonder why you use a 64 bit type here. > + int res = 0; > + > + do_div(period, 125); Please use a define instead of plain 125. > + if (period > 0xffff) { > + dev_warn(pwm->dev, > + "Period is not representable in 16 bits: %llu\n", period); > + return -ERANGE; > + } > + > + do_div(duty, 125); > + if (duty > 0xffff) { > + dev_warn(pwm->dev, "Duty cycle is not representable in 16 bits: %llu\n", > + duty); > + return -ERANGE; > + } This check isn't necessary as the pwm core ensures that duty <= period. > + res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_HIGH, period >> 8); > + res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_LOW, period); > + res |= ntxec_write8(pwm->ec, NTXEC_DUTY_HIGH, duty >> 8); > + res |= ntxec_write8(pwm->ec, NTXEC_DUTY_LOW, duty); Does this complete the currently running period? Can it happen that a new period starts between the first and the last write and so a mixed period can be seen at the output? > + > + return (res < 0) ? -EIO : 0; > +} > + > +static int ntxec_pwm_enable(struct pwm_chip *chip, > + struct pwm_device *pwm_dev) > +{ > + struct ntxec_pwm *pwm = pwmchip_to_pwm(chip); > + > + return ntxec_write8(pwm->ec, NTXEC_ENABLE, 1); > +} > + > +static void ntxec_pwm_disable(struct pwm_chip *chip, > + struct pwm_device *pwm_dev) > +{ > + struct ntxec_pwm *pwm = pwmchip_to_pwm(chip); > + > + ntxec_write8(pwm->ec, NTXEC_ENABLE, 0); > +} > + > +static struct pwm_ops ntxec_pwm_ops = { > + .config = ntxec_pwm_config, > + .enable = ntxec_pwm_enable, > + .disable = ntxec_pwm_disable, > + .owner = THIS_MODULE, Please don't align the =, just a single space before them is fine. More important: Please implement .apply() (and .get_state()) instead of the old API. Also please enable PWM_DEBUG which might save us a review iteration. > +}; > + > +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); > + > + res |= ntxec_write8(pwm->ec, NTXEC_ENABLE, 0); > + res |= ntxec_write8(pwm->ec, NTXEC_UNK_A, 0xff); > + res |= ntxec_write8(pwm->ec, NTXEC_UNK_B, 0xff); > + > + return (res < 0) ? -EIO : 0; This is broken for several reasons: - You're not supposed to modify the output in .probe - if ntxec_write8 results in an error you keep the pwm registered. - From the moment on pwmchip_add returns the callbacks can be called. The calls to ntxec_write8 probably interfere here. Best regards Uwe
On Mon, Jun 22, 2020 at 10:18:02AM +0200, Uwe Kleine-König wrote: > Hello, > > On Sun, Jun 21, 2020 at 12:42:17AM +0200, Jonathan Neuschäfer wrote: > > The Netronix EC provides a PWM output, which is used for the backlight > > s/,// > > > on ebook readers. This patches adds a driver for the PWM output. > > on *some* ebook readers Ok, I'll fix these. > > > +#define NTXEC_UNK_A 0xa1 > > +#define NTXEC_UNK_B 0xa2 > > +#define NTXEC_ENABLE 0xa3 > > +#define NTXEC_PERIOD_LOW 0xa4 > > +#define NTXEC_PERIOD_HIGH 0xa5 > > +#define NTXEC_DUTY_LOW 0xa6 > > +#define NTXEC_DUTY_HIGH 0xa7 > > + > > +/* > > + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are > > + * measured in this unit. > > + */ > > +static int ntxec_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm_dev, > > + int duty_ns, int period_ns) > > +{ > > + struct ntxec_pwm *pwm = pwmchip_to_pwm(chip); > > + uint64_t duty = duty_ns; > > + uint64_t period = period_ns; > > As you cannot use values bigger than 8191999 anyhow, I wonder why you > use a 64 bit type here. No particular reason; I possibly got confused by the division API. I'll use uint32_t instead. > > + int res = 0; > > + > > + do_div(period, 125); > > Please use a define instead of plain 125. Will do. > > + if (period > 0xffff) { > > + dev_warn(pwm->dev, > > + "Period is not representable in 16 bits: %llu\n", period); > > + return -ERANGE; > > + } > > + > > + do_div(duty, 125); > > + if (duty > 0xffff) { > > + dev_warn(pwm->dev, "Duty cycle is not representable in 16 bits: %llu\n", > > + duty); > > + return -ERANGE; > > + } > > This check isn't necessary as the pwm core ensures that duty <= period. Ok, I'll remove it. > > > + res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_HIGH, period >> 8); > > + res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_LOW, period); > > + res |= ntxec_write8(pwm->ec, NTXEC_DUTY_HIGH, duty >> 8); > > + res |= ntxec_write8(pwm->ec, NTXEC_DUTY_LOW, duty); > > Does this complete the currently running period? Can it happen that a > new period starts between the first and the last write and so a mixed > period can be seen at the output? Good question. I haven't measured it, and also don't have the code running on the EC. > > > + > > + return (res < 0) ? -EIO : 0; > > +} > > + > > +static int ntxec_pwm_enable(struct pwm_chip *chip, > > + struct pwm_device *pwm_dev) > > +{ > > + struct ntxec_pwm *pwm = pwmchip_to_pwm(chip); > > + > > + return ntxec_write8(pwm->ec, NTXEC_ENABLE, 1); > > +} > > + > > +static void ntxec_pwm_disable(struct pwm_chip *chip, > > + struct pwm_device *pwm_dev) > > +{ > > + struct ntxec_pwm *pwm = pwmchip_to_pwm(chip); > > + > > + ntxec_write8(pwm->ec, NTXEC_ENABLE, 0); > > +} > > + > > +static struct pwm_ops ntxec_pwm_ops = { > > + .config = ntxec_pwm_config, > > + .enable = ntxec_pwm_enable, > > + .disable = ntxec_pwm_disable, > > + .owner = THIS_MODULE, > > Please don't align the =, just a single space before them is fine. Ok > More important: Please implement .apply() (and .get_state()) instead of > the old API. Also please enable PWM_DEBUG which might save us a review > iteration. Will do! > > > +}; > > + > > +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); > > + > > + res |= ntxec_write8(pwm->ec, NTXEC_ENABLE, 0); > > + res |= ntxec_write8(pwm->ec, NTXEC_UNK_A, 0xff); > > + res |= ntxec_write8(pwm->ec, NTXEC_UNK_B, 0xff); > > + > > + return (res < 0) ? -EIO : 0; > > This is broken for several reasons: > > - You're not supposed to modify the output in .probe > - if ntxec_write8 results in an error you keep the pwm registered. > - From the moment on pwmchip_add returns the callbacks can be called. > The calls to ntxec_write8 probably interfere here. Ok, I'll rework the probe function to avoid these issues. Thanks for the review, Jonathan Neuschäfer
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index cb8d739067d2f..147d6629e662c 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -350,6 +350,10 @@ 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 + 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 a59c710e98c76..15507a6d9ca12 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_PUV3) += pwm-puv3.o diff --git a/drivers/pwm/pwm-ntxec.c b/drivers/pwm/pwm-ntxec.c new file mode 100644 index 0000000000000..eca305d8e915b --- /dev/null +++ b/drivers/pwm/pwm-ntxec.c @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: GPL-2.0-only +// Copyright 2020 Jonathan Neuschäfer <j.neuschaefer@gmx.net> +// +// PWM driver for Netronix embedded controller. + +#include <linux/mfd/ntxec.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/of_device.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_UNK_A 0xa1 +#define NTXEC_UNK_B 0xa2 +#define NTXEC_ENABLE 0xa3 +#define NTXEC_PERIOD_LOW 0xa4 +#define NTXEC_PERIOD_HIGH 0xa5 +#define NTXEC_DUTY_LOW 0xa6 +#define NTXEC_DUTY_HIGH 0xa7 + +/* + * The time base used in the EC is 8MHz, or 125ns. Period and duty cycle are + * measured in this unit. + */ +static int ntxec_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm_dev, + int duty_ns, int period_ns) +{ + struct ntxec_pwm *pwm = pwmchip_to_pwm(chip); + uint64_t duty = duty_ns; + uint64_t period = period_ns; + int res = 0; + + do_div(period, 125); + if (period > 0xffff) { + dev_warn(pwm->dev, + "Period is not representable in 16 bits: %llu\n", period); + return -ERANGE; + } + + do_div(duty, 125); + if (duty > 0xffff) { + dev_warn(pwm->dev, "Duty cycle is not representable in 16 bits: %llu\n", + duty); + return -ERANGE; + } + + res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_HIGH, period >> 8); + res |= ntxec_write8(pwm->ec, NTXEC_PERIOD_LOW, period); + res |= ntxec_write8(pwm->ec, NTXEC_DUTY_HIGH, duty >> 8); + res |= ntxec_write8(pwm->ec, NTXEC_DUTY_LOW, duty); + + return (res < 0) ? -EIO : 0; +} + +static int ntxec_pwm_enable(struct pwm_chip *chip, + struct pwm_device *pwm_dev) +{ + struct ntxec_pwm *pwm = pwmchip_to_pwm(chip); + + return ntxec_write8(pwm->ec, NTXEC_ENABLE, 1); +} + +static void ntxec_pwm_disable(struct pwm_chip *chip, + struct pwm_device *pwm_dev) +{ + struct ntxec_pwm *pwm = pwmchip_to_pwm(chip); + + ntxec_write8(pwm->ec, NTXEC_ENABLE, 0); +} + +static struct pwm_ops ntxec_pwm_ops = { + .config = ntxec_pwm_config, + .enable = ntxec_pwm_enable, + .disable = ntxec_pwm_disable, + .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); + + res |= ntxec_write8(pwm->ec, NTXEC_ENABLE, 0); + res |= ntxec_write8(pwm->ec, NTXEC_UNK_A, 0xff); + res |= ntxec_write8(pwm->ec, NTXEC_UNK_B, 0xff); + + 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 ebook readers. This patches adds a driver for the PWM output. Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> --- drivers/pwm/Kconfig | 4 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-ntxec.c | 148 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 153 insertions(+) create mode 100644 drivers/pwm/pwm-ntxec.c -- 2.27.0