[PATCH v2 0/4] pwm: rcar: Add support "atomic" API and improve calculation

[PATCH v2 0/4] pwm: rcar: Add support "atomic" API and improve calculation

[Yoshihiro Shimoda]

This patch series adds support for PWM "atomic" API and improve
the clock calculation.

Changes from v1 [1]:
 - Remove workaround code of output pseudo low level for now because it is
   rejected on the ML.
 - Add a condition for polarity because the HW doesn't support polarity change
   in patch 1/4.
 - Improve the clock calculation in patch 4/4.

[1]
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=53193


Yoshihiro Shimoda (4):
  pwm: rcar: Add support "atomic" API
  pwm: rcar: Use "atomic" API on rcar_pwm_resume()
  pwm: rcar: remove legacy APIs
  pwm: rcar: improve calculation of divider

 drivers/pwm/pwm-rcar.c | 99 ++++++++++++++++++++++++--------------------------
 1 file changed, 48 insertions(+), 51 deletions(-)

-- 
1.9.1

[PATCH v2 1/4] pwm: rcar: Add support "atomic" API

[Yoshihiro Shimoda]

This patch adds support for "atomic" API. Behavior is the same as
when using legacy APIs.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/pwm-rcar.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index a41812f..ba70e83 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -192,12 +192,49 @@ static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
 }
 
+static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			  struct pwm_state *state)
+{
+	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
+	struct pwm_state cur_state;
+	int div, ret;
+
+	/* This HW doesn't support changing polarity */
+	pwm_get_state(pwm, &cur_state);
+	if (state->polarity != cur_state.polarity)
+		return -ENOTSUPP;
+
+	div = rcar_pwm_get_clock_division(rp, state->period);
+	if (div < 0)
+		return div;
+
+	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
+
+	ret = rcar_pwm_set_counter(rp, div, state->duty_cycle, state->period);
+	if (!ret)
+		rcar_pwm_set_clock_control(rp, div);
+
+	/* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */
+	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
+
+	if (!ret && state->enabled)
+		ret = rcar_pwm_enable(chip, pwm);
+
+	if (!state->enabled) {
+		rcar_pwm_disable(chip, pwm);
+		ret = 0;
+	}
+
+	return ret;
+}
+
 static const struct pwm_ops rcar_pwm_ops = {
 	.request = rcar_pwm_request,
 	.free = rcar_pwm_free,
 	.config = rcar_pwm_config,
 	.enable = rcar_pwm_enable,
 	.disable = rcar_pwm_disable,
+	.apply = rcar_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
-- 
1.9.1

Re: [PATCH v2 1/4] pwm: rcar: Add support "atomic" API

[Uwe Kleine-König]

Hello,

On Tue, Jan 08, 2019 at 12:28:11PM +0900, Yoshihiro Shimoda wrote:
> This patch adds support for "atomic" API. Behavior is the same as
> when using legacy APIs.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pwm/pwm-rcar.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index a41812f..ba70e83 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -192,12 +192,49 @@ static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
>  }
>  
> +static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  struct pwm_state *state)
> +{
> +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> +	struct pwm_state cur_state;
> +	int div, ret;
> +
> +	/* This HW doesn't support changing polarity */
> +	pwm_get_state(pwm, &cur_state);
> +	if (state->polarity != cur_state.polarity)
> +		return -ENOTSUPP;

Does the driver only support normal polarity or only inversed polarity?
If so checking against that would be more clear here.

> +
> +	div = rcar_pwm_get_clock_division(rp, state->period);
> +	if (div < 0)
> +		return div;
> +
> +	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
> +
> +	ret = rcar_pwm_set_counter(rp, div, state->duty_cycle, state->period);
> +	if (!ret)
> +		rcar_pwm_set_clock_control(rp, div);
> +
> +	/* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */
> +	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
> +
> +	if (!ret && state->enabled)
> +		ret = rcar_pwm_enable(chip, pwm);
> +
> +	if (!state->enabled) {
> +		rcar_pwm_disable(chip, pwm);
> +		ret = 0;
> +	}

Assume the PWM runs with duty cycle 33% when
pwm_apply_state({ .enabled=0, .duty_cycle=66, .period=100 }) is called.

Does this might result in a 66% wave form being emitted? If yes, this
needs fixing.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH v2 1/4] pwm: rcar: Add support "atomic" API

