diff mbox series

[3/3] pwm: Handle .get_state() failures

Message ID 20220916151506.298488-3-u.kleine-koenig@pengutronix.de
State Accepted
Commit c73a3107624ddc305483ced13deca9ce8a073783
Headers show
Series [1/3] pwm: Change prototype of .get_state() callback to return an error | expand

Commit Message

Uwe Kleine-König Sept. 16, 2022, 3:15 p.m. UTC
This suppresses diagnosis for PWM_DEBUG routines and makes sure that
pwm->state isn't modified in pwm_device_request() if .get_state() fails.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/core.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Comments

Thierry Reding Sept. 28, 2022, 12:55 p.m. UTC | #1
On Fri, Sep 16, 2022 at 05:15:06PM +0200, Uwe Kleine-König wrote:
> This suppresses diagnosis for PWM_DEBUG routines and makes sure that
> pwm->state isn't modified in pwm_device_request() if .get_state() fails.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/core.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 381db04cfa00..421573590613 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -108,9 +108,14 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
>  	}
>  
>  	if (pwm->chip->ops->get_state) {
> -		err = pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
> +		struct pwm_state state;
> +
> +		err = pwm->chip->ops->get_state(pwm->chip, pwm, &state);
>  		trace_pwm_get(pwm, &pwm->state, err);
>  
> +		if (!err)
> +			pwm->state = state;

So basically this means that callers of pwm_get_state() will get the
zeroed out pwm->state. This can cause issues with the likes of
pwm_set_relative_duty_cycle() which many drivers would use. Do we
perhaps want to set an internal error in this case so that it can be
propagated to callers in pwm_get_state()? That would allow them to fall
back to some default configuration rather than potentially breaking
altogether.

Thierry
diff mbox series

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 381db04cfa00..421573590613 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -108,9 +108,14 @@  static int pwm_device_request(struct pwm_device *pwm, const char *label)
 	}
 
 	if (pwm->chip->ops->get_state) {
-		err = pwm->chip->ops->get_state(pwm->chip, pwm, &pwm->state);
+		struct pwm_state state;
+
+		err = pwm->chip->ops->get_state(pwm->chip, pwm, &state);
 		trace_pwm_get(pwm, &pwm->state, err);
 
+		if (!err)
+			pwm->state = state;
+
 		if (IS_ENABLED(CONFIG_PWM_DEBUG))
 			pwm->last = pwm->state;
 	}
@@ -459,6 +464,9 @@  static void pwm_apply_state_debug(struct pwm_device *pwm,
 
 	err = chip->ops->get_state(chip, pwm, &s1);
 	trace_pwm_get(pwm, &s1, err);
+	if (err)
+		/* If that failed there isn't much to debug */
+		return;
 
 	/*
 	 * The lowlevel driver either ignored .polarity (which is a bug) or as
@@ -523,6 +531,8 @@  static void pwm_apply_state_debug(struct pwm_device *pwm,
 
 	err = chip->ops->get_state(chip, pwm, last);
 	trace_pwm_get(pwm, last, err);
+	if (err)
+		return;
 
 	/* reapplication of the current state should give an exact match */
 	if (s1.enabled != last->enabled ||