[PATCH v2] pwm: imx27: fix race condition .apply,.get_state

[PATCH v2] pwm: imx27: fix race condition .apply,.get_state

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>

With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR
register in .apply(), the register is read in .get_state().
Unless a period completed in the meantime, this read yields the
previously used duty cycle configuration. As the PWM_DEBUG code
applies the read out configuration for testing purposes this
effectively undoes the intended effect by rewriting the previous
hardware state.

Note that this change merely implements a sensible heuristic.
The i.MX has a 4 slot FIFO to configure the duty cycle. This FIFO
cannot be read back in its entirety. The "write x then read back
x from hw" semantics are therefore not easily applicable.
With this change, the .get_state() function tries to wait for some
stabilization in the FIFO (empty state). In this state it keeps
applying the last value written to the sample register.

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
---
 drivers/pwm/pwm-imx27.c | 50 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 29a3089c534c..32389ca2da3e 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -75,6 +75,7 @@
 						   (x)) + 1)
 
 #define MX3_PWM_SWR_LOOP		5
+#define MX3_PWM_FIFOAV_EMPTY_LOOP	4
 
 /* PWMPR register value of 0xffff has the same effect as 0xfffe */
 #define MX3_PWMPR_MAX			0xfffe
@@ -118,8 +119,28 @@ static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
 	clk_disable_unprepare(imx->clk_ipg);
 }
 
+static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
+				     struct pwm_device *pwm)
+{
+	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
+	struct device *dev = chip->dev;
+	unsigned int period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm), NSEC_PER_MSEC);
+	int tries = MX3_PWM_FIFOAV_EMPTY_LOOP;
+	int fifoav;
+	u32 sr;
+
+	while (tries--) {
+		sr = readl(imx->mmio_base + MX3_PWMSR);
+		fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
+		if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
+			return;
+		msleep(period_ms);
+	}
+	dev_warn(dev, "FIFO has been refilled concurrently\n");
+}
+
 static int pwm_imx27_get_state(struct pwm_chip *chip,
-			       struct pwm_device *pwm, struct pwm_state *state)
+				struct pwm_device *pwm, struct pwm_state *state)
 {
 	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
 	u32 period, prescaler, pwm_clk, val;
@@ -161,10 +182,33 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
 	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
 	 * use the cached value.
 	 */
-	if (state->enabled)
+	if (state->enabled) {
+		/*
+		 * From the i.MX PWM reference manual:
+		 * "A read on the sample register yields the current FIFO value that
+		 *  is being used, or will be used, by the PWM for generation on the
+		 *  output signal. Therefore, a write and a subsequent read on the
+		 *  sample register may result in different values being obtained."
+		 * Furthermore:
+		 * "When a new value is written, the duty cycle changes after the
+		 *  current period is over."
+		 * Note "changes" vs. "changes to the given value"!
+		 * Finally:
+		 * "The PWM will run at the last set duty-cycle setting if all the
+		 *  values of the FIFO has been utilized, until the FIFO is reloaded
+		 *  or the PWM is disabled."
+		 * Try to be at least a bit more deterministic about which value is
+		 * read by waiting until the FIFO is empty. In this state the last/most
+		 * recently pushed sample (duty cycle) value is continuously applied.
+		 * Beware that this approach is still racy, as a new value could have
+		 * been supplied and a period expired between the call of the wait
+		 * function and the subsequent readl.
+		 */
+		pwm_imx27_wait_fifo_empty(chip, pwm);
 		val = readl(imx->mmio_base + MX3_PWMSAR);
-	else
+	} else {
 		val = imx->duty_cycle;
+	}
 
 	tmp = NSEC_PER_SEC * (u64)(val) * prescaler;
 	state->duty_cycle = DIV_ROUND_UP_ULL(tmp, pwm_clk);
-- 
2.39.1

Re: [PATCH v2] pwm: imx27: fix race condition .apply,.get_state

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4892 bytes --]

