| 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 |
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 --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 ||
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(-)