Re: [PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for constant output levels
From: Xilin Wu
Date: Wed Apr 08 2026 - 09:23:11 EST
On 4/8/2026 6:42 PM, Nikita Travkin wrote:
Xilin Wu писал(а) 08.04.2026 15:07:
The clk-pwm driver cannot guarantee a defined output level when the
PWM is disabled or when 0%/100% duty cycle is requested, because the
pin state when the clock is stopped is hardware-dependent.
Add optional GPIO and pinctrl support: when a GPIO descriptor and
pinctrl states ("default" for clock mux, "gpio" for GPIO mode) are
provided in the device tree, the driver switches the pin to GPIO mode
and drives the appropriate level for disabled/0%/100% states. For
normal PWM output, the pin is switched back to its clock function mux.
If no GPIO is provided, the driver falls back to the original
clock-only behavior.
Signed-off-by: Xilin Wu <sophon@xxxxxxxxx>
---
drivers/pwm/pwm-clk.c | 84 ++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 80 insertions(+), 4 deletions(-)
diff --git a/drivers/pwm/pwm-clk.c b/drivers/pwm/pwm-clk.c
index f8f5af57acba..d7d8d2c2dd0f 100644
--- a/drivers/pwm/pwm-clk.c
+++ b/drivers/pwm/pwm-clk.c
@@ -11,11 +11,20 @@
* - Due to the fact that exact behavior depends on the underlying
* clock driver, various limitations are possible.
* - Underlying clock may not be able to give 0% or 100% duty cycle
- * (constant off or on), exact behavior will depend on the clock.
+ * (constant off or on), exact behavior will depend on the clock,
+ * unless a gpio pinctrl state is supplied.
* - When the PWM is disabled, the clock will be disabled as well,
- * line state will depend on the clock.
+ * line state will depend on the clock, unless a gpio pinctrl
+ * state is supplied.
* - The clk API doesn't expose the necessary calls to implement
* .get_state().
+ *
+ * Optionally, a GPIO descriptor and pinctrl states ("default" and
+ * "gpio") can be provided. When a constant output level is needed
+ * (0% duty, 100% duty, or disabled), the driver switches the pin to
+ * GPIO mode and drives the appropriate level. For normal PWM output
+ * the pin is switched back to its clock function mux. If no GPIO is
+ * provided, the driver falls back to the original clock-only behavior.
*/
#include <linux/kernel.h>
@@ -25,11 +34,17 @@
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/clk.h>
+#include <linux/gpio/consumer.h>
+#include <linux/pinctrl/consumer.h>
#include <linux/pwm.h>
struct pwm_clk_chip {
struct clk *clk;
bool clk_enabled;
+ struct pinctrl *pinctrl;
+ struct pinctrl_state *pins_default; /* clock function mux */
+ struct pinctrl_state *pins_gpio; /* GPIO mode */
+ struct gpio_desc *gpiod;
};
static inline struct pwm_clk_chip *to_pwm_clk_chip(struct pwm_chip *chip)
@@ -45,14 +60,36 @@ static int pwm_clk_apply(struct pwm_chip *chip, struct pwm_device *pwm,
u32 rate;
u64 period = state->period;
u64 duty_cycle = state->duty_cycle;
+ bool constant_level = false;
+ int gpio_value = 0;
if (!state->enabled) {
- if (pwm->state.enabled) {
+ constant_level = true;
+ gpio_value = 0;
+ } else if (state->duty_cycle == 0) {
+ constant_level = true;
+ gpio_value = (state->polarity == PWM_POLARITY_INVERSED) ? 1 : 0;
+ } else if (state->duty_cycle >= state->period) {
+ constant_level = true;
+ gpio_value = (state->polarity == PWM_POLARITY_INVERSED) ? 0 : 1;
+ }
+
So I'm looking at it again, and I'm a bit confused.
Old behavior was:
- pwm was enabled and being disabled -> stop the clock and hope state is 0;
- pwm is still enabled but
- duty=0% -> set clk duty to 0%
- duty=100% -> set clk duty to 100%
New behavior if we have gpio:
- pwm was enabled and being disabled -> constant 0
- pwm is still enabled but
- duty=0% -> constant 0
- duty=100% -> constant 1
New behavior if we don't have gpio:
Same as above but
- if we need constant 0 -> clock is halted and we pray it's 0
- if we need constant 1 -> clock is halted and we pray it's 1 (??)
Per my recollection, when I wrote this driver 5 years ago, I've manually
verified that at least on qcom setting duty cycle to 0% and 100% worked
properly, so this feels like it would regress it if left as-is...
(Btw I wonder what's the platform you need this for?)
I took a careful look at clk_rcg2_set_duty_cycle() in drivers/clk/qcom/clk-rcg2.c, and I believe the Qualcomm RCG2 MND counter cannot produce a true 0% or 100% duty cycle. For a 0% duty request, the actual duty cycle can become very small, but never exactly zero. Likewise, for a 100% duty request, it can get very close to 100%, but not exactly 100%.
I agree that the current change may cause a regression. Do you think it would make more sense to keep the old behavior when no GPIO is available, and still set the clock duty cycle to 0% or 100% in that case?
We need this for many of our future Qualcomm-based products, because the PMIC that comes with the SoC usually provides only one PWM output.
+ if (constant_level) {
+ if (pcchip->gpiod) {
+ gpiod_direction_output(pcchip->gpiod, gpio_value);
+ pinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);
+ }
+ if (pcchip->clk_enabled) {
clk_disable(pcchip->clk);
pcchip->clk_enabled = false;
}
return 0;
- } else if (!pwm->state.enabled) {
+ }
+
+ if (pcchip->gpiod)
+ pinctrl_select_state(pcchip->pinctrl, pcchip->pins_default);
+
+ if (!pcchip->clk_enabled) {
ret = clk_enable(pcchip->clk);
if (ret)
return ret;
@@ -97,6 +134,45 @@ static int pwm_clk_probe(struct platform_device *pdev)
return dev_err_probe(&pdev->dev, PTR_ERR(pcchip->clk),
"Failed to get clock\n");
+ pcchip->pinctrl = devm_pinctrl_get(&pdev->dev);
+ if (IS_ERR(pcchip->pinctrl)) {
+ ret = PTR_ERR(pcchip->pinctrl);
+ pcchip->pinctrl = NULL;
+ if (ret == -EPROBE_DEFER)
+ return ret;
+ } else {
+ pcchip->pins_default = pinctrl_lookup_state(pcchip->pinctrl,
+ PINCTRL_STATE_DEFAULT);
+ pcchip->pins_gpio = pinctrl_lookup_state(pcchip->pinctrl,
+ "gpio");
+ if (IS_ERR(pcchip->pins_default) || IS_ERR(pcchip->pins_gpio))
+ pcchip->pinctrl = NULL;
+ }
+
+ /*
+ * Switch to GPIO pinctrl state before requesting the GPIO.
+ * The driver core has already applied the "default" state, which
+ * muxes the pin to the clock function and claims it. We must
+ * release that claim first so that gpiolib can request the pin.
+ */
+ if (pcchip->pinctrl)
+ pinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);
+
+ pcchip->gpiod = devm_gpiod_get_optional(&pdev->dev, NULL, GPIOD_ASIS);
+ if (IS_ERR(pcchip->gpiod))
+ return dev_err_probe(&pdev->dev, PTR_ERR(pcchip->gpiod),
+ "Failed to get gpio\n");
+
+ /*
+ * If pinctrl states were found but no GPIO was provided, the pin is
+ * stuck in GPIO mode from the switch above. Restore the default
+ * (clock-function) mux and fall back to clock-only operation.
+ */
Feels slightly weird to silently allow "broken" DT, it would make no sense
for it to have "gpio" pinctrl and not have a gpio defined, would it?
Perhaps it makes more sense to put getting a gpio under having pins_gpio
and make it strict, so two allowed states for the driver would be either
no pinctrl-1 and no gpio, or having both at the same time?
(maybe then also worth adding cross dependency of pinctrl-1 and gpio in
the binding, it's one way only currently, not sure what's the correct
way to describe it tho)
Nikita
Yeah, good point. Having a gpio pinctrl state without an actual gpio property is indeed a broken DT and there's no reason to silently work around it. Do you think the following change would work?
if (pcchip->pinctrl) {
pinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);
pcchip->gpiod = devm_gpiod_get(&pdev->dev, NULL, GPIOD_ASIS);
if (IS_ERR(pcchip->gpiod))
return dev_err_probe(&pdev->dev, PTR_ERR(pcchip->gpiod),
"GPIO required when 'gpio' pinctrl state is present\n");
}
+ if (pcchip->pinctrl && !pcchip->gpiod) {
+ pinctrl_select_state(pcchip->pinctrl, pcchip->pins_default);
+ pcchip->pinctrl = NULL;
+ }
+
chip->ops = &pwm_clk_ops;
ret = pwmchip_add(chip);
--
Best regards,
Xilin Wu <sophon@xxxxxxxxx>