[PATCH v2 0/2] pwm: clk-pwm: Add GPIO support for constant output levels

[PATCH v2 0/2] pwm: clk-pwm: Add GPIO support for constant output levels

[Xilin Wu]

The clk-pwm driver uses a clock with duty cycle control to generate
PWM output. However, when the PWM is disabled or a 0%/100% duty cycle
is requested, the clock must be stopped, and the resulting pin level
is undefined and hardware-dependent.

This series adds optional GPIO and pinctrl support to the clk-pwm
driver. When a GPIO 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 a deterministic output 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@radxa.com>
---
Changes in v2:
- Restore the original limitation comments
- Swap the order of pinctrl_select_state and gpiod_direction_output
- Handle a situation where pinctrl states were found but no GPIO was provided
- Link to v1: https://patch.msgid.link/20260406-clk-pwm-gpio-v1-0-40d2f3a20aff@radxa.com

---
Xilin Wu (2):
      dt-bindings: pwm: clk-pwm: add optional GPIO and pinctrl properties
      pwm: clk-pwm: add GPIO and pinctrl support for constant output levels

 Documentation/devicetree/bindings/pwm/clk-pwm.yaml | 36 +++++++++-
 drivers/pwm/pwm-clk.c                              | 84 ++++++++++++++++++++--
 2 files changed, 115 insertions(+), 5 deletions(-)
---
base-commit: 2febe6e6ee6e34c7754eff3c4d81aa7b0dcb7979
change-id: 20260406-clk-pwm-gpio-7f63b38908a5

Best regards,
--  
Xilin Wu <sophon@radxa.com>

[PATCH v2 1/2] dt-bindings: pwm: clk-pwm: add optional GPIO and pinctrl properties

[Xilin Wu]

The clk-pwm driver cannot produce constant output levels (0% or 100%
duty cycle, or disabled state) through the clock hardware alone - the
actual pin level when the clock is off is undefined and
hardware-dependent.

Document optional gpios, pinctrl-names, pinctrl-0, and pinctrl-1
properties that allow the driver to switch the pin between clock
function mux (for normal PWM output) and GPIO mode (to drive a
deterministic constant level).

Signed-off-by: Xilin Wu <sophon@radxa.com>
---
 Documentation/devicetree/bindings/pwm/clk-pwm.yaml | 36 +++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/clk-pwm.yaml b/Documentation/devicetree/bindings/pwm/clk-pwm.yaml
index ec1768291503..2a0e3e02d27b 100644
--- a/Documentation/devicetree/bindings/pwm/clk-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/clk-pwm.yaml
@@ -15,6 +15,11 @@ description: |
   It's often possible to control duty-cycle of such clocks which makes them
   suitable for generating PWM signal.
 
+  Optionally, a GPIO and pinctrl states can be provided. When a constant
+  output level is needed (0%, 100%, or disabled), the pin is switched to
+  GPIO mode to drive the level directly. For normal PWM output the pin is
+  switched back to its clock function mux.
+
 allOf:
   - $ref: pwm.yaml#
 
@@ -29,6 +34,26 @@ properties:
   "#pwm-cells":
     const: 2
 
+  gpios:
+    description:
+      Optional GPIO used to drive a constant level when the PWM output is
+      disabled or set to 0% / 100% duty cycle. When provided, pinctrl states
+      "default" (clock mux) and "gpio" must also be defined.
+    maxItems: 1
+
+  pinctrl-names: true
+
+  pinctrl-0:
+    description: Pin configuration for clock function mux (normal PWM).
+    maxItems: 1
+
+  pinctrl-1:
+    description: Pin configuration for GPIO mode (constant level output).
+    maxItems: 1
+
+dependencies:
+  gpios: [ pinctrl-0, pinctrl-1 ]
+
 unevaluatedProperties: false
 
 required:
@@ -41,6 +66,15 @@ examples:
       compatible = "clk-pwm";
       #pwm-cells = <2>;
       clocks = <&gcc 0>;
-      pinctrl-names = "default";
+    };
+
+  - |
+    pwm {
+      compatible = "clk-pwm";
+      #pwm-cells = <2>;
+      clocks = <&gcc 0>;
+      pinctrl-names = "default", "gpio";
       pinctrl-0 = <&pwm_clk_flash_default>;
+      pinctrl-1 = <&pwm_clk_flash_gpio>;
+      gpios = <&tlmm 32 0>;
     };

-- 
2.53.0

[PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for constant output levels

[Xilin Wu]

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@radxa.com>
---
 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;
+	}
+
+	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.
+	 */
+	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);

-- 
2.53.0

Re: [PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for constant output levels

[Nikita Travkin]

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@radxa.com>
> ---
>  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?)

> +	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

> +	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);

Re: [PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for constant output levels

[Xilin Wu]

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@radxa.com>
>> ---
>>   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@radxa.com>

Re: [PATCH v2 2/2] pwm: clk-pwm: add GPIO and pinctrl support for constant output levels

[Nikita Travkin]

Xilin Wu писал(а) 08.04.2026 18:19:
> 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@radxa.com>
>>> ---
>>>   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%.
> 

Are you aware of the hardware quick of the clock [1] where you can't get
full range if your dividers aren't configured properly? I don't know if
new hardware is different in that regard comapred to the old sd410 I was
working with, but I recall spending a while with oscilloscope until I've
figured out why I wasn't getting full range from 0 to 100%.

I'm pretty convinced I saw full coverage (i.e. flat 0 when clock is at
0% and flat 1 when clock is at 100%) but perhaps I was measuring it wrong
or I misremember as it was long ago... I still think your solution here
is clever though, as long as you don't accidentally mask bugged gcc config.

[1] https://elixir.bootlin.com/linux/v6.19/source/drivers/clk/qcom/gcc-msm8916.c#L958-L973

> 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?
> 

Yes please, keep the old behavior when there is no gpio. There are
certainly a few existing users for this and it would be sad to have
someone's backlight go out when they set it to 100% xD

> 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");
> 	}
> 

This makes sense to me, yes.

Nikita

>>> +	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);
>>