* [PATCH] pwm: rockchip: State of pwm clock should synchronize with pwm enabled state
@ 2017-02-28 6:43 ` David Wu
0 siblings, 0 replies; 5+ messages in thread
From: David Wu @ 2017-02-28 6:43 UTC (permalink / raw)
To: boris.brezillon, briannorris, heiko, thierry.reding
Cc: dianders, b.galvani, caesar.wang, huangtao, linux-rockchip,
linux-pwm, linux-arm-kernel, linux-kernel, david.wu
From: "david.wu" <david.wu@rock-chips.com>
If the pwm was not enabled at uboot loader, pwm could not work for clock
always disabled at pwm driver. The pwm clock is enabled at beginning of
pwm_apply(), but disabled at end of pwm_apply().
If the pwm was enabled at uboot loader, pwm clock is always enabled unless
closed by ATF. The pwm-backlight might turn off the power at early suspend,
should disable pwm clock for saving power consume.
It is important to provide opportunity to enable/disable clock at pwm driver,
the pwm consumer should ensure correct order to call pwm enable/disable, and
pwm driver ensure state of pwm clock synchronized with pwm enabled state.
Signed-off-by: David Wu <david.wu@rock-chips.com>
---
drivers/pwm/pwm-rockchip.c | 38 +++++++++++++++++++++++++++++++-------
1 file changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index ef89df1..14a0445 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -191,6 +191,26 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
+static int rk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
+ bool enable, enum pwm_polarity polarity)
+{
+ struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+ int ret;
+
+ if (enable) {
+ ret = clk_enable(pc->clk);
+ if (ret)
+ return ret;
+ }
+
+ pc->data->set_enable(chip, pwm, enable, polarity);
+
+ if (!enable)
+ clk_disable(pc->clk);
+
+ return 0;
+}
+
static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)
{
@@ -207,22 +227,26 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return ret;
if (state->polarity != curstate.polarity && enabled) {
- pc->data->set_enable(chip, pwm, false, state->polarity);
+ ret = rk_pwm_enable(chip, pwm, false, state->polarity);
+ if (ret)
+ goto out;
enabled = false;
}
ret = rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
if (ret) {
if (enabled != curstate.enabled)
- pc->data->set_enable(chip, pwm, !enabled,
- state->polarity);
-
+ rk_pwm_enable(chip, pwm, !enabled,
+ state->polarity);
goto out;
}
- if (state->enabled != enabled)
- pc->data->set_enable(chip, pwm, state->enabled,
- state->polarity);
+ if (state->enabled != enabled) {
+ ret = rk_pwm_enable(chip, pwm, state->enabled,
+ state->polarity);
+ if (ret)
+ goto out;
+ }
/*
* Update the state with the real hardware, which can differ a bit
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] pwm: rockchip: State of pwm clock should synchronize with pwm enabled state
@ 2017-02-28 6:43 ` David Wu
0 siblings, 0 replies; 5+ messages in thread
From: David Wu @ 2017-02-28 6:43 UTC (permalink / raw)
To: linux-arm-kernel
From: "david.wu" <david.wu@rock-chips.com>
If the pwm was not enabled at uboot loader, pwm could not work for clock
always disabled at pwm driver. The pwm clock is enabled at beginning of
pwm_apply(), but disabled at end of pwm_apply().
If the pwm was enabled at uboot loader, pwm clock is always enabled unless
closed by ATF. The pwm-backlight might turn off the power at early suspend,
should disable pwm clock for saving power consume.
It is important to provide opportunity to enable/disable clock at pwm driver,
the pwm consumer should ensure correct order to call pwm enable/disable, and
pwm driver ensure state of pwm clock synchronized with pwm enabled state.
Signed-off-by: David Wu <david.wu@rock-chips.com>
---
drivers/pwm/pwm-rockchip.c | 38 +++++++++++++++++++++++++++++++-------
1 file changed, 31 insertions(+), 7 deletions(-)
diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index ef89df1..14a0445 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -191,6 +191,26 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
return 0;
}
+static int rk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
+ bool enable, enum pwm_polarity polarity)
+{
+ struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
+ int ret;
+
+ if (enable) {
+ ret = clk_enable(pc->clk);
+ if (ret)
+ return ret;
+ }
+
+ pc->data->set_enable(chip, pwm, enable, polarity);
+
+ if (!enable)
+ clk_disable(pc->clk);
+
+ return 0;
+}
+
static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
struct pwm_state *state)
{
@@ -207,22 +227,26 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
return ret;
if (state->polarity != curstate.polarity && enabled) {
- pc->data->set_enable(chip, pwm, false, state->polarity);
+ ret = rk_pwm_enable(chip, pwm, false, state->polarity);
+ if (ret)
+ goto out;
enabled = false;
}
ret = rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
if (ret) {
if (enabled != curstate.enabled)
- pc->data->set_enable(chip, pwm, !enabled,
- state->polarity);
-
+ rk_pwm_enable(chip, pwm, !enabled,
+ state->polarity);
goto out;
}
- if (state->enabled != enabled)
- pc->data->set_enable(chip, pwm, state->enabled,
- state->polarity);
+ if (state->enabled != enabled) {
+ ret = rk_pwm_enable(chip, pwm, state->enabled,
+ state->polarity);
+ if (ret)
+ goto out;
+ }
/*
* Update the state with the real hardware, which can differ a bit
--
1.9.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] pwm: rockchip: State of pwm clock should synchronize with pwm enabled state
2017-02-28 6:43 ` David Wu
(?)
@ 2017-02-28 7:55 ` Boris Brezillon
-1 siblings, 0 replies; 5+ messages in thread
From: Boris Brezillon @ 2017-02-28 7:55 UTC (permalink / raw)
To: David Wu
Cc: briannorris, heiko, thierry.reding, dianders, b.galvani,
caesar.wang, huangtao, linux-rockchip, linux-pwm,
linux-arm-kernel, linux-kernel
Hi David,
On Tue, 28 Feb 2017 14:43:36 +0800
David Wu <david.wu@rock-chips.com> wrote:
> From: "david.wu" <david.wu@rock-chips.com>
>
> If the pwm was not enabled at uboot loader, pwm could not work for clock
> always disabled at pwm driver. The pwm clock is enabled at beginning of
> pwm_apply(), but disabled at end of pwm_apply().
>
> If the pwm was enabled at uboot loader, pwm clock is always enabled unless
> closed by ATF. The pwm-backlight might turn off the power at early suspend,
> should disable pwm clock for saving power consume.
>
> It is important to provide opportunity to enable/disable clock at pwm driver,
> the pwm consumer should ensure correct order to call pwm enable/disable, and
> pwm driver ensure state of pwm clock synchronized with pwm enabled state.
Oops. It seems I'm the one who introduced this regression when moving
to the atomic PWM API.
You should add
Fixes: 2bf1c98aa5a4 ("pwm: rockchip: Add support for atomic update")
Cc: stable@vger.kernel.org
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
> drivers/pwm/pwm-rockchip.c | 38 +++++++++++++++++++++++++++++++-------
> 1 file changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index ef89df1..14a0445 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -191,6 +191,26 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> return 0;
> }
>
> +static int rk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> + bool enable, enum pwm_polarity polarity)
Please stay consistent with the existing naming scheme:
static int rockchip_pwm_enable(struct pwm_chip *chip,
struct pwm_device *pwm,
bool enable,
enum pwm_polarity polarity)
Once fixed, you can add
Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> +{
> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> + int ret;
> +
> + if (enable) {
> + ret = clk_enable(pc->clk);
> + if (ret)
> + return ret;
> + }
> +
> + pc->data->set_enable(chip, pwm, enable, polarity);
> +
> + if (!enable)
> + clk_disable(pc->clk);
> +
> + return 0;
> +}
> +
> static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> struct pwm_state *state)
> {
> @@ -207,22 +227,26 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> return ret;
>
> if (state->polarity != curstate.polarity && enabled) {
> - pc->data->set_enable(chip, pwm, false, state->polarity);
> + ret = rk_pwm_enable(chip, pwm, false, state->polarity);
> + if (ret)
> + goto out;
> enabled = false;
> }
>
> ret = rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
> if (ret) {
> if (enabled != curstate.enabled)
> - pc->data->set_enable(chip, pwm, !enabled,
> - state->polarity);
> -
> + rk_pwm_enable(chip, pwm, !enabled,
> + state->polarity);
> goto out;
> }
>
> - if (state->enabled != enabled)
> - pc->data->set_enable(chip, pwm, state->enabled,
> - state->polarity);
> + if (state->enabled != enabled) {
> + ret = rk_pwm_enable(chip, pwm, state->enabled,
> + state->polarity);
> + if (ret)
> + goto out;
> + }
>
> /*
> * Update the state with the real hardware, which can differ a bit
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] pwm: rockchip: State of pwm clock should synchronize with pwm enabled state
@ 2017-02-28 7:55 ` Boris Brezillon
0 siblings, 0 replies; 5+ messages in thread
From: Boris Brezillon @ 2017-02-28 7:55 UTC (permalink / raw)
To: David Wu
Cc: huangtao, linux-pwm, heiko, briannorris, dianders, linux-kernel,
b.galvani, linux-rockchip, thierry.reding, caesar.wang,
linux-arm-kernel
Hi David,
On Tue, 28 Feb 2017 14:43:36 +0800
David Wu <david.wu@rock-chips.com> wrote:
> From: "david.wu" <david.wu@rock-chips.com>
>
> If the pwm was not enabled at uboot loader, pwm could not work for clock
> always disabled at pwm driver. The pwm clock is enabled at beginning of
> pwm_apply(), but disabled at end of pwm_apply().
>
> If the pwm was enabled at uboot loader, pwm clock is always enabled unless
> closed by ATF. The pwm-backlight might turn off the power at early suspend,
> should disable pwm clock for saving power consume.
>
> It is important to provide opportunity to enable/disable clock at pwm driver,
> the pwm consumer should ensure correct order to call pwm enable/disable, and
> pwm driver ensure state of pwm clock synchronized with pwm enabled state.
Oops. It seems I'm the one who introduced this regression when moving
to the atomic PWM API.
You should add
Fixes: 2bf1c98aa5a4 ("pwm: rockchip: Add support for atomic update")
Cc: stable@vger.kernel.org
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
> drivers/pwm/pwm-rockchip.c | 38 +++++++++++++++++++++++++++++++-------
> 1 file changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index ef89df1..14a0445 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -191,6 +191,26 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> return 0;
> }
>
> +static int rk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> + bool enable, enum pwm_polarity polarity)
Please stay consistent with the existing naming scheme:
static int rockchip_pwm_enable(struct pwm_chip *chip,
struct pwm_device *pwm,
bool enable,
enum pwm_polarity polarity)
Once fixed, you can add
Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> +{
> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> + int ret;
> +
> + if (enable) {
> + ret = clk_enable(pc->clk);
> + if (ret)
> + return ret;
> + }
> +
> + pc->data->set_enable(chip, pwm, enable, polarity);
> +
> + if (!enable)
> + clk_disable(pc->clk);
> +
> + return 0;
> +}
> +
> static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> struct pwm_state *state)
> {
> @@ -207,22 +227,26 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> return ret;
>
> if (state->polarity != curstate.polarity && enabled) {
> - pc->data->set_enable(chip, pwm, false, state->polarity);
> + ret = rk_pwm_enable(chip, pwm, false, state->polarity);
> + if (ret)
> + goto out;
> enabled = false;
> }
>
> ret = rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
> if (ret) {
> if (enabled != curstate.enabled)
> - pc->data->set_enable(chip, pwm, !enabled,
> - state->polarity);
> -
> + rk_pwm_enable(chip, pwm, !enabled,
> + state->polarity);
> goto out;
> }
>
> - if (state->enabled != enabled)
> - pc->data->set_enable(chip, pwm, state->enabled,
> - state->polarity);
> + if (state->enabled != enabled) {
> + ret = rk_pwm_enable(chip, pwm, state->enabled,
> + state->polarity);
> + if (ret)
> + goto out;
> + }
>
> /*
> * Update the state with the real hardware, which can differ a bit
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] pwm: rockchip: State of pwm clock should synchronize with pwm enabled state
@ 2017-02-28 7:55 ` Boris Brezillon
0 siblings, 0 replies; 5+ messages in thread
From: Boris Brezillon @ 2017-02-28 7:55 UTC (permalink / raw)
To: linux-arm-kernel
Hi David,
On Tue, 28 Feb 2017 14:43:36 +0800
David Wu <david.wu@rock-chips.com> wrote:
> From: "david.wu" <david.wu@rock-chips.com>
>
> If the pwm was not enabled at uboot loader, pwm could not work for clock
> always disabled at pwm driver. The pwm clock is enabled at beginning of
> pwm_apply(), but disabled at end of pwm_apply().
>
> If the pwm was enabled at uboot loader, pwm clock is always enabled unless
> closed by ATF. The pwm-backlight might turn off the power at early suspend,
> should disable pwm clock for saving power consume.
>
> It is important to provide opportunity to enable/disable clock at pwm driver,
> the pwm consumer should ensure correct order to call pwm enable/disable, and
> pwm driver ensure state of pwm clock synchronized with pwm enabled state.
Oops. It seems I'm the one who introduced this regression when moving
to the atomic PWM API.
You should add
Fixes: 2bf1c98aa5a4 ("pwm: rockchip: Add support for atomic update")
Cc: stable at vger.kernel.org
>
> Signed-off-by: David Wu <david.wu@rock-chips.com>
> ---
> drivers/pwm/pwm-rockchip.c | 38 +++++++++++++++++++++++++++++++-------
> 1 file changed, 31 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index ef89df1..14a0445 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -191,6 +191,26 @@ static int rockchip_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> return 0;
> }
>
> +static int rk_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm,
> + bool enable, enum pwm_polarity polarity)
Please stay consistent with the existing naming scheme:
static int rockchip_pwm_enable(struct pwm_chip *chip,
struct pwm_device *pwm,
bool enable,
enum pwm_polarity polarity)
Once fixed, you can add
Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> +{
> + struct rockchip_pwm_chip *pc = to_rockchip_pwm_chip(chip);
> + int ret;
> +
> + if (enable) {
> + ret = clk_enable(pc->clk);
> + if (ret)
> + return ret;
> + }
> +
> + pc->data->set_enable(chip, pwm, enable, polarity);
> +
> + if (!enable)
> + clk_disable(pc->clk);
> +
> + return 0;
> +}
> +
> static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> struct pwm_state *state)
> {
> @@ -207,22 +227,26 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> return ret;
>
> if (state->polarity != curstate.polarity && enabled) {
> - pc->data->set_enable(chip, pwm, false, state->polarity);
> + ret = rk_pwm_enable(chip, pwm, false, state->polarity);
> + if (ret)
> + goto out;
> enabled = false;
> }
>
> ret = rockchip_pwm_config(chip, pwm, state->duty_cycle, state->period);
> if (ret) {
> if (enabled != curstate.enabled)
> - pc->data->set_enable(chip, pwm, !enabled,
> - state->polarity);
> -
> + rk_pwm_enable(chip, pwm, !enabled,
> + state->polarity);
> goto out;
> }
>
> - if (state->enabled != enabled)
> - pc->data->set_enable(chip, pwm, state->enabled,
> - state->polarity);
> + if (state->enabled != enabled) {
> + ret = rk_pwm_enable(chip, pwm, state->enabled,
> + state->polarity);
> + if (ret)
> + goto out;
> + }
>
> /*
> * Update the state with the real hardware, which can differ a bit
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-02-28 7:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-28 6:43 [PATCH] pwm: rockchip: State of pwm clock should synchronize with pwm enabled state David Wu
2017-02-28 6:43 ` David Wu
2017-02-28 7:55 ` Boris Brezillon
2017-02-28 7:55 ` Boris Brezillon
2017-02-28 7:55 ` Boris Brezillon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.