linux-kernel - Re: [v3,PATCH 3/3] pwm: mtk_disp: implement .get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210406102756.4kncasxe4um3q6wi@pengutronix.de>
Date:   Tue, 6 Apr 2021 12:27:56 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Rex-BC Chen <rex-bc.chen@...iatek.com>
Cc:     thierry.reding@...il.com, lee.jones@...aro.org,
        matthias.bgg@...il.com, linux-pwm@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        Jitao Shi <jitao.shi@...iatek.com>
Subject: Re: [v3,PATCH 3/3] pwm: mtk_disp: implement .get_state()

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@...iatek.com>
> Signed-off-by: Jitao Shi <jitao.shi@...iatek.com>

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)