* [PATCH v2] pwm: iqs620a: Correct a stale state variable
@ 2021-01-19 4:30 Jeff LaBundy
2021-01-22 18:12 ` Uwe Kleine-König
0 siblings, 1 reply; 4+ messages in thread
From: Jeff LaBundy @ 2021-01-19 4:30 UTC (permalink / raw)
To: thierry.reding; +Cc: u.kleine-koenig, lee.jones, linux-pwm, Jeff LaBundy
If duty cycle is first set to a value that is sufficiently high to
enable the output (e.g. 10000 ns) but then lowered to a value that
is quantized to zero (e.g. 1000 ns), the output is disabled as the
device cannot drive a constant zero (as expected).
However if the device is later re-initialized due to watchdog bite,
the output is re-enabled at the next-to-last duty cycle (10000 ns).
This is because the iqs620_pwm->out_en flag unconditionally tracks
state->enabled instead of what was actually written to the device.
To solve this problem, use one state variable that encodes all 257
states of the output (duty_scale) with 0 representing tri-state, 1
representing the minimum available duty cycle and 256 representing
100% duty cycle.
Signed-off-by: Jeff LaBundy <jeff@labundy.com>
---
NOTE: This patch is intended to be applied after the following patch:
http://patchwork.ozlabs.org/patch/1426818/
Changes in v2:
- Collapsed out_en and duty_val into a single state variable (duty_scale)
that is decoded from the newly added iqs620_pwm_init() function
drivers/pwm/pwm-iqs620a.c | 88 ++++++++++++++++++++---------------------------
1 file changed, 37 insertions(+), 51 deletions(-)
diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
index 14b18fb..957b972 100644
--- a/drivers/pwm/pwm-iqs620a.c
+++ b/drivers/pwm/pwm-iqs620a.c
@@ -37,15 +37,32 @@ struct iqs620_pwm_private {
struct pwm_chip chip;
struct notifier_block notifier;
struct mutex lock;
- bool out_en;
- u8 duty_val;
+ unsigned int duty_scale;
};
+static int iqs620_pwm_init(struct iqs620_pwm_private *iqs620_pwm,
+ unsigned int duty_scale)
+{
+ struct iqs62x_core *iqs62x = iqs620_pwm->iqs62x;
+ int ret;
+
+ if (!duty_scale)
+ return regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
+ IQS620_PWR_SETTINGS_PWM_OUT, 0);
+
+ ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE,
+ duty_scale - 1);
+ if (ret)
+ return ret;
+
+ return regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
+ IQS620_PWR_SETTINGS_PWM_OUT, 0xff);
+}
+
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;
unsigned int duty_cycle;
unsigned int duty_scale;
int ret;
@@ -57,7 +74,6 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return -EINVAL;
iqs620_pwm = container_of(chip, struct iqs620_pwm_private, chip);
- iqs62x = iqs620_pwm->iqs62x;
/*
* The duty cycle generated by the device is calculated as follows:
@@ -74,36 +90,15 @@ static int iqs620_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
duty_cycle = min_t(u64, state->duty_cycle, IQS620_PWM_PERIOD_NS);
duty_scale = duty_cycle * 256 / IQS620_PWM_PERIOD_NS;
- mutex_lock(&iqs620_pwm->lock);
-
- if (!state->enabled || !duty_scale) {
- ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
- IQS620_PWR_SETTINGS_PWM_OUT, 0);
- if (ret)
- goto err_mutex;
- }
-
- if (duty_scale) {
- u8 duty_val = duty_scale - 1;
+ if (!state->enabled)
+ duty_scale = 0;
- ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE,
- duty_val);
- if (ret)
- goto err_mutex;
-
- iqs620_pwm->duty_val = duty_val;
- }
-
- if (state->enabled && duty_scale) {
- ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
- IQS620_PWR_SETTINGS_PWM_OUT, 0xff);
- if (ret)
- goto err_mutex;
- }
+ mutex_lock(&iqs620_pwm->lock);
- iqs620_pwm->out_en = state->enabled;
+ ret = iqs620_pwm_init(iqs620_pwm, duty_scale);
+ if (!ret)
+ iqs620_pwm->duty_scale = duty_scale;
-err_mutex:
mutex_unlock(&iqs620_pwm->lock);
return ret;
@@ -121,12 +116,11 @@ static void iqs620_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
/*
* 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
+ * as disabled. This is not ideal, but is the best compromise based on
* the capabilities of the device.
*/
- state->enabled = iqs620_pwm->out_en;
- state->duty_cycle = DIV_ROUND_UP((iqs620_pwm->duty_val + 1) *
+ state->enabled = iqs620_pwm->duty_scale > 0;
+ state->duty_cycle = DIV_ROUND_UP(iqs620_pwm->duty_scale *
IQS620_PWM_PERIOD_NS, 256);
mutex_unlock(&iqs620_pwm->lock);
@@ -138,7 +132,6 @@ static int iqs620_pwm_notifier(struct notifier_block *notifier,
unsigned long event_flags, void *context)
{
struct iqs620_pwm_private *iqs620_pwm;
- struct iqs62x_core *iqs62x;
int ret;
if (!(event_flags & BIT(IQS62X_EVENT_SYS_RESET)))
@@ -146,7 +139,6 @@ static int iqs620_pwm_notifier(struct notifier_block *notifier,
iqs620_pwm = container_of(notifier, struct iqs620_pwm_private,
notifier);
- iqs62x = iqs620_pwm->iqs62x;
mutex_lock(&iqs620_pwm->lock);
@@ -155,16 +147,8 @@ static int iqs620_pwm_notifier(struct notifier_block *notifier,
* of a device reset, so nothing else is printed here unless there is
* an additional failure.
*/
- ret = regmap_write(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE,
- iqs620_pwm->duty_val);
- if (ret)
- goto err_mutex;
+ ret = iqs620_pwm_init(iqs620_pwm, iqs620_pwm->duty_scale);
- ret = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,
- IQS620_PWR_SETTINGS_PWM_OUT,
- iqs620_pwm->out_en ? 0xff : 0);
-
-err_mutex:
mutex_unlock(&iqs620_pwm->lock);
if (ret) {
@@ -211,12 +195,14 @@ static int iqs620_pwm_probe(struct platform_device *pdev)
ret = regmap_read(iqs62x->regmap, IQS620_PWR_SETTINGS, &val);
if (ret)
return ret;
- iqs620_pwm->out_en = val & IQS620_PWR_SETTINGS_PWM_OUT;
- ret = regmap_read(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE, &val);
- if (ret)
- return ret;
- iqs620_pwm->duty_val = val;
+ if (val & IQS620_PWR_SETTINGS_PWM_OUT) {
+ ret = regmap_read(iqs62x->regmap, IQS620_PWM_DUTY_CYCLE, &val);
+ if (ret)
+ return ret;
+
+ iqs620_pwm->duty_scale = val + 1;
+ }
iqs620_pwm->chip.dev = &pdev->dev;
iqs620_pwm->chip.ops = &iqs620_pwm_ops;
--
2.7.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] pwm: iqs620a: Correct a stale state variable
2021-01-19 4:30 [PATCH v2] pwm: iqs620a: Correct a stale state variable Jeff LaBundy
@ 2021-01-22 18:12 ` Uwe Kleine-König
2021-02-15 1:19 ` Jeff LaBundy
0 siblings, 1 reply; 4+ messages in thread
From: Uwe Kleine-König @ 2021-01-22 18:12 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: thierry.reding, lee.jones, linux-pwm
[-- Attachment #1: Type: text/plain, Size: 1263 bytes --]
Hello,
On Mon, Jan 18, 2021 at 10:30:29PM -0600, Jeff LaBundy wrote:
> If duty cycle is first set to a value that is sufficiently high to
> enable the output (e.g. 10000 ns) but then lowered to a value that
> is quantized to zero (e.g. 1000 ns), the output is disabled as the
> device cannot drive a constant zero (as expected).
>
> However if the device is later re-initialized due to watchdog bite,
> the output is re-enabled at the next-to-last duty cycle (10000 ns).
> This is because the iqs620_pwm->out_en flag unconditionally tracks
> state->enabled instead of what was actually written to the device.
>
> To solve this problem, use one state variable that encodes all 257
> states of the output (duty_scale) with 0 representing tri-state, 1
> representing the minimum available duty cycle and 256 representing
> 100% duty cycle.
>
> Signed-off-by: Jeff LaBundy <jeff@labundy.com>
Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 1 file changed, 37 insertions(+), 51 deletions(-)
Nice cleanup, thanks for picking up this idea.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] pwm: iqs620a: Correct a stale state variable
2021-01-22 18:12 ` Uwe Kleine-König
@ 2021-02-15 1:19 ` Jeff LaBundy
2021-02-26 9:47 ` Uwe Kleine-König
0 siblings, 1 reply; 4+ messages in thread
From: Jeff LaBundy @ 2021-02-15 1:19 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: thierry.reding, lee.jones, linux-pwm
Hi Thierry,
Do you have any objection to applying Uwe's patch [1] followed by
this one so that they can land in 5.12?
Hi Uwe,
On Fri, Jan 22, 2021 at 07:12:39PM +0100, Uwe Kleine-König wrote:
> Hello,
>
> On Mon, Jan 18, 2021 at 10:30:29PM -0600, Jeff LaBundy wrote:
> > If duty cycle is first set to a value that is sufficiently high to
> > enable the output (e.g. 10000 ns) but then lowered to a value that
> > is quantized to zero (e.g. 1000 ns), the output is disabled as the
> > device cannot drive a constant zero (as expected).
> >
> > However if the device is later re-initialized due to watchdog bite,
> > the output is re-enabled at the next-to-last duty cycle (10000 ns).
> > This is because the iqs620_pwm->out_en flag unconditionally tracks
> > state->enabled instead of what was actually written to the device.
> >
> > To solve this problem, use one state variable that encodes all 257
> > states of the output (duty_scale) with 0 representing tri-state, 1
> > representing the minimum available duty cycle and 256 representing
> > 100% duty cycle.
> >
> > Signed-off-by: Jeff LaBundy <jeff@labundy.com>
>
> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> > 1 file changed, 37 insertions(+), 51 deletions(-)
>
> Nice cleanup, thanks for picking up this idea.
Thank you for your kind words as well as your assistance throughout
this and other patches.
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | https://www.pengutronix.de/ |
Kind regards,
Jeff LaBundy
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] pwm: iqs620a: Correct a stale state variable
2021-02-15 1:19 ` Jeff LaBundy
@ 2021-02-26 9:47 ` Uwe Kleine-König
0 siblings, 0 replies; 4+ messages in thread
From: Uwe Kleine-König @ 2021-02-26 9:47 UTC (permalink / raw)
To: Jeff LaBundy; +Cc: thierry.reding, lee.jones, linux-pwm
[-- Attachment #1: Type: text/plain, Size: 421 bytes --]
Hello Jeff,
On Sun, Feb 14, 2021 at 07:19:27PM -0600, Jeff LaBundy wrote:
> Do you have any objection to applying Uwe's patch [1] followed by
> this one so that they can land in 5.12?
FTR: These two patches are in Linus' tree now.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-02-26 9:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-19 4:30 [PATCH v2] pwm: iqs620a: Correct a stale state variable Jeff LaBundy
2021-01-22 18:12 ` Uwe Kleine-König
2021-02-15 1:19 ` Jeff LaBundy
2021-02-26 9:47 ` Uwe Kleine-König
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.