From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Rex-BC Chen <rex-bc.chen@mediatek.com>
Cc: thierry.reding@gmail.com, lee.jones@linaro.org,
matthias.bgg@gmail.com, linux-pwm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
Project_Global_Chrome_Upstream_Group@mediatek.com,
Jitao Shi <jitao.shi@mediatek.com>
Subject: Re: [v3,PATCH 2/3] pwm: mtk_disp: convert the driver to atomic API
Date: Tue, 6 Apr 2021 12:19:36 +0200 [thread overview]
Message-ID: <20210406101936.uscvtvrd6opm5pgf@pengutronix.de> (raw)
In-Reply-To: <1617703062-4251-3-git-send-email-rex-bc.chen@mediatek.com>
[-- Attachment #1.1: Type: text/plain, Size: 2969 bytes --]
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/ |
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-04-06 10:21 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-06 9:57 [v3,PATCH 0/3] Convert the mtk_disp driver to aotmic API Rex-BC Chen
2021-04-06 9:57 ` [v3,PATCH 1/3] pwm: mtk_disp: clear the clock operations Rex-BC Chen
2021-04-06 10:07 ` Uwe Kleine-König
2021-04-06 9:57 ` [v3,PATCH 2/3] pwm: mtk_disp: convert the driver to atomic API Rex-BC Chen
2021-04-06 10:19 ` Uwe Kleine-König [this message]
2021-04-06 14:37 ` kernel test robot
2021-04-06 14:52 ` kernel test robot
2021-04-06 9:57 ` [v3,PATCH 3/3] pwm: mtk_disp: implement .get_state() Rex-BC Chen
2021-04-06 10:27 ` Uwe Kleine-König
2021-04-09 12:24 ` Thierry Reding
2021-04-09 21:52 ` Uwe Kleine-König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210406101936.uscvtvrd6opm5pgf@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=Project_Global_Chrome_Upstream_Group@mediatek.com \
--cc=jitao.shi@mediatek.com \
--cc=lee.jones@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-pwm@vger.kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=rex-bc.chen@mediatek.com \
--cc=thierry.reding@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).