| Message ID | 1575851866-18919-5-git-send-email-jeff@labundy.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for Azoteq IQS620A/621/622/624/625 | expand |
On Mon, Dec 09, 2019 at 12:38:36AM +0000, Jeff LaBundy wrote: > This patch adds support for the Azoteq IQS620A, capable of generating > a 1-kHz PWM output with duty cycle between 0.4% and 100% (inclusive). > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > --- > Changes in v2: > - Merged 'Copyright' and 'Author' lines into one in introductory comments > - Added 'Limitations' section to introductory comments > - Replaced 'error' with 'ret' throughout > - Added const qualifier to state argument of iqs620_pwm_apply and removed all > modifications to the variable's contents > - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested > polarity is inverted or the requested period is below 1 ms, respectively > - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero > - Added iqs620_pwm_get_state > - Eliminated tabbed alignment of pwm_ops and platform_driver struct members > - Moved notifier unregistration to already present iqs620_pwm_remove, which > eliminated the need for a device-managed action and ready flag > - Added a comment in iqs620_pwm_probe to explain the order of operations > - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST > > drivers/pwm/Kconfig | 10 +++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-iqs620a.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 217 insertions(+) > create mode 100644 drivers/pwm/pwm-iqs620a.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index bd21655..60bcf6c 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -222,6 +222,16 @@ config PWM_IMX_TPM > To compile this driver as a module, choose M here: the module > will be called pwm-imx-tpm. > > +config PWM_IQS620A > + tristate "Azoteq IQS620A PWM support" > + depends on MFD_IQS62X || COMPILE_TEST > + help > + Generic PWM framework driver for the Azoteq IQS620A multi-function > + sensor. > + > + To compile this driver as a module, choose M here: the module will > + be called pwm-iqs620a. > + > config PWM_JZ4740 > tristate "Ingenic JZ47xx PWM support" > depends on MACH_INGENIC > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index 9a47507..a59c710 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > +obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c > new file mode 100644 > index 0000000..1ea11b9 > --- /dev/null > +++ b/drivers/pwm/pwm-iqs620a.c > @@ -0,0 +1,206 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Azoteq IQS620A PWM Generator > + * > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> > + * > + * Limitations: > + * - The period is not guaranteed to run to completion when the duty cycle is > + * changed or the output is disabled. Do you know more details here? "not guaranteed" means that the new period starts immediately when duty_cycle or the enabled bit is written? > + * - The period is fixed to 1 ms. > + */ > + > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/mfd/iqs62x.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > + > +#define IQS620_PWR_SETTINGS 0xD2 > +#define IQS620_PWR_SETTINGS_PWM_OUT BIT(7) > + > +#define IQS620_PWM_DUTY_CYCLE 0xD8 > + > +#define IQS620_PWM_PERIOD_NS 1000000 > + > +struct iqs620_pwm_private { > + struct iqs62x_core *iqs62x; > + struct pwm_chip chip; > + struct notifier_block notifier; > +}; > + > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct iqs620_pwm_private *iqs620_pwm; > + struct iqs62x_core *iqs62x; > + unsigned int pwm_out = 0; > + int duty_scale, ret; > + > + if (state->polarity != PWM_POLARITY_NORMAL) > + return -ENOTSUPP; > + > + if (state->period < IQS620_PWM_PERIOD_NS) > + return -EINVAL; > + > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > + iqs62x = iqs620_pwm->iqs62x; > + > + duty_scale = DIV_ROUND_CLOSEST(state->duty_cycle * 256, > + IQS620_PWM_PERIOD_NS); > + > + if (duty_scale) { > + ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > + min(duty_scale - 1, 0xFF)); > + if (ret) > + return ret; > + > + if (state->enabled) > + pwm_out = IQS620_PWR_SETTINGS_PWM_OUT; > + } > + > + return regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > + IQS620_PWR_SETTINGS_PWM_OUT, pwm_out); A comment explaining the semantic here would be good. I assume IQS620_PWM_DUTY_CYCLE takes a value between 0 and 255 and the resulting duty cycle is: (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms . If this is right, please use: duty_scale = (state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS Also, when the hardware is running at .enabled = 1, .duty_cycle = 1/256 ms, .period = 1ms and you reconfigure to .enabled = 0, .duty_cycle = 1ms, .period = 1ms the output might be active for > 1/256 ms if the process is preempted between writing IQS620_PWM_DUTY_CYCLE and IQS620_PWR_SETTINGS_PWM_OUT. > +} > + > +static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct iqs620_pwm_private *iqs620_pwm; > + struct iqs62x_core *iqs62x; > + unsigned int val; > + int ret; > + > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > + iqs62x = iqs620_pwm->iqs62x; > + > + ret = regmap_read(iqs62x->map, IQS620_PWR_SETTINGS, &val); > + if (ret) > + goto err_out; > + state->enabled = val & IQS620_PWR_SETTINGS_PWM_OUT; > + > + ret = regmap_read(iqs62x->map, IQS620_PWM_DUTY_CYCLE, &val); > + if (ret) > + goto err_out; > + state->duty_cycle = DIV_ROUND_CLOSEST((val + 1) * IQS620_PWM_PERIOD_NS, > + 256); Please round up. > + state->period = IQS620_PWM_PERIOD_NS; > + > +err_out: > + if (ret) > + dev_err(iqs620_pwm->chip.dev, "Failed to get state: %d\n", ret); > +} > + > +static int iqs620_pwm_notifier(struct notifier_block *notifier, > + unsigned long event_flags, void *context) > +{ > + struct iqs620_pwm_private *iqs620_pwm; > + struct pwm_state state; > + int ret; > + > + if (!(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > + return NOTIFY_DONE; > + > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > + notifier); > + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); Please don't call pwm API functions in callbacks. I assume you rely on pwm_get_state returning the previously set state and that iqs620_pwm_get_state isn't called. Please use pwm->state for that. > + ret = iqs620_pwm_apply(&iqs620_pwm->chip, > + &iqs620_pwm->chip.pwms[0], &state); > + if (ret) { > + dev_err(iqs620_pwm->chip.dev, > + "Failed to re-initialize device: %d\n", ret); > + return NOTIFY_BAD; > + } > + > + return NOTIFY_OK; > +} > + > +static const struct pwm_ops iqs620_pwm_ops = { > + .apply = iqs620_pwm_apply, > + .get_state = iqs620_pwm_get_state, > + .owner = THIS_MODULE, > +}; > + > +static int iqs620_pwm_probe(struct platform_device *pdev) > +{ > + struct iqs620_pwm_private *iqs620_pwm; > + int ret1, ret2; > + > + iqs620_pwm = devm_kzalloc(&pdev->dev, sizeof(*iqs620_pwm), GFP_KERNEL); > + if (!iqs620_pwm) > + return -ENOMEM; > + > + platform_set_drvdata(pdev, iqs620_pwm); > + iqs620_pwm->iqs62x = dev_get_drvdata(pdev->dev.parent); > + > + iqs620_pwm->chip.dev = &pdev->dev; > + iqs620_pwm->chip.ops = &iqs620_pwm_ops; > + iqs620_pwm->chip.base = -1; > + iqs620_pwm->chip.npwm = 1; > + > + ret1 = pwmchip_add(&iqs620_pwm->chip); > + if (ret1) { > + dev_err(&pdev->dev, "Failed to add device: %d\n", ret1); > + return ret1; > + } > + > + /* > + * Since iqs620_pwm_notifier uses iqs620_pwm->chip.pwms[], the notifier > + * is not registered until pwmchip_add (which allocates that array) has > + * been called. If registration fails, the newly added device has to be > + * removed because the driver fails to probe and iqs620_pwm_remove will > + * never be called. > + */ > + iqs620_pwm->notifier.notifier_call = iqs620_pwm_notifier; > + ret1 = blocking_notifier_chain_register(&iqs620_pwm->iqs62x->nh, > + &iqs620_pwm->notifier); > + if (ret1) { > + dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret1); > + > + ret2 = pwmchip_remove(&iqs620_pwm->chip); > + if (ret2) { > + dev_err(&pdev->dev, "Failed to remove device: %d\n", > + ret2); > + return ret2; This exitpoint is bad. The PWM driver is active but the module gets unloaded. I liked the approach from v1 better. ret2 could be local to this block. > + } > + } > + > + return ret1; > +} Best regards Uwe
Hi Uwe, Thank you for your continued support on this project. On Mon, Dec 09, 2019 at 08:32:06AM +0100, Uwe Kleine-König wrote: > On Mon, Dec 09, 2019 at 12:38:36AM +0000, Jeff LaBundy wrote: > > This patch adds support for the Azoteq IQS620A, capable of generating > > a 1-kHz PWM output with duty cycle between 0.4% and 100% (inclusive). > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > --- > > Changes in v2: > > - Merged 'Copyright' and 'Author' lines into one in introductory comments > > - Added 'Limitations' section to introductory comments > > - Replaced 'error' with 'ret' throughout > > - Added const qualifier to state argument of iqs620_pwm_apply and removed all > > modifications to the variable's contents > > - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested > > polarity is inverted or the requested period is below 1 ms, respectively > > - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero > > - Added iqs620_pwm_get_state > > - Eliminated tabbed alignment of pwm_ops and platform_driver struct members > > - Moved notifier unregistration to already present iqs620_pwm_remove, which > > eliminated the need for a device-managed action and ready flag > > - Added a comment in iqs620_pwm_probe to explain the order of operations > > - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST > > > > drivers/pwm/Kconfig | 10 +++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-iqs620a.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 217 insertions(+) > > create mode 100644 drivers/pwm/pwm-iqs620a.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index bd21655..60bcf6c 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -222,6 +222,16 @@ config PWM_IMX_TPM > > To compile this driver as a module, choose M here: the module > > will be called pwm-imx-tpm. > > > > +config PWM_IQS620A > > + tristate "Azoteq IQS620A PWM support" > > + depends on MFD_IQS62X || COMPILE_TEST > > + help > > + Generic PWM framework driver for the Azoteq IQS620A multi-function > > + sensor. > > + > > + To compile this driver as a module, choose M here: the module will > > + be called pwm-iqs620a. > > + > > config PWM_JZ4740 > > tristate "Ingenic JZ47xx PWM support" > > depends on MACH_INGENIC > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > index 9a47507..a59c710 100644 > > --- a/drivers/pwm/Makefile > > +++ b/drivers/pwm/Makefile > > @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o > > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > +obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c > > new file mode 100644 > > index 0000000..1ea11b9 > > --- /dev/null > > +++ b/drivers/pwm/pwm-iqs620a.c > > @@ -0,0 +1,206 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Azoteq IQS620A PWM Generator > > + * > > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> > > + * > > + * Limitations: > > + * - The period is not guaranteed to run to completion when the duty cycle is > > + * changed or the output is disabled. > > Do you know more details here? "not guaranteed" means that the new > period starts immediately when duty_cycle or the enabled bit is written? > Increasing the duty cycle on-the-fly (e.g. 25% to 75%) results in the following behavior (depending on where the I2C write falls): I2C write __ __ __ V_ ______ ______ ______ __ __| |______| |______| |_|x|__| |__| |__| |__| ^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^ The PWM continues to tick at 1 ms, but the currently running period suffers an extraneous pulse as the output is abruptly set high to "catch up" to the new duty cycle. A similar behavior can occur if the duty cycle is decreased, meaning the output is abruptly set low if the I2C transaction completes in what has suddenly become the inactive region of the currently running period. The PWM seems to be a simple counter that rolls over at a period of 1 ms. Both the counter and the IQS620_PWM_DUTY_CYCLE register effectively go to a comparator whose output is ANDed with IQS620_PWR_SETTINGS_PWM_OUT which then drives the PWM output. As such, if either IQS620_PWM_DUTY_CYCLE or IQS620_PWR_SETTINGS_PWM_OUT change, so may the PWM output state depending on the counter's value at the time the I2C write is completed within the 1-ms continuous loop. For v3 I will update the note as follows: - Changes in duty cycle or enable/disable state are immediately reflected by the PWM output and are not aligned to the start of any period. > > + * - The period is fixed to 1 ms. > > + */ > > + > > +#include <linux/device.h> > > +#include <linux/kernel.h> > > +#include <linux/mfd/iqs62x.h> > > +#include <linux/module.h> > > +#include <linux/platform_device.h> > > +#include <linux/pwm.h> > > +#include <linux/regmap.h> > > +#include <linux/slab.h> > > + > > +#define IQS620_PWR_SETTINGS 0xD2 > > +#define IQS620_PWR_SETTINGS_PWM_OUT BIT(7) > > + > > +#define IQS620_PWM_DUTY_CYCLE 0xD8 > > + > > +#define IQS620_PWM_PERIOD_NS 1000000 > > + > > +struct iqs620_pwm_private { > > + struct iqs62x_core *iqs62x; > > + struct pwm_chip chip; > > + struct notifier_block notifier; > > +}; > > + > > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + const struct pwm_state *state) > > +{ > > + struct iqs620_pwm_private *iqs620_pwm; > > + struct iqs62x_core *iqs62x; > > + unsigned int pwm_out = 0; > > + int duty_scale, ret; > > + > > + if (state->polarity != PWM_POLARITY_NORMAL) > > + return -ENOTSUPP; > > + > > + if (state->period < IQS620_PWM_PERIOD_NS) > > + return -EINVAL; > > + > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > + iqs62x = iqs620_pwm->iqs62x; > > + > > + duty_scale = DIV_ROUND_CLOSEST(state->duty_cycle * 256, > > + IQS620_PWM_PERIOD_NS); > > + > > + if (duty_scale) { > > + ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > > + min(duty_scale - 1, 0xFF)); > > + if (ret) > > + return ret; > > + > > + if (state->enabled) > > + pwm_out = IQS620_PWR_SETTINGS_PWM_OUT; > > + } > > + > > + return regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > > + IQS620_PWR_SETTINGS_PWM_OUT, pwm_out); > > A comment explaining the semantic here would be good. I assume > IQS620_PWM_DUTY_CYCLE takes a value between 0 and 255 and the resulting > duty cycle is: > > (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > > . > > If this is right, please use: > > duty_scale = (state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS > Sure thing, will do. I'll add a comment and round down. Your assumption is correct as well. > Also, when the hardware is running at > > .enabled = 1, .duty_cycle = 1/256 ms, .period = 1ms > > and you reconfigure to > > .enabled = 0, .duty_cycle = 1ms, .period = 1ms > > the output might be active for > 1/256 ms if the process is preempted > between writing IQS620_PWM_DUTY_CYCLE and IQS620_PWR_SETTINGS_PWM_OUT. > Good catch. I think we can solve this by writing IQS620_PWM_DUTY_CYCLE first followed by IQS620_PWR_SETTINGS_PWM_OUT when the PWM is going to be enabled, and the reverse when the PWM is going to be disabled (i.e. turn OFF to prevent a stale duty cycle from being temporarily driven). > > +} > > + > > +static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > +{ > > + struct iqs620_pwm_private *iqs620_pwm; > > + struct iqs62x_core *iqs62x; > > + unsigned int val; > > + int ret; > > + > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > + iqs62x = iqs620_pwm->iqs62x; > > + > > + ret = regmap_read(iqs62x->map, IQS620_PWR_SETTINGS, &val); > > + if (ret) > > + goto err_out; > > + state->enabled = val & IQS620_PWR_SETTINGS_PWM_OUT; > > + > > + ret = regmap_read(iqs62x->map, IQS620_PWM_DUTY_CYCLE, &val); > > + if (ret) > > + goto err_out; > > + state->duty_cycle = DIV_ROUND_CLOSEST((val + 1) * IQS620_PWM_PERIOD_NS, > > + 256); > > Please round up. > Sure thing, will do. > > + state->period = IQS620_PWM_PERIOD_NS; > > + > > +err_out: > > + if (ret) > > + dev_err(iqs620_pwm->chip.dev, "Failed to get state: %d\n", ret); > > +} > > + > > +static int iqs620_pwm_notifier(struct notifier_block *notifier, > > + unsigned long event_flags, void *context) > > +{ > > + struct iqs620_pwm_private *iqs620_pwm; > > + struct pwm_state state; > > + int ret; > > + > > + if (!(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > + return NOTIFY_DONE; > > + > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > + notifier); > > + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); > > Please don't call pwm API functions in callbacks. I assume you rely on > pwm_get_state returning the previously set state and that > iqs620_pwm_get_state isn't called. Please use pwm->state for that. > Sure thing, will do. Your assumption is correct. If pwm_get_state called chip->ops->get_state instead of return pwm->state as it does today, this function would break because it would restore the hardware using default register values (since this function follows a reset). Just for my own understanding, are you saying the PWM framework reserves the right to update pwm_get_state to call chip->ops->get_state some time in the future? In any event I will refer to pwm->state as that is what I ultimately need here. FWIW, I borrowed the idea from the resume callback of [0] which possibly suffers the same fate if I have understood the concern correctly. This discussion also made me realize that we need a lock around back-to- back access to IQS620_PWR_SETTINGS_PWM_OUT and IQS620_PWM_DUTY_CYCLE in case iqs620_pwm_get_state is called while iqs620_pwm_apply restores them on behalf of iqs620_pwm_notifier. I will add that in v3. > > + ret = iqs620_pwm_apply(&iqs620_pwm->chip, > > + &iqs620_pwm->chip.pwms[0], &state); > > + if (ret) { > > + dev_err(iqs620_pwm->chip.dev, > > + "Failed to re-initialize device: %d\n", ret); > > + return NOTIFY_BAD; > > + } > > + > > + return NOTIFY_OK; > > +} > > + > > +static const struct pwm_ops iqs620_pwm_ops = { > > + .apply = iqs620_pwm_apply, > > + .get_state = iqs620_pwm_get_state, > > + .owner = THIS_MODULE, > > +}; > > + > > +static int iqs620_pwm_probe(struct platform_device *pdev) > > +{ > > + struct iqs620_pwm_private *iqs620_pwm; > > + int ret1, ret2; > > + > > + iqs620_pwm = devm_kzalloc(&pdev->dev, sizeof(*iqs620_pwm), GFP_KERNEL); > > + if (!iqs620_pwm) > > + return -ENOMEM; > > + > > + platform_set_drvdata(pdev, iqs620_pwm); > > + iqs620_pwm->iqs62x = dev_get_drvdata(pdev->dev.parent); > > + > > + iqs620_pwm->chip.dev = &pdev->dev; > > + iqs620_pwm->chip.ops = &iqs620_pwm_ops; > > + iqs620_pwm->chip.base = -1; > > + iqs620_pwm->chip.npwm = 1; > > + > > + ret1 = pwmchip_add(&iqs620_pwm->chip); > > + if (ret1) { > > + dev_err(&pdev->dev, "Failed to add device: %d\n", ret1); > > + return ret1; > > + } > > + > > + /* > > + * Since iqs620_pwm_notifier uses iqs620_pwm->chip.pwms[], the notifier > > + * is not registered until pwmchip_add (which allocates that array) has > > + * been called. If registration fails, the newly added device has to be > > + * removed because the driver fails to probe and iqs620_pwm_remove will > > + * never be called. > > + */ > > + iqs620_pwm->notifier.notifier_call = iqs620_pwm_notifier; > > + ret1 = blocking_notifier_chain_register(&iqs620_pwm->iqs62x->nh, > > + &iqs620_pwm->notifier); > > + if (ret1) { > > + dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret1); > > + > > + ret2 = pwmchip_remove(&iqs620_pwm->chip); > > + if (ret2) { > > + dev_err(&pdev->dev, "Failed to remove device: %d\n", > > + ret2); > > + return ret2; > > This exitpoint is bad. The PWM driver is active but the module gets > unloaded. I liked the approach from v1 better. > Sure thing, I'll revert the behavior to that of v1. > ret2 could be local to this block. > This will go away automatically. > > + } > > + } > > + > > + return ret1; > > +} > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | [0] drivers/pwm/pwm-atmel-hlcdc.c Kind regards, Jeff LaBundy
Hello Jeff, On Tue, Dec 10, 2019 at 12:03:02AM +0000, Jeff LaBundy wrote: > On Mon, Dec 09, 2019 at 08:32:06AM +0100, Uwe Kleine-König wrote: > > On Mon, Dec 09, 2019 at 12:38:36AM +0000, Jeff LaBundy wrote: > > > This patch adds support for the Azoteq IQS620A, capable of generating > > > a 1-kHz PWM output with duty cycle between 0.4% and 100% (inclusive). > > > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > --- > > > Changes in v2: > > > - Merged 'Copyright' and 'Author' lines into one in introductory comments > > > - Added 'Limitations' section to introductory comments > > > - Replaced 'error' with 'ret' throughout > > > - Added const qualifier to state argument of iqs620_pwm_apply and removed all > > > modifications to the variable's contents > > > - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested > > > polarity is inverted or the requested period is below 1 ms, respectively > > > - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero > > > - Added iqs620_pwm_get_state > > > - Eliminated tabbed alignment of pwm_ops and platform_driver struct members > > > - Moved notifier unregistration to already present iqs620_pwm_remove, which > > > eliminated the need for a device-managed action and ready flag > > > - Added a comment in iqs620_pwm_probe to explain the order of operations > > > - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST > > > > > > drivers/pwm/Kconfig | 10 +++ > > > drivers/pwm/Makefile | 1 + > > > drivers/pwm/pwm-iqs620a.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 217 insertions(+) > > > create mode 100644 drivers/pwm/pwm-iqs620a.c > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > index bd21655..60bcf6c 100644 > > > --- a/drivers/pwm/Kconfig > > > +++ b/drivers/pwm/Kconfig > > > @@ -222,6 +222,16 @@ config PWM_IMX_TPM > > > To compile this driver as a module, choose M here: the module > > > will be called pwm-imx-tpm. > > > > > > +config PWM_IQS620A > > > + tristate "Azoteq IQS620A PWM support" > > > + depends on MFD_IQS62X || COMPILE_TEST > > > + help > > > + Generic PWM framework driver for the Azoteq IQS620A multi-function > > > + sensor. > > > + > > > + To compile this driver as a module, choose M here: the module will > > > + be called pwm-iqs620a. > > > + > > > config PWM_JZ4740 > > > tristate "Ingenic JZ47xx PWM support" > > > depends on MACH_INGENIC > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > > index 9a47507..a59c710 100644 > > > --- a/drivers/pwm/Makefile > > > +++ b/drivers/pwm/Makefile > > > @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o > > > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > > > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > > > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > > +obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c > > > new file mode 100644 > > > index 0000000..1ea11b9 > > > --- /dev/null > > > +++ b/drivers/pwm/pwm-iqs620a.c > > > @@ -0,0 +1,206 @@ > > > +// SPDX-License-Identifier: GPL-2.0+ > > > +/* > > > + * Azoteq IQS620A PWM Generator > > > + * > > > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> > > > + * > > > + * Limitations: > > > + * - The period is not guaranteed to run to completion when the duty cycle is > > > + * changed or the output is disabled. > > > > Do you know more details here? "not guaranteed" means that the new > > period starts immediately when duty_cycle or the enabled bit is written? > > > > Increasing the duty cycle on-the-fly (e.g. 25% to 75%) results in the > following behavior (depending on where the I2C write falls): > > I2C write > __ __ __ V_ ______ ______ ______ __ > __| |______| |______| |_|x|__| |__| |__| |__| > ^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^ > > The PWM continues to tick at 1 ms, but the currently running period suffers > an extraneous pulse as the output is abruptly set high to "catch up" to the > new duty cycle. > > A similar behavior can occur if the duty cycle is decreased, meaning the > output is abruptly set low if the I2C transaction completes in what has > suddenly become the inactive region of the currently running period. > > The PWM seems to be a simple counter that rolls over at a period of 1 ms. > Both the counter and the IQS620_PWM_DUTY_CYCLE register effectively go to > a comparator whose output is ANDed with IQS620_PWR_SETTINGS_PWM_OUT which > then drives the PWM output. > > As such, if either IQS620_PWM_DUTY_CYCLE or IQS620_PWR_SETTINGS_PWM_OUT > change, so may the PWM output state depending on the counter's value at > the time the I2C write is completed within the 1-ms continuous loop. > > For v3 I will update the note as follows: > > - Changes in duty cycle or enable/disable state are immediately reflected > by the PWM output and are not aligned to the start of any period. I'd like to see a bit more information in the driver. Something about the 1ms rhythm being unaffected by the duty_cycle and enable setting. Maybe: - The periods run continuously with a fixed length of 1 ms which is unaffected by register updates. Writing duty cycle or enable registers gets active immediately which might result in glitches. ? > > > > + * - The period is fixed to 1 ms. > > > + */ > > > + > > > +#include <linux/device.h> > > > +#include <linux/kernel.h> > > > +#include <linux/mfd/iqs62x.h> > > > +#include <linux/module.h> > > > +#include <linux/platform_device.h> > > > +#include <linux/pwm.h> > > > +#include <linux/regmap.h> > > > +#include <linux/slab.h> > > > + > > > +#define IQS620_PWR_SETTINGS 0xD2 > > > +#define IQS620_PWR_SETTINGS_PWM_OUT BIT(7) > > > + > > > +#define IQS620_PWM_DUTY_CYCLE 0xD8 > > > + > > > +#define IQS620_PWM_PERIOD_NS 1000000 > > > + > > > +struct iqs620_pwm_private { > > > + struct iqs62x_core *iqs62x; > > > + struct pwm_chip chip; > > > + struct notifier_block notifier; > > > +}; > > > + > > > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > + const struct pwm_state *state) > > > +{ > > > + struct iqs620_pwm_private *iqs620_pwm; > > > + struct iqs62x_core *iqs62x; > > > + unsigned int pwm_out = 0; > > > + int duty_scale, ret; > > > + > > > + if (state->polarity != PWM_POLARITY_NORMAL) > > > + return -ENOTSUPP; > > > + > > > + if (state->period < IQS620_PWM_PERIOD_NS) > > > + return -EINVAL; > > > + > > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > > + iqs62x = iqs620_pwm->iqs62x; > > > + > > > + duty_scale = DIV_ROUND_CLOSEST(state->duty_cycle * 256, > > > + IQS620_PWM_PERIOD_NS); > > > + > > > + if (duty_scale) { > > > + ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > > > + min(duty_scale - 1, 0xFF)); > > > + if (ret) > > > + return ret; > > > + > > > + if (state->enabled) > > > + pwm_out = IQS620_PWR_SETTINGS_PWM_OUT; > > > + } > > > + > > > + return regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > > > + IQS620_PWR_SETTINGS_PWM_OUT, pwm_out); > > > > A comment explaining the semantic here would be good. I assume > > IQS620_PWM_DUTY_CYCLE takes a value between 0 and 255 and the resulting > > duty cycle is: > > > > (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > > > > . > > > > If this is right, please use: > > > > duty_scale = (state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS > > > > Sure thing, will do. I'll add a comment and round down. Your assumption is > correct as well. > > > Also, when the hardware is running at > > > > .enabled = 1, .duty_cycle = 1/256 ms, .period = 1ms > > > > and you reconfigure to > > > > .enabled = 0, .duty_cycle = 1ms, .period = 1ms > > > > the output might be active for > 1/256 ms if the process is preempted > > between writing IQS620_PWM_DUTY_CYCLE and IQS620_PWR_SETTINGS_PWM_OUT. > > > > Good catch. I think we can solve this by writing IQS620_PWM_DUTY_CYCLE > first followed by IQS620_PWR_SETTINGS_PWM_OUT when the PWM is going to > be enabled, and the reverse when the PWM is going to be disabled (i.e. > turn OFF to prevent a stale duty cycle from being temporarily driven). Sounds like a plan. After disabling you even don't need to write the duty cycle register. (But there might be a discussion ahead that .get_state should return the duty cycle.) > > > +static int iqs620_pwm_notifier(struct notifier_block *notifier, > > > + unsigned long event_flags, void *context) > > > +{ > > > + struct iqs620_pwm_private *iqs620_pwm; > > > + struct pwm_state state; > > > + int ret; > > > + > > > + if (!(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > > + return NOTIFY_DONE; > > > + > > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > > + notifier); > > > + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); > > > > Please don't call pwm API functions in callbacks. I assume you rely on > > pwm_get_state returning the previously set state and that > > iqs620_pwm_get_state isn't called. Please use pwm->state for that. > > > > Sure thing, will do. Your assumption is correct. If pwm_get_state called > chip->ops->get_state instead of return pwm->state as it does today, this > function would break because it would restore the hardware using default > register values (since this function follows a reset). > > Just for my own understanding, are you saying the PWM framework reserves > the right to update pwm_get_state to call chip->ops->get_state some time > in the future? In any event I will refer to pwm->state as that is what I > ultimately need here. This already was the case for a short time before v5.4. See 01ccf903edd6 and 40a6b9a00930. (And note that the lazyness mentioned above about not needing to write duty_cycle when the PWM is off is what made the approach break however.) I don't know yet how to proceed here. Being able to get the actually implemented setting would be nice, probably it is prudent to do this with another API function. Other than that I consider it a layer violation to call a function that is designed for consumers in a lowlevel driver. I don't know if we need locking at some time, but if the core holded a lock when .apply is called, .apply calls pwm_get_state which wanted to grab the lock again we get a dead-lock. > FWIW, I borrowed the idea from the resume callback of [0] which possibly > suffers the same fate if I have understood the concern correctly. Yeah, there are many drivers that are not up to date with my review requirements. The problem is a) missing time to update them and b) for some drivers it's hard to get test coverage for changes. Best regards Uwe
Hi Uwe, On Tue, Dec 10, 2019 at 08:22:27AM +0100, Uwe Kleine-König wrote: > Hello Jeff, > > On Tue, Dec 10, 2019 at 12:03:02AM +0000, Jeff LaBundy wrote: > > On Mon, Dec 09, 2019 at 08:32:06AM +0100, Uwe Kleine-König wrote: > > > On Mon, Dec 09, 2019 at 12:38:36AM +0000, Jeff LaBundy wrote: > > > > This patch adds support for the Azoteq IQS620A, capable of generating > > > > a 1-kHz PWM output with duty cycle between 0.4% and 100% (inclusive). > > > > > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > > --- > > > > Changes in v2: > > > > - Merged 'Copyright' and 'Author' lines into one in introductory comments > > > > - Added 'Limitations' section to introductory comments > > > > - Replaced 'error' with 'ret' throughout > > > > - Added const qualifier to state argument of iqs620_pwm_apply and removed all > > > > modifications to the variable's contents > > > > - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested > > > > polarity is inverted or the requested period is below 1 ms, respectively > > > > - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero > > > > - Added iqs620_pwm_get_state > > > > - Eliminated tabbed alignment of pwm_ops and platform_driver struct members > > > > - Moved notifier unregistration to already present iqs620_pwm_remove, which > > > > eliminated the need for a device-managed action and ready flag > > > > - Added a comment in iqs620_pwm_probe to explain the order of operations > > > > - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST > > > > > > > > drivers/pwm/Kconfig | 10 +++ > > > > drivers/pwm/Makefile | 1 + > > > > drivers/pwm/pwm-iqs620a.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 217 insertions(+) > > > > create mode 100644 drivers/pwm/pwm-iqs620a.c > > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > index bd21655..60bcf6c 100644 > > > > --- a/drivers/pwm/Kconfig > > > > +++ b/drivers/pwm/Kconfig > > > > @@ -222,6 +222,16 @@ config PWM_IMX_TPM > > > > To compile this driver as a module, choose M here: the module > > > > will be called pwm-imx-tpm. > > > > > > > > +config PWM_IQS620A > > > > + tristate "Azoteq IQS620A PWM support" > > > > + depends on MFD_IQS62X || COMPILE_TEST > > > > + help > > > > + Generic PWM framework driver for the Azoteq IQS620A multi-function > > > > + sensor. > > > > + > > > > + To compile this driver as a module, choose M here: the module will > > > > + be called pwm-iqs620a. > > > > + > > > > config PWM_JZ4740 > > > > tristate "Ingenic JZ47xx PWM support" > > > > depends on MACH_INGENIC > > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > > > index 9a47507..a59c710 100644 > > > > --- a/drivers/pwm/Makefile > > > > +++ b/drivers/pwm/Makefile > > > > @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o > > > > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > > > > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > > > > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > > > +obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > > > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c > > > > new file mode 100644 > > > > index 0000000..1ea11b9 > > > > --- /dev/null > > > > +++ b/drivers/pwm/pwm-iqs620a.c > > > > @@ -0,0 +1,206 @@ > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > +/* > > > > + * Azoteq IQS620A PWM Generator > > > > + * > > > > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> > > > > + * > > > > + * Limitations: > > > > + * - The period is not guaranteed to run to completion when the duty cycle is > > > > + * changed or the output is disabled. > > > > > > Do you know more details here? "not guaranteed" means that the new > > > period starts immediately when duty_cycle or the enabled bit is written? > > > > > > > Increasing the duty cycle on-the-fly (e.g. 25% to 75%) results in the > > following behavior (depending on where the I2C write falls): > > > > I2C write > > __ __ __ V_ ______ ______ ______ __ > > __| |______| |______| |_|x|__| |__| |__| |__| > > ^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^ > > > > The PWM continues to tick at 1 ms, but the currently running period suffers > > an extraneous pulse as the output is abruptly set high to "catch up" to the > > new duty cycle. > > > > A similar behavior can occur if the duty cycle is decreased, meaning the > > output is abruptly set low if the I2C transaction completes in what has > > suddenly become the inactive region of the currently running period. > > > > The PWM seems to be a simple counter that rolls over at a period of 1 ms. > > Both the counter and the IQS620_PWM_DUTY_CYCLE register effectively go to > > a comparator whose output is ANDed with IQS620_PWR_SETTINGS_PWM_OUT which > > then drives the PWM output. > > > > As such, if either IQS620_PWM_DUTY_CYCLE or IQS620_PWR_SETTINGS_PWM_OUT > > change, so may the PWM output state depending on the counter's value at > > the time the I2C write is completed within the 1-ms continuous loop. > > > > For v3 I will update the note as follows: > > > > - Changes in duty cycle or enable/disable state are immediately reflected > > by the PWM output and are not aligned to the start of any period. > > I'd like to see a bit more information in the driver. Something about > the 1ms rhythm being unaffected by the duty_cycle and enable setting. > Maybe: > > - The periods run continuously with a fixed length of 1 ms which is > unaffected by register updates. Writing duty cycle or enable > registers gets active immediately which might result in glitches. > > ? > I adjusted the wording a bit as per my preference and settled on the following: - The period is fixed to 1 ms and is generated continuously despite changes to the duty cycle or enable/disable state. - Changes to the duty cycle or enable/disable state take effect immediately and may result in a glitch during the period in which the change is made. I believe these capture the spirit of your message; please let me know if you have any concerns. Upon further experimentation, I found that disabling the output (which v2 does so as to simulate a 0% duty cycle) does not actively drive zero, but rather places the output in a high-impedance state with only the device's own internal leakage eventually discharging the pin. This is fundamentally different than actively driving the pin low to make a 0% duty cycle, which does not appear to be possible at all. Therefore I have removed the control of IQS620_PWR_SETTINGS_PWM_OUT based on the duty cycle requested by the user and reverted to the behavior of v1, where the duty cycle requested by the user is mapped only to IQS620_PWM_DUTY_CYCLE. As such, I have also added a third bullet point similar to what you first suggested following v1: - The device cannot generate a 0% duty cycle. > > > > > > + * - The period is fixed to 1 ms. > > > > + */ > > > > + > > > > +#include <linux/device.h> > > > > +#include <linux/kernel.h> > > > > +#include <linux/mfd/iqs62x.h> > > > > +#include <linux/module.h> > > > > +#include <linux/platform_device.h> > > > > +#include <linux/pwm.h> > > > > +#include <linux/regmap.h> > > > > +#include <linux/slab.h> > > > > + > > > > +#define IQS620_PWR_SETTINGS 0xD2 > > > > +#define IQS620_PWR_SETTINGS_PWM_OUT BIT(7) > > > > + > > > > +#define IQS620_PWM_DUTY_CYCLE 0xD8 > > > > + > > > > +#define IQS620_PWM_PERIOD_NS 1000000 > > > > + > > > > +struct iqs620_pwm_private { > > > > + struct iqs62x_core *iqs62x; > > > > + struct pwm_chip chip; > > > > + struct notifier_block notifier; > > > > +}; > > > > + > > > > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > + const struct pwm_state *state) > > > > +{ > > > > + struct iqs620_pwm_private *iqs620_pwm; > > > > + struct iqs62x_core *iqs62x; > > > > + unsigned int pwm_out = 0; > > > > + int duty_scale, ret; > > > > + > > > > + if (state->polarity != PWM_POLARITY_NORMAL) > > > > + return -ENOTSUPP; > > > > + > > > > + if (state->period < IQS620_PWM_PERIOD_NS) > > > > + return -EINVAL; > > > > + > > > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > > > + iqs62x = iqs620_pwm->iqs62x; > > > > + > > > > + duty_scale = DIV_ROUND_CLOSEST(state->duty_cycle * 256, > > > > + IQS620_PWM_PERIOD_NS); > > > > + > > > > + if (duty_scale) { > > > > + ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > > > > + min(duty_scale - 1, 0xFF)); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + if (state->enabled) > > > > + pwm_out = IQS620_PWR_SETTINGS_PWM_OUT; > > > > + } > > > > + > > > > + return regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > > > > + IQS620_PWR_SETTINGS_PWM_OUT, pwm_out); > > > > > > A comment explaining the semantic here would be good. I assume > > > IQS620_PWM_DUTY_CYCLE takes a value between 0 and 255 and the resulting > > > duty cycle is: > > > > > > (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > > > > > > . > > > > > > If this is right, please use: > > > > > > duty_scale = (state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS > > > > > > > Sure thing, will do. I'll add a comment and round down. Your assumption is > > correct as well. > > > > > Also, when the hardware is running at > > > > > > .enabled = 1, .duty_cycle = 1/256 ms, .period = 1ms > > > > > > and you reconfigure to > > > > > > .enabled = 0, .duty_cycle = 1ms, .period = 1ms > > > > > > the output might be active for > 1/256 ms if the process is preempted > > > between writing IQS620_PWM_DUTY_CYCLE and IQS620_PWR_SETTINGS_PWM_OUT. > > > > > > > Good catch. I think we can solve this by writing IQS620_PWM_DUTY_CYCLE > > first followed by IQS620_PWR_SETTINGS_PWM_OUT when the PWM is going to > > be enabled, and the reverse when the PWM is going to be disabled (i.e. > > turn OFF to prevent a stale duty cycle from being temporarily driven). > > Sounds like a plan. After disabling you even don't need to write the > duty cycle register. (But there might be a discussion ahead that > .get_state should return the duty cycle.) > For v3, I've opted to write IQS620_PWM_DUTY_CYCLE regardless of whether the PWM is enabled as a matter of principle (i.e. faithfully pass the entire state down to the hardware without making assumptions). And since some consumers (e.g. leds-pwm, the primary use-case for this PWM) may pre-configure the duty cycle while the PWM is disabled, this method ensures the driver is already compliant in case 01ccf903edd6 returns. Also, as mentioned above, I have dropped the automatic disabling of the PWM to simulate a 0% duty cycle if the requested duty cycle is < 1 / 256 ms since the device does not actively drive a zero with IQS620_PWR_SETTINGS_PWM_OUT = 0. In the interest of transparency, here is what I currently have for v3: static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { struct iqs620_pwm_private *iqs620_pwm; struct iqs62x_core *iqs62x; int duty_scale, ret; if (state->polarity != PWM_POLARITY_NORMAL) return -ENOTSUPP; if (state->period < IQS620_PWM_PERIOD_NS) return -EINVAL; iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); iqs62x = iqs620_pwm->iqs62x; if (!state->enabled) { ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, IQS620_PWR_SETTINGS_PWM_OUT, 0); if (ret) return ret; } /* * The duty cycle generated by the device is calculated as follows: * * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms * * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255 * (inclusive). Therefore the lowest duty cycle the device can generate * while the output is enabled is 1 / 256 ms. */ duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, clamp(duty_scale, 0, 0xFF)); if (ret) return ret; if (state->enabled) ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, IQS620_PWR_SETTINGS_PWM_OUT, 0xFF); return ret; } I believe this captures all of the discussion thus far; please let me know if you have any concerns. > > > > +static int iqs620_pwm_notifier(struct notifier_block *notifier, > > > > + unsigned long event_flags, void *context) > > > > +{ > > > > + struct iqs620_pwm_private *iqs620_pwm; > > > > + struct pwm_state state; > > > > + int ret; > > > > + > > > > + if (!(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > > > + return NOTIFY_DONE; > > > > + > > > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > > > + notifier); > > > > + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); > > > > > > Please don't call pwm API functions in callbacks. I assume you rely on > > > pwm_get_state returning the previously set state and that > > > iqs620_pwm_get_state isn't called. Please use pwm->state for that. > > > > > > > Sure thing, will do. Your assumption is correct. If pwm_get_state called > > chip->ops->get_state instead of return pwm->state as it does today, this > > function would break because it would restore the hardware using default > > register values (since this function follows a reset). > > > > Just for my own understanding, are you saying the PWM framework reserves > > the right to update pwm_get_state to call chip->ops->get_state some time > > in the future? In any event I will refer to pwm->state as that is what I > > ultimately need here. > > This already was the case for a short time before v5.4. See 01ccf903edd6 > and 40a6b9a00930. (And note that the lazyness mentioned above about not > needing to write duty_cycle when the PWM is off is what made the > approach break however.) I don't know yet how to proceed here. Being > able to get the actually implemented setting would be nice, probably it > is prudent to do this with another API function. > > Other than that I consider it a layer violation to call a function that > is designed for consumers in a lowlevel driver. I don't know if we need > locking at some time, but if the core holded a lock when .apply is > called, .apply calls pwm_get_state which wanted to grab the lock again > we get a dead-lock. > I think we may be imagining two different hazards (please correct me if I have misunderstood). Originally I thought you were warning that pwm_get_state (which simply returns pwm->state) may in the future call chip->ops->get_state instead, which here would have caused iqs620_pwm_notifier to call iqs620_pwm_apply with a reset-like state rather than the last state requested by the user (since this notifier is called after the device resets itself). The short-lived change during the 5.4-rc phase, however, was to fill pwm->state in pwm_apply_state with the quantized state from the hardware instead of the raw state requested by the user. Whether pwm->state (accessed directly or by calling pwm_get_state) gives the raw state or the quantized output of get_state following the last call to pwm_apply_state does not change the outcome (for this particular driver) because iqs620_pwm_apply ultimately writes the same register values. Just to be safe, I've been testing with and without 01ccf903edd6 applied locally so as to validate the behavior in both scenarios. What I missed originally is that pwm_get_state is intended for consumers only, in which case I agree it is fundamentally wrong to cannibalize it in the driver. For v3 I have changed iqs620_pwm_notifier to reference pwm->state directly since that is what is ultimately needed in the first place. You're correct that a lock within the core would cause a deadlock; in this case I was proposing a lock around the pair of reads/writes to IQS620_PWM_DUTY_CYCLE and IQS620_PWR_SETTINGS_PWM_OUT since v2 introduced an implied relationship between the two. That would be safe since chip->ops->apply returns before pwm_apply_state calls chip->ops->get_state. However, even that is no longer necessary since IQS620_PWR_SETTINGS_PWM_OUT is now written independently of IQS620_PWM_DUTY_CYCLE due to withdrawing support for a 0% duty cycle starting in v3. In short, I didn't end up adding any locks as there was no need. > > FWIW, I borrowed the idea from the resume callback of [0] which possibly > > suffers the same fate if I have understood the concern correctly. > > Yeah, there are many drivers that are not up to date with my review > requirements. The problem is a) missing time to update them and b) for > some drivers it's hard to get test coverage for changes. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | Kind regards, Jeff LaBundy
Hello Jeff, On Sun, Dec 15, 2019 at 08:36:12PM +0000, Jeff LaBundy wrote: > On Tue, Dec 10, 2019 at 08:22:27AM +0100, Uwe Kleine-König wrote: > > On Tue, Dec 10, 2019 at 12:03:02AM +0000, Jeff LaBundy wrote: > > > On Mon, Dec 09, 2019 at 08:32:06AM +0100, Uwe Kleine-König wrote: > > > > On Mon, Dec 09, 2019 at 12:38:36AM +0000, Jeff LaBundy wrote: > > > > > This patch adds support for the Azoteq IQS620A, capable of generating > > > > > a 1-kHz PWM output with duty cycle between 0.4% and 100% (inclusive). > > > > > > > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > > > --- > > > > > Changes in v2: > > > > > - Merged 'Copyright' and 'Author' lines into one in introductory comments > > > > > - Added 'Limitations' section to introductory comments > > > > > - Replaced 'error' with 'ret' throughout > > > > > - Added const qualifier to state argument of iqs620_pwm_apply and removed all > > > > > modifications to the variable's contents > > > > > - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested > > > > > polarity is inverted or the requested period is below 1 ms, respectively > > > > > - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero > > > > > - Added iqs620_pwm_get_state > > > > > - Eliminated tabbed alignment of pwm_ops and platform_driver struct members > > > > > - Moved notifier unregistration to already present iqs620_pwm_remove, which > > > > > eliminated the need for a device-managed action and ready flag > > > > > - Added a comment in iqs620_pwm_probe to explain the order of operations > > > > > - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST > > > > > > > > > > drivers/pwm/Kconfig | 10 +++ > > > > > drivers/pwm/Makefile | 1 + > > > > > drivers/pwm/pwm-iqs620a.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > > 3 files changed, 217 insertions(+) > > > > > create mode 100644 drivers/pwm/pwm-iqs620a.c > > > > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > > index bd21655..60bcf6c 100644 > > > > > --- a/drivers/pwm/Kconfig > > > > > +++ b/drivers/pwm/Kconfig > > > > > @@ -222,6 +222,16 @@ config PWM_IMX_TPM > > > > > To compile this driver as a module, choose M here: the module > > > > > will be called pwm-imx-tpm. > > > > > > > > > > +config PWM_IQS620A > > > > > + tristate "Azoteq IQS620A PWM support" > > > > > + depends on MFD_IQS62X || COMPILE_TEST > > > > > + help > > > > > + Generic PWM framework driver for the Azoteq IQS620A multi-function > > > > > + sensor. > > > > > + > > > > > + To compile this driver as a module, choose M here: the module will > > > > > + be called pwm-iqs620a. > > > > > + > > > > > config PWM_JZ4740 > > > > > tristate "Ingenic JZ47xx PWM support" > > > > > depends on MACH_INGENIC > > > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > > > > index 9a47507..a59c710 100644 > > > > > --- a/drivers/pwm/Makefile > > > > > +++ b/drivers/pwm/Makefile > > > > > @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o > > > > > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > > > > > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > > > > > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > > > > +obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > > > > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > > > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > > > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > > > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c > > > > > new file mode 100644 > > > > > index 0000000..1ea11b9 > > > > > --- /dev/null > > > > > +++ b/drivers/pwm/pwm-iqs620a.c > > > > > @@ -0,0 +1,206 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > +/* > > > > > + * Azoteq IQS620A PWM Generator > > > > > + * > > > > > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> > > > > > + * > > > > > + * Limitations: > > > > > + * - The period is not guaranteed to run to completion when the duty cycle is > > > > > + * changed or the output is disabled. > > > > > > > > Do you know more details here? "not guaranteed" means that the new > > > > period starts immediately when duty_cycle or the enabled bit is written? > > > > > > > > > > Increasing the duty cycle on-the-fly (e.g. 25% to 75%) results in the > > > following behavior (depending on where the I2C write falls): > > > > > > I2C write > > > __ __ __ V_ ______ ______ ______ __ > > > __| |______| |______| |_|x|__| |__| |__| |__| > > > ^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^ > > > > > > The PWM continues to tick at 1 ms, but the currently running period suffers > > > an extraneous pulse as the output is abruptly set high to "catch up" to the > > > new duty cycle. > > > > > > A similar behavior can occur if the duty cycle is decreased, meaning the > > > output is abruptly set low if the I2C transaction completes in what has > > > suddenly become the inactive region of the currently running period. > > > > > > The PWM seems to be a simple counter that rolls over at a period of 1 ms. > > > Both the counter and the IQS620_PWM_DUTY_CYCLE register effectively go to > > > a comparator whose output is ANDed with IQS620_PWR_SETTINGS_PWM_OUT which > > > then drives the PWM output. > > > > > > As such, if either IQS620_PWM_DUTY_CYCLE or IQS620_PWR_SETTINGS_PWM_OUT > > > change, so may the PWM output state depending on the counter's value at > > > the time the I2C write is completed within the 1-ms continuous loop. > > > > > > For v3 I will update the note as follows: > > > > > > - Changes in duty cycle or enable/disable state are immediately reflected > > > by the PWM output and are not aligned to the start of any period. > > > > I'd like to see a bit more information in the driver. Something about > > the 1ms rhythm being unaffected by the duty_cycle and enable setting. > > Maybe: > > > > - The periods run continuously with a fixed length of 1 ms which is > > unaffected by register updates. Writing duty cycle or enable > > registers gets active immediately which might result in glitches. > > > > ? > > > > I adjusted the wording a bit as per my preference and settled on the > following: > > - The period is fixed to 1 ms and is generated continuously despite changes > to the duty cycle or enable/disable state. > - Changes to the duty cycle or enable/disable state take effect immediately > and may result in a glitch during the period in which the change is made. > > I believe these capture the spirit of your message; please let me know if > you have any concerns. That's fine. > Upon further experimentation, I found that disabling the output (which v2 > does so as to simulate a 0% duty cycle) does not actively drive zero, but > rather places the output in a high-impedance state with only the device's > own internal leakage eventually discharging the pin. But maybe this is still the best you can do in this case. @Thierry, what do you think? > This is fundamentally different than actively driving the pin low to make > a 0% duty cycle, which does not appear to be possible at all. Therefore I > have removed the control of IQS620_PWR_SETTINGS_PWM_OUT based on the duty > cycle requested by the user and reverted to the behavior of v1, where the > duty cycle requested by the user is mapped only to IQS620_PWM_DUTY_CYCLE. > > As such, I have also added a third bullet point similar to what you first > suggested following v1: > > - The device cannot generate a 0% duty cycle. Then this would be: - The device cannot actively drive a 0% duty cycle. The driver is disabled for small duty_cycles relying on a pull down on the board. But maybe it would be more prudent to do this only if the board configuration suggests this is save?! > > > > > + * - The period is fixed to 1 ms. > > > > > + */ > > > > > + > > > > > +#include <linux/device.h> > > > > > +#include <linux/kernel.h> > > > > > +#include <linux/mfd/iqs62x.h> > > > > > +#include <linux/module.h> > > > > > +#include <linux/platform_device.h> > > > > > +#include <linux/pwm.h> > > > > > +#include <linux/regmap.h> > > > > > +#include <linux/slab.h> > > > > > + > > > > > +#define IQS620_PWR_SETTINGS 0xD2 > > > > > +#define IQS620_PWR_SETTINGS_PWM_OUT BIT(7) > > > > > + > > > > > +#define IQS620_PWM_DUTY_CYCLE 0xD8 > > > > > + > > > > > +#define IQS620_PWM_PERIOD_NS 1000000 > > > > > + > > > > > +struct iqs620_pwm_private { > > > > > + struct iqs62x_core *iqs62x; > > > > > + struct pwm_chip chip; > > > > > + struct notifier_block notifier; > > > > > +}; > > > > > + > > > > > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > + const struct pwm_state *state) > > > > > +{ > > > > > + struct iqs620_pwm_private *iqs620_pwm; > > > > > + struct iqs62x_core *iqs62x; > > > > > + unsigned int pwm_out = 0; > > > > > + int duty_scale, ret; > > > > > + > > > > > + if (state->polarity != PWM_POLARITY_NORMAL) > > > > > + return -ENOTSUPP; > > > > > + > > > > > + if (state->period < IQS620_PWM_PERIOD_NS) > > > > > + return -EINVAL; > > > > > + > > > > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > > > > + iqs62x = iqs620_pwm->iqs62x; > > > > > + > > > > > + duty_scale = DIV_ROUND_CLOSEST(state->duty_cycle * 256, > > > > > + IQS620_PWM_PERIOD_NS); > > > > > + > > > > > + if (duty_scale) { > > > > > + ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > > > > > + min(duty_scale - 1, 0xFF)); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + if (state->enabled) > > > > > + pwm_out = IQS620_PWR_SETTINGS_PWM_OUT; > > > > > + } > > > > > + > > > > > + return regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > > > > > + IQS620_PWR_SETTINGS_PWM_OUT, pwm_out); > > > > > > > > A comment explaining the semantic here would be good. I assume > > > > IQS620_PWM_DUTY_CYCLE takes a value between 0 and 255 and the resulting > > > > duty cycle is: > > > > > > > > (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > > > > > > > > . > > > > > > > > If this is right, please use: > > > > > > > > duty_scale = (state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS > > > > > > > > > > Sure thing, will do. I'll add a comment and round down. Your assumption is > > > correct as well. > > > > > > > Also, when the hardware is running at > > > > > > > > .enabled = 1, .duty_cycle = 1/256 ms, .period = 1ms > > > > > > > > and you reconfigure to > > > > > > > > .enabled = 0, .duty_cycle = 1ms, .period = 1ms > > > > > > > > the output might be active for > 1/256 ms if the process is preempted > > > > between writing IQS620_PWM_DUTY_CYCLE and IQS620_PWR_SETTINGS_PWM_OUT. > > > > > > > > > > Good catch. I think we can solve this by writing IQS620_PWM_DUTY_CYCLE > > > first followed by IQS620_PWR_SETTINGS_PWM_OUT when the PWM is going to > > > be enabled, and the reverse when the PWM is going to be disabled (i.e. > > > turn OFF to prevent a stale duty cycle from being temporarily driven). > > > > Sounds like a plan. After disabling you even don't need to write the > > duty cycle register. (But there might be a discussion ahead that > > .get_state should return the duty cycle.) > > > > For v3, I've opted to write IQS620_PWM_DUTY_CYCLE regardless of whether the PWM > is enabled as a matter of principle (i.e. faithfully pass the entire state down > to the hardware without making assumptions). > > And since some consumers (e.g. leds-pwm, the primary use-case for this PWM) may > pre-configure the duty cycle while the PWM is disabled, this method ensures the > driver is already compliant in case 01ccf903edd6 returns. > > Also, as mentioned above, I have dropped the automatic disabling of the PWM to > simulate a 0% duty cycle if the requested duty cycle is < 1 / 256 ms since the > device does not actively drive a zero with IQS620_PWR_SETTINGS_PWM_OUT = 0. In > the interest of transparency, here is what I currently have for v3: > > static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { > struct iqs620_pwm_private *iqs620_pwm; > struct iqs62x_core *iqs62x; > int duty_scale, ret; > > if (state->polarity != PWM_POLARITY_NORMAL) > return -ENOTSUPP; > > if (state->period < IQS620_PWM_PERIOD_NS) > return -EINVAL; > > iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > iqs62x = iqs620_pwm->iqs62x; > > if (!state->enabled) { > ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > IQS620_PWR_SETTINGS_PWM_OUT, 0); > if (ret) > return ret; > } > > /* > * The duty cycle generated by the device is calculated as follows: > * > * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > * > * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255 > * (inclusive). Therefore the lowest duty cycle the device can generate > * while the output is enabled is 1 / 256 ms. > */ > duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; Hmm, this is violating the policy to implement a value not bigger than requested with state->duty_cycle == 0. I see this has downsides to not simply cheat here, but only claiming to have implemented 0% can hurt, too. pwm-rcar returns -EINVAL in this case. > ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > clamp(duty_scale, 0, 0xFF)); > if (ret) > return ret; I understand your motivation to configure the duty cycle also when the the request has enabled=false, but a strange side effect is that a failure to configure the dutycycle with .enabled=false isn't really a problem, is it? (This is not a request to change anything, it's only expression of my frustration that we cannot get away without strange effects.) > > if (state->enabled) > ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > IQS620_PWR_SETTINGS_PWM_OUT, 0xFF); > > return ret; > } > > I believe this captures all of the discussion thus far; please let me know if you > have any concerns. > > > > > > +static int iqs620_pwm_notifier(struct notifier_block *notifier, > > > > > + unsigned long event_flags, void *context) > > > > > +{ > > > > > + struct iqs620_pwm_private *iqs620_pwm; > > > > > + struct pwm_state state; > > > > > + int ret; > > > > > + > > > > > + if (!(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > > > > + return NOTIFY_DONE; > > > > > + > > > > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > > > > + notifier); > > > > > + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); > > > > > > > > Please don't call pwm API functions in callbacks. I assume you rely on > > > > pwm_get_state returning the previously set state and that > > > > iqs620_pwm_get_state isn't called. Please use pwm->state for that. > > > > > > > > > > Sure thing, will do. Your assumption is correct. If pwm_get_state called > > > chip->ops->get_state instead of return pwm->state as it does today, this > > > function would break because it would restore the hardware using default > > > register values (since this function follows a reset). > > > > > > Just for my own understanding, are you saying the PWM framework reserves > > > the right to update pwm_get_state to call chip->ops->get_state some time > > > in the future? In any event I will refer to pwm->state as that is what I > > > ultimately need here. > > > > This already was the case for a short time before v5.4. See 01ccf903edd6 > > and 40a6b9a00930. (And note that the lazyness mentioned above about not > > needing to write duty_cycle when the PWM is off is what made the > > approach break however.) I don't know yet how to proceed here. Being > > able to get the actually implemented setting would be nice, probably it > > is prudent to do this with another API function. > > > > Other than that I consider it a layer violation to call a function that > > is designed for consumers in a lowlevel driver. I don't know if we need > > locking at some time, but if the core holded a lock when .apply is > > called, .apply calls pwm_get_state which wanted to grab the lock again > > we get a dead-lock. > > I think we may be imagining two different hazards (please correct me if I have > misunderstood). Originally I thought you were warning that pwm_get_state (which > simply returns pwm->state) may in the future call chip->ops->get_state instead, > which here would have caused iqs620_pwm_notifier to call iqs620_pwm_apply with > a reset-like state rather than the last state requested by the user (since this > notifier is called after the device resets itself). > > The short-lived change during the 5.4-rc phase, however, was to fill pwm->state > in pwm_apply_state with the quantized state from the hardware instead of the raw > state requested by the user. Whether pwm->state (accessed directly or by calling > pwm_get_state) gives the raw state or the quantized output of get_state following > the last call to pwm_apply_state does not change the outcome (for this particular > driver) because iqs620_pwm_apply ultimately writes the same register values. Just > to be safe, I've been testing with and without 01ccf903edd6 applied locally so as > to validate the behavior in both scenarios. > > What I missed originally is that pwm_get_state is intended for consumers only, in > which case I agree it is fundamentally wrong to cannibalize it in the driver. For > v3 I have changed iqs620_pwm_notifier to reference pwm->state directly since that > is what is ultimately needed in the first place. > > You're correct that a lock within the core would cause a deadlock; in this case I > was proposing a lock around the pair of reads/writes to IQS620_PWM_DUTY_CYCLE and > IQS620_PWR_SETTINGS_PWM_OUT since v2 introduced an implied relationship between > the two. That would be safe since chip->ops->apply returns before pwm_apply_state > calls chip->ops->get_state. > > However, even that is no longer necessary since IQS620_PWR_SETTINGS_PWM_OUT is now > written independently of IQS620_PWM_DUTY_CYCLE due to withdrawing support for a 0% > duty cycle starting in v3. In short, I didn't end up adding any locks as there was > no need. That's fine. I'm not sure we actually need locks in the framework in the long run because (other than clocks) a PWM always has at most a single consumer and up to now it seems reasonable that this single consumer doesn't race with itself. Best regards Uwe
Hi Uwe, I apologize for my delayed replies as I have been traveling. On Mon, Dec 16, 2019 at 10:19:12AM +0100, Uwe Kleine-König wrote: > Hello Jeff, > > On Sun, Dec 15, 2019 at 08:36:12PM +0000, Jeff LaBundy wrote: > > On Tue, Dec 10, 2019 at 08:22:27AM +0100, Uwe Kleine-König wrote: > > > On Tue, Dec 10, 2019 at 12:03:02AM +0000, Jeff LaBundy wrote: > > > > On Mon, Dec 09, 2019 at 08:32:06AM +0100, Uwe Kleine-König wrote: > > > > > On Mon, Dec 09, 2019 at 12:38:36AM +0000, Jeff LaBundy wrote: > > > > > > This patch adds support for the Azoteq IQS620A, capable of generating > > > > > > a 1-kHz PWM output with duty cycle between 0.4% and 100% (inclusive). > > > > > > > > > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > > > > --- > > > > > > Changes in v2: > > > > > > - Merged 'Copyright' and 'Author' lines into one in introductory comments > > > > > > - Added 'Limitations' section to introductory comments > > > > > > - Replaced 'error' with 'ret' throughout > > > > > > - Added const qualifier to state argument of iqs620_pwm_apply and removed all > > > > > > modifications to the variable's contents > > > > > > - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested > > > > > > polarity is inverted or the requested period is below 1 ms, respectively > > > > > > - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero > > > > > > - Added iqs620_pwm_get_state > > > > > > - Eliminated tabbed alignment of pwm_ops and platform_driver struct members > > > > > > - Moved notifier unregistration to already present iqs620_pwm_remove, which > > > > > > eliminated the need for a device-managed action and ready flag > > > > > > - Added a comment in iqs620_pwm_probe to explain the order of operations > > > > > > - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST > > > > > > > > > > > > drivers/pwm/Kconfig | 10 +++ > > > > > > drivers/pwm/Makefile | 1 + > > > > > > drivers/pwm/pwm-iqs620a.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > 3 files changed, 217 insertions(+) > > > > > > create mode 100644 drivers/pwm/pwm-iqs620a.c > > > > > > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > > > index bd21655..60bcf6c 100644 > > > > > > --- a/drivers/pwm/Kconfig > > > > > > +++ b/drivers/pwm/Kconfig > > > > > > @@ -222,6 +222,16 @@ config PWM_IMX_TPM > > > > > > To compile this driver as a module, choose M here: the module > > > > > > will be called pwm-imx-tpm. > > > > > > > > > > > > +config PWM_IQS620A > > > > > > + tristate "Azoteq IQS620A PWM support" > > > > > > + depends on MFD_IQS62X || COMPILE_TEST > > > > > > + help > > > > > > + Generic PWM framework driver for the Azoteq IQS620A multi-function > > > > > > + sensor. > > > > > > + > > > > > > + To compile this driver as a module, choose M here: the module will > > > > > > + be called pwm-iqs620a. > > > > > > + > > > > > > config PWM_JZ4740 > > > > > > tristate "Ingenic JZ47xx PWM support" > > > > > > depends on MACH_INGENIC > > > > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > > > > > index 9a47507..a59c710 100644 > > > > > > --- a/drivers/pwm/Makefile > > > > > > +++ b/drivers/pwm/Makefile > > > > > > @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o > > > > > > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > > > > > > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > > > > > > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > > > > > +obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > > > > > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > > > > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > > > > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > > > > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c > > > > > > new file mode 100644 > > > > > > index 0000000..1ea11b9 > > > > > > --- /dev/null > > > > > > +++ b/drivers/pwm/pwm-iqs620a.c > > > > > > @@ -0,0 +1,206 @@ > > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > > +/* > > > > > > + * Azoteq IQS620A PWM Generator > > > > > > + * > > > > > > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> > > > > > > + * > > > > > > + * Limitations: > > > > > > + * - The period is not guaranteed to run to completion when the duty cycle is > > > > > > + * changed or the output is disabled. > > > > > > > > > > Do you know more details here? "not guaranteed" means that the new > > > > > period starts immediately when duty_cycle or the enabled bit is written? > > > > > > > > > > > > > Increasing the duty cycle on-the-fly (e.g. 25% to 75%) results in the > > > > following behavior (depending on where the I2C write falls): > > > > > > > > I2C write > > > > __ __ __ V_ ______ ______ ______ __ > > > > __| |______| |______| |_|x|__| |__| |__| |__| > > > > ^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^ > > > > > > > > The PWM continues to tick at 1 ms, but the currently running period suffers > > > > an extraneous pulse as the output is abruptly set high to "catch up" to the > > > > new duty cycle. > > > > > > > > A similar behavior can occur if the duty cycle is decreased, meaning the > > > > output is abruptly set low if the I2C transaction completes in what has > > > > suddenly become the inactive region of the currently running period. > > > > > > > > The PWM seems to be a simple counter that rolls over at a period of 1 ms. > > > > Both the counter and the IQS620_PWM_DUTY_CYCLE register effectively go to > > > > a comparator whose output is ANDed with IQS620_PWR_SETTINGS_PWM_OUT which > > > > then drives the PWM output. > > > > > > > > As such, if either IQS620_PWM_DUTY_CYCLE or IQS620_PWR_SETTINGS_PWM_OUT > > > > change, so may the PWM output state depending on the counter's value at > > > > the time the I2C write is completed within the 1-ms continuous loop. > > > > > > > > For v3 I will update the note as follows: > > > > > > > > - Changes in duty cycle or enable/disable state are immediately reflected > > > > by the PWM output and are not aligned to the start of any period. > > > > > > I'd like to see a bit more information in the driver. Something about > > > the 1ms rhythm being unaffected by the duty_cycle and enable setting. > > > Maybe: > > > > > > - The periods run continuously with a fixed length of 1 ms which is > > > unaffected by register updates. Writing duty cycle or enable > > > registers gets active immediately which might result in glitches. > > > > > > ? > > > > > > > I adjusted the wording a bit as per my preference and settled on the > > following: > > > > - The period is fixed to 1 ms and is generated continuously despite changes > > to the duty cycle or enable/disable state. > > - Changes to the duty cycle or enable/disable state take effect immediately > > and may result in a glitch during the period in which the change is made. > > > > I believe these capture the spirit of your message; please let me know if > > you have any concerns. > > That's fine. > > > Upon further experimentation, I found that disabling the output (which v2 > > does so as to simulate a 0% duty cycle) does not actively drive zero, but > > rather places the output in a high-impedance state with only the device's > > own internal leakage eventually discharging the pin. > > But maybe this is still the best you can do in this case. @Thierry, what > do you think? > > > This is fundamentally different than actively driving the pin low to make > > a 0% duty cycle, which does not appear to be possible at all. Therefore I > > have removed the control of IQS620_PWR_SETTINGS_PWM_OUT based on the duty > > cycle requested by the user and reverted to the behavior of v1, where the > > duty cycle requested by the user is mapped only to IQS620_PWM_DUTY_CYCLE. > > > > As such, I have also added a third bullet point similar to what you first > > suggested following v1: > > > > - The device cannot generate a 0% duty cycle. > > Then this would be: > > - The device cannot actively drive a 0% duty cycle. The driver is > disabled for small duty_cycles relying on a pull down on the board. > > But maybe it would be more prudent to do this only if the board > configuration suggests this is save?! > Given the policy for the actual duty cycle generated by the hardware not to exceed that which is requested by the user, it seems there are ultimately 3 options for duty_cycle below 1 / 256 ms: 1. Return -EINVAL. 2. Disable the output as in v2. 3. Add an optional boolean in the dts that identifies whether a pull-down is present; default to option (1) but use option (2) if the boolean is there. I don't like option (1) because consumers (including leds-pwm) do in fact ask for a 0% duty cycle which would make iqs620_pwm_apply return an error. Things happen to still work since leds-pwm does not check pwm_config's return value, but I don't want to rely on this coincidence. Option (2) is better, but I know from experience that board designers do not consult driver comments and the requirement to add a pull-down may be easily missed as it is not discussed in the data sheet (which is where that sort of information belongs, in my opinion). Option (3) seems like overkill for such a simple PWM, and ultimately doesn't add any value because I don't want to allow option (1) behavior in any case. Whether the PWM is disabled because it is truly disabled or to simulate a 0% duty cycle as in option (2), the pull-down is ultimately required regardless of whether or not the data sheet happens to go into such detail. Therefore I have opted to carry forward option (2) from v2 to v3. I reworded the third limitation a bit as follows: - The device cannot generate a 0% duty cycle. For duty cycles below 1 / 256 ms, the output is disabled and relies upon an external pull-down resistor to hold the GPIO3/LTX pin low. I did reach out to the vendor and asked them to consider recommending a pull- down resistor in a future revision of the data sheet, although at the time of this writing I have not heard back. > > > > > > + * - The period is fixed to 1 ms. > > > > > > + */ > > > > > > + > > > > > > +#include <linux/device.h> > > > > > > +#include <linux/kernel.h> > > > > > > +#include <linux/mfd/iqs62x.h> > > > > > > +#include <linux/module.h> > > > > > > +#include <linux/platform_device.h> > > > > > > +#include <linux/pwm.h> > > > > > > +#include <linux/regmap.h> > > > > > > +#include <linux/slab.h> > > > > > > + > > > > > > +#define IQS620_PWR_SETTINGS 0xD2 > > > > > > +#define IQS620_PWR_SETTINGS_PWM_OUT BIT(7) > > > > > > + > > > > > > +#define IQS620_PWM_DUTY_CYCLE 0xD8 > > > > > > + > > > > > > +#define IQS620_PWM_PERIOD_NS 1000000 > > > > > > + > > > > > > +struct iqs620_pwm_private { > > > > > > + struct iqs62x_core *iqs62x; > > > > > > + struct pwm_chip chip; > > > > > > + struct notifier_block notifier; > > > > > > +}; > > > > > > + > > > > > > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > > + const struct pwm_state *state) > > > > > > +{ > > > > > > + struct iqs620_pwm_private *iqs620_pwm; > > > > > > + struct iqs62x_core *iqs62x; > > > > > > + unsigned int pwm_out = 0; > > > > > > + int duty_scale, ret; > > > > > > + > > > > > > + if (state->polarity != PWM_POLARITY_NORMAL) > > > > > > + return -ENOTSUPP; > > > > > > + > > > > > > + if (state->period < IQS620_PWM_PERIOD_NS) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > > > > > + iqs62x = iqs620_pwm->iqs62x; > > > > > > + > > > > > > + duty_scale = DIV_ROUND_CLOSEST(state->duty_cycle * 256, > > > > > > + IQS620_PWM_PERIOD_NS); > > > > > > + > > > > > > + if (duty_scale) { > > > > > > + ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > > > > > > + min(duty_scale - 1, 0xFF)); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + if (state->enabled) > > > > > > + pwm_out = IQS620_PWR_SETTINGS_PWM_OUT; > > > > > > + } > > > > > > + > > > > > > + return regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > > > > > > + IQS620_PWR_SETTINGS_PWM_OUT, pwm_out); > > > > > > > > > > A comment explaining the semantic here would be good. I assume > > > > > IQS620_PWM_DUTY_CYCLE takes a value between 0 and 255 and the resulting > > > > > duty cycle is: > > > > > > > > > > (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > > > > > > > > > > . > > > > > > > > > > If this is right, please use: > > > > > > > > > > duty_scale = (state->duty_cycle * 256) / IQS620_PWM_PERIOD_NS > > > > > > > > > > > > > Sure thing, will do. I'll add a comment and round down. Your assumption is > > > > correct as well. > > > > > > > > > Also, when the hardware is running at > > > > > > > > > > .enabled = 1, .duty_cycle = 1/256 ms, .period = 1ms > > > > > > > > > > and you reconfigure to > > > > > > > > > > .enabled = 0, .duty_cycle = 1ms, .period = 1ms > > > > > > > > > > the output might be active for > 1/256 ms if the process is preempted > > > > > between writing IQS620_PWM_DUTY_CYCLE and IQS620_PWR_SETTINGS_PWM_OUT. > > > > > > > > > > > > > Good catch. I think we can solve this by writing IQS620_PWM_DUTY_CYCLE > > > > first followed by IQS620_PWR_SETTINGS_PWM_OUT when the PWM is going to > > > > be enabled, and the reverse when the PWM is going to be disabled (i.e. > > > > turn OFF to prevent a stale duty cycle from being temporarily driven). > > > > > > Sounds like a plan. After disabling you even don't need to write the > > > duty cycle register. (But there might be a discussion ahead that > > > .get_state should return the duty cycle.) > > > > > > > For v3, I've opted to write IQS620_PWM_DUTY_CYCLE regardless of whether the PWM > > is enabled as a matter of principle (i.e. faithfully pass the entire state down > > to the hardware without making assumptions). > > > > And since some consumers (e.g. leds-pwm, the primary use-case for this PWM) may > > pre-configure the duty cycle while the PWM is disabled, this method ensures the > > driver is already compliant in case 01ccf903edd6 returns. > > > > Also, as mentioned above, I have dropped the automatic disabling of the PWM to > > simulate a 0% duty cycle if the requested duty cycle is < 1 / 256 ms since the > > device does not actively drive a zero with IQS620_PWR_SETTINGS_PWM_OUT = 0. In > > the interest of transparency, here is what I currently have for v3: > > > > static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > const struct pwm_state *state) > > { > > struct iqs620_pwm_private *iqs620_pwm; > > struct iqs62x_core *iqs62x; > > int duty_scale, ret; > > > > if (state->polarity != PWM_POLARITY_NORMAL) > > return -ENOTSUPP; > > > > if (state->period < IQS620_PWM_PERIOD_NS) > > return -EINVAL; > > > > iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > iqs62x = iqs620_pwm->iqs62x; > > > > if (!state->enabled) { > > ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > > IQS620_PWR_SETTINGS_PWM_OUT, 0); > > if (ret) > > return ret; > > } > > > > /* > > * The duty cycle generated by the device is calculated as follows: > > * > > * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > > * > > * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255 > > * (inclusive). Therefore the lowest duty cycle the device can generate > > * while the output is enabled is 1 / 256 ms. > > */ > > duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; > > Hmm, this is violating the policy to implement a value not bigger than > requested with state->duty_cycle == 0. I see this has downsides to not > simply cheat here, but only claiming to have implemented 0% can hurt, > too. pwm-rcar returns -EINVAL in this case. > That's a great point and is addressed by sticking with option (2) described above. Here is what I've got for v3: static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { struct iqs620_pwm_private *iqs620_pwm; struct iqs62x_core *iqs62x; int duty_scale, ret; if (state->polarity != PWM_POLARITY_NORMAL) return -ENOTSUPP; if (state->period < IQS620_PWM_PERIOD_NS) return -EINVAL; iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); iqs62x = iqs620_pwm->iqs62x; mutex_lock(&iqs620_pwm->lock); /* * The duty cycle generated by the device is calculated as follows: * * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms * * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255 * (inclusive). Therefore the lowest duty cycle the device can generate * while the output is enabled is 1 / 256 ms. * * For lower duty cycles (e.g. 0), the PWM output is simply disabled to * allow an on-board pull-down resistor to hold the GPIO3/LTX pin low. */ duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS; if (!state->enabled || !duty_scale) { ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, IQS620_PWR_SETTINGS_PWM_OUT, 0); if (ret) goto err_mutex; } if (duty_scale) { ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, min(duty_scale - 1, 0xFF)); if (ret) goto err_mutex; } if (state->enabled && duty_scale) ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, IQS620_PWR_SETTINGS_PWM_OUT, 0xFF); err_mutex: mutex_unlock(&iqs620_pwm->lock); return ret; } And for the get_state callback: static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state) { struct iqs620_pwm_private *iqs620_pwm; struct iqs62x_core *iqs62x; unsigned int val; int ret; iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); iqs62x = iqs620_pwm->iqs62x; mutex_lock(&iqs620_pwm->lock); ret = regmap_read(iqs62x->map, IQS620_PWR_SETTINGS, &val); if (ret) goto err_mutex; state->enabled = val & IQS620_PWR_SETTINGS_PWM_OUT; ret = regmap_read(iqs62x->map, IQS620_PWM_DUTY_CYCLE, &val); if (ret) goto err_mutex; state->duty_cycle = DIV_ROUND_UP((val + 1) * IQS620_PWM_PERIOD_NS, 256); state->period = IQS620_PWM_PERIOD_NS; err_mutex: mutex_unlock(&iqs620_pwm->lock); if (ret) dev_err(iqs620_pwm->chip.dev, "Failed to get state: %d\n", ret); } If you and/or Thierry have any concerns, please let me know. > > ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > > clamp(duty_scale, 0, 0xFF)); > > if (ret) > > return ret; > > I understand your motivation to configure the duty cycle also when the > the request has enabled=false, but a strange side effect is that a > failure to configure the dutycycle with .enabled=false isn't really a > problem, is it? > (This is not a request to change anything, it's only expression of my > frustration that we cannot get away without strange effects.) > True, but it would definitely be a problem in case 01ccf903edd6 returns and we're relying on the device's own registers to hold the PWM state. Thinking about this more, I agree with your earlier comment that a means to get the actual (quantized) state needs to be a new API function (of integer type). Since chip->ops->get_state is void, there is no way for the callback to warn the core that a register read failed and the PWM state may be junk. > > > > if (state->enabled) > > ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > > IQS620_PWR_SETTINGS_PWM_OUT, 0xFF); > > > > return ret; > > } > > > > I believe this captures all of the discussion thus far; please let me know if you > > have any concerns. > > > > > > > > +static int iqs620_pwm_notifier(struct notifier_block *notifier, > > > > > > + unsigned long event_flags, void *context) > > > > > > +{ > > > > > > + struct iqs620_pwm_private *iqs620_pwm; > > > > > > + struct pwm_state state; > > > > > > + int ret; > > > > > > + > > > > > > + if (!(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > > > > > + return NOTIFY_DONE; > > > > > > + > > > > > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > > > > > + notifier); > > > > > > + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); > > > > > > > > > > Please don't call pwm API functions in callbacks. I assume you rely on > > > > > pwm_get_state returning the previously set state and that > > > > > iqs620_pwm_get_state isn't called. Please use pwm->state for that. > > > > > > > > > > > > > Sure thing, will do. Your assumption is correct. If pwm_get_state called > > > > chip->ops->get_state instead of return pwm->state as it does today, this > > > > function would break because it would restore the hardware using default > > > > register values (since this function follows a reset). > > > > > > > > Just for my own understanding, are you saying the PWM framework reserves > > > > the right to update pwm_get_state to call chip->ops->get_state some time > > > > in the future? In any event I will refer to pwm->state as that is what I > > > > ultimately need here. > > > > > > This already was the case for a short time before v5.4. See 01ccf903edd6 > > > and 40a6b9a00930. (And note that the lazyness mentioned above about not > > > needing to write duty_cycle when the PWM is off is what made the > > > approach break however.) I don't know yet how to proceed here. Being > > > able to get the actually implemented setting would be nice, probably it > > > is prudent to do this with another API function. > > > > > > Other than that I consider it a layer violation to call a function that > > > is designed for consumers in a lowlevel driver. I don't know if we need > > > locking at some time, but if the core holded a lock when .apply is > > > called, .apply calls pwm_get_state which wanted to grab the lock again > > > we get a dead-lock. > > > > I think we may be imagining two different hazards (please correct me if I have > > misunderstood). Originally I thought you were warning that pwm_get_state (which > > simply returns pwm->state) may in the future call chip->ops->get_state instead, > > which here would have caused iqs620_pwm_notifier to call iqs620_pwm_apply with > > a reset-like state rather than the last state requested by the user (since this > > notifier is called after the device resets itself). > > > > The short-lived change during the 5.4-rc phase, however, was to fill pwm->state > > in pwm_apply_state with the quantized state from the hardware instead of the raw > > state requested by the user. Whether pwm->state (accessed directly or by calling > > pwm_get_state) gives the raw state or the quantized output of get_state following > > the last call to pwm_apply_state does not change the outcome (for this particular > > driver) because iqs620_pwm_apply ultimately writes the same register values. Just > > to be safe, I've been testing with and without 01ccf903edd6 applied locally so as > > to validate the behavior in both scenarios. > > > > What I missed originally is that pwm_get_state is intended for consumers only, in > > which case I agree it is fundamentally wrong to cannibalize it in the driver. For > > v3 I have changed iqs620_pwm_notifier to reference pwm->state directly since that > > is what is ultimately needed in the first place. > > > > You're correct that a lock within the core would cause a deadlock; in this case I > > was proposing a lock around the pair of reads/writes to IQS620_PWM_DUTY_CYCLE and > > IQS620_PWR_SETTINGS_PWM_OUT since v2 introduced an implied relationship between > > the two. That would be safe since chip->ops->apply returns before pwm_apply_state > > calls chip->ops->get_state. > > > > However, even that is no longer necessary since IQS620_PWR_SETTINGS_PWM_OUT is now > > written independently of IQS620_PWM_DUTY_CYCLE due to withdrawing support for a 0% > > duty cycle starting in v3. In short, I didn't end up adding any locks as there was > > no need. > > That's fine. I'm not sure we actually need locks in the framework in the > long run because (other than clocks) a PWM always has at most a single > consumer and up to now it seems reasonable that this single consumer > doesn't race with itself. > Agreed. You will see that I have added my own lock in the apply/get_state functions now that I've restored v2's behavior, but that is to account for this device's case where its interrupt handler is in the process of re-initializing the couple of PWM- related registers when get_state is called. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | Kind regards, Jeff LaBundy
Hello Jeff, On Fri, Dec 20, 2019 at 03:19:31AM +0000, Jeff LaBundy wrote: > I apologize for my delayed replies as I have been traveling. No problem, I didn't hold my breath :-) > On Mon, Dec 16, 2019 at 10:19:12AM +0100, Uwe Kleine-König wrote: > > On Sun, Dec 15, 2019 at 08:36:12PM +0000, Jeff LaBundy wrote: > > > On Tue, Dec 10, 2019 at 08:22:27AM +0100, Uwe Kleine-König wrote: > > > > On Tue, Dec 10, 2019 at 12:03:02AM +0000, Jeff LaBundy wrote: > > > > > On Mon, Dec 09, 2019 at 08:32:06AM +0100, Uwe Kleine-König wrote: > > > > > > On Mon, Dec 09, 2019 at 12:38:36AM +0000, Jeff LaBundy wrote: > > > > > > > This patch adds support for the Azoteq IQS620A, capable of generating > > > > > > > a 1-kHz PWM output with duty cycle between 0.4% and 100% (inclusive). > > > > > > > > > > > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > > > > > --- > > > > > > > Changes in v2: > > > > > > > - Merged 'Copyright' and 'Author' lines into one in introductory comments > > > > > > > - Added 'Limitations' section to introductory comments > > > > > > > - Replaced 'error' with 'ret' throughout > > > > > > > - Added const qualifier to state argument of iqs620_pwm_apply and removed all > > > > > > > modifications to the variable's contents > > > > > > > - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested > > > > > > > polarity is inverted or the requested period is below 1 ms, respectively > > > > > > > - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero > > > > > > > - Added iqs620_pwm_get_state > > > > > > > - Eliminated tabbed alignment of pwm_ops and platform_driver struct members > > > > > > > - Moved notifier unregistration to already present iqs620_pwm_remove, which > > > > > > > eliminated the need for a device-managed action and ready flag > > > > > > > - Added a comment in iqs620_pwm_probe to explain the order of operations > > > > > > > - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST > > > > > > > > > > > > > > drivers/pwm/Kconfig | 10 +++ > > > > > > > drivers/pwm/Makefile | 1 + > > > > > > > drivers/pwm/pwm-iqs620a.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > 3 files changed, 217 insertions(+) > > > > > > > create mode 100644 drivers/pwm/pwm-iqs620a.c > > > > > > > > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > > > > index bd21655..60bcf6c 100644 > > > > > > > --- a/drivers/pwm/Kconfig > > > > > > > +++ b/drivers/pwm/Kconfig > > > > > > > @@ -222,6 +222,16 @@ config PWM_IMX_TPM > > > > > > > To compile this driver as a module, choose M here: the module > > > > > > > will be called pwm-imx-tpm. > > > > > > > > > > > > > > +config PWM_IQS620A > > > > > > > + tristate "Azoteq IQS620A PWM support" > > > > > > > + depends on MFD_IQS62X || COMPILE_TEST > > > > > > > + help > > > > > > > + Generic PWM framework driver for the Azoteq IQS620A multi-function > > > > > > > + sensor. > > > > > > > + > > > > > > > + To compile this driver as a module, choose M here: the module will > > > > > > > + be called pwm-iqs620a. > > > > > > > + > > > > > > > config PWM_JZ4740 > > > > > > > tristate "Ingenic JZ47xx PWM support" > > > > > > > depends on MACH_INGENIC > > > > > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > > > > > > index 9a47507..a59c710 100644 > > > > > > > --- a/drivers/pwm/Makefile > > > > > > > +++ b/drivers/pwm/Makefile > > > > > > > @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o > > > > > > > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > > > > > > > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > > > > > > > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > > > > > > +obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > > > > > > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > > > > > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > > > > > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > > > > > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c > > > > > > > new file mode 100644 > > > > > > > index 0000000..1ea11b9 > > > > > > > --- /dev/null > > > > > > > +++ b/drivers/pwm/pwm-iqs620a.c > > > > > > > @@ -0,0 +1,206 @@ > > > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > > > +/* > > > > > > > + * Azoteq IQS620A PWM Generator > > > > > > > + * > > > > > > > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> > > > > > > > + * > > > > > > > + * Limitations: > > > > > > > + * - The period is not guaranteed to run to completion when the duty cycle is > > > > > > > + * changed or the output is disabled. > > > > > > > > > > > > Do you know more details here? "not guaranteed" means that the new > > > > > > period starts immediately when duty_cycle or the enabled bit is written? > > > > > > > > > > > > > > > > Increasing the duty cycle on-the-fly (e.g. 25% to 75%) results in the > > > > > following behavior (depending on where the I2C write falls): > > > > > > > > > > I2C write > > > > > __ __ __ V_ ______ ______ ______ __ > > > > > __| |______| |______| |_|x|__| |__| |__| |__| > > > > > ^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^ > > > > > > > > > > The PWM continues to tick at 1 ms, but the currently running period suffers > > > > > an extraneous pulse as the output is abruptly set high to "catch up" to the > > > > > new duty cycle. > > > > > > > > > > A similar behavior can occur if the duty cycle is decreased, meaning the > > > > > output is abruptly set low if the I2C transaction completes in what has > > > > > suddenly become the inactive region of the currently running period. > > > > > > > > > > The PWM seems to be a simple counter that rolls over at a period of 1 ms. > > > > > Both the counter and the IQS620_PWM_DUTY_CYCLE register effectively go to > > > > > a comparator whose output is ANDed with IQS620_PWR_SETTINGS_PWM_OUT which > > > > > then drives the PWM output. > > > > > > > > > > As such, if either IQS620_PWM_DUTY_CYCLE or IQS620_PWR_SETTINGS_PWM_OUT > > > > > change, so may the PWM output state depending on the counter's value at > > > > > the time the I2C write is completed within the 1-ms continuous loop. > > > > > > > > > > For v3 I will update the note as follows: > > > > > > > > > > - Changes in duty cycle or enable/disable state are immediately reflected > > > > > by the PWM output and are not aligned to the start of any period. > > > > > > > > I'd like to see a bit more information in the driver. Something about > > > > the 1ms rhythm being unaffected by the duty_cycle and enable setting. > > > > Maybe: > > > > > > > > - The periods run continuously with a fixed length of 1 ms which is > > > > unaffected by register updates. Writing duty cycle or enable > > > > registers gets active immediately which might result in glitches. > > > > > > > > ? > > > > > > > > > > I adjusted the wording a bit as per my preference and settled on the > > > following: > > > > > > - The period is fixed to 1 ms and is generated continuously despite changes > > > to the duty cycle or enable/disable state. > > > - Changes to the duty cycle or enable/disable state take effect immediately > > > and may result in a glitch during the period in which the change is made. > > > > > > I believe these capture the spirit of your message; please let me know if > > > you have any concerns. > > > > That's fine. > > > > > Upon further experimentation, I found that disabling the output (which v2 > > > does so as to simulate a 0% duty cycle) does not actively drive zero, but > > > rather places the output in a high-impedance state with only the device's > > > own internal leakage eventually discharging the pin. > > > > But maybe this is still the best you can do in this case. @Thierry, what > > do you think? > > > > > This is fundamentally different than actively driving the pin low to make > > > a 0% duty cycle, which does not appear to be possible at all. Therefore I > > > have removed the control of IQS620_PWR_SETTINGS_PWM_OUT based on the duty > > > cycle requested by the user and reverted to the behavior of v1, where the > > > duty cycle requested by the user is mapped only to IQS620_PWM_DUTY_CYCLE. > > > > > > As such, I have also added a third bullet point similar to what you first > > > suggested following v1: > > > > > > - The device cannot generate a 0% duty cycle. > > > > Then this would be: > > > > - The device cannot actively drive a 0% duty cycle. The driver is > > disabled for small duty_cycles relying on a pull down on the board. > > > > But maybe it would be more prudent to do this only if the board > > configuration suggests this is save?! > > > > Given the policy for the actual duty cycle generated by the hardware not to > exceed that which is requested by the user, it seems there are ultimately 3 > options for duty_cycle below 1 / 256 ms: > > 1. Return -EINVAL. > 2. Disable the output as in v2. > 3. Add an optional boolean in the dts that identifies whether a pull-down is > present; default to option (1) but use option (2) if the boolean is there. > > I don't like option (1) because consumers (including leds-pwm) do in fact ask > for a 0% duty cycle which would make iqs620_pwm_apply return an error. Things > happen to still work since leds-pwm does not check pwm_config's return value, > but I don't want to rely on this coincidence. People implementing PWM drivers seems to mostly care about leds-pwm :-) With that only nearly hitting the requested state isn't that bad. But if you control a step motor that positions a laser, you want to be sure that the request to stop moving it actually worked. > Option (2) is better, but I know from experience that board designers do not > consult driver comments and the requirement to add a pull-down may be easily > missed as it is not discussed in the data sheet (which is where that sort of > information belongs, in my opinion). Hmm, well, actually I think the problem happened earlier when the hardware designer considered providing 0% to be not important. > Option (3) seems like overkill for such a simple PWM, and ultimately doesn't > add any value because I don't want to allow option (1) behavior in any case. > Whether the PWM is disabled because it is truly disabled or to simulate a 0% > duty cycle as in option (2), the pull-down is ultimately required regardless > of whether or not the data sheet happens to go into such detail. Actually I like option 3 best. > Therefore I have opted to carry forward option (2) from v2 to v3. I reworded > the third limitation a bit as follows: > > - The device cannot generate a 0% duty cycle. For duty cycles below 1 / 256 > ms, the output is disabled and relies upon an external pull-down resistor > to hold the GPIO3/LTX pin low. > > I did reach out to the vendor and asked them to consider recommending a pull- > down resistor in a future revision of the data sheet, although at the time of > this writing I have not heard back. Good. > > > /* > > > * The duty cycle generated by the device is calculated as follows: > > > * > > > * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > > > * > > > * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255 > > > * (inclusive). Therefore the lowest duty cycle the device can generate > > > * while the output is enabled is 1 / 256 ms. > > > */ > > > duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; > > > > Hmm, this is violating the policy to implement a value not bigger than > > requested with state->duty_cycle == 0. I see this has downsides to not > > simply cheat here, but only claiming to have implemented 0% can hurt, > > too. pwm-rcar returns -EINVAL in this case. > > > > That's a great point and is addressed by sticking with option (2) described > above. Here is what I've got for v3: > > static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { > struct iqs620_pwm_private *iqs620_pwm; > struct iqs62x_core *iqs62x; > int duty_scale, ret; > > if (state->polarity != PWM_POLARITY_NORMAL) > return -ENOTSUPP; > > if (state->period < IQS620_PWM_PERIOD_NS) > return -EINVAL; > > iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > iqs62x = iqs620_pwm->iqs62x; > > mutex_lock(&iqs620_pwm->lock); > > /* > * The duty cycle generated by the device is calculated as follows: > * > * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > * > * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255 > * (inclusive). Therefore the lowest duty cycle the device can generate > * while the output is enabled is 1 / 256 ms. > * > * For lower duty cycles (e.g. 0), the PWM output is simply disabled to > * allow an on-board pull-down resistor to hold the GPIO3/LTX pin low. > */ > duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS; > > if (!state->enabled || !duty_scale) { > ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > IQS620_PWR_SETTINGS_PWM_OUT, 0); > if (ret) > goto err_mutex; > } > > if (duty_scale) { > ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > min(duty_scale - 1, 0xFF)); > if (ret) > goto err_mutex; > } > > if (state->enabled && duty_scale) > ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > IQS620_PWR_SETTINGS_PWM_OUT, 0xFF); > > err_mutex: > mutex_unlock(&iqs620_pwm->lock); > > return ret; > } Looks ok. (Even though it implements (2) which isn't my favorite :-) > And for the get_state callback: > > static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > struct pwm_state *state) > { > struct iqs620_pwm_private *iqs620_pwm; > struct iqs62x_core *iqs62x; > unsigned int val; > int ret; > > iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > iqs62x = iqs620_pwm->iqs62x; > > mutex_lock(&iqs620_pwm->lock); > > ret = regmap_read(iqs62x->map, IQS620_PWR_SETTINGS, &val); > if (ret) > goto err_mutex; > state->enabled = val & IQS620_PWR_SETTINGS_PWM_OUT; > > ret = regmap_read(iqs62x->map, IQS620_PWM_DUTY_CYCLE, &val); > if (ret) > goto err_mutex; > state->duty_cycle = DIV_ROUND_UP((val + 1) * IQS620_PWM_PERIOD_NS, 256); > state->period = IQS620_PWM_PERIOD_NS; > > err_mutex: > mutex_unlock(&iqs620_pwm->lock); > > if (ret) > dev_err(iqs620_pwm->chip.dev, "Failed to get state: %d\n", ret); > } > > If you and/or Thierry have any concerns, please let me know. Looks good, too. Maybe add a comment like: /* * As the hardware cannot implement "enabled with * duty_cycle == 0", we're reporting "disabled with * duty_cycle = 1/256 ms" after 0% was requested. This is ugly * but the best we can achieve. */ > > > ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > > > clamp(duty_scale, 0, 0xFF)); > > > if (ret) > > > return ret; > > > > I understand your motivation to configure the duty cycle also when the > > the request has enabled=false, but a strange side effect is that a > > failure to configure the dutycycle with .enabled=false isn't really a > > problem, is it? > > (This is not a request to change anything, it's only expression of my > > frustration that we cannot get away without strange effects.) > > > > True, but it would definitely be a problem in case 01ccf903edd6 returns and > we're relying on the device's own registers to hold the PWM state. You can assume it won't come back as is. There are too many drivers that suffer the same problem. My goal is to let the core not depend on the lowlevel drivers to memorize a duty-cycle for disabled PWMs. The details are not yet thought out. Obvious options are: - cache the value in the core - make consumers not depend on that > Thinking about this more, I agree with your earlier comment that a means to > get the actual (quantized) state needs to be a new API function (of integer > type). Since chip->ops->get_state is void, there is no way for the callback > to warn the core that a register read failed and the PWM state may be junk. Yeah, this is something I intend to change, too. .get_state should return a status code in the long run. Best regards Uwe
Hi Uwe, On Fri, Dec 20, 2019 at 09:59:48AM +0100, Uwe Kleine-König wrote: > Hello Jeff, > > On Fri, Dec 20, 2019 at 03:19:31AM +0000, Jeff LaBundy wrote: > > I apologize for my delayed replies as I have been traveling. > > No problem, I didn't hold my breath :-) > > > On Mon, Dec 16, 2019 at 10:19:12AM +0100, Uwe Kleine-König wrote: > > > On Sun, Dec 15, 2019 at 08:36:12PM +0000, Jeff LaBundy wrote: > > > > On Tue, Dec 10, 2019 at 08:22:27AM +0100, Uwe Kleine-König wrote: > > > > > On Tue, Dec 10, 2019 at 12:03:02AM +0000, Jeff LaBundy wrote: > > > > > > On Mon, Dec 09, 2019 at 08:32:06AM +0100, Uwe Kleine-König wrote: > > > > > > > On Mon, Dec 09, 2019 at 12:38:36AM +0000, Jeff LaBundy wrote: > > > > > > > > This patch adds support for the Azoteq IQS620A, capable of generating > > > > > > > > a 1-kHz PWM output with duty cycle between 0.4% and 100% (inclusive). > > > > > > > > > > > > > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > > > > > > > --- > > > > > > > > Changes in v2: > > > > > > > > - Merged 'Copyright' and 'Author' lines into one in introductory comments > > > > > > > > - Added 'Limitations' section to introductory comments > > > > > > > > - Replaced 'error' with 'ret' throughout > > > > > > > > - Added const qualifier to state argument of iqs620_pwm_apply and removed all > > > > > > > > modifications to the variable's contents > > > > > > > > - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested > > > > > > > > polarity is inverted or the requested period is below 1 ms, respectively > > > > > > > > - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero > > > > > > > > - Added iqs620_pwm_get_state > > > > > > > > - Eliminated tabbed alignment of pwm_ops and platform_driver struct members > > > > > > > > - Moved notifier unregistration to already present iqs620_pwm_remove, which > > > > > > > > eliminated the need for a device-managed action and ready flag > > > > > > > > - Added a comment in iqs620_pwm_probe to explain the order of operations > > > > > > > > - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST > > > > > > > > > > > > > > > > drivers/pwm/Kconfig | 10 +++ > > > > > > > > drivers/pwm/Makefile | 1 + > > > > > > > > drivers/pwm/pwm-iqs620a.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++ > > > > > > > > 3 files changed, 217 insertions(+) > > > > > > > > create mode 100644 drivers/pwm/pwm-iqs620a.c > > > > > > > > > > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > > > > > > > index bd21655..60bcf6c 100644 > > > > > > > > --- a/drivers/pwm/Kconfig > > > > > > > > +++ b/drivers/pwm/Kconfig > > > > > > > > @@ -222,6 +222,16 @@ config PWM_IMX_TPM > > > > > > > > To compile this driver as a module, choose M here: the module > > > > > > > > will be called pwm-imx-tpm. > > > > > > > > > > > > > > > > +config PWM_IQS620A > > > > > > > > + tristate "Azoteq IQS620A PWM support" > > > > > > > > + depends on MFD_IQS62X || COMPILE_TEST > > > > > > > > + help > > > > > > > > + Generic PWM framework driver for the Azoteq IQS620A multi-function > > > > > > > > + sensor. > > > > > > > > + > > > > > > > > + To compile this driver as a module, choose M here: the module will > > > > > > > > + be called pwm-iqs620a. > > > > > > > > + > > > > > > > > config PWM_JZ4740 > > > > > > > > tristate "Ingenic JZ47xx PWM support" > > > > > > > > depends on MACH_INGENIC > > > > > > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > > > > > > > > index 9a47507..a59c710 100644 > > > > > > > > --- a/drivers/pwm/Makefile > > > > > > > > +++ b/drivers/pwm/Makefile > > > > > > > > @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o > > > > > > > > obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o > > > > > > > > obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o > > > > > > > > obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o > > > > > > > > +obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o > > > > > > > > obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o > > > > > > > > obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o > > > > > > > > obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o > > > > > > > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c > > > > > > > > new file mode 100644 > > > > > > > > index 0000000..1ea11b9 > > > > > > > > --- /dev/null > > > > > > > > +++ b/drivers/pwm/pwm-iqs620a.c > > > > > > > > @@ -0,0 +1,206 @@ > > > > > > > > +// SPDX-License-Identifier: GPL-2.0+ > > > > > > > > +/* > > > > > > > > + * Azoteq IQS620A PWM Generator > > > > > > > > + * > > > > > > > > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> > > > > > > > > + * > > > > > > > > + * Limitations: > > > > > > > > + * - The period is not guaranteed to run to completion when the duty cycle is > > > > > > > > + * changed or the output is disabled. > > > > > > > > > > > > > > Do you know more details here? "not guaranteed" means that the new > > > > > > > period starts immediately when duty_cycle or the enabled bit is written? > > > > > > > > > > > > > > > > > > > Increasing the duty cycle on-the-fly (e.g. 25% to 75%) results in the > > > > > > following behavior (depending on where the I2C write falls): > > > > > > > > > > > > I2C write > > > > > > __ __ __ V_ ______ ______ ______ __ > > > > > > __| |______| |______| |_|x|__| |__| |__| |__| > > > > > > ^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^ > > > > > > > > > > > > The PWM continues to tick at 1 ms, but the currently running period suffers > > > > > > an extraneous pulse as the output is abruptly set high to "catch up" to the > > > > > > new duty cycle. > > > > > > > > > > > > A similar behavior can occur if the duty cycle is decreased, meaning the > > > > > > output is abruptly set low if the I2C transaction completes in what has > > > > > > suddenly become the inactive region of the currently running period. > > > > > > > > > > > > The PWM seems to be a simple counter that rolls over at a period of 1 ms. > > > > > > Both the counter and the IQS620_PWM_DUTY_CYCLE register effectively go to > > > > > > a comparator whose output is ANDed with IQS620_PWR_SETTINGS_PWM_OUT which > > > > > > then drives the PWM output. > > > > > > > > > > > > As such, if either IQS620_PWM_DUTY_CYCLE or IQS620_PWR_SETTINGS_PWM_OUT > > > > > > change, so may the PWM output state depending on the counter's value at > > > > > > the time the I2C write is completed within the 1-ms continuous loop. > > > > > > > > > > > > For v3 I will update the note as follows: > > > > > > > > > > > > - Changes in duty cycle or enable/disable state are immediately reflected > > > > > > by the PWM output and are not aligned to the start of any period. > > > > > > > > > > I'd like to see a bit more information in the driver. Something about > > > > > the 1ms rhythm being unaffected by the duty_cycle and enable setting. > > > > > Maybe: > > > > > > > > > > - The periods run continuously with a fixed length of 1 ms which is > > > > > unaffected by register updates. Writing duty cycle or enable > > > > > registers gets active immediately which might result in glitches. > > > > > > > > > > ? > > > > > > > > > > > > > I adjusted the wording a bit as per my preference and settled on the > > > > following: > > > > > > > > - The period is fixed to 1 ms and is generated continuously despite changes > > > > to the duty cycle or enable/disable state. > > > > - Changes to the duty cycle or enable/disable state take effect immediately > > > > and may result in a glitch during the period in which the change is made. > > > > > > > > I believe these capture the spirit of your message; please let me know if > > > > you have any concerns. > > > > > > That's fine. > > > > > > > Upon further experimentation, I found that disabling the output (which v2 > > > > does so as to simulate a 0% duty cycle) does not actively drive zero, but > > > > rather places the output in a high-impedance state with only the device's > > > > own internal leakage eventually discharging the pin. > > > > > > But maybe this is still the best you can do in this case. @Thierry, what > > > do you think? > > > > > > > This is fundamentally different than actively driving the pin low to make > > > > a 0% duty cycle, which does not appear to be possible at all. Therefore I > > > > have removed the control of IQS620_PWR_SETTINGS_PWM_OUT based on the duty > > > > cycle requested by the user and reverted to the behavior of v1, where the > > > > duty cycle requested by the user is mapped only to IQS620_PWM_DUTY_CYCLE. > > > > > > > > As such, I have also added a third bullet point similar to what you first > > > > suggested following v1: > > > > > > > > - The device cannot generate a 0% duty cycle. > > > > > > Then this would be: > > > > > > - The device cannot actively drive a 0% duty cycle. The driver is > > > disabled for small duty_cycles relying on a pull down on the board. > > > > > > But maybe it would be more prudent to do this only if the board > > > configuration suggests this is save?! > > > > > > > Given the policy for the actual duty cycle generated by the hardware not to > > exceed that which is requested by the user, it seems there are ultimately 3 > > options for duty_cycle below 1 / 256 ms: > > > > 1. Return -EINVAL. > > 2. Disable the output as in v2. > > 3. Add an optional boolean in the dts that identifies whether a pull-down is > > present; default to option (1) but use option (2) if the boolean is there. > > > > I don't like option (1) because consumers (including leds-pwm) do in fact ask > > for a 0% duty cycle which would make iqs620_pwm_apply return an error. Things > > happen to still work since leds-pwm does not check pwm_config's return value, > > but I don't want to rely on this coincidence. > > People implementing PWM drivers seems to mostly care about leds-pwm :-) > With that only nearly hitting the requested state isn't that bad. But if > you control a step motor that positions a laser, you want to be sure > that the request to stop moving it actually worked. > > > Option (2) is better, but I know from experience that board designers do not > > consult driver comments and the requirement to add a pull-down may be easily > > missed as it is not discussed in the data sheet (which is where that sort of > > information belongs, in my opinion). > > Hmm, well, actually I think the problem happened earlier when the > hardware designer considered providing 0% to be not important. > I heard back from the vendor today; they've acknowledged the limitation and are considering adding support for 0% in a future ROM spin. In the meantime, they've agreed to describe the high-impedance behavior in the data sheet as well as include the pull-down resistor in an example schematic. > > Option (3) seems like overkill for such a simple PWM, and ultimately doesn't > > add any value because I don't want to allow option (1) behavior in any case. > > Whether the PWM is disabled because it is truly disabled or to simulate a 0% > > duty cycle as in option (2), the pull-down is ultimately required regardless > > of whether or not the data sheet happens to go into such detail. > > Actually I like option 3 best. > Based on your other feedback, I'm moving forward under the impression that you'll still accept option (2); please let me know if I have misunderstood (thank you for being flexible). My argument is that even if duty cycle is limited to 1 / 256 ms within the "no pull-down present" option, the output will still be disabled anyway if state->enabled = false. In that case, the pull-down is required to prevent noise from coupling onto the high-impedance pin (which will likely be tied to the high-impedance gate of a MOSFET) and flickering an LED. Stated another way, I do not feel option (3) is suitable because the pull- down is in fact not optional, but required in my opinion. > > Therefore I have opted to carry forward option (2) from v2 to v3. I reworded > > the third limitation a bit as follows: > > > > - The device cannot generate a 0% duty cycle. For duty cycles below 1 / 256 > > ms, the output is disabled and relies upon an external pull-down resistor > > to hold the GPIO3/LTX pin low. > > > > I did reach out to the vendor and asked them to consider recommending a pull- > > down resistor in a future revision of the data sheet, although at the time of > > this writing I have not heard back. > > Good. > > > > > /* > > > > * The duty cycle generated by the device is calculated as follows: > > > > * > > > > * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > > > > * > > > > * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255 > > > > * (inclusive). Therefore the lowest duty cycle the device can generate > > > > * while the output is enabled is 1 / 256 ms. > > > > */ > > > > duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; > > > > > > Hmm, this is violating the policy to implement a value not bigger than > > > requested with state->duty_cycle == 0. I see this has downsides to not > > > simply cheat here, but only claiming to have implemented 0% can hurt, > > > too. pwm-rcar returns -EINVAL in this case. > > > > > > > That's a great point and is addressed by sticking with option (2) described > > above. Here is what I've got for v3: > > > > static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > const struct pwm_state *state) > > { > > struct iqs620_pwm_private *iqs620_pwm; > > struct iqs62x_core *iqs62x; > > int duty_scale, ret; > > > > if (state->polarity != PWM_POLARITY_NORMAL) > > return -ENOTSUPP; > > > > if (state->period < IQS620_PWM_PERIOD_NS) > > return -EINVAL; > > > > iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > iqs62x = iqs620_pwm->iqs62x; > > > > mutex_lock(&iqs620_pwm->lock); > > > > /* > > * The duty cycle generated by the device is calculated as follows: > > * > > * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms > > * > > * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255 > > * (inclusive). Therefore the lowest duty cycle the device can generate > > * while the output is enabled is 1 / 256 ms. > > * > > * For lower duty cycles (e.g. 0), the PWM output is simply disabled to > > * allow an on-board pull-down resistor to hold the GPIO3/LTX pin low. > > */ > > duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS; > > > > if (!state->enabled || !duty_scale) { > > ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > > IQS620_PWR_SETTINGS_PWM_OUT, 0); > > if (ret) > > goto err_mutex; > > } > > > > if (duty_scale) { > > ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > > min(duty_scale - 1, 0xFF)); > > if (ret) > > goto err_mutex; > > } > > > > if (state->enabled && duty_scale) > > ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > > IQS620_PWR_SETTINGS_PWM_OUT, 0xFF); > > > > err_mutex: > > mutex_unlock(&iqs620_pwm->lock); > > > > return ret; > > } > > Looks ok. (Even though it implements (2) which isn't my favorite :-) > > > And for the get_state callback: > > > > static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > > struct pwm_state *state) > > { > > struct iqs620_pwm_private *iqs620_pwm; > > struct iqs62x_core *iqs62x; > > unsigned int val; > > int ret; > > > > iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > iqs62x = iqs620_pwm->iqs62x; > > > > mutex_lock(&iqs620_pwm->lock); > > > > ret = regmap_read(iqs62x->map, IQS620_PWR_SETTINGS, &val); > > if (ret) > > goto err_mutex; > > state->enabled = val & IQS620_PWR_SETTINGS_PWM_OUT; > > > > ret = regmap_read(iqs62x->map, IQS620_PWM_DUTY_CYCLE, &val); > > if (ret) > > goto err_mutex; > > state->duty_cycle = DIV_ROUND_UP((val + 1) * IQS620_PWM_PERIOD_NS, 256); > > state->period = IQS620_PWM_PERIOD_NS; > > > > err_mutex: > > mutex_unlock(&iqs620_pwm->lock); > > > > if (ret) > > dev_err(iqs620_pwm->chip.dev, "Failed to get state: %d\n", ret); > > } > > > > If you and/or Thierry have any concerns, please let me know. > > Looks good, too. Maybe add a comment like: > > /* > * As the hardware cannot implement "enabled with > * duty_cycle == 0", we're reporting "disabled with > * duty_cycle = 1/256 ms" after 0% was requested. This is ugly > * but the best we can achieve. > */ > Sure thing, will do. > > > > ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, > > > > clamp(duty_scale, 0, 0xFF)); > > > > if (ret) > > > > return ret; > > > > > > I understand your motivation to configure the duty cycle also when the > > > the request has enabled=false, but a strange side effect is that a > > > failure to configure the dutycycle with .enabled=false isn't really a > > > problem, is it? > > > (This is not a request to change anything, it's only expression of my > > > frustration that we cannot get away without strange effects.) > > > > > > > True, but it would definitely be a problem in case 01ccf903edd6 returns and > > we're relying on the device's own registers to hold the PWM state. > > You can assume it won't come back as is. There are too many drivers that > suffer the same problem. My goal is to let the core not depend on the > lowlevel drivers to memorize a duty-cycle for disabled PWMs. The details > are not yet thought out. Obvious options are: > > - cache the value in the core > - make consumers not depend on that > > > Thinking about this more, I agree with your earlier comment that a means to > > get the actual (quantized) state needs to be a new API function (of integer > > type). Since chip->ops->get_state is void, there is no way for the callback > > to warn the core that a register read failed and the PWM state may be junk. > > Yeah, this is something I intend to change, too. .get_state should > return a status code in the long run. > Makes sense. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | I'll add a comment in get_state and send out a v3 after the holidays. Kind regards, Jeff LaBundy
Hello Jeff, On Sat, Dec 21, 2019 at 03:28:01AM +0000, Jeff LaBundy wrote: > I heard back from the vendor today; they've acknowledged the limitation and > are considering adding support for 0% in a future ROM spin. In the meantime, > they've agreed to describe the high-impedance behavior in the data sheet as > well as include the pull-down resistor in an example schematic. Oh wow, seems like a good vendor then. :-) > > > Option (3) seems like overkill for such a simple PWM, and ultimately doesn't > > > add any value because I don't want to allow option (1) behavior in any case. > > > Whether the PWM is disabled because it is truly disabled or to simulate a 0% > > > duty cycle as in option (2), the pull-down is ultimately required regardless > > > of whether or not the data sheet happens to go into such detail. > > > > Actually I like option 3 best. > > > > Based on your other feedback, I'm moving forward under the impression that > you'll still accept option (2); please let me know if I have misunderstood > (thank you for being flexible). Yeah, that's fine. If in the end it shows that this is a bad idea we can still change to (3). Best regards Uwe
Hi Uwe, On Sun, Dec 22, 2019 at 10:48:51PM +0100, Uwe Kleine-König wrote: > Hello Jeff, > > On Sat, Dec 21, 2019 at 03:28:01AM +0000, Jeff LaBundy wrote: > > I heard back from the vendor today; they've acknowledged the limitation and > > are considering adding support for 0% in a future ROM spin. In the meantime, > > they've agreed to describe the high-impedance behavior in the data sheet as > > well as include the pull-down resistor in an example schematic. > > Oh wow, seems like a good vendor then. :-) > > > > > Option (3) seems like overkill for such a simple PWM, and ultimately doesn't > > > > add any value because I don't want to allow option (1) behavior in any case. > > > > Whether the PWM is disabled because it is truly disabled or to simulate a 0% > > > > duty cycle as in option (2), the pull-down is ultimately required regardless > > > > of whether or not the data sheet happens to go into such detail. > > > > > > Actually I like option 3 best. > > > > > > > Based on your other feedback, I'm moving forward under the impression that > > you'll still accept option (2); please let me know if I have misunderstood > > (thank you for being flexible). > > Yeah, that's fine. If in the end it shows that this is a bad idea we can > still change to (3). > Sounds great. As soon as 5.5-rc5 lands this weekend, I'll rebase v3 and send it out. I failed to catch this in my previous reply, but the comment I've added to iqs620_pwm_get_state actually reads as follows: /* * Since the device cannot generate a 0% duty cycle, requests to do so * force subsequent calls to iqs620_pwm_get_state to report the output * as disabled with duty cycle equal to that which was in use prior to * the request. This is not ideal, but is the best compromise based on * the capabilities of the device. */ This matches the present implementation, not your proposed comment that claims duty cycle is clamped to 1 / 256 ms following a request for a 0% duty cycle. This seems OK since the concept of a duty cycle or period aren't really relevant if the output is disabled in my opinion. However if you prefer I update iqs620_pwm_apply to clamp duty cycle to 1 / 256 ms (instead of leaving it untouched) in this case, please let me know. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | Wishing you a Happy New Year, Jeff LaBundy
Hi Jeff, On Wed, Jan 01, 2020 at 10:39:36PM +0000, Jeff LaBundy wrote: > On Sun, Dec 22, 2019 at 10:48:51PM +0100, Uwe Kleine-König wrote: > > On Sat, Dec 21, 2019 at 03:28:01AM +0000, Jeff LaBundy wrote: > > > Based on your other feedback, I'm moving forward under the impression that > > > you'll still accept option (2); please let me know if I have misunderstood > > > (thank you for being flexible). > > > > Yeah, that's fine. If in the end it shows that this is a bad idea we can > > still change to (3). > > Sounds great. As soon as 5.5-rc5 lands this weekend, I'll rebase v3 and > send it out. > > I failed to catch this in my previous reply, but the comment I've added > to iqs620_pwm_get_state actually reads as follows: > > /* > * Since the device cannot generate a 0% duty cycle, requests to do so > * force subsequent calls to iqs620_pwm_get_state to report the output > * as disabled with duty cycle equal to that which was in use prior to > * the request. This is not ideal, but is the best compromise based on > * the capabilities of the device. > */ > > This matches the present implementation, not your proposed comment that > claims duty cycle is clamped to 1 / 256 ms following a request for a 0% > duty cycle. Yeah, if that's the mechanism that is actually implemented, that's fine of course. > This seems OK since the concept of a duty cycle or period aren't really > relevant if the output is disabled in my opinion. However if you prefer > I update iqs620_pwm_apply to clamp duty cycle to 1 / 256 ms (instead of > leaving it untouched) in this case, please let me know. For a disabled PWM the duty_cycle and period are not relevant, for an enabled PWM running with 0% the period matters (at least in theory) however. Best regards Uwe
Hi Uwe, On Tue, Jan 07, 2020 at 12:19:40PM +0100, Uwe Kleine-König wrote: > Hi Jeff, > > On Wed, Jan 01, 2020 at 10:39:36PM +0000, Jeff LaBundy wrote: > > On Sun, Dec 22, 2019 at 10:48:51PM +0100, Uwe Kleine-König wrote: > > > On Sat, Dec 21, 2019 at 03:28:01AM +0000, Jeff LaBundy wrote: > > > > Based on your other feedback, I'm moving forward under the impression that > > > > you'll still accept option (2); please let me know if I have misunderstood > > > > (thank you for being flexible). > > > > > > Yeah, that's fine. If in the end it shows that this is a bad idea we can > > > still change to (3). > > > > Sounds great. As soon as 5.5-rc5 lands this weekend, I'll rebase v3 and > > send it out. > > > > I failed to catch this in my previous reply, but the comment I've added > > to iqs620_pwm_get_state actually reads as follows: > > > > /* > > * Since the device cannot generate a 0% duty cycle, requests to do so > > * force subsequent calls to iqs620_pwm_get_state to report the output > > * as disabled with duty cycle equal to that which was in use prior to > > * the request. This is not ideal, but is the best compromise based on > > * the capabilities of the device. > > */ > > > > This matches the present implementation, not your proposed comment that > > claims duty cycle is clamped to 1 / 256 ms following a request for a 0% > > duty cycle. > > Yeah, if that's the mechanism that is actually implemented, that's fine > of course. > > > This seems OK since the concept of a duty cycle or period aren't really > > relevant if the output is disabled in my opinion. However if you prefer > > I update iqs620_pwm_apply to clamp duty cycle to 1 / 256 ms (instead of > > leaving it untouched) in this case, please let me know. > > For a disabled PWM the duty_cycle and period are not relevant, for an > enabled PWM running with 0% the period matters (at least in theory) > however. > Agreed in full. We should be covered here since we report the (fixed) period in all cases. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | I managed to send out v3 this past weekend; please let me know if you have any further feedback or you find it to be satisfactory. Kind regards, Jeff LaBundy
Hello Jeff, On Fri, Jan 10, 2020 at 04:29:08AM +0000, Jeff LaBundy wrote: > I managed to send out v3 this past weekend; please let me know if you > have any further feedback or you find it to be satisfactory. Yeah, I'm aware. It's on my todo list to take a deeper look. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index bd21655..60bcf6c 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -222,6 +222,16 @@ config PWM_IMX_TPM To compile this driver as a module, choose M here: the module will be called pwm-imx-tpm. +config PWM_IQS620A + tristate "Azoteq IQS620A PWM support" + depends on MFD_IQS62X || COMPILE_TEST + help + Generic PWM framework driver for the Azoteq IQS620A multi-function + sensor. + + To compile this driver as a module, choose M here: the module will + be called pwm-iqs620a. + config PWM_JZ4740 tristate "Ingenic JZ47xx PWM support" depends on MACH_INGENIC diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index 9a47507..a59c710 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG) += pwm-img.o obj-$(CONFIG_PWM_IMX1) += pwm-imx1.o obj-$(CONFIG_PWM_IMX27) += pwm-imx27.o obj-$(CONFIG_PWM_IMX_TPM) += pwm-imx-tpm.o +obj-$(CONFIG_PWM_IQS620A) += pwm-iqs620a.o obj-$(CONFIG_PWM_JZ4740) += pwm-jz4740.o obj-$(CONFIG_PWM_LP3943) += pwm-lp3943.o obj-$(CONFIG_PWM_LPC18XX_SCT) += pwm-lpc18xx-sct.o diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c new file mode 100644 index 0000000..1ea11b9 --- /dev/null +++ b/drivers/pwm/pwm-iqs620a.c @@ -0,0 +1,206 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Azoteq IQS620A PWM Generator + * + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> + * + * Limitations: + * - The period is not guaranteed to run to completion when the duty cycle is + * changed or the output is disabled. + * - The period is fixed to 1 ms. + */ + +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/mfd/iqs62x.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#define IQS620_PWR_SETTINGS 0xD2 +#define IQS620_PWR_SETTINGS_PWM_OUT BIT(7) + +#define IQS620_PWM_DUTY_CYCLE 0xD8 + +#define IQS620_PWM_PERIOD_NS 1000000 + +struct iqs620_pwm_private { + struct iqs62x_core *iqs62x; + struct pwm_chip chip; + struct notifier_block notifier; +}; + +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct iqs620_pwm_private *iqs620_pwm; + struct iqs62x_core *iqs62x; + unsigned int pwm_out = 0; + int duty_scale, ret; + + if (state->polarity != PWM_POLARITY_NORMAL) + return -ENOTSUPP; + + if (state->period < IQS620_PWM_PERIOD_NS) + return -EINVAL; + + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); + iqs62x = iqs620_pwm->iqs62x; + + duty_scale = DIV_ROUND_CLOSEST(state->duty_cycle * 256, + IQS620_PWM_PERIOD_NS); + + if (duty_scale) { + ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, + min(duty_scale - 1, 0xFF)); + if (ret) + return ret; + + if (state->enabled) + pwm_out = IQS620_PWR_SETTINGS_PWM_OUT; + } + + return regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, + IQS620_PWR_SETTINGS_PWM_OUT, pwm_out); +} + +static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct iqs620_pwm_private *iqs620_pwm; + struct iqs62x_core *iqs62x; + unsigned int val; + int ret; + + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); + iqs62x = iqs620_pwm->iqs62x; + + ret = regmap_read(iqs62x->map, IQS620_PWR_SETTINGS, &val); + if (ret) + goto err_out; + state->enabled = val & IQS620_PWR_SETTINGS_PWM_OUT; + + ret = regmap_read(iqs62x->map, IQS620_PWM_DUTY_CYCLE, &val); + if (ret) + goto err_out; + state->duty_cycle = DIV_ROUND_CLOSEST((val + 1) * IQS620_PWM_PERIOD_NS, + 256); + state->period = IQS620_PWM_PERIOD_NS; + +err_out: + if (ret) + dev_err(iqs620_pwm->chip.dev, "Failed to get state: %d\n", ret); +} + +static int iqs620_pwm_notifier(struct notifier_block *notifier, + unsigned long event_flags, void *context) +{ + struct iqs620_pwm_private *iqs620_pwm; + struct pwm_state state; + int ret; + + if (!(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) + return NOTIFY_DONE; + + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, + notifier); + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); + + ret = iqs620_pwm_apply(&iqs620_pwm->chip, + &iqs620_pwm->chip.pwms[0], &state); + if (ret) { + dev_err(iqs620_pwm->chip.dev, + "Failed to re-initialize device: %d\n", ret); + return NOTIFY_BAD; + } + + return NOTIFY_OK; +} + +static const struct pwm_ops iqs620_pwm_ops = { + .apply = iqs620_pwm_apply, + .get_state = iqs620_pwm_get_state, + .owner = THIS_MODULE, +}; + +static int iqs620_pwm_probe(struct platform_device *pdev) +{ + struct iqs620_pwm_private *iqs620_pwm; + int ret1, ret2; + + iqs620_pwm = devm_kzalloc(&pdev->dev, sizeof(*iqs620_pwm), GFP_KERNEL); + if (!iqs620_pwm) + return -ENOMEM; + + platform_set_drvdata(pdev, iqs620_pwm); + iqs620_pwm->iqs62x = dev_get_drvdata(pdev->dev.parent); + + iqs620_pwm->chip.dev = &pdev->dev; + iqs620_pwm->chip.ops = &iqs620_pwm_ops; + iqs620_pwm->chip.base = -1; + iqs620_pwm->chip.npwm = 1; + + ret1 = pwmchip_add(&iqs620_pwm->chip); + if (ret1) { + dev_err(&pdev->dev, "Failed to add device: %d\n", ret1); + return ret1; + } + + /* + * Since iqs620_pwm_notifier uses iqs620_pwm->chip.pwms[], the notifier + * is not registered until pwmchip_add (which allocates that array) has + * been called. If registration fails, the newly added device has to be + * removed because the driver fails to probe and iqs620_pwm_remove will + * never be called. + */ + iqs620_pwm->notifier.notifier_call = iqs620_pwm_notifier; + ret1 = blocking_notifier_chain_register(&iqs620_pwm->iqs62x->nh, + &iqs620_pwm->notifier); + if (ret1) { + dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret1); + + ret2 = pwmchip_remove(&iqs620_pwm->chip); + if (ret2) { + dev_err(&pdev->dev, "Failed to remove device: %d\n", + ret2); + return ret2; + } + } + + return ret1; +} + +static int iqs620_pwm_remove(struct platform_device *pdev) +{ + struct iqs620_pwm_private *iqs620_pwm = platform_get_drvdata(pdev); + int ret; + + ret = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh, + &iqs620_pwm->notifier); + if (ret) { + dev_err(&pdev->dev, "Failed to unregister notifier: %d\n", ret); + return ret; + } + + ret = pwmchip_remove(&iqs620_pwm->chip); + if (ret) + dev_err(&pdev->dev, "Failed to remove device: %d\n", ret); + + return ret; +} + +static struct platform_driver iqs620_pwm_platform_driver = { + .driver = { + .name = IQS620_DRV_NAME_PWM, + }, + .probe = iqs620_pwm_probe, + .remove = iqs620_pwm_remove, +}; +module_platform_driver(iqs620_pwm_platform_driver); + +MODULE_AUTHOR("Jeff LaBundy <jeff@labundy.com>"); +MODULE_DESCRIPTION("Azoteq IQS620A PWM Generator"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS("platform:" IQS620_DRV_NAME_PWM);
This patch adds support for the Azoteq IQS620A, capable of generating a 1-kHz PWM output with duty cycle between 0.4% and 100% (inclusive). Signed-off-by: Jeff LaBundy <jeff@labundy.com> --- Changes in v2: - Merged 'Copyright' and 'Author' lines into one in introductory comments - Added 'Limitations' section to introductory comments - Replaced 'error' with 'ret' throughout - Added const qualifier to state argument of iqs620_pwm_apply and removed all modifications to the variable's contents - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested polarity is inverted or the requested period is below 1 ms, respectively - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero - Added iqs620_pwm_get_state - Eliminated tabbed alignment of pwm_ops and platform_driver struct members - Moved notifier unregistration to already present iqs620_pwm_remove, which eliminated the need for a device-managed action and ready flag - Added a comment in iqs620_pwm_probe to explain the order of operations - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST drivers/pwm/Kconfig | 10 +++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-iqs620a.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 217 insertions(+) create mode 100644 drivers/pwm/pwm-iqs620a.c -- 2.7.4