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