On Tue, Jul 28, 2020 at 09:55:22PM +0200, Hans de Goede wrote:
On 7/28/20 8:57 PM, Andy Shevchenko wrote:
On Fri, Jul 17, 2020 at 03:37:43PM +0200, Hans de Goede wrote:
...
Maybe I'm too picky, but I would go even further and split apply to two versions
static int pwm_lpss_apply_on_resume(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state)
{ struct pwm_lpss_chip *lpwm = to_lpwm(chip); if (state->enabled) return pwm_lpss_prepare_enable(lpwm, pwm, state, !pwm_is_enabled(pwm)); if (pwm_is_enabled(pwm)) { pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); return 0; }
and another one for !from_resume.
It is a bit picky :) But that is actually not a bad idea, although I would write it like this for more symmetry with the normal (not on_resume) apply version, while at it I also renamed the function:
/*
- This is a mirror of pwm_lpss_apply() without pm_runtime reference handling
- for restoring the PWM state on resume.
*/ static int pwm_lpss_restore_state(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { struct pwm_lpss_chip *lpwm = to_lpwm(chip); int ret = 0;
if (state->enabled) ret = pwm_lpss_prepare_enable(lpwm, pwm, state, !pwm_is_enabled(pwm)); else if (pwm_is_enabled(pwm)) pwm_lpss_write(pwm, pwm_lpss_read(pwm) & ~PWM_ENABLE); return ret;}
Would that work for you?
Yes.
...
ret = __pwm_lpss_apply(&lpwm->chip, pwm, &saved_state, true);if (ret)dev_err(dev, "Error restoring state on resume\n");I'm wondering if it's a real error why we do not bail out? Otherwise dev_warn() ?
It is a real error, but a single PWM chip might have multiple controllers and bailing out early would mean not even trying to restore the state on the other controllers. As for propagating the error, AFAIK the pm framework does not do anything with resume errors other then log an extra error.
OK.