| Message ID | 1578271620-2159-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 |
Hello Jeff, On Mon, Jan 06, 2020 at 12:48:02AM +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). Overall a very nice driver, some minor issues below. > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > --- > Changes in v3: > - Updated the commit message to say "~0.4%" instead of "0.4%" > - Clarified the effect of duty cycle and state changes in the 'Limitations' > section and added a restriction regarding 0% duty cycle > - Added a comment in iqs620_pwm_apply to explain how duty cycle is derived > - Updated iqs620_pwm_apply to disable the output first and enable it last to > prevent temporarily driving a stale duty cycle > - Rounded the calculation for duty cycle up and down in iqs620_pwm_get_state > and iqs620_pwm_apply, respectively > - Added a comment in iqs620_pwm_get_state to explain what it reports follow- > ing requests to set duty cycle to 0% > - Added a lock to prevent back-to-back access of IQS620_PWR_SETTINGS_PWM_OUT > and IQS620_PWM_DUTY_CYCLE from being interrupted > - Updated iqs620_pwm_notifier to reference pwm->state directly as opposed to > calling pwm_get_state > - Moved notifier unregistration back to a device-managed action > - Added a completion to prevent iqs620_pwm_notifier from referencing the > pwm_chip structure until it has been initialized by pwmchip_add > > 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 | 254 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 265 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..ee5d8b5 > --- /dev/null > +++ b/drivers/pwm/pwm-iqs620a.c > @@ -0,0 +1,254 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Azoteq IQS620A PWM Generator > + * > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> > + * > + * Limitations: > + * - 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. > + * - 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. > + */ > + > +#include <linux/completion.h> > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/mfd/iqs62x.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/notifier.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; > + struct completion chip_ready; > + struct mutex lock; > +}; > + > +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; minor optimisation: You could do the division before grabbing the lock. (But unsure if this actually gives a better result or the compiler is clever enough to do this.) > + > + 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; > +} > + > +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); > + > + /* > + * Since the device cannot generate a 0% duty cycle, requests to do so > + * cause 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. > + */ > + 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); > +} > + I thought we dicussed having a comment here, saying something like: The device might reset when [...] and as a result looses it's configuration. So the registers must be rewritten when this happens to restore the expected operation. Is it worth to issue a warning when this happens? > +static int iqs620_pwm_notifier(struct notifier_block *notifier, > + unsigned long event_flags, void *context) > +{ > + struct iqs620_pwm_private *iqs620_pwm; > + int ret; > + > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > + notifier); > + > + if (!completion_done(&iqs620_pwm->chip_ready) || > + !(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > + return NOTIFY_DONE; Is here a (relevant?) race? Consider the notifier triggers just when pwmchip_add returned, (maybe even a consumer configured the device) and before complete_all() is called. With my limited knowledge about notifiers I'd say waiting for the completion here might be reasonable and safe. > + ret = iqs620_pwm_apply(&iqs620_pwm->chip, &iqs620_pwm->chip.pwms[0], > + &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 void iqs620_pwm_notifier_unregister(void *context) > +{ > + struct iqs620_pwm_private *iqs620_pwm = context; > + int ret; > + > + ret = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh, > + &iqs620_pwm->notifier); > + if (ret) > + dev_err(iqs620_pwm->chip.dev, > + "Failed to unregister notifier: %d\n", ret); dev_err(iqs620_pwm->chip.dev, "Failed to unregister notifier: %pe\n", ERR_PTR(ret)); gives a nicer output. (Also applies to other error messages of course.) Best regards Uwe
Hi Uwe, Thank you for your kind words and thorough review. On Tue, Jan 14, 2020 at 09:08:28AM +0100, Uwe Kleine-König wrote: > Hello Jeff, > > On Mon, Jan 06, 2020 at 12:48:02AM +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). > > Overall a very nice driver, some minor issues below. > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com> > > --- > > Changes in v3: > > - Updated the commit message to say "~0.4%" instead of "0.4%" > > - Clarified the effect of duty cycle and state changes in the 'Limitations' > > section and added a restriction regarding 0% duty cycle > > - Added a comment in iqs620_pwm_apply to explain how duty cycle is derived > > - Updated iqs620_pwm_apply to disable the output first and enable it last to > > prevent temporarily driving a stale duty cycle > > - Rounded the calculation for duty cycle up and down in iqs620_pwm_get_state > > and iqs620_pwm_apply, respectively > > - Added a comment in iqs620_pwm_get_state to explain what it reports follow- > > ing requests to set duty cycle to 0% > > - Added a lock to prevent back-to-back access of IQS620_PWR_SETTINGS_PWM_OUT > > and IQS620_PWM_DUTY_CYCLE from being interrupted > > - Updated iqs620_pwm_notifier to reference pwm->state directly as opposed to > > calling pwm_get_state > > - Moved notifier unregistration back to a device-managed action > > - Added a completion to prevent iqs620_pwm_notifier from referencing the > > pwm_chip structure until it has been initialized by pwmchip_add > > > > 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 | 254 ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 265 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..ee5d8b5 > > --- /dev/null > > +++ b/drivers/pwm/pwm-iqs620a.c > > @@ -0,0 +1,254 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Azoteq IQS620A PWM Generator > > + * > > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> > > + * > > + * Limitations: > > + * - 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. > > + * - 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. > > + */ > > + > > +#include <linux/completion.h> > > +#include <linux/device.h> > > +#include <linux/kernel.h> > > +#include <linux/mfd/iqs62x.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/notifier.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; > > + struct completion chip_ready; > > + struct mutex lock; > > +}; > > + > > +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; > > minor optimisation: You could do the division before grabbing the lock. > (But unsure if this actually gives a better result or the compiler is > clever enough to do this.) > That's a great point. Regardless of the result I think it is better to re-order as you suggest, simply to keep with the idea of locking around only the minimum necessary code. > > + > > + 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; > > +} > > + > > +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); > > + > > + /* > > + * Since the device cannot generate a 0% duty cycle, requests to do so > > + * cause 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. > > + */ > > + 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); > > +} > > + > > I thought we dicussed having a comment here, saying something like: > > The device might reset when [...] and as a result looses it's > configuration. So the registers must be rewritten when this > happens to restore the expected operation. > > Is it worth to issue a warning when this happens? > The detailed comments and an error message have always been in iqs62x_irq of the parent MFD driver. The pwm is only one of up to three sub-devices that subscribe to the chain and must update their locally owned registers after the MFD handles the interrupt and restores the device's firmware. I opted to keep these comments in the common MFD rather than repeating throughout the series. > > +static int iqs620_pwm_notifier(struct notifier_block *notifier, > > + unsigned long event_flags, void *context) > > +{ > > + struct iqs620_pwm_private *iqs620_pwm; > > + int ret; > > + > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > + notifier); > > + > > + if (!completion_done(&iqs620_pwm->chip_ready) || > > + !(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > + return NOTIFY_DONE; > > Is here a (relevant?) race? Consider the notifier triggers just when > pwmchip_add returned, (maybe even a consumer configured the device) and > before complete_all() is called. With my limited knowledge about > notifiers I'd say waiting for the completion here might be reasonable > and safe. > Great catch; this is theoretically possible. The problem with waiting, however, is if the notifier is triggered right away during probe but probe returns early due to an error (and completion never happens). At this point, I think the best option is to simply cache the values written to IQS620_PWR_SETTINGS_PWM_OUT and IQS620_PWM_DUTY_CYCLE and restore them from the notifier, which is essentially what is done for the IIO drivers in this series. This simple solution avoids messy tear-down paths and race conditions resulting from trying to piggy-back chip->pwms[0] before it may be available. As an added bonus it also alleviates any register access from get_state, which could simply use the cached values to return the quantized duty cycle. > > + ret = iqs620_pwm_apply(&iqs620_pwm->chip, &iqs620_pwm->chip.pwms[0], > > + &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 void iqs620_pwm_notifier_unregister(void *context) > > +{ > > + struct iqs620_pwm_private *iqs620_pwm = context; > > + int ret; > > + > > + ret = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh, > > + &iqs620_pwm->notifier); > > + if (ret) > > + dev_err(iqs620_pwm->chip.dev, > > + "Failed to unregister notifier: %d\n", ret); > > dev_err(iqs620_pwm->chip.dev, > "Failed to unregister notifier: %pe\n", ERR_PTR(ret)); > > gives a nicer output. (Also applies to other error messages of course.) > I don't disagree, but this gives me some pause. If I made this change here, I'd prefer to do so across the series for consistency. However, I am hesitant to do so at this stage in the review since several patches are somewhat stable by now (unless there was a compelling reason from a functional perspective). Another reason is that there are many dev_err cases throughout this series, and adopting this very recently introduced functionality would make the series even harder to back port to the present lot of LTS kernels. Unless this is a deal breaker, I'd like to pass on this for v4. However, please let me know if you feel strongly about it. In the meantime, I'll get started on the couple of other changes discussed here. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | Kind regards, Jeff LaBundy
Hello Jeff, On Wed, Jan 15, 2020 at 03:29:52AM +0000, Jeff LaBundy wrote: > Thank you for your kind words and thorough review. Great my feedback is welcome. > On Tue, Jan 14, 2020 at 09:08:28AM +0100, Uwe Kleine-König wrote: > > On Mon, Jan 06, 2020 at 12:48:02AM +0000, Jeff LaBundy wrote: > > I thought we dicussed having a comment here, saying something like: > > > > The device might reset when [...] and as a result looses it's > > configuration. So the registers must be rewritten when this > > happens to restore the expected operation. > > > > Is it worth to issue a warning when this happens? > > The detailed comments and an error message have always been in iqs62x_irq of the > parent MFD driver. The pwm is only one of up to three sub-devices that subscribe > to the chain and must update their locally owned registers after the MFD handles > the interrupt and restores the device's firmware. I opted to keep these comments > in the common MFD rather than repeating throughout the series. That's fine then, a comment that the parent driver issues a message would be great then. > > > +static int iqs620_pwm_notifier(struct notifier_block *notifier, > > > + unsigned long event_flags, void *context) > > > +{ > > > + struct iqs620_pwm_private *iqs620_pwm; > > > + int ret; > > > + > > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > > + notifier); > > > + > > > + if (!completion_done(&iqs620_pwm->chip_ready) || > > > + !(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > > + return NOTIFY_DONE; > > > > Is here a (relevant?) race? Consider the notifier triggers just when > > pwmchip_add returned, (maybe even a consumer configured the device) and > > before complete_all() is called. With my limited knowledge about > > notifiers I'd say waiting for the completion here might be reasonable > > and safe. > > Great catch; this is theoretically possible. The problem with waiting, however, > is if the notifier is triggered right away during probe but probe returns early > due to an error (and completion never happens). OK, the error path would need to complete .chip_ready then and the notifier then check for this error. Indeed messy. > At this point, I think the best option is to simply cache the values written to > IQS620_PWR_SETTINGS_PWM_OUT and IQS620_PWM_DUTY_CYCLE and restore them from the > notifier, which is essentially what is done for the IIO drivers in this series. Sounds good. > > > + ret = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh, > > > + &iqs620_pwm->notifier); > > > + if (ret) > > > + dev_err(iqs620_pwm->chip.dev, > > > + "Failed to unregister notifier: %d\n", ret); > > > > dev_err(iqs620_pwm->chip.dev, > > "Failed to unregister notifier: %pe\n", ERR_PTR(ret)); > > > > gives a nicer output. (Also applies to other error messages of course.) > > > > I don't disagree, but this gives me some pause. If I made this change here, I'd > prefer to do so across the series for consistency. However, I am hesitant to do > so at this stage in the review since several patches are somewhat stable by now > (unless there was a compelling reason from a functional perspective). > > Another reason is that there are many dev_err cases throughout this series, and > adopting this very recently introduced functionality would make the series even > harder to back port to the present lot of LTS kernels. > > Unless this is a deal breaker, I'd like to pass on this for v4. However, please > let me know if you feel strongly about it. In the meantime, I'll get started on > the couple of other changes discussed here. OK, being able to backport is a valid excuse. Consistency over the whole series wouldn't be one of my reasons, your mileage obviously varies (which is OK). This can also be done later. Conversion to this is on my todo-list (not at the top though), but if you beat me to it, I won't be angry :-) Best regards Uwe
Hi Uwe, On Wed, Jan 15, 2020 at 08:33:36AM +0100, Uwe Kleine-König wrote: > Hello Jeff, > > On Wed, Jan 15, 2020 at 03:29:52AM +0000, Jeff LaBundy wrote: > > Thank you for your kind words and thorough review. > > Great my feedback is welcome. > > > On Tue, Jan 14, 2020 at 09:08:28AM +0100, Uwe Kleine-König wrote: > > > On Mon, Jan 06, 2020 at 12:48:02AM +0000, Jeff LaBundy wrote: > > > I thought we dicussed having a comment here, saying something like: > > > > > > The device might reset when [...] and as a result looses it's > > > configuration. So the registers must be rewritten when this > > > happens to restore the expected operation. > > > > > > Is it worth to issue a warning when this happens? > > > > The detailed comments and an error message have always been in iqs62x_irq of the > > parent MFD driver. The pwm is only one of up to three sub-devices that subscribe > > to the chain and must update their locally owned registers after the MFD handles > > the interrupt and restores the device's firmware. I opted to keep these comments > > in the common MFD rather than repeating throughout the series. > > That's fine then, a comment that the parent driver issues a message > would be great then. Sure thing, will do. > > > > > +static int iqs620_pwm_notifier(struct notifier_block *notifier, > > > > + unsigned long event_flags, void *context) > > > > +{ > > > > + struct iqs620_pwm_private *iqs620_pwm; > > > > + int ret; > > > > + > > > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > > > + notifier); > > > > + > > > > + if (!completion_done(&iqs620_pwm->chip_ready) || > > > > + !(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > > > + return NOTIFY_DONE; > > > > > > Is here a (relevant?) race? Consider the notifier triggers just when > > > pwmchip_add returned, (maybe even a consumer configured the device) and > > > before complete_all() is called. With my limited knowledge about > > > notifiers I'd say waiting for the completion here might be reasonable > > > and safe. > > > > Great catch; this is theoretically possible. The problem with waiting, however, > > is if the notifier is triggered right away during probe but probe returns early > > due to an error (and completion never happens). > > OK, the error path would need to complete .chip_ready then and the > notifier then check for this error. Indeed messy. > > > At this point, I think the best option is to simply cache the values written to > > IQS620_PWR_SETTINGS_PWM_OUT and IQS620_PWM_DUTY_CYCLE and restore them from the > > notifier, which is essentially what is done for the IIO drivers in this series. > > Sounds good. > > > > > + ret = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh, > > > > + &iqs620_pwm->notifier); > > > > + if (ret) > > > > + dev_err(iqs620_pwm->chip.dev, > > > > + "Failed to unregister notifier: %d\n", ret); > > > > > > dev_err(iqs620_pwm->chip.dev, > > > "Failed to unregister notifier: %pe\n", ERR_PTR(ret)); > > > > > > gives a nicer output. (Also applies to other error messages of course.) > > > > > > > I don't disagree, but this gives me some pause. If I made this change here, I'd > > prefer to do so across the series for consistency. However, I am hesitant to do > > so at this stage in the review since several patches are somewhat stable by now > > (unless there was a compelling reason from a functional perspective). > > > > Another reason is that there are many dev_err cases throughout this series, and > > adopting this very recently introduced functionality would make the series even > > harder to back port to the present lot of LTS kernels. > > > > Unless this is a deal breaker, I'd like to pass on this for v4. However, please > > let me know if you feel strongly about it. In the meantime, I'll get started on > > the couple of other changes discussed here. > > OK, being able to backport is a valid excuse. Consistency over the whole > series wouldn't be one of my reasons, your mileage obviously varies > (which is OK). > > This can also be done later. Conversion to this is on my todo-list (not > at the top though), but if you beat me to it, I won't be angry :-) > Thank you for your understanding. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ | I'll send out v4 in the next day or so after I finish some testing. Kind regards, Jeff LaBundy
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..ee5d8b5 --- /dev/null +++ b/drivers/pwm/pwm-iqs620a.c @@ -0,0 +1,254 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Azoteq IQS620A PWM Generator + * + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com> + * + * Limitations: + * - 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. + * - 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. + */ + +#include <linux/completion.h> +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/mfd/iqs62x.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/notifier.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; + struct completion chip_ready; + struct mutex lock; +}; + +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; +} + +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); + + /* + * Since the device cannot generate a 0% duty cycle, requests to do so + * cause 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. + */ + 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); +} + +static int iqs620_pwm_notifier(struct notifier_block *notifier, + unsigned long event_flags, void *context) +{ + struct iqs620_pwm_private *iqs620_pwm; + int ret; + + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, + notifier); + + if (!completion_done(&iqs620_pwm->chip_ready) || + !(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) + return NOTIFY_DONE; + + ret = iqs620_pwm_apply(&iqs620_pwm->chip, &iqs620_pwm->chip.pwms[0], + &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 void iqs620_pwm_notifier_unregister(void *context) +{ + struct iqs620_pwm_private *iqs620_pwm = context; + int ret; + + ret = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh, + &iqs620_pwm->notifier); + if (ret) + dev_err(iqs620_pwm->chip.dev, + "Failed to unregister notifier: %d\n", ret); +} + +static int iqs620_pwm_probe(struct platform_device *pdev) +{ + struct iqs620_pwm_private *iqs620_pwm; + int ret; + + 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; + + init_completion(&iqs620_pwm->chip_ready); + mutex_init(&iqs620_pwm->lock); + + iqs620_pwm->notifier.notifier_call = iqs620_pwm_notifier; + ret = blocking_notifier_chain_register(&iqs620_pwm->iqs62x->nh, + &iqs620_pwm->notifier); + if (ret) { + dev_err(&pdev->dev, "Failed to register notifier: %d\n", ret); + return ret; + } + + ret = devm_add_action_or_reset(&pdev->dev, + iqs620_pwm_notifier_unregister, + iqs620_pwm); + if (ret) + return ret; + + ret = pwmchip_add(&iqs620_pwm->chip); + if (ret) { + dev_err(&pdev->dev, "Failed to add device: %d\n", ret); + return ret; + } + + /* + * pwmchip_add is called last to avoid a messy tear-down path, so the + * following completion prevents iqs620_pwm_notifier from referencing + * the pwm_chip structure until it has been completely initialized. + */ + complete_all(&iqs620_pwm->chip_ready); + + return 0; +} + +static int iqs620_pwm_remove(struct platform_device *pdev) +{ + struct iqs620_pwm_private *iqs620_pwm = platform_get_drvdata(pdev); + int 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 v3: - Updated the commit message to say "~0.4%" instead of "0.4%" - Clarified the effect of duty cycle and state changes in the 'Limitations' section and added a restriction regarding 0% duty cycle - Added a comment in iqs620_pwm_apply to explain how duty cycle is derived - Updated iqs620_pwm_apply to disable the output first and enable it last to prevent temporarily driving a stale duty cycle - Rounded the calculation for duty cycle up and down in iqs620_pwm_get_state and iqs620_pwm_apply, respectively - Added a comment in iqs620_pwm_get_state to explain what it reports follow- ing requests to set duty cycle to 0% - Added a lock to prevent back-to-back access of IQS620_PWR_SETTINGS_PWM_OUT and IQS620_PWM_DUTY_CYCLE from being interrupted - Updated iqs620_pwm_notifier to reference pwm->state directly as opposed to calling pwm_get_state - Moved notifier unregistration back to a device-managed action - Added a completion to prevent iqs620_pwm_notifier from referencing the pwm_chip structure until it has been initialized by pwmchip_add 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 | 254 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 265 insertions(+) create mode 100644 drivers/pwm/pwm-iqs620a.c -- 2.7.4