[PATCH v1 0/6] leds: spi-byte: Cleanup, fix a leak, and make it agnostic

[PATCH v1 0/6] leds: spi-byte: Cleanup, fix a leak, and make it agnostic

[Andy Shevchenko]

Clean up the driver, fix one leak, and make it possible to use outside
of OF systems.

Andy Shevchenko (6):
  leds: spi-byte: call of_node_put() on error path
  leds: spi-byte: Get rid of custom led_init_default_state_get()
  leds: spi-byte: Make use of device properties
  leds: spi-byte: Utilise temporary variable for struct device
  leds: spi-byte: Use devm_mutex_init() for mutex initialization
  leds: spi-byte: Move OF ID table closer to their user

 drivers/leds/Kconfig         |  1 -
 drivers/leds/leds-spi-byte.c | 63 +++++++++++++-----------------------
 2 files changed, 22 insertions(+), 42 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/6] leds: spi-byte: call of_node_put() on error path

[Andy Shevchenko]

Add a missing call to of_node_put(np) on error.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/leds-spi-byte.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
index 96296db5f410..b04cf502e603 100644
--- a/drivers/leds/leds-spi-byte.c
+++ b/drivers/leds/leds-spi-byte.c
@@ -91,7 +91,6 @@ static int spi_byte_probe(struct spi_device *spi)
 		dev_err(dev, "Device must have exactly one LED sub-node.");
 		return -EINVAL;
 	}
-	child = of_get_next_available_child(dev_of_node(dev), NULL);
 
 	led = devm_kzalloc(dev, sizeof(*led), GFP_KERNEL);
 	if (!led)
@@ -104,11 +103,13 @@ static int spi_byte_probe(struct spi_device *spi)
 	led->ldev.max_brightness = led->cdef->max_value - led->cdef->off_value;
 	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
 
+	child = of_get_next_available_child(dev_of_node(dev), NULL);
 	state = of_get_property(child, "default-state", NULL);
 	if (state) {
 		if (!strcmp(state, "on")) {
 			led->ldev.brightness = led->ldev.max_brightness;
 		} else if (strcmp(state, "off")) {
+			of_node_put(child);
 			/* all other cases except "off" */
 			dev_err(dev, "default-state can only be 'on' or 'off'");
 			return -EINVAL;
@@ -123,9 +124,12 @@ static int spi_byte_probe(struct spi_device *spi)
 
 	ret = devm_led_classdev_register_ext(&spi->dev, &led->ldev, &init_data);
 	if (ret) {
+		of_node_put(child);
 		mutex_destroy(&led->mutex);
 		return ret;
 	}
+
+	of_node_put(child);
 	spi_set_drvdata(spi, led);
 
 	return 0;
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/6] leds: spi-byte: call of_node_put() on error path

[Andy Shevchenko]

On Thu, Jun 06, 2024 at 08:29:18PM +0300, Andy Shevchenko wrote:
> Add a missing call to of_node_put(np) on error.

Oh, yeah, this might need the Fixes tag. depending if you want to backport
this fix or no hurry.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/6] leds: spi-byte: Get rid of custom led_init_default_state_get()

[Andy Shevchenko]

LED core provides a helper to parse default state from firmware node.
Use it instead of custom implementation.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/leds-spi-byte.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
index b04cf502e603..76733946c99a 100644
--- a/drivers/leds/leds-spi-byte.c
+++ b/drivers/leds/leds-spi-byte.c
@@ -84,7 +84,7 @@ static int spi_byte_probe(struct spi_device *spi)
 	struct device *dev = &spi->dev;
 	struct spi_byte_led *led;
 	struct led_init_data init_data = {};
-	const char *state;
+	enum led_default_state state;
 	int ret;
 
 	if (of_get_available_child_count(dev_of_node(dev)) != 1) {
@@ -104,17 +104,10 @@ static int spi_byte_probe(struct spi_device *spi)
 	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
 
 	child = of_get_next_available_child(dev_of_node(dev), NULL);
-	state = of_get_property(child, "default-state", NULL);
-	if (state) {
-		if (!strcmp(state, "on")) {
-			led->ldev.brightness = led->ldev.max_brightness;
-		} else if (strcmp(state, "off")) {
-			of_node_put(child);
-			/* all other cases except "off" */
-			dev_err(dev, "default-state can only be 'on' or 'off'");
-			return -EINVAL;
-		}
-	}
+
+	state = led_init_default_state_get(of_fwnode_handle(child));
+	if (state == LEDS_DEFSTATE_ON)
+		led->ldev.brightness = led->ldev.max_brightness;
 	spi_byte_brightness_set_blocking(&led->ldev,
 					 led->ldev.brightness);
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 3/6] leds: spi-byte: Make use of device properties

[Andy Shevchenko]

Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.

