[PATCH v2 4/5] pwm: imx27: fix disable state for inverted PWMs
Uwe Kleine-König
u.kleine-koenig at pengutronix.de
Mon Sep 28 03:47:36 EDT 2020
Hello,
On Fri, Sep 25, 2020 at 05:53:29PM +0200, Marco Felsch wrote:
> 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 together with 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.
This sounds as if the pwm-imx27 behaviour is a reason to believe that
.duty_cycle = 0 + .enabled = false should drive the inactive level.
I'd write:
The pwm-bl driver for example uses this setting to disable the
backlight. Up to now however, this request makes the pwm-imx27
enable the 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() otherwise the PWMCR.EN is cleared too. Therefore the
> pwm_imx27_wait_fifo_slot() is extended and renamed to guarantee a free
> FIFO slot and to reflect the new meaning.
>
> Signed-off-by: Marco Felsch <m.felsch at pengutronix.de>
> ---
> v2:
> - fix driver remove function
> - rename pwm_imx27_wait_fifo_slot
> - pwm_imx27_get_fifo_slot now returns the number of used fifo slots
> rather than 0 on success (needed for next patch).
>
> drivers/pwm/pwm-imx27.c | 78 ++++++++++++++++++++++++++---------------
> 1 file changed, 49 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 3b6bcd8d58b7..07c6a263a39c 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -141,12 +141,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);
I'm not a big fan. IMHO the driver should report about reality and the
framework (and maybe the consumers) should be able to handle that
.get_state() reports
.enabled = true
.duty_cycle = 0
after
.enabled = false
was requested.
> switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> case MX3_PWMCR_POUTC_NORMAL:
> @@ -169,8 +166,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);
> @@ -199,8 +196,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_get_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;
> @@ -216,9 +213,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 fifoav;
> }
>
> static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> @@ -257,16 +258,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_get_fifo_slot(chip, pwm);
> + if (ret < 0)
> + 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:
According to the reference manual:
> + * 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
Did you forget to insert the end of this sentence here?
> */
> - if (imx->enabled)
> - pwm_imx27_wait_fifo_slot(chip, pwm);
> + if (state->enabled)
> + writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> else
> - pwm_imx27_sw_reset(imx, chip->dev);
> -
> - writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> + writel(0, imx->mmio_base + MX3_PWMSAR);
> writel(period_cycles, imx->mmio_base + MX3_PWMPR);
I think you can simplify the code a bit using the following idiom:
/*
* comment as above
*/
if (!state->enabled)
duty_cycle = 0;
writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
With the change from the next patch I could also imagine to write a
smaller period in the !enabled case. The upside would be that the second
call in:
pwm_apply(mypwm, { .enabled = false, .period = 3s });
pwm_apply(mypwm, { .enabled = true, ... });
wouldn't take longer than a second in the average case.
@Thierry, we really need to agree on the expected behaviour in these
cases and document them.
> /*
> @@ -276,15 +286,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> imx->duty_cycle = duty_cycles;
>
> cr = MX3_PWMCR_PRESCALER_SET(prescale);
> -
> if (state->polarity == PWM_POLARITY_INVERSED)
> - cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> - MX3_PWMCR_POUTC_INVERTED);
> -
> - if (state->enabled)
> - cr |= MX3_PWMCR_EN;
> + cr |= FIELD_PREP(MX3_PWMCR_POUTC, MX3_PWMCR_POUTC_INVERTED);
>
> - 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);
>
> @@ -373,10 +378,13 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> if (!(pwmcr & MX3_PWMCR_EN)) {
> 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 {
> @@ -385,6 +393,7 @@ static int pwm_imx27_probe(struct platform_device *pdev)
> pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> MX3_PWMCR_DBGEN;
> pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> + imx->enabled = true;
> }
>
> return pwmchip_add(&imx->chip);
> @@ -392,11 +401,22 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>
> static int pwm_imx27_remove(struct platform_device *pdev)
> {
> - struct pwm_imx27_chip *imx;
> + struct pwm_imx27_chip *imx = platform_get_drvdata(pdev);
> + int ret;
>
> - imx = platform_get_drvdata(pdev);
> + ret = pwm_imx27_clk_prepare_enable(imx);
> + if (ret)
> + return ret;
>
> - return pwmchip_remove(&imx->chip);
> + ret = pwmchip_remove(&imx->chip);
> + if (ret)
> + return ret;
> +
> + /* Ensure module is disabled after remove */
> + pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, MX3_PWMCR_EN, 0);
> + pwm_imx27_clk_disable_unprepare(imx);
This is wrong. You are supposed to assume the PWM is already off in
.remove and don't touch it.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20200928/70d8f56f/attachment.sig>
More information about the linux-arm-kernel
mailing list