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

[LI Qingwu]

> -----Original Message-----
> From: Lee Jones <lee@xxxxxxxxxx>
> Sent: Thursday, July 31, 2025 6:46 PM
> To: LI Qingwu <Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx>
> Cc: pavel@xxxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; linux-leds@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH V8 2/2] leds: pwm: Add optional GPIO enable pin support
> 
> 
> On Thu, 24 Jul 2025, LI Qingwu wrote:
> 
> > Add support for optional GPIO-based enable pin control to PWM LED driver.
> > Some PWM LED driver chips like TPS92380 and LT3743 require a separate
> > enable signal in addition to PWM control. Implement support for such
> > GPIO control through the "enable-gpios" device tree property,
> > activating the pin when LED brightness is non-zero and deactivating it when
> off.
> >
> > Tested on i.MX8MP EVK with TPS92380 LED driver chip
> >
> > Signed-off-by: LI Qingwu <Qing-wu.Li@xxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/leds/leds-pwm.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c index
> > c73134e7b9514..08a1f735166ad 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>
> 
> This list is alphabetical.
> 
The original code's include order is not alphabetical,
If you prefer, I can reorder all or just put "#include <linux/gpio/consumer.h>" to first line,
Please let me know your preference.

Li Qingwu

> >  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,8 @@ 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);
> 
> How about the more succinct:
> 
>   brightness ? 1 : 0
> 
> Or:
> 
>   !!brightness
> 
> > +
> >       led_dat->pwmstate.duty_cycle = duty;
> >       /*
> >        * Disabling a PWM doesn't guarantee that it emits the inactive level.
> > @@ -132,6 +136,22 @@ static int led_pwm_add(struct device *dev, struct
> led_pwm_priv *priv,
> >               break;
> >       }
> >
> > +     /*
> > +      * Claim the GPIO as GPIOD_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);
> > +
> 
> Remove this line.
> 
> > +     if (IS_ERR(led_data->enable_gpio)) {
> > +             if (PTR_ERR(led_data->enable_gpio) == -ENOENT)
> > +                     /* Enable GPIO is optional */
> > +                     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
> >
> 
> --
> Lee Jones [李琼斯]