Re: [PATCH v3 4/7] pwm: Add support for Azoteq IQS620A PWM generator

[Jeff LaBundy <jeff@labundy.com>]

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