[Yoshihiro Shimoda]

Hello Uwe,

Thank you for the review!

> From: Uwe Kleine-Konig, Sent: Tuesday, January 8, 2019 4:44 PM
<snip>
> > +static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > +			  struct pwm_state *state)
> > +{
> > +	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
> > +	struct pwm_state cur_state;
> > +	int div, ret;
> > +
> > +	/* This HW doesn't support changing polarity */
> > +	pwm_get_state(pwm, &cur_state);
> > +	if (state->polarity != cur_state.polarity)
> > +		return -ENOTSUPP;
> 
> Does the driver only support normal polarity or only inversed polarity?
> If so checking against that would be more clear here.

This driver only supports normal polarity. So, I'll change the code as the following:

	/* This HW/driver only support normal polarity */
	pwm_get_state(pwm, &cur_state);
	if (state->polarity != PWM_POLARITY_NORMAL)
		return -ENOTSUPP;

> > +	div = rcar_pwm_get_clock_division(rp, state->period);
> > +	if (div < 0)
> > +		return div;
> > +
> > +	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
> > +
> > +	ret = rcar_pwm_set_counter(rp, div, state->duty_cycle, state->period);
> > +	if (!ret)
> > +		rcar_pwm_set_clock_control(rp, div);
> > +
> > +	/* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */
> > +	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
> > +
> > +	if (!ret && state->enabled)
> > +		ret = rcar_pwm_enable(chip, pwm);
> > +
> > +	if (!state->enabled) {
> > +		rcar_pwm_disable(chip, pwm);
> > +		ret = 0;
> > +	}
> 
> Assume the PWM runs with duty cycle 33% when
> pwm_apply_state({ .enabled=0, .duty_cycle=66, .period=100 }) is called.
> 
> Does this might result in a 66% wave form being emitted? If yes, this
> needs fixing.

Thank you for the pointing it out. This code is possible to output 66% wave form
between the second rcar_pwm_update() and rcar_pwm_disable(). So, I'll fix this.

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()

[Yoshihiro Shimoda]

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>
---
 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 ba70e83..4987c12 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);
-- 
1.9.1

Re: [PATCH v2 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()

[Uwe Kleine-König]

On Tue, Jan 08, 2019 at 12:28:12PM +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>
> ---
>  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 ba70e83..4987c12 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);
>  }

Orthogonal to this patch I wonder what the intended behaviour for a pwm
is on suspend. Should it stop oscilating unconditionally? Or should it
only stop if the consumer stops it as part of its own suspend callback?

As the patch only reworks and improves the code without a change in
behaviour, it is fine for me.

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

RE: [PATCH v2 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()

[Yoshihiro Shimoda]

Hello Uwe,

> From: Uwe Kleine-Konig, Sent: Tuesday, January 8, 2019 4:48 PM
> > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> > index ba70e83..4987c12 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);
> >  }
> 
> Orthogonal to this patch I wonder what the intended behaviour for a pwm
> is on suspend. Should it stop oscilating unconditionally? Or should it
> only stop if the consumer stops it as part of its own suspend callback?

I think the second one is better. I checked drivers/video/backlight/pwm_bl.c
and it is possible to call pwm_apply_state() by the driver after rcar_pwm_suspend()
was called. So, I'll fix this as other patch.

> As the patch only reworks and improves the code without a change in
> behaviour, it is fine for me.
> 
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks!

Best regards,
Yoshihiro Shimoda

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()

[Uwe Kleine-König]

On Tue, Jan 08, 2019 at 08:46:30AM +0000, Yoshihiro Shimoda wrote:
> Hello Uwe,
> 
> > From: Uwe Kleine-Konig, Sent: Tuesday, January 8, 2019 4:48 PM
> > > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> > > index ba70e83..4987c12 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);
> > >  }
> > 
> > Orthogonal to this patch I wonder what the intended behaviour for a pwm
> > is on suspend. Should it stop oscilating unconditionally? Or should it
> > only stop if the consumer stops it as part of its own suspend callback?
> 
> I think the second one is better.