On Thu, Jan 26, 2023 at 04:03:13PM +0100, Leif Middelschulte wrote:
> From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
> 
> With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR
> register in .apply(), the register is read in .get_state().
> Unless a period completed in the meantime, this read yields the
> previously used duty cycle configuration. As the PWM_DEBUG code
> applies the read out configuration for testing purposes this
> effectively undoes the intended effect by rewriting the previous
> hardware state.
> 
> Note that this change merely implements a sensible heuristic.
> The i.MX has a 4 slot FIFO to configure the duty cycle. This FIFO
> cannot be read back in its entirety. The "write x then read back
> x from hw" semantics are therefore not easily applicable.
> With this change, the .get_state() function tries to wait for some
> stabilization in the FIFO (empty state). In this state it keeps
> applying the last value written to the sample register.
> 
> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
> ---
>  drivers/pwm/pwm-imx27.c | 50 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 29a3089c534c..32389ca2da3e 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -75,6 +75,7 @@
>  						   (x)) + 1)
>  
>  #define MX3_PWM_SWR_LOOP		5
> +#define MX3_PWM_FIFOAV_EMPTY_LOOP	4
>  
>  /* PWMPR register value of 0xffff has the same effect as 0xfffe */
>  #define MX3_PWMPR_MAX			0xfffe
> @@ -118,8 +119,28 @@ static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
>  	clk_disable_unprepare(imx->clk_ipg);
>  }
>  
> +static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
> +				     struct pwm_device *pwm)
> +{
> +	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> +	struct device *dev = chip->dev;
> +	unsigned int period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm), NSEC_PER_MSEC);

Please don't use pwm_get_period. This one is intended for PWM consumers
only. Directly using pwm->state.period is fine however.

> +	int tries = MX3_PWM_FIFOAV_EMPTY_LOOP;
> +	int fifoav;
> +	u32 sr;
> +
> +	while (tries--) {
> +		sr = readl(imx->mmio_base + MX3_PWMSR);
> +		fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
> +		if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
> +			return;
> +		msleep(period_ms);

This is a rather long sleep. Maybe check in each iteration that fifoav
decreases as expected?

> +	}
> +	dev_warn(dev, "FIFO has been refilled concurrently\n");
> +}
> +
>  static int pwm_imx27_get_state(struct pwm_chip *chip,
> -			       struct pwm_device *pwm, struct pwm_state *state)
> +				struct pwm_device *pwm, struct pwm_state *state)
>  {
>  	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>  	u32 period, prescaler, pwm_clk, val;
> @@ -161,10 +182,33 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
>  	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
>  	 * use the cached value.
>  	 */
> -	if (state->enabled)
> +	if (state->enabled) {
> +		/*
> +		 * From the i.MX PWM reference manual:
> +		 * "A read on the sample register yields the current FIFO value that
> +		 *  is being used, or will be used, by the PWM for generation on the
> +		 *  output signal. Therefore, a write and a subsequent read on the
> +		 *  sample register may result in different values being obtained."
> +		 * Furthermore:
> +		 * "When a new value is written, the duty cycle changes after the
> +		 *  current period is over."
> +		 * Note "changes" vs. "changes to the given value"!
> +		 * Finally:
> +		 * "The PWM will run at the last set duty-cycle setting if all the
> +		 *  values of the FIFO has been utilized, until the FIFO is reloaded
> +		 *  or the PWM is disabled."
> +		 * Try to be at least a bit more deterministic about which value is
> +		 * read by waiting until the FIFO is empty. In this state the last/most
> +		 * recently pushed sample (duty cycle) value is continuously applied.
> +		 * Beware that this approach is still racy, as a new value could have
> +		 * been supplied and a period expired between the call of the wait
> +		 * function and the subsequent readl.
> +		 */
> +		pwm_imx27_wait_fifo_empty(chip, pwm);

Instead of issuing a warning, I'd return an error code if
wait_fifo_empty fails.

>  		val = readl(imx->mmio_base + MX3_PWMSAR);
> -	else
> +	} else {
>  		val = imx->duty_cycle;
> +	}
>  
>  	tmp = NSEC_PER_SEC * (u64)(val) * prescaler;
>  	state->duty_cycle = DIV_ROUND_UP_ULL(tmp, pwm_clk);

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 --]

[PATCH v3 1/4] pwm: imx27: fix race condition .apply,.get_state

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>

With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR
register in .apply(), the register is read in .get_state().
Unless a period completed in the meantime, this read yields the
previously used duty cycle configuration. As the PWM_DEBUG code
applies the read out configuration for testing purposes this
effectively undoes the intended effect by rewriting the previous
hardware state.

Note that this change merely implements a sensible heuristic.
The i.MX has a 4 slot FIFO to configure the duty cycle. This FIFO
cannot be read back in its entirety. The "write x then read back
x from hw" semantics are therefore not easily applicable.
With this change, the .get_state() function tries to wait for some
stabilization in the FIFO (empty state). In this state it keeps
applying the last value written to the sample register.

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
---
 drivers/pwm/pwm-imx27.c | 50 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 29a3089c534c..32389ca2da3e 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -75,6 +75,7 @@
 						   (x)) + 1)
 
 #define MX3_PWM_SWR_LOOP		5
+#define MX3_PWM_FIFOAV_EMPTY_LOOP	4
 
 /* PWMPR register value of 0xffff has the same effect as 0xfffe */
 #define MX3_PWMPR_MAX			0xfffe
