Re: [3/3] pwm: imx: Implement get_state() function for hardware readout

[Uwe Kleine-König]

Hello,

On Mon, Oct 01, 2018 at 04:19:48PM +0200, Michal VokÃÄ wrote:
> Implement the get_state() function and set the initial state to reflect
> real state of the hardware. This allows to keep the PWM running if it was
> enabled in bootloader. It is very similar to the GPIO behavior. GPIO pin
> set as output in bootloader keep the same setting in Linux unless it is
> reconfigured.
> 
> If we find the PWM block enabled we need to prepare and enable its source
> clock otherwise the clock will be disabled late in the boot as unused.
> That will leave the PWM in enabled state but with disabled clock. That has
> a side effect that the PWM output is left at its current level at which
> the clock was disabled. It is totally non-deterministic and it may be LOW
> or HIGH.

Does this problem still exist if the pwm-imx driver is a module?

> Signed-off-by: Michal VokÃÄ <michal.vokac@xxxxxxxxx>
> ---
>  drivers/pwm/pwm-imx.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-imx.c b/drivers/pwm/pwm-imx.c
> index 7a4907b..6cd3b72 100644
> --- a/drivers/pwm/pwm-imx.c
> +++ b/drivers/pwm/pwm-imx.c
> @@ -83,6 +83,9 @@
>  
>  #define MX3_PWM_SWR_LOOP		5
>  
> +/* PWMPR register value of 0xffff has the same effect as 0xfffe */
> +#define MX3_PWMPR_MAX			0xfffe
> +
>  struct imx_chip {
>  	struct clk	*clk_per;
>  
> @@ -93,6 +96,55 @@ struct imx_chip {
>  
>  #define to_imx_chip(chip)	container_of(chip, struct imx_chip, chip)
>  
> +static void imx_pwm_get_state(struct pwm_chip *chip,
> +		struct pwm_device *pwm, struct pwm_state *state)

broken alignment.

> +{
> +	struct imx_chip *imx = to_imx_chip(chip);
> +	u32 period, prescaler, pwm_clk, ret, val;
> +	u64 tmp;
> +
> +	val = readl(imx->mmio_base + MX3_PWMCR);
> +
> +	if (val & MX3_PWMCR_EN) {
> +		state->enabled = true;
> +		ret = clk_prepare_enable(imx->clk_per);
> +		if (ret)
> +			return;
> +	} else {
> +		state->enabled = false;
> +	}
> +
> +	switch (FIELD_GET(MX3_PWMCR_POUTC, val)) {
> +	case MX3_PWMCR_POUTC_NORMAL:
> +		state->polarity = PWM_POLARITY_NORMAL;
> +		break;
> +	case MX3_PWMCR_POUTC_INVERTED:
> +		state->polarity = PWM_POLARITY_INVERSED;
> +		break;
> +	default:
> +		dev_warn(chip->dev, "can't set polarity, output disconnected");

Should we return an error here?

> +	}
> +
> +	prescaler = MX3_PWMCR_PRESCALER_GET(val);
> +	pwm_clk = clk_get_rate(imx->clk_per);
> +	pwm_clk = DIV_ROUND_CLOSEST_ULL(pwm_clk, prescaler);
> +	val = readl(imx->mmio_base + MX3_PWMPR);

It would be more cautionous to not rely on the reserved bits to read as
zero. So I suggest to mask the value with 0xffff.

> +	period = val >= MX3_PWMPR_MAX ? MX3_PWMPR_MAX : val;
> +
> +	/* PWMOUT (Hz) = PWMCLK / (PWMPR + 2) */
> +	tmp = NSEC_PER_SEC * (u64)(period + 2);
> +	state->period = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);

Would it make sense to introduce a policy about how to round in this
case? (Similarily for .apply?) This is of course out of scope for this
patch.

> +
> +	/* PWMSAR can be read only if PWM is enabled */
> +	if (state->enabled) {
> +		val = readl(imx->mmio_base + MX3_PWMSAR);
> +		tmp = NSEC_PER_SEC * (u64)(val);
> +		state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, pwm_clk);
> +	} else {
> +		state->duty_cycle = 0;
> +	}
> +}
> +
>  static int imx_pwm_config_v1(struct pwm_chip *chip,
>  		struct pwm_device *pwm, int duty_ns, int period_ns)
>  {
> @@ -272,6 +324,7 @@ static const struct pwm_ops imx_pwm_ops_v1 = {
>  
>  static const struct pwm_ops imx_pwm_ops_v2 = {
>  	.apply = imx_pwm_apply_v2,
> +	.get_state = imx_pwm_get_state,
>  	.owner = THIS_MODULE,
>  };
>

Attachment: signature.asc
Description: PGP signature