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

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2920 bytes --]

Hello,

On Mon, Apr 06, 2026 at 11:50:02PM +0800, Xilin Wu wrote:
> @@ -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);

Please swap these two calls to reduce glitches.

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

I think you want to return all failures of devm_pinctrl_get() to the
caller.

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

Why is this needed? Ideally the probe function doesn't interrupt an
already setup output.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]