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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Hardware access after shutdown due to teardown inversion.
- [Medium] Hardware left powered on upon probe failure.
--

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

leds: is31fl32xx: Add powerdown pin for hardware shutdown mode

The IS31FL32XX series features an SDB shutdown pin. Driving it low
(active low) places the chip into hardware shutdown 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.

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

[ ... ]

> @@ -421,6 +423,15 @@ 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);
> +	if (IS_ERR(priv->powerdown_gpio))
> +		return dev_err_probe(dev, PTR_ERR(priv->powerdown_gpio),
> +				"Failed to get 'powerdown' GPIO\n");
> +

[Severity: Medium]
Does this leave the chip actively powered on if probe fails later?

The powerdown gpio is acquired and driven logically low with GPIOD_OUT_LOW,
which wakes the chip from hardware shutdown.

If a subsequent initialization step in probe() fails (for example, if
is31fl32xx_init_regs() returns an error), probe() will return an error
without explicitly reverting the GPIO to its shutdown state.

The devres framework automatically frees the GPIO descriptor, but it does
not revert its physical output level. Will this leave the device powered
on indefinitely instead of returning it to its low-power standby state?

[ ... ]

> @@ -609,6 +620,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_cansleep(priv->powerdown_gpio, 1);

[Severity: Medium]
Could this cause an I2C error cascade during driver unbind due to the teardown
order?

The driver registers LEDs using devm_led_classdev_register_ext(), which
means the LED class devices are unregistered by devres after the driver's
remove() callback completes.

When this asserts the powerdown_gpio, it places the chip into hardware
shutdown mode. Subsequently, devres invokes led_classdev_unregister() which
attempts to gracefully turn off each LED:

drivers/leds/led-class.c:led_classdev_unregister() {
    ...
    if (!(led_cdev->flags & LED_RETAIN_AT_SHUTDOWN))
        led_set_brightness(led_cdev, LED_OFF);
    ...
}

This triggers an I2C write to the chip via is31fl32xx_brightness_set().
Because the hardware is already in hardware shutdown mode, will these
I2C writes fail and result in errors?

>  }
>  
>  /*

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