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

Xilin Wu
2026-04-08T10:00:07Z

On 4/7/2026 12:20 AM, Nikita Travkin wrote:
> Xilin Wu писал(а) 06.04.2026 20:50:
>> 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 | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----
>> 1 file changed, 66 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-clk.c b/drivers/pwm/pwm-clk.c
>> index f8f5af57acba..99821fae54e7 100644
>> --- a/drivers/pwm/pwm-clk.c
>> +++ b/drivers/pwm/pwm-clk.c
>> @@ -10,12 +10,15 @@
>> * Limitations:
>> * - 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.
>> - * - When the PWM is disabled, the clock will be disabled as well,
>> - * line state will depend on the clock.
>
> nit: I think those limitations would still stand for existing
> users, perhaps we could just add "... unless gpio pinctrl state
> is supplied" to these two?

Ack.

>
>> * - 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 +28,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 +54,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) {
>> + pinctrl_select_state(pcchip->pinctrl, pcchip->pins_gpio);
>> + gpiod_direction_output(pcchip->gpiod, gpio_value);
>
> Is this the same case as below where gpio state has to be set
> before we can control it, or can we swap those so we first
> put gpio into a known state and only then mux it to the pad?
>

Yeah, I think it makes sense to set the gpio state first. Thanks for the
suggestion.

>
> Thanks for improving this driver,
> Nikita
>
>> + }
>> + 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 +128,35 @@ 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");
>> +
>> chip->ops = &pwm_clk_ops;
>>
>> ret = pwmchip_add(chip);
>

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