I agree. @Thierry: Do you agree, too? Then we should document that.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()

[Geert Uytterhoeven]

Hi Uwe,

On Tue, Jan 8, 2019 at 8:48 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> Orthogonal to this patch I wonder what the intended behaviour for a pwm
> is on suspend. Should it stop oscilating unconditionally? Or should it
> only stop if the consumer stops it as part of its own suspend callback?

I guess you mean system suspend, not runtime suspend, as the device is
runtime-resumed when a PWM is requested?

Permitted behavior depends on the system: on R-Car Gen3 (arm64), PSCI system
suspend will power down the SoC, so PWM output will stop for sure.

On R-Car Gen2 (or R-Car Gen3 with s2idle instead of s2ram), the PM Domain
code will turn of the PWM module's clock. Hence it will stop oscillating, unless
you take special countermeasures, like for modules that need to stay powered
for wake-up handling.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()

[Uwe Kleine-König]

Hello Geert,

On Tue, Jan 08, 2019 at 09:56:28AM +0100, Geert Uytterhoeven wrote:
> On Tue, Jan 8, 2019 at 8:48 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > Orthogonal to this patch I wonder what the intended behaviour for a pwm
> > is on suspend. Should it stop oscilating unconditionally? Or should it
> > only stop if the consumer stops it as part of its own suspend callback?
> 
> I guess you mean system suspend, not runtime suspend, as the device is
> runtime-resumed when a PWM is requested?

I admit that suspend (both system and runtime) is a bit of a black box
for me. So please take that into account when judging my statements.
 
> Permitted behavior depends on the system: on R-Car Gen3 (arm64), PSCI system
> suspend will power down the SoC, so PWM output will stop for sure.
> 
> On R-Car Gen2 (or R-Car Gen3 with s2idle instead of s2ram), the PM Domain
> code will turn of the PWM module's clock. Hence it will stop oscillating, unless
> you take special countermeasures, like for modules that need to stay powered
> for wake-up handling.

Whatever "suspend" here means, I want to prevent that a stopping pwm is
a surprise for the consumer. So I think suspend should be inhibited if
the consumer might expect the pwm to continue running but the pwm is
about to stop. So if the suspend affects the consumer, too, it's the
consumer that should be responsible to stop the pwm in a controlled
manner.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()

[Geert Uytterhoeven]

Hi Uwe,

On Tue, Jan 8, 2019 at 10:19 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Jan 08, 2019 at 09:56:28AM +0100, Geert Uytterhoeven wrote:
> > On Tue, Jan 8, 2019 at 8:48 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > Orthogonal to this patch I wonder what the intended behaviour for a pwm
> > > is on suspend. Should it stop oscilating unconditionally? Or should it
> > > only stop if the consumer stops it as part of its own suspend callback?
> >
> > I guess you mean system suspend, not runtime suspend, as the device is
> > runtime-resumed when a PWM is requested?
>
> I admit that suspend (both system and runtime) is a bit of a black box
> for me. So please take that into account when judging my statements.
>
> > Permitted behavior depends on the system: on R-Car Gen3 (arm64), PSCI system
> > suspend will power down the SoC, so PWM output will stop for sure.
> >
> > On R-Car Gen2 (or R-Car Gen3 with s2idle instead of s2ram), the PM Domain
> > code will turn of the PWM module's clock. Hence it will stop oscillating, unless
> > you take special countermeasures, like for modules that need to stay powered
> > for wake-up handling.
>
> Whatever "suspend" here means, I want to prevent that a stopping pwm is
> a surprise for the consumer. So I think suspend should be inhibited if
> the consumer might expect the pwm to continue running but the pwm is
> about to stop. So if the suspend affects the consumer, too, it's the
> consumer that should be responsible to stop the pwm in a controlled
> manner.

