Re: [PATCH v7 4/6] leds: is31fl32xx: Add powerdown pin for hardware shutdown mode

[Jun Yan]

> On Wed, 20 May 2026, Jun Yan wrote:
> 
> > The IS31FL32XX series features an SDB shutdown pin.
> > Driving it low (active low) places the chip into hardware shutdown
> 
> Nit: No need to wrap for a new sentence.

Got it.

> > mode for power saving, while all register contents are preserved
> > and registers are not reset.
> > 
> > Deassert/assert the pin accordingly during probe() and remove()
> > to manage hardware shutdown mode.
> > 
> > Signed-off-by: Jun Yan <jerrysteve1101@xxxxxxxxx>
> > ---
> >  drivers/leds/leds-is31fl32xx.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> > index fe07acbb103a..5b9ed5a1818f 100644
> > --- a/drivers/leds/leds-is31fl32xx.c
> > +++ b/drivers/leds/leds-is31fl32xx.c
> > @@ -10,6 +10,7 @@
> >   */
> >  
> >  #include <linux/device.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/i2c.h>
> >  #include <linux/kernel.h>
> >  #include <linux/leds.h>
> > @@ -60,6 +61,7 @@ struct is31fl32xx_led_data {
> >  struct is31fl32xx_priv {
> >  	const struct is31fl32xx_chipdef *cdef;
> >  	struct i2c_client *client;
> > +	struct gpio_desc *powerdown_gpio;
> >  	unsigned int num_leds;
> >  	struct is31fl32xx_led_data leds[];
> >  };
> > @@ -421,6 +423,14 @@ static int is31fl32xx_parse_dt(struct device *dev,
> >  	const struct is31fl32xx_chipdef *cdef = priv->cdef;
> >  	int ret = 0;
> >  
> > +	/* Driving this GPIO line low takes the chip out of shutdown,
> > +	 * as it is flagged as GPIO_ACTIVE_LOW in provider (such as the device tree).
> > +	 */
> 
> Please use properly formatted multi-line comments.

OK.

> 
> > +	priv->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_LOW);
> > +	if (IS_ERR(priv->powerdown_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(priv->powerdown_gpio),
> > +				"Failed to get powerdown gpio\n");
> 
> Nit: GPIO

OK.

Thanks for the review, will fix all of in v8.

> 
> > +
> >  	if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&
> >  	    of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm")) {
> >  
> > @@ -609,6 +619,9 @@ static void is31fl32xx_remove(struct i2c_client *client)
> >  	if (ret)
> >  		dev_err(&client->dev, "Failed to reset registers on removal (%pe)\n",
> >  			ERR_PTR(ret));
> > +
> > +	if (priv->powerdown_gpio)
> > +		gpiod_set_value(priv->powerdown_gpio, 1);
> >  }
> >  
> >  /*
> > -- 
> > 2.54.0
> > 
> > 
> 
> -- 
> Lee Jones