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

[Jeff LaBundy <jeff@labundy.com>]

Hi Uwe,

On Fri, Dec 20, 2019 at 09:59:48AM +0100, Uwe Kleine-König wrote:
> Hello Jeff,
> 
> On Fri, Dec 20, 2019 at 03:19:31AM +0000, Jeff LaBundy wrote:
> > I apologize for my delayed replies as I have been traveling.
> 
> No problem, I didn't hold my breath :-)
> 
> > On Mon, Dec 16, 2019 at 10:19:12AM +0100, Uwe Kleine-König wrote:
> > > On Sun, Dec 15, 2019 at 08:36:12PM +0000, Jeff LaBundy wrote:
> > > > On Tue, Dec 10, 2019 at 08:22:27AM +0100, Uwe Kleine-König wrote:
> > > > > On Tue, Dec 10, 2019 at 12:03:02AM +0000, Jeff LaBundy wrote:
> > > > > > On Mon, Dec 09, 2019 at 08:32:06AM +0100, Uwe Kleine-König wrote:
> > > > > > > On Mon, Dec 09, 2019 at 12:38:36AM +0000, Jeff LaBundy wrote:
> > > > > > > > This patch adds support for the Azoteq IQS620A, capable of generating
> > > > > > > > a 1-kHz PWM output with duty cycle between 0.4% and 100% (inclusive).
> > > > > > > > 
> > > > > > > > Signed-off-by: Jeff LaBundy <jeff@labundy.com>
> > > > > > > > ---
> > > > > > > > Changes in v2:
> > > > > > > >   - Merged 'Copyright' and 'Author' lines into one in introductory comments
> > > > > > > >   - Added 'Limitations' section to introductory comments
> > > > > > > >   - Replaced 'error' with 'ret' throughout
> > > > > > > >   - Added const qualifier to state argument of iqs620_pwm_apply and removed all
> > > > > > > >     modifications to the variable's contents
> > > > > > > >   - Updated iqs620_pwm_apply to return -ENOTSUPP or -EINVAL if the requested
> > > > > > > >     polarity is inverted or the requested period is below 1 ms, respectively
> > > > > > > >   - Updated iqs620_pwm_apply to disable the PWM output if duty cycle is zero
> > > > > > > >   - Added iqs620_pwm_get_state
> > > > > > > >   - Eliminated tabbed alignment of pwm_ops and platform_driver struct members
> > > > > > > >   - Moved notifier unregistration to already present iqs620_pwm_remove, which
> > > > > > > >     eliminated the need for a device-managed action and ready flag
> > > > > > > >   - Added a comment in iqs620_pwm_probe to explain the order of operations
> > > > > > > >   - Changed Kconfig "depends on" logic to MFD_IQS62X || COMPILE_TEST
> > > > > > > > 
> > > > > > > >  drivers/pwm/Kconfig       |  10 +++
> > > > > > > >  drivers/pwm/Makefile      |   1 +
> > > > > > > >  drivers/pwm/pwm-iqs620a.c | 206 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > > > >  3 files changed, 217 insertions(+)
> > > > > > > >  create mode 100644 drivers/pwm/pwm-iqs620a.c
> > > > > > > > 
> > > > > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> > > > > > > > index bd21655..60bcf6c 100644
> > > > > > > > --- a/drivers/pwm/Kconfig
> > > > > > > > +++ b/drivers/pwm/Kconfig
> > > > > > > > @@ -222,6 +222,16 @@ config PWM_IMX_TPM
> > > > > > > >  	  To compile this driver as a module, choose M here: the module
> > > > > > > >  	  will be called pwm-imx-tpm.
> > > > > > > > 
> > > > > > > > +config PWM_IQS620A
> > > > > > > > +	tristate "Azoteq IQS620A PWM support"
> > > > > > > > +	depends on MFD_IQS62X || COMPILE_TEST
> > > > > > > > +	help
> > > > > > > > +	  Generic PWM framework driver for the Azoteq IQS620A multi-function
> > > > > > > > +	  sensor.
> > > > > > > > +
> > > > > > > > +	  To compile this driver as a module, choose M here: the module will
> > > > > > > > +	  be called pwm-iqs620a.
> > > > > > > > +
> > > > > > > >  config PWM_JZ4740
> > > > > > > >  	tristate "Ingenic JZ47xx PWM support"
> > > > > > > >  	depends on MACH_INGENIC
> > > > > > > > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> > > > > > > > index 9a47507..a59c710 100644
> > > > > > > > --- a/drivers/pwm/Makefile
> > > > > > > > +++ b/drivers/pwm/Makefile
> > > > > > > > @@ -20,6 +20,7 @@ obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
> > > > > > > >  obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
> > > > > > > >  obj-$(CONFIG_PWM_IMX27)		+= pwm-imx27.o
> > > > > > > >  obj-$(CONFIG_PWM_IMX_TPM)	+= pwm-imx-tpm.o
> > > > > > > > +obj-$(CONFIG_PWM_IQS620A)	+= pwm-iqs620a.o
> > > > > > > >  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
> > > > > > > >  obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
> > > > > > > >  obj-$(CONFIG_PWM_LPC18XX_SCT)	+= pwm-lpc18xx-sct.o
> > > > > > > > diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
> > > > > > > > new file mode 100644
> > > > > > > > index 0000000..1ea11b9
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/pwm/pwm-iqs620a.c
> > > > > > > > @@ -0,0 +1,206 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0+
> > > > > > > > +/*
> > > > > > > > + * Azoteq IQS620A PWM Generator
> > > > > > > > + *
> > > > > > > > + * Copyright (C) 2019 Jeff LaBundy <jeff@labundy.com>
> > > > > > > > + *
> > > > > > > > + * Limitations:
> > > > > > > > + * - The period is not guaranteed to run to completion when the duty cycle is
> > > > > > > > + *   changed or the output is disabled.
> > > > > > > 
> > > > > > > Do you know more details here? "not guaranteed" means that the new
> > > > > > > period starts immediately when duty_cycle or the enabled bit is written?
> > > > > > > 
> > > > > > 
> > > > > > Increasing the duty cycle on-the-fly (e.g. 25% to 75%) results in the
> > > > > > following behavior (depending on where the I2C write falls):
> > > > > > 
> > > > > >                        I2C write
> > > > > >    __        __        __  V_    ______    ______    ______    __
> > > > > > __|  |______|  |______|  |_|x|__|      |__|      |__|      |__|
> > > > > >   ^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^---1ms---^
> > > > > > 
> > > > > > The PWM continues to tick at 1 ms, but the currently running period suffers
> > > > > > an extraneous pulse as the output is abruptly set high to "catch up" to the
> > > > > > new duty cycle.
> > > > > > 
> > > > > > A similar behavior can occur if the duty cycle is decreased, meaning the
> > > > > > output is abruptly set low if the I2C transaction completes in what has
> > > > > > suddenly become the inactive region of the currently running period.
> > > > > > 
> > > > > > The PWM seems to be a simple counter that rolls over at a period of 1 ms.
> > > > > > Both the counter and the IQS620_PWM_DUTY_CYCLE register effectively go to
> > > > > > a comparator whose output is ANDed with IQS620_PWR_SETTINGS_PWM_OUT which
> > > > > > then drives the PWM output.
> > > > > > 
> > > > > > As such, if either IQS620_PWM_DUTY_CYCLE or IQS620_PWR_SETTINGS_PWM_OUT
> > > > > > change, so may the PWM output state depending on the counter's value at
> > > > > > the time the I2C write is completed within the 1-ms continuous loop.
> > > > > > 
> > > > > > For v3 I will update the note as follows:
> > > > > > 
> > > > > > - Changes in duty cycle or enable/disable state are immediately reflected
> > > > > >   by the PWM output and are not aligned to the start of any period.
> > > > > 
> > > > > I'd like to see a bit more information in the driver. Something about
> > > > > the 1ms rhythm being unaffected by the duty_cycle and enable setting.
> > > > > Maybe:
> > > > > 
> > > > >  - The periods run continuously with a fixed length of 1 ms which is
> > > > >    unaffected by register updates. Writing duty cycle or enable
> > > > >    registers gets active immediately which might result in glitches.
> > > > > 
> > > > > ?
> > > > > 
> > > > 
> > > > I adjusted the wording a bit as per my preference and settled on the
> > > > following:
> > > > 
> > > >   - The period is fixed to 1 ms and is generated continuously despite changes
> > > >     to the duty cycle or enable/disable state.
> > > >   - Changes to the duty cycle or enable/disable state take effect immediately
> > > >     and may result in a glitch during the period in which the change is made.
> > > > 
> > > > I believe these capture the spirit of your message; please let me know if
> > > > you have any concerns.
> > > 
> > > That's fine.
> > > 
> > > > Upon further experimentation, I found that disabling the output (which v2
> > > > does so as to simulate a 0% duty cycle) does not actively drive zero, but
> > > > rather places the output in a high-impedance state with only the device's
> > > > own internal leakage eventually discharging the pin.
> > > 
> > > But maybe this is still the best you can do in this case. @Thierry, what
> > > do you think?
> > > 
> > > > This is fundamentally different than actively driving the pin low to make
> > > > a 0% duty cycle, which does not appear to be possible at all. Therefore I
> > > > have removed the control of IQS620_PWR_SETTINGS_PWM_OUT based on the duty
> > > > cycle requested by the user and reverted to the behavior of v1, where the
> > > > duty cycle requested by the user is mapped only to IQS620_PWM_DUTY_CYCLE.
> > > > 
> > > > As such, I have also added a third bullet point similar to what you first
> > > > suggested following v1:
> > > > 
> > > >   - The device cannot generate a 0% duty cycle.
> > > 
> > > Then this would be:
> > > 
> > >   - The device cannot actively drive a 0% duty cycle. The driver is
> > >     disabled for small duty_cycles relying on a pull down on the board.
> > > 
> > > But maybe it would be more prudent to do this only if the board
> > > configuration suggests this is save?!
> > > 
> > 
> > Given the policy for the actual duty cycle generated by the hardware not to
> > exceed that which is requested by the user, it seems there are ultimately 3
> > options for duty_cycle below 1 / 256 ms:
> > 
> > 1. Return -EINVAL.
> > 2. Disable the output as in v2.
> > 3. Add an optional boolean in the dts that identifies whether a pull-down is
> >    present; default to option (1) but use option (2) if the boolean is there.
> > 
> > I don't like option (1) because consumers (including leds-pwm) do in fact ask
> > for a 0% duty cycle which would make iqs620_pwm_apply return an error. Things
> > happen to still work since leds-pwm does not check pwm_config's return value,
> > but I don't want to rely on this coincidence.
> 
> People implementing PWM drivers seems to mostly care about leds-pwm :-)
> With that only nearly hitting the requested state isn't that bad. But if
> you control a step motor that positions a laser, you want to be sure
> that the request to stop moving it actually worked.
> 
> > Option (2) is better, but I know from experience that board designers do not
> > consult driver comments and the requirement to add a pull-down may be easily
> > missed as it is not discussed in the data sheet (which is where that sort of
> > information belongs, in my opinion).
> 
> Hmm, well, actually I think the problem happened earlier when the
> hardware designer considered providing 0% to be not important.
> 

I heard back from the vendor today; they've acknowledged the limitation and
are considering adding support for 0% in a future ROM spin. In the meantime,
they've agreed to describe the high-impedance behavior in the data sheet as
well as include the pull-down resistor in an example schematic.

> > Option (3) seems like overkill for such a simple PWM, and ultimately doesn't
> > add any value because I don't want to allow option (1) behavior in any case.
> > Whether the PWM is disabled because it is truly disabled or to simulate a 0%
> > duty cycle as in option (2), the pull-down is ultimately required regardless
> > of whether or not the data sheet happens to go into such detail.
> 
> Actually I like option 3 best.
>  

Based on your other feedback, I'm moving forward under the impression that
you'll still accept option (2); please let me know if I have misunderstood
(thank you for being flexible).

My argument is that even if duty cycle is limited to 1 / 256 ms within the
"no pull-down present" option, the output will still be disabled anyway if
state->enabled = false. In that case, the pull-down is required to prevent
noise from coupling onto the high-impedance pin (which will likely be tied
to the high-impedance gate of a MOSFET) and flickering an LED.

Stated another way, I do not feel option (3) is suitable because the pull-
down is in fact not optional, but required in my opinion.

> > Therefore I have opted to carry forward option (2) from v2 to v3. I reworded
> > the third limitation a bit as follows:
> > 
> > - The device cannot generate a 0% duty cycle. For duty cycles below 1 / 256
> >   ms, the output is disabled and relies upon an external pull-down resistor
> >   to hold the GPIO3/LTX pin low.
> > 
> > I did reach out to the vendor and asked them to consider recommending a pull-
> > down resistor in a future revision of the data sheet, although at the time of
> > this writing I have not heard back.
> 
> Good.
> 
> > > > 	/*
> > > > 	 * The duty cycle generated by the device is calculated as follows:
> > > > 	 *
> > > > 	 * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms
> > > > 	 *
> > > > 	 * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255
> > > > 	 * (inclusive). Therefore the lowest duty cycle the device can generate
> > > > 	 * while the output is enabled is 1 / 256 ms.
> > > > 	 */
> > > > 	duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS - 1;
> > > 
> > > Hmm, this is violating the policy to implement a value not bigger than
> > > requested with state->duty_cycle == 0. I see this has downsides to not
> > > simply cheat here, but only claiming to have implemented 0% can hurt,
> > > too. pwm-rcar returns -EINVAL in this case.
> > > 
> > 
> > That's a great point and is addressed by sticking with option (2) described
> > above. Here is what I've got for v3:
> > 
> > static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > 			    const struct pwm_state *state)
> > {
> > 	struct iqs620_pwm_private *iqs620_pwm;
> > 	struct iqs62x_core *iqs62x;
> > 	int duty_scale, ret;
> > 
> > 	if (state->polarity != PWM_POLARITY_NORMAL)
> > 		return -ENOTSUPP;
> > 
> > 	if (state->period < IQS620_PWM_PERIOD_NS)
> > 		return -EINVAL;
> > 
> > 	iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip);
> > 	iqs62x = iqs620_pwm->iqs62x;
> > 
> > 	mutex_lock(&iqs620_pwm->lock);
> > 
> > 	/*
> > 	 * The duty cycle generated by the device is calculated as follows:
> > 	 *
> > 	 * duty_cycle = (IQS620_PWM_DUTY_CYCLE + 1) / 256 * 1 ms
> > 	 *
> > 	 * ...where IQS620_PWM_DUTY_CYCLE is a register value between 0 and 255
> > 	 * (inclusive). Therefore the lowest duty cycle the device can generate
> > 	 * while the output is enabled is 1 / 256 ms.
> > 	 *
> > 	 * For lower duty cycles (e.g. 0), the PWM output is simply disabled to
> > 	 * allow an on-board pull-down resistor to hold the GPIO3/LTX pin low.
> > 	 */
> > 	duty_scale = state->duty_cycle * 256 / IQS620_PWM_PERIOD_NS;
> > 
> > 	if (!state->enabled || !duty_scale) {
> > 		ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS,
> > 					 IQS620_PWR_SETTINGS_PWM_OUT, 0);
> > 		if (ret)
> > 			goto err_mutex;
> > 	}
> > 
> > 	if (duty_scale) {
> > 		ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE,
> > 				   min(duty_scale - 1, 0xFF));
> > 		if (ret)
> > 			goto err_mutex;
> > 	}
> > 
> > 	if (state->enabled && duty_scale)
> > 		ret = regmap_update_bits(iqs62x->map, IQS620_PWR_SETTINGS,
> > 					 IQS620_PWR_SETTINGS_PWM_OUT, 0xFF);
> > 
> > err_mutex:
> > 	mutex_unlock(&iqs620_pwm->lock);
> > 
> > 	return ret;
> > }
> 
> Looks ok. (Even though it implements (2) which isn't my favorite :-)
> 
> > And for the get_state callback:
> > 
> > static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> > 				 struct pwm_state *state)
> > {
> > 	struct iqs620_pwm_private *iqs620_pwm;
> > 	struct iqs62x_core *iqs62x;
> > 	unsigned int val;
> > 	int ret;
> > 
> > 	iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip);
> > 	iqs62x = iqs620_pwm->iqs62x;
> > 
> > 	mutex_lock(&iqs620_pwm->lock);
> > 
> > 	ret = regmap_read(iqs62x->map, IQS620_PWR_SETTINGS, &val);
> > 	if (ret)
> > 		goto err_mutex;
> > 	state->enabled = val & IQS620_PWR_SETTINGS_PWM_OUT;
> > 
> > 	ret = regmap_read(iqs62x->map, IQS620_PWM_DUTY_CYCLE, &val);
> > 	if (ret)
> > 		goto err_mutex;
> > 	state->duty_cycle = DIV_ROUND_UP((val + 1) * IQS620_PWM_PERIOD_NS, 256);
> > 	state->period = IQS620_PWM_PERIOD_NS;
> > 
> > err_mutex:
> > 	mutex_unlock(&iqs620_pwm->lock);
> > 
> > 	if (ret)
> > 		dev_err(iqs620_pwm->chip.dev, "Failed to get state: %d\n", ret);
> > }
> > 
> > If you and/or Thierry have any concerns, please let me know.
> 
> Looks good, too. Maybe add a comment like:
> 
> 	/*
> 	 * As the hardware cannot implement "enabled with
> 	 * duty_cycle == 0", we're reporting "disabled with
> 	 * duty_cycle = 1/256 ms" after 0% was requested. This is ugly
> 	 * but the best we can achieve.
> 	 */
> 

