[1/4] pwm: mxs: implement ->apply

In preparation for supporting setting the polarity, switch the driver
to support the ->apply method.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/pwm/pwm-mxs.c | 62 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

Hello Rasmus,

On Mon, Sep 23, 2019 at 10:13:45AM +0200, Rasmus Villemoes wrote:
> In preparation for supporting setting the polarity, switch the driver
> to support the ->apply method.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/pwm/pwm-mxs.c | 62 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> index 04c0f6b95c1a..c70c26a9ff68 100644
> --- a/drivers/pwm/pwm-mxs.c
> +++ b/drivers/pwm/pwm-mxs.c
> @@ -26,6 +26,7 @@
>  #define  PERIOD_PERIOD_MAX	0x10000
>  #define  PERIOD_ACTIVE_HIGH	(3 << 16)
>  #define  PERIOD_INACTIVE_LOW	(2 << 18)
> +#define  PERIOD_POLARITY_NORMAL	(PERIOD_ACTIVE_HIGH | PERIOD_INACTIVE_LOW)
>  #define  PERIOD_CDIV(div)	(((div) & 0x7) << 20)
>  #define  PERIOD_CDIV_MAX	8
>  
> @@ -41,6 +42,66 @@ struct mxs_pwm_chip {
>  
>  #define to_mxs_pwm_chip(_chip) container_of(_chip, struct mxs_pwm_chip, chip)
>  
> +static int mxs_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			 struct pwm_state *state)
> +{
> +	struct mxs_pwm_chip *mxs = to_mxs_pwm_chip(chip);
> +	int ret, div = 0;
> +	unsigned int period_cycles, duty_cycles;
> +	unsigned long rate;
> +	unsigned long long c;
> +
> +	if (state->polarity != PWM_POLARITY_NORMAL)
> +		return -ENOTSUPP;
> +
> +	rate = clk_get_rate(mxs->clk);
> +	while (1) {
> +		c = rate / cdiv[div];
> +		c = c * state->period;
> +		do_div(c, 1000000000);
> +		if (c < PERIOD_PERIOD_MAX)
> +			break;
> +		div++;
> +		if (div >= PERIOD_CDIV_MAX)
> +			return -EINVAL;
> +	}
> +
> +	period_cycles = c;
> +	c *= state->duty_cycle;
> +	do_div(c, state->period);
> +	duty_cycles = c;
> +
> +	/*
> +	 * If the PWM channel is disabled, make sure to turn on the clock
> +	 * before writing the register. Otherwise, keep it enabled.
> +	 */
> +	if (!pwm_is_enabled(pwm)) {
> +		ret = clk_prepare_enable(mxs->clk);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	writel(duty_cycles << 16,
> +	       mxs->base + PWM_ACTIVE0 + pwm->hwpwm * 0x20);
> +	writel(PERIOD_PERIOD(period_cycles) | PERIOD_POLARITY_NORMAL | PERIOD_CDIV(div),
> +	       mxs->base + PWM_PERIOD0 + pwm->hwpwm * 0x20);
> +
> +	if (state->enabled) {
> +		if (!pwm_is_enabled(pwm)) {
> +			/*
> +			 * The clock was enabled above. Just enable
> +			 * the channel in the control register.
> +			 */
> +			writel(1 << pwm->hwpwm, mxs->base + PWM_CTRL + SET);
> +		}
> +	} else {
> +		if (pwm_is_enabled(pwm))
> +			writel(1 << pwm->hwpwm, mxs->base + PWM_CTRL + CLR);
> +		clk_disable_unprepare(mxs->clk);
> +	}
> +	return 0;
> +}

Maybe it would be easier to review when converting from .config +
.enable + .disable to .apply in a single step. (Note this "maybe" is
honest, I'm not entirely sure.)

There is a bug: If the PWM is running at (say) period=100ms, duty=0ms
and we call
pwm_apply_state(pwm, { .enabled = false, duty=100000, period=1000000 });
the output might get high which it should not.

Also there is a bug already in .config: You are not supposed to call
clk_get_rate if the clk might be off.

Best regards
Uwe

On 23/09/2019 10.24, Uwe Kleine-König wrote:
> Hello Rasmus,
> 
> On Mon, Sep 23, 2019 at 10:13:45AM +0200, Rasmus Villemoes wrote:
>> In preparation for supporting setting the polarity, switch the driver
>> to support the ->apply method.
>>
> 
> Maybe it would be easier to review when converting from .config +
> .enable + .disable to .apply in a single step. (Note this "maybe" is
> honest, I'm not entirely sure.)

I tried to make .apply do exactly what the old sequence of calls from
the core to the individual methods would do, and for that it seemed a
little easier to keep the old methods around - but yes, I do need to be
more careful than that to provide the atomicity guarantee that the
legacy methods did not. It's also much easier to squash than to split,
so for now I'll leave them separate - if somebody prefers them squashed,
I'll do that.

> There is a bug: If the PWM is running at (say) period=100ms, duty=0ms
> and we call
> pwm_apply_state(pwm, { .enabled = false, duty=100000, period=1000000 });
> the output might get high which it should not.

Ah, yes. So I suppose that if we're changing from enabled to disabled,
we should simply disable it in the CTRL register before changing the
duty/period.

> Also there is a bug already in .config: You are not supposed to call
> clk_get_rate if the clk might be off.

Interesting, I didn't know that. So the prepare_enable logic needs to be
moved before we start computing the period/duty cycles. Do you know why
it has apparently worked so far? I would have thought such a rule would
be enforced by the clock framework, or at least produced a warning.

Thanks for the fast review. I'll wait a day or two to see if there are
other comments before sending out a v2.

Rasmus

Hello,

[expanded the recipents to include RMK and the clk list]

On Mon, Sep 23, 2019 at 11:04:39AM +0200, Rasmus Villemoes wrote:
> On 23/09/2019 10.24, Uwe Kleine-König wrote:
> > Also there is a bug already in .config: You are not supposed to call
> > clk_get_rate if the clk might be off.
> 
> Interesting, I didn't know that. So the prepare_enable logic needs to be
> moved before we start computing the period/duty cycles. Do you know why
> it has apparently worked so far? I would have thought such a rule would
> be enforced by the clock framework, or at least produced a warning.

FTR: This is documented in the kerneldoc code comment to clk_get_rate in
include/linux/clk.h.

Assuming this is relevant, it might indeed make sense to add a
WARN_ONCE for this.

Best regards
Uwe