[PATCH 1/2] pwm: stmpe: Handle errors when disabling the signal

[PATCH 1/2] pwm: stmpe: Handle errors when disabling the signal

[Uwe Kleine-König]

Before the pwm framework implementedatomic updates (with the .apply()
callback) the .disable() callback returned void. This is still visible
in the stmpe driver which drops errors in the disable path.

Improve the driver to forward failures in stmpe_24xx_pwm_disable() to
the caller of pwm_apply_state().

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-stmpe.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-stmpe.c b/drivers/pwm/pwm-stmpe.c
index 5d4a4762ce0c..e205405c4828 100644
--- a/drivers/pwm/pwm-stmpe.c
+++ b/drivers/pwm/pwm-stmpe.c
@@ -61,8 +61,8 @@ static int stmpe_24xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	return 0;
 }
 
-static void stmpe_24xx_pwm_disable(struct pwm_chip *chip,
-				   struct pwm_device *pwm)
+static int stmpe_24xx_pwm_disable(struct pwm_chip *chip,
+				  struct pwm_device *pwm)
 {
 	struct stmpe_pwm *stmpe_pwm = to_stmpe_pwm(chip);
 	u8 value;
@@ -72,17 +72,16 @@ static void stmpe_24xx_pwm_disable(struct pwm_chip *chip,
 	if (ret < 0) {
 		dev_err(chip->dev, "error reading PWM#%u control\n",
 			pwm->hwpwm);
-		return;
+		return ret;
 	}
 
 	value = ret & ~BIT(pwm->hwpwm);
 
 	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, value);
-	if (ret) {
+	if (ret)
 		dev_err(chip->dev, "error writing PWM#%u control\n",
 			pwm->hwpwm);
-		return;
-	}
+	return ret;
 }
 
 /* STMPE 24xx PWM instructions */
@@ -111,7 +110,9 @@ static int stmpe_24xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	/* Make sure we are disabled */
 	if (pwm_is_enabled(pwm)) {
-		stmpe_24xx_pwm_disable(chip, pwm);
+		ret = stmpe_24xx_pwm_disable(chip, pwm);
+		if (ret)
+			return ret;
 	} else {
 		/* Connect the PWM to the pin */
 		pin = pwm->hwpwm;
@@ -269,7 +270,7 @@ static int stmpe_24xx_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	if (!state->enabled) {
 		if (pwm->state.enabled)
-			stmpe_24xx_pwm_disable(chip, pwm);
+			return stmpe_24xx_pwm_disable(chip, pwm);
 
 		return 0;
 	}

base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
-- 
2.39.2

[PATCH 2/2] pwm: stmpe: Don't issue error messages for problems in .apply_state()

[Uwe Kleine-König]

pwm drivers are supposed to be silent for failures in .apply_state()
because a problem is likely to be persistent and so it can easily flood
the kernel log. So remove all error messages from .apply_state() and the
functions that are (only) called by that callback.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-stmpe.c | 35 ++++++-----------------------------
 1 file changed, 6 insertions(+), 29 deletions(-)

diff --git a/drivers/pwm/pwm-stmpe.c b/drivers/pwm/pwm-stmpe.c
index e205405c4828..4a8d0d9b9cfc 100644
--- a/drivers/pwm/pwm-stmpe.c
+++ b/drivers/pwm/pwm-stmpe.c
@@ -43,22 +43,12 @@ static int stmpe_24xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	int ret;
 
 	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
-	if (ret < 0) {
-		dev_err(chip->dev, "error reading PWM#%u control\n",
-			pwm->hwpwm);
+	if (ret < 0)
 		return ret;
-	}
 
 	value = ret | BIT(pwm->hwpwm);
 
-	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, value);
-	if (ret) {
-		dev_err(chip->dev, "error writing PWM#%u control\n",
-			pwm->hwpwm);
-		return ret;
-	}
-
-	return 0;
+	return stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, value);
 }
 
 static int stmpe_24xx_pwm_disable(struct pwm_chip *chip,
@@ -69,19 +59,12 @@ static int stmpe_24xx_pwm_disable(struct pwm_chip *chip,
 	int ret;
 
 	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
-	if (ret < 0) {
-		dev_err(chip->dev, "error reading PWM#%u control\n",
-			pwm->hwpwm);
+	if (ret < 0)
 		return ret;
-	}
 
 	value = ret & ~BIT(pwm->hwpwm);
 
-	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, value);
-	if (ret)
-		dev_err(chip->dev, "error writing PWM#%u control\n",
-			pwm->hwpwm);
-	return ret;
+	return stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, value);
 }
 
 /* STMPE 24xx PWM instructions */
