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

[Thierry Reding <thierry.reding@gmail.com>]

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