Re: [PATCH V3 2/2] leds: pwm: Add optional GPIO enable pin support

[Lee Jones <lee@kernel.org>]

On Thu, 03 Jul 2025, LI Qingwu wrote:

> add support for optional GPIO-based enable pin control to PWM LED driver.
> some PWM LED chips have a dedicated enable GPIO. This commit adds the
> support to specify such GPIO, activating the pin when LED brightness
> is non-zero and deactivating it when off.
> 
> Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> ---
>  drivers/leds/leds-pwm.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

Couple of nits.

> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index c73134e7b9514..1397149464b35 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -17,6 +17,7 @@
>  #include <linux/err.h>
>  #include <linux/pwm.h>
>  #include <linux/slab.h>
> +#include <linux/gpio/consumer.h>
>  
>  struct led_pwm {
>  	const char	*name;
> @@ -29,6 +30,7 @@ struct led_pwm_data {
>  	struct led_classdev	cdev;
>  	struct pwm_device	*pwm;
>  	struct pwm_state	pwmstate;
> +	struct gpio_desc	*enable_gpio;
>  	unsigned int		active_low;
>  };
>  
> @@ -51,6 +53,9 @@ static int led_pwm_set(struct led_classdev *led_cdev,
>  	if (led_dat->active_low)
>  		duty = led_dat->pwmstate.period - duty;
>  
> +	gpiod_set_value_cansleep(led_dat->enable_gpio,
> +				 brightness == LED_OFF ? 0 : 1);

Put this on one line.

> +
>  	led_dat->pwmstate.duty_cycle = duty;
>  	/*
>  	 * Disabling a PWM doesn't guarantee that it emits the inactive level.
> @@ -132,6 +137,23 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>  		break;
>  	}
>  
> +	/* Claim the GPIO as ASIS and set the value

Explain what ASIS is please.

> +	 * later on to honor the different default states
> +	 */

Use proper multi-line comments please.

> +	led_data->enable_gpio =
> +		devm_fwnode_gpiod_get(dev, fwnode, "enable", GPIOD_ASIS, NULL);

One line please.

> +

Drop this line.

> +	/* enable_gpio is optional */

Comments start with a capital letter.

Place this comment inside the second if () statement.

> +	if (IS_ERR(led_data->enable_gpio)) {
> +		if (PTR_ERR(led_data->enable_gpio) == -ENOENT)
> +			led_data->enable_gpio = NULL;
> +		else
> +			return PTR_ERR(led_data->enable_gpio);
> +	}
> +
> +	gpiod_direction_output(led_data->enable_gpio,
> +			       !!led_data->cdev.brightness);

One line.

> +
>  	ret = devm_led_classdev_register_ext(dev, &led_data->cdev, &init_data);
>  	if (ret) {
>  		dev_err(dev, "failed to register PWM led for %s: %d\n",
> -- 
> 2.43.0
> 

-- 
Lee Jones [李琼斯]