I think you can inhibit suspend by letting the .suspend() callback return an
error. However, I think the PWM driver cannot make that decision on its own.
Shutting down the PWM for a status LED is harmless, and the status LED being
enabled should not prevent a system suspend.
Shutting down e.g. a motor may be different.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/4] pwm: rcar: Use "atomic" API on rcar_pwm_resume()

[Uwe Kleine-König]

On Tue, Jan 08, 2019 at 10:35:29AM +0100, Geert Uytterhoeven wrote:
> Hi Uwe,
> 
> On Tue, Jan 8, 2019 at 10:19 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Tue, Jan 08, 2019 at 09:56:28AM +0100, Geert Uytterhoeven wrote:
> > > On Tue, Jan 8, 2019 at 8:48 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > Orthogonal to this patch I wonder what the intended behaviour for a pwm
> > > > is on suspend. Should it stop oscilating unconditionally? Or should it
> > > > only stop if the consumer stops it as part of its own suspend callback?
> > >
> > > I guess you mean system suspend, not runtime suspend, as the device is
> > > runtime-resumed when a PWM is requested?
> >
> > I admit that suspend (both system and runtime) is a bit of a black box
> > for me. So please take that into account when judging my statements.
> >
> > > Permitted behavior depends on the system: on R-Car Gen3 (arm64), PSCI system
> > > suspend will power down the SoC, so PWM output will stop for sure.
> > >
> > > On R-Car Gen2 (or R-Car Gen3 with s2idle instead of s2ram), the PM Domain
> > > code will turn of the PWM module's clock. Hence it will stop oscillating, unless
> > > you take special countermeasures, like for modules that need to stay powered
> > > for wake-up handling.
> >
> > Whatever "suspend" here means, I want to prevent that a stopping pwm is
> > a surprise for the consumer. So I think suspend should be inhibited if
> > the consumer might expect the pwm to continue running but the pwm is
> > about to stop. So if the suspend affects the consumer, too, it's the
> > consumer that should be responsible to stop the pwm in a controlled
> > manner.
> 
> I think you can inhibit suspend by letting the .suspend() callback return an
> error. However, I think the PWM driver cannot make that decision on its own.
> Shutting down the PWM for a status LED is harmless, and the status LED being
> enabled should not prevent a system suspend.
> Shutting down e.g. a motor may be different.

Also this is something that should be implemented in the core (or at
least consistently in all drivers). So I think it's not sensible to
continue discussion that in the context of this rcar change. I added it
to my list of stuff to discuss.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 3/4] pwm: rcar: remove legacy APIs

[Yoshihiro Shimoda]

This patch removes legacy APIs. Since rcar_pwm_{en,dis}able()
functions are reused on "atomic" API, this patch changes
the arguments of these functions. No change in behavior.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/pwm-rcar.c | 44 ++++----------------------------------------
 1 file changed, 4 insertions(+), 40 deletions(-)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index 4987c12..6dbb70c 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -139,39 +139,8 @@ static void rcar_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	pm_runtime_put(chip->dev);
 }
 
-static int rcar_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			   int duty_ns, int period_ns)
+static int rcar_pwm_enable(struct rcar_pwm_chip *rp)
 {
-	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
-	int div, ret;
-
-	div = rcar_pwm_get_clock_division(rp, period_ns);
-	if (div < 0)
-		return div;
-
-	/*
-	 * Let the core driver set pwm->period if disabled and duty_ns == 0.
-	 * But, this driver should prevent to set the new duty_ns if current
-	 * duty_cycle is not set
-	 */
-	if (!pwm_is_enabled(pwm) && !duty_ns && !pwm->state.duty_cycle)
-		return 0;
-
-	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, RCAR_PWMCR_SYNC, RCAR_PWMCR);
-
-	ret = rcar_pwm_set_counter(rp, div, duty_ns, period_ns);
-	if (!ret)
-		rcar_pwm_set_clock_control(rp, div);
-
-	/* The SYNC should be set to 0 even if rcar_pwm_set_counter failed */
-	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
-
-	return ret;
-}
-
-static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
-{
-	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
 	u32 value;
 
 	/* Don't enable the PWM device if CYC0 or PH0 is 0 */
@@ -185,10 +154,8 @@ static int rcar_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	return 0;
 }
 