@@ -118,8 +119,28 @@ static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
 	clk_disable_unprepare(imx->clk_ipg);
 }
 
+static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
+				     struct pwm_device *pwm)
+{
+	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
+	struct device *dev = chip->dev;
+	unsigned int period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm), NSEC_PER_MSEC);
+	int tries = MX3_PWM_FIFOAV_EMPTY_LOOP;
+	int fifoav;
+	u32 sr;
+
+	while (tries--) {
+		sr = readl(imx->mmio_base + MX3_PWMSR);
+		fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
+		if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
+			return;
+		msleep(period_ms);
+	}
+	dev_warn(dev, "FIFO has been refilled concurrently\n");
+}
+
 static int pwm_imx27_get_state(struct pwm_chip *chip,
-			       struct pwm_device *pwm, struct pwm_state *state)
+				struct pwm_device *pwm, struct pwm_state *state)
 {
 	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
 	u32 period, prescaler, pwm_clk, val;
@@ -161,10 +182,33 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
 	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
 	 * use the cached value.
 	 */
-	if (state->enabled)
+	if (state->enabled) {
+		/*
+		 * From the i.MX PWM reference manual:
+		 * "A read on the sample register yields the current FIFO value that
+		 *  is being used, or will be used, by the PWM for generation on the
+		 *  output signal. Therefore, a write and a subsequent read on the
+		 *  sample register may result in different values being obtained."
+		 * Furthermore:
+		 * "When a new value is written, the duty cycle changes after the
+		 *  current period is over."
+		 * Note "changes" vs. "changes to the given value"!
+		 * Finally:
+		 * "The PWM will run at the last set duty-cycle setting if all the
+		 *  values of the FIFO has been utilized, until the FIFO is reloaded
+		 *  or the PWM is disabled."
+		 * Try to be at least a bit more deterministic about which value is
+		 * read by waiting until the FIFO is empty. In this state the last/most
+		 * recently pushed sample (duty cycle) value is continuously applied.
+		 * Beware that this approach is still racy, as a new value could have
+		 * been supplied and a period expired between the call of the wait
+		 * function and the subsequent readl.
+		 */
+		pwm_imx27_wait_fifo_empty(chip, pwm);
 		val = readl(imx->mmio_base + MX3_PWMSAR);
-	else
+	} else {
 		val = imx->duty_cycle;
+	}
 
 	tmp = NSEC_PER_SEC * (u64)(val) * prescaler;
 	state->duty_cycle = DIV_ROUND_UP_ULL(tmp, pwm_clk);
-- 
2.39.2 (Apple Git-143)

[PATCH v3 2/4] pwm: imx27: avoid PWM consumer API

[Leif Middelschulte]

Access struct members directly as adviced[0].

[0] https://lore.kernel.org/lkml/20230310174517.rb7xxrougkse2lrc@pengutronix.de/T/#ec9560c1f613d9c0d7b77d72ad9051768812f80db

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
---
 drivers/pwm/pwm-imx27.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 32389ca2da3e..c2a1e2030072 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -124,7 +124,7 @@ static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
 {
 	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
 	struct device *dev = chip->dev;
-	unsigned int period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm), NSEC_PER_MSEC);
+	unsigned int period_ms = DIV_ROUND_UP_ULL(pwm->state.period, NSEC_PER_MSEC);
 	int tries = MX3_PWM_FIFOAV_EMPTY_LOOP;
 	int fifoav;
 	u32 sr;
@@ -248,7 +248,7 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
 	sr = readl(imx->mmio_base + MX3_PWMSR);
 	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
 	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
-		period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),
+		period_ms = DIV_ROUND_UP_ULL(pwm->state.period,
 					 NSEC_PER_MSEC);
 		msleep(period_ms);
 
-- 
2.39.2 (Apple Git-143)

[PATCH v3 3/4] pwm: imx27: verify decreasing PWM FIFO av value

[Leif Middelschulte]

Avoid unnecessary sleeps as adviced[0], if the FIFO cannot be emptied.

[0] https://lore.kernel.org/lkml/20230310174517.rb7xxrougkse2lrc@pengutronix.de/T/#ec9560c1f613d9c0d7b77d72ad9051768812f80db

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
---
 drivers/pwm/pwm-imx27.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index c2a1e2030072..9673e809d212 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -126,7 +126,7 @@ static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
 	struct device *dev = chip->dev;
 	unsigned int period_ms = DIV_ROUND_UP_ULL(pwm->state.period, NSEC_PER_MSEC);
 	int tries = MX3_PWM_FIFOAV_EMPTY_LOOP;
