Re: [PATCH 5/8] pwm: Add support for Azoteq IQS620A PWM generator
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 = 500,
> .duty_cycle = 100,
>
> the hardware is actually configured for
>
> .period = 100,
> .duty_cycle = 100,
>
> . 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 = 100, .duty_cycle
> = 100, });
> pwm_apply_state(pwm, { .enabled = false, .period = $DC, .duty_cycle =
> $DC, });
> pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
> = 100, });
>
> and compare it to the expected result of
>
> pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
> = 100, });
> pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
> = 0, });
> pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
> = 100, });
>
> 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
Re: [PATCH 5/8] pwm: Add support for Azoteq IQS620A PWM generator
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 = 500,
.duty_cycle = 100,
the hardware is actually configured for
.period = 100,
.duty_cycle = 100,
. 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 = 100, .duty_cycle
= 100, });
pwm_apply_state(pwm, { .enabled = false, .period = $DC, .duty_cycle =
$DC, });
pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
= 100, });
and compare it to the expected result of
pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
= 100, });
pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
= 0, });
pwm_apply_state(pwm, { .enabled = true, .period = 100, .duty_cycle
= 100, });
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 = 100, .enabled
> > = true)
> > ...
> > pwm_apply_state(pwm, { .duty_cycle = 100, .period = 100,
> > .enabled = false)
> >
> > the output is active for longer than 4 µs because the iqs620_pwm_apply
> > function is
Re: [PATCH 5/8] pwm: Add support for Azoteq IQS620A PWM generator
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
> > ---
> > 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 000..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
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define IQS620_PWR_SETTINGS0xD2
> > +#define IQS620_PWR_SETTINGS_PWM_OUTBIT(7)
> > +
> > +#define IQS620_PWM_DUTY_CYCLE 0xD8
> > +
> > +#define IQS620_PWM_PERIOD_NS 100
> > +
> > +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
