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

Message ID 1546918094-13960-2-git-send-email-yoshihiro.shimoda.uh@renesas.com
State Superseded
Headers show
Series
  • [v2,1/4] pwm: rcar: Add support "atomic" API
Related show

Commit Message

Yoshihiro Shimoda Jan. 8, 2019, 3:28 a.m.
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(+)

Comments

Uwe Kleine-König Jan. 8, 2019, 7:43 a.m. | #1
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
Yoshihiro Shimoda Jan. 8, 2019, 8:11 a.m. | #2
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

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,
 };