-	int fifoav;
+	int fifoav, previous_fifoav = INT_MAX;
 	u32 sr;
 
 	while (tries--) {
@@ -134,6 +134,10 @@ static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
 		fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
 		if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
 			return;
+		/* if the FIFO value does not decrease, there is another problem */
+		if (previous_fifoav == fifoav)
+			break;
+		previous_fifoav = fifoav;
 		msleep(period_ms);
 	}
 	dev_warn(dev, "FIFO has been refilled concurrently\n");
-- 
2.39.2 (Apple Git-143)

[PATCH v3 4/4] pwm: imx27: return error, if clean PWM setup fails

[Leif Middelschulte]

Instead of issuing a warning, return an error (as adviced[0]), if the
FIFO cannot be cleanly set up.

[0] https://lore.kernel.org/lkml/20230310174517.rb7xxrougkse2lrc@pengutronix.de/T/#ec9560c1f613d9c0d7b77d72ad9051768812f80db

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
---
 drivers/pwm/pwm-imx27.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 9673e809d212..5fd6d34a7722 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -133,14 +133,15 @@ static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
 		sr = readl(imx->mmio_base + MX3_PWMSR);
 		fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
 		if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
-			return;
+			return 0;
 		/* if the FIFO value does not decrease, there is another problem */
 		if (previous_fifoav == fifoav)
 			break;
 		previous_fifoav = fifoav;
 		msleep(period_ms);
 	}
-	dev_warn(dev, "FIFO has been refilled concurrently\n");
+
+	return -EAGAIN;
 }
 
 static int pwm_imx27_get_state(struct pwm_chip *chip,
@@ -208,7 +209,9 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
 		 * been supplied and a period expired between the call of the wait
 		 * function and the subsequent readl.
 		 */
-		pwm_imx27_wait_fifo_empty(chip, pwm);
+		ret = pwm_imx27_wait_fifo_empty(chip, pwm);
+		if (ret)
+			return ret;
 		val = readl(imx->mmio_base + MX3_PWMSAR);
 	} else {
 		val = imx->duty_cycle;
-- 
2.39.2 (Apple Git-143)

Re: [PATCH v3 2/4] pwm: imx27: avoid PWM consumer API

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 563 bytes --]

On Tue, Aug 15, 2023 at 12:43:30PM +0200, Leif Middelschulte wrote:
> Access struct members directly as adviced[0].
> 
> [0] https://lore.kernel.org/lkml/20230310174517.rb7xxrougkse2lrc@pengutronix.de/T/#ec9560c1f613d9c0d7b77d72ad9051768812f80db
> 
> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>

Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks,
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 --]

Re: [PATCH v3 1/4] pwm: imx27: fix race condition .apply,.get_state

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 4608 bytes --]

On Tue, Aug 15, 2023 at 12:43:29PM +0200, Leif Middelschulte wrote:
> From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
> 
> With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR
> register in .apply(), the register is read in .get_state().
> Unless a period completed in the meantime, this read yields the
> previously used duty cycle configuration. As the PWM_DEBUG code
> applies the read out configuration for testing purposes this
> effectively undoes the intended effect by rewriting the previous
> hardware state.
> 
> Note that this change merely implements a sensible heuristic.
> The i.MX has a 4 slot FIFO to configure the duty cycle. This FIFO
> cannot be read back in its entirety. The "write x then read back
> x from hw" semantics are therefore not easily applicable.
> With this change, the .get_state() function tries to wait for some
> stabilization in the FIFO (empty state). In this state it keeps
> applying the last value written to the sample register.
> 
> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
> ---
>  drivers/pwm/pwm-imx27.c | 50 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 47 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 29a3089c534c..32389ca2da3e 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -75,6 +75,7 @@
>  						   (x)) + 1)
>  
>  #define MX3_PWM_SWR_LOOP		5
> +#define MX3_PWM_FIFOAV_EMPTY_LOOP	4
>  
>  /* PWMPR register value of 0xffff has the same effect as 0xfffe */
>  #define MX3_PWMPR_MAX			0xfffe
> @@ -118,8 +119,28 @@ static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
>  	clk_disable_unprepare(imx->clk_ipg);
>  }
>  
> +static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
> +				     struct pwm_device *pwm)
> +{
> +	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> +	struct device *dev = chip->dev;
> +	unsigned int period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm), NSEC_PER_MSEC);
> +	int tries = MX3_PWM_FIFOAV_EMPTY_LOOP;
> +	int fifoav;
> +	u32 sr;
> +
> +	while (tries--) {
> +		sr = readl(imx->mmio_base + MX3_PWMSR);
> +		fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
> +		if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
> +			return;
> +		msleep(period_ms);
> +	}
> +	dev_warn(dev, "FIFO has been refilled concurrently\n");
> +}
> +
>  static int pwm_imx27_get_state(struct pwm_chip *chip,
> -			       struct pwm_device *pwm, struct pwm_state *state)
> +				struct pwm_device *pwm, struct pwm_state *state)

