Allow to use leds-ns2 with n090401 boards

From: Vincent Donnefort <[email protected]>

On the board n090401 (Seagate NAS 4-Bay), the LED mode mapping (GPIO values to LED mode) is different from the one used on other boards supported by the leds-ns2 driver.

With this patch the hardcoded mapping is removed from leds-ns2. Now, it must be defined either in the platform data (if an old-fashion board setup file is used) or in the DT node. In order to allow the later, this patch also introduces a modes-map property for the leds-ns2 DT binding.

Signed-off-by Vincent Donnefort <[email protected]>

---
  .../devicetree/bindings/leds/leds-ns2.txt          |   9 ++
  drivers/leds/leds-ns2.c                            | 102 +++++++++++----------
  include/dt-bindings/leds/leds-ns2.h                |   8 ++
  include/linux/platform_data/leds-kirkwood-ns2.h    |  14 +++
  4 files changed, 85 insertions(+), 48 deletions(-)

create mode 100644 include/dt-bindings/leds/leds-ns2.h

Documentation/devicetree/bindings/leds/leds-ns2.txt +5 -0

index aef3aca34d2d..9f81258a5b6e 100644
@@ -8,6 +8,9 @@ Each LED is represented as a sub-node of the ns2-leds device.
  Required sub-node properties:
  - cmd-gpio: Command LED GPIO. See OF device-tree GPIO specification.
  - slow-gpio: Slow LED GPIO. See OF device-tree GPIO specification.
+- modes-map: A mapping between LED modes (off, on or SATA activity blinking) and
+  the corresponding cmd-gpio/slow-gpio values. All the GPIO values combinations
+  should be given in order to avoid having an unknown mode at driver probe time.
 
  Optional sub-node properties:
  - label: Name for this LED. If omitted, the label is taken from the node name.

@@ -15,6 +18,8 @@ Optional sub-node properties:
 
  Example:
 