@@ -124,11 +107,8 @@ static int stmpe_24xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		ret = stmpe_set_altfunc(stmpe_pwm->stmpe, BIT(pin),
 					STMPE_BLOCK_PWM);
-		if (ret) {
-			dev_err(chip->dev, "unable to connect PWM#%u to pin\n",
-				pwm->hwpwm);
+		if (ret)
 			return ret;
-		}
 	}
 
 	/* STMPE24XX */
@@ -241,11 +221,8 @@ static int stmpe_24xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		value = program[i] & 0xff;
 
 		ret = stmpe_reg_write(stmpe_pwm->stmpe, offset, value);
-		if (ret) {
-			dev_err(chip->dev, "error writing register %02x: %d\n",
-				offset, ret);
+		if (ret)
 			return ret;
-		}
 	}
 
 	/* If we were enabled, re-enable this PWM */
-- 
2.39.2

Re: [PATCH 2/2] pwm: stmpe: Don't issue error messages for problems in .apply_state()

[Thierry Reding]

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

On Fri, Jul 14, 2023 at 11:45:19PM +0200, Uwe Kleine-König wrote:
> pwm drivers are supposed to be silent for failures in .apply_state()
> because a problem is likely to be persistent and so it can easily flood
> the kernel log. So remove all error messages from .apply_state() and the
> functions that are (only) called by that callback.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-stmpe.c | 35 ++++++-----------------------------
>  1 file changed, 6 insertions(+), 29 deletions(-)

I don't necessarily agree with that claim. Given that some of the
implementations can be quite complex, the error messages may be useful
to diagnose what exactly is going wrong. It's also quite rare for any
consumers to call pwm_apply_state() in quick succession, so I don't
think "flooding" is really a problem.

Is this an actual problem anywhere?

Looking at where these errors would originate, these can be either from
I2C or SPI and the MFD driver will do some probing and failures in that
code would lead to probe failure, so any persistent problems are likely
going to be detected early on. And if there are errors that start to
happen at runtime, it's probably not a bad idea to be as noisy as
possible.

On the other hand, the stmpe_reg_read() and stmpe_reg_write() produce
error messages of their own, so the ones in this driver mainly serve as
adding context. Perhaps rather than removing these, turning them into
dev_dbg() would be a good compromise?

Thierry

