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@xxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxx>
> + *
> + * 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
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
