Re: [PATCH v10 14/27] pwm: jz4740: Improve algorithm of clock calculation

From: Thierry Reding <thierry.reding@gmail.com>
To: Paul Cercueil <paul@crapouillou.net>
Cc: "Daniel Lezcano" <daniel.lezcano@linaro.org>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Ralf Baechle" <ralf@linux-mips.org>,
	"Paul Burton" <paul.burton@mips.com>,
	"James Hogan" <jhogan@kernel.org>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>,
	"Mathieu Malaterre" <malat@debian.org>,
	od@zcrc.me, linux-pwm@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-watchdog@vger.kernel.org, linux-mips@vger.kernel.org,
	linux-doc@vger.kernel.org, linux-clk@vger.kernel.org
Subject: Re: [PATCH v10 14/27] pwm: jz4740: Improve algorithm of clock calculation
Date: Mon, 4 Mar 2019 13:33:13 +0100	[thread overview]
Message-ID: <20190304123313.GF9040@ulmo> (raw)
In-Reply-To: <20190302233413.14813-15-paul@crapouillou.net>

[-- Attachment #1: Type: text/plain, Size: 2831 bytes --]

On Sat, Mar 02, 2019 at 08:34:00PM -0300, Paul Cercueil wrote:
> The previous algorithm hardcoded details about how the TCU clocks work.
> The new algorithm will use clk_round_rate to find the perfect clock rate
> for the PWM channel.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> Tested-by: Mathieu Malaterre <malat@debian.org>
> Tested-by: Artur Rojek <contact@artur-rojek.eu>
> ---
> 
> Notes:
>          v9: New patch
>     
>         v10: - New algorithm. Instead of starting with the maximum clock rate
>                and using clk_round_rate(rate - 1) to get the next (smaller)
>     	   clock, we compute the maximum rate we can use before the
>     	   register overflows, and apply it with clk_set_max_rate.
>     	   Then we read the new clock rate and compute the register values
>     	   of the period and duty from that.
>     	 - Use NSEC_PER_SEC instead of magic 1000000000 value
> 
>  drivers/pwm/pwm-jz4740.c | 49 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
> index c6136bd4434b..f497388fc818 100644
> --- a/drivers/pwm/pwm-jz4740.c
> +++ b/drivers/pwm/pwm-jz4740.c
> @@ -110,24 +110,45 @@ static int jz4740_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	struct jz4740_pwm_chip *jz4740 = to_jz4740(pwm->chip);
>  	struct clk *clk = jz4740->clks[pwm->hwpwm],
>  		   *parent_clk = clk_get_parent(clk);
> -	unsigned long rate, period, duty;
> +	unsigned long rate, parent_rate, period, duty;
>  	unsigned long long tmp;
> -	unsigned int prescaler = 0;
> +	int ret;
>  
> -	rate = clk_get_rate(parent_clk);
> -	tmp = (unsigned long long)rate * state->period;
> -	do_div(tmp, 1000000000);
> -	period = tmp;
> +	parent_rate = clk_get_rate(parent_clk);
> +
> +	jz4740_pwm_disable(chip, pwm);
> +
> +	/* Reset the clock to the maximum rate, and we'll reduce it if needed */
> +	ret = clk_set_rate(clk, parent_rate);
> +	if (ret)
> +		return ret;
>  
> -	while (period > 0xffff && prescaler < 6) {
> -		period >>= 2;
> -		rate >>= 2;
> -		++prescaler;
> +	/* Limit the clock to a maximum rate that still gives us a period value
> +	 * which fits in 16 bits.
> +	 */

Please use proper block-comment style.

> +	tmp = 0xffffull * NSEC_PER_SEC;
> +	do_div(tmp, state->period);
> +
> +	ret = clk_set_max_rate(clk, tmp);
> +	if (ret) {
> +		dev_err(chip->dev, "Unable to set max rate: %i\n", ret);
> +		return ret;
>  	}
>  
> -	if (prescaler == 6)
> -		return -EINVAL;
> +	/* Read back the clock rate, as it may have been modified by
> +	 * clk_set_max_rate()
> +	 */
> +	rate = clk_get_rate(clk);

That's a pretty neat trick. But also, please use proper block-comment
style.

Thierry

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