Re: [v3,PATCH 3/3] pwm: mtk_disp: implement .get_state()

[Uwe Kleine-König]

On Tue, Apr 06, 2021 at 05:57:42PM +0800, Rex-BC Chen wrote:
> implement get_state function for pwm-mtk-disp
> 
> Signed-off-by: Rex-BC Chen <rex-bc.chen@xxxxxxxxxxxx>
> Signed-off-by: Jitao Shi <jitao.shi@xxxxxxxxxxxx>

Ideally you S-o-b line is the last one to show the order in which this
patch went from one person to another.

> ---
>  drivers/pwm/pwm-mtk-disp.c | 46 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c
> index 502228adf718..166e0a8ca703 100644
> --- a/drivers/pwm/pwm-mtk-disp.c
> +++ b/drivers/pwm/pwm-mtk-disp.c
> @@ -179,8 +179,54 @@ static int mtk_disp_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	return mtk_disp_pwm_enable(chip, state);
>  }
>  
> +static void mtk_disp_pwm_get_state(struct pwm_chip *chip,
> +				   struct pwm_device *pwm,
> +				   struct pwm_state *state)
> +{
> +	struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip);
> +	u32 clk_div, period, high_width, con0, con1;
> +	u64 rate;
> +	int err;
> +
> +	err = clk_prepare_enable(mdp->clk_main);
> +	if (err < 0) {
> +		dev_err(chip->dev, "Can't enable mdp->clk_main: %d\n", err);
> +		return;
> +	}
> +	err = clk_prepare_enable(mdp->clk_mm);
> +	if (err < 0) {
> +		dev_err(chip->dev, "Can't enable mdp->clk_mm: %d\n", err);
> +		clk_disable_unprepare(mdp->clk_main);

As before: %pe please

> +		return;
> +	}
> +
> +	rate = clk_get_rate(mdp->clk_main);
> +
> +	con0 = readl(mdp->base + mdp->data->con0);
> +	con1 = readl(mdp->base + mdp->data->con1);
> +
> +	state->polarity = con0 & PWM_POLARITY ?
> +			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
> +	state->enabled = !!(con0 & BIT(0));
> +
> +	clk_div = (con0 & PWM_CLKDIV_MASK) >> PWM_CLKDIV_SHIFT;
> +	period = con1 & PWM_PERIOD_MASK;
> +	state->period = div_u64(period * (clk_div + 1) * NSEC_PER_SEC, rate);
> +	high_width = (con1 & PWM_HIGH_WIDTH_MASK) >> PWM_HIGH_WIDTH_SHIFT;
> +	state->duty_cycle = div_u64(high_width * (clk_div + 1) * NSEC_PER_SEC,
> +				    rate);

Please round up these divisions as in .apply() we're rounding down.
Otherwise .get_state isn't the inverse of .apply().

> +
> +	if (!state->enabled) {
> +		clk_disable_unprepare(mdp->clk_mm);
> +		clk_disable_unprepare(mdp->clk_main);

That's wrong, you enabled unconditionally so you should also disable
unconditionally. If you want to prevent that clocks of a running PWM are
disabled by the unused clk initcall, this must be done in .probe() to be
sure it runs exactly once and early enough.

> +	}
> +
> +	mdp->enabled = state->enabled;
> +}
> +

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature