[PATCH v2] pwm: atmel-tcb: Implement .apply callback

[PATCH v2] pwm: atmel-tcb: Implement .apply callback

[Uwe Kleine-König]

This is just pushing down the core's compat code down into the driver using
the legacy callback nearly unchanged. The call to .enable() was just
dropped from .config() because .apply() calls it unconditionally.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Changes since (implicit) v1:

 - fix typo in Subject (s/tcp/tcb/)
 - Add S-o-b

 drivers/pwm/pwm-atmel-tcb.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
index ee70a615532b..4d2253f3048c 100644
--- a/drivers/pwm/pwm-atmel-tcb.c
+++ b/drivers/pwm/pwm-atmel-tcb.c
@@ -362,20 +362,37 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	tcbpwm->div = i;
 	tcbpwm->duty = duty;
 
-	/* If the PWM is enabled, call enable to apply the new conf */
-	if (pwm_is_enabled(pwm))
-		atmel_tcb_pwm_enable(chip, pwm);
-
 	return 0;
 }
 
+static int atmel_tcb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			       const struct pwm_state *state)
+{
+	int duty_cycle, period;
+	int ret;
+
+	/* This function only sets a flag in driver data */
+	atmel_tcb_pwm_set_polarity(chip, pwm, state->polarity);
+
+	if (!state->enabled) {
+		atmel_tcb_pwm_disable(chip, pwm);
+		return 0;
+	}
+
+	period = state->period < INT_MAX ? state->period : INT_MAX;
+	duty_cycle = state->duty_cycle < INT_MAX ? state->duty_cycle : INT_MAX;
+
+	ret = atmel_tcb_pwm_config(chip, pwm, duty_cycle, period);
+	if (ret)
+		return ret;
+
+	return atmel_tcb_pwm_enable(chip, pwm);
+}
+
 static const struct pwm_ops atmel_tcb_pwm_ops = {
 	.request = atmel_tcb_pwm_request,
 	.free = atmel_tcb_pwm_free,
-	.config = atmel_tcb_pwm_config,
-	.set_polarity = atmel_tcb_pwm_set_polarity,
-	.enable = atmel_tcb_pwm_enable,
-	.disable = atmel_tcb_pwm_disable,
+	.apply = atmel_tcb_pwm_apply,
 	.owner = THIS_MODULE,
 };
 
-- 
2.30.1

Re: [PATCH v2] pwm: atmel-tcb: Implement .apply callback

[Thierry Reding]

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

On Mon, Mar 08, 2021 at 10:50:12AM +0100, Uwe Kleine-König wrote:
> This is just pushing down the core's compat code down into the driver using
> the legacy callback nearly unchanged. The call to .enable() was just
> dropped from .config() because .apply() calls it unconditionally.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Changes since (implicit) v1:
> 
>  - fix typo in Subject (s/tcp/tcb/)
>  - Add S-o-b
> 
>  drivers/pwm/pwm-atmel-tcb.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-atmel-tcb.c b/drivers/pwm/pwm-atmel-tcb.c
> index ee70a615532b..4d2253f3048c 100644
> --- a/drivers/pwm/pwm-atmel-tcb.c
> +++ b/drivers/pwm/pwm-atmel-tcb.c
> @@ -362,20 +362,37 @@ static int atmel_tcb_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	tcbpwm->div = i;
>  	tcbpwm->duty = duty;
>  
> -	/* If the PWM is enabled, call enable to apply the new conf */
> -	if (pwm_is_enabled(pwm))
> -		atmel_tcb_pwm_enable(chip, pwm);
> -
>  	return 0;
>  }
>  
> +static int atmel_tcb_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			       const struct pwm_state *state)
> +{
> +	int duty_cycle, period;
> +	int ret;
> +
> +	/* This function only sets a flag in driver data */
> +	atmel_tcb_pwm_set_polarity(chip, pwm, state->polarity);
> +
> +	if (!state->enabled) {
> +		atmel_tcb_pwm_disable(chip, pwm);
> +		return 0;
> +	}
> +
> +	period = state->period < INT_MAX ? state->period : INT_MAX;
> +	duty_cycle = state->duty_cycle < INT_MAX ? state->duty_cycle : INT_MAX;
> +
> +	ret = atmel_tcb_pwm_config(chip, pwm, duty_cycle, period);
> +	if (ret)
> +		return ret;
> +
> +	return atmel_tcb_pwm_enable(chip, pwm);
> +}

Given that this applies the state in a hardware specific manner, I think
the shortcut for the disable case is fine because the internal, SW state
remains consistent. That is, if someone were to reactivate the PWM, they
would be able to do so by just switching the "enabled" state to "true".

The fact that the period and duty-cycle changes aren't applied to the
hardware is irrelevant at this point because there are no observable
changes to the physical signal.

Just mentioning this as additional information to highlight what I said
earlier in the PCA driver thread and to the "glitch" patch that I
requested a change on.

Applied, thanks.

Thierry

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