[PATCH V3 1/2] dt-bindings: leds: pwm: Add enable-gpios property

[PATCH V3 1/2] dt-bindings: leds: pwm: Add enable-gpios property

[LI Qingwu]

Some PWM LED chips have a dedicated enable GPIO.
This commit adds the support to specify such GPIO.

Signed-off-by: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
---
 Documentation/devicetree/bindings/leds/leds-pwm.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-pwm.yaml b/Documentation/devicetree/bindings/leds/leds-pwm.yaml
index 61b97e8bc36d0..6c4fcefbe25f9 100644
--- a/Documentation/devicetree/bindings/leds/leds-pwm.yaml
+++ b/Documentation/devicetree/bindings/leds/leds-pwm.yaml
@@ -40,6 +40,13 @@ patternProperties:
           initialization. If the option is not set then max brightness is used.
         $ref: /schemas/types.yaml#/definitions/uint32
 
+      enable-gpios:
+        description:
+          GPIO for LED hardware enable control. Set active when brightness is
+          non-zero and inactive when brightness is zero.
+          The GPIO default state follows the "default-state" property.
+        maxItems: 1
+
     required:
       - pwms
       - max-brightness
-- 
2.43.0

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

[LI Qingwu]

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(+)

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);
+
 	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
+	 * later on to honor the different default states
+	 */
+	led_data->enable_gpio =
+		devm_fwnode_gpiod_get(dev, fwnode, "enable", GPIOD_ASIS, NULL);
+
+	/* enable_gpio is optional */
+	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);
+
 	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

Re: [PATCH V3 1/2] dt-bindings: leds: pwm: Add enable-gpios property

[Krzysztof Kozlowski]

On 03/07/2025 05:52, LI Qingwu wrote:
> Some PWM LED chips have a dedicated enable GPIO.
> This commit adds the support to specify such GPIO.


Please do not use "This commit/patch/change", but imperative mood. See
longer explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

"Add a dedicated enable GPIO, because some PWM LED chips (e.g. foo bar
models from baz boo) ..."

Give us concrete, verifiable example where this is present. You can also
upstream your DTS as a proof, works for me.


Best regards,
Krzysztof

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

[Lee Jones]

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 [李琼斯]

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

[LI Qingwu]

> -----Original Message-----
> From: Lee Jones <lee@kernel.org>
> Sent: Thursday, July 10, 2025 5:37 PM
> To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> Cc: pavel@kernel.org; robh@kernel.org; krzk+dt@kernel.org;
> conor+dt@kernel.org; linux-leds@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org; GEO-CHHER-bsp-development
> <bsp-development.geo@leica-geosystems.com>
> Subject: Re: [PATCH V3 2/2] leds: pwm: Add optional GPIO enable pin support
> 
> This email is not from Hexagon’s Office 365 instance. Please be careful while
> clicking links, opening attachments, or replying to this email.
> 
> 
> 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.
> 

putting it on one line would result in 87 columns as you noted. 
I was following the 80-column guideline from
Documentation/process/coding-style.rst, which states "The preferred 
limit on the length of a single line is 80 columns."
Additionally, I used clang-format to format the code, which automatically 
split this line to stay within the 80-column limit. The current formatting 
follows the kernel's .clang-format configuration.

> > +
> >       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.
> 
You're absolutely right about the multi-line comment format, I'll fix that.

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

result in 96 columns, do you really want that?
> > +
> 
> 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.

result in 84 columns.

> > +
> >       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 [李琼斯]

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

[Lee Jones]

On Mon, 14 Jul 2025, LI Qingwu wrote:

> 
> > -----Original Message-----
> > From: Lee Jones <lee@kernel.org>
> > Sent: Thursday, July 10, 2025 5:37 PM
> > To: LI Qingwu <Qing-wu.Li@leica-geosystems.com.cn>
> > Cc: pavel@kernel.org; robh@kernel.org; krzk+dt@kernel.org;
> > conor+dt@kernel.org; linux-leds@vger.kernel.org; devicetree@vger.kernel.org;
> > linux-kernel@vger.kernel.org; GEO-CHHER-bsp-development
> > <bsp-development.geo@leica-geosystems.com>
> > Subject: Re: [PATCH V3 2/2] leds: pwm: Add optional GPIO enable pin support
> > 
> > This email is not from Hexagon’s Office 365 instance. Please be careful while
> > clicking links, opening attachments, or replying to this email.
> > 
> > 
> > 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.
> > 
> 
> putting it on one line would result in 87 columns as you noted. 
> I was following the 80-column guideline from
> Documentation/process/coding-style.rst, which states "The preferred 
> limit on the length of a single line is 80 columns."
> Additionally, I used clang-format to format the code, which automatically 
> split this line to stay within the 80-column limit. The current formatting 
> follows the kernel's .clang-format configuration.

The 80-char limit was penned by the ancient Egyptians.  We have 4k
monitors now.  Feel free to use up to 100-chars in order to prevent
these silly line-wraps in this subsystem.

> > > +
> > >       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.
> > 
> You're absolutely right about the multi-line comment format, I'll fix that.
> 
> > > +     led_data->enable_gpio =
> > > +             devm_fwnode_gpiod_get(dev, fwnode, "enable",
> > GPIOD_ASIS,
> > > + NULL);
> > 
> > One line please.
> > 
> 
> result in 96 columns, do you really want that?
> > > +
> > 
> > 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.
> 
> result in 84 columns.

Great!  16 left to go!

[...]

-- 
Lee Jones [李琼斯]