Re: [PATCH v5 4/5] leds: is31fl32xx: Add powerdown pin to exit hardware shutdown mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing stabilization delay after exiting hardware shutdown.
- [Medium] Device is not placed back into hardware shutdown on driver removal.
- [Low] Incorrect header included for the descriptor-based GPIO API.
--

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

leds: is31fl32xx: Add powerdown pin to exit hardware shutdown mode

This commit adds support for the SDB shutdown pin on the IS31FL32XX series
LED controllers. It requests the "powerdown" GPIO during probe and drives it
low to exit hardware shutdown mode so the chip can be initialized.

> diff --git a/drivers/leds/leds-is31fl32xx.c b/drivers/leds/leds-is31fl32xx.c
> index fe07acbb103a8..57d779e07a624 100644
> --- a/drivers/leds/leds-is31fl32xx.c
> +++ b/drivers/leds/leds-is31fl32xx.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include <linux/device.h>
> +#include <linux/gpio.h>

This isn't a bug, but since this patch uses the modern descriptor-based GPIO
API (devm_gpiod_get_optional and GPIOD_OUT_LOW), should this include
<linux/gpio/consumer.h> instead of the legacy <linux/gpio.h>?

[ ... ]

> @@ -421,6 +423,11 @@ static int is31fl32xx_parse_dt(struct device *dev,
>  	const struct is31fl32xx_chipdef *cdef = priv->cdef;
>  	int ret = 0;
>  
> +	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");
> +
>  	if ((cdef->output_frequency_setting_reg != IS31FL32XX_REG_NONE) &&
>  	    of_property_read_bool(dev_of_node(dev), "issi,22khz-pwm")) {
>  
>  		ret = is31fl32xx_write(priv, cdef->output_frequency_setting_reg,

Does the hardware require a stabilization delay after the powerdown pin is
driven low?

Hardware LED controllers typically require some time, often around 400us to
1ms, for their internal oscillator to stabilize before the I2C interface
becomes responsive.

If we initiate I2C communications immediately after asserting the GPIO, could
the initial I2C transaction fail with a NACK and cause the probe to abort on
physical hardware?

Also, a general question about the lifecycle of the powerdown pin:

The commit message notes this feature is used to exit hardware shutdown mode
for power saving. Does the device need to be placed back into hardware shutdown
during driver removal?

While the devres framework automatically frees the GPIO descriptor upon driver
unbind, it only releases the reference and does not alter the physical state
of the pin. Would it make sense to explicitly assert the shutdown state in the
remove function to ensure the device actually powers down when the driver is
unloaded?

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