This looks wrong. Aligning at the opening ( was right.

>  {
>  	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>  	u32 period, prescaler, pwm_clk, val;
> @@ -161,10 +182,33 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
>  	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
>  	 * use the cached value.
>  	 */
> -	if (state->enabled)
> +	if (state->enabled) {
> +		/*
> +		 * From the i.MX PWM reference manual:
> +		 * "A read on the sample register yields the current FIFO value that
> +		 *  is being used, or will be used, by the PWM for generation on the
> +		 *  output signal. Therefore, a write and a subsequent read on the
> +		 *  sample register may result in different values being obtained."
> +		 * Furthermore:
> +		 * "When a new value is written, the duty cycle changes after the
> +		 *  current period is over."
> +		 * Note "changes" vs. "changes to the given value"!
> +		 * Finally:
> +		 * "The PWM will run at the last set duty-cycle setting if all the
> +		 *  values of the FIFO has been utilized, until the FIFO is reloaded
> +		 *  or the PWM is disabled."
> +		 * Try to be at least a bit more deterministic about which value is
> +		 * read by waiting until the FIFO is empty. In this state the last/most
> +		 * recently pushed sample (duty cycle) value is continuously applied.
> +		 * Beware that this approach is still racy, as a new value could have
> +		 * been supplied and a period expired between the call of the wait
> +		 * function and the subsequent readl.

this would only happen if there are concurrent calls into the driver,
wouldn't it? I think it's safe to assume this doesn't happen.

Patch 3 of this series improves the function that is only introduced
here. I suggest to squash these together.

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 --]

Re: [PATCH v3 4/4] pwm: imx27: return error, if clean PWM setup fails

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1942 bytes --]

On Tue, Aug 15, 2023 at 12:43:32PM +0200, Leif Middelschulte wrote:
> Instead of issuing a warning, return an error (as adviced[0]), if the
> FIFO cannot be cleanly set up.
> 
> [0] https://lore.kernel.org/lkml/20230310174517.rb7xxrougkse2lrc@pengutronix.de/T/#ec9560c1f613d9c0d7b77d72ad9051768812f80db
> 
> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
> ---
>  drivers/pwm/pwm-imx27.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 9673e809d212..5fd6d34a7722 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -133,14 +133,15 @@ static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
>  		sr = readl(imx->mmio_base + MX3_PWMSR);
>  		fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
>  		if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
> -			return;
> +			return 0;
>  		/* if the FIFO value does not decrease, there is another problem */
>  		if (previous_fifoav == fifoav)
>  			break;
>  		previous_fifoav = fifoav;
>  		msleep(period_ms);
>  	}
> -	dev_warn(dev, "FIFO has been refilled concurrently\n");
> +
> +	return -EAGAIN;
>  }
>  
>  static int pwm_imx27_get_state(struct pwm_chip *chip,
> @@ -208,7 +209,9 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
>  		 * been supplied and a period expired between the call of the wait
>  		 * function and the subsequent readl.
>  		 */
> -		pwm_imx27_wait_fifo_empty(chip, pwm);
> +		ret = pwm_imx27_wait_fifo_empty(chip, pwm);
> +		if (ret)
> +			return ret;
>  		val = readl(imx->mmio_base + MX3_PWMSAR);
>  	} else {
>  		val = imx->duty_cycle;

I'd squash this into the patch introducing pwm_imx27_wait_fifo_empty,
too.

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 --]

Re: [PATCH v3 1/4] pwm: imx27: fix race condition .apply,.get_state

[Leif Middelschulte]

> Am 06.09.2023 um 17:42 schrieb Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> 
> On Tue, Aug 15, 2023 at 12:43:29PM +0200, Leif Middelschulte wrote:
>> From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
>> 
>> With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR
>> register in .apply(), the register is read in .get_state().
>> Unless a period completed in the meantime, this read yields the
>> previously used duty cycle configuration. As the PWM_DEBUG code
>> applies the read out configuration for testing purposes this
>> effectively undoes the intended effect by rewriting the previous
>> hardware state.
>> 
>> Note that this change merely implements a sensible heuristic.
>> The i.MX has a 4 slot FIFO to configure the duty cycle. This FIFO
>> cannot be read back in its entirety. The "write x then read back
>> x from hw" semantics are therefore not easily applicable.
>> With this change, the .get_state() function tries to wait for some
>> stabilization in the FIFO (empty state). In this state it keeps
>> applying the last value written to the sample register.
>> 
>> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
>> ---
>> drivers/pwm/pwm-imx27.c | 50 ++++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 47 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
>> index 29a3089c534c..32389ca2da3e 100644
>> --- a/drivers/pwm/pwm-imx27.c
>> +++ b/drivers/pwm/pwm-imx27.c
>> @@ -75,6 +75,7 @@
>>   (x)) + 1)
>> 
>> #define MX3_PWM_SWR_LOOP 5
>> +#define MX3_PWM_FIFOAV_EMPTY_LOOP 4
>> 
>> /* PWMPR register value of 0xffff has the same effect as 0xfffe */
>> #define MX3_PWMPR_MAX 0xfffe
>> @@ -118,8 +119,28 @@ static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
>> clk_disable_unprepare(imx->clk_ipg);
>> }
>> 
>> +static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
>> +     struct pwm_device *pwm)
>> +{
>> + struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>> + struct device *dev = chip->dev;
>> + unsigned int period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm), NSEC_PER_MSEC);
>> + int tries = MX3_PWM_FIFOAV_EMPTY_LOOP;
>> + int fifoav;
>> + u32 sr;
>> +
>> + while (tries--) {
>> + sr = readl(imx->mmio_base + MX3_PWMSR);
>> + fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
>> + if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
>> + return;
>> + msleep(period_ms);
>> + }
>> + dev_warn(dev, "FIFO has been refilled concurrently\n");
>> +}
>> +
>> static int pwm_imx27_get_state(struct pwm_chip *chip,
>> -       struct pwm_device *pwm, struct pwm_state *state)
>> + struct pwm_device *pwm, struct pwm_state *state)
> 
> This looks wrong. Aligning at the opening ( was right.

Good catch. That change as unintentional.

> 
>> {
>> struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
>> u32 period, prescaler, pwm_clk, val;
>> @@ -161,10 +182,33 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
>> * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
>> * use the cached value.
>> */
>> - if (state->enabled)
>> + if (state->enabled) {
>> + /*
>> + * From the i.MX PWM reference manual:
>> + * "A read on the sample register yields the current FIFO value that
>> + *  is being used, or will be used, by the PWM for generation on the
>> + *  output signal. Therefore, a write and a subsequent read on the
>> + *  sample register may result in different values being obtained."
>> + * Furthermore:
>> + * "When a new value is written, the duty cycle changes after the
>> + *  current period is over."
>> + * Note "changes" vs. "changes to the given value"!
>> + * Finally:
>> + * "The PWM will run at the last set duty-cycle setting if all the
>> + *  values of the FIFO has been utilized, until the FIFO is reloaded
>> + *  or the PWM is disabled."
>> + * Try to be at least a bit more deterministic about which value is
>> + * read by waiting until the FIFO is empty. In this state the last/most
>> + * recently pushed sample (duty cycle) value is continuously applied.
>> + * Beware that this approach is still racy, as a new value could have
>> + * been supplied and a period expired between the call of the wait
>> + * function and the subsequent readl.
> 
> this would only happen if there are concurrent calls into the driver,
> wouldn't it? I think it's safe to assume this doesn't happen.

This assessment seems correct at them moment. This comment is supposed to be a source of explanation to future developers if - for some reason - the driver/subsystem is changed and they trip over this behaviour (again), just as I did.

> 
> Patch 3 of this series improves the function that is only introduced
> here. I suggest to squash these together.

I will provide v4 with the changes squashed into the initial commit, that introduced the function.

> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Thank you,

Leif

[PATCH v4 1/2] pwm: imx27: fix race condition .apply,.get_state

[Leif Middelschulte]

From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>

With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR
register in .apply(), the register is read in .get_state().
Unless a period completed in the meantime, this read yields the
previously used duty cycle configuration. As the PWM_DEBUG code
applies the read out configuration for testing purposes this
effectively undoes the intended effect by rewriting the previous
hardware state.

Note that this change merely implements a sensible heuristic.
The i.MX has a 4 slot FIFO to configure the duty cycle. This FIFO
cannot be read back in its entirety. The "write x then read back
x from hw" semantics are therefore not easily applicable.
With this change, the .get_state() function tries to wait for some
stabilization in the FIFO (empty state). In this state it keeps
applying the last value written to the sample register.

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
---
 drivers/pwm/pwm-imx27.c | 55 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index 7d9bc43f12b0..cb564460b79c 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -75,6 +75,7 @@
 						   (x)) + 1)
 
 #define MX3_PWM_SWR_LOOP		5
+#define MX3_PWM_FIFOAV_EMPTY_LOOP	4
 
 /* PWMPR register value of 0xffff has the same effect as 0xfffe */
 #define MX3_PWMPR_MAX			0xfffe
@@ -118,6 +119,31 @@ static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
 	clk_disable_unprepare(imx->clk_ipg);
 }
 
+static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
+				     struct pwm_device *pwm)
+{
+	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
+	struct device *dev = chip->dev;
+	unsigned int period_ms = DIV_ROUND_UP_ULL(pwm->state.period, NSEC_PER_MSEC);
+	int tries = MX3_PWM_FIFOAV_EMPTY_LOOP;
+	int fifoav, previous_fifoav = INT_MAX;
+	u32 sr;
+
+	while (tries--) {
+		sr = readl(imx->mmio_base + MX3_PWMSR);
+		fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
+		if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
+			return 0;
+		/* if the FIFO value does not decrease, there is another problem */
+		if (previous_fifoav == fifoav)
+			break;
+		previous_fifoav = fifoav;
+		msleep(period_ms);
+	}
+
+	return -EAGAIN;
+}
+
 static int pwm_imx27_get_state(struct pwm_chip *chip,
 			       struct pwm_device *pwm, struct pwm_state *state)
 {
@@ -161,10 +187,35 @@ static int pwm_imx27_get_state(struct pwm_chip *chip,
 	 * PWMSAR can be read only if PWM is enabled. If the PWM is disabled,
 	 * use the cached value.
 	 */
-	if (state->enabled)
+	if (state->enabled) {
+		/*
+		 * From the i.MX PWM reference manual:
+		 * "A read on the sample register yields the current FIFO value that
+		 *  is being used, or will be used, by the PWM for generation on the
+		 *  output signal. Therefore, a write and a subsequent read on the
+		 *  sample register may result in different values being obtained."
+		 * Furthermore:
+		 * "When a new value is written, the duty cycle changes after the
+		 *  current period is over."
+		 * Note "changes" vs. "changes to the given value"!
+		 * Finally:
+		 * "The PWM will run at the last set duty-cycle setting if all the
+		 *  values of the FIFO has been utilized, until the FIFO is reloaded
+		 *  or the PWM is disabled."
+		 * Try to be at least a bit more deterministic about which value is
+		 * read by waiting until the FIFO is empty. In this state the last/most
+		 * recently pushed sample (duty cycle) value is continuously applied.
+		 * Beware that this approach is still racy, as a new value could have
+		 * been supplied and a period expired between the call of the wait
+		 * function and the subsequent readl.
+		 */
+		ret = pwm_imx27_wait_fifo_empty(chip, pwm);
+		if (ret)
+			return ret;
 		val = readl(imx->mmio_base + MX3_PWMSAR);
-	else
+	} else {
 		val = imx->duty_cycle;
+	}
 
 	tmp = NSEC_PER_SEC * (u64)(val) * prescaler;
 	state->duty_cycle = DIV_ROUND_UP_ULL(tmp, pwm_clk);
-- 
2.39.3 (Apple Git-145)

[PATCH v4 2/2] pwm: imx27: avoid PWM consumer API

[Leif Middelschulte]

Access struct members directly.

Signed-off-by: Leif Middelschulte <Leif.Middelschulte@gmail.com>
---
 drivers/pwm/pwm-imx27.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
index cb564460b79c..d00d79a5edb1 100644
--- a/drivers/pwm/pwm-imx27.c
+++ b/drivers/pwm/pwm-imx27.c
@@ -255,7 +255,7 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
 	sr = readl(imx->mmio_base + MX3_PWMSR);
 	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
 	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
-		period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),
+		period_ms = DIV_ROUND_UP_ULL(pwm->state.period,
 					 NSEC_PER_MSEC);
 		msleep(period_ms);
 
-- 
2.39.3 (Apple Git-145)

Re: [PATCH v4 1/2] pwm: imx27: fix race condition .apply,.get_state

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3423 bytes --]

Hello Leif,

thanks for this new round addressing the identified issues.

On Sat, Feb 24, 2024 at 12:29:00PM +0100, Leif Middelschulte wrote:
> From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
> 
> With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR
> register in .apply(), the register is read in .get_state().
> Unless a period completed in the meantime, this read yields the
> previously used duty cycle configuration. As the PWM_DEBUG code
> applies the read out configuration for testing purposes this
> effectively undoes the intended effect by rewriting the previous
> hardware state.
> 
> Note that this change merely implements a sensible heuristic.
> The i.MX has a 4 slot FIFO to configure the duty cycle. This FIFO
> cannot be read back in its entirety. The "write x then read back
> x from hw" semantics are therefore not easily applicable.
> With this change, the .get_state() function tries to wait for some
> stabilization in the FIFO (empty state). In this state it keeps
> applying the last value written to the sample register.
> 
> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
> ---
>  drivers/pwm/pwm-imx27.c | 55 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 7d9bc43f12b0..cb564460b79c 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -75,6 +75,7 @@
>  						   (x)) + 1)
>  
>  #define MX3_PWM_SWR_LOOP		5
> +#define MX3_PWM_FIFOAV_EMPTY_LOOP	4

