linux-kernel - Re: [v3,PATCH 2/3] pwm: mtk_disp: convert the driver to atomic API

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: <20210406101936.uscvtvrd6opm5pgf@pengutronix.de>
Date:   Tue, 6 Apr 2021 12:19:36 +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 2/3] pwm: mtk_disp: convert the driver to atomic API

Hello,

On Tue, Apr 06, 2021 at 05:57:41PM +0800, Rex-BC Chen wrote:
> @@ -84,33 +86,47 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * period = (PWM_CLK_RATE * period_ns) / (10^9 * (clk_div + 1)) - 1
>  	 * high_width = (PWM_CLK_RATE * duty_ns) / (10^9 * (clk_div + 1))
>  	 */
> +	if (!mdp->enabled) {
> +		err = clk_prepare_enable(mdp->clk_main);
> +		if (err < 0) {
> +			dev_err(chip->dev, "Can't enable mdp->clk_main: %d\n",
> +				err);

Please use %pe, as this yields better readable error messages.

Also it might be sensible to first use the fact that (without patch 1
from this series) the clocks are always on and then rework the clk usage
in a separate patch.

> +			return err;
> +		}
> +		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);
> +			return err;
> +		}
> +	}
>  	rate = clk_get_rate(mdp->clk_main);
> -	clk_div = div_u64(rate * period_ns, NSEC_PER_SEC) >>
> +	clk_div = div_u64(rate * state->period, NSEC_PER_SEC) >>
>  			  PWM_PERIOD_BIT_WIDTH;

rate * state->period might overflow, it would be great if this could be
catched. (But I don't consider this a stopper for this series.)

> -	if (clk_div > PWM_CLKDIV_MAX)
> +	if (clk_div > PWM_CLKDIV_MAX) {
> +		dev_err(chip->dev, "clock rate is too high: rate = %d Hz\n",
> +			rate);

rate is an u64, %d isn't the right format for it. Doesn't this result in
a compiler warning?

> +		clk_disable_unprepare(mdp->clk_mm);
> +		clk_disable_unprepare(mdp->clk_main);
>  		return -EINVAL;
> -
> +	}
>  	div = NSEC_PER_SEC * (clk_div + 1);
> -	period = div64_u64(rate * period_ns, div);
> +	period = div64_u64(rate * state->period, div);
>  	if (period > 0)
>  		period--;
>  
> -	high_width = div64_u64(rate * duty_ns, div);
> +	high_width = div64_u64(rate * state->duty_cycle, div);
>  	value = period | (high_width << PWM_HIGH_WIDTH_SHIFT);
> -
> -	err = clk_enable(mdp->clk_main);
> -	if (err < 0)
> -		return err;
> -
> -	err = clk_enable(mdp->clk_mm);
> -	if (err < 0) {
> -		clk_disable(mdp->clk_main);
> -		return err;
> -	}
> +	polarity = 0;
> +	if (state->polarity == PWM_POLARITY_INVERSED)
> +		polarity = PWM_POLARITY;

I'm unsure if support for polarity should be added en passant in this
patch. Maybe it would be clearer to add is separately.

>  	mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
>  				 PWM_CLKDIV_MASK,
>  				 clk_div << PWM_CLKDIV_SHIFT);
> +	mtk_disp_pwm_update_bits(mdp, mdp->data->con0,
> +				 PWM_POLARITY, polarity);
>  	mtk_disp_pwm_update_bits(mdp, mdp->data->con1,
>  				 PWM_PERIOD_MASK | PWM_HIGH_WIDTH_MASK,
>  				 value);

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)