* [PATCH v4 0/2] clocks aren't disable when pwm_mtk_disp suspend @ 2019-12-17 4:02 Jitao Shi 2019-12-17 4:02 ` [PATCH v4 1/2] pwm: mtk_disp: fix pwm clocks not poweroff when disable pwm Jitao Shi 2019-12-17 4:02 ` [PATCH v4 2/2] pwm: mtk_disp: keep the trigger register after pwm setting Jitao Shi 0 siblings, 2 replies; 6+ messages in thread From: Jitao Shi @ 2019-12-17 4:02 UTC (permalink / raw) To: Thierry Reding, Uwe Kleine-Koenig, Matthias Brugger, linux-pwm, linux-arm-kernel, linux-kernel, CK Hu Cc: Jitao Shi, sj.huang, linux-mediatek Changes since to v3: - Add prefix "pwm: mtk_disp" in commit msg title. Changes since to v2: - Edit commit msg. Changes since to v1: - Edit commit msg. - Remove the register trigger in probe. - Rebase to v5.5-rc1. Changes in patches: - match pwm_mtk_disp clock when suspend/resume - trigger pwm_mtk_disp reg working after config Jitao Shi (2): pwm: mtk_disp: fix pwm clocks not poweroff when disable pwm pwm: mtk_disp: keep the trigger register after pwm setting. drivers/pwm/pwm-mtk-disp.c | 63 ++++++++++++-------------------------- 1 file changed, 19 insertions(+), 44 deletions(-) -- 2.21.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 1/2] pwm: mtk_disp: fix pwm clocks not poweroff when disable pwm 2019-12-17 4:02 [PATCH v4 0/2] clocks aren't disable when pwm_mtk_disp suspend Jitao Shi @ 2019-12-17 4:02 ` Jitao Shi 2020-01-20 11:52 ` Thierry Reding 2020-01-20 14:31 ` Uwe Kleine-König 2019-12-17 4:02 ` [PATCH v4 2/2] pwm: mtk_disp: keep the trigger register after pwm setting Jitao Shi 1 sibling, 2 replies; 6+ messages in thread From: Jitao Shi @ 2019-12-17 4:02 UTC (permalink / raw) To: Thierry Reding, Uwe Kleine-Koenig, Matthias Brugger, linux-pwm, linux-arm-kernel, linux-kernel, CK Hu Cc: Jitao Shi, sj.huang, linux-mediatek The clocks of pwm are still in prepare state when disable pwm. Because the clocks is prepared in mtk_disp_pwm_probe() and unprepared in mtk_disp_pwm_remove(). clk_prepare and clk_unprepare aren't called by mtk_disp_pwm_enable() and mtk_disp_pwm_disable(). Modified: So clk_enable() is instread with clk_prepare_enable(). clk_disable() is instread with clk_disable_unprepare(). And remove the clk_prepare() from mtk_disp_pwm_probe(), remove the clk_unprepare from mtk_disp_pwm_remove(). Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> --- drivers/pwm/pwm-mtk-disp.c | 43 +++++++++++--------------------------- 1 file changed, 12 insertions(+), 31 deletions(-) diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c index 83b8be0209b7..c7b14acc9316 100644 --- a/drivers/pwm/pwm-mtk-disp.c +++ b/drivers/pwm/pwm-mtk-disp.c @@ -98,13 +98,13 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, high_width = div64_u64(rate * duty_ns, div); value = period | (high_width << PWM_HIGH_WIDTH_SHIFT); - err = clk_enable(mdp->clk_main); + err = clk_prepare_enable(mdp->clk_main); if (err < 0) return err; - err = clk_enable(mdp->clk_mm); + err = clk_prepare_enable(mdp->clk_mm); if (err < 0) { - clk_disable(mdp->clk_main); + clk_disable_unprepare(mdp->clk_main); return err; } @@ -124,8 +124,8 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, 0x0); } - clk_disable(mdp->clk_mm); - clk_disable(mdp->clk_main); + clk_disable_unprepare(mdp->clk_mm); + clk_disable_unprepare(mdp->clk_main); return 0; } @@ -135,13 +135,13 @@ static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip); int err; - err = clk_enable(mdp->clk_main); + err = clk_prepare_enable(mdp->clk_main); if (err < 0) return err; - err = clk_enable(mdp->clk_mm); + err = clk_prepare_enable(mdp->clk_mm); if (err < 0) { - clk_disable(mdp->clk_main); + clk_disable_unprepare(mdp->clk_main); return err; } @@ -158,8 +158,8 @@ static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask, 0x0); - clk_disable(mdp->clk_mm); - clk_disable(mdp->clk_main); + clk_disable_unprepare(mdp->clk_mm); + clk_disable_unprepare(mdp->clk_main); } static const struct pwm_ops mtk_disp_pwm_ops = { @@ -194,14 +194,6 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev) if (IS_ERR(mdp->clk_mm)) return PTR_ERR(mdp->clk_mm); - ret = clk_prepare(mdp->clk_main); - if (ret < 0) - return ret; - - ret = clk_prepare(mdp->clk_mm); - if (ret < 0) - goto disable_clk_main; - mdp->chip.dev = &pdev->dev; mdp->chip.ops = &mtk_disp_pwm_ops; mdp->chip.base = -1; @@ -210,7 +202,7 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev) ret = pwmchip_add(&mdp->chip); if (ret < 0) { dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); - goto disable_clk_mm; + return ret; } platform_set_drvdata(pdev, mdp); @@ -229,24 +221,13 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev) } return 0; - -disable_clk_mm: - clk_unprepare(mdp->clk_mm); -disable_clk_main: - clk_unprepare(mdp->clk_main); - return ret; } static int mtk_disp_pwm_remove(struct platform_device *pdev) { struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev); - int ret; - - ret = pwmchip_remove(&mdp->chip); - clk_unprepare(mdp->clk_mm); - clk_unprepare(mdp->clk_main); - return ret; + return pwmchip_remove(&mdp->chip); } static const struct mtk_pwm_data mt2701_pwm_data = { -- 2.21.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] pwm: mtk_disp: fix pwm clocks not poweroff when disable pwm 2019-12-17 4:02 ` [PATCH v4 1/2] pwm: mtk_disp: fix pwm clocks not poweroff when disable pwm Jitao Shi @ 2020-01-20 11:52 ` Thierry Reding 2020-01-20 14:31 ` Uwe Kleine-König 1 sibling, 0 replies; 6+ messages in thread From: Thierry Reding @ 2020-01-20 11:52 UTC (permalink / raw) To: Jitao Shi Cc: linux-pwm, linux-kernel, CK Hu, sj.huang, linux-mediatek, Uwe Kleine-Koenig, Matthias Brugger, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 5975 bytes --] On Tue, Dec 17, 2019 at 12:02:36PM +0800, Jitao Shi wrote: > The clocks of pwm are still in prepare state when disable pwm. > > Because the clocks is prepared in mtk_disp_pwm_probe() and unprepared > in mtk_disp_pwm_remove(). > > clk_prepare and clk_unprepare aren't called by mtk_disp_pwm_enable() > and mtk_disp_pwm_disable(). > > Modified: > So clk_enable() is instread with clk_prepare_enable(). > clk_disable() is instread with clk_disable_unprepare(). > > And remove the clk_prepare() from mtk_disp_pwm_probe(), > remove the clk_unprepare from mtk_disp_pwm_remove(). This commit message is basically a description of what the patch does, which is pretty useless because I can look at the patch to see what it does. Use the commit message to describe why you want to make this change and what the benefits are of doing what you're doing. Describe why and how the patch improves the driver compared to before. With regards to clk_prepare()/clk_enable() vs. clk_prepare_enable(), there are valid reasons for splitting the call up like this driver did. clk_prepare() for example may sleep, so you can't call it from interrupt context. clk_enable() on the other hand does not sleep, so it can be called from interrupt context. So there might be legitimate reasons for this split in the driver. When you say that clocks are still in prepared state when you disable the PWM this probably means that clk_disable() doesn't do anything on your platform and clk_unprepare() is when the clock is actually disabled. That's perfectly valid. It should also be safe to do this now, since a while ago the PWM API as a whole was made "sleepable", so it should be safe to call clk_prepare_enable() and clk_disable_unprepare() from any callbacks because users of the PWM API already need to assume that the API can sleep. So, I don't object to the patch, I just wanted to make sure that you've thought about the consequences and to describe in the commit message what you're actually trying to achieve and why it's better. In particular it'd be interesting to know what the effects are that you are noticing if the clock isn't off when the PWM is disabled and how you found out. Those are the kinds of details that make the commit message really useful because they help readers of the git log figure out what the problems where that you encountered and why you fixed them the way you did. Thierry > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > --- > drivers/pwm/pwm-mtk-disp.c | 43 +++++++++++--------------------------- > 1 file changed, 12 insertions(+), 31 deletions(-) > > diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c > index 83b8be0209b7..c7b14acc9316 100644 > --- a/drivers/pwm/pwm-mtk-disp.c > +++ b/drivers/pwm/pwm-mtk-disp.c > @@ -98,13 +98,13 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > high_width = div64_u64(rate * duty_ns, div); > value = period | (high_width << PWM_HIGH_WIDTH_SHIFT); > > - err = clk_enable(mdp->clk_main); > + err = clk_prepare_enable(mdp->clk_main); > if (err < 0) > return err; > > - err = clk_enable(mdp->clk_mm); > + err = clk_prepare_enable(mdp->clk_mm); > if (err < 0) { > - clk_disable(mdp->clk_main); > + clk_disable_unprepare(mdp->clk_main); > return err; > } > > @@ -124,8 +124,8 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, > 0x0); > } > > - clk_disable(mdp->clk_mm); > - clk_disable(mdp->clk_main); > + clk_disable_unprepare(mdp->clk_mm); > + clk_disable_unprepare(mdp->clk_main); > > return 0; > } > @@ -135,13 +135,13 @@ static int mtk_disp_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm) > struct mtk_disp_pwm *mdp = to_mtk_disp_pwm(chip); > int err; > > - err = clk_enable(mdp->clk_main); > + err = clk_prepare_enable(mdp->clk_main); > if (err < 0) > return err; > > - err = clk_enable(mdp->clk_mm); > + err = clk_prepare_enable(mdp->clk_mm); > if (err < 0) { > - clk_disable(mdp->clk_main); > + clk_disable_unprepare(mdp->clk_main); > return err; > } > > @@ -158,8 +158,8 @@ static void mtk_disp_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm) > mtk_disp_pwm_update_bits(mdp, DISP_PWM_EN, mdp->data->enable_mask, > 0x0); > > - clk_disable(mdp->clk_mm); > - clk_disable(mdp->clk_main); > + clk_disable_unprepare(mdp->clk_mm); > + clk_disable_unprepare(mdp->clk_main); > } > > static const struct pwm_ops mtk_disp_pwm_ops = { > @@ -194,14 +194,6 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev) > if (IS_ERR(mdp->clk_mm)) > return PTR_ERR(mdp->clk_mm); > > - ret = clk_prepare(mdp->clk_main); > - if (ret < 0) > - return ret; > - > - ret = clk_prepare(mdp->clk_mm); > - if (ret < 0) > - goto disable_clk_main; > - > mdp->chip.dev = &pdev->dev; > mdp->chip.ops = &mtk_disp_pwm_ops; > mdp->chip.base = -1; > @@ -210,7 +202,7 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev) > ret = pwmchip_add(&mdp->chip); > if (ret < 0) { > dev_err(&pdev->dev, "pwmchip_add() failed: %d\n", ret); > - goto disable_clk_mm; > + return ret; > } > > platform_set_drvdata(pdev, mdp); > @@ -229,24 +221,13 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev) > } > > return 0; > - > -disable_clk_mm: > - clk_unprepare(mdp->clk_mm); > -disable_clk_main: > - clk_unprepare(mdp->clk_main); > - return ret; > } > > static int mtk_disp_pwm_remove(struct platform_device *pdev) > { > struct mtk_disp_pwm *mdp = platform_get_drvdata(pdev); > - int ret; > - > - ret = pwmchip_remove(&mdp->chip); > - clk_unprepare(mdp->clk_mm); > - clk_unprepare(mdp->clk_main); > > - return ret; > + return pwmchip_remove(&mdp->chip); > } > > static const struct mtk_pwm_data mt2701_pwm_data = { > -- > 2.21.0 [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v4 1/2] pwm: mtk_disp: fix pwm clocks not poweroff when disable pwm 2019-12-17 4:02 ` [PATCH v4 1/2] pwm: mtk_disp: fix pwm clocks not poweroff when disable pwm Jitao Shi 2020-01-20 11:52 ` Thierry Reding @ 2020-01-20 14:31 ` Uwe Kleine-König 1 sibling, 0 replies; 6+ messages in thread From: Uwe Kleine-König @ 2020-01-20 14:31 UTC (permalink / raw) To: Jitao Shi Cc: linux-pwm, linux-kernel, CK Hu, sj.huang, Thierry Reding, linux-mediatek, Matthias Brugger, linux-arm-kernel Hello, I fully agree to what Thierry said about the commit log. One more comment: On Tue, Dec 17, 2019 at 12:02:36PM +0800, Jitao Shi wrote: > @@ -194,14 +194,6 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev) > if (IS_ERR(mdp->clk_mm)) > return PTR_ERR(mdp->clk_mm); > > - ret = clk_prepare(mdp->clk_main); > - if (ret < 0) > - return ret; > - > - ret = clk_prepare(mdp->clk_mm); > - if (ret < 0) > - goto disable_clk_main; > - > mdp->chip.dev = &pdev->dev; > mdp->chip.ops = &mtk_disp_pwm_ops; > mdp->chip.base = -1; After this there is: if (!mdp->data->has_commit) { mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug, ...); ... } So I wonder if dropping the clk_enables above is safe if there are some register accesses later on. Side note: The driver you're touching here is still using the legacy API. Would be great to convert it to .apply() instead. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v4 2/2] pwm: mtk_disp: keep the trigger register after pwm setting. 2019-12-17 4:02 [PATCH v4 0/2] clocks aren't disable when pwm_mtk_disp suspend Jitao Shi 2019-12-17 4:02 ` [PATCH v4 1/2] pwm: mtk_disp: fix pwm clocks not poweroff when disable pwm Jitao Shi @ 2019-12-17 4:02 ` Jitao Shi 2020-01-20 11:54 ` Thierry Reding 1 sibling, 1 reply; 6+ messages in thread From: Jitao Shi @ 2019-12-17 4:02 UTC (permalink / raw) To: Thierry Reding, Uwe Kleine-Koenig, Matthias Brugger, linux-pwm, linux-arm-kernel, linux-kernel, CK Hu Cc: Jitao Shi, sj.huang, linux-mediatek Move the trigger after pwm setting to avoid the pwm wrong signal output. Remove the regist enable trigger setting in probe. Move the trigger to end of mtk_disp_pwm_config(). Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> --- drivers/pwm/pwm-mtk-disp.c | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/drivers/pwm/pwm-mtk-disp.c b/drivers/pwm/pwm-mtk-disp.c index c7b14acc9316..c1aae5b5693b 100644 --- a/drivers/pwm/pwm-mtk-disp.c +++ b/drivers/pwm/pwm-mtk-disp.c @@ -122,6 +122,13 @@ static int mtk_disp_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm, mtk_disp_pwm_update_bits(mdp, mdp->data->commit, mdp->data->commit_mask, 0x0); + } else { + mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug, + mdp->data->bls_debug_mask, + mdp->data->bls_debug_mask); + mtk_disp_pwm_update_bits(mdp, mdp->data->con0, + mdp->data->con0_sel, + mdp->data->con0_sel); } clk_disable_unprepare(mdp->clk_mm); @@ -207,19 +214,6 @@ static int mtk_disp_pwm_probe(struct platform_device *pdev) platform_set_drvdata(pdev, mdp); - /* - * For MT2701, disable double buffer before writing register - * and select manual mode and use PWM_PERIOD/PWM_HIGH_WIDTH. - */ - if (!mdp->data->has_commit) { - mtk_disp_pwm_update_bits(mdp, mdp->data->bls_debug, - mdp->data->bls_debug_mask, - mdp->data->bls_debug_mask); - mtk_disp_pwm_update_bits(mdp, mdp->data->con0, - mdp->data->con0_sel, - mdp->data->con0_sel); - } - return 0; } -- 2.21.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v4 2/2] pwm: mtk_disp: keep the trigger register after pwm setting. 2019-12-17 4:02 ` [PATCH v4 2/2] pwm: mtk_disp: keep the trigger register after pwm setting Jitao Shi @ 2020-01-20 11:54 ` Thierry Reding 0 siblings, 0 replies; 6+ messages in thread From: Thierry Reding @ 2020-01-20 11:54 UTC (permalink / raw) To: Jitao Shi Cc: linux-pwm, linux-kernel, CK Hu, sj.huang, linux-mediatek, Uwe Kleine-Koenig, Matthias Brugger, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 656 bytes --] On Tue, Dec 17, 2019 at 12:02:37PM +0800, Jitao Shi wrote: > Move the trigger after pwm setting to avoid the pwm wrong signal > output. > > Remove the regist enable trigger setting in probe. > Move the trigger to end of mtk_disp_pwm_config(). > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com> > --- > drivers/pwm/pwm-mtk-disp.c | 20 +++++++------------- > 1 file changed, 7 insertions(+), 13 deletions(-) Same as for patch 1/2, please make the commit message more useful. Don't say what you do (because the patch already shows that), but instead say why you do it, what the specific problems are that you're fixing, etc. Thierry [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-20 14:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-12-17 4:02 [PATCH v4 0/2] clocks aren't disable when pwm_mtk_disp suspend Jitao Shi 2019-12-17 4:02 ` [PATCH v4 1/2] pwm: mtk_disp: fix pwm clocks not poweroff when disable pwm Jitao Shi 2020-01-20 11:52 ` Thierry Reding 2020-01-20 14:31 ` Uwe Kleine-König 2019-12-17 4:02 ` [PATCH v4 2/2] pwm: mtk_disp: keep the trigger register after pwm setting Jitao Shi 2020-01-20 11:54 ` Thierry Reding
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).