Re: [PATCH v5 06/16] pwm: lpss: Use pwm_lpss_apply() when restoring state on resume

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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