Re: [PATCH] leds: leds-lp50xx: enable chip before any communication

[Lee Jones <lee@kernel.org>]

On Thu, 16 Oct 2025, Christian Hitz wrote:

> From: Christian Hitz <christian.hitz@bbv.ch>
> 
> If a GPIO is used to control the chip's enable pin, it needs to be pulled
> high before any SPI communication is attempted.
> Split lp50xx_enable_disable() into two distinct functions to enforce
> correct ordering.
> Observe correct timing after manipulating the enable GPIO and SPI
> communication.

Is this currently broken?  How did it test okay before?

You need to explain more about why you are changing the semantics.

See below.

> Fixes: 242b81170fb8 ("leds: lp50xx: Add the LP50XX family of the RGB LED driver")
> 
> Signed-off-by: Christian Hitz <christian.hitz@bbv.ch>
> Cc: stable@vger.kernel.org
> ---
>  drivers/leds/leds-lp50xx.c | 51 +++++++++++++++++++++++++++-----------
>  1 file changed, 36 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/leds/leds-lp50xx.c b/drivers/leds/leds-lp50xx.c
> index d19b6a459151..f23e9ae434e4 100644
> --- a/drivers/leds/leds-lp50xx.c
> +++ b/drivers/leds/leds-lp50xx.c
> @@ -52,6 +52,8 @@
>  
>  #define LP50XX_SW_RESET		0xff
>  #define LP50XX_CHIP_EN		BIT(6)
> +#define LP50XX_START_TIME_US	500
> +#define LP50XX_RESET_TIME_US	3
>  
>  /* There are 3 LED outputs per bank */
>  #define LP50XX_LEDS_PER_MODULE	3
> @@ -374,19 +376,42 @@ static int lp50xx_reset(struct lp50xx *priv)
>  	return regmap_write(priv->regmap, priv->chip_info->reset_reg, LP50XX_SW_RESET);
>  }
>  
> -static int lp50xx_enable_disable(struct lp50xx *priv, int enable_disable)
> +static int lp50xx_enable(struct lp50xx *priv)
>  {
>  	int ret;
>  
> -	ret = gpiod_direction_output(priv->enable_gpio, enable_disable);
> +	if (priv->enable_gpio) {

Why have you added this check back in?

See: 5d2bfb3fb95b ("leds: lp50xx: Get rid of redundant check in lp50xx_enable_disable()")

> +		ret = gpiod_direction_output(priv->enable_gpio, 1);

Take the opportunity to define the magic numbers '0' and '1'.

> +		if (ret)
> +			return ret;
> +
> +		udelay(LP50XX_START_TIME_US);
> +	} else {

In this old code we did both.  Why are we now choosing?

> +		ret = lp50xx_reset(priv);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return regmap_write(priv->regmap, LP50XX_DEV_CFG0, LP50XX_CHIP_EN);
> +}
> +
> +static int lp50xx_disable(struct lp50xx *priv)
> +{
> +	int ret;
> +
> +	ret = regmap_write(priv->regmap, LP50XX_DEV_CFG0, 0);
>  	if (ret)
>  		return ret;
>  
> -	if (enable_disable)
> -		return regmap_write(priv->regmap, LP50XX_DEV_CFG0, LP50XX_CHIP_EN);
> -	else
> -		return regmap_write(priv->regmap, LP50XX_DEV_CFG0, 0);
> +	if (priv->enable_gpio) {
> +		ret = gpiod_direction_output(priv->enable_gpio, 0);
> +		if (ret)
> +			return ret;
> +
> +		udelay(LP50XX_RESET_TIME_US);
> +	}
>  
> +	return 0;
>  }
>  
>  static int lp50xx_probe_leds(struct fwnode_handle *child, struct lp50xx *priv,
> @@ -453,6 +478,10 @@ static int lp50xx_probe_dt(struct lp50xx *priv)
>  		return dev_err_probe(priv->dev, PTR_ERR(priv->enable_gpio),
>  				     "Failed to get enable GPIO\n");
>  
> +	ret = lp50xx_enable(priv);
> +	if (ret)
> +		return ret;
> +
>  	priv->regulator = devm_regulator_get(priv->dev, "vled");
>  	if (IS_ERR(priv->regulator))
>  		priv->regulator = NULL;
> @@ -550,14 +579,6 @@ static int lp50xx_probe(struct i2c_client *client)
>  		return ret;
>  	}
>  
> -	ret = lp50xx_reset(led);
> -	if (ret)
> -		return ret;
> -
> -	ret = lp50xx_enable_disable(led, 1);
> -	if (ret)
> -		return ret;
> -
>  	return lp50xx_probe_dt(led);
>  }
>  
> @@ -566,7 +587,7 @@ static void lp50xx_remove(struct i2c_client *client)
>  	struct lp50xx *led = i2c_get_clientdata(client);
>  	int ret;
>  
> -	ret = lp50xx_enable_disable(led, 0);
> +	ret = lp50xx_disable(led);
>  	if (ret)
>  		dev_err(led->dev, "Failed to disable chip\n");
>  
> -- 
> 2.51.0
> 

-- 
Lee Jones [李琼斯]