This looks like a register definition, but it's only a number that
defines the iterations waiting for the FIFO to empty. (The same critic
applies to MX3_PWM_SWR_LOOP, though.)

>  /* PWMPR register value of 0xffff has the same effect as 0xfffe */
>  #define MX3_PWMPR_MAX			0xfffe
> @@ -118,6 +119,31 @@ static void pwm_imx27_clk_disable_unprepare(struct pwm_imx27_chip *imx)
>  	clk_disable_unprepare(imx->clk_ipg);
>  }
>  
> +static int pwm_imx27_wait_fifo_empty(struct pwm_chip *chip,
> +				     struct pwm_device *pwm)
> +{
> +	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> +	struct device *dev = chip->dev;
> +	unsigned int period_ms = DIV_ROUND_UP_ULL(pwm->state.period, NSEC_PER_MSEC);

Given that waiting here is unfortunate it would be nice so reduce the
waiting as much as possible. So it might make sense to read the actual
period from the hardware and use that as it might be smaller that
pwm->state.period.

> +	int tries = MX3_PWM_FIFOAV_EMPTY_LOOP;
> +	int fifoav, previous_fifoav = INT_MAX;
> +	u32 sr;

Most variables can go into the while loop.

> +	while (tries--) {
> +		sr = readl(imx->mmio_base + MX3_PWMSR);
> +		fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
> +		if (fifoav == MX3_PWMSR_FIFOAV_EMPTY)
> +			return 0;
> +		/* if the FIFO value does not decrease, there is another problem */
> +		if (previous_fifoav == fifoav)
> +			break;
> +		previous_fifoav = fifoav;
> +		msleep(period_ms);
> +	}

I wonder if a loop is necessary at all. Why not use
msleep(FIELD_GET(MX3_PWMSR_FIFOAV, sr) * period_ms)?

Maybe take PWMCNR into account to shorten the sleep a bit.

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 --]