-static void rcar_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+static void rcar_pwm_disable(struct rcar_pwm_chip *rp)
 {
-	struct rcar_pwm_chip *rp = to_rcar_pwm_chip(chip);
-
 	rcar_pwm_update(rp, RCAR_PWMCR_EN0, 0, RCAR_PWMCR);
 }
 
@@ -218,10 +185,10 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	rcar_pwm_update(rp, RCAR_PWMCR_SYNC, 0, RCAR_PWMCR);
 
 	if (!ret && state->enabled)
-		ret = rcar_pwm_enable(chip, pwm);
+		ret = rcar_pwm_enable(rp);
 
 	if (!state->enabled) {
-		rcar_pwm_disable(chip, pwm);
+		rcar_pwm_disable(rp);
 		ret = 0;
 	}
 
@@ -231,9 +198,6 @@ static int rcar_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 static const struct pwm_ops rcar_pwm_ops = {
 	.request = rcar_pwm_request,
 	.free = rcar_pwm_free,
-	.config = rcar_pwm_config,
-	.enable = rcar_pwm_enable,
-	.disable = rcar_pwm_disable,
 	.apply = rcar_pwm_apply,
 	.owner = THIS_MODULE,
 };
-- 
1.9.1

Re: [PATCH v2 3/4] pwm: rcar: remove legacy APIs

[Uwe Kleine-König]

On Tue, Jan 08, 2019 at 12:28:13PM +0900, Yoshihiro Shimoda wrote:
> This patch removes legacy APIs. Since rcar_pwm_{en,dis}able()
> functions are reused on "atomic" API, this patch changes
> the arguments of these functions. 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>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2 4/4] pwm: rcar: improve calculation of divider

[Yoshihiro Shimoda]

The rcar_pwm_get_clock_division() has a loop to calculate the divider,
but the value of div should be calculatable without a loop. So,
this patch improves it.

This algorithm is suggested by Uwe Kleine-König and Laurent Pinchart.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/pwm/pwm-rcar.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
index 6dbb70c..0498a93 100644
--- a/drivers/pwm/pwm-rcar.c
+++ b/drivers/pwm/pwm-rcar.c
@@ -8,6 +8,8 @@
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/log2.h>
+#include <linux/math64.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -68,19 +70,15 @@ static void rcar_pwm_update(struct rcar_pwm_chip *rp, u32 mask, u32 data,
 static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int period_ns)
 {
 	unsigned long clk_rate = clk_get_rate(rp->clk);
-	unsigned long long max; /* max cycle / nanoseconds */
-	unsigned int div;
+	u64 div, tmp;
 
 	if (clk_rate == 0)
 		return -EINVAL;
 
-	for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
-		max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
-			(1 << div);
-		do_div(max, clk_rate);
-		if (period_ns <= max)
-			break;
-	}
+	div = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
+	tmp = (u64)period_ns * clk_rate + div - 1;
+	tmp = div64_u64(tmp, div);
+	div = ilog2(tmp - 1) + 1;
 
 	return (div <= RCAR_PWM_MAX_DIVISION) ? div : -ERANGE;
 }
-- 
1.9.1

Re: [PATCH v2 4/4] pwm: rcar: improve calculation of divider

[Uwe Kleine-König]

On Tue, Jan 08, 2019 at 12:28:14PM +0900, Yoshihiro Shimoda wrote:
> The rcar_pwm_get_clock_division() has a loop to calculate the divider,
> but the value of div should be calculatable without a loop. So,
> this patch improves it.
> 
> This algorithm is suggested by Uwe Kleine-König and Laurent Pinchart.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH v2 4/4] pwm: rcar: improve calculation of divider

[Geert Uytterhoeven]

Hi Shimoda-san,

