[PATCH 1/6] leds: lp8860: Use regmap_multi_reg_write for EEPROM writes

[PATCH 1/6] leds: lp8860: Use regmap_multi_reg_write for EEPROM writes

[Andrew Davis]

This helper does the same thing as manual looping, use it instead.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/leds/leds-lp8860.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
index 995f2adf85696..2b1c68e609495 100644
--- a/drivers/leds/leds-lp8860.c
+++ b/drivers/leds/leds-lp8860.c
@@ -103,12 +103,7 @@ struct lp8860_led {
 	struct regulator *regulator;
 };
 
-struct lp8860_eeprom_reg {
-	uint8_t reg;
-	uint8_t value;
-};
-
-static struct lp8860_eeprom_reg lp8860_eeprom_disp_regs[] = {
+static const struct reg_sequence lp8860_eeprom_disp_regs[] = {
 	{ LP8860_EEPROM_REG_0, 0xed },
 	{ LP8860_EEPROM_REG_1, 0xdf },
 	{ LP8860_EEPROM_REG_2, 0xdc },
@@ -238,7 +233,7 @@ static int lp8860_brightness_set(struct led_classdev *led_cdev,
 static int lp8860_init(struct lp8860_led *led)
 {
 	unsigned int read_buf;
-	int ret, i, reg_count;
+	int ret, reg_count;
 
 	if (led->regulator) {
 		ret = regulator_enable(led->regulator);
@@ -266,14 +261,10 @@ static int lp8860_init(struct lp8860_led *led)
 	}
 
 	reg_count = ARRAY_SIZE(lp8860_eeprom_disp_regs);
-	for (i = 0; i < reg_count; i++) {
-		ret = regmap_write(led->eeprom_regmap,
-				lp8860_eeprom_disp_regs[i].reg,
-				lp8860_eeprom_disp_regs[i].value);
-		if (ret) {
-			dev_err(&led->client->dev, "Failed writing EEPROM\n");
-			goto out;
-		}
+	ret = regmap_multi_reg_write(led->eeprom_regmap, lp8860_eeprom_disp_regs, reg_count);
+	if (ret) {
+		dev_err(&led->client->dev, "Failed writing EEPROM\n");
+		goto out;
 	}
 
 	ret = lp8860_unlock_eeprom(led, LP8860_LOCK_EEPROM);
-- 
2.39.2

[PATCH 2/6] leds: lp8860: Use new mutex guards to cleanup function exits

[Andrew Davis]

Use scoped mutex guards to simplify return paths. While here use
devm_mutex_init() to register the muxex so it also is cleaned
up automatically.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/leds/leds-lp8860.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
index 2b1c68e609495..2d91f476f0b79 100644
--- a/drivers/leds/leds-lp8860.c
+++ b/drivers/leds/leds-lp8860.c
@@ -135,7 +135,7 @@ static int lp8860_unlock_eeprom(struct lp8860_led *led, int lock)
 {
 	int ret;
 
-	mutex_lock(&led->lock);
+	guard(mutex)(&led->lock);
 
 	if (lock == LP8860_UNLOCK_EEPROM) {
 		ret = regmap_write(led->regmap,
@@ -143,7 +143,7 @@ static int lp8860_unlock_eeprom(struct lp8860_led *led, int lock)
 			LP8860_EEPROM_CODE_1);
 		if (ret) {
 			dev_err(&led->client->dev, "EEPROM Unlock failed\n");
-			goto out;
+			return ret;
 		}
 
 		ret = regmap_write(led->regmap,
@@ -151,14 +151,14 @@ static int lp8860_unlock_eeprom(struct lp8860_led *led, int lock)
 			LP8860_EEPROM_CODE_2);
 		if (ret) {
 			dev_err(&led->client->dev, "EEPROM Unlock failed\n");
-			goto out;
+			return ret;
 		}
 		ret = regmap_write(led->regmap,
 			LP8860_EEPROM_UNLOCK,
 			LP8860_EEPROM_CODE_3);
 		if (ret) {
 			dev_err(&led->client->dev, "EEPROM Unlock failed\n");
-			goto out;
+			return ret;
 		}
 	} else {
 		ret = regmap_write(led->regmap,
@@ -166,8 +166,6 @@ static int lp8860_unlock_eeprom(struct lp8860_led *led, int lock)
 			LP8860_LOCK_EEPROM);
 	}
 
-out:
-	mutex_unlock(&led->lock);
 	return ret;
 }
 
@@ -204,30 +202,29 @@ static int lp8860_brightness_set(struct led_classdev *led_cdev,
 	int disp_brightness = brt_val * 255;
 	int ret;
 
-	mutex_lock(&led->lock);
+	guard(mutex)(&led->lock);
 
 	ret = lp8860_fault_check(led);
 	if (ret) {
 		dev_err(&led->client->dev, "Cannot read/clear faults\n");
-		goto out;
+		return ret;
 	}
 
 	ret = regmap_write(led->regmap, LP8860_DISP_CL1_BRT_MSB,
 			(disp_brightness & 0xff00) >> 8);
 	if (ret) {
 		dev_err(&led->client->dev, "Cannot write CL1 MSB\n");
-		goto out;
+		return ret;
 	}
 
 	ret = regmap_write(led->regmap, LP8860_DISP_CL1_BRT_LSB,
 			disp_brightness & 0xff);
 	if (ret) {
 		dev_err(&led->client->dev, "Cannot write CL1 LSB\n");
-		goto out;
+		return ret;
 	}
-out:
-	mutex_unlock(&led->lock);
-	return ret;
+
+	return 0;
 }
 
 static int lp8860_init(struct lp8860_led *led)
@@ -392,7 +389,7 @@ static int lp8860_probe(struct i2c_client *client)
 	led->client = client;
 	led->led_dev.brightness_set_blocking = lp8860_brightness_set;
 
-	mutex_init(&led->lock);
+	devm_mutex_init(&client->dev, &led->lock);
 
 	i2c_set_clientdata(client, led);
 
@@ -443,8 +440,6 @@ static void lp8860_remove(struct i2c_client *client)
 			dev_err(&led->client->dev,
 				"Failed to disable regulator\n");
 	}
-
-	mutex_destroy(&led->lock);
 }
 
 static const struct i2c_device_id lp8860_id[] = {
-- 
2.39.2

[PATCH 3/6] leds: lp8860: Remove default regs when not caching

[Andrew Davis]

If we are not using regmap caches, then the value will be read
in every time, having a default value does not change anything in
that case. Remove the unused defaults.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/leds/leds-lp8860.c | 52 --------------------------------------
 1 file changed, 52 deletions(-)

diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
index 2d91f476f0b79..4cd1b960d504f 100644
--- a/drivers/leds/leds-lp8860.c
+++ b/drivers/leds/leds-lp8860.c
@@ -292,61 +292,11 @@ static int lp8860_init(struct lp8860_led *led)
 	return ret;
 }
 
-static const struct reg_default lp8860_reg_defs[] = {
-	{ LP8860_DISP_CL1_BRT_MSB, 0x00},
-	{ LP8860_DISP_CL1_BRT_LSB, 0x00},
-	{ LP8860_DISP_CL1_CURR_MSB, 0x00},
-	{ LP8860_DISP_CL1_CURR_LSB, 0x00},
-	{ LP8860_CL2_BRT_MSB, 0x00},
-	{ LP8860_CL2_BRT_LSB, 0x00},
-	{ LP8860_CL2_CURRENT, 0x00},
-	{ LP8860_CL3_BRT_MSB, 0x00},
-	{ LP8860_CL3_BRT_LSB, 0x00},
-	{ LP8860_CL3_CURRENT, 0x00},
-	{ LP8860_CL4_BRT_MSB, 0x00},
-	{ LP8860_CL4_BRT_LSB, 0x00},
-	{ LP8860_CL4_CURRENT, 0x00},
-	{ LP8860_CONFIG, 0x00},
-	{ LP8860_FAULT_CLEAR, 0x00},
-	{ LP8860_EEPROM_CNTRL, 0x80},
-	{ LP8860_EEPROM_UNLOCK, 0x00},
-};
-
 static const struct regmap_config lp8860_regmap_config = {
 	.reg_bits = 8,
 	.val_bits = 8,
 
 	.max_register = LP8860_EEPROM_UNLOCK,
-	.reg_defaults = lp8860_reg_defs,
-	.num_reg_defaults = ARRAY_SIZE(lp8860_reg_defs),
-};
-
-static const struct reg_default lp8860_eeprom_defs[] = {
-	{ LP8860_EEPROM_REG_0, 0x00 },
-	{ LP8860_EEPROM_REG_1, 0x00 },
-	{ LP8860_EEPROM_REG_2, 0x00 },
-	{ LP8860_EEPROM_REG_3, 0x00 },
-	{ LP8860_EEPROM_REG_4, 0x00 },
-	{ LP8860_EEPROM_REG_5, 0x00 },
-	{ LP8860_EEPROM_REG_6, 0x00 },
-	{ LP8860_EEPROM_REG_7, 0x00 },
-	{ LP8860_EEPROM_REG_8, 0x00 },
-	{ LP8860_EEPROM_REG_9, 0x00 },
-	{ LP8860_EEPROM_REG_10, 0x00 },
-	{ LP8860_EEPROM_REG_11, 0x00 },
-	{ LP8860_EEPROM_REG_12, 0x00 },
-	{ LP8860_EEPROM_REG_13, 0x00 },
-	{ LP8860_EEPROM_REG_14, 0x00 },
-	{ LP8860_EEPROM_REG_15, 0x00 },
-	{ LP8860_EEPROM_REG_16, 0x00 },
-	{ LP8860_EEPROM_REG_17, 0x00 },
-	{ LP8860_EEPROM_REG_18, 0x00 },
-	{ LP8860_EEPROM_REG_19, 0x00 },
-	{ LP8860_EEPROM_REG_20, 0x00 },
-	{ LP8860_EEPROM_REG_21, 0x00 },
-	{ LP8860_EEPROM_REG_22, 0x00 },
-	{ LP8860_EEPROM_REG_23, 0x00 },
-	{ LP8860_EEPROM_REG_24, 0x00 },
 };
 
 static const struct regmap_config lp8860_eeprom_regmap_config = {
@@ -354,8 +304,6 @@ static const struct regmap_config lp8860_eeprom_regmap_config = {
 	.val_bits = 8,
 
 	.max_register = LP8860_EEPROM_REG_24,
-	.reg_defaults = lp8860_eeprom_defs,
-	.num_reg_defaults = ARRAY_SIZE(lp8860_eeprom_defs),
 };
 
 static int lp8860_probe(struct i2c_client *client)
-- 
2.39.2

[PATCH 4/6] leds: lp8860: Enable regulator using enable_optional helper

[Andrew Davis]

This allows the regulator to be optional which is the same as
done here with all the checks for NULL. This also disables
on remove for us, so remove the manual disabling.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/leds/leds-lp8860.c | 31 ++++---------------------------
 1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
index 4cd1b960d504f..f913a895d8a97 100644
--- a/drivers/leds/leds-lp8860.c
+++ b/drivers/leds/leds-lp8860.c
@@ -100,7 +100,6 @@ struct lp8860_led {
 	struct regmap *regmap;
 	struct regmap *eeprom_regmap;
 	struct gpio_desc *enable_gpio;
-	struct regulator *regulator;
 };
 
 static const struct reg_sequence lp8860_eeprom_disp_regs[] = {
@@ -232,15 +231,6 @@ static int lp8860_init(struct lp8860_led *led)
 	unsigned int read_buf;
 	int ret, reg_count;
 
-	if (led->regulator) {
-		ret = regulator_enable(led->regulator);
-		if (ret) {
-			dev_err(&led->client->dev,
-				"Failed to enable regulator\n");
-			return ret;
-		}
-	}
-
 	gpiod_direction_output(led->enable_gpio, 1);
 
 	ret = lp8860_fault_check(led);
@@ -282,13 +272,6 @@ static int lp8860_init(struct lp8860_led *led)
 	if (ret)
 		gpiod_direction_output(led->enable_gpio, 0);
 
-	if (led->regulator) {
-		ret = regulator_disable(led->regulator);
-		if (ret)
-			dev_err(&led->client->dev,
-				"Failed to disable regulator\n");
-	}
-
 	return ret;
 }
 
@@ -330,9 +313,10 @@ static int lp8860_probe(struct i2c_client *client)
 		return ret;
 	}
 
-	led->regulator = devm_regulator_get(&client->dev, "vled");
-	if (IS_ERR(led->regulator))
-		led->regulator = NULL;
+	ret = devm_regulator_get_enable_optional(&client->dev, "vled");
+	if (ret && ret != -ENODEV)
+		return dev_err_probe(&client->dev, ret,
+				     "Failed to enable vled regulator\n");
 
 	led->client = client;
 	led->led_dev.brightness_set_blocking = lp8860_brightness_set;
@@ -381,13 +365,6 @@ static void lp8860_remove(struct i2c_client *client)
 	int ret;
 
 	gpiod_direction_output(led->enable_gpio, 0);
-
-	if (led->regulator) {
-		ret = regulator_disable(led->regulator);
-		if (ret)
-			dev_err(&led->client->dev,
-				"Failed to disable regulator\n");
-	}
 }
 
 static const struct i2c_device_id lp8860_id[] = {
-- 
2.39.2

[PATCH 5/6] leds: lp8860: Only unlock in lp8860_unlock_eeprom()

[Andrew Davis]

Locking is a single register write, so no need to have the unlock
function also lock. This removes the need to pass in the option
and reduces the nesting level in the function.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/leds/leds-lp8860.c | 48 ++++++++++++++------------------------
 1 file changed, 18 insertions(+), 30 deletions(-)

diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
index f913a895d8a97..088610988138f 100644
--- a/drivers/leds/leds-lp8860.c
+++ b/drivers/leds/leds-lp8860.c
@@ -130,39 +130,27 @@ static const struct reg_sequence lp8860_eeprom_disp_regs[] = {
 	{ LP8860_EEPROM_REG_24, 0x3E },
 };
 
-static int lp8860_unlock_eeprom(struct lp8860_led *led, int lock)
+static int lp8860_unlock_eeprom(struct lp8860_led *led)
 {
 	int ret;
 
 	guard(mutex)(&led->lock);
 
-	if (lock == LP8860_UNLOCK_EEPROM) {
-		ret = regmap_write(led->regmap,
-			LP8860_EEPROM_UNLOCK,
-			LP8860_EEPROM_CODE_1);
-		if (ret) {
-			dev_err(&led->client->dev, "EEPROM Unlock failed\n");
-			return ret;
-		}
-
-		ret = regmap_write(led->regmap,
-			LP8860_EEPROM_UNLOCK,
-			LP8860_EEPROM_CODE_2);
-		if (ret) {
-			dev_err(&led->client->dev, "EEPROM Unlock failed\n");
-			return ret;
-		}
-		ret = regmap_write(led->regmap,
-			LP8860_EEPROM_UNLOCK,
-			LP8860_EEPROM_CODE_3);
-		if (ret) {
-			dev_err(&led->client->dev, "EEPROM Unlock failed\n");
-			return ret;
-		}
-	} else {
-		ret = regmap_write(led->regmap,
-			LP8860_EEPROM_UNLOCK,
-			LP8860_LOCK_EEPROM);
+	ret = regmap_write(led->regmap, LP8860_EEPROM_UNLOCK, LP8860_EEPROM_CODE_1);
+	if (ret) {
+		dev_err(&led->client->dev, "EEPROM Unlock failed\n");
+		return ret;
+	}
+
+	ret = regmap_write(led->regmap, LP8860_EEPROM_UNLOCK, LP8860_EEPROM_CODE_2);
+	if (ret) {
+		dev_err(&led->client->dev, "EEPROM Unlock failed\n");
+		return ret;
+	}
+	ret = regmap_write(led->regmap, LP8860_EEPROM_UNLOCK, LP8860_EEPROM_CODE_3);
+	if (ret) {
+		dev_err(&led->client->dev, "EEPROM Unlock failed\n");
+		return ret;
 	}
 
 	return ret;
@@ -241,7 +229,7 @@ static int lp8860_init(struct lp8860_led *led)
 	if (ret)
 		goto out;
 
-	ret = lp8860_unlock_eeprom(led, LP8860_UNLOCK_EEPROM);
+	ret = lp8860_unlock_eeprom(led);
 	if (ret) {
 		dev_err(&led->client->dev, "Failed unlocking EEPROM\n");
 		goto out;
@@ -254,7 +242,7 @@ static int lp8860_init(struct lp8860_led *led)
 		goto out;
 	}
 
-	ret = lp8860_unlock_eeprom(led, LP8860_LOCK_EEPROM);
+	ret = regmap_write(led->regmap, LP8860_EEPROM_UNLOCK, LP8860_LOCK_EEPROM);
 	if (ret)
 		goto out;
 
-- 
2.39.2

[PATCH 6/6] leds: lp8860: Disable GPIO with devm action

[Andrew Davis]

This helps prevent mistakes like disable out of order in cleanup functions
and forgetting to free on error paths (as was done here).

This was the last thing the .remove() function did, so remove that too.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/leds/leds-lp8860.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
index 088610988138f..7a9eb9a247ae7 100644
--- a/drivers/leds/leds-lp8860.c
+++ b/drivers/leds/leds-lp8860.c
@@ -99,7 +99,6 @@ struct lp8860_led {
 	struct led_classdev led_dev;
 	struct regmap *regmap;
 	struct regmap *eeprom_regmap;
-	struct gpio_desc *enable_gpio;
 };
 
 static const struct reg_sequence lp8860_eeprom_disp_regs[] = {
@@ -219,8 +218,6 @@ static int lp8860_init(struct lp8860_led *led)
 	unsigned int read_buf;
 	int ret, reg_count;
 
-	gpiod_direction_output(led->enable_gpio, 1);
-
 	ret = lp8860_fault_check(led);
 	if (ret)
 		goto out;
@@ -257,9 +254,6 @@ static int lp8860_init(struct lp8860_led *led)
 	return ret;
 
 out:
-	if (ret)
-		gpiod_direction_output(led->enable_gpio, 0);
-
 	return ret;
 }
 
@@ -277,6 +271,13 @@ static const struct regmap_config lp8860_eeprom_regmap_config = {
 	.max_register = LP8860_EEPROM_REG_24,
 };
 
+static void lp8860_disable_gpio(void *data)
+{
+	struct gpio_desc *gpio = data;
+
+	gpiod_set_value(gpio, 0);
+}
+
 static int lp8860_probe(struct i2c_client *client)
 {
 	int ret;
@@ -284,6 +285,7 @@ static int lp8860_probe(struct i2c_client *client)
 	struct device_node *np = dev_of_node(&client->dev);
 	struct device_node *child_node;
 	struct led_init_data init_data = {};
+	struct gpio_desc *enable_gpio;
 
 	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
 	if (!led)
@@ -293,13 +295,11 @@ static int lp8860_probe(struct i2c_client *client)
 	if (!child_node)
 		return -EINVAL;
 
-	led->enable_gpio = devm_gpiod_get_optional(&client->dev,
-						   "enable", GPIOD_OUT_LOW);
-	if (IS_ERR(led->enable_gpio)) {
-		ret = PTR_ERR(led->enable_gpio);
-		dev_err(&client->dev, "Failed to get enable gpio: %d\n", ret);
-		return ret;
-	}
+	enable_gpio = devm_gpiod_get_optional(&client->dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(enable_gpio))
+		return dev_err_probe(&client->dev, PTR_ERR(enable_gpio),
+				     "Failed to get enable GPIO\n");
+	devm_add_action_or_reset(&client->dev, lp8860_disable_gpio, enable_gpio);
 
 	ret = devm_regulator_get_enable_optional(&client->dev, "vled");
 	if (ret && ret != -ENODEV)
@@ -311,8 +311,6 @@ static int lp8860_probe(struct i2c_client *client)
 
 	devm_mutex_init(&client->dev, &led->lock);
 
-	i2c_set_clientdata(client, led);
-
 	led->regmap = devm_regmap_init_i2c(client, &lp8860_regmap_config);
 	if (IS_ERR(led->regmap)) {
 		ret = PTR_ERR(led->regmap);
@@ -347,14 +345,6 @@ static int lp8860_probe(struct i2c_client *client)
 	return 0;
 }
 
-static void lp8860_remove(struct i2c_client *client)
-{
-	struct lp8860_led *led = i2c_get_clientdata(client);
-	int ret;
-
-	gpiod_direction_output(led->enable_gpio, 0);
-}
-
 static const struct i2c_device_id lp8860_id[] = {
 	{ "lp8860" },
 	{ }
@@ -373,7 +363,6 @@ static struct i2c_driver lp8860_driver = {
 		.of_match_table = of_lp8860_leds_match,
 	},
 	.probe		= lp8860_probe,
-	.remove		= lp8860_remove,
 	.id_table	= lp8860_id,
 };
 module_i2c_driver(lp8860_driver);
-- 
2.39.2

Re: [PATCH 1/6] leds: lp8860: Use regmap_multi_reg_write for EEPROM writes

[Lee Jones]

On Mon, 07 Apr 2025 13:35:50 -0500, Andrew Davis wrote:
> This helper does the same thing as manual looping, use it instead.
> 
> 

Applied, thanks!

[1/6] leds: lp8860: Use regmap_multi_reg_write for EEPROM writes
      commit: 868242c7dca3b5625e08b9e682ed56aa63e74e29
[2/6] leds: lp8860: Use new mutex guards to cleanup function exits
      commit: c20b0f27b352e9d26b12f16e7214f397e7ef1cd1
[3/6] leds: lp8860: Remove default regs when not caching
      commit: f4a9dd5a9a67d813b89ca4ccfdd61f7a3aaf5afa
[4/6] leds: lp8860: Enable regulator using enable_optional helper
      commit: fa604baf13ced084636afbfa8d93f030ad8b593c
[5/6] leds: lp8860: Only unlock in lp8860_unlock_eeprom()
      commit: 16df093d79e43babdb8a35f66291f382402a414b
[6/6] leds: lp8860: Disable GPIO with devm action
      commit: e9bde6230972ad76fc91279850edd8dbc6f1cb4d

--
Lee Jones [李琼斯]