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

[Nikita Travkin <nikita@trvn.ru>]

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