[v2,6/6] pwm: atmel: implement .get_state()

This function reads back the configured parameters from the hardware. As
.apply rounds down (mostly) I'm rounding up in .get_state() to achieve
that applying a state just read from hardware is a no-op.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
New in v2

 drivers/pwm/pwm-atmel.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

On 24.08.2019 03:10, Uwe Kleine-König wrote:
> External E-Mail
> 
> 
> This function reads back the configured parameters from the hardware. As
> .apply rounds down (mostly) I'm rounding up in .get_state() to achieve
> that applying a state just read from hardware is a no-op.

Since this read is only at probing, at least for the moment, and, as far as
I remember, the idea w/ .get_state was to reflect in Linux the states of
PWMs that were setup before Linux takes control (e.g. PWMs setup in
bootloaders) I think it would no problem if it would be no-ops in this
scenario. In case of run-time state retrieval, pwm_get_state() should be
enough. If one would get the state previously saved w/ this .get_state API
he/she would change it, then it would apply the changes to the hardware. No
changes of PWM state would be anyway skipped from the beginning, in
pwm_apply_state() by this code:

        if (state->period == pwm->state.period &&
            state->duty_cycle == pwm->state.duty_cycle &&
	    state->polarity == pwm->state.polarity &&
            state->enabled == pwm->state.enabled)
		return 0;

But maybe I'm missing something.

> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> New in v2
> 
>  drivers/pwm/pwm-atmel.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 152c2b7dd6df..f788501ab6ca 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -295,8 +295,47 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return 0;
>  }
>  
> +static void atmel_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				struct pwm_state *state)
> +{
> +	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
> +	u32 sr, cmr;
> +
> +	sr = atmel_pwm_readl(atmel_pwm, PWM_SR);
> +	cmr = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR);
> +
> +	if (sr & (1 << pwm->hwpwm)) {
> +		unsigned long rate = clk_get_rate(atmel_pwm->clk);
> +		u32 cdty, cprd, pres; 

There is a whitespace at the end of this line.

> +		u64 tmp;
> +
> +		pres = cmr & PWM_CMR_CPRE_MSK;
> +
> +		cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.period);

If this is possible, please try to keep it at 80 chars per line. In my
opinion this still looks good:
		cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm,
					  atmel_pwm->data->regs.period);

> +		tmp = (u64)cprd * NSEC_PER_SEC;
> +		tmp <<= pres;
> +		state->period = DIV64_U64_ROUND_UP(tmp, rate);
> +
> +		cdty = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, atmel_pwm->data->regs.duty);

Ditto.

> +		tmp = (u64)cdty * NSEC_PER_SEC;
> +		tmp <<= pres;
> +		state->duty_cycle = DIV64_U64_ROUND_UP(tmp, rate);
> +
> +		state->enabled = true;
> +	} else {
> +		state->enabled = false;
> +	}
> +
> +	if (cmr & PWM_CMR_CPOL)
> +		state->polarity = PWM_POLARITY_INVERSED;
> +	else
> +		state->polarity = PWM_POLARITY_NORMAL;
> +
> +}
> +
>  static const struct pwm_ops atmel_pwm_ops = {
>  	.apply = atmel_pwm_apply,
> +	.get_state = atmel_pwm_get_state,
>  	.owner = THIS_MODULE,
>  };

Other than the minor things above,
Acked-by: Claudiu Beznea <claudiu.beznea@microchip.com>

>  
>

Hello Claudiu,

On Wed, Aug 28, 2019 at 10:26:18AM +0000, Claudiu.Beznea@microchip.com wrote:
> On 24.08.2019 03:10, Uwe Kleine-König wrote:
> > External E-Mail
> > This function reads back the configured parameters from the hardware. As
> > .apply rounds down (mostly) I'm rounding up in .get_state() to achieve
> > that applying a state just read from hardware is a no-op.
> 
> Since this read is only at probing, at least for the moment, and, as far as

Yes, up to now .get_state() is only called at probing time. There is a
patch series (by me) on the list that changes that. (Though I'm not
entirely sure this is a good idea. Will comment my doubts in that thread
later.)

> I remember, the idea w/ .get_state was to reflect in Linux the states of
> PWMs that were setup before Linux takes control (e.g. PWMs setup in
> bootloaders) I think it would no problem if it would be no-ops in this
> scenario.

IMHO it should be a no-op.

> In case of run-time state retrieval, pwm_get_state() should be
> enough. If one would get the state previously saved w/ this .get_state API
> he/she would change it, then it would apply the changes to the hardware. No
> changes of PWM state would be anyway skipped from the beginning, in
> pwm_apply_state() by this code:
> 
>         if (state->period == pwm->state.period &&
>             state->duty_cycle == pwm->state.duty_cycle &&
> 	    state->polarity == pwm->state.polarity &&
>             state->enabled == pwm->state.enabled)
> 		return 0;
> 
> But maybe I'm missing something.

There is a problem I want to solve generally, not only for the atmel driver.

For example I consider it "expected" that

	s1 = pwm_get_state(pwm)
	pwm_apply_state(pwm, s2)
	pwm_apply_state(pwm, s1)

ends in the same configuration as it started. For that it is necessary
(even for the atmel driver with the guard you pointed out above) to
round up in .get_state if .apply rounds down.

Best regards
Uwe