Re: [PATCH v4 2/2] pwm: imx27: avoid PWM consumer API

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 968 bytes --]

Hello Leif,

On Sat, Feb 24, 2024 at 12:29:01PM +0100, Leif Middelschulte wrote:
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index cb564460b79c..d00d79a5edb1 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -255,7 +255,7 @@ static void pwm_imx27_wait_fifo_slot(struct pwm_chip *chip,
>  	sr = readl(imx->mmio_base + MX3_PWMSR);
>  	fifoav = FIELD_GET(MX3_PWMSR_FIFOAV, sr);
>  	if (fifoav == MX3_PWMSR_FIFOAV_4WORDS) {
> -		period_ms = DIV_ROUND_UP_ULL(pwm_get_period(pwm),
> +		period_ms = DIV_ROUND_UP_ULL(pwm->state.period,
>  					 NSEC_PER_MSEC);
>  		msleep(period_ms);

This doesn't apply anymore on top of pwm/for-next. See commit
1706175c682fdb3fbe0b70d50b137aaa270959db which includes this change (and
more).

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 --]

Re: [PATCH v4 1/2] pwm: imx27: fix race condition .apply,.get_state

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

Hello,

On Sat, Feb 24, 2024 at 12:29:00PM +0100, Leif Middelschulte wrote:
> From: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>
> 
> With CONFIG_PWM_DEBUG=y after writing a value to the PWMSAR
> register in .apply(), the register is read in .get_state().
> Unless a period completed in the meantime, this read yields the
> previously used duty cycle configuration. As the PWM_DEBUG code
> applies the read out configuration for testing purposes this
> effectively undoes the intended effect by rewriting the previous
> hardware state.
> 
> Note that this change merely implements a sensible heuristic.
> The i.MX has a 4 slot FIFO to configure the duty cycle. This FIFO
> cannot be read back in its entirety. The "write x then read back
> x from hw" semantics are therefore not easily applicable.
> With this change, the .get_state() function tries to wait for some
> stabilization in the FIFO (empty state). In this state it keeps
> applying the last value written to the sample register.
> 
> Signed-off-by: Leif Middelschulte <Leif.Middelschulte@klsmartin.com>

Another few things I noticed only after replying to this patch and
trying to apply #2:

 - Please make sure you have a S-o-b line for the sender (i.e. you with
   your gmail identity).
 - My reply to your klsmartin.com address couldn't be delivered, The MS
   mailserver told: "leif.middelschulte wasn't found at klsmartin.com."
 - Please start a new thread if you send a v5, as applying patches from
   a thread containing many patches is a bit less trivial.
 - Consider using git format-patch's --base parameter to document your
   patch base.

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 --]