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

Uwe Kleine-König
2026-04-13T08:55:06Z

[-- 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 --]