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.
--
With Best Regards,
Andy Shevchenko