> 
> diff --git a/drivers/pwm/pwm-stmpe.c b/drivers/pwm/pwm-stmpe.c
> index e205405c4828..4a8d0d9b9cfc 100644
> --- a/drivers/pwm/pwm-stmpe.c
> +++ b/drivers/pwm/pwm-stmpe.c
> @@ -43,22 +43,12 @@ static int stmpe_24xx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	int ret;
>  
>  	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
> -	if (ret < 0) {
> -		dev_err(chip->dev, "error reading PWM#%u control\n",
> -			pwm->hwpwm);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	value = ret | BIT(pwm->hwpwm);
>  
> -	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, value);
> -	if (ret) {
> -		dev_err(chip->dev, "error writing PWM#%u control\n",
> -			pwm->hwpwm);
> -		return ret;
> -	}
> -
> -	return 0;
> +	return stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, value);
>  }
>  
>  static int stmpe_24xx_pwm_disable(struct pwm_chip *chip,
> @@ -69,19 +59,12 @@ static int stmpe_24xx_pwm_disable(struct pwm_chip *chip,
>  	int ret;
>  
>  	ret = stmpe_reg_read(stmpe_pwm->stmpe, STMPE24XX_PWMCS);
> -	if (ret < 0) {
> -		dev_err(chip->dev, "error reading PWM#%u control\n",
> -			pwm->hwpwm);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	value = ret & ~BIT(pwm->hwpwm);
>  
> -	ret = stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, value);
> -	if (ret)
> -		dev_err(chip->dev, "error writing PWM#%u control\n",
> -			pwm->hwpwm);
> -	return ret;
> +	return stmpe_reg_write(stmpe_pwm->stmpe, STMPE24XX_PWMCS, value);
>  }
>  
>  /* STMPE 24xx PWM instructions */
> @@ -124,11 +107,8 @@ static int stmpe_24xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  		ret = stmpe_set_altfunc(stmpe_pwm->stmpe, BIT(pin),
>  					STMPE_BLOCK_PWM);
> -		if (ret) {
> -			dev_err(chip->dev, "unable to connect PWM#%u to pin\n",
> -				pwm->hwpwm);
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	/* STMPE24XX */
> @@ -241,11 +221,8 @@ static int stmpe_24xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		value = program[i] & 0xff;
>  
>  		ret = stmpe_reg_write(stmpe_pwm->stmpe, offset, value);
> -		if (ret) {
> -			dev_err(chip->dev, "error writing register %02x: %d\n",
> -				offset, ret);
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	/* If we were enabled, re-enable this PWM */
> -- 
> 2.39.2
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/2] pwm: stmpe: Don't issue error messages for problems in .apply_state()

[Uwe Kleine-König]

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

On Fri, Jul 28, 2023 at 10:02:22AM +0200, Thierry Reding wrote:
> On Fri, Jul 14, 2023 at 11:45:19PM +0200, Uwe Kleine-König wrote:
> > pwm drivers are supposed to be silent for failures in .apply_state()
> > because a problem is likely to be persistent and so it can easily flood
> > the kernel log. So remove all error messages from .apply_state() and the
> > functions that are (only) called by that callback.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-stmpe.c | 35 ++++++-----------------------------
> >  1 file changed, 6 insertions(+), 29 deletions(-)
> 
> I don't necessarily agree with that claim. Given that some of the
> implementations can be quite complex, the error messages may be useful
> to diagnose what exactly is going wrong. It's also quite rare for any
> consumers to call pwm_apply_state() in quick succession, so I don't
> think "flooding" is really a problem.
> 
> Is this an actual problem anywhere?

The real motivation for this change was to stop the driver making use of
struct pwm_chip::dev as I'm changing that one in my effort to track a
pwm_chip's lifetime using a struct device preparing character device
support for PWMs. So each usage that is gone doesn't need to be touched
later together with the pwm_chip restructurings.

While I think that the error messages are hardly useful I also never
were in a situation where they triggered, so maybe my judgement isn't
well-founded.

> On the other hand, the stmpe_reg_read() and stmpe_reg_write() produce
> error messages of their own, so the ones in this driver mainly serve as
> adding context. Perhaps rather than removing these, turning them into
> dev_dbg() would be a good compromise?

I will consider that. It would still use chip->dev, but I can cope with
that. :-)

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: (subset) [PATCH 1/2] pwm: stmpe: Handle errors when disabling the signal

[Thierry Reding]

On Fri, 14 Jul 2023 23:45:18 +0200, Uwe Kleine-König wrote:
> Before the pwm framework implementedatomic updates (with the .apply()
> callback) the .disable() callback returned void. This is still visible
> in the stmpe driver which drops errors in the disable path.
> 
> Improve the driver to forward failures in stmpe_24xx_pwm_disable() to
> the caller of pwm_apply_state().
> 
> [...]

Applied, thanks!

[1/2] pwm: stmpe: Handle errors when disabling the signal
      commit: b2c71e9f8dd0d023a847f6c38f9a83c0949ec01a

Best regards,
-- 
Thierry Reding <thierry.reding@gmail.com>