[PATCH 0/4] clk: pwm: A few improvements

[PATCH 0/4] clk: pwm: A few improvements

[Uwe Kleine-König]

Hello,

my main objective here to get rid of the call to pwm_config() as I want
to drop that function. While implementing that I found a few changes
left and right of it that I think improve the driver.

Only compile tested.

Best regards
Uwe

Uwe Kleine-König (4):
  clk: pwm: Let .get_duty_cycle() return the real duty cycle
  clk: pwm: Convert to use pwm_apply_might_sleep()
  clk: pwm: Don't reconfigure running PWM at probe time
  clk: pwm: Make use of non-sleeping PWMs

 drivers/clk/clk-pwm.c | 49 +++++++++++++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 11 deletions(-)

base-commit: 8ffd015db85fea3e15a77027fda6c02ced4d2444
-- 
2.47.2

[PATCH 1/4] clk: pwm: Let .get_duty_cycle() return the real duty cycle

[Uwe Kleine-König]

pwm_get_state() returns the last requested pwm_state which might differ
from what the lowlevel PWM driver actually implemented. For the purpose
of .get_duty_cycle() the latter is the more interesting info, so use
that to determine the output parameter.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/clk/clk-pwm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
index bd4f21c22004..429150bba8cf 100644
--- a/drivers/clk/clk-pwm.c
+++ b/drivers/clk/clk-pwm.c
@@ -48,8 +48,11 @@ static int clk_pwm_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
 {
 	struct clk_pwm *clk_pwm = to_clk_pwm(hw);
 	struct pwm_state state;
+	int ret;
 
-	pwm_get_state(clk_pwm->pwm, &state);
+	ret = pwm_get_state_hw(clk_pwm->pwm, &state);
+	if (ret)
+		return ret;
 
 	duty->num = state.duty_cycle;
 	duty->den = state.period;
-- 
2.47.2

[PATCH 2/4] clk: pwm: Convert to use pwm_apply_might_sleep()

[Uwe Kleine-König]

pwm_config() is an old function that I'd like to remove. So convert this
driver to use pwm_apply_might_sleep().

There is a minor change in behaviour as the explicitly calculated
duty_cycle used an uprounding division while pwm_set_relative_duty_cycle()
rounds down. I don't expect that difference to matter in practice though.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/clk/clk-pwm.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
index 429150bba8cf..f5e6fef3f4d5 100644
--- a/drivers/clk/clk-pwm.c
+++ b/drivers/clk/clk-pwm.c
@@ -14,6 +14,7 @@
 struct clk_pwm {
 	struct clk_hw hw;
 	struct pwm_device *pwm;
+	struct pwm_state state;
 	u32 fixed_rate;
 };
 
@@ -26,7 +27,7 @@ static int clk_pwm_prepare(struct clk_hw *hw)
 {
 	struct clk_pwm *clk_pwm = to_clk_pwm(hw);
 
-	return pwm_enable(clk_pwm->pwm);
+	return pwm_apply_might_sleep(clk_pwm->pwm, &clk_pwm->state);
 }
 
 static void clk_pwm_unprepare(struct clk_hw *hw)
@@ -106,15 +107,16 @@ static int clk_pwm_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to the
-	 * atomic PWM API.
-	 */
-	pwm_apply_args(pwm);
-	ret = pwm_config(pwm, (pargs.period + 1) >> 1, pargs.period);
+	pwm_init_state(pwm, &clk_pwm->state);
+	pwm_set_relative_duty_cycle(&clk_pwm->state, 1, 2);
+
+	ret = pwm_apply_might_sleep(pwm, &clk_pwm->state);
 	if (ret < 0)
 		return ret;
 
+	/* set enabled only now to not enable output above */
+	clk_pwm->state.enabled = true;
+
 	clk_name = node->name;
 	of_property_read_string(node, "clock-output-names", &clk_name);
 
-- 
2.47.2

[PATCH 3/4] clk: pwm: Don't reconfigure running PWM at probe time

[Uwe Kleine-König]

If the PWM is enabled already when .probe() is entered, period and
duty_cycle are updated which essentially corresponds to a clock frequency
change. This is unusual and surprising. So update the settings only when
the clock gets prepared.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/clk/clk-pwm.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
index f5e6fef3f4d5..856828d5f58c 100644
--- a/drivers/clk/clk-pwm.c
+++ b/drivers/clk/clk-pwm.c
@@ -109,12 +109,6 @@ static int clk_pwm_probe(struct platform_device *pdev)
 
 	pwm_init_state(pwm, &clk_pwm->state);
 	pwm_set_relative_duty_cycle(&clk_pwm->state, 1, 2);
-
-	ret = pwm_apply_might_sleep(pwm, &clk_pwm->state);
-	if (ret < 0)
-		return ret;
-
-	/* set enabled only now to not enable output above */
 	clk_pwm->state.enabled = true;
 
 	clk_name = node->name;
-- 
2.47.2

[PATCH 4/4] clk: pwm: Make use of non-sleeping PWMs

[Uwe Kleine-König]

For some PWMs applying a configuration doesn't sleep. For these enabling
and disabling can be done in the clk callbacks .enable() and .disable()
instead of .prepare() and .unprepare().

Do that to possibly reduce the time the PWM is enabled and so save some
energy.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/clk/clk-pwm.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/clk-pwm.c b/drivers/clk/clk-pwm.c
index 856828d5f58c..4709f0338e37 100644
--- a/drivers/clk/clk-pwm.c
+++ b/drivers/clk/clk-pwm.c
@@ -23,6 +23,23 @@ static inline struct clk_pwm *to_clk_pwm(struct clk_hw *hw)
 	return container_of(hw, struct clk_pwm, hw);
 }
 
