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

[Lee Jones <lee@kernel.org>]

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