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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Asserting the powerdown GPIO in `remove()` powers down the chip before `devres` unregisters the LED class devices, causing I2C transfer failures.
- [Medium] The driver uses `gpiod_set_value()` instead of `gpiod_set_value_cansleep()` to assert the powerdown GPIO, which can trigger a kernel warning.
- [Low] The powerdown GPIO is not asserted on probe failure, leaving the device permanently powered on.
--

commit e29dbb241facd1fc077de99378c689b24d27209d
Author: Jun Yan <jerrysteve1101@gmail.com>

leds: is31fl32xx: Add powerdown pin for hardware shutdown mode

This commit adds support for an optional powerdown GPIO in the IS31FL32XX
LED driver. It asserts the pin during probe and deasserts it during
removal to manage the hardware shutdown mode for power saving.

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index fe07acbb103a8..5b9ed5a1818fa 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c

[ ... ]

> @@ -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).
> +	 */
> +	priv->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_LOW);

[Severity: Low]
Does this leave the device permanently powered on if probe fails later?

If a failure occurs during probe after the chip is taken out of shutdown,
the devm framework releases the GPIO descriptor but does not revert its
physical line level. A custom devm action might be needed to ensure the
pin is correctly reverted to shutdown mode on probe failure.

> +	if (IS_ERR(priv->powerdown_gpio))
> +		return dev_err_probe(dev, PTR_ERR(priv->powerdown_gpio),
> +				"Failed to get powerdown gpio\n");

[ ... ]

> @@ -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);

[Severity: Medium]
Will this cause I2C transfer failures during device removal?

The LED class devices are registered with devm_led_classdev_register_ext(),
meaning they are unregistered by devres after is31fl32xx_remove() returns.

When devres later unregisters the LED class devices,
led_classdev_unregister() may try to turn off the LEDs by issuing I2C
transactions. Since the powerdown GPIO was already asserted here, the chip
will be in hardware shutdown and these transactions will result in I2C
NACKs and error logs.

Managing the GPIO teardown via devres could ensure it executes after LED
unregistration.

[Severity: Medium]
Should this use gpiod_set_value_cansleep() instead?

The GPIO might belong to a sleepable controller, such as an I2C or SPI
expander. Since is31fl32xx_remove() executes in a sleepable context (it
calls the sleepable is31fl32xx_reset_regs() just above), calling the
non-sleeping variant on a sleepable GPIO can trigger a WARN_ON() inside
the gpiolib core.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520135659.1430008-1-jerrysteve1101@gmail.com?part=4