[PATCH 3/3] pwm: imx27: fix disable state for inverted PWMs

[Marco Felsch <m.felsch@pengutronix.de>]

Up to now disabling the PWM is done using the PWMCR.EN register bit.
Setting this bit to zero results in the output pin driving a low value
independent of the polarity setting (PWMCR.POUTC).

There is only little documentation about expectations and requirements
in the PWM framework but the usual expectation seems to be that
disabling a PWM or setting .duty_cycle = 0 results in the output driving
the inactive level. The pwm-bl driver for example uses this setting to
disable the backlight and with the pwm-imx27 driver this results in an
enabled backlight if the pwm signal is inverted.

Keep the PWMCR.EN bit always enabled and simulate a disabled PWM using
duty_cycle = 0 to fix this. Furthermore we have to drop the sw-reset
from apply() else the PWMCR.EN is cleared too. Therefore the
pwm_imx27_wait_fifo_slot() is extended to guarantee a free FIFO slot.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/pwm/pwm-imx27.c | 69 ++++++++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 29 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 30388a9ece04..d98e8df14eb9 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -92,6 +92,7 @@ struct pwm_imx27_chip {
 	 */
 	unsigned int duty_cycle;
 	bool clk_on;
+	bool enabled;
 };
 
 #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
@@ -151,12 +152,9 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
 	if (ret < 0)
 		return;
 
-	val = readl(imx->mmio_base + MX3_PWMCR);
+	state->enabled = imx->enabled;
 
-	if (val & MX3_PWMCR_EN)
-		state->enabled = true;
-	else
-		state->enabled = false;
+	val = readl(imx->mmio_base + MX3_PWMCR);
 
 	switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
 	case MX3_PWMCR_POUTC_NORMAL:
@@ -179,8 +177,8 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
 	state->period = DIV_ROUND_UP_ULL(tmp, pwm_clk);
 
 	/*
-	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
-	 * use the cached value.
+	 * Use the cached value if the PWM is disabled since we are using the
+	 * PWMSAR to disable the PWM (see the notes in pwm_imx27_apply())
 	 */
 	if (state->enabled)
 		val = readl(imx->mmio_base + MX3_PWMSAR);
@@ -209,8 +207,8 @@ static void pwm_imx27_sw_reset(struct pwm_imx27_chip *imx, struct device *dev)
 		dev_warn(dev, "software reset timeout\n");
 }
 
-static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
-				     struct pwm_device *pwm)
+static int pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
+				    struct pwm_device *pwm)
 {
 	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
 	struct device *dev = chip->dev;
@@ -226,9 +224,13 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
 		msleep(period_ms);
 
 		sr = readl(imx->mmio_base + MX3_PWMSR);
-		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr))
+		if (fifoav == FIELD_GET(MX3_PWMSR_FIFOAV, sr)) {
 			dev_warn(dev, "there is no free FIFO slot\n");
+			return -EBUSY;
+		}
 	}
+
+	return 0;
 }
 
 static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -270,17 +272,25 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	else
 		period_cycles = 0;
 
+	/* Wait for a free FIFO slot */
+	ret = pwm_imx27_wait_fifo_slot(chip, pwm);
+	if (ret)
+		goto out;
+
 	/*
-	 * Wait for a free FIFO slot if the PWM is already enabled, and flush
-	 * the FIFO if the PWM was disabled and is about to be enabled.
+	 * We can't use the enable bit to control the en-/disable squence
+	 * correctly because the output pin is pulled low if setting this bit
+	 * to '0' regardless of the poutc value. Instead we have to use the
+	 * sample register. According the RM:
+	 * A value of zero in the sample register will result in the PWMO output
+	 * signal always being low/high (POUTC = 00 it will be low and
+	 * POUTC = 01 it will be high), and no output waveform will be produced.
+	 * If the value in this register is higher than the PERIOD
 	 */
-	if (cstate.enabled) {
-		pwm_imx27_wait_fifo_slot(chip, pwm);
-	} else {
-		pwm_imx27_sw_reset(imx, chip->dev);
-	}
-
-	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	if (state->enabled)
+		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
+	else
+		writel(0, imx->mmio_base + MX3_PWMSAR);
 	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
 
 	/*
@@ -288,24 +298,21 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * MX3_PWMSAR register can't be read (i.e. when the PWM is disabled).
 	 */
 	imx->duty_cycle = duty_cycles;
+	imx->enabled = state->enabled;
 
 	cr = MX3_PWMCR_PRESCALER_SET(prescale);
-
 	if (state->polarity == PWM_POLARITY_INVERSED)
-		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
-				MX3_PWMCR_POUTC_INVERTED);
+		cr |= FIELD_PREP(MX3_PWMCR_POUTC, MX3_PWMCR_POUTC_INVERTED);
 
-	if (state->enabled)
-		cr |= MX3_PWMCR_EN;
-
-	mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC | MX3_PWMCR_EN;
+	mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC;
 
 	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, cr);
 
+out:
 	if (!state->enabled)
 		pwm_imx27_clk_disable_unprepare(imx);
 
-	return 0;
+	return ret;
 }
 
 static const struct pwm_ops pwm_imx27_ops = {
@@ -378,12 +385,16 @@ static int pwm_imx27_probe(struct platform_device *pdev)
 
 		pwm_imx27_sw_reset(imx, &pdev->dev);
 		mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
-			MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC;
+			MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC | MX3_PWMCR_POUTC | MX3_PWMCR_EN;
 		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
 			MX3_PWMCR_DBGEN |
-			FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
+			FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
+			FIELD_PREP(MX3_PWMCR_POUTC, MX3_PWMCR_POUTC_OFF) |
+			MX3_PWMCR_EN;
 		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
 		pwm_imx27_clk_disable_unprepare(imx);
+	} else {
+		imx->enabled = true;
 	}
 
 	return pwmchip_add(&imx->chip);
-- 
2.20.1