+static int clk_pwm_enable(struct clk_hw *hw)
+{
+	struct clk_pwm *clk_pwm = to_clk_pwm(hw);
+
+	return pwm_apply_atomic(clk_pwm->pwm, &clk_pwm->state);
+}
+
+static void clk_pwm_disable(struct clk_hw *hw)
+{
+	struct clk_pwm *clk_pwm = to_clk_pwm(hw);
+	struct pwm_state state = clk_pwm->state;
+
+	state.enabled = false;
+
+	pwm_apply_atomic(clk_pwm->pwm, &state);
+}
+
 static int clk_pwm_prepare(struct clk_hw *hw)
 {
 	struct clk_pwm *clk_pwm = to_clk_pwm(hw);
@@ -61,6 +78,13 @@ static int clk_pwm_get_duty_cycle(struct clk_hw *hw, struct clk_duty *duty)
 	return 0;
 }
 
+static const struct clk_ops clk_pwm_ops_atomic = {
+	.enable = clk_pwm_enable,
+	.disable = clk_pwm_disable,
+	.recalc_rate = clk_pwm_recalc_rate,
+	.get_duty_cycle = clk_pwm_get_duty_cycle,
+};
+
 static const struct clk_ops clk_pwm_ops = {
 	.prepare = clk_pwm_prepare,
 	.unprepare = clk_pwm_unprepare,
@@ -115,7 +139,11 @@ static int clk_pwm_probe(struct platform_device *pdev)
 	of_property_read_string(node, "clock-output-names", &clk_name);
 
 	init.name = clk_name;
-	init.ops = &clk_pwm_ops;
+	if (pwm_might_sleep(pwm))
+		init.ops = &clk_pwm_ops;
+	else
+		init.ops = &clk_pwm_ops_atomic;
+
 	init.flags = 0;
 	init.num_parents = 0;
 
-- 
2.47.2

Re: [PATCH 0/4] clk: pwm: A few improvements

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

Hello,

On Wed, Apr 30, 2025 at 11:57:45AM +0200, Uwe Kleine-König wrote:
> my main objective here to get rid of the call to pwm_config() as I want
> to drop that function. While implementing that I found a few changes
> left and right of it that I think improve the driver.

This was sent a few weeks before v6.15 and it missed the window for
v6.16-rc1. Is it only lack of review capacities that there was no
reaction yet, or did it fall through the cracks?

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] clk: pwm: Let .get_duty_cycle() return the real duty cycle

[Stephen Boyd]

Quoting Uwe Kleine-König (2025-04-30 02:57:46)
> pwm_get_state() returns the last requested pwm_state which might differ
> from what the lowlevel PWM driver actually implemented. For the purpose
> of .get_duty_cycle() the latter is the more interesting info, so use
> that to determine the output parameter.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

Applied to clk-next

Re: [PATCH 2/4] clk: pwm: Convert to use pwm_apply_might_sleep()

[Stephen Boyd]

Quoting Uwe Kleine-König (2025-04-30 02:57:47)
> pwm_config() is an old function that I'd like to remove. So convert this
> driver to use pwm_apply_might_sleep().
> 
> There is a minor change in behaviour as the explicitly calculated
> duty_cycle used an uprounding division while pwm_set_relative_duty_cycle()
> rounds down. I don't expect that difference to matter in practice though.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

Applied to clk-next

Re: [PATCH 3/4] clk: pwm: Don't reconfigure running PWM at probe time

[Stephen Boyd]

Quoting Uwe Kleine-König (2025-04-30 02:57:48)
> If the PWM is enabled already when .probe() is entered, period and
> duty_cycle are updated which essentially corresponds to a clock frequency
> change. This is unusual and surprising. So update the settings only when
> the clock gets prepared.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

Applied to clk-next

Re: [PATCH 4/4] clk: pwm: Make use of non-sleeping PWMs

[Stephen Boyd]

Quoting Uwe Kleine-König (2025-04-30 02:57:49)
> For some PWMs applying a configuration doesn't sleep. For these enabling
> and disabling can be done in the clk callbacks .enable() and .disable()
> instead of .prepare() and .unprepare().
> 
> Do that to possibly reduce the time the PWM is enabled and so save some
> energy.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

Applied to clk-next