Add mod_devicetable.h include.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/Kconfig         |  1 -
 drivers/leds/leds-spi-byte.c | 15 +++++++--------
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7211f0a4f88f..2e1925d2068c 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -865,7 +865,6 @@ config LEDS_SPI_BYTE
 	tristate "LED support for SPI LED controller with a single byte"
 	depends on LEDS_CLASS
 	depends on SPI
-	depends on OF
 	help
 	  This option enables support for LED controller which use a single byte
 	  for controlling the brightness. Currently the following controller is
diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
index 76733946c99a..1fc0a8cc123e 100644
--- a/drivers/leds/leds-spi-byte.c
+++ b/drivers/leds/leds-spi-byte.c
@@ -29,8 +29,9 @@
  */
 
 #include <linux/leds.h>
+#include <linux/mod_devicetable.h>
 #include <linux/module.h>
-#include <linux/of.h>
+#include <linux/property.h>
 #include <linux/spi/spi.h>
 #include <linux/mutex.h>
 #include <uapi/linux/uleds.h>
@@ -80,14 +81,14 @@ static int spi_byte_brightness_set_blocking(struct led_classdev *dev,
 
 static int spi_byte_probe(struct spi_device *spi)
 {
-	struct device_node *child;
+	struct fwnode_handle *child __free(fwnode_handle) = NULL;
 	struct device *dev = &spi->dev;
 	struct spi_byte_led *led;
 	struct led_init_data init_data = {};
 	enum led_default_state state;
 	int ret;
 
-	if (of_get_available_child_count(dev_of_node(dev)) != 1) {
+	if (device_get_child_node_count(dev) != 1) {
 		dev_err(dev, "Device must have exactly one LED sub-node.");
 		return -EINVAL;
 	}
@@ -103,26 +104,24 @@ static int spi_byte_probe(struct spi_device *spi)
 	led->ldev.max_brightness = led->cdef->max_value - led->cdef->off_value;
 	led->ldev.brightness_set_blocking = spi_byte_brightness_set_blocking;
 
-	child = of_get_next_available_child(dev_of_node(dev), NULL);
+	child = device_get_next_child_node(dev, NULL);
 
-	state = led_init_default_state_get(of_fwnode_handle(child));
+	state = led_init_default_state_get(child);
 	if (state == LEDS_DEFSTATE_ON)
 		led->ldev.brightness = led->ldev.max_brightness;
 	spi_byte_brightness_set_blocking(&led->ldev,
 					 led->ldev.brightness);
 
-	init_data.fwnode = of_fwnode_handle(child);
+	init_data.fwnode = child;
 	init_data.devicename = "leds-spi-byte";
 	init_data.default_label = ":";
 
 	ret = devm_led_classdev_register_ext(&spi->dev, &led->ldev, &init_data);
 	if (ret) {
-		of_node_put(child);
 		mutex_destroy(&led->mutex);
 		return ret;
 	}
 
-	of_node_put(child);
 	spi_set_drvdata(spi, led);
 
 	return 0;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 4/6] leds: spi-byte: Utilise temporary variable for struct device

[Andy Shevchenko]

We have a temporary variable to keep a pointer to struct device.
Utilise it where it makes sense.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/leds-spi-byte.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
index 1fc0a8cc123e..e63958e584c2 100644
--- a/drivers/leds/leds-spi-byte.c
+++ b/drivers/leds/leds-spi-byte.c
@@ -116,7 +116,7 @@ static int spi_byte_probe(struct spi_device *spi)
 	init_data.devicename = "leds-spi-byte";
 	init_data.default_label = ":";
 
-	ret = devm_led_classdev_register_ext(&spi->dev, &led->ldev, &init_data);
+	ret = devm_led_classdev_register_ext(dev, &led->ldev, &init_data);
 	if (ret) {
 		mutex_destroy(&led->mutex);
 		return ret;
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 5/6] leds: spi-byte: Use devm_mutex_init() for mutex initialization

[Andy Shevchenko]

In this driver LEDs are registered using devm_led_classdev_register()
so they are automatically unregistered after module's remove() is done.
led_classdev_unregister() calls module's led_set_brightness() to turn off
the LEDs and that callback uses mutex which was destroyed already
in module's remove() so use devm API instead.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/leds-spi-byte.c | 26 ++++++--------------------
 1 file changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
index e63958e584c2..985bbbed251b 100644
--- a/drivers/leds/leds-spi-byte.c
+++ b/drivers/leds/leds-spi-byte.c
@@ -31,9 +31,9 @@
 #include <linux/leds.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/property.h>
 #include <linux/spi/spi.h>
-#include <linux/mutex.h>
 #include <uapi/linux/uleds.h>
 
 struct spi_byte_chipdef {
@@ -97,8 +97,11 @@ static int spi_byte_probe(struct spi_device *spi)
 	if (!led)
 		return -ENOMEM;
 
+	ret = devm_mutex_init(dev, &led->mutex);
+	if (ret)
+		return ret;
+
 	led->spi = spi;
-	mutex_init(&led->mutex);
 	led->cdef = device_get_match_data(dev);
 	led->ldev.brightness = LED_OFF;
 	led->ldev.max_brightness = led->cdef->max_value - led->cdef->off_value;
@@ -116,33 +119,16 @@ static int spi_byte_probe(struct spi_device *spi)
 	init_data.devicename = "leds-spi-byte";
 	init_data.default_label = ":";
 
-	ret = devm_led_classdev_register_ext(dev, &led->ldev, &init_data);
-	if (ret) {
-		mutex_destroy(&led->mutex);
-		return ret;
-	}
-
-	spi_set_drvdata(spi, led);
-
-	return 0;
-}
-
-static void spi_byte_remove(struct spi_device *spi)
-{
-	struct spi_byte_led	*led = spi_get_drvdata(spi);
-
-	mutex_destroy(&led->mutex);
+	return devm_led_classdev_register_ext(dev, &led->ldev, &init_data);
 }
 
 static struct spi_driver spi_byte_driver = {
 	.probe		= spi_byte_probe,
-	.remove		= spi_byte_remove,
 	.driver = {
 		.name		= KBUILD_MODNAME,
 		.of_match_table	= spi_byte_dt_ids,
 	},
 };
-
 module_spi_driver(spi_byte_driver);
 
 MODULE_AUTHOR("Christian Mauderer <oss@c-mauderer.de>");
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 6/6] leds: spi-byte: Move OF ID table closer to their user

[Andy Shevchenko]

There is no code that uses ID table directly, except the
struct device_driver at the end of the file. Hence, move
table closer to its user. It's always possible to access
them via a pointer.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/leds/leds-spi-byte.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/leds/leds-spi-byte.c b/drivers/leds/leds-spi-byte.c
index 985bbbed251b..d24d0ddf347c 100644
--- a/drivers/leds/leds-spi-byte.c
+++ b/drivers/leds/leds-spi-byte.c
@@ -56,13 +56,6 @@ static const struct spi_byte_chipdef ubnt_acb_spi_led_cdef = {
 	.max_value = 0x3F,
 };
 
-static const struct of_device_id spi_byte_dt_ids[] = {
-	{ .compatible = "ubnt,acb-spi-led", .data = &ubnt_acb_spi_led_cdef },
-	{},
-};
-
-MODULE_DEVICE_TABLE(of, spi_byte_dt_ids);
-
 static int spi_byte_brightness_set_blocking(struct led_classdev *dev,
 					    enum led_brightness brightness)
 {
@@ -122,6 +115,12 @@ static int spi_byte_probe(struct spi_device *spi)
 	return devm_led_classdev_register_ext(dev, &led->ldev, &init_data);
 }
 
+static const struct of_device_id spi_byte_dt_ids[] = {
+	{ .compatible = "ubnt,acb-spi-led", .data = &ubnt_acb_spi_led_cdef },
+	{}
+};
+MODULE_DEVICE_TABLE(of, spi_byte_dt_ids);
+
 static struct spi_driver spi_byte_driver = {
 	.probe		= spi_byte_probe,
 	.driver = {
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 0/6] leds: spi-byte: Cleanup, fix a leak, and make it agnostic

[Lee Jones]

On Thu, 06 Jun 2024 20:29:17 +0300, Andy Shevchenko wrote:
> Clean up the driver, fix one leak, and make it possible to use outside
> of OF systems.
> 
> Andy Shevchenko (6):
>   leds: spi-byte: call of_node_put() on error path
>   leds: spi-byte: Get rid of custom led_init_default_state_get()
>   leds: spi-byte: Make use of device properties
>   leds: spi-byte: Utilise temporary variable for struct device
>   leds: spi-byte: Use devm_mutex_init() for mutex initialization
>   leds: spi-byte: Move OF ID table closer to their user
> 
> [...]

Applied, thanks!

[1/6] leds: spi-byte: call of_node_put() on error path
      commit: 057739faa00602ca64090e084a664715ee74eb3c
[2/6] leds: spi-byte: Get rid of custom led_init_default_state_get()
      commit: 9cb832b2c7bcd8b5decd9e9c07302c802ee25faa
[3/6] leds: spi-byte: Make use of device properties
      commit: 3e3735a40ec6694fa70ae20d33545783b9d555af
[4/6] leds: spi-byte: Utilise temporary variable for struct device
      commit: dc5f64e4c1531d935c239eb4db3957c5cf569bb6
[5/6] leds: spi-byte: Use devm_mutex_init() for mutex initialization
      commit: cc5f66ad5640340ab8fb5753acc7218e2b60d36c
[6/6] leds: spi-byte: Move OF ID table closer to their user
      commit: 2ac2628cc23cae5c3a317eaa7f0d3a1d9359a3c9

--
Lee Jones [李琼斯]