+#include <dt-bindings/leds/leds-ns2.h>
+
  ns2-leds {
  	compatible = "lacie,ns2-leds";
 

drivers/leds/leds-ns2.c +22 -45

index 1fd6adbb43b7..b0bc03539dbb 100644
@@ -33,46 +33,20 @@
  #include <linux/of_gpio.h>
 
  /*
- * The Network Space v2 dual-GPIO LED is wired to a CPLD and can blink in
- * relation with the SATA activity. This capability is exposed through the
- * "sata" sysfs attribute.
- *
- * The following array detail the different LED registers and the combination
- * of their possible values:
- *
- *  cmd_led   |  slow_led  | /SATA active | LED state
- *            |            |              |
- *     1      |     0      |      x       |  off
- *     -      |     1      |      x       |  on
- *     0      |     0      |      1       |  on
- *     0      |     0      |      0       |  blink (rate 300ms)
+ * The Network Space v2 dual-GPIO LED is wired to a CPLD. Three different LED
+ * modes are available: off, on and SATA activity blinking. The LED modes are
+ * controlled through two GPIOs (command and slow): each combination of values
+ * for the command/slow GPIOs corresponds to a LED mode.
   */
 
-enum ns2_led_modes {
-	NS_V2_LED_OFF,
-	NS_V2_LED_ON,
-	NS_V2_LED_SATA,
-};
-
-struct ns2_led_mode_value {
-	enum ns2_led_modes	mode;
-	int			cmd_level;
-	int			slow_level;
-};
-
-static struct ns2_led_mode_value ns2_led_modval[] = {
-	{ NS_V2_LED_OFF	, 1, 0 },
-	{ NS_V2_LED_ON	, 0, 1 },
-	{ NS_V2_LED_ON	, 1, 1 },
-	{ NS_V2_LED_SATA, 0, 0 },
-};
-
  struct ns2_led_data {
  	struct led_classdev	cdev;
  	unsigned		cmd;
  	unsigned		slow;
  	unsigned char		sata; /* True when SATA mode active. */
  	rwlock_t		rw_lock; /* Lock GPIOs. */
+	int			num_modes;
+	struct ns2_led_modval	*modval;
  };
 
  static int ns2_led_get_mode(struct ns2_led_data *led_dat,

@@ -88,10 +62,10 @@ static int ns2_led_get_mode(struct ns2_led_data *led_dat,
  	cmd_level = gpio_get_value(led_dat->cmd);
  	slow_level = gpio_get_value(led_dat->slow);
 
-	for (i = 0; i < ARRAY_SIZE(ns2_led_modval); i++) {
-		if (cmd_level == ns2_led_modval[i].cmd_level &&
-		    slow_level == ns2_led_modval[i].slow_level) {
-			*mode = ns2_led_modval[i].mode;
+	for (i = 0; i < led_dat->num_modes; i++) {
+		if (cmd_level == led_dat->modval[i].cmd_level &&
+		    slow_level == led_dat->modval[i].slow_level) {
+			*mode = led_dat->modval[i].mode;
  			ret = 0;
  			break;
  		}

@@ -110,12 +84,12 @@ static void ns2_led_set_mode(struct ns2_led_data *led_dat,
 
  	write_lock_irqsave(&led_dat->rw_lock, flags);
 
-	for (i = 0; i < ARRAY_SIZE(ns2_led_modval); i++) {
-		if (mode == ns2_led_modval[i].mode) {
+	for (i = 0; i < led_dat->num_modes; i++) {
+		if (mode == led_dat->modval[i].mode) {
  			gpio_set_value(led_dat->cmd,
-				       ns2_led_modval[i].cmd_level);
+				       led_dat->modval[i].cmd_level);
  			gpio_set_value(led_dat->slow,
-				       ns2_led_modval[i].slow_level);
+				       led_dat->modval[i].slow_level);
  		}
  	}
 

@@ -228,6 +202,8 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
  	led_dat->cdev.groups = ns2_led_groups;
  	led_dat->cmd = template->cmd;
  	led_dat->slow = template->slow;
+	led_dat->modval = template->modval;
+	led_dat->num_modes = template->num_modes;
 
  	ret = ns2_led_get_mode(led_dat, &mode);
  	if (ret < 0)

@@ -259,9 +235,8 @@ ns2_leds_get_of_pdata(struct device *dev, struct ns2_led_platform_data *pdata)
  {
  	struct device_node *np = dev->of_node;
  	struct device_node *child;
-	struct ns2_led *leds;
+	struct ns2_led *led, *leds;
  	int num_leds = 0;
-	int i = 0;
 
  	num_leds = of_get_child_count(np);
  	if (!num_leds)

@@ -272,26 +247,57 @@ ns2_leds_get_of_pdata(struct device *dev, struct ns2_led_platform_data *pdata)
  	if (!leds)
  		return -ENOMEM;
 
+	led = leds;
  	for_each_child_of_node(np, child) {
  		const char *string;
-		int ret;
+		int ret, i, num_modes;
+		struct ns2_led_modval *modval;
 
  		ret = of_get_named_gpio(child, "cmd-gpio", 0);
  		if (ret < 0)
  			return ret;
-		leds[i].cmd = ret;
+		led->cmd = ret;
  		ret = of_get_named_gpio(child, "slow-gpio", 0);
  		if (ret < 0)
  			return ret;
-		leds[i].slow = ret;
+		led->slow = ret;

drivers/leds/leds-ns2.c +28 -1

+		if (ret < 0 || ret % 3) {
+			dev_err(dev,
+				"Missing or malformed modes-map property\n");
+			return -EINVAL;
+		}
+
+		num_modes = ret / 3;
+		modval = devm_kzalloc(dev,
+				      num_modes * sizeof(struct ns2_led_modval),
+				      GFP_KERNEL);
+		if (!modval)
+			return -ENOMEM;
+
+		for (i = 0; i < num_modes; i++) {
+			of_property_read_u32_index(child,
+						"modes-map", 3 * i,
+						(u32 *) &modval[i].mode);
+			of_property_read_u32_index(child,
+						"modes-map", 3 * i + 1,
+						(u32 *) &modval[i].cmd_level);
+			of_property_read_u32_index(child,
+						"modes-map", 3 * i + 2,
+						(u32 *) &modval[i].slow_level);
+		}
+
+		led->num_modes = num_modes;
+		led->modval = modval;
 
-		i++;
+		led++;
  	}
 
  	pdata->leds = leds;

include/dt-bindings/leds/leds-ns2.h new file +8 -0

new file mode 100644
index 000000000000..491c5f974a92
@@ -0,0 +1,8 @@
+#ifndef _DT_BINDINGS_LEDS_NS2_H
+#define _DT_BINDINGS_LEDS_NS2_H
+
+#define NS_V2_LED_OFF	0
+#define NS_V2_LED_ON	1
+#define NS_V2_LED_SATA	2
+
+#endif

include/linux/platform_data/leds-kirkwood-ns2.h +14 -0

index 6a9fed57f346..eb8a6860e816 100644
@@ -9,11 +9,25 @@
  #ifndef __LEDS_KIRKWOOD_NS2_H
  #define __LEDS_KIRKWOOD_NS2_H
 
+enum ns2_led_modes {
+	NS_V2_LED_OFF,
+	NS_V2_LED_ON,
+	NS_V2_LED_SATA,
+};
+
+struct ns2_led_modval {
+	enum ns2_led_modes	mode;
+	int			cmd_level;
+	int			slow_level;
+};
+
  struct ns2_led {
  	const char	*name;
  	const char	*default_trigger;
  	unsigned	cmd;
  	unsigned	slow;
+	int		num_modes;
+	struct ns2_led_modval *modval;
  };
 
  struct ns2_led_platform_data {

Hi Simon,

Quoted 43 lines

From: Vincent Donnefort <[email protected]>

On the board n090401 (Seagate NAS 4-Bay), the LED mode mapping (GPIO values to LED mode) is different from the one used on other boards supported by the leds-ns2 driver.

With this patch the hardcoded mapping is removed from leds-ns2. Now, it must be defined either in the platform data (if an old-fashion board setup file is used) or in the DT node. In order to allow the later, this patch also introduces a modes-map property for the leds-ns2 DT binding.

Signed-off-by Vincent Donnefort <[email protected]>

---
 .../devicetree/bindings/leds/leds-ns2.txt          |   9 ++
 drivers/leds/leds-ns2.c                            | 102 +++++++++++----------
 include/dt-bindings/leds/leds-ns2.h                |   8 ++
 include/linux/platform_data/leds-kirkwood-ns2.h    |  14 +++
 4 files changed, 85 insertions(+), 48 deletions(-)
 create mode 100644 include/dt-bindings/leds/leds-ns2.h

Documentation/devicetree/bindings/leds/leds-ns2.txt +5 -0

index aef3aca34d2d..9f81258a5b6e 100644
@@ -8,6 +8,9 @@ Each LED is represented as a sub-node of the ns2-leds device.
 Required sub-node properties:
 - cmd-gpio: Command LED GPIO. See OF device-tree GPIO specification.
 - slow-gpio: Slow LED GPIO. See OF device-tree GPIO specification.
+- modes-map: A mapping between LED modes (off, on or SATA activity blinking) and
+  the corresponding cmd-gpio/slow-gpio values. All the GPIO values combinations
+  should be given in order to avoid having an unknown mode at driver probe time.
 
 Optional sub-node properties:
 - label: Name for this LED. If omitted, the label is taken from the node name.

@@ -15,6 +18,8 @@ Optional sub-node properties:
 
 Example:
 
+#include <dt-bindings/leds/leds-ns2.h>
+
 ns2-leds {
 	compatible = "lacie,ns2-leds";
 

Documentation/devicetree/bindings/leds/leds-ns2.txt ns2-leds { · line 35

Quoted 10 lines

Documentation/devicetree/bindings/leds/leds-ns2.txt +4 -0

@@ -22,5 +27,9 @@ ns2-leds {
 		label = "ns2:blue:sata";
 		slow-gpio = <&gpio0 29 0>;
 		cmd-gpio = <&gpio0 30 0>;
+		modes-map = <NS_V2_LED_OFF  0 1
+			     NS_V2_LED_ON   1 0
+			     NS_V2_LED_ON   0 0
+			     NS_V2_LED_SATA 1 1>;
 	};
 };

This looks good to me but please cc devicetree at vger.kernel.org when you modify DT bindings.

Quoted 47 lines

drivers/leds/leds-ns2.c +4 -32

index 1fd6adbb43b7..b0bc03539dbb 100644
@@ -33,46 +33,20 @@
 #include <linux/of_gpio.h>
 
 /*
- * The Network Space v2 dual-GPIO LED is wired to a CPLD and can blink in
- * relation with the SATA activity. This capability is exposed through the
- * "sata" sysfs attribute.
- *
- * The following array detail the different LED registers and the combination
- * of their possible values:
- *
- *  cmd_led   |  slow_led  | /SATA active | LED state
- *            |            |              |
- *     1      |     0      |      x       |  off
- *     -      |     1      |      x       |  on
- *     0      |     0      |      1       |  on
- *     0      |     0      |      0       |  blink (rate 300ms)
+ * The Network Space v2 dual-GPIO LED is wired to a CPLD. Three different LED
+ * modes are available: off, on and SATA activity blinking. The LED modes are
+ * controlled through two GPIOs (command and slow): each combination of values
+ * for the command/slow GPIOs corresponds to a LED mode.
  */
 
-enum ns2_led_modes {
-	NS_V2_LED_OFF,
-	NS_V2_LED_ON,
-	NS_V2_LED_SATA,
-};
-
-struct ns2_led_mode_value {
-	enum ns2_led_modes	mode;
-	int			cmd_level;
-	int			slow_level;
-};
-
-static struct ns2_led_mode_value ns2_led_modval[] = {
-	{ NS_V2_LED_OFF	, 1, 0 },
-	{ NS_V2_LED_ON	, 0, 1 },
-	{ NS_V2_LED_ON	, 1, 1 },
-	{ NS_V2_LED_SATA, 0, 0 },
-};
-

Quoted 23 lines

drivers/leds/leds-ns2.c +6 -4

+	led = leds;
 	for_each_child_of_node(np, child) {
 		const char *string;
-		int ret;
+		int ret, i, num_modes;
+		struct ns2_led_modval *modval;
 
 		ret = of_get_named_gpio(child, "cmd-gpio", 0);
 		if (ret < 0)
 			return ret;
-		leds[i].cmd = ret;
+		led->cmd = ret;
 		ret = of_get_named_gpio(child, "slow-gpio", 0);
 		if (ret < 0)
 			return ret;
-		leds[i].slow = ret;
+		led->slow = ret;
 		ret = of_property_read_string(child, "label", &string);
-		leds[i].name = (ret == 0) ? string : child->name;
+		led->name = (ret == 0) ? string : child->name;
 		ret = of_property_read_string(child, "linux,default-trigger",
 					      &string);
 		if (ret == 0)

drivers/leds/leds-ns2.c ns2_leds_get_of_pdata(struct device *dev, struct ns2_led_platform_data *pdata) · line 271

Quoted 4 lines

drivers/leds/leds-ns2.c +3 -1

-			leds[i].default_trigger = string;
+			led->default_trigger = string;
+
+		ret = of_property_count_u32_elems(child, "modes-map");

I think that we shouldn't fail if the property is absent, but default to the mapping that is currently hard coded in the driver. Otherwise we would break existing users.

Quoted 24 lines

Quoted 24 lines

drivers/leds/leds-ns2.c +6 -4

+	led = leds;
  	for_each_child_of_node(np, child) {
  		const char *string;
-		int ret;
+		int ret, i, num_modes;
+		struct ns2_led_modval *modval;
 
  		ret = of_get_named_gpio(child, "cmd-gpio", 0);
  		if (ret < 0)
  			return ret;
-		leds[i].cmd = ret;
+		led->cmd = ret;
  		ret = of_get_named_gpio(child, "slow-gpio", 0);
  		if (ret < 0)
  			return ret;
-		leds[i].slow = ret;
+		led->slow = ret;
  		ret = of_property_read_string(child, "label", &string);
-		leds[i].name = (ret == 0) ? string : child->name;
+		led->name = (ret == 0) ? string : child->name;
  		ret = of_property_read_string(child, "linux,default-trigger",
  					      &string);
  		if (ret == 0)

Quoted 8 lines

drivers/leds/leds-ns2.c ns2_leds_get_of_pdata(struct device *dev, struct ns2_led_platform_data *pdata) · line 271

Quoted 4 lines

drivers/leds/leds-ns2.c +3 -1

-			leds[i].default_trigger = string;
+			led->default_trigger = string;
+
+		ret = of_property_count_u32_elems(child, "modes-map");

I think that we shouldn't fail if the property is absent, but default to the mapping that is currently hard coded in the driver. Otherwise we would break existing users.

I don't think there is a risk of breaking existing users. On platforms where the leds-ns2 driver is used, DTB will be updated with the kernel image. Moreover, removing the hard coded mapping is a nice clean-up.

Let me know if you still want me to add a fallback.

On the board n090401 (Seagate NAS 4-Bay), some of the LEDs are handled by the leds-ns2 driver. This LEDs are connected to an I2C GPIO expander (PCA95554PW) which means that GPIO access may sleep. This patch makes leds-ns2 compatible with such GPIOs by using the *_cansleep() variant of the GPIO functions. As a drawback this functions can't be used safely in a timer context (with the timer LED trigger for example). To fix this issue, a workqueue mechanism (copied from the leds-gpio driver) is used.

Note that this patch also updates slightly the ns2_led_sata_store function. The LED state is now retrieved from cached values instead of reading the GPIOs previously. This prevents ns2_led_sata_store from working with a stale LED state (which may happen when a delayed work is pending).

Signed-off-by Simon Guinot <[email protected]>

Signed-off-by Vincent Donnefort <[email protected]>

---
  drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
  1 file changed, 42 insertions(+), 14 deletions(-)

On the board n090401 (Seagate NAS 4-Bay), some of the LEDs are handled by the leds-ns2 driver. This LEDs are connected to an I2C GPIO expander (PCA95554PW) which means that GPIO access may sleep. This patch makes leds-ns2 compatible with such GPIOs by using the *_cansleep() variant of the GPIO functions. As a drawback this functions can't be used safely in a timer context (with the timer LED trigger for example). To fix this issue, a workqueue mechanism (copied from the leds-gpio driver) is used.

Note that this patch also updates slightly the ns2_led_sata_store function. The LED state is now retrieved from cached values instead of reading the GPIOs previously. This prevents ns2_led_sata_store from working with a stale LED state (which may happen when a delayed work is pending).

Signed-off-by Simon Guinot <[email protected]>

Signed-off-by Vincent Donnefort <[email protected]>

---
  drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
  1 file changed, 42 insertions(+), 14 deletions(-)

drivers/leds/leds-ns2.c +4 -0

+static void ns2_led_work(struct work_struct *work)
+{
+	struct ns2_led_data *led_dat =
+		container_of(work, struct ns2_led_data, work);

Quoted 32 lines · Simon Guinot

On the board n090401 (Seagate NAS 4-Bay), some of the LEDs are handled by the leds-ns2 driver. This LEDs are connected to an I2C GPIO expander (PCA95554PW) which means that GPIO access may sleep. This patch makes leds-ns2 compatible with such GPIOs by using the *_cansleep() variant of the GPIO functions. As a drawback this functions can't be used safely in a timer context (with the timer LED trigger for example). To fix this issue, a workqueue mechanism (copied from the leds-gpio driver) is used.

Note that this patch also updates slightly the ns2_led_sata_store function. The LED state is now retrieved from cached values instead of reading the GPIOs previously. This prevents ns2_led_sata_store from working with a stale LED state (which may happen when a delayed work is pending).

Signed-off-by Simon Guinot <[email protected]>

Signed-off-by Vincent Donnefort <[email protected]>

---
 drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 14 deletions(-)

drivers/leds/leds-ns2.c +10 -0

+static void ns2_led_work(struct work_struct *work)
+{
+	struct ns2_led_data *led_dat =
+		container_of(work, struct ns2_led_data, work);
+	int i = led_dat->new_mode_index;
+
+	write_lock(&led_dat->rw_lock);
+
+	gpio_set_value_cansleep(led_dat->cmd, led_dat->modval[i].cmd_level);
+	gpio_set_value_cansleep(led_dat->slow, led_dat->modval[i].slow_level);

drivers/leds/leds-ns2.c struct ns2_led_data { · line 69

Quoted 4 lines

drivers/leds/leds-ns2.c +4 -0

+
+	write_unlock(&led_dat->rw_lock);
+}
+

I've just realized that this can break one of the basic rules:
no sleeping should occur while holding a spinlock. Did you consider this?

Quoted 33 lines · Jacek Anaszewski

Quoted 32 lines · Simon Guinot

On the board n090401 (Seagate NAS 4-Bay), some of the LEDs are handled by the leds-ns2 driver. This LEDs are connected to an I2C GPIO expander (PCA95554PW) which means that GPIO access may sleep. This patch makes leds-ns2 compatible with such GPIOs by using the *_cansleep() variant of the GPIO functions. As a drawback this functions can't be used safely in a timer context (with the timer LED trigger for example). To fix this issue, a workqueue mechanism (copied from the leds-gpio driver) is used.

Note that this patch also updates slightly the ns2_led_sata_store function. The LED state is now retrieved from cached values instead of reading the GPIOs previously. This prevents ns2_led_sata_store from working with a stale LED state (which may happen when a delayed work is pending).

Signed-off-by Simon Guinot <[email protected]>

Signed-off-by Vincent Donnefort <[email protected]>

---
  drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
  1 file changed, 42 insertions(+), 14 deletions(-)

drivers/leds/leds-ns2.c +10 -0

+static void ns2_led_work(struct work_struct *work)
+{
+	struct ns2_led_data *led_dat =
+		container_of(work, struct ns2_led_data, work);
+	int i = led_dat->new_mode_index;
+
+	write_lock(&led_dat->rw_lock);
+
+	gpio_set_value_cansleep(led_dat->cmd, led_dat->modval[i].cmd_level);
+	gpio_set_value_cansleep(led_dat->slow, led_dat->modval[i].slow_level);

Quoted 8 lines

drivers/leds/leds-ns2.c struct ns2_led_data { · line 69

Quoted 4 lines

drivers/leds/leds-ns2.c +4 -0

+
+	write_unlock(&led_dat->rw_lock);
+}
+

I've just realized that this can break one of the basic rules:
no sleeping should occur while holding a spinlock. Did you consider this?

Well, if I did, I can't say I have done a good job here :/

You have to know that this code is used on a large number of boards. Thus, I have to thank you for spotting this bug.As a relief, this don't actually lead to a bug with the configuration we are using: UP machine and !CONFIG_SMP.

It should be simple to fix it because using a spinlock in ns2_led_work() is not needed. The GPIO writing calls are protected by the workqueue
itself: a single instance is running at a time. We are only let with the new_mode_index reading which must be made coherent.

Note that the very same issue also applies to ns2_led_get_mode(). And again using a lock here is not needed either. This function is only called once at probe time and there is no possible concurrency.

I'll fix all this issues with the v2.

Thanks.

Simon

Quoted 55 lines · Simon Guinot

Quoted 33 lines · Jacek Anaszewski

Quoted 32 lines · Simon Guinot

On the board n090401 (Seagate NAS 4-Bay), some of the LEDs are handled by the leds-ns2 driver. This LEDs are connected to an I2C GPIO expander (PCA95554PW) which means that GPIO access may sleep. This patch makes leds-ns2 compatible with such GPIOs by using the *_cansleep() variant of the GPIO functions. As a drawback this functions can't be used safely in a timer context (with the timer LED trigger for example). To fix this issue, a workqueue mechanism (copied from the leds-gpio driver) is used.

Note that this patch also updates slightly the ns2_led_sata_store function. The LED state is now retrieved from cached values instead of reading the GPIOs previously. This prevents ns2_led_sata_store from working with a stale LED state (which may happen when a delayed work is pending).

Signed-off-by Simon Guinot <[email protected]>

Signed-off-by Vincent Donnefort <[email protected]>

---
 drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 14 deletions(-)

drivers/leds/leds-ns2.c +10 -0

+static void ns2_led_work(struct work_struct *work)
+{
+	struct ns2_led_data *led_dat =
+		container_of(work, struct ns2_led_data, work);
+	int i = led_dat->new_mode_index;
+
+	write_lock(&led_dat->rw_lock);
+
+	gpio_set_value_cansleep(led_dat->cmd, led_dat->modval[i].cmd_level);
+	gpio_set_value_cansleep(led_dat->slow, led_dat->modval[i].slow_level);

Quoted 8 lines

drivers/leds/leds-ns2.c struct ns2_led_data { · line 69

Quoted 4 lines

drivers/leds/leds-ns2.c +4 -0

+
+	write_unlock(&led_dat->rw_lock);
+}
+

I've just realized that this can break one of the basic rules:
no sleeping should occur while holding a spinlock. Did you consider this?

Well, if I did, I can't say I have done a good job here :/

You have to know that this code is used on a large number of boards. Thus, I have to thank you for spotting this bug.As a relief, this don't actually lead to a bug with the configuration we are using: UP machine and !CONFIG_SMP.

It should be simple to fix it because using a spinlock in ns2_led_work() is not needed. The GPIO writing calls are protected by the workqueue
itself: a single instance is running at a time. We are only let with the new_mode_index reading which must be made coherent.

Quoted 3 lines

Note that the very same issue also applies to ns2_led_get_mode(). And again using a lock here is not needed either. This function is only called once at probe time and there is no possible concurrency.

Switching to gpio_get_value_cansleep would be nice there too.

The leds-ns2 driver is also used by the n090401 board (Seagate NAS 4-Bay), which is based on the Marvell Armada-370 SoC.

Then this patch allows to select the leds-ns2 driver if MACH_ARMADA_370 is enabled. Additionally, this also updates the Kconfig help message.

Signed-off-by Simon Guinot <[email protected]>

---
  drivers/leds/Kconfig | 12 ++++++++----
  1 file changed, 8 insertions(+), 4 deletions(-)

drivers/leds/Kconfig +1 -4

index 966b9605f5f0..68fd6b2b12be 100644
@@ -431,12 +431,16 @@ config LEDS_MC13783
  config LEDS_NS2
  	tristate "LED support for Network Space v2 GPIO LEDs"
  	depends on LEDS_CLASS
-	depends on MACH_KIRKWOOD
+	depends on MACH_KIRKWOOD || MACH_ARMADA_370
  	default y
  	help
-	  This option enable support for the dual-GPIO LED found on the
-	  Network Space v2 board (and parents). This include Internet Space v2,
-	  Network Space (Max) v2 and d2 Network v2 boards.