Commit 28208c7b authored by Jeff LaBundy's avatar Jeff LaBundy Committed by Thierry Reding
Browse files

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: default avatarJeff LaBundy <jeff@labundy.com>
Reviewed-by: default avatarUwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: default avatarThierry Reding <thierry.reding@gmail.com>
parent 72d6b245

drivers/pwm/pwm-iqs620a.c

+37 −51
Original line number Diff line number Diff line
@@ -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 = 0;



	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;



		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 = regmap_update_bits(iqs62x->regmap, IQS620_PWR_SETTINGS,

				 IQS620_PWR_SETTINGS_PWM_OUT,

				 iqs620_pwm->out_en ? 0xff : 0);

	ret = iqs620_pwm_init(iqs620_pwm, iqs620_pwm->duty_scale);



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;



	if (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;



		iqs620_pwm->duty_scale = val + 1;

	}



	iqs620_pwm->chip.dev = &pdev->dev;

	iqs620_pwm->chip.ops = &iqs620_pwm_ops;