[PATCH 2/3] pwm: imx27: move static pwmcr values into probe() function

[Uwe Kleine-König]

Hello,

the usage of "static" in the subject is unclear. I guess you mean:

	pwm: imx27: setup some PWMCR bits in .probe()

On Wed, Sep 09, 2020 at 03:07:38PM +0200, Marco Felsch wrote:
> The STOPEN, DOZEN, WAITEN, DBGEN and the CLKSRC bit values never change.
> So it should be save to move this bit settings into probe() and change

s/save/safe/

> only the necessary bits during apply(). Therefore I added the
> pwm_imx27_update_bits() helper.
> 
> Furthermore the patch adds the support to reset the pwm device during
> probe() if the pwm device is disabled.

IMHO this needs a better motivation ...

> Both steps are required in preparation of the further patch which fixes
> the "pwm-disabled" state for inverted pwms.

... and should maybe go into this other patch?

> Signed-off-by: Marco Felsch <m.felsch at pengutronix.de>
> ---
>  drivers/pwm/pwm-imx27.c | 41 +++++++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index 3cf9f1774244..30388a9ece04 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -96,6 +96,16 @@ struct pwm_imx27_chip {
>  
>  #define to_pwm_imx27_chip(chip)	container_of(chip, struct pwm_imx27_chip, chip)
>  
> +static void pwm_imx27_update_bits(void __iomem *reg, u32 mask, u32 val)
> +{
> +	u32 tmp;
> +
> +	tmp = readl(reg);
> +	tmp &= ~mask;
> +	tmp |= val & mask;
> +	return writel(tmp, reg);
> +}
> +
>  static int pwm_imx27_clk_prepare_enable(struct pwm_imx27_chip *imx)
>  {
>  	int ret;
> @@ -183,10 +193,8 @@ static void pwm_imx27_get_state(struct pwm_chip *chip,
>  	pwm_imx27_clk_disable_unprepare(imx);
>  }
>  
> -static void pwm_imx27_sw_reset(struct pwm_chip *chip)
> +static void pwm_imx27_sw_reset(struct pwm_imx27_chip *imx, struct device *dev)
>  {
> -	struct pwm_imx27_chip *imx = to_pwm_imx27_chip(chip);
> -	struct device *dev = chip->dev;

This change is fine, but it does belong into a separate patch.

>  	int wait_count = 0;
>  	u32 cr;
>  
> @@ -232,7 +240,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	unsigned long long c;
>  	unsigned long long clkrate;
>  	int ret;
> -	u32 cr;
> +	u32 cr, mask;
>  
>  	ret = pwm_imx27_clk_prepare_enable(imx);
>  	if (ret)
> @@ -269,7 +277,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (cstate.enabled) {
>  		pwm_imx27_wait_fifo_slot(chip, pwm);
>  	} else {
> -		pwm_imx27_sw_reset(chip);
> +		pwm_imx27_sw_reset(imx, chip->dev);
>  	}
>  
>  	writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> @@ -281,10 +289,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 */
>  	imx->duty_cycle = duty_cycles;
>  
> -	cr = MX3_PWMCR_PRESCALER_SET(prescale) |
> -	     MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> -	     FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH) |
> -	     MX3_PWMCR_DBGEN;
> +	cr = MX3_PWMCR_PRESCALER_SET(prescale);
>  
>  	if (state->polarity == PWM_POLARITY_INVERSED)
>  		cr |= FIELD_PREP(MX3_PWMCR_POUTC,
> @@ -293,7 +298,9 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (state->enabled)
>  		cr |= MX3_PWMCR_EN;
>  
> -	writel(cr, imx->mmio_base + MX3_PWMCR);
> +	mask = MX3_PWMCR_PRESCALER | MX3_PWMCR_POUTC | MX3_PWMCR_EN;
> +
> +	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, cr);
>  
>  	if (!state->enabled)
>  		pwm_imx27_clk_disable_unprepare(imx);
> @@ -364,10 +371,20 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	/* keep clks on if pwm is running */
> +	/* Keep clks on and pwm settings unchanged if the PWM is already running */
>  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
> -	if (!(pwmcr & MX3_PWMCR_EN))
> +	if (!(pwmcr & MX3_PWMCR_EN)) {
> +		u32 mask;
> +
> +		pwm_imx27_sw_reset(imx, &pdev->dev);
> +		mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +			MX3_PWMCR_DBGEN | MX3_PWMCR_CLKSRC;
> +		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +			MX3_PWMCR_DBGEN |
> +			FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> +		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
>  		pwm_imx27_clk_disable_unprepare(imx);
> +	}

So you don't enforce MX3_PWMCR_STOPEN (and the others) if the PWM is
already running. Is this by design?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20200921/3a6248f6/attachment.sig>