[PATCH 1/3] leds: leds-lp5569: Convert to sysfs_emit API

[PATCH 1/3] leds: leds-lp5569: Convert to sysfs_emit API

[Christian Marangi]

Convert sprintf to the much safer sysfs_emit API to handle output for
sysfs.

Also better handle situation where on the same chip there may be LED
open and shorted at the same time.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/leds-lp5569.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/leds/leds-lp5569.c b/drivers/leds/leds-lp5569.c
index 7ccd8dd6026a..e5e7e61c8916 100644
--- a/drivers/leds/leds-lp5569.c
+++ b/drivers/leds/leds-lp5569.c
@@ -268,8 +268,8 @@ static ssize_t lp5569_led_open_test(struct lp55xx_led *led, char *buf)
 	led_tmp = led;
 	for (i = 0; i < pdata->num_channels; i++) {
 		if (leds_fault[led_tmp->chan_nr])
-			pos += sprintf(buf + pos, "LED %d OPEN FAIL\n",
-				       led_tmp->chan_nr);
+			pos += sysfs_emit_at(buf, pos, "LED %d OPEN FAIL\n",
+					     led_tmp->chan_nr);
 
 		led_tmp++;
 	}
@@ -366,8 +366,8 @@ static ssize_t lp5569_led_short_test(struct lp55xx_led *led, char *buf)
 	led_tmp = led;
 	for (i = 0; i < pdata->num_channels; i++) {
 		if (leds_fault[led_tmp->chan_nr])
-			pos += sprintf(buf + pos, "LED %d SHORTED FAIL\n",
-				       led_tmp->chan_nr);
+			pos += sysfs_emit_at(buf, pos, "LED %d SHORTED FAIL\n",
+					     led_tmp->chan_nr);
 
 		led_tmp++;
 	}
