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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <AM9PR06MB7955567B37C06BA7FCB05E22D754A@AM9PR06MB7955.eurprd06.prod.outlook.com>
Date: Mon, 14 Jul 2025 03:32:33 +0000
From: LI Qingwu <Qing-wu.Li@...ca-geosystems.com.cn>
To: Lee Jones <lee@...nel.org>
CC: "pavel@...nel.org" <pavel@...nel.org>, "robh@...nel.org"
	<robh@...nel.org>, "krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>, "linux-leds@...r.kernel.org"
	<linux-leds@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, GEO-CHHER-bsp-development
	<bsp-development.geo@...ca-geosystems.com>
Subject: RE: [PATCH V3 2/2] leds: pwm: Add optional GPIO enable pin support


> -----Original Message-----
> From: Lee Jones <lee@...nel.org>
> Sent: Thursday, July 10, 2025 5:37 PM
> To: LI Qingwu <Qing-wu.Li@...ca-geosystems.com.cn>
> Cc: pavel@...nel.org; robh@...nel.org; krzk+dt@...nel.org;
> conor+dt@...nel.org; linux-leds@...r.kernel.org; devicetree@...r.kernel.org;
> linux-kernel@...r.kernel.org; GEO-CHHER-bsp-development
> <bsp-development.geo@...ca-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@...ca-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 [李琼斯]