pwm: iqs620a: Correct a stale state variable

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>
Signed-off-by: Thierry Reding <thierry.reding@gmail.com>

diff --git a/drivers/pwm/pwm-iqs620a.c b/drivers/pwm/pwm-iqs620a.c
index 14b18fb4f5274667bfe67a20205bb65ac5523480..957b972c458bcefb687cfd96239bfa32d0cbbbf4 100644 (file)
--- 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;