Sure thing, will do.

> > > > 	ret = regmap_write(iqs62x->map, IQS620_PWM_DUTY_CYCLE,
> > > > 			   clamp(duty_scale, 0, 0xFF));
> > > > 	if (ret)
> > > > 		return ret;
> > > 
> > > I understand your motivation to configure the duty cycle also when the
> > > the request has enabled=false, but a strange side effect is that a
> > > failure to configure the dutycycle with .enabled=false isn't really a
> > > problem, is it?
> > > (This is not a request to change anything, it's only expression of my
> > > frustration that we cannot get away without strange effects.)
> > > 
> > 
> > True, but it would definitely be a problem in case 01ccf903edd6 returns and
> > we're relying on the device's own registers to hold the PWM state.
> 
> You can assume it won't come back as is. There are too many drivers that
> suffer the same problem. My goal is to let the core not depend on the
> lowlevel drivers to memorize a duty-cycle for disabled PWMs. The details
> are not yet thought out. Obvious options are:
> 
>  - cache the value in the core
>  - make consumers not depend on that
> 
> > Thinking about this more, I agree with your earlier comment that a means to
> > get the actual (quantized) state needs to be a new API function (of integer
> > type). Since chip->ops->get_state is void, there is no way for the callback
> > to warn the core that a register read failed and the PWM state may be junk.
> 
> Yeah, this is something I intend to change, too. .get_state should
> return a status code in the long run.
> 

Makes sense.

> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

I'll add a comment in get_state and send out a v3 after the holidays.

Kind regards,
Jeff LaBundy