Message-Id: <D09EA9A0-6EC5-45D5-B75C-FF79C8A201E0@klarinett.li>
Date: Sat, 25 Oct 2025 14:42:24 +0200
From: Christian Hitz <christian@...rinett.li>
To: Lee Jones <lee@...nel.org>
Cc: Pavel Machek <pavel@...nel.org>,
 Jacek Anaszewski <jacek.anaszewski@...il.com>,
 Dan Murphy <dmurphy@...com>,
 Christian Hitz <christian.hitz@....ch>,
 stable@...r.kernel.org,
 Pavel Machek <pavel@....cz>,
 linux-leds@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] leds: leds-lp50xx: enable chip before any communication

Hi Lee

> Am 25.10.2025 um 13:37 schrieb Lee Jones <lee@...nel.org>:
> 
> On Thu, 16 Oct 2025, Christian Hitz wrote:
> 
>> From: Christian Hitz <christian.hitz@....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?

Yes, the driver ist currently broken when used with an enable GPIO.

Assume the used enable GPIO is low when the probe function is entered. In this case
the device is in SHUTDOWN mode and does not react to i2c commands.

- The call to lp50xx_reset() on line 548 has therefore no effect as i2c is not possible yet.
- Then - on line 552 - lp50xx_enable_disable() is called. As "priv->enable_gpio“ has not
yet been initialized, setting the GPIO has no effect. Also the i2c enable command is not
executed as the device is still in SHUTDOWN.
- On line 556 lp50xx_probe_dt() finally the rest of the DT is parsed and the configured 
priv->enable_gpio is set up.

As a result the device is still in SHUTDOWN mode and not ready for operation.

> 
> 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@....ch>
>> Cc: stable@...r.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()")

I have re-added the check because of the „udelay()“ call below. This delay is not necessary 
when there is no enable_gpio. I assumed that it is better to do a double check here than to wait
500us in cases where no enable_gpio is used.

> 
>> + ret = gpiod_direction_output(priv->enable_gpio, 1);
> 
> Take the opportunity to define the magic numbers '0' and '1'.

Will do.

> 
>> + if (ret)
>> + return ret;
>> +
>> + udelay(LP50XX_START_TIME_US);
>> + } else {
> 
> In this old code we did both.  Why are we now choosing?

Coming out of SHUTDOWN state the device is already reset and lp50xx_reset() is a no-op.
To ensure a clean reset in all cases (also when enable GPIO was already high before) I will
make the reset unconditional again.

Thanks for the review and comments. I will prepare a v2 to include the comments and expand
the commit message.

Regards
Christian 

> 
>> + 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 [李琼斯]

Powered by blists - more mailing lists