On Tue, Jan 8, 2019 at 4:31 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> The rcar_pwm_get_clock_division() has a loop to calculate the divider,
> but the value of div should be calculatable without a loop. So,
> this patch improves it.
>
> This algorithm is suggested by Uwe Kleine-König and Laurent Pinchart.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/pwm/pwm-rcar.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> index 6dbb70c..0498a93 100644
> --- a/drivers/pwm/pwm-rcar.c
> +++ b/drivers/pwm/pwm-rcar.c
> @@ -8,6 +8,8 @@
>  #include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> +#include <linux/log2.h>
> +#include <linux/math64.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -68,19 +70,15 @@ static void rcar_pwm_update(struct rcar_pwm_chip *rp, u32 mask, u32 data,
>  static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int period_ns)
>  {
>         unsigned long clk_rate = clk_get_rate(rp->clk);
> -       unsigned long long max; /* max cycle / nanoseconds */
> -       unsigned int div;
> +       u64 div, tmp;
>
>         if (clk_rate == 0)
>                 return -EINVAL;
>
> -       for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> -               max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> -                       (1 << div);
> -               do_div(max, clk_rate);
> -               if (period_ns <= max)
> -                       break;
> -       }
> +       div = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;

As we have:

    #define NSEC_PER_SEC    1000000000L
    #define RCAR_PWM_MAX_CYCLE      1023

NSEC_PER_SEC is 64-bit on arm64, and 32-bit on arm32.

Hence you should use

    div = (u64)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;

to avoid overflow on arm32.

> +       tmp = (u64)period_ns * clk_rate + div - 1;
> +       tmp = div64_u64(tmp, div);
> +       div = ilog2(tmp - 1) + 1;
>
>         return (div <= RCAR_PWM_MAX_DIVISION) ? div : -ERANGE;
>  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v2 4/4] pwm: rcar: improve calculation of divider

[Yoshihiro Shimoda]

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, January 8, 2019 5:22 PM
> 
> Hi Shimoda-san,
> 
> On Tue, Jan 8, 2019 at 4:31 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > The rcar_pwm_get_clock_division() has a loop to calculate the divider,
> > but the value of div should be calculatable without a loop. So,
> > this patch improves it.
> >
> > This algorithm is suggested by Uwe Kleine-König and Laurent Pinchart.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/pwm/pwm-rcar.c | 16 +++++++---------
> >  1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-rcar.c b/drivers/pwm/pwm-rcar.c
> > index 6dbb70c..0498a93 100644
> > --- a/drivers/pwm/pwm-rcar.c
> > +++ b/drivers/pwm/pwm-rcar.c
> > @@ -8,6 +8,8 @@
> >  #include <linux/clk.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> > +#include <linux/log2.h>
> > +#include <linux/math64.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > @@ -68,19 +70,15 @@ static void rcar_pwm_update(struct rcar_pwm_chip *rp, u32 mask, u32 data,
> >  static int rcar_pwm_get_clock_division(struct rcar_pwm_chip *rp, int period_ns)
> >  {
> >         unsigned long clk_rate = clk_get_rate(rp->clk);
> > -       unsigned long long max; /* max cycle / nanoseconds */
> > -       unsigned int div;
> > +       u64 div, tmp;
> >
> >         if (clk_rate == 0)
> >                 return -EINVAL;
> >
> > -       for (div = 0; div <= RCAR_PWM_MAX_DIVISION; div++) {
> > -               max = (unsigned long long)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE *
> > -                       (1 << div);
> > -               do_div(max, clk_rate);
> > -               if (period_ns <= max)
> > -                       break;
> > -       }
> > +       div = NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> 
> As we have:
> 
>     #define NSEC_PER_SEC    1000000000L
>     #define RCAR_PWM_MAX_CYCLE      1023
> 
> NSEC_PER_SEC is 64-bit on arm64, and 32-bit on arm32.
> 
> Hence you should use
> 
>     div = (u64)NSEC_PER_SEC * RCAR_PWM_MAX_CYCLE;
> 
> to avoid overflow on arm32.

Thank you for the comment! I'll fix this.

Best regards,
Yoshihiro Shimoda

> > +       tmp = (u64)period_ns * clk_rate + div - 1;
> > +       tmp = div64_u64(tmp, div);
> > +       div = ilog2(tmp - 1) + 1;
> >
> >         return (div <= RCAR_PWM_MAX_DIVISION) ? div : -ERANGE;
> >  }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds