Re: [PATCH v2 3/5] pwm: imx27: reset the PWM if it is not running

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3550 bytes --]

On Fri, Sep 25, 2020 at 05:53:28PM +0200, Marco Felsch wrote:
> Trigger a software reset during probe to clear the FIFO and reset the
> register values to their default. According the datasheet the DBGEN,
> STOPEN, DOZEN and WAITEN bits should be untouched by the software reset
> but this is not the case.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
> v2:
> - new patch
> 
>  drivers/pwm/pwm-imx27.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> index b761764b8375..3b6bcd8d58b7 100644
> --- a/drivers/pwm/pwm-imx27.c
> +++ b/drivers/pwm/pwm-imx27.c
> @@ -183,10 +183,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;
>  	int wait_count = 0;
>  	u32 cr;

This is an unrelated hunk that I don't expect to result in any changes
in the code. If you consider it better this way, you should at least
mention it in the commit log.

> @@ -266,7 +264,7 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (imx->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);
>  	writel(period_cycles, imx->mmio_base + MX3_PWMPR);
> @@ -370,19 +368,23 @@ static int pwm_imx27_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> -	       MX3_PWMCR_DBGEN;
> -	pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> -		MX3_PWMCR_DBGEN;
> -	pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
> -
>  	/* keep clks on and clk settings unchanged if pwm is running */
>  	pwmcr = readl(imx->mmio_base + MX3_PWMCR);
>  	if (!(pwmcr & MX3_PWMCR_EN)) {
> -		mask = MX3_PWMCR_CLKSRC;
> -		pwmcr = FIELD_PREP(MX3_PWMCR_CLKSRC, MX3_PWMCR_CLKSRC_IPG_HIGH);
> +		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);
> +	} else {
> +		mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +			MX3_PWMCR_DBGEN;
> +		pwmcr = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN |
> +			MX3_PWMCR_DBGEN;
> +		pwm_imx27_update_bits(imx->mmio_base + MX3_PWMCR, mask, pwmcr);
>  	}

IMHO this is worse than the stuff I suggested for one of the earlier
patches because there is much repetition. I'd put

	mask = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
	value = MX3_PWMCR_STOPEN | MX3_PWMCR_DOZEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;

before the if and just modify as necessary in the first branch of the
if.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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