Linux-Renesas-SoC Archive on lore.kernel.org
 help / color / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Cc: linux-pwm@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH v3 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()
Date: Mon, 4 Mar 2019 12:18:33 +0100
Message-ID: <20190304111833.GD5121@ulmo> (raw)
In-Reply-To: <1547021948-7664-3-git-send-email-yoshihiro.shimoda.uh@renesas.com>

[-- Attachment #1: Type: text/plain, Size: 2236 bytes --]

On Wed, Jan 09, 2019 at 05:19:06PM +0900, Yoshihiro Shimoda wrote:
> To remove legacy API related functions in the future, this patch
> uses "atomic" related function instead. No change in behavior.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-rcar.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index e3ab663..652a937 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -316,18 +316,16 @@ static int rcar_pwm_suspend(struct device *dev)
>  static int rcar_pwm_resume(struct device *dev)
>  {
>  	struct pwm_device *pwm = rcar_pwm_dev_to_pwm_dev(dev);
> +	struct pwm_state state;
>  
>  	if (!test_bit(PWMF_REQUESTED, &pwm->flags))
>  		return 0;
>  
>  	pm_runtime_get_sync(dev);
>  
> -	rcar_pwm_config(pwm->chip, pwm, pwm->state.duty_cycle,
> -			pwm->state.period);
> -	if (pwm_is_enabled(pwm))
> -		rcar_pwm_enable(pwm->chip, pwm);
> +	pwm_get_state(pwm, &state);
>  
> -	return 0;
> +	return rcar_pwm_apply(pwm->chip, pwm, &state);
>  }
>  #endif /* CONFIG_PM_SLEEP */
>  static SIMPLE_DEV_PM_OPS(rcar_pwm_pm_ops, rcar_pwm_suspend, rcar_pwm_resume);

As was recently discussed elsewhere, it should really be up to the
consumer of PWMs to reinitialize the PWM on resume. However, this is a
preexisting condition, so let's do it in a follow-up.

Do you have any cases where you really rely on this? Resume ordering
does not guarantee that the PWM and consumer will be resumed in the
right order, so you may be enabling the PWM too soon, or too late, by
doing this in the PWM driver.

There are patches in the works to help with this using device links, but
I think even with those there will be cases where the consumer will want
to directly control when the PWM is restored, so you should consider
moving the driver away from implementing suspend/resume itself.

It'd be interesting to hear if there any cases where things break if you
simply remove these PM callbacks. If so we should investigate and fix
them so that these can be removed.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply index

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-01-09  8:19 [PATCH v3 0/4] pwm: rcar: Add support "atomic" API and improve calculation Yoshihiro Shimoda
2019-01-09  8:19 ` [PATCH v3 1/4] pwm: rcar: Add support "atomic" API Yoshihiro Shimoda
2019-01-09  8:19 ` [PATCH v3 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume() Yoshihiro Shimoda
2019-03-04 11:18   ` Thierry Reding [this message]
2019-01-09  8:19 ` [PATCH v3 3/4] pwm: rcar: remove legacy APIs Yoshihiro Shimoda
2019-01-09  8:19 ` [PATCH v3 4/4] pwm: rcar: improve calculation of divider Yoshihiro Shimoda
2019-03-04 11:14 ` [PATCH v3 0/4] pwm: rcar: Add support "atomic" API and improve calculation Thierry Reding

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190304111833.GD5121@ulmo \
    --to=thierry.reding@gmail.com \
    --cc=linux-pwm@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=yoshihiro.shimoda.uh@renesas.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox