Re: [PATCH v2 3/5] pwm: imx27: reset the PWM if it is not running - Uwe Kleine-König

From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Marco Felsch <m.felsch@pengutronix.de>
Cc: thierry.reding@gmail.com, lee.jones@linaro.org,
	shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com,
	linux-imx@nxp.com, Anson.Huang@nxp.com, michal.vokac@ysoft.com,
	l.majewski@majess.pl, linux-pwm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org, kernel@pengutronix.de
Subject: Re: [PATCH v2 3/5] pwm: imx27: reset the PWM if it is not running
Date: Mon, 28 Sep 2020 09:30:19 +0200	[thread overview]
Message-ID: <20200928073019.led4eyfl3emv6aau@pengutronix.de> (raw)
In-Reply-To: <20200925155330.32301-4-m.felsch@pengutronix.de>

[-- Attachment #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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]