Re: [PATCH 1/3] pwm: imx27: track clock enable/disable to simplify code

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

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

Hello,

On Wed, Sep 09, 2020 at 03:07:37PM +0200, Marco Felsch wrote:
> Introduce a simple clock state so we can enable/disable the clock
> without the need to check if we are running or not.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/pwm/pwm-imx27.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

This doesn't suggest to be more simple.

> @@ -223,6 +234,10 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	int ret;
>  	u32 cr;
>  
> +	ret = pwm_imx27_clk_prepare_enable(imx);
> +	if (ret)
> +		return ret;
> +
>  	pwm_get_state(pwm, &cstate);
>  
>  	clkrate = clk_get_rate(imx->clk_per);
> @@ -254,10 +269,6 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	if (cstate.enabled) {
>  		pwm_imx27_wait_fifo_slot(chip, pwm);
>  	} else {
> -		ret = pwm_imx27_clk_prepare_enable(imx);
> -		if (ret)
> -			return ret;
> -
>  		pwm_imx27_sw_reset(chip);
>  	}

There are two clocks, I assume one if for register access and the other
to actually drive the PWM. With that what I would consider simple is to
enable the register clock at the start of each function and disable it
at the end. And for the hardware clock enable it when the hardware
should be enabled and disable it when it should be disabled.

Probably this doesn't reduce the line count, too, but it makes the
function more efficient (if this is measurable at all).

If you want to keep the pwm_imx27_clk_prepare_enable() function that
handles both clocks, just calling pwm_imx27_clk_prepare_enable
unconditionally at the function entry and
pwm_imx27_clk_disable_unprepare at the end should be easier and not
require the .on member in the driver struct.

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 --]