| Message ID | 1571631083-4962-6-git-send-email-jeff@labundy.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for Azoteq IQS620A/621/622/624/625 | expand |
Hello Jeff, On Sun, Oct 20, 2019 at 11:11:20PM -0500, 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> > --- > drivers/pwm/Kconfig | 10 +++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-iqs620a.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 178 insertions(+) > create mode 100644 drivers/pwm/pwm-iqs620a.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index e3a2518..712445e 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 This is only a runtime dependency if I'm not mistaken, so it would be great to have depends on MFD_IQS62X || COMPILE_TEST depends on REGMAP here. > + 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 26326ad..27c9bfa 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..6451eb1 > --- /dev/null > +++ b/drivers/pwm/pwm-iqs620a.c > @@ -0,0 +1,167 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * Azoteq IQS620A PWM Generator > + * > + * Copyright (C) 2019 > + * Author: Jeff LaBundy <jeff@labundy.com> > + */ > + > +#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; > + bool ready; This is always true, so you can drop it. > +}; > + > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) Since 71523d1812ac ("pwm: Ensure pwm_apply_state() doesn't modify the state argument") this isn't the right prototype. > +{ > + struct iqs620_pwm_private *iqs620_pwm; > + struct iqs62x_core *iqs62x; > + int error; > + int duty_calc = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; > + u8 duty_clamp = clamp(duty_calc, 0, 0xFF); > + > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > + iqs62x = iqs620_pwm->iqs62x; > + > + error = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, duty_clamp); > + if (error) > + return error; > + > + state->period = IQS620_PWM_PERIOD_NS; > + state->duty_cycle = (duty_clamp + 1) * IQS620_PWM_PERIOD_NS / 256; This suggests that if the value in the IQS620_PWM_DUTY_CYCLE is 0 the duty cycle is 1/256 ms with a period of 1 ms and the output cannot be constant inactive. If this is right please add a paragraph in the driver's comment at the top: * Limitations: * - The hardware cannot generate a 0% duty cycle (Please stick to this format, other drivers use it, too.) How does the hardware behave on changes? For example you're first committing the duty cycle and then on/off. Can it happen that between pwm_apply_state(pwm, { .duty_cycle = 3900, .period = 1000000, .enabled = true) ... pwm_apply_state(pwm, { .duty_cycle = 1000000, .period = 1000000, .enabled = false) the output is active for longer than 4 µs because the iqs620_pwm_apply function is preempted between the two register writes and so we already have .duty_cycle = 1000000 but still .enabled = true in the hardware? Does a change complete the currently running period? Does disabling complete the currently running period? If so, does regmap_update_bits block until the new setting is active? The .apply function fails to check for .pwm_polarity. You want something like: if (state->polarity != PWM_POLARITY_NORMAL) return -ENOTSUPP; (That's what pwm-rcar and the core (in the absence of .set_polarity for old-style drivers) are using. @Thierry: It would be great to fix the vaule that should be returned in this case. pwm-lpss and sifive use -EINVAL.) > + return regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > + IQS620_PWR_SETTINGS_PWM_OUT, > + state->enabled ? 0xFF : 0); > +} > + > +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 error; > + > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > + notifier); > + > + if (!iqs620_pwm->ready || !(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > + return NOTIFY_DONE; > + > + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); > + > + error = iqs620_pwm_apply(&iqs620_pwm->chip, > + &iqs620_pwm->chip.pwms[0], &state); > + if (error) { > + dev_err(iqs620_pwm->chip.dev, > + "Failed to re-initialize device: %d\n", error); > + return NOTIFY_BAD; > + } > + > + return NOTIFY_OK; So the PWM can loose it's state sometimes? When does that happen? > +} > + > +static void iqs620_pwm_notifier_unregister(void *context) > +{ > + struct iqs620_pwm_private *iqs620_pwm = context; > + int error; > + > + error = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh, > + &iqs620_pwm->notifier); > + if (error) > + dev_err(iqs620_pwm->chip.dev, > + "Failed to unregister notifier: %d\n", error); > +} > + > +static const struct pwm_ops iqs620_pwm_ops = { > + .apply = iqs620_pwm_apply, Please implement a .get_state callback. > + .owner = THIS_MODULE, > +}; > + > +static int iqs620_pwm_probe(struct platform_device *pdev) > +{ > + struct iqs620_pwm_private *iqs620_pwm; > + int error; > + > + 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; > + > + iqs620_pwm->notifier.notifier_call = iqs620_pwm_notifier; > + error = blocking_notifier_chain_register(&iqs620_pwm->iqs62x->nh, > + &iqs620_pwm->notifier); > + if (error) { > + dev_err(&pdev->dev, "Failed to register notifier: %d\n", error); > + return error; > + } > + > + error = devm_add_action_or_reset(&pdev->dev, > + iqs620_pwm_notifier_unregister, > + iqs620_pwm); I wonder if this is safe. If in iqs620_pwm_notifier_unregister() unregistering of the notifier goes wrong (not sure when this can happen) the memory behind iqs620_pwm goes away. Then later iqs620_pwm_notifier might be called trying to use *iqs620_pwm ... > + if (error) { > + dev_err(&pdev->dev, "Failed to add action: %d\n", error); > + return error; > [...] > > +static struct platform_driver iqs620_pwm_platform_driver = { > + .driver = { > + .name = IQS620_DRV_NAME_PWM, > + }, > + .probe = iqs620_pwm_probe, > + .remove = iqs620_pwm_remove, > +}; I'm not a big fan of aligning the = in struct initializers. The downside is that if you later add .prevent_deferred_probe = true, you either have to touch all (otherwise unrelated) lines to realign which adds churn, or the structure is only partially aligned which looks ugly. That's why I stick to a single space before the =. Best regards Uwe
Hi Uwe, Thank you for your prompt review. On Mon, Oct 21, 2019 at 09:34:19AM +0200, Uwe Kleine-König wrote: > Hello Jeff, > > On Sun, Oct 20, 2019 at 11:11:20PM -0500, 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> > > --- > > drivers/pwm/Kconfig | 10 +++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-iqs620a.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 178 insertions(+) > > create mode 100644 drivers/pwm/pwm-iqs620a.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index e3a2518..712445e 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 > > This is only a runtime dependency if I'm not mistaken, so it would be > great to have > > depends on MFD_IQS62X || COMPILE_TEST > depends on REGMAP > > here. > Sure thing; will do. Actually, it seems I can add this to all but the input driver, as that one relies on iqs62x_events exported from the MFD. > > + 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 26326ad..27c9bfa 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..6451eb1 > > --- /dev/null > > +++ b/drivers/pwm/pwm-iqs620a.c > > @@ -0,0 +1,167 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Azoteq IQS620A PWM Generator > > + * > > + * Copyright (C) 2019 > > + * Author: Jeff LaBundy <jeff@labundy.com> > > + */ > > + > > +#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; > > + bool ready; > > This is always true, so you can drop it. > This is here because iqs620_pwm_notifier references chip.pwms, which is not allocated until after the notifier is registered and pwmchip_add is called. So it protects against this (albeit unlikely) race condition: 1. iqs620_pwm_notifier is registered 2. Device immediately suffers an asynchronous reset and notifier chain is called (more on that in a bit) 3. iqs620_pwm_notifier evaluates chips.pwms (NULL) I felt this was simpler than calling pwmchip_add before registering the notifier and adding an error/tear-down path in iqs620_pwm_probe in case of failure. I would be happy to add a comment or two to explain the not- so-obvious purpose of this flag. > > +}; > > + > > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + struct pwm_state *state) > > Since > > 71523d1812ac ("pwm: Ensure pwm_apply_state() doesn't modify the state argument") > > this isn't the right prototype. > Sure thing; I will add the 'const' qualifier and remove the two changes to the state argument. > > +{ > > + struct iqs620_pwm_private *iqs620_pwm; > > + struct iqs62x_core *iqs62x; > > + int error; > > + int duty_calc = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; > > + u8 duty_clamp = clamp(duty_calc, 0, 0xFF); > > + > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > + iqs62x = iqs620_pwm->iqs62x; > > + > > + error = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, duty_clamp); > > + if (error) > > + return error; > > + > > + state->period = IQS620_PWM_PERIOD_NS; > > + state->duty_cycle = (duty_clamp + 1) * IQS620_PWM_PERIOD_NS / 256; > > This suggests that if the value in the IQS620_PWM_DUTY_CYCLE is 0 the > duty cycle is 1/256 ms with a period of 1 ms and the output cannot be > constant inactive. If this is right please add a paragraph in the > driver's comment at the top: > > * Limitations: > * - The hardware cannot generate a 0% duty cycle > > (Please stick to this format, other drivers use it, too.) > That's correct; the lowest duty cycle that can be achieved using only the IQS620_PWM_DUTY_CYCLE register is 0.4%. We can, however, generate 0% duty cycle by disabling the output altogether using a separate register. Would that be better than flat-out saying it's impossible? > How does the hardware behave on changes? For example you're first > committing the duty cycle and then on/off. Can it happen that between > > pwm_apply_state(pwm, { .duty_cycle = 3900, .period = 1000000, .enabled = true) > ... > pwm_apply_state(pwm, { .duty_cycle = 1000000, .period = 1000000, .enabled = false) > > the output is active for longer than 4 µs because the iqs620_pwm_apply > function is preempted between the two register writes and so we already > have .duty_cycle = 1000000 but still .enabled = true in the hardware? > My results show that it is possible to generate up to two irregular periods by changing the duty cycle while the output is active. Depending on the ratio of old-to-new duty cycle and the position of the I2C write relative to the asynchronous output, the device may produce one pulse for which the width represents neither the old nor the new duty cycle. > Does a change complete the currently running period? Does disabling > complete the currently running period? If so, does regmap_update_bits > block until the new setting is active? > A quick test reveals the following: * Duty cycle changes may interrupt a running period, i.e., the output may transition in the middle of the period to accommodate the new duty cycle. * Disabling the output drives it to zero immediately, i.e., the period does does not run to completion. I will add a 'Limitations' section at the top as other drivers do, and call these points out specifically. > The .apply function fails to check for .pwm_polarity. You want something > like: > > if (state->polarity != PWM_POLARITY_NORMAL) > return -ENOTSUPP; > > (That's what pwm-rcar and the core (in the absence of .set_polarity for > old-style drivers) are using. @Thierry: It would be great to fix the > vaule that should be returned in this case. pwm-lpss and sifive use > -EINVAL.) > Sure thing; I'll return -ENOTSUPP in this case. > > + return regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, > > + IQS620_PWR_SETTINGS_PWM_OUT, > > + state->enabled ? 0xFF : 0); > > +} > > + > > +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 error; > > + > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > + notifier); > > + > > + if (!iqs620_pwm->ready || !(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > + return NOTIFY_DONE; > > + > > + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); > > + > > + error = iqs620_pwm_apply(&iqs620_pwm->chip, > > + &iqs620_pwm->chip.pwms[0], &state); > > + if (error) { > > + dev_err(iqs620_pwm->chip.dev, > > + "Failed to re-initialize device: %d\n", error); > > + return NOTIFY_BAD; > > + } > > + > > + return NOTIFY_OK; > > So the PWM can loose it's state sometimes? When does that happen? > That's correct. The device performs an internal soft reset in the presence of what it considers to be an I2C timeout error; in this case all registers are restored to their default values. The data sheet goes so far as to recommend monitoring for this interrupt and restoring the device on-the-fly. I have added some comments in iqs62x_irq in patch [2/8] which provides some further detail. > > +} > > + > > +static void iqs620_pwm_notifier_unregister(void *context) > > +{ > > + struct iqs620_pwm_private *iqs620_pwm = context; > > + int error; > > + > > + error = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh, > > + &iqs620_pwm->notifier); > > + if (error) > > + dev_err(iqs620_pwm->chip.dev, > > + "Failed to unregister notifier: %d\n", error); > > +} > > + > > +static const struct pwm_ops iqs620_pwm_ops = { > > + .apply = iqs620_pwm_apply, > > Please implement a .get_state callback. > Sure thing; will do. > > + .owner = THIS_MODULE, > > +}; > > + > > +static int iqs620_pwm_probe(struct platform_device *pdev) > > +{ > > + struct iqs620_pwm_private *iqs620_pwm; > > + int error; > > + > > + 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; > > + > > + iqs620_pwm->notifier.notifier_call = iqs620_pwm_notifier; > > + error = blocking_notifier_chain_register(&iqs620_pwm->iqs62x->nh, > > + &iqs620_pwm->notifier); > > + if (error) { > > + dev_err(&pdev->dev, "Failed to register notifier: %d\n", error); > > + return error; > > + } > > + > > + error = devm_add_action_or_reset(&pdev->dev, > > + iqs620_pwm_notifier_unregister, > > + iqs620_pwm); > > I wonder if this is safe. If in iqs620_pwm_notifier_unregister() > unregistering of the notifier goes wrong (not sure when this can happen) > the memory behind iqs620_pwm goes away. Then later iqs620_pwm_notifier > might be called trying to use *iqs620_pwm ... I think this is purely theoretical, as blocking_notifier_chain_unregister only fails if the notifier is not found in the chain. If for some reason blocking_notifier_chain_register fails (which currently cannot happen, as it always returns zero), the driver will fail to probe before the action could be added. This of course means the error message in iqs620_pwm_notifier_unregister is unnecessary; it is simply provided for debug/visibility. > > > + if (error) { > > + dev_err(&pdev->dev, "Failed to add action: %d\n", error); > > + return error; > > [...] > > > > +static struct platform_driver iqs620_pwm_platform_driver = { > > + .driver = { > > + .name = IQS620_DRV_NAME_PWM, > > + }, > > + .probe = iqs620_pwm_probe, > > + .remove = iqs620_pwm_remove, > > +}; > > I'm not a big fan of aligning the = in struct initializers. The downside > is that if you later add > > .prevent_deferred_probe = true, > > you either have to touch all (otherwise unrelated) lines to realign > which adds churn, or the structure is only partially aligned which looks > ugly. That's why I stick to a single space before the =. > Sure thing; your argument is valid and I will reduce to a single space. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Kind regards, Jeff LaBundy
Hello Jeff, On Mon, Oct 21, 2019 at 11:36:49PM -0500, Jeff LaBundy wrote: > On Mon, Oct 21, 2019 at 09:34:19AM +0200, Uwe Kleine-König wrote: > > > +struct iqs620_pwm_private { > > > + struct iqs62x_core *iqs62x; > > > + struct pwm_chip chip; > > > + struct notifier_block notifier; > > > + bool ready; > > > > This is always true, so you can drop it. > > > > This is here because iqs620_pwm_notifier references chip.pwms, which is > not allocated until after the notifier is registered and pwmchip_add is > called. So it protects against this (albeit unlikely) race condition: > > 1. iqs620_pwm_notifier is registered > 2. Device immediately suffers an asynchronous reset and notifier chain > is called (more on that in a bit) > 3. iqs620_pwm_notifier evaluates chips.pwms (NULL) > > I felt this was simpler than calling pwmchip_add before registering the > notifier and adding an error/tear-down path in iqs620_pwm_probe in case > of failure. I would be happy to add a comment or two to explain the not- > so-obvious purpose of this flag. Ah, understood. A comment is definitively necessary here. > > > +}; > > > + > > > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > + struct pwm_state *state) > > > > Since > > > > 71523d1812ac ("pwm: Ensure pwm_apply_state() doesn't modify the state argument") > > > > this isn't the right prototype. > > > > Sure thing; I will add the 'const' qualifier and remove the two changes > to the state argument. > > > > +{ > > > + struct iqs620_pwm_private *iqs620_pwm; > > > + struct iqs62x_core *iqs62x; > > > + int error; > > > + int duty_calc = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; > > > + u8 duty_clamp = clamp(duty_calc, 0, 0xFF); Another problem that we have here is that the period is fixed to 1 ms and if a consumer requests for example: .period = 5000000, .duty_cycle = 1000000, the hardware is actually configured for .period = 1000000, .duty_cycle = 1000000, . I don't have a good suggestion how to fix this. We'd need to draw a line somewhere and decline a request that is too far from the result. But where this line should be is not obvious, it should definitively not be implemented in the driver itself IMHO. (The only halfway sane approach would be to let lowlevel drivers implement a .round_state callback and then let the framework judge. But we're a long way from having that, so that's not a solution for today.) > > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > > + iqs62x = iqs620_pwm->iqs62x; > > > + > > > + error = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, duty_clamp); > > > + if (error) > > > + return error; > > > + > > > + state->period = IQS620_PWM_PERIOD_NS; > > > + state->duty_cycle = (duty_clamp + 1) * IQS620_PWM_PERIOD_NS / 256; > > > > This suggests that if the value in the IQS620_PWM_DUTY_CYCLE is 0 the > > duty cycle is 1/256 ms with a period of 1 ms and the output cannot be > > constant inactive. If this is right please add a paragraph in the > > driver's comment at the top: > > > > * Limitations: > > * - The hardware cannot generate a 0% duty cycle > > > > (Please stick to this format, other drivers use it, too.) > > That's correct; the lowest duty cycle that can be achieved using only the > IQS620_PWM_DUTY_CYCLE register is 0.4%. We can, however, generate 0% duty > cycle by disabling the output altogether using a separate register. Would > that be better than flat-out saying it's impossible? There is (maybe) a small difference between disabled and 0% duty cycle, at least from the framework's POV: If you do: pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); pwm_apply_state(pwm, { .enabled = false, .period = $DC, .duty_cycle = $DC, }); pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); and compare it to the expected result of pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 0, }); pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); the difference is that the duration of the inactive phase in the latter case is a multiple of 1 ms. There is no policy for lowlevel drivers what to do, but disabling when 0% is requested is at least not unseen and probably more what consumers expect. > > How does the hardware behave on changes? For example you're first > > committing the duty cycle and then on/off. Can it happen that between > > > > pwm_apply_state(pwm, { .duty_cycle = 3900, .period = 1000000, .enabled = true) > > ... > > pwm_apply_state(pwm, { .duty_cycle = 1000000, .period = 1000000, .enabled = false) > > > > the output is active for longer than 4 µs because the iqs620_pwm_apply > > function is preempted between the two register writes and so we already > > have .duty_cycle = 1000000 but still .enabled = true in the hardware? > > > > My results show that it is possible to generate up to two irregular periods > by changing the duty cycle while the output is active. > > Depending on the ratio of old-to-new duty cycle and the position of the I2C > write relative to the asynchronous output, the device may produce one pulse > for which the width represents neither the old nor the new duty cycle. > > > Does a change complete the currently running period? Does disabling > > complete the currently running period? If so, does regmap_update_bits > > block until the new setting is active? > > > > A quick test reveals the following: > > * Duty cycle changes may interrupt a running period, i.e., the output may > transition in the middle of the period to accommodate the new duty cycle. > * Disabling the output drives it to zero immediately, i.e., the period does > does not run to completion. > > I will add a 'Limitations' section at the top as other drivers do, and call > these points out specifically. Great. Thanks. > > > +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 error; > > > + > > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > > + notifier); > > > + > > > + if (!iqs620_pwm->ready || !(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > > + return NOTIFY_DONE; > > > + > > > + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); > > > + > > > + error = iqs620_pwm_apply(&iqs620_pwm->chip, > > > + &iqs620_pwm->chip.pwms[0], &state); > > > + if (error) { > > > + dev_err(iqs620_pwm->chip.dev, > > > + "Failed to re-initialize device: %d\n", error); > > > + return NOTIFY_BAD; > > > + } > > > + > > > + return NOTIFY_OK; > > > > So the PWM can loose it's state sometimes? When does that happen? > > That's correct. The device performs an internal soft reset in the presence > of what it considers to be an I2C timeout error; in this case all registers > are restored to their default values. Is this a theoretic problem or does that happen from time to time? > The data sheet goes so far as to recommend monitoring for this interrupt and > restoring the device on-the-fly. I have added some comments in iqs62x_irq in > patch [2/8] which provides some further detail. Monitoring that interrupt seems reasonable. > > > + error = devm_add_action_or_reset(&pdev->dev, > > > + iqs620_pwm_notifier_unregister, > > > + iqs620_pwm); > > > > I wonder if this is safe. If in iqs620_pwm_notifier_unregister() > > unregistering of the notifier goes wrong (not sure when this can happen) > > the memory behind iqs620_pwm goes away. Then later iqs620_pwm_notifier > > might be called trying to use *iqs620_pwm ... > > I think this is purely theoretical, as blocking_notifier_chain_unregister > only fails if the notifier is not found in the chain. If for some reason > blocking_notifier_chain_register fails (which currently cannot happen, as > it always returns zero), the driver will fail to probe before the action > could be added. > > This of course means the error message in iqs620_pwm_notifier_unregister > is unnecessary; it is simply provided for debug/visibility. I'd suggest to do the unregister call in the remove callback which you have for pwm unregistration anyhow. Or alternatively implement a devm_ variant of the notifier registration that explains in the comments that it is safe. Best regards Uwe
Hi Uwe, On Tue, Oct 22, 2019 at 08:54:15AM +0200, Uwe Kleine-König wrote: > Hello Jeff, > > On Mon, Oct 21, 2019 at 11:36:49PM -0500, Jeff LaBundy wrote: > > On Mon, Oct 21, 2019 at 09:34:19AM +0200, Uwe Kleine-König wrote: > > > > +struct iqs620_pwm_private { > > > > + struct iqs62x_core *iqs62x; > > > > + struct pwm_chip chip; > > > > + struct notifier_block notifier; > > > > + bool ready; > > > > > > This is always true, so you can drop it. > > > > > > > This is here because iqs620_pwm_notifier references chip.pwms, which is > > not allocated until after the notifier is registered and pwmchip_add is > > called. So it protects against this (albeit unlikely) race condition: > > > > 1. iqs620_pwm_notifier is registered > > 2. Device immediately suffers an asynchronous reset and notifier chain > > is called (more on that in a bit) > > 3. iqs620_pwm_notifier evaluates chips.pwms (NULL) > > > > I felt this was simpler than calling pwmchip_add before registering the > > notifier and adding an error/tear-down path in iqs620_pwm_probe in case > > of failure. I would be happy to add a comment or two to explain the not- > > so-obvious purpose of this flag. > > Ah, understood. A comment is definitively necessary here. > Sure thing; will do. > > > > +}; > > > > + > > > > +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > > + struct pwm_state *state) > > > > > > Since > > > > > > 71523d1812ac ("pwm: Ensure pwm_apply_state() doesn't modify the state argument") > > > > > > this isn't the right prototype. > > > > > > > Sure thing; I will add the 'const' qualifier and remove the two changes > > to the state argument. > > > > > > +{ > > > > + struct iqs620_pwm_private *iqs620_pwm; > > > > + struct iqs62x_core *iqs62x; > > > > + int error; > > > > + int duty_calc = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; > > > > + u8 duty_clamp = clamp(duty_calc, 0, 0xFF); > > Another problem that we have here is that the period is fixed to 1 ms > and if a consumer requests for example: > > .period = 5000000, > .duty_cycle = 1000000, > > the hardware is actually configured for > > .period = 1000000, > .duty_cycle = 1000000, > > . I don't have a good suggestion how to fix this. We'd need to > draw a line somewhere and decline a request that is too far from the > result. But where this line should be is not obvious, it should > definitively not be implemented in the driver itself IMHO. > > (The only halfway sane approach would be to let lowlevel drivers > implement a .round_state callback and then let the framework judge. But > we're a long way from having that, so that's not a solution for today.) > Agreed on all counts. For now, I will mention in the 'Limitations' heading that the period cannot be adjusted. > > > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > > > + iqs62x = iqs620_pwm->iqs62x; > > > > + > > > > + error = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, duty_clamp); > > > > + if (error) > > > > + return error; > > > > + > > > > + state->period = IQS620_PWM_PERIOD_NS; > > > > + state->duty_cycle = (duty_clamp + 1) * IQS620_PWM_PERIOD_NS / 256; > > > > > > This suggests that if the value in the IQS620_PWM_DUTY_CYCLE is 0 the > > > duty cycle is 1/256 ms with a period of 1 ms and the output cannot be > > > constant inactive. If this is right please add a paragraph in the > > > driver's comment at the top: > > > > > > * Limitations: > > > * - The hardware cannot generate a 0% duty cycle > > > > > > (Please stick to this format, other drivers use it, too.) > > > > That's correct; the lowest duty cycle that can be achieved using only the > > IQS620_PWM_DUTY_CYCLE register is 0.4%. We can, however, generate 0% duty > > cycle by disabling the output altogether using a separate register. Would > > that be better than flat-out saying it's impossible? > > There is (maybe) a small difference between disabled and 0% duty cycle, > at least from the framework's POV: If you do: > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > pwm_apply_state(pwm, { .enabled = false, .period = $DC, .duty_cycle = $DC, }); > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > > and compare it to the expected result of > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 0, }); > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > > the difference is that the duration of the inactive phase in the latter > case is a multiple of 1 ms. > > There is no policy for lowlevel drivers what to do, but disabling when > 0% is requested is at least not unseen and probably more what consumers > expect. > With the change I am proposing, the output will be driven to zero if enabled = false OR duty_cycle < 4000 ns. Stated another way: enable duty_cycle IQS620_PWR_SETTINGS[7] IQS620_PWM_DUTY_CYCLE ------ ---------- ---------------------- --------------------- 0 don't care 0 don't care 1 0 ... 3999 0 don't care 1 4000 ... x 1 0 1 x+1 ... y 1 1 ...and so on. For context, if IQS620_PWR_SETTINGS[7] = 0 then the output is held to zero. If IQS620_PWR_SETTINGS[7] = 1 then the output toggles at a duty cycle between 0.4% and 100% as a function of IQS620_PWM_DUTY_CYCLE. Based on how the device behaves in response to its two available registers, I think your two examples will appear equal, but please let me know if I have understood. > > > How does the hardware behave on changes? For example you're first > > > committing the duty cycle and then on/off. Can it happen that between > > > > > > pwm_apply_state(pwm, { .duty_cycle = 3900, .period = 1000000, .enabled = true) > > > ... > > > pwm_apply_state(pwm, { .duty_cycle = 1000000, .period = 1000000, .enabled = false) > > > > > > the output is active for longer than 4 µs because the iqs620_pwm_apply > > > function is preempted between the two register writes and so we already > > > have .duty_cycle = 1000000 but still .enabled = true in the hardware? > > > > > > > My results show that it is possible to generate up to two irregular periods > > by changing the duty cycle while the output is active. > > > > Depending on the ratio of old-to-new duty cycle and the position of the I2C > > write relative to the asynchronous output, the device may produce one pulse > > for which the width represents neither the old nor the new duty cycle. > > > > > Does a change complete the currently running period? Does disabling > > > complete the currently running period? If so, does regmap_update_bits > > > block until the new setting is active? > > > > > > > A quick test reveals the following: > > > > * Duty cycle changes may interrupt a running period, i.e., the output may > > transition in the middle of the period to accommodate the new duty cycle. > > * Disabling the output drives it to zero immediately, i.e., the period does > > does not run to completion. > > > > I will add a 'Limitations' section at the top as other drivers do, and call > > these points out specifically. > > Great. Thanks. > > > > > +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 error; > > > > + > > > > + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, > > > > + notifier); > > > > + > > > > + if (!iqs620_pwm->ready || !(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) > > > > + return NOTIFY_DONE; > > > > + > > > > + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); > > > > + > > > > + error = iqs620_pwm_apply(&iqs620_pwm->chip, > > > > + &iqs620_pwm->chip.pwms[0], &state); > > > > + if (error) { > > > > + dev_err(iqs620_pwm->chip.dev, > > > > + "Failed to re-initialize device: %d\n", error); > > > > + return NOTIFY_BAD; > > > > + } > > > > + > > > > + return NOTIFY_OK; > > > > > > So the PWM can loose it's state sometimes? When does that happen? > > > > That's correct. The device performs an internal soft reset in the presence > > of what it considers to be an I2C timeout error; in this case all registers > > are restored to their default values. > > Is this a theoretic problem or does that happen from time to time? > This event can occur if the I2C master stalls a transaction for 10's of ms. It's not a theoretical problem, but it should not happen during normal circumstances. > > The data sheet goes so far as to recommend monitoring for this interrupt and > > restoring the device on-the-fly. I have added some comments in iqs62x_irq in > > patch [2/8] which provides some further detail. > > Monitoring that interrupt seems reasonable. > > > > > + error = devm_add_action_or_reset(&pdev->dev, > > > > + iqs620_pwm_notifier_unregister, > > > > + iqs620_pwm); > > > > > > I wonder if this is safe. If in iqs620_pwm_notifier_unregister() > > > unregistering of the notifier goes wrong (not sure when this can happen) > > > the memory behind iqs620_pwm goes away. Then later iqs620_pwm_notifier > > > might be called trying to use *iqs620_pwm ... > > > > I think this is purely theoretical, as blocking_notifier_chain_unregister > > only fails if the notifier is not found in the chain. If for some reason > > blocking_notifier_chain_register fails (which currently cannot happen, as > > it always returns zero), the driver will fail to probe before the action > > could be added. > > > > This of course means the error message in iqs620_pwm_notifier_unregister > > is unnecessary; it is simply provided for debug/visibility. > > I'd suggest to do the unregister call in the remove callback which you > have for pwm unregistration anyhow. Or alternatively implement a devm_ > variant of the notifier registration that explains in the comments that > it is safe. Sure thing; I'll unregister the notifier in iqs620_pwm_remove. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Kind regards, Jeff LaBundy
Hello Jeff, On Tue, Oct 22, 2019 at 09:45:25PM -0500, Jeff LaBundy wrote: > On Tue, Oct 22, 2019 at 08:54:15AM +0200, Uwe Kleine-König wrote: > > On Mon, Oct 21, 2019 at 11:36:49PM -0500, Jeff LaBundy wrote: > > > On Mon, Oct 21, 2019 at 09:34:19AM +0200, Uwe Kleine-König wrote: > > > > > +{ > > > > > + struct iqs620_pwm_private *iqs620_pwm; > > > > > + struct iqs62x_core *iqs62x; > > > > > + int error; > > > > > + int duty_calc = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; > > > > > + u8 duty_clamp = clamp(duty_calc, 0, 0xFF); > > > > Another problem that we have here is that the period is fixed to 1 ms > > and if a consumer requests for example: > > > > .period = 5000000, > > .duty_cycle = 1000000, > > > > the hardware is actually configured for > > > > .period = 1000000, > > .duty_cycle = 1000000, > > > > . I don't have a good suggestion how to fix this. We'd need to > > draw a line somewhere and decline a request that is too far from the > > result. But where this line should be is not obvious, it should > > definitively not be implemented in the driver itself IMHO. > > > > (The only halfway sane approach would be to let lowlevel drivers > > implement a .round_state callback and then let the framework judge. But > > we're a long way from having that, so that's not a solution for today.) > > > > Agreed on all counts. For now, I will mention in the 'Limitations' heading that > the period cannot be adjusted. Ack. My longterm plan is to require .apply_state() to round down both .period and .duty_cycle. This isn't wrong already today, so I suggest you decline a request to set the period to something smaller than 1 ms with an error code. (I think most drivers use -EINVAL here, conceptually -EDOM might be sensible. I'd stick to EINVAL for now.) > > > > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > > > > + iqs62x = iqs620_pwm->iqs62x; > > > > > + > > > > > + error = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, duty_clamp); > > > > > + if (error) > > > > > + return error; > > > > > + > > > > > + state->period = IQS620_PWM_PERIOD_NS; > > > > > + state->duty_cycle = (duty_clamp + 1) * IQS620_PWM_PERIOD_NS / 256; > > > > > > > > This suggests that if the value in the IQS620_PWM_DUTY_CYCLE is 0 the > > > > duty cycle is 1/256 ms with a period of 1 ms and the output cannot be > > > > constant inactive. If this is right please add a paragraph in the > > > > driver's comment at the top: > > > > > > > > * Limitations: > > > > * - The hardware cannot generate a 0% duty cycle > > > > > > > > (Please stick to this format, other drivers use it, too.) > > > > > > That's correct; the lowest duty cycle that can be achieved using only the > > > IQS620_PWM_DUTY_CYCLE register is 0.4%. We can, however, generate 0% duty > > > cycle by disabling the output altogether using a separate register. Would > > > that be better than flat-out saying it's impossible? > > > > There is (maybe) a small difference between disabled and 0% duty cycle, > > at least from the framework's POV: If you do: > > > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > > pwm_apply_state(pwm, { .enabled = false, .period = $DC, .duty_cycle = $DC, }); > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > > > > and compare it to the expected result of > > > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 0, }); > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > > > > the difference is that the duration of the inactive phase in the latter > > case is a multiple of 1 ms. > > > > There is no policy for lowlevel drivers what to do, but disabling when > > 0% is requested is at least not unseen and probably more what consumers > > expect. > > > > With the change I am proposing, the output will be driven to zero if enabled = false > OR duty_cycle < 4000 ns. Stated another way: > > enable duty_cycle IQS620_PWR_SETTINGS[7] IQS620_PWM_DUTY_CYCLE > ------ ---------- ---------------------- --------------------- > 0 don't care 0 don't care > 1 0 ... 3999 0 don't care > 1 4000 ... x 1 0 > 1 x+1 ... y 1 1 > > ...and so on. For context, if IQS620_PWR_SETTINGS[7] = 0 then the output is held to > zero. If IQS620_PWR_SETTINGS[7] = 1 then the output toggles at a duty cycle between > 0.4% and 100% as a function of IQS620_PWM_DUTY_CYCLE. Your table isn't accurate. IQS620_PWM_DUTY_CYCLE=0 results in a duty_cycle of 3906.25 ns so the table should look as follows: enable duty_cycle IQS620_PWR_SETTINGS[7] IQS620_PWM_DUTY_CYCLE ------ ------------ ---------------------- --------------------- 0 don't care 0 don't care 1 [0, 3906] 0 don't care 1 [3907, 7812] 1 0 1 [7813,11718] 1 1 In general: dc = state->duty_cycle * 256 / 1000000 if state->enabled == false or dc == 0: IQS620_PWR_SETTINGS[7] = 0 else: IQS620_PWM_DUTY_CYCLE = min(dc - 1, 0xff) IQS620_PWR_SETTINGS[7] = 1 > Based on how the device behaves in response to its two available > registers, I think your two examples will appear equal, but please let > me know if I have understood. Yeah, that's the expectation. With the rounding as I suggested above this yields strange effects like if .period = 1 s, .duty_cycle = 0.5 s is requested you end up in .period = 1 ms, .duty_cycle = 1 ms but I think there is nothing we can reasonably do about this. Best regards Uwe
Hi Uwe, On Wed, Oct 23, 2019 at 09:23:04AM +0200, Uwe Kleine-König wrote: > Hello Jeff, > > On Tue, Oct 22, 2019 at 09:45:25PM -0500, Jeff LaBundy wrote: > > On Tue, Oct 22, 2019 at 08:54:15AM +0200, Uwe Kleine-König wrote: > > > On Mon, Oct 21, 2019 at 11:36:49PM -0500, Jeff LaBundy wrote: > > > > On Mon, Oct 21, 2019 at 09:34:19AM +0200, Uwe Kleine-König wrote: > > > > > > +{ > > > > > > + struct iqs620_pwm_private *iqs620_pwm; > > > > > > + struct iqs62x_core *iqs62x; > > > > > > + int error; > > > > > > + int duty_calc = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; > > > > > > + u8 duty_clamp = clamp(duty_calc, 0, 0xFF); > > > > > > Another problem that we have here is that the period is fixed to 1 ms > > > and if a consumer requests for example: > > > > > > .period = 5000000, > > > .duty_cycle = 1000000, > > > > > > the hardware is actually configured for > > > > > > .period = 1000000, > > > .duty_cycle = 1000000, > > > > > > . I don't have a good suggestion how to fix this. We'd need to > > > draw a line somewhere and decline a request that is too far from the > > > result. But where this line should be is not obvious, it should > > > definitively not be implemented in the driver itself IMHO. > > > > > > (The only halfway sane approach would be to let lowlevel drivers > > > implement a .round_state callback and then let the framework judge. But > > > we're a long way from having that, so that's not a solution for today.) > > > > > > > Agreed on all counts. For now, I will mention in the 'Limitations' heading that > > the period cannot be adjusted. > > Ack. My longterm plan is to require .apply_state() to round down both > .period and .duty_cycle. This isn't wrong already today, so I suggest > you decline a request to set the period to something smaller than 1 ms > with an error code. (I think most drivers use -EINVAL here, conceptually > -EDOM might be sensible. I'd stick to EINVAL for now.) > Sure thing; will do. > > > > > > + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); > > > > > > + iqs62x = iqs620_pwm->iqs62x; > > > > > > + > > > > > > + error = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, duty_clamp); > > > > > > + if (error) > > > > > > + return error; > > > > > > + > > > > > > + state->period = IQS620_PWM_PERIOD_NS; > > > > > > + state->duty_cycle = (duty_clamp + 1) * IQS620_PWM_PERIOD_NS / 256; > > > > > > > > > > This suggests that if the value in the IQS620_PWM_DUTY_CYCLE is 0 the > > > > > duty cycle is 1/256 ms with a period of 1 ms and the output cannot be > > > > > constant inactive. If this is right please add a paragraph in the > > > > > driver's comment at the top: > > > > > > > > > > * Limitations: > > > > > * - The hardware cannot generate a 0% duty cycle > > > > > > > > > > (Please stick to this format, other drivers use it, too.) > > > > > > > > That's correct; the lowest duty cycle that can be achieved using only the > > > > IQS620_PWM_DUTY_CYCLE register is 0.4%. We can, however, generate 0% duty > > > > cycle by disabling the output altogether using a separate register. Would > > > > that be better than flat-out saying it's impossible? > > > > > > There is (maybe) a small difference between disabled and 0% duty cycle, > > > at least from the framework's POV: If you do: > > > > > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > > > pwm_apply_state(pwm, { .enabled = false, .period = $DC, .duty_cycle = $DC, }); > > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > > > > > > and compare it to the expected result of > > > > > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 0, }); > > > pwm_apply_state(pwm, { .enabled = true, .period = 1000000, .duty_cycle = 1000000, }); > > > > > > the difference is that the duration of the inactive phase in the latter > > > case is a multiple of 1 ms. > > > > > > There is no policy for lowlevel drivers what to do, but disabling when > > > 0% is requested is at least not unseen and probably more what consumers > > > expect. > > > > > > > With the change I am proposing, the output will be driven to zero if enabled = false > > OR duty_cycle < 4000 ns. Stated another way: > > > > enable duty_cycle IQS620_PWR_SETTINGS[7] IQS620_PWM_DUTY_CYCLE > > ------ ---------- ---------------------- --------------------- > > 0 don't care 0 don't care > > 1 0 ... 3999 0 don't care > > 1 4000 ... x 1 0 > > 1 x+1 ... y 1 1 > > > > ...and so on. For context, if IQS620_PWR_SETTINGS[7] = 0 then the output is held to > > zero. If IQS620_PWR_SETTINGS[7] = 1 then the output toggles at a duty cycle between > > 0.4% and 100% as a function of IQS620_PWM_DUTY_CYCLE. > > Your table isn't accurate. IQS620_PWM_DUTY_CYCLE=0 results in a > duty_cycle of 3906.25 ns so the table should look as follows: > > enable duty_cycle IQS620_PWR_SETTINGS[7] IQS620_PWM_DUTY_CYCLE > ------ ------------ ---------------------- --------------------- > 0 don't care 0 don't care > 1 [0, 3906] 0 don't care > 1 [3907, 7812] 1 0 > 1 [7813,11718] 1 1 > > In general: > > dc = state->duty_cycle * 256 / 1000000 > if state->enabled == false or dc == 0: > IQS620_PWR_SETTINGS[7] = 0 > > else: > IQS620_PWM_DUTY_CYCLE = min(dc - 1, 0xff) > IQS620_PWR_SETTINGS[7] = 1 > Sure thing; will do. Thank you for catching that! > > Based on how the device behaves in response to its two available > > registers, I think your two examples will appear equal, but please let > > me know if I have understood. > > Yeah, that's the expectation. > > With the rounding as I suggested above this yields strange effects like > if > > .period = 1 s, .duty_cycle = 0.5 s > > is requested you end up in > > .period = 1 ms, .duty_cycle = 1 ms > > but I think there is nothing we can reasonably do about this. > Acknowledged on all counts. FWIW, I expect the most common consumer of this PWM to be leds-pwm. That is to say, I think the limitations in this case are pretty harmless. Users will typically pin the period to 1000000 ns like the example in patch [1/8]. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ | > Kind regards, Jeff LaBundy
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index e3a2518..712445e 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 + 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 26326ad..27c9bfa 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..6451eb1 --- /dev/null +++ b/drivers/pwm/pwm-iqs620a.c @@ -0,0 +1,167 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Azoteq IQS620A PWM Generator + * + * Copyright (C) 2019 + * Author: Jeff LaBundy <jeff@labundy.com> + */ + +#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; + bool ready; +}; + +static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct iqs620_pwm_private *iqs620_pwm; + struct iqs62x_core *iqs62x; + int error; + int duty_calc = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1; + u8 duty_clamp = clamp(duty_calc, 0, 0xFF); + + iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip); + iqs62x = iqs620_pwm->iqs62x; + + error = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE, duty_clamp); + if (error) + return error; + + state->period = IQS620_PWM_PERIOD_NS; + state->duty_cycle = (duty_clamp + 1) * IQS620_PWM_PERIOD_NS / 256; + + return regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS, + IQS620_PWR_SETTINGS_PWM_OUT, + state->enabled ? 0xFF : 0); +} + +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 error; + + iqs620_pwm = container_of(notifier, struct iqs620_pwm_private, + notifier); + + if (!iqs620_pwm->ready || !(event_flags & BIT(IQS62X_EVENT_SYS_RESET))) + return NOTIFY_DONE; + + pwm_get_state(&iqs620_pwm->chip.pwms[0], &state); + + error = iqs620_pwm_apply(&iqs620_pwm->chip, + &iqs620_pwm->chip.pwms[0], &state); + if (error) { + dev_err(iqs620_pwm->chip.dev, + "Failed to re-initialize device: %d\n", error); + return NOTIFY_BAD; + } + + return NOTIFY_OK; +} + +static void iqs620_pwm_notifier_unregister(void *context) +{ + struct iqs620_pwm_private *iqs620_pwm = context; + int error; + + error = blocking_notifier_chain_unregister(&iqs620_pwm->iqs62x->nh, + &iqs620_pwm->notifier); + if (error) + dev_err(iqs620_pwm->chip.dev, + "Failed to unregister notifier: %d\n", error); +} + +static const struct pwm_ops iqs620_pwm_ops = { + .apply = iqs620_pwm_apply, + .owner = THIS_MODULE, +}; + +static int iqs620_pwm_probe(struct platform_device *pdev) +{ + struct iqs620_pwm_private *iqs620_pwm; + int error; + + 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; + + iqs620_pwm->notifier.notifier_call = iqs620_pwm_notifier; + error = blocking_notifier_chain_register(&iqs620_pwm->iqs62x->nh, + &iqs620_pwm->notifier); + if (error) { + dev_err(&pdev->dev, "Failed to register notifier: %d\n", error); + return error; + } + + error = devm_add_action_or_reset(&pdev->dev, + iqs620_pwm_notifier_unregister, + iqs620_pwm); + if (error) { + dev_err(&pdev->dev, "Failed to add action: %d\n", error); + return error; + } + + error = pwmchip_add(&iqs620_pwm->chip); + if (error) { + dev_err(&pdev->dev, "Failed to add device: %d\n", error); + return error; + } + + iqs620_pwm->ready = true; + + return 0; +} + +static int iqs620_pwm_remove(struct platform_device *pdev) +{ + struct iqs620_pwm_private *iqs620_pwm = platform_get_drvdata(pdev); + int error; + + error = pwmchip_remove(&iqs620_pwm->chip); + if (error) + dev_err(&pdev->dev, "Failed to remove device: %d\n", error); + + return error; +} + +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> --- drivers/pwm/Kconfig | 10 +++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-iqs620a.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+) create mode 100644 drivers/pwm/pwm-iqs620a.c