@@ -404,7 +404,7 @@ static ssize_t lp5569_selftest(struct device *dev,
 		goto fail;
 
 	/* Test LED Shorted */
-	pos = lp5569_led_short_test(led, buf);
+	pos += lp5569_led_short_test(led, buf);
 	if (pos < 0)
 		goto fail;
 
@@ -420,10 +420,10 @@ static ssize_t lp5569_selftest(struct device *dev,
 	}
 
 	if (pos == 0)
-		pos = sprintf(buf, "OK\n");
+		pos = sysfs_emit(buf, "OK\n");
 	goto release_lock;
 fail:
-	pos = sprintf(buf, "FAIL\n");
+	pos = sysfs_emit(buf, "FAIL\n");
 
 release_lock:
 	mutex_unlock(&chip->lock);
-- 
2.45.1

[PATCH 2/3] leds: leds-lp5523: Convert to sysfs_emit API

[Christian Marangi]

Convert sprintf to the much safer sysfs_emit API to handle output for
sysfs.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/leds-lp5523.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 57df920192d2..095060533d1a 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -245,8 +245,8 @@ static ssize_t lp5523_selftest(struct device *dev,
 			goto fail;
 
 		if (adc >= vdd || adc < LP5523_ADC_SHORTCIRC_LIM)
-			pos += sprintf(buf + pos, "LED %d FAIL\n",
-				       led->chan_nr);
+			pos += sysfs_emit_at(buf, pos, "LED %d FAIL\n",
+					     led->chan_nr);
 
 		lp55xx_write(chip, LP5523_REG_LED_PWM_BASE + led->chan_nr,
 			     0x00);
@@ -257,10 +257,10 @@ static ssize_t lp5523_selftest(struct device *dev,
 		led++;
 	}
 	if (pos == 0)
-		pos = sprintf(buf, "OK\n");
+		pos = sysfs_emit(buf, "OK\n");
 	goto release_lock;
 fail:
-	pos = sprintf(buf, "FAIL\n");
+	pos = sysfs_emit(buf, "FAIL\n");
 
 release_lock:
 	mutex_unlock(&chip->lock);
-- 
2.45.1

[PATCH 3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Christian Marangi]

Convert any entry of mutex lock/unlock to guard API and simplify code.
With the use of guard API, handling for selttest functions can be
greatly simplified.

Suggested-by: Markus Elfring <Markus.Elfring@web.de>
Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/leds/leds-lp5521.c        |  5 +-
 drivers/leds/leds-lp5523.c        | 25 +++-----
 drivers/leds/leds-lp5562.c        | 13 +++--
 drivers/leds/leds-lp5569.c        | 18 ++----
 drivers/leds/leds-lp55xx-common.c | 94 +++++++++++++------------------
 5 files changed, 64 insertions(+), 91 deletions(-)

diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
index de0f8ea48eba..56d16ea18617 100644
--- a/drivers/leds/leds-lp5521.c
+++ b/drivers/leds/leds-lp5521.c
@@ -9,6 +9,7 @@
  *          Milo(Woogyom) Kim <milo.kim@ti.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
 #include <linux/i2c.h>
@@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev,
 	struct lp55xx_chip *chip = led->chip;
 	int ret;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
+
 	ret = lp5521_run_selftest(chip, buf);
-	mutex_unlock(&chip->lock);
 
 	return sysfs_emit(buf, "%s\n", ret ? "FAIL" : "OK");
 }
diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
index 095060533d1a..baa1a3ac1a56 100644
--- a/drivers/leds/leds-lp5523.c
+++ b/drivers/leds/leds-lp5523.c
@@ -9,6 +9,7 @@
  *          Milo(Woogyom) Kim <milo.kim@ti.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
 #include <linux/i2c.h>
@@ -188,16 +189,16 @@ static ssize_t lp5523_selftest(struct device *dev,
 	int ret, pos = 0;
 	u8 status, adc, vdd, i;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
 
 	ret = lp55xx_read(chip, LP5523_REG_STATUS, &status);
 	if (ret < 0)
-		goto fail;
+		return sysfs_emit(buf, "FAIL\n");
 
 	/* Check that ext clock is really in use if requested */
 	if (pdata->clock_mode == LP55XX_CLOCK_EXT) {
 		if  ((status & LP5523_EXT_CLK_USED) == 0)
-			goto fail;
+			return sysfs_emit(buf, "FAIL\n");
 	}
 
 	/* Measure VDD (i.e. VBAT) first (channel 16 corresponds to VDD) */
@@ -205,14 +206,14 @@ static ssize_t lp5523_selftest(struct device *dev,
 	usleep_range(3000, 6000); /* ADC conversion time is typically 2.7 ms */
 	ret = lp55xx_read(chip, LP5523_REG_STATUS, &status);
 	if (ret < 0)
-		goto fail;
+		return sysfs_emit(buf, "FAIL\n");
 
 	if (!(status & LP5523_LEDTEST_DONE))
 		usleep_range(3000, 6000); /* Was not ready. Wait little bit */
 
 	ret = lp55xx_read(chip, LP5523_REG_LED_TEST_ADC, &vdd);
 	if (ret < 0)
-		goto fail;
+		return sysfs_emit(buf, "FAIL\n");
 
 	vdd--;	/* There may be some fluctuation in measurement */
 
@@ -235,14 +236,14 @@ static ssize_t lp5523_selftest(struct device *dev,
 		usleep_range(3000, 6000);
 		ret = lp55xx_read(chip, LP5523_REG_STATUS, &status);
 		if (ret < 0)
-			goto fail;
+			return sysfs_emit(buf, "FAIL\n");
 
 		if (!(status & LP5523_LEDTEST_DONE))
 			usleep_range(3000, 6000); /* Was not ready. Wait. */
 
 		ret = lp55xx_read(chip, LP5523_REG_LED_TEST_ADC, &adc);
 		if (ret < 0)
-			goto fail;
+			return sysfs_emit(buf, "FAIL\n");
 
 		if (adc >= vdd || adc < LP5523_ADC_SHORTCIRC_LIM)
 			pos += sysfs_emit_at(buf, pos, "LED %d FAIL\n",
@@ -256,16 +257,8 @@ static ssize_t lp5523_selftest(struct device *dev,
 			     led->led_current);
 		led++;
 	}
-	if (pos == 0)
-		pos = sysfs_emit(buf, "OK\n");
-	goto release_lock;
-fail:
-	pos = sysfs_emit(buf, "FAIL\n");
 
-release_lock:
-	mutex_unlock(&chip->lock);
-
-	return pos;
+	return pos == 0 ? sysfs_emit(buf, "OK\n") : pos;
 }
 
 LP55XX_DEV_ATTR_ENGINE_MODE(1);
diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c
index 6ba5dbb9cace..69a4e3d5a126 100644
--- a/drivers/leds/leds-lp5562.c
+++ b/drivers/leds/leds-lp5562.c
@@ -7,6 +7,7 @@
  * Author: Milo(Woogyom) Kim <milo.kim@ti.com>
  */
 
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
 #include <linux/i2c.h>
@@ -171,9 +172,9 @@ static int lp5562_led_brightness(struct lp55xx_led *led)
 	};
 	int ret;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
+
 	ret = lp55xx_write(chip, addr[led->chan_nr], led->brightness);
-	mutex_unlock(&chip->lock);
 
 	return ret;
 }
@@ -268,9 +269,9 @@ static ssize_t lp5562_store_pattern(struct device *dev,
 	if (mode > num_patterns || !ptn)
 		return -EINVAL;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
+
 	ret = lp5562_run_predef_led_pattern(chip, mode);
-	mutex_unlock(&chip->lock);
 
 	if (ret)
 		return ret;
@@ -320,9 +321,9 @@ static ssize_t lp5562_store_engine_mux(struct device *dev,
 		return -EINVAL;
 	}
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
+
 	lp55xx_update_bits(chip, LP5562_REG_ENG_SEL, mask, val);
-	mutex_unlock(&chip->lock);
 
 	return len;
 }
diff --git a/drivers/leds/leds-lp5569.c b/drivers/leds/leds-lp5569.c
index e5e7e61c8916..dc8efb25b78e 100644
--- a/drivers/leds/leds-lp5569.c
+++ b/drivers/leds/leds-lp5569.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
 #include <linux/i2c.h>
@@ -396,17 +397,17 @@ static ssize_t lp5569_selftest(struct device *dev,
 	struct lp55xx_chip *chip = led->chip;
 	int i, pos = 0;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
 
 	/* Test LED Open */
 	pos = lp5569_led_open_test(led, buf);
 	if (pos < 0)
-		goto fail;
+		return sprintf(buf, "FAIL\n");
 
 	/* Test LED Shorted */
 	pos += lp5569_led_short_test(led, buf);
 	if (pos < 0)
-		goto fail;
+		return sprintf(buf, "FAIL\n");
 
 	for (i = 0; i < chip->pdata->num_channels; i++) {
 		/* Restore current */
@@ -419,16 +420,7 @@ static ssize_t lp5569_selftest(struct device *dev,
 		led++;
 	}
 
-	if (pos == 0)
-		pos = sysfs_emit(buf, "OK\n");
-	goto release_lock;
-fail:
-	pos = sysfs_emit(buf, "FAIL\n");
-
-release_lock:
-	mutex_unlock(&chip->lock);
-
-	return pos;
+	return pos == 0 ? sysfs_emit(buf, "OK\n") : pos;
 }
 
 LP55XX_DEV_ATTR_ENGINE_MODE(1);
diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
index 1b71f512206d..f8cf5c0e983a 100644
--- a/drivers/leds/leds-lp55xx-common.c
+++ b/drivers/leds/leds-lp55xx-common.c
@@ -10,6 +10,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/cleanup.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/firmware.h>
@@ -272,10 +273,10 @@ int lp55xx_led_brightness(struct lp55xx_led *led)
 	const struct lp55xx_device_config *cfg = chip->cfg;
 	int ret;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
+
 	ret = lp55xx_write(chip, cfg->reg_led_pwm_base.addr + led->chan_nr,
 			   led->brightness);
-	mutex_unlock(&chip->lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(lp55xx_led_brightness);
@@ -287,7 +288,8 @@ int lp55xx_multicolor_brightness(struct lp55xx_led *led)
 	int ret;
 	int i;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
+
 	for (i = 0; i < led->mc_cdev.num_colors; i++) {
 		ret = lp55xx_write(chip,
 				   cfg->reg_led_pwm_base.addr +
@@ -296,7 +298,7 @@ int lp55xx_multicolor_brightness(struct lp55xx_led *led)
 		if (ret)
 			break;
 	}
-	mutex_unlock(&chip->lock);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(lp55xx_multicolor_brightness);
@@ -404,9 +406,9 @@ static ssize_t led_current_store(struct device *dev,
 	if (!chip->cfg->set_led_current)
 		return len;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
+
 	chip->cfg->set_led_current(led, (u8)curr);
-	mutex_unlock(&chip->lock);
 
 	return len;
 }
@@ -541,14 +543,12 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
 	}
 
 	/* handling firmware data is chip dependent */
-	mutex_lock(&chip->lock);
-
-	chip->engines[idx - 1].mode = LP55XX_ENGINE_LOAD;
-	chip->fw = fw;
-	if (chip->cfg->firmware_cb)
-		chip->cfg->firmware_cb(chip);
-
-	mutex_unlock(&chip->lock);
+	scoped_guard(mutex, &chip->lock) {
+		chip->engines[idx - 1].mode = LP55XX_ENGINE_LOAD;
+		chip->fw = fw;
+		if (chip->cfg->firmware_cb)
+			chip->cfg->firmware_cb(chip);
+	}
 
 	/* firmware should be released for other channel use */
 	release_firmware(chip->fw);
@@ -592,10 +592,10 @@ static ssize_t select_engine_store(struct device *dev,
 	case LP55XX_ENGINE_1:
 	case LP55XX_ENGINE_2:
 	case LP55XX_ENGINE_3:
-		mutex_lock(&chip->lock);
-		chip->engine_idx = val;
-		ret = lp55xx_request_firmware(chip);
-		mutex_unlock(&chip->lock);
+		scoped_guard(mutex, &chip->lock) {
+			chip->engine_idx = val;
+			ret = lp55xx_request_firmware(chip);
+		}
 		break;
 	default:
 		dev_err(dev, "%lu: invalid engine index. (1, 2, 3)\n", val);
@@ -634,9 +634,9 @@ static ssize_t run_engine_store(struct device *dev,
 		return len;
 	}
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
+
 	lp55xx_run_engine(chip, true);
-	mutex_unlock(&chip->lock);
 
 	return len;
 }
@@ -673,7 +673,7 @@ ssize_t lp55xx_store_engine_mode(struct device *dev,
 	const struct lp55xx_device_config *cfg = chip->cfg;
 	struct lp55xx_engine *engine = &chip->engines[nr - 1];
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
 
 	chip->engine_idx = nr;
 
@@ -689,8 +689,6 @@ ssize_t lp55xx_store_engine_mode(struct device *dev,
 		engine->mode = LP55XX_ENGINE_DISABLED;
 	}
 
-	mutex_unlock(&chip->lock);
-
 	return len;
 }
 EXPORT_SYMBOL_GPL(lp55xx_store_engine_mode);
@@ -703,14 +701,12 @@ ssize_t lp55xx_store_engine_load(struct device *dev,
 	struct lp55xx_chip *chip = led->chip;
 	int ret;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
 
 	chip->engine_idx = nr;
 	lp55xx_load_engine(chip);
 	ret = lp55xx_update_program_memory(chip, buf, len);
 
-	mutex_unlock(&chip->lock);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(lp55xx_store_engine_load);
@@ -804,21 +800,17 @@ ssize_t lp55xx_store_engine_leds(struct device *dev,
 	if (lp55xx_mux_parse(chip, buf, &mux, len))
 		return -EINVAL;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
 
 	chip->engine_idx = nr;
-	ret = -EINVAL;
 
 	if (engine->mode != LP55XX_ENGINE_LOAD)
-		goto leave;
+		return -EINVAL;
 
 	if (lp55xx_load_mux(chip, mux, nr))
-		goto leave;
+		return -EINVAL;
 
-	ret = len;
-leave:
-	mutex_unlock(&chip->lock);
-	return ret;
+	return len;
 }
 EXPORT_SYMBOL_GPL(lp55xx_store_engine_leds);
 
@@ -832,9 +824,9 @@ ssize_t lp55xx_show_master_fader(struct device *dev,
 	int ret;
 	u8 val;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
+
 	ret = lp55xx_read(chip, cfg->reg_master_fader_base.addr + nr - 1, &val);
-	mutex_unlock(&chip->lock);
 
 	return ret ? ret : sysfs_emit(buf, "%u\n", val);
 }
@@ -856,10 +848,10 @@ ssize_t lp55xx_store_master_fader(struct device *dev,
 	if (val > 0xff)
 		return -EINVAL;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
+
 	ret = lp55xx_write(chip, cfg->reg_master_fader_base.addr + nr - 1,
 			   (u8)val);
-	mutex_unlock(&chip->lock);
 
 	return ret ? ret : len;
 }
@@ -875,25 +867,22 @@ ssize_t lp55xx_show_master_fader_leds(struct device *dev,
 	int i, ret, pos = 0;
 	u8 val;
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
 
 	for (i = 0; i < cfg->max_channel; i++) {
 		ret = lp55xx_read(chip, cfg->reg_led_ctrl_base.addr + i, &val);
 		if (ret)
-			goto leave;
+			return ret;
 
 		val = FIELD_GET(LP55xx_FADER_MAPPING_MASK, val);
 		if (val > FIELD_MAX(LP55xx_FADER_MAPPING_MASK)) {
-			ret = -EINVAL;
-			goto leave;
+			return -EINVAL;
 		}
 		buf[pos++] = val + '0';
 	}
 	buf[pos++] = '\n';
-	ret = pos;
-leave:
-	mutex_unlock(&chip->lock);
-	return ret;
+
+	return pos;
 }
 EXPORT_SYMBOL_GPL(lp55xx_show_master_fader_leds);
 
@@ -909,7 +898,7 @@ ssize_t lp55xx_store_master_fader_leds(struct device *dev,
 
 	n = min_t(int, len, cfg->max_channel);
 
-	mutex_lock(&chip->lock);
+	guard(mutex, &chip->lock);
 
 	for (i = 0; i < n; i++) {
 		if (buf[i] >= '0' && buf[i] <= '3') {
@@ -919,16 +908,13 @@ ssize_t lp55xx_store_master_fader_leds(struct device *dev,
 						 LP55xx_FADER_MAPPING_MASK,
 						 val);
 			if (ret)
-				goto leave;
+				return ret;
 		} else {
-			ret = -EINVAL;
-			goto leave;
+			return -EINVAL;
 		}
 	}
-	ret = len;
-leave:
-	mutex_unlock(&chip->lock);
-	return ret;
+
+	return len;
 }
 EXPORT_SYMBOL_GPL(lp55xx_store_master_fader_leds);
 
-- 
2.45.1

Re: [PATCH 3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Markus Elfring]

> Convert any entry of mutex lock/unlock to guard API and simplify code.

Thanks that you would like to support another bit of collateral evolution.

* Would you get into the mood to benefit any more from applications
  of scope-based resource management?

* Will development interests accordingly grow to adjust further source code places
  according to known pairs of function calls?


> With the use of guard API, handling for selttest functions can be

                                          selftest?


> greatly simplified.

I find cover letters helpful for patch series.


…
> +++ b/drivers/leds/leds-lp5521.c
> @@ -9,6 +9,7 @@
>   *          Milo(Woogyom) Kim <milo.kim@ti.com>
>   */
>
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/firmware.h>
…

I guess that this proposed addition is not directly needed here (and related places)
because the header file is included for the macro call “DEFINE_GUARD(mutex, …)” already.
https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/mutex.h#L22


…
> @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev,
>  	struct lp55xx_chip *chip = led->chip;
>  	int ret;
>
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	ret = lp5521_run_selftest(chip, buf);
> -	mutex_unlock(&chip->lock);
>
>  	return sysfs_emit(buf, "%s\n", ret ? "FAIL" : "OK");
>  }
…

How do you think about to omit any blank lines (also at similar places)?

Regards,
Markus

Re: [PATCH 3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Lee Jones]

On Thu, 27 Jun 2024, Markus Elfring wrote:

> > Convert any entry of mutex lock/unlock to guard API and simplify code.
> 
> Thanks that you would like to support another bit of collateral evolution.
> 
> * Would you get into the mood to benefit any more from applications
>   of scope-based resource management?
> 
> * Will development interests accordingly grow to adjust further source code places
>   according to known pairs of function calls?
> 
> 
> > With the use of guard API, handling for selttest functions can be
> 
>                                           selftest?
> 
> 
> > greatly simplified.
> 
> I find cover letters helpful for patch series.
> 
> 
> …
> > +++ b/drivers/leds/leds-lp5521.c
> > @@ -9,6 +9,7 @@
> >   *          Milo(Woogyom) Kim <milo.kim@ti.com>
> >   */
> >
> > +#include <linux/cleanup.h>
> >  #include <linux/delay.h>
> >  #include <linux/firmware.h>
> …
> 
> I guess that this proposed addition is not directly needed here (and related places)
> because the header file is included for the macro call “DEFINE_GUARD(mutex, …)” already.
> https://elixir.bootlin.com/linux/v6.10-rc5/source/include/linux/mutex.h#L22
> 
> 
> …
> > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev,
> >  	struct lp55xx_chip *chip = led->chip;
> >  	int ret;
> >
> > -	mutex_lock(&chip->lock);
> > +	guard(mutex, &chip->lock);
> > +
> >  	ret = lp5521_run_selftest(chip, buf);
> > -	mutex_unlock(&chip->lock);
> >
> >  	return sysfs_emit(buf, "%s\n", ret ? "FAIL" : "OK");
> >  }
> …
> 
> How do you think about to omit any blank lines (also at similar places)?

Please do not omit the blank lines.

-- 
Lee Jones [李琼斯]

Re: [PATCH 3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Lee Jones]

On Thu, 27 Jun 2024, Markus Elfring wrote:

> > Convert any entry of mutex lock/unlock to guard API and simplify code.
> 
> Thanks that you would like to support another bit of collateral evolution.
> 
> * Would you get into the mood to benefit any more from applications
>   of scope-based resource management?

Why don't you submit them yourself instead of asking others to do work?

-- 
Lee Jones [李琼斯]

Re: [3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Markus Elfring]

>>> Convert any entry of mutex lock/unlock to guard API and simplify code.
>>
>> Thanks that you would like to support another bit of collateral evolution.
>>
>> * Would you get into the mood to benefit any more from applications
>>   of scope-based resource management?
>
> Why don't you submit them yourself instead of asking others to do work?

1. The change resistance (or acceptance) is varying for possible software transformations
   in wide ranges, isn't it?


2. I would appreciate better support and collaboration with additional development resources.


3. I hope that further improvements can be achieved also by the means of
   the semantic patch language (Coccinelle software) in safer and more convenient ways.
   Are you looking for any extensions according to the coccicheck tool?


Regards,
Markus

Re: [3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Lee Jones]

On Thu, 27 Jun 2024, Markus Elfring wrote:

> >>> Convert any entry of mutex lock/unlock to guard API and simplify code.
> >>
> >> Thanks that you would like to support another bit of collateral evolution.
> >>
> >> * Would you get into the mood to benefit any more from applications
> >>   of scope-based resource management?
> >
> > Why don't you submit them yourself instead of asking others to do work?
> 
> 1. The change resistance (or acceptance) is varying for possible software transformations
>    in wide ranges, isn't it?

How would that be any different for anyone else?

Resistance/acceptance should be based on patch quality alone.

> 2. I would appreciate better support and collaboration with additional development resources.

In what regard?

Make the changes and submit them.

What additional resources could you possibly need?

> 3. I hope that further improvements can be achieved also by the means of
>    the semantic patch language (Coccinelle software) in safer and more convenient ways.
>    Are you looking for any extensions according to the coccicheck tool?

Sounds good.  Submit a patch.

-- 
Lee Jones [李琼斯]

Re: [3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Markus Elfring]

>>>>> Convert any entry of mutex lock/unlock to guard API and simplify code.
>>>>
>>>> Thanks that you would like to support another bit of collateral evolution.
>>>>
>>>> * Would you get into the mood to benefit any more from applications
>>>>   of scope-based resource management?
>>>
>>> Why don't you submit them yourself instead of asking others to do work?
>>
>> 1. The change resistance (or acceptance) is varying for possible software transformations
>>    in wide ranges, isn't it?
>
> How would that be any different for anyone else?

Maybe not.


> Resistance/acceptance should be based on patch quality alone.

There are further factors involved also according to usual communication challenges.


>> 2. I would appreciate better support and collaboration with additional development resources.
>
> In what regard?

I got the impression that progress would be hindered (or even blocked?) for a while
in selected subsystem areas.


> What additional resources could you possibly need?

Possibly known examples:

* More powerful computation equipment?

* Software improvements?

* Financial incentives?


>> 3. I hope that further improvements can be achieved also by the means of
>>    the semantic patch language (Coccinelle software) in safer and more convenient ways.
>>    Are you looking for any extensions according to the coccicheck tool?
>
> Sounds good.

Thanks for such positive feedback.


> Submit a patch.

How long will the integration take (if you would like to take another look at
growing product backlogs)?

Regards,
Markus

Re: [PATCH 3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Lee Jones]

On Thu, 27 Jun 2024, Christian Marangi wrote:

> Convert any entry of mutex lock/unlock to guard API and simplify code.
> With the use of guard API, handling for selttest functions can be
> greatly simplified.
> 
> Suggested-by: Markus Elfring <Markus.Elfring@web.de>
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/leds/leds-lp5521.c        |  5 +-
>  drivers/leds/leds-lp5523.c        | 25 +++-----
>  drivers/leds/leds-lp5562.c        | 13 +++--
>  drivers/leds/leds-lp5569.c        | 18 ++----
>  drivers/leds/leds-lp55xx-common.c | 94 +++++++++++++------------------
>  5 files changed, 64 insertions(+), 91 deletions(-)

How was this one tested?

Can you try build-testing it again please?

> diff --git a/drivers/leds/leds-lp5521.c b/drivers/leds/leds-lp5521.c
> index de0f8ea48eba..56d16ea18617 100644
> --- a/drivers/leds/leds-lp5521.c
> +++ b/drivers/leds/leds-lp5521.c
> @@ -9,6 +9,7 @@
>   *          Milo(Woogyom) Kim <milo.kim@ti.com>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/firmware.h>
>  #include <linux/i2c.h>
> @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev,
>  	struct lp55xx_chip *chip = led->chip;
>  	int ret;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	ret = lp5521_run_selftest(chip, buf);
> -	mutex_unlock(&chip->lock);
>  
>  	return sysfs_emit(buf, "%s\n", ret ? "FAIL" : "OK");
>  }
> diff --git a/drivers/leds/leds-lp5523.c b/drivers/leds/leds-lp5523.c
> index 095060533d1a..baa1a3ac1a56 100644
> --- a/drivers/leds/leds-lp5523.c
> +++ b/drivers/leds/leds-lp5523.c
> @@ -9,6 +9,7 @@
>   *          Milo(Woogyom) Kim <milo.kim@ti.com>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/firmware.h>
>  #include <linux/i2c.h>
> @@ -188,16 +189,16 @@ static ssize_t lp5523_selftest(struct device *dev,
>  	int ret, pos = 0;
>  	u8 status, adc, vdd, i;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
>  
>  	ret = lp55xx_read(chip, LP5523_REG_STATUS, &status);
>  	if (ret < 0)
> -		goto fail;
> +		return sysfs_emit(buf, "FAIL\n");
>  
>  	/* Check that ext clock is really in use if requested */
>  	if (pdata->clock_mode == LP55XX_CLOCK_EXT) {
>  		if  ((status & LP5523_EXT_CLK_USED) == 0)
> -			goto fail;
> +			return sysfs_emit(buf, "FAIL\n");
>  	}
>  
>  	/* Measure VDD (i.e. VBAT) first (channel 16 corresponds to VDD) */
> @@ -205,14 +206,14 @@ static ssize_t lp5523_selftest(struct device *dev,
>  	usleep_range(3000, 6000); /* ADC conversion time is typically 2.7 ms */
>  	ret = lp55xx_read(chip, LP5523_REG_STATUS, &status);
>  	if (ret < 0)
> -		goto fail;
> +		return sysfs_emit(buf, "FAIL\n");
>  
>  	if (!(status & LP5523_LEDTEST_DONE))
>  		usleep_range(3000, 6000); /* Was not ready. Wait little bit */
>  
>  	ret = lp55xx_read(chip, LP5523_REG_LED_TEST_ADC, &vdd);
>  	if (ret < 0)
> -		goto fail;
> +		return sysfs_emit(buf, "FAIL\n");
>  
>  	vdd--;	/* There may be some fluctuation in measurement */
>  
> @@ -235,14 +236,14 @@ static ssize_t lp5523_selftest(struct device *dev,
>  		usleep_range(3000, 6000);
>  		ret = lp55xx_read(chip, LP5523_REG_STATUS, &status);
>  		if (ret < 0)
> -			goto fail;
> +			return sysfs_emit(buf, "FAIL\n");
>  
>  		if (!(status & LP5523_LEDTEST_DONE))
>  			usleep_range(3000, 6000); /* Was not ready. Wait. */
>  
>  		ret = lp55xx_read(chip, LP5523_REG_LED_TEST_ADC, &adc);
>  		if (ret < 0)
> -			goto fail;
> +			return sysfs_emit(buf, "FAIL\n");
>  
>  		if (adc >= vdd || adc < LP5523_ADC_SHORTCIRC_LIM)
>  			pos += sysfs_emit_at(buf, pos, "LED %d FAIL\n",
> @@ -256,16 +257,8 @@ static ssize_t lp5523_selftest(struct device *dev,
>  			     led->led_current);
>  		led++;
>  	}
> -	if (pos == 0)
> -		pos = sysfs_emit(buf, "OK\n");
> -	goto release_lock;
> -fail:
> -	pos = sysfs_emit(buf, "FAIL\n");
>  
> -release_lock:
> -	mutex_unlock(&chip->lock);
> -
> -	return pos;
> +	return pos == 0 ? sysfs_emit(buf, "OK\n") : pos;
>  }
>  
>  LP55XX_DEV_ATTR_ENGINE_MODE(1);
> diff --git a/drivers/leds/leds-lp5562.c b/drivers/leds/leds-lp5562.c
> index 6ba5dbb9cace..69a4e3d5a126 100644
> --- a/drivers/leds/leds-lp5562.c
> +++ b/drivers/leds/leds-lp5562.c
> @@ -7,6 +7,7 @@
>   * Author: Milo(Woogyom) Kim <milo.kim@ti.com>
>   */
>  
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/firmware.h>
>  #include <linux/i2c.h>
> @@ -171,9 +172,9 @@ static int lp5562_led_brightness(struct lp55xx_led *led)
>  	};
>  	int ret;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	ret = lp55xx_write(chip, addr[led->chan_nr], led->brightness);
> -	mutex_unlock(&chip->lock);
>  
>  	return ret;
>  }
> @@ -268,9 +269,9 @@ static ssize_t lp5562_store_pattern(struct device *dev,
>  	if (mode > num_patterns || !ptn)
>  		return -EINVAL;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	ret = lp5562_run_predef_led_pattern(chip, mode);
> -	mutex_unlock(&chip->lock);
>  
>  	if (ret)
>  		return ret;
> @@ -320,9 +321,9 @@ static ssize_t lp5562_store_engine_mux(struct device *dev,
>  		return -EINVAL;
>  	}
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	lp55xx_update_bits(chip, LP5562_REG_ENG_SEL, mask, val);
> -	mutex_unlock(&chip->lock);
>  
>  	return len;
>  }
> diff --git a/drivers/leds/leds-lp5569.c b/drivers/leds/leds-lp5569.c
> index e5e7e61c8916..dc8efb25b78e 100644
> --- a/drivers/leds/leds-lp5569.c
> +++ b/drivers/leds/leds-lp5569.c
> @@ -4,6 +4,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/firmware.h>
>  #include <linux/i2c.h>
> @@ -396,17 +397,17 @@ static ssize_t lp5569_selftest(struct device *dev,
>  	struct lp55xx_chip *chip = led->chip;
>  	int i, pos = 0;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
>  
>  	/* Test LED Open */
>  	pos = lp5569_led_open_test(led, buf);
>  	if (pos < 0)
> -		goto fail;
> +		return sprintf(buf, "FAIL\n");
>  
>  	/* Test LED Shorted */
>  	pos += lp5569_led_short_test(led, buf);
>  	if (pos < 0)
> -		goto fail;
> +		return sprintf(buf, "FAIL\n");
>  
>  	for (i = 0; i < chip->pdata->num_channels; i++) {
>  		/* Restore current */
> @@ -419,16 +420,7 @@ static ssize_t lp5569_selftest(struct device *dev,
>  		led++;
>  	}
>  
> -	if (pos == 0)
> -		pos = sysfs_emit(buf, "OK\n");
> -	goto release_lock;
> -fail:
> -	pos = sysfs_emit(buf, "FAIL\n");
> -
> -release_lock:
> -	mutex_unlock(&chip->lock);
> -
> -	return pos;
> +	return pos == 0 ? sysfs_emit(buf, "OK\n") : pos;
>  }
>  
>  LP55XX_DEV_ATTR_ENGINE_MODE(1);
> diff --git a/drivers/leds/leds-lp55xx-common.c b/drivers/leds/leds-lp55xx-common.c
> index 1b71f512206d..f8cf5c0e983a 100644
> --- a/drivers/leds/leds-lp55xx-common.c
> +++ b/drivers/leds/leds-lp55xx-common.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
>  #include <linux/firmware.h>
> @@ -272,10 +273,10 @@ int lp55xx_led_brightness(struct lp55xx_led *led)
>  	const struct lp55xx_device_config *cfg = chip->cfg;
>  	int ret;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	ret = lp55xx_write(chip, cfg->reg_led_pwm_base.addr + led->chan_nr,
>  			   led->brightness);
> -	mutex_unlock(&chip->lock);
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(lp55xx_led_brightness);
> @@ -287,7 +288,8 @@ int lp55xx_multicolor_brightness(struct lp55xx_led *led)
>  	int ret;
>  	int i;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	for (i = 0; i < led->mc_cdev.num_colors; i++) {
>  		ret = lp55xx_write(chip,
>  				   cfg->reg_led_pwm_base.addr +
> @@ -296,7 +298,7 @@ int lp55xx_multicolor_brightness(struct lp55xx_led *led)
>  		if (ret)
>  			break;
>  	}
> -	mutex_unlock(&chip->lock);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(lp55xx_multicolor_brightness);
> @@ -404,9 +406,9 @@ static ssize_t led_current_store(struct device *dev,
>  	if (!chip->cfg->set_led_current)
>  		return len;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	chip->cfg->set_led_current(led, (u8)curr);
> -	mutex_unlock(&chip->lock);
>  
>  	return len;
>  }
> @@ -541,14 +543,12 @@ static void lp55xx_firmware_loaded(const struct firmware *fw, void *context)
>  	}
>  
>  	/* handling firmware data is chip dependent */
> -	mutex_lock(&chip->lock);
> -
> -	chip->engines[idx - 1].mode = LP55XX_ENGINE_LOAD;
> -	chip->fw = fw;
> -	if (chip->cfg->firmware_cb)
> -		chip->cfg->firmware_cb(chip);
> -
> -	mutex_unlock(&chip->lock);
> +	scoped_guard(mutex, &chip->lock) {
> +		chip->engines[idx - 1].mode = LP55XX_ENGINE_LOAD;
> +		chip->fw = fw;
> +		if (chip->cfg->firmware_cb)
> +			chip->cfg->firmware_cb(chip);
> +	}
>  
>  	/* firmware should be released for other channel use */
>  	release_firmware(chip->fw);
> @@ -592,10 +592,10 @@ static ssize_t select_engine_store(struct device *dev,
>  	case LP55XX_ENGINE_1:
>  	case LP55XX_ENGINE_2:
>  	case LP55XX_ENGINE_3:
> -		mutex_lock(&chip->lock);
> -		chip->engine_idx = val;
> -		ret = lp55xx_request_firmware(chip);
> -		mutex_unlock(&chip->lock);
> +		scoped_guard(mutex, &chip->lock) {
> +			chip->engine_idx = val;
> +			ret = lp55xx_request_firmware(chip);
> +		}
>  		break;
>  	default:
>  		dev_err(dev, "%lu: invalid engine index. (1, 2, 3)\n", val);
> @@ -634,9 +634,9 @@ static ssize_t run_engine_store(struct device *dev,
>  		return len;
>  	}
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	lp55xx_run_engine(chip, true);
> -	mutex_unlock(&chip->lock);
>  
>  	return len;
>  }
> @@ -673,7 +673,7 @@ ssize_t lp55xx_store_engine_mode(struct device *dev,
>  	const struct lp55xx_device_config *cfg = chip->cfg;
>  	struct lp55xx_engine *engine = &chip->engines[nr - 1];
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
>  
>  	chip->engine_idx = nr;
>  
> @@ -689,8 +689,6 @@ ssize_t lp55xx_store_engine_mode(struct device *dev,
>  		engine->mode = LP55XX_ENGINE_DISABLED;
>  	}
>  
> -	mutex_unlock(&chip->lock);
> -
>  	return len;
>  }
>  EXPORT_SYMBOL_GPL(lp55xx_store_engine_mode);
> @@ -703,14 +701,12 @@ ssize_t lp55xx_store_engine_load(struct device *dev,
>  	struct lp55xx_chip *chip = led->chip;
>  	int ret;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
>  
>  	chip->engine_idx = nr;
>  	lp55xx_load_engine(chip);
>  	ret = lp55xx_update_program_memory(chip, buf, len);
>  
> -	mutex_unlock(&chip->lock);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(lp55xx_store_engine_load);
> @@ -804,21 +800,17 @@ ssize_t lp55xx_store_engine_leds(struct device *dev,
>  	if (lp55xx_mux_parse(chip, buf, &mux, len))
>  		return -EINVAL;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
>  
>  	chip->engine_idx = nr;
> -	ret = -EINVAL;
>  
>  	if (engine->mode != LP55XX_ENGINE_LOAD)
> -		goto leave;
> +		return -EINVAL;
>  
>  	if (lp55xx_load_mux(chip, mux, nr))
> -		goto leave;
> +		return -EINVAL;
>  
> -	ret = len;
> -leave:
> -	mutex_unlock(&chip->lock);
> -	return ret;
> +	return len;
>  }
>  EXPORT_SYMBOL_GPL(lp55xx_store_engine_leds);
>  
> @@ -832,9 +824,9 @@ ssize_t lp55xx_show_master_fader(struct device *dev,
>  	int ret;
>  	u8 val;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	ret = lp55xx_read(chip, cfg->reg_master_fader_base.addr + nr - 1, &val);
> -	mutex_unlock(&chip->lock);
>  
>  	return ret ? ret : sysfs_emit(buf, "%u\n", val);
>  }
> @@ -856,10 +848,10 @@ ssize_t lp55xx_store_master_fader(struct device *dev,
>  	if (val > 0xff)
>  		return -EINVAL;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
> +
>  	ret = lp55xx_write(chip, cfg->reg_master_fader_base.addr + nr - 1,
>  			   (u8)val);
> -	mutex_unlock(&chip->lock);
>  
>  	return ret ? ret : len;
>  }
> @@ -875,25 +867,22 @@ ssize_t lp55xx_show_master_fader_leds(struct device *dev,
>  	int i, ret, pos = 0;
>  	u8 val;
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
>  
>  	for (i = 0; i < cfg->max_channel; i++) {
>  		ret = lp55xx_read(chip, cfg->reg_led_ctrl_base.addr + i, &val);
>  		if (ret)
> -			goto leave;
> +			return ret;
>  
>  		val = FIELD_GET(LP55xx_FADER_MAPPING_MASK, val);
>  		if (val > FIELD_MAX(LP55xx_FADER_MAPPING_MASK)) {
> -			ret = -EINVAL;
> -			goto leave;
> +			return -EINVAL;
>  		}
>  		buf[pos++] = val + '0';
>  	}
>  	buf[pos++] = '\n';
> -	ret = pos;
> -leave:
> -	mutex_unlock(&chip->lock);
> -	return ret;
> +
> +	return pos;
>  }
>  EXPORT_SYMBOL_GPL(lp55xx_show_master_fader_leds);
>  
> @@ -909,7 +898,7 @@ ssize_t lp55xx_store_master_fader_leds(struct device *dev,
>  
>  	n = min_t(int, len, cfg->max_channel);
>  
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);
>  
>  	for (i = 0; i < n; i++) {
>  		if (buf[i] >= '0' && buf[i] <= '3') {
> @@ -919,16 +908,13 @@ ssize_t lp55xx_store_master_fader_leds(struct device *dev,
>  						 LP55xx_FADER_MAPPING_MASK,
>  						 val);
>  			if (ret)
> -				goto leave;
> +				return ret;
>  		} else {
> -			ret = -EINVAL;
> -			goto leave;
> +			return -EINVAL;
>  		}
>  	}
> -	ret = len;
> -leave:
> -	mutex_unlock(&chip->lock);
> -	return ret;
> +
> +	return len;
>  }
>  EXPORT_SYMBOL_GPL(lp55xx_store_master_fader_leds);
>  
> -- 
> 2.45.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH 3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Markus Elfring]

…
> +++ b/drivers/leds/leds-lp5521.c
…
> @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev,
>  	struct lp55xx_chip *chip = led->chip;
>  	int ret;
>
> -	mutex_lock(&chip->lock);
> +	guard(mutex, &chip->lock);

How did you come to the conclusion to try such a syntax variant out?

Would the following statement (with additional parentheses) be more appropriate?

	guard(mutex)(&chip->lock);


Regards,
Markus

Re: [PATCH 3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Lee Jones]

On Wed, 10 Jul 2024, Markus Elfring wrote:

> …
> > +++ b/drivers/leds/leds-lp5521.c
> …
> > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev,
> >  	struct lp55xx_chip *chip = led->chip;
> >  	int ret;
> >
> > -	mutex_lock(&chip->lock);
> > +	guard(mutex, &chip->lock);
> 
> How did you come to the conclusion to try such a syntax variant out?
> 
> Would the following statement (with additional parentheses) be more appropriate?
> 
> 	guard(mutex)(&chip->lock);

Yes, that's the fix.

I'm more concerned with how untested patches came to being submitted.

-- 
Lee Jones [李琼斯]

Re: [PATCH 3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Christian Marangi]

On Wed, Jul 10, 2024 at 05:55:28PM +0100, Lee Jones wrote:
> On Wed, 10 Jul 2024, Markus Elfring wrote:
> 
> > …
> > > +++ b/drivers/leds/leds-lp5521.c
> > …
> > > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev,
> > >  	struct lp55xx_chip *chip = led->chip;
> > >  	int ret;
> > >
> > > -	mutex_lock(&chip->lock);
> > > +	guard(mutex, &chip->lock);
> > 
> > How did you come to the conclusion to try such a syntax variant out?
> > 
> > Would the following statement (with additional parentheses) be more appropriate?
> > 
> > 	guard(mutex)(&chip->lock);
> 
> Yes, that's the fix.
> 
> I'm more concerned with how untested patches came to being submitted.
>

Hi Lee,
profoundly sorry for the happening... Obviusly something went wrong in
me changing branch and the driver wasn't actually compiled in the
test... 

Also with the comments from Markus I tought this needed more changes and
I leaved out for a bit, so again I'm really sorry that this manage to
reach next.

What is the next step? Any way I can pose a fix on this and apologize for
the situation?

-- 
	Ansuel

Re: [3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Markus Elfring]

> What is the next step?

I hope that you would dare to offer a subsequent improved patch version.

Regards,
Markus

Re: [3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Lee Jones]

On Wed, 10 Jul 2024, Markus Elfring wrote:

> > What is the next step?
> 
> I hope that you would dare to offer a subsequent improved patch version.

That arrangement should only be made between the submitter and the
maintainer.

-- 
Lee Jones [李琼斯]

Re: [PATCH 3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API

[Lee Jones]

On Wed, 10 Jul 2024, Christian Marangi wrote:

> On Wed, Jul 10, 2024 at 05:55:28PM +0100, Lee Jones wrote:
> > On Wed, 10 Jul 2024, Markus Elfring wrote:
> > 
> > > …
> > > > +++ b/drivers/leds/leds-lp5521.c
> > > …
> > > > @@ -185,9 +186,9 @@ static ssize_t lp5521_selftest(struct device *dev,
> > > >  	struct lp55xx_chip *chip = led->chip;
> > > >  	int ret;
> > > >
> > > > -	mutex_lock(&chip->lock);
> > > > +	guard(mutex, &chip->lock);
> > > 
> > > How did you come to the conclusion to try such a syntax variant out?
> > > 
> > > Would the following statement (with additional parentheses) be more appropriate?
> > > 
> > > 	guard(mutex)(&chip->lock);
> > 
> > Yes, that's the fix.
> > 
> > I'm more concerned with how untested patches came to being submitted.
> >
> 
> Hi Lee,
> profoundly sorry for the happening... Obviusly something went wrong in
> me changing branch and the driver wasn't actually compiled in the
> test... 
> 
> Also with the comments from Markus I tought this needed more changes and
> I leaved out for a bit, so again I'm really sorry that this manage to
> reach next.

No worries.

> What is the next step? Any way I can pose a fix on this and apologize for
> the situation?

I'll fix it up and test it.

-- 
Lee Jones [李琼斯]

Re: [PATCH 1/3] leds: leds-lp5569: Convert to sysfs_emit API

[Lee Jones]

On Thu, 27 Jun 2024 00:15:11 +0200, Christian Marangi wrote:
> Convert sprintf to the much safer sysfs_emit API to handle output for
> sysfs.
> 
> Also better handle situation where on the same chip there may be LED
> open and shorted at the same time.
> 
> 
> [...]

Applied, thanks!

[1/3] leds: leds-lp5569: Convert to sysfs_emit API
      commit: 6f2fdde9096f3c4d35a7711c91a78c086be66aed
[2/3] leds: leds-lp5523: Convert to sysfs_emit API
      commit: 8eac0379d3bd9d048b1144d74d9309a198fd3f40
[3/3] leds: leds-lp55xx: Convert mutex lock/unlock to guard API
      commit: efd0d1cbb8c5dd1049922e839fa7d85811facd53

--
Lee Jones [李琼斯]