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

[<Claudiu.Beznea@microchip.com>]

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>

>  
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel