[RESEND PATCH 0/4] Allow to use leds-ns2 with n090401 boards

[RESEND PATCH 0/4] Allow to use leds-ns2 with n090401 boards

[Simon Guinot]

Hello,

This patch series allows to use the leds-ns2 driver with the n090401
boards (Seagate NAS 4-Bay), based on an Armada-370 SoC.

Here is the list changes:

- Allow to configure the LED mode mapping.
- Handle can_sleep GPIOs. On n090401, the LEDs are connected to an I2C
  GPIO expander.
- Make leds-ns2 depends on MACH_ARMADA_370.

Simon

Simon Guinot (2):
  leds: leds-ns2: handle can_sleep GPIOs
  leds: leds-ns2: depends on MACH_ARMADA_370

Vincent Donnefort (2):
  leds: leds-ns2: move LED modes mapping outside of the driver
  ARM: Kirkwood: add modes-map property to ns2-leds nodes

 .../devicetree/bindings/leds/leds-ns2.txt          |   9 ++
 arch/arm/boot/dts/kirkwood-d2net.dts               |   5 +
 arch/arm/boot/dts/kirkwood-is2.dts                 |   5 +
 arch/arm/boot/dts/kirkwood-ns2.dts                 |   5 +
 arch/arm/boot/dts/kirkwood-ns2max.dts              |   5 +
 arch/arm/boot/dts/kirkwood-ns2mini.dts             |   5 +
 drivers/leds/Kconfig                               |  12 +-
 drivers/leds/leds-ns2.c                            | 156 +++++++++++++--------
 include/dt-bindings/leds/leds-ns2.h                |   8 ++
 include/linux/platform_data/leds-kirkwood-ns2.h    |  14 ++
 10 files changed, 159 insertions(+), 65 deletions(-)
 create mode 100644 include/dt-bindings/leds/leds-ns2.h

-- 
2.1.4

[RESEND PATCH 1/4] leds: leds-ns2: move LED modes mapping outside of the driver

[Simon Guinot]

From: Vincent Donnefort <vdonnefort@gmail.com>

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 <vdonnefort@gmail.com>
---
 .../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

diff --git a/Documentation/devicetree/bindings/leds/leds-ns2.txt b/Documentation/devicetree/bindings/leds/leds-ns2.txt
index aef3aca34d2d..9f81258a5b6e 100644
--- a/Documentation/devicetree/bindings/leds/leds-ns2.txt
+++ b/Documentation/devicetree/bindings/leds/leds-ns2.txt
@@ -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";
 
@@ -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>;
 	};
 };
diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
index 1fd6adbb43b7..b0bc03539dbb 100644
--- a/drivers/leds/leds-ns2.c
+++ b/drivers/leds/leds-ns2.c
@@ -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;
 		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)
-			leds[i].default_trigger = string;
+			led->default_trigger = string;
+
+		ret = of_property_count_u32_elems(child, "modes-map");
+		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;
diff --git a/include/dt-bindings/leds/leds-ns2.h b/include/dt-bindings/leds/leds-ns2.h
new file mode 100644
index 000000000000..491c5f974a92
--- /dev/null
+++ b/include/dt-bindings/leds/leds-ns2.h
@@ -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
diff --git a/include/linux/platform_data/leds-kirkwood-ns2.h b/include/linux/platform_data/leds-kirkwood-ns2.h
index 6a9fed57f346..eb8a6860e816 100644
--- a/include/linux/platform_data/leds-kirkwood-ns2.h
+++ b/include/linux/platform_data/leds-kirkwood-ns2.h
@@ -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 {
-- 
2.1.4

[RESEND PATCH 1/4] leds: leds-ns2: move LED modes mapping outside of the driver

[Jacek Anaszewski]

Hi Simon,

On 06/18/2015 05:17 PM, Simon Guinot wrote:
> From: Vincent Donnefort <vdonnefort@gmail.com>
>
> 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 <vdonnefort@gmail.com>
> ---
>   .../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
>
> diff --git a/Documentation/devicetree/bindings/leds/leds-ns2.txt b/Documentation/devicetree/bindings/leds/leds-ns2.txt
> index aef3aca34d2d..9f81258a5b6e 100644
> --- a/Documentation/devicetree/bindings/leds/leds-ns2.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-ns2.txt
> @@ -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";
>
> @@ -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.

> diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
> index 1fd6adbb43b7..b0bc03539dbb 100644
> --- a/drivers/leds/leds-ns2.c
> +++ b/drivers/leds/leds-ns2.c
> @@ -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;
>   		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)
> -			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.

> +		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;
> diff --git a/include/dt-bindings/leds/leds-ns2.h b/include/dt-bindings/leds/leds-ns2.h
> new file mode 100644
> index 000000000000..491c5f974a92
> --- /dev/null
> +++ b/include/dt-bindings/leds/leds-ns2.h
> @@ -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
> diff --git a/include/linux/platform_data/leds-kirkwood-ns2.h b/include/linux/platform_data/leds-kirkwood-ns2.h
> index 6a9fed57f346..eb8a6860e816 100644
> --- a/include/linux/platform_data/leds-kirkwood-ns2.h
> +++ b/include/linux/platform_data/leds-kirkwood-ns2.h
> @@ -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 {
>


-- 
Best Regards,
Jacek Anaszewski

[RESEND PATCH 1/4] leds: leds-ns2: move LED modes mapping outside of the driver

[Simon Guinot]

On Mon, Jun 22, 2015 at 04:32:37PM +0200, Jacek Anaszewski wrote:
> Hi Simon,

Hi Jacek,

Thanks for taking care of this patches.

> 
> On 06/18/2015 05:17 PM, Simon Guinot wrote:
> >From: Vincent Donnefort <vdonnefort@gmail.com>
> >
> >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 <vdonnefort@gmail.com>
> >---
> >  .../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
> >
> >diff --git a/Documentation/devicetree/bindings/leds/leds-ns2.txt b/Documentation/devicetree/bindings/leds/leds-ns2.txt
> >index aef3aca34d2d..9f81258a5b6e 100644
> >--- a/Documentation/devicetree/bindings/leds/leds-ns2.txt
> >+++ b/Documentation/devicetree/bindings/leds/leds-ns2.txt
> >@@ -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";
> >
> >@@ -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.

OK.

> 
> >diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
> >index 1fd6adbb43b7..b0bc03539dbb 100644
> >--- a/drivers/leds/leds-ns2.c
> >+++ b/drivers/leds/leds-ns2.c
> >@@ -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 },
> >-};
> >-

...

> >+	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)
> >-			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.

Thanks,

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150623/ba8ccd4d/attachment.sig>

[RESEND PATCH 1/4] leds: leds-ns2: move LED modes mapping outside of the driver

[Jacek Anaszewski]

Hi Simon,

On 06/23/2015 08:12 PM, Simon Guinot wrote:
[...]
>
>>> +	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)
>>> -			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.

Since you modify also dts file in this patch set this is OK.
You can keep this part as is.

-- 
Best Regards,
Jacek Anaszewski

[RESEND PATCH 2/4] ARM: Kirkwood: add modes-map property to ns2-leds nodes

[Simon Guinot]

From: Vincent Donnefort <vdonnefort@gmail.com>

Since the LED modes mapping is no longer hardcoded inside the leds-ns2
driver, then it must be provided through the modes-map property in the
ns2-leds nodes.

Signed-off-by: Vincent Donnefort <vdonnefort@gmail.com>
---
 arch/arm/boot/dts/kirkwood-d2net.dts   | 5 +++++
 arch/arm/boot/dts/kirkwood-is2.dts     | 5 +++++
 arch/arm/boot/dts/kirkwood-ns2.dts     | 5 +++++
 arch/arm/boot/dts/kirkwood-ns2max.dts  | 5 +++++
 arch/arm/boot/dts/kirkwood-ns2mini.dts | 5 +++++
 5 files changed, 25 insertions(+)

diff --git a/arch/arm/boot/dts/kirkwood-d2net.dts b/arch/arm/boot/dts/kirkwood-d2net.dts
index 6b7856025001..e1c25c35e9ce 100644
--- a/arch/arm/boot/dts/kirkwood-d2net.dts
+++ b/arch/arm/boot/dts/kirkwood-d2net.dts
@@ -10,6 +10,7 @@
 
 /dts-v1/;
 
+#include <dt-bindings/leds/leds-ns2.h>
 #include "kirkwood-netxbig.dtsi"
 
 / {
@@ -28,6 +29,10 @@
 			label = "d2net_v2:blue:sata";
 			slow-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
 			cmd-gpio = <&gpio0 30 GPIO_ACTIVE_HIGH>;
+			modes-map = <NS_V2_LED_OFF  1 0
+				     NS_V2_LED_ON   0 1
+				     NS_V2_LED_ON   1 1
+				     NS_V2_LED_SATA 0 0>;
 		};
 	};
 
diff --git a/arch/arm/boot/dts/kirkwood-is2.dts b/arch/arm/boot/dts/kirkwood-is2.dts
index da674bbd49a8..4121674abd1c 100644
--- a/arch/arm/boot/dts/kirkwood-is2.dts
+++ b/arch/arm/boot/dts/kirkwood-is2.dts
@@ -1,5 +1,6 @@
 /dts-v1/;
 
+#include <dt-bindings/leds/leds-ns2.h>
 #include "kirkwood-ns2-common.dtsi"
 
 / {
@@ -27,6 +28,10 @@
 			label = "ns2:blue:sata";
 			slow-gpio = <&gpio0 29 0>;
 			cmd-gpio = <&gpio0 30 0>;
+			modes-map = <NS_V2_LED_OFF  1 0
+				     NS_V2_LED_ON   0 1
+				     NS_V2_LED_ON   1 1
+				     NS_V2_LED_SATA 0 0>;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/kirkwood-ns2.dts b/arch/arm/boot/dts/kirkwood-ns2.dts
index 53368d1022cc..190189d235e6 100644
--- a/arch/arm/boot/dts/kirkwood-ns2.dts
+++ b/arch/arm/boot/dts/kirkwood-ns2.dts
@@ -1,5 +1,6 @@
 /dts-v1/;
 
+#include <dt-bindings/leds/leds-ns2.h>
 #include "kirkwood-ns2-common.dtsi"
 
 / {
@@ -27,6 +28,10 @@
 			label = "ns2:blue:sata";
 			slow-gpio = <&gpio0 29 0>;
 			cmd-gpio = <&gpio0 30 0>;
+			modes-map = <NS_V2_LED_OFF  1 0
+				     NS_V2_LED_ON   0 1
+				     NS_V2_LED_ON   1 1
+				     NS_V2_LED_SATA 0 0>;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/kirkwood-ns2max.dts b/arch/arm/boot/dts/kirkwood-ns2max.dts
index 72c78d0b1116..55cc41d9c80c 100644
--- a/arch/arm/boot/dts/kirkwood-ns2max.dts
+++ b/arch/arm/boot/dts/kirkwood-ns2max.dts
@@ -1,5 +1,6 @@
 /dts-v1/;
 
+#include <dt-bindings/leds/leds-ns2.h>
 #include "kirkwood-ns2-common.dtsi"
 
 / {
@@ -46,6 +47,10 @@
 			label = "ns2:blue:sata";
 			slow-gpio = <&gpio0 29 0>;
 			cmd-gpio = <&gpio0 30 0>;
+			modes-map = <NS_V2_LED_OFF  1 0
+				     NS_V2_LED_ON   0 1
+				     NS_V2_LED_ON   1 1
+				     NS_V2_LED_SATA 0 0>;
 		};
 	};
 };
diff --git a/arch/arm/boot/dts/kirkwood-ns2mini.dts b/arch/arm/boot/dts/kirkwood-ns2mini.dts
index c441bf62c09f..9935f3ec29b4 100644
--- a/arch/arm/boot/dts/kirkwood-ns2mini.dts
+++ b/arch/arm/boot/dts/kirkwood-ns2mini.dts
@@ -1,5 +1,6 @@
 /dts-v1/;
 
+#include <dt-bindings/leds/leds-ns2.h>
 #include "kirkwood-ns2-common.dtsi"
 
 / {
@@ -47,6 +48,10 @@
 			label = "ns2:blue:sata";
 			slow-gpio = <&gpio0 29 0>;
 			cmd-gpio = <&gpio0 30 0>;
+			modes-map = <NS_V2_LED_OFF  1 0
+				     NS_V2_LED_ON   0 1
+				     NS_V2_LED_ON   1 1
+				     NS_V2_LED_SATA 0 0>;
 		};
 	};
 };
-- 
2.1.4

[RESEND PATCH 3/4] leds: leds-ns2: handle can_sleep GPIOs

[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 <simon.guinot@sequanux.org>
Signed-off-by: Vincent Donnefort <vdonnefort@gmail.com>
---
 drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 14 deletions(-)

diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
index b0bc03539dbb..ea1542db9ba4 100644
--- a/drivers/leds/leds-ns2.c
+++ b/drivers/leds/leds-ns2.c
@@ -31,6 +31,7 @@
 #include <linux/platform_data/leds-kirkwood-ns2.h>
 #include <linux/of.h>
 #include <linux/of_gpio.h>
+#include "leds.h"
 
 /*
  * The Network Space v2 dual-GPIO LED is wired to a CPLD. Three different LED
@@ -43,12 +44,29 @@ struct ns2_led_data {
 	struct led_classdev	cdev;
 	unsigned		cmd;
 	unsigned		slow;
+	bool			can_sleep;
+	int			new_mode_index;
 	unsigned char		sata; /* True when SATA mode active. */
 	rwlock_t		rw_lock; /* Lock GPIOs. */
+	struct work_struct	work;
 	int			num_modes;
 	struct ns2_led_modval	*modval;
 };
 
+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);
+
+	write_unlock(&led_dat->rw_lock);
+}
+
 static int ns2_led_get_mode(struct ns2_led_data *led_dat,
 			    enum ns2_led_modes *mode)
 {
@@ -59,8 +77,8 @@ static int ns2_led_get_mode(struct ns2_led_data *led_dat,
 
 	read_lock_irq(&led_dat->rw_lock);
 
-	cmd_level = gpio_get_value(led_dat->cmd);
-	slow_level = gpio_get_value(led_dat->slow);
+	cmd_level = gpio_get_value_cansleep(led_dat->cmd);
+	slow_level = gpio_get_value_cansleep(led_dat->slow);
 
 	for (i = 0; i < led_dat->num_modes; i++) {
 		if (cmd_level == led_dat->modval[i].cmd_level &&
@@ -85,7 +103,13 @@ static void ns2_led_set_mode(struct ns2_led_data *led_dat,
 	write_lock_irqsave(&led_dat->rw_lock, flags);
 
 	for (i = 0; i < led_dat->num_modes; i++) {
-		if (mode == led_dat->modval[i].mode) {
+		if (mode != led_dat->modval[i].mode)
+			continue;
+
+		if (led_dat->can_sleep) {
+			led_dat->new_mode_index = i;
+			schedule_work(&led_dat->work);
+		} else {
 			gpio_set_value(led_dat->cmd,
 				       led_dat->modval[i].cmd_level);
 			gpio_set_value(led_dat->slow,
@@ -122,7 +146,6 @@ static ssize_t ns2_led_sata_store(struct device *dev,
 		container_of(led_cdev, struct ns2_led_data, cdev);
 	int ret;
 	unsigned long enable;
-	enum ns2_led_modes mode;
 
 	ret = kstrtoul(buff, 10, &enable);
 	if (ret < 0)
@@ -131,19 +154,19 @@ static ssize_t ns2_led_sata_store(struct device *dev,
 	enable = !!enable;
 
 	if (led_dat->sata == enable)
-		return count;
+		goto exit;
 
-	ret = ns2_led_get_mode(led_dat, &mode);
-	if (ret < 0)
-		return ret;
+	led_dat->sata = enable;
+
+	if (!led_get_brightness(led_cdev))
+		goto exit;
 
-	if (enable && mode == NS_V2_LED_ON)
+	if (enable)
 		ns2_led_set_mode(led_dat, NS_V2_LED_SATA);
-	if (!enable && mode == NS_V2_LED_SATA)
+	else
 		ns2_led_set_mode(led_dat, NS_V2_LED_ON);
 
-	led_dat->sata = enable;
-
+exit:
 	return count;
 }
 
@@ -173,7 +196,7 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
 	enum ns2_led_modes mode;
 
 	ret = devm_gpio_request_one(&pdev->dev, template->cmd,
-			gpio_get_value(template->cmd) ?
+			gpio_get_value_cansleep(template->cmd) ?
 			GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
 			template->name);
 	if (ret) {
@@ -183,7 +206,7 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
 	}
 
 	ret = devm_gpio_request_one(&pdev->dev, template->slow,
-			gpio_get_value(template->slow) ?
+			gpio_get_value_cansleep(template->slow) ?
 			GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
 			template->name);
 	if (ret) {
@@ -202,6 +225,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->can_sleep = gpio_cansleep(led_dat->cmd) |
+				gpio_cansleep(led_dat->slow);
 	led_dat->modval = template->modval;
 	led_dat->num_modes = template->num_modes;
 
@@ -214,6 +239,8 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
 	led_dat->cdev.brightness =
 		(mode == NS_V2_LED_OFF) ? LED_OFF : LED_FULL;
 
+	INIT_WORK(&led_dat->work, ns2_led_work);
+
 	ret = led_classdev_register(&pdev->dev, &led_dat->cdev);
 	if (ret < 0)
 		return ret;
@@ -224,6 +251,7 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
 static void delete_ns2_led(struct ns2_led_data *led_dat)
 {
 	led_classdev_unregister(&led_dat->cdev);
+	cancel_work_sync(&led_dat->work);
 }
 
 #ifdef CONFIG_OF_GPIO
-- 
2.1.4

[RESEND PATCH 3/4] leds: leds-ns2: handle can_sleep GPIOs

[Jacek Anaszewski]

Hi Simon,

On 06/18/2015 05:17 PM, Simon Guinot wrote:
> 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 <simon.guinot@sequanux.org>
> Signed-off-by: Vincent Donnefort <vdonnefort@gmail.com>
> ---
>   drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 42 insertions(+), 14 deletions(-)

Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>

-- 
Best Regards,
Jacek Anaszewski

[RESEND PATCH 3/4] leds: leds-ns2: handle can_sleep GPIOs

[Jacek Anaszewski]

On 06/18/2015 05:17 PM, Simon Guinot wrote:
> 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 <simon.guinot@sequanux.org>
> Signed-off-by: Vincent Donnefort <vdonnefort@gmail.com>
> ---
>   drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
>   1 file changed, 42 insertions(+), 14 deletions(-)
>

>
> +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);
> +
> +	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?

-- 
Best Regards,
Jacek Anaszewski

[RESEND PATCH 3/4] leds: leds-ns2: handle can_sleep GPIOs

[Simon Guinot]

On Wed, Jun 24, 2015 at 04:18:29PM +0200, Jacek Anaszewski wrote:
> On 06/18/2015 05:17 PM, Simon Guinot wrote:
> >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 <simon.guinot@sequanux.org>
> >Signed-off-by: Vincent Donnefort <vdonnefort@gmail.com>
> >---
> >  drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 42 insertions(+), 14 deletions(-)
> >
> 
> >
> >+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);
> >+
> >+	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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150626/40279a2e/attachment.sig>

[RESEND PATCH 3/4] leds: leds-ns2: handle can_sleep GPIOs

[Jacek Anaszewski]

On 06/26/2015 07:10 PM, Simon Guinot wrote:
> On Wed, Jun 24, 2015 at 04:18:29PM +0200, Jacek Anaszewski wrote:
>> On 06/18/2015 05:17 PM, Simon Guinot wrote:
>>> 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 <simon.guinot@sequanux.org>
>>> Signed-off-by: Vincent Donnefort <vdonnefort@gmail.com>
>>> ---
>>>   drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
>>>   1 file changed, 42 insertions(+), 14 deletions(-)
>>>
>>
>>>
>>> +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);
>>> +
>>> +	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.

Switching to gpio_get_value_cansleep would be nice there too.

>
> I'll fix all this issues with the v2.
>
> Thanks.
>
> Simon
>


-- 
Best Regards,
Jacek Anaszewski

[RESEND PATCH 3/4] leds: leds-ns2: handle can_sleep GPIOs

[Simon Guinot]

On Mon, Jun 29, 2015 at 04:25:13PM +0200, Jacek Anaszewski wrote:
> On 06/26/2015 07:10 PM, Simon Guinot wrote:
> >On Wed, Jun 24, 2015 at 04:18:29PM +0200, Jacek Anaszewski wrote:
> >>On 06/18/2015 05:17 PM, Simon Guinot wrote:
> >>>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 <simon.guinot@sequanux.org>
> >>>Signed-off-by: Vincent Donnefort <vdonnefort@gmail.com>
> >>>---
> >>>  drivers/leds/leds-ns2.c | 56 ++++++++++++++++++++++++++++++++++++-------------
> >>>  1 file changed, 42 insertions(+), 14 deletions(-)
> >>>
> >>
> >>>
> >>>+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);
> >>>+
> >>>+	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.
> 
> Switching to gpio_get_value_cansleep would be nice there too.

It is already the case.

Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150629/d1395546/attachment.sig>

[RESEND PATCH 4/4] leds: leds-ns2: depends on MACH_ARMADA_370

[Simon Guinot]

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 <simon.guinot@sequanux.org>
---
 drivers/leds/Kconfig | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 966b9605f5f0..68fd6b2b12be 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -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.
+	  This option enables support for the dual-GPIO LEDs found on the
+	  following LaCie/Seagate boards:
+
+		Network Space v2 (and parents: Max, Mini)
+		Internet Space v2
+		d2 Network v2
+		n090401 (Seagate NAS 4-Bay)
 
 config LEDS_NETXBIG
 	tristate "LED support for Big Network series LEDs"
-- 
2.1.4

[RESEND PATCH 4/4] leds: leds-ns2: depends on MACH_ARMADA_370

[Jacek Anaszewski]

Hi Simon,

On 06/18/2015 05:17 PM, Simon Guinot wrote:
> 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 <simon.guinot@sequanux.org>
> ---
>   drivers/leds/Kconfig | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 966b9605f5f0..68fd6b2b12be 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -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.
> +	  This option enables support for the dual-GPIO LEDs found on the
> +	  following LaCie/Seagate boards:
> +
> +		Network Space v2 (and parents: Max, Mini)
> +		Internet Space v2
> +		d2 Network v2
> +		n090401 (Seagate NAS 4-Bay)
>
>   config LEDS_NETXBIG
>   	tristate "LED support for Big Network series LEDs"
>

Acked-by: Jacek Anaszewski <j.anaszewski@samsung.com>

-- 
Best Regards,
Jacek Anaszewski