[PATCH v4 0/3] Add DT support for netxbig LEDs

[PATCH v4 0/3] Add DT support for netxbig LEDs

[Simon Guinot]

Hello,

This patch series adds DT support for the LEDs found on the Kirkwood-based
LaCie boards 2Big and 5Big Network v2.

Changes for v2:
- Check timer mode value retrieved from DT.
- In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
  timer delay values from DT with function of_property_read_u32_index.
  Instead, use a temporary u32 variable. This allows to silence a static
  checker warning.
- Make timer property optional in the binding documentation. It is now
  aligned with the driver code.

Changes for v3:
- Fix pointer usage with the temporary u32 variable while calling
  of_property_read_u32_index.

Changes for v4:
- In DT binding document netxbig-gpio-ext.txt, detail byte order for
  registers and latch mechanism for "enable-gpio".
- In leds-netxbig.c, add some error messages.
- In leds-netxbig.c, fix some "sizeof" style issues.
- In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
  of_property_read_string() calls after the error-prone checks.
- Add some Acked-by.

Jacek,

I did not convert the bright-max DT property into led-max-microamp.
The reason is that I can't translate the bright-max values into
microamperes. Therefore, it looks more consistent to keep the same
naming prefix for the bright-addr and bright-max DT properties.

Thanks,

Simon

Simon Guinot (3):
  leds: netxbig: add device tree binding
  ARM: Kirkwood: add LED DT entries for netxbig boards
  ARM: mvebu: remove static LED setup for netxbig boards

 .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
 .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++++
 arch/arm/boot/dts/kirkwood-net5big.dts             |  60 +++++
 arch/arm/boot/dts/kirkwood-netxbig.dtsi            |  80 +++++++
 arch/arm/mach-mvebu/Kconfig                        |   7 -
 arch/arm/mach-mvebu/Makefile                       |   1 -
 arch/arm/mach-mvebu/board.h                        |  21 --
 arch/arm/mach-mvebu/kirkwood.c                     |   4 -
 arch/arm/mach-mvebu/netxbig.c                      | 191 ---------------
 drivers/leds/leds-netxbig.c                        | 258 +++++++++++++++++++--
 include/dt-bindings/leds/leds-netxbig.h            |  18 ++
 11 files changed, 509 insertions(+), 245 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
 create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt
 delete mode 100644 arch/arm/mach-mvebu/board.h
 delete mode 100644 arch/arm/mach-mvebu/netxbig.c
 create mode 100644 include/dt-bindings/leds/leds-netxbig.h

-- 
2.1.4

[PATCH v4 1/3] leds: netxbig: add device tree binding

[Simon Guinot]

This patch adds device tree support for the netxbig LEDs.

This also introduces a additionnal DT binding for the GPIO extension bus
(netxbig-gpio-ext) used to configure the LEDs. Since this bus could also
be used to control other devices, then it seems more suitable to have it
in a separate DT binding.

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
 .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
 .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++++
 drivers/leds/leds-netxbig.c                        | 258 +++++++++++++++++++--
 include/dt-bindings/leds/leds-netxbig.h            |  18 ++
 4 files changed, 369 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
 create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt
 create mode 100644 include/dt-bindings/leds/leds-netxbig.h

Changes for v2:
- Check timer mode value retrieved from DT.
- In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
  timer delay values from DT with function of_property_read_u32_index.
  Instead, use a temporary u32 variable. This allows to silence a static
  checker warning.
- Make timer property optional in the binding documentation. It is now
  aligned with the driver code.

Changes for v3:
- Fix pointer usage with the temporary u32 variable while calling
  of_property_read_u32_index.

Changes for v4:
- In DT binding document netxbig-gpio-ext.txt, detail byte order for
  registers and latch mechanism for "enable-gpio".
- In leds-netxbig.c, add some error messages.
- In leds-netxbig.c, fix some "sizeof" style issues.
- In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
  of_property_read_string() calls after the error-prone checks.
- Add some Acked-by.

diff --git a/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
new file mode 100644
index 000000000000..50ec2e690701
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
@@ -0,0 +1,22 @@
+Binding for the GPIO extension bus found on some LaCie/Seagate boards
+(Example: 2Big/5Big Network v2, 2Big NAS).
+
+Required properties:
+- compatible: "lacie,netxbig-gpio-ext".
+- addr-gpios: GPIOs representing the address register (LSB -> MSB).
+- data-gpios: GPIOs representing the data register (LSB -> MSB).
+- enable-gpio: latches the new configuration (address, data) on raising edge.
+
+Example:
+
+netxbig_gpio_ext: netxbig-gpio-ext {
+	compatible = "lacie,netxbig-gpio-ext";
+
+	addr-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH
+		      &gpio1 16 GPIO_ACTIVE_HIGH
+		      &gpio1 17 GPIO_ACTIVE_HIGH>;
+	data-gpios = <&gpio1 12 GPIO_ACTIVE_HIGH
+		      &gpio1 13 GPIO_ACTIVE_HIGH
+		      &gpio1 14 GPIO_ACTIVE_HIGH>;
+	enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
+};
diff --git a/Documentation/devicetree/bindings/leds/leds-netxbig.txt b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
new file mode 100644
index 000000000000..efadbecbfeb9
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
@@ -0,0 +1,92 @@
+Binding for the CPLD LEDs (GPIO extension bus) found on some LaCie/Seagate
+boards (Example: 2Big/5Big Network v2, 2Big NAS).
+
+Required properties:
+- compatible: "lacie,netxbig-leds".
+- gpio-ext: Phandle for the gpio-ext bus.
+
+Optional properties:
+- timers: Timer array. Each timer entry is represented by three integers:
+  Mode (gpio-ext bus), delay_on and delay_off.
+
+Each LED is represented as a sub-node of the netxbig-leds device.
+
+Required sub-node properties:
+- mode-addr: Mode register address on gpio-ext bus.
+- mode-val: Mode to value mapping. Each entry is represented by two integers:
+  A mode and the corresponding value on the gpio-ext bus.
+- bright-addr: Brightness register address on gpio-ext bus.
+- bright-max: Maximum brightness value.
+
+Optional sub-node properties:
+- label: Name for this LED. If omitted, the label is taken from the node name.
+- linux,default-trigger: Trigger assigned to the LED.
+
+Example:
+
+netxbig-leds {
+	compatible = "lacie,netxbig-leds";
+
+	gpio-ext = &gpio_ext;
+
+	timers = <NETXBIG_LED_TIMER1 500 500
+		  NETXBIG_LED_TIMER2 500 1000>;
+
+	blue-power {
+		label = "netxbig:blue:power";
+		mode-addr = <0>;
+		mode-val = <NETXBIG_LED_OFF 0
+			    NETXBIG_LED_ON 1
+			    NETXBIG_LED_TIMER1 3
+			    NETXBIG_LED_TIMER2 7>;
+		bright-addr = <1>;
+		bright-max = <7>;
+	};
+	red-power {
+		label = "netxbig:red:power";
+		mode-addr = <0>;
+		mode-val = <NETXBIG_LED_OFF 0
+			    NETXBIG_LED_ON 2
+			    NETXBIG_LED_TIMER1 4>;
+		bright-addr = <1>;
+		bright-max = <7>;
+	};
+	blue-sata0 {
+		label = "netxbig:blue:sata0";
+		mode-addr = <3>;
+		mode-val = <NETXBIG_LED_OFF 0
+			    NETXBIG_LED_ON 7
+			    NETXBIG_LED_SATA 1
+			    NETXBIG_LED_TIMER1 3>;
+		bright-addr = <2>;
+		bright-max = <7>;
+	};
+	red-sata0 {
+		label = "netxbig:red:sata0";
+		mode-addr = <3>;
+		mode-val = <NETXBIG_LED_OFF 0
+			    NETXBIG_LED_ON 2
+			    NETXBIG_LED_TIMER1 4>;
+		bright-addr = <2>;
+		bright-max = <7>;
+	};
+	blue-sata1 {
+		label = "netxbig:blue:sata1";
+		mode-addr = <4>;
+		mode-val = <NETXBIG_LED_OFF 0
+			    NETXBIG_LED_ON 7
+			    NETXBIG_LED_SATA 1
+			    NETXBIG_LED_TIMER1 3>;
+		bright-addr = <2>;
+		bright-max = <7>;
+	};
+	red-sata1 {
+		label = "netxbig:red:sata1";
+		mode-addr = <4>;
+		mode-val = <NETXBIG_LED_OFF 0
+			    NETXBIG_LED_ON 2
+			    NETXBIG_LED_TIMER1 4>;
+		bright-addr = <2>;
+		bright-max = <7>;
+	};
+};
diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
index 25e419752a7b..dcd8302cccaf 100644
--- a/drivers/leds/leds-netxbig.c
+++ b/drivers/leds/leds-netxbig.c
@@ -26,6 +26,7 @@
 #include <linux/spinlock.h>
 #include <linux/platform_device.h>
 #include <linux/gpio.h>
+#include <linux/of_gpio.h>
 #include <linux/leds.h>
 #include <linux/platform_data/leds-kirkwood-netxbig.h>
 
@@ -140,6 +141,11 @@ struct netxbig_led_data {
 	spinlock_t		lock;
 };
 
+struct netxbig_led_priv {
+	struct netxbig_led_platform_data *pdata;
+	struct netxbig_led_data leds_data[];
+};
+
 static int netxbig_led_get_timer_mode(enum netxbig_led_mode *mode,
 				      unsigned long delay_on,
 				      unsigned long delay_off,
@@ -304,12 +310,12 @@ static void delete_netxbig_led(struct netxbig_led_data *led_dat)
 	led_classdev_unregister(&led_dat->cdev);
 }
 
-static int
-create_netxbig_led(struct platform_device *pdev,
-		   struct netxbig_led_data *led_dat,
-		   const struct netxbig_led *template)
+static int create_netxbig_led(struct platform_device *pdev, int led,
+			      struct netxbig_led_priv *priv)
 {
-	struct netxbig_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct netxbig_led_platform_data *pdata = priv->pdata;
+	struct netxbig_led_data *led_dat = &priv->leds_data[led];
+	const struct netxbig_led *template = &priv->pdata->leds[led];
 
 	spin_lock_init(&led_dat->lock);
 	led_dat->gpio_ext = pdata->gpio_ext;
@@ -346,38 +352,249 @@ create_netxbig_led(struct platform_device *pdev,
 	return led_classdev_register(&pdev->dev, &led_dat->cdev);
 }
 
+#ifdef CONFIG_OF_GPIO
+static int gpio_ext_get_of_pdata(struct device *dev, struct device_node *np,
+				 struct netxbig_gpio_ext *gpio_ext)
+{
+	int *addr, *data;
+	int num_addr, num_data;
+	int ret;
+	int i;
+
+	ret = of_gpio_named_count(np, "addr-gpios");
+	if (ret < 0) {
+		dev_err(dev,
+			"Failed to count GPIOs in DT property addr-gpios\n");
+		return ret;
+	}
+	num_addr = ret;
+	addr = devm_kzalloc(dev, num_addr * sizeof(*addr), GFP_KERNEL);
+	if (!addr)
+		return -ENOMEM;
+
+	for (i = 0; i < num_addr; i++) {
+		ret = of_get_named_gpio(np, "addr-gpios", i);
+		if (ret < 0)
+			return ret;
+		addr[i] = ret;
+	}
+	gpio_ext->addr = addr;
+	gpio_ext->num_addr = num_addr;
+
+	ret = of_gpio_named_count(np, "data-gpios");
+	if (ret < 0) {
+		dev_err(dev,
+			"Failed to count GPIOs in DT property data-gpios\n");
+		return ret;
+	}
+	num_data = ret;
+	data = devm_kzalloc(dev, num_data * sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	for (i = 0; i < num_data; i++) {
+		ret = of_get_named_gpio(np, "data-gpios", i);
+		if (ret < 0)
+			return ret;
+		data[i] = ret;
+	}
+	gpio_ext->data = data;
+	gpio_ext->num_data = num_data;
+
+	ret = of_get_named_gpio(np, "enable-gpio", 0);
+	if (ret < 0) {
+		dev_err(dev,
+			"Failed to get GPIO from DT property enable-gpio\n");
+		return ret;
+	}
+	gpio_ext->enable = ret;
+
+	return 0;
+}
+
+static int netxbig_leds_get_of_pdata(struct device *dev,
+				     struct netxbig_led_platform_data *pdata)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *gpio_ext_np;
+	struct device_node *child;
+	struct netxbig_gpio_ext *gpio_ext;
+	struct netxbig_led_timer *timers;
+	struct netxbig_led *leds, *led;
+	int num_timers;
+	int num_leds = 0;
+	int ret;
+	int i;
+
+	/* GPIO extension */
+	gpio_ext_np = of_parse_phandle(np, "gpio-ext", 0);
+	if (!gpio_ext_np) {
+		dev_err(dev, "Failed to get DT handle gpio-ext\n");
+		return -EINVAL;
+	}
+
+	gpio_ext = devm_kzalloc(dev, sizeof(*gpio_ext), GFP_KERNEL);
+	if (!gpio_ext)
+		return -ENOMEM;
+	ret = gpio_ext_get_of_pdata(dev, gpio_ext_np, gpio_ext);
+	if (ret)
+		return ret;
+	of_node_put(gpio_ext_np);
+	pdata->gpio_ext = gpio_ext;
+
+	/* Timers (optional) */
+	ret = of_property_count_u32_elems(np, "timers");
+	if (ret > 0) {
+		if (ret % 3)
+			return -EINVAL;
+		num_timers = ret / 3;
+		timers = devm_kzalloc(dev, num_timers * sizeof(*timers),
+				      GFP_KERNEL);
+		if (!timers)
+			return -ENOMEM;
+		for (i = 0; i < num_timers; i++) {
+			u32 tmp;
+
+			of_property_read_u32_index(np, "timers", 3 * i,
+						   &timers[i].mode);
+			if (timers[i].mode >= NETXBIG_LED_MODE_NUM)
+				return -EINVAL;
+			of_property_read_u32_index(np, "timers",
+						   3 * i + 1, &tmp);
+			timers[i].delay_on = tmp;
+			of_property_read_u32_index(np, "timers",
+						   3 * i + 2, &tmp);
+			timers[i].delay_off = tmp;
+		}
+		pdata->timer = timers;
+		pdata->num_timer = num_timers;
+	}
+
+	/* LEDs */
+	num_leds = of_get_child_count(np);
+	if (!num_leds) {
+		dev_err(dev, "No LED subnodes found in DT\n");
+		return -ENODEV;
+	}
+
+	leds = devm_kzalloc(dev, num_leds * sizeof(*leds), GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	led = leds;
+	for_each_child_of_node(np, child) {
+		const char *string;
+		int *mode_val;
+		int num_modes;
+
+		if (of_property_read_u32(child, "mode-addr",
+					 &led->mode_addr))
+			return -EINVAL;
+
+		if (of_property_read_u32(child, "bright-addr",
+					 &led->bright_addr))
+			return -EINVAL;
+
+		mode_val =
+			devm_kzalloc(dev,
+				     NETXBIG_LED_MODE_NUM * sizeof(*mode_val),
+				     GFP_KERNEL);
+		if (!mode_val)
+			return -ENOMEM;
+
+		for (i = 0; i < NETXBIG_LED_MODE_NUM; i++)
+			mode_val[i] = NETXBIG_LED_INVALID_MODE;
+
+		ret = of_property_count_u32_elems(child, "mode-val");
+		if (ret < 0 || ret % 2)
+			return -EINVAL;
+		num_modes = ret / 2;
+		if (num_modes > NETXBIG_LED_MODE_NUM)
+			return -EINVAL;
+
+		for (i = 0; i < num_modes; i++) {
+			int mode;
+			int val;
+
+			of_property_read_u32_index(child,
+						   "mode-val", 2 * i, &mode);
+			of_property_read_u32_index(child,
+						   "mode-val", 2 * i + 1, &val);
+			if (mode >= NETXBIG_LED_MODE_NUM)
+				return -EINVAL;
+			mode_val[mode] = val;
+		}
+		led->mode_val = mode_val;
+
+		if (!of_property_read_string(child, "label", &string))
+			led->name = string;
+		else
+			led->name = child->name;
+
+		if (!of_property_read_string(child,
+					     "linux,default-trigger", &string))
+			led->default_trigger = string;
+
+		led++;
+	}
+
+	pdata->leds = leds;
+	pdata->num_leds = num_leds;
+
+	return 0;
+}
+
+static const struct of_device_id of_netxbig_leds_match[] = {
+	{ .compatible = "lacie,netxbig-leds", },
+	{},
+};
+#else
+static int netxbig_leds_get_of_pdata(struct device *dev,
+				     struct netxbig_led_platform_data *pdata)
+{
+	return -ENODEV;
+}
+#endif /* CONFIG_OF_GPIO */
+
 static int netxbig_led_probe(struct platform_device *pdev)
 {
 	struct netxbig_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
-	struct netxbig_led_data *leds_data;
+	struct netxbig_led_priv *priv;
 	int i;
 	int ret;
 
-	if (!pdata)
-		return -EINVAL;
+	if (!pdata) {
+		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			return -ENOMEM;
+		ret = netxbig_leds_get_of_pdata(&pdev->dev, pdata);
+		if (ret)
+			return ret;
+	}
 
-	leds_data = devm_kzalloc(&pdev->dev,
-		sizeof(struct netxbig_led_data) * pdata->num_leds, GFP_KERNEL);
-	if (!leds_data)
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv) +
+			    pdata->num_leds * sizeof(struct netxbig_led_data),
+			    GFP_KERNEL);
+	if (!priv)
 		return -ENOMEM;
+	priv->pdata = pdata;
 
 	ret = gpio_ext_init(pdata->gpio_ext);
 	if (ret < 0)
 		return ret;
 
 	for (i = 0; i < pdata->num_leds; i++) {
-		ret = create_netxbig_led(pdev, &leds_data[i], &pdata->leds[i]);
+		ret = create_netxbig_led(pdev, i, priv);
 		if (ret < 0)
 			goto err_free_leds;
 	}
-
-	platform_set_drvdata(pdev, leds_data);
+	platform_set_drvdata(pdev, priv);
 
 	return 0;
 
 err_free_leds:
 	for (i = i - 1; i >= 0; i--)
-		delete_netxbig_led(&leds_data[i]);
+		delete_netxbig_led(&priv->leds_data[i]);
 
 	gpio_ext_free(pdata->gpio_ext);
 	return ret;
@@ -385,14 +602,12 @@ err_free_leds:
 
 static int netxbig_led_remove(struct platform_device *pdev)
 {
-	struct netxbig_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
-	struct netxbig_led_data *leds_data;
+	struct netxbig_led_priv *priv = platform_get_drvdata(pdev);
+	struct netxbig_led_platform_data *pdata = priv->pdata;
 	int i;
 
-	leds_data = platform_get_drvdata(pdev);
-
 	for (i = 0; i < pdata->num_leds; i++)
-		delete_netxbig_led(&leds_data[i]);
+		delete_netxbig_led(&priv->leds_data[i]);
 
 	gpio_ext_free(pdata->gpio_ext);
 
@@ -403,7 +618,8 @@ static struct platform_driver netxbig_led_driver = {
 	.probe		= netxbig_led_probe,
 	.remove		= netxbig_led_remove,
 	.driver		= {
-		.name	= "leds-netxbig",
+		.name		= "leds-netxbig",
+		.of_match_table	= of_match_ptr(of_netxbig_leds_match),
 	},
 };
 
diff --git a/include/dt-bindings/leds/leds-netxbig.h b/include/dt-bindings/leds/leds-netxbig.h
new file mode 100644
index 000000000000..92658b0310b2
--- /dev/null
+++ b/include/dt-bindings/leds/leds-netxbig.h
@@ -0,0 +1,18 @@
+/*
+ * This header provides constants for netxbig LED bindings.
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#ifndef _DT_BINDINGS_LEDS_NETXBIG_H
+#define _DT_BINDINGS_LEDS_NETXBIG_H
+
+#define NETXBIG_LED_OFF		0
+#define NETXBIG_LED_ON		1
+#define NETXBIG_LED_SATA	2
+#define NETXBIG_LED_TIMER1	3
+#define NETXBIG_LED_TIMER2	4
+
+#endif /* _DT_BINDINGS_LEDS_NETXBIG_H */
-- 
2.1.4

[PATCH v4 2/3] ARM: Kirkwood: add LED DT entries for netxbig boards

[Simon Guinot]

This patch adds DT entries for the LEDs found on the Kirkwood-based
LaCie boards 2Big and 5Big Network v2.

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/boot/dts/kirkwood-net5big.dts  | 60 +++++++++++++++++++++++++
 arch/arm/boot/dts/kirkwood-netxbig.dtsi | 80 +++++++++++++++++++++++++++++++++
 2 files changed, 140 insertions(+)

Changes for v4:
- Add some Acked-by.

diff --git a/arch/arm/boot/dts/kirkwood-net5big.dts b/arch/arm/boot/dts/kirkwood-net5big.dts
index 36155b749d9f..3cc5e469df06 100644
--- a/arch/arm/boot/dts/kirkwood-net5big.dts
+++ b/arch/arm/boot/dts/kirkwood-net5big.dts
@@ -86,6 +86,66 @@
 			 clock-frequency = <32768>;
 	       };
 	};
+
+	netxbig-leds {
+		blue-sata2 {
+			label = "netxbig:blue:sata2";
+			mode-addr = <5>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 7
+				    NETXBIG_LED_SATA 1
+				    NETXBIG_LED_TIMER1 3>;
+			bright-addr = <2>;
+			bright-max = <7>;
+		};
+		red-sata2 {
+			label = "netxbig:red:sata2";
+			mode-addr = <5>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 2
+				    NETXBIG_LED_TIMER1 4>;
+			bright-addr = <2>;
+			bright-max = <7>;
+		};
+		blue-sata3 {
+			label = "netxbig:blue:sata3";
+			mode-addr = <6>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 7
+				    NETXBIG_LED_SATA 1
+				    NETXBIG_LED_TIMER1 3>;
+			bright-addr = <2>;
+			bright-max = <7>;
+		};
+		red-sata3 {
+			label = "netxbig:red:sata3";
+			mode-addr = <6>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 2
+				    NETXBIG_LED_TIMER1 4>;
+			bright-addr = <2>;
+			bright-max = <7>;
+		};
+		blue-sata4 {
+			label = "netxbig:blue:sata4";
+			mode-addr = <7>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 7
+				    NETXBIG_LED_SATA 1
+				    NETXBIG_LED_TIMER1 3>;
+			bright-addr = <2>;
+			bright-max = <7>;
+		};
+		red-sata4 {
+			label = "netxbig:red:sata4";
+			mode-addr = <7>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 2
+				    NETXBIG_LED_TIMER1 4>;
+			bright-addr = <2>;
+			bright-max = <7>;
+		};
+	};
 };
 
 &mdio {
diff --git a/arch/arm/boot/dts/kirkwood-netxbig.dtsi b/arch/arm/boot/dts/kirkwood-netxbig.dtsi
index 1508b12147df..4de5e53cd557 100644
--- a/arch/arm/boot/dts/kirkwood-netxbig.dtsi
+++ b/arch/arm/boot/dts/kirkwood-netxbig.dtsi
@@ -13,6 +13,7 @@
  * warranty of any kind, whether express or implied.
 */
 
+#include <dt-bindings/leds/leds-netxbig.h>
 #include "kirkwood.dtsi"
 #include "kirkwood-6281.dtsi"
 
@@ -105,6 +106,85 @@
 			gpio = <&gpio0 16 GPIO_ACTIVE_HIGH>;
 		};
 	};
+
+	netxbig_gpio_ext: netxbig-gpio-ext {
+		compatible = "lacie,netxbig-gpio-ext";
+
+		addr-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH
+			      &gpio1 16 GPIO_ACTIVE_HIGH
+			      &gpio1 17 GPIO_ACTIVE_HIGH>;
+		data-gpios = <&gpio1 12 GPIO_ACTIVE_HIGH
+			      &gpio1 13 GPIO_ACTIVE_HIGH
+			      &gpio1 14 GPIO_ACTIVE_HIGH>;
+		enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
+	};
+
+	netxbig-leds {
+		compatible = "lacie,netxbig-leds";
+
+		gpio-ext = <&netxbig_gpio_ext>;
+
+		timers = <NETXBIG_LED_TIMER1 500 500
+			  NETXBIG_LED_TIMER2 500 1000>;
+
+		blue-power {
+			label = "netxbig:blue:power";
+			mode-addr = <0>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 1
+				    NETXBIG_LED_TIMER1 3
+				    NETXBIG_LED_TIMER2 7>;
+			bright-addr = <1>;
+			bright-max = <7>;
+		};
+		red-power {
+			label = "netxbig:red:power";
+			mode-addr = <0>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 2
+				    NETXBIG_LED_TIMER1 4>;
+			bright-addr = <1>;
+			bright-max = <7>;
+		};
+		blue-sata0 {
+			label = "netxbig:blue:sata0";
+			mode-addr = <3>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 7
+				    NETXBIG_LED_SATA 1
+				    NETXBIG_LED_TIMER1 3>;
+			bright-addr = <2>;
+			bright-max = <7>;
+		};
+		red-sata0 {
+			label = "netxbig:red:sata0";
+			mode-addr = <3>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 2
+				    NETXBIG_LED_TIMER1 4>;
+			bright-addr = <2>;
+			bright-max = <7>;
+		};
+		blue-sata1 {
+			label = "netxbig:blue:sata1";
+			mode-addr = <4>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 7
+				    NETXBIG_LED_SATA 1
+				    NETXBIG_LED_TIMER1 3>;
+			bright-addr = <2>;
+			bright-max = <7>;
+		};
+		red-sata1 {
+			label = "netxbig:red:sata1";
+			mode-addr = <4>;
+			mode-val = <NETXBIG_LED_OFF 0
+				    NETXBIG_LED_ON 2
+				    NETXBIG_LED_TIMER1 4>;
+			bright-addr = <2>;
+			bright-max = <7>;
+		};
+	};
 };
 
 &mdio {
-- 
2.1.4

[PATCH v4 3/3] ARM: mvebu: remove static LED setup for netxbig boards

[Simon Guinot]

Since DT support is now available for the LEDs found on the LaCie
netxbig boards (Kirkwood-based), then the old-fashion netxbig board
setup file is no longer needed. This patch removes this file.

Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
 arch/arm/mach-mvebu/Kconfig    |   7 --
 arch/arm/mach-mvebu/Makefile   |   1 -
 arch/arm/mach-mvebu/board.h    |  21 -----
 arch/arm/mach-mvebu/kirkwood.c |   4 -
 arch/arm/mach-mvebu/netxbig.c  | 191 -----------------------------------------
 5 files changed, 224 deletions(-)
 delete mode 100644 arch/arm/mach-mvebu/board.h
 delete mode 100644 arch/arm/mach-mvebu/netxbig.c

Changes for v4:
- Add some Acked-by.

diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig
index 97473168d6b6..5453f901099b 100644
--- a/arch/arm/mach-mvebu/Kconfig
+++ b/arch/arm/mach-mvebu/Kconfig
@@ -116,11 +116,4 @@ config MACH_KIRKWOOD
 	  Say 'Y' here if you want your kernel to support boards based
 	  on the Marvell Kirkwood device tree.
 
-config MACH_NETXBIG
-	bool "LaCie 2Big and 5Big Network v2"
-	depends on MACH_KIRKWOOD
-	help
-	  Say 'Y' here if you want your kernel to support the
-	  LaCie 2Big and 5Big Network v2
-
 endif
diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
index b4f01497ce0b..ecf9e0c3b107 100644
--- a/arch/arm/mach-mvebu/Makefile
+++ b/arch/arm/mach-mvebu/Makefile
@@ -13,4 +13,3 @@ endif
 
 obj-$(CONFIG_MACH_DOVE)		 += dove.o
 obj-$(CONFIG_MACH_KIRKWOOD)	 += kirkwood.o kirkwood-pm.o
-obj-$(CONFIG_MACH_NETXBIG)	 += netxbig.o
diff --git a/arch/arm/mach-mvebu/board.h b/arch/arm/mach-mvebu/board.h
deleted file mode 100644
index 98e32cc2ef3d..000000000000
--- a/arch/arm/mach-mvebu/board.h
+++ /dev/null
@@ -1,21 +0,0 @@
-/*
- * Board functions for Marvell System On Chip
- *
- * Copyright (C) 2014
- *
- * Andrew Lunn <andrew@lunn.ch>
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2.  This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
- */
-
-#ifndef __ARCH_MVEBU_BOARD_H
-#define __ARCH_MVEBU_BOARD_H
-
-#ifdef CONFIG_MACH_NETXBIG
-void netxbig_init(void);
-#else
-static inline void netxbig_init(void) {};
-#endif
-#endif
diff --git a/arch/arm/mach-mvebu/kirkwood.c b/arch/arm/mach-mvebu/kirkwood.c
index 925f75f54268..f9d8e1ea7183 100644
--- a/arch/arm/mach-mvebu/kirkwood.c
+++ b/arch/arm/mach-mvebu/kirkwood.c
@@ -25,7 +25,6 @@
 #include "kirkwood.h"
 #include "kirkwood-pm.h"
 #include "common.h"
-#include "board.h"
 
 static struct resource kirkwood_cpufreq_resources[] = {
 	[0] = {
@@ -180,9 +179,6 @@ static void __init kirkwood_dt_init(void)
 	kirkwood_pm_init();
 	kirkwood_dt_eth_fixup();
 
-	if (of_machine_is_compatible("lacie,netxbig"))
-		netxbig_init();
-
 	of_platform_populate(NULL, of_default_bus_match_table, auxdata, NULL);
 }
 
diff --git a/arch/arm/mach-mvebu/netxbig.c b/arch/arm/mach-mvebu/netxbig.c
deleted file mode 100644
index 94b11b6585a4..000000000000
--- a/arch/arm/mach-mvebu/netxbig.c
+++ /dev/null
@@ -1,191 +0,0 @@
-/*
- * arch/arm/mach-mvbu/board-netxbig.c
- *
- * LaCie 2Big and 5Big Network v2 board setup
- *
- * Copyright (C) 2010 Simon Guinot <sguinot@lacie.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <linux/kernel.h>
-#include <linux/of.h>
-#include <linux/platform_device.h>
-#include <linux/platform_data/leds-kirkwood-netxbig.h>
-#include "common.h"
-
-/*****************************************************************************
- * GPIO extension LEDs
- ****************************************************************************/
-
-/*
- * The LEDs are controlled by a CPLD and can be configured through a GPIO
- * extension bus:
- *
- * - address register : bit [0-2] -> GPIO [47-49]
- * - data register    : bit [0-2] -> GPIO [44-46]
- * - enable register  : GPIO 29
- */
-
-static int netxbig_v2_gpio_ext_addr[] = { 47, 48, 49 };
-static int netxbig_v2_gpio_ext_data[] = { 44, 45, 46 };
-
-static struct netxbig_gpio_ext netxbig_v2_gpio_ext = {
-	.addr		= netxbig_v2_gpio_ext_addr,
-	.num_addr	= ARRAY_SIZE(netxbig_v2_gpio_ext_addr),
-	.data		= netxbig_v2_gpio_ext_data,
-	.num_data	= ARRAY_SIZE(netxbig_v2_gpio_ext_data),
-	.enable		= 29,
-};
-
-/*
- * Address register selection:
- *
- * addr | register
- * ----------------------------
- *   0  | front LED
- *   1  | front LED brightness
- *   2  | SATA LED brightness
- *   3  | SATA0 LED
- *   4  | SATA1 LED
- *   5  | SATA2 LED
- *   6  | SATA3 LED
- *   7  | SATA4 LED
- *
- * Data register configuration:
- *
- * data | LED brightness
- * -------------------------------------------------
- *   0  | min (off)
- *   -  | -
- *   7  | max
- *
- * data | front LED mode
- * -------------------------------------------------
- *   0  | fix off
- *   1  | fix blue on
- *   2  | fix red on
- *   3  | blink blue on=1 sec and blue off=1 sec
- *   4  | blink red on=1 sec and red off=1 sec
- *   5  | blink blue on=2.5 sec and red on=0.5 sec
- *   6  | blink blue on=1 sec and red on=1 sec
- *   7  | blink blue on=0.5 sec and blue off=2.5 sec
- *
- * data | SATA LED mode
- * -------------------------------------------------
- *   0  | fix off
- *   1  | SATA activity blink
- *   2  | fix red on
- *   3  | blink blue on=1 sec and blue off=1 sec
- *   4  | blink red on=1 sec and red off=1 sec
- *   5  | blink blue on=2.5 sec and red on=0.5 sec
- *   6  | blink blue on=1 sec and red on=1 sec
- *   7  | fix blue on
- */
-
-static int netxbig_v2_red_mled[NETXBIG_LED_MODE_NUM] = {
-	[NETXBIG_LED_OFF]	= 0,
-	[NETXBIG_LED_ON]	= 2,
-	[NETXBIG_LED_SATA]	= NETXBIG_LED_INVALID_MODE,
-	[NETXBIG_LED_TIMER1]	= 4,
-	[NETXBIG_LED_TIMER2]	= NETXBIG_LED_INVALID_MODE,
-};
-
-static int netxbig_v2_blue_pwr_mled[NETXBIG_LED_MODE_NUM] = {
-	[NETXBIG_LED_OFF]	= 0,
-	[NETXBIG_LED_ON]	= 1,
-	[NETXBIG_LED_SATA]	= NETXBIG_LED_INVALID_MODE,
-	[NETXBIG_LED_TIMER1]	= 3,
-	[NETXBIG_LED_TIMER2]	= 7,
-};
-
-static int netxbig_v2_blue_sata_mled[NETXBIG_LED_MODE_NUM] = {
-	[NETXBIG_LED_OFF]	= 0,
-	[NETXBIG_LED_ON]	= 7,
-	[NETXBIG_LED_SATA]	= 1,
-	[NETXBIG_LED_TIMER1]	= 3,
-	[NETXBIG_LED_TIMER2]	= NETXBIG_LED_INVALID_MODE,
-};
-
-static struct netxbig_led_timer netxbig_v2_led_timer[] = {
-	[0] = {
-		.delay_on	= 500,
-		.delay_off	= 500,
-		.mode		= NETXBIG_LED_TIMER1,
-	},
-	[1] = {
-		.delay_on	= 500,
-		.delay_off	= 1000,
-		.mode		= NETXBIG_LED_TIMER2,
-	},
-};
-
-#define NETXBIG_LED(_name, maddr, mval, baddr)			\
-	{ .name		= _name,				\
-	  .mode_addr	= maddr,				\
-	  .mode_val	= mval,					\
-	  .bright_addr	= baddr }
-
-static struct netxbig_led net2big_v2_leds_ctrl[] = {
-	NETXBIG_LED("net2big-v2:blue:power", 0, netxbig_v2_blue_pwr_mled,  1),
-	NETXBIG_LED("net2big-v2:red:power",  0, netxbig_v2_red_mled,       1),
-	NETXBIG_LED("net2big-v2:blue:sata0", 3, netxbig_v2_blue_sata_mled, 2),
-	NETXBIG_LED("net2big-v2:red:sata0",  3, netxbig_v2_red_mled,       2),
-	NETXBIG_LED("net2big-v2:blue:sata1", 4, netxbig_v2_blue_sata_mled, 2),
-	NETXBIG_LED("net2big-v2:red:sata1",  4, netxbig_v2_red_mled,       2),
-};
-
-static struct netxbig_led_platform_data net2big_v2_leds_data = {
-	.gpio_ext	= &netxbig_v2_gpio_ext,
-	.timer		= netxbig_v2_led_timer,
-	.num_timer	= ARRAY_SIZE(netxbig_v2_led_timer),
-	.leds		= net2big_v2_leds_ctrl,
-	.num_leds	= ARRAY_SIZE(net2big_v2_leds_ctrl),
-};
-
-static struct netxbig_led net5big_v2_leds_ctrl[] = {
-	NETXBIG_LED("net5big-v2:blue:power", 0, netxbig_v2_blue_pwr_mled,  1),
-	NETXBIG_LED("net5big-v2:red:power",  0, netxbig_v2_red_mled,       1),
-	NETXBIG_LED("net5big-v2:blue:sata0", 3, netxbig_v2_blue_sata_mled, 2),
-	NETXBIG_LED("net5big-v2:red:sata0",  3, netxbig_v2_red_mled,       2),
-	NETXBIG_LED("net5big-v2:blue:sata1", 4, netxbig_v2_blue_sata_mled, 2),
-	NETXBIG_LED("net5big-v2:red:sata1",  4, netxbig_v2_red_mled,       2),
-	NETXBIG_LED("net5big-v2:blue:sata2", 5, netxbig_v2_blue_sata_mled, 2),
-	NETXBIG_LED("net5big-v2:red:sata2",  5, netxbig_v2_red_mled,       2),
-	NETXBIG_LED("net5big-v2:blue:sata3", 6, netxbig_v2_blue_sata_mled, 2),
-	NETXBIG_LED("net5big-v2:red:sata3",  6, netxbig_v2_red_mled,       2),
-	NETXBIG_LED("net5big-v2:blue:sata4", 7, netxbig_v2_blue_sata_mled, 2),
-	NETXBIG_LED("net5big-v2:red:sata4",  7, netxbig_v2_red_mled,       2),
-};
-
-static struct netxbig_led_platform_data net5big_v2_leds_data = {
-	.gpio_ext	= &netxbig_v2_gpio_ext,
-	.timer		= netxbig_v2_led_timer,
-	.num_timer	= ARRAY_SIZE(netxbig_v2_led_timer),
-	.leds		= net5big_v2_leds_ctrl,
-	.num_leds	= ARRAY_SIZE(net5big_v2_leds_ctrl),
-};
-
-static struct platform_device netxbig_v2_leds = {
-	.name		= "leds-netxbig",
-	.id		= -1,
-	.dev		= {
-		.platform_data	= &net2big_v2_leds_data,
-	},
-};
-
-void __init netxbig_init(void)
-{
-
-	if (of_machine_is_compatible("lacie,net5big_v2"))
-		netxbig_v2_leds.dev.platform_data = &net5big_v2_leds_data;
-	platform_device_register(&netxbig_v2_leds);
-}
-- 
2.1.4

Re: [PATCH v4 0/3] Add DT support for netxbig LEDs

[Jacek Anaszewski]

Hi Simon,

Thanks for the update.

On 09/17/2015 05:59 PM, Simon Guinot wrote:
> Hello,
>
> This patch series adds DT support for the LEDs found on the Kirkwood-based
> LaCie boards 2Big and 5Big Network v2.
>
> Changes for v2:
> - Check timer mode value retrieved from DT.
> - In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
>    timer delay values from DT with function of_property_read_u32_index.
>    Instead, use a temporary u32 variable. This allows to silence a static
>    checker warning.
> - Make timer property optional in the binding documentation. It is now
>    aligned with the driver code.
>
> Changes for v3:
> - Fix pointer usage with the temporary u32 variable while calling
>    of_property_read_u32_index.
>
> Changes for v4:
> - In DT binding document netxbig-gpio-ext.txt, detail byte order for
>    registers and latch mechanism for "enable-gpio".
> - In leds-netxbig.c, add some error messages.
> - In leds-netxbig.c, fix some "sizeof" style issues.
> - In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
>    of_property_read_string() calls after the error-prone checks.
> - Add some Acked-by.
>
> Jacek,
>
> I did not convert the bright-max DT property into led-max-microamp.
> The reason is that I can't translate the bright-max values into
> microamperes.

Doesn't specification of your device say what current value given
brightness level reflects?


> Therefore, it looks more consistent to keep the same
> naming prefix for the bright-addr and bright-max DT properties.




-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v4 1/3] leds: netxbig: add device tree binding

[Jacek Anaszewski]

Hi Simon,

Please find my comments in the code below.

On 09/17/2015 05:59 PM, Simon Guinot wrote:
> This patch adds device tree support for the netxbig LEDs.
>
> This also introduces a additionnal DT binding for the GPIO extension bus
> (netxbig-gpio-ext) used to configure the LEDs. Since this bus could also
> be used to control other devices, then it seems more suitable to have it
> in a separate DT binding.
>
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>   .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
>   .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++++
>   drivers/leds/leds-netxbig.c                        | 258 +++++++++++++++++++--
>   include/dt-bindings/leds/leds-netxbig.h            |  18 ++
>   4 files changed, 369 insertions(+), 21 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
>   create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt
>   create mode 100644 include/dt-bindings/leds/leds-netxbig.h
>
> Changes for v2:
> - Check timer mode value retrieved from DT.
> - In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
>    timer delay values from DT with function of_property_read_u32_index.
>    Instead, use a temporary u32 variable. This allows to silence a static
>    checker warning.
> - Make timer property optional in the binding documentation. It is now
>    aligned with the driver code.
>
> Changes for v3:
> - Fix pointer usage with the temporary u32 variable while calling
>    of_property_read_u32_index.
>
> Changes for v4:
> - In DT binding document netxbig-gpio-ext.txt, detail byte order for
>    registers and latch mechanism for "enable-gpio".
> - In leds-netxbig.c, add some error messages.
> - In leds-netxbig.c, fix some "sizeof" style issues.
> - In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
>    of_property_read_string() calls after the error-prone checks.
> - Add some Acked-by.
>
> diff --git a/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> new file mode 100644
> index 000000000000..50ec2e690701
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> @@ -0,0 +1,22 @@
> +Binding for the GPIO extension bus found on some LaCie/Seagate boards
> +(Example: 2Big/5Big Network v2, 2Big NAS).
> +
> +Required properties:
> +- compatible: "lacie,netxbig-gpio-ext".
> +- addr-gpios: GPIOs representing the address register (LSB -> MSB).
> +- data-gpios: GPIOs representing the data register (LSB -> MSB).
> +- enable-gpio: latches the new configuration (address, data) on raising edge.
> +
> +Example:
> +
> +netxbig_gpio_ext: netxbig-gpio-ext {
> +	compatible = "lacie,netxbig-gpio-ext";
> +
> +	addr-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH
> +		      &gpio1 16 GPIO_ACTIVE_HIGH
> +		      &gpio1 17 GPIO_ACTIVE_HIGH>;
> +	data-gpios = <&gpio1 12 GPIO_ACTIVE_HIGH
> +		      &gpio1 13 GPIO_ACTIVE_HIGH
> +		      &gpio1 14 GPIO_ACTIVE_HIGH>;
> +	enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
> +};
> diff --git a/Documentation/devicetree/bindings/leds/leds-netxbig.txt b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
> new file mode 100644
> index 000000000000..efadbecbfeb9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
> @@ -0,0 +1,92 @@
> +Binding for the CPLD LEDs (GPIO extension bus) found on some LaCie/Seagate
> +boards (Example: 2Big/5Big Network v2, 2Big NAS).
> +
> +Required properties:
> +- compatible: "lacie,netxbig-leds".
> +- gpio-ext: Phandle for the gpio-ext bus.
> +
> +Optional properties:
> +- timers: Timer array. Each timer entry is represented by three integers:
> +  Mode (gpio-ext bus), delay_on and delay_off.
> +
> +Each LED is represented as a sub-node of the netxbig-leds device.
> +
> +Required sub-node properties:
> +- mode-addr: Mode register address on gpio-ext bus.
> +- mode-val: Mode to value mapping. Each entry is represented by two integers:
> +  A mode and the corresponding value on the gpio-ext bus.
> +- bright-addr: Brightness register address on gpio-ext bus.
> +- bright-max: Maximum brightness value.
> +
> +Optional sub-node properties:
> +- label: Name for this LED. If omitted, the label is taken from the node name.
> +- linux,default-trigger: Trigger assigned to the LED.
> +
> +Example:
> +
> +netxbig-leds {
> +	compatible = "lacie,netxbig-leds";
> +
> +	gpio-ext = &gpio_ext;
> +
> +	timers = <NETXBIG_LED_TIMER1 500 500
> +		  NETXBIG_LED_TIMER2 500 1000>;
> +
> +	blue-power {
> +		label = "netxbig:blue:power";
> +		mode-addr = <0>;
> +		mode-val = <NETXBIG_LED_OFF 0
> +			    NETXBIG_LED_ON 1
> +			    NETXBIG_LED_TIMER1 3
> +			    NETXBIG_LED_TIMER2 7>;
> +		bright-addr = <1>;
> +		bright-max = <7>;
> +	};
> +	red-power {
> +		label = "netxbig:red:power";
> +		mode-addr = <0>;
> +		mode-val = <NETXBIG_LED_OFF 0
> +			    NETXBIG_LED_ON 2
> +			    NETXBIG_LED_TIMER1 4>;
> +		bright-addr = <1>;
> +		bright-max = <7>;
> +	};
> +	blue-sata0 {
> +		label = "netxbig:blue:sata0";
> +		mode-addr = <3>;
> +		mode-val = <NETXBIG_LED_OFF 0
> +			    NETXBIG_LED_ON 7
> +			    NETXBIG_LED_SATA 1
> +			    NETXBIG_LED_TIMER1 3>;
> +		bright-addr = <2>;
> +		bright-max = <7>;
> +	};
> +	red-sata0 {
> +		label = "netxbig:red:sata0";
> +		mode-addr = <3>;
> +		mode-val = <NETXBIG_LED_OFF 0
> +			    NETXBIG_LED_ON 2
> +			    NETXBIG_LED_TIMER1 4>;
> +		bright-addr = <2>;
> +		bright-max = <7>;
> +	};
> +	blue-sata1 {
> +		label = "netxbig:blue:sata1";
> +		mode-addr = <4>;
> +		mode-val = <NETXBIG_LED_OFF 0
> +			    NETXBIG_LED_ON 7
> +			    NETXBIG_LED_SATA 1
> +			    NETXBIG_LED_TIMER1 3>;
> +		bright-addr = <2>;
> +		bright-max = <7>;
> +	};
> +	red-sata1 {
> +		label = "netxbig:red:sata1";
> +		mode-addr = <4>;
> +		mode-val = <NETXBIG_LED_OFF 0
> +			    NETXBIG_LED_ON 2
> +			    NETXBIG_LED_TIMER1 4>;
> +		bright-addr = <2>;
> +		bright-max = <7>;
> +	};
> +};
> diff --git a/drivers/leds/leds-netxbig.c b/drivers/leds/leds-netxbig.c
> index 25e419752a7b..dcd8302cccaf 100644
> --- a/drivers/leds/leds-netxbig.c
> +++ b/drivers/leds/leds-netxbig.c
> @@ -26,6 +26,7 @@
>   #include <linux/spinlock.h>
>   #include <linux/platform_device.h>
>   #include <linux/gpio.h>
> +#include <linux/of_gpio.h>
>   #include <linux/leds.h>
>   #include <linux/platform_data/leds-kirkwood-netxbig.h>
>
> @@ -140,6 +141,11 @@ struct netxbig_led_data {
>   	spinlock_t		lock;
>   };
>
> +struct netxbig_led_priv {
> +	struct netxbig_led_platform_data *pdata;
> +	struct netxbig_led_data leds_data[];
> +};
> +
>   static int netxbig_led_get_timer_mode(enum netxbig_led_mode *mode,
>   				      unsigned long delay_on,
>   				      unsigned long delay_off,
> @@ -304,12 +310,12 @@ static void delete_netxbig_led(struct netxbig_led_data *led_dat)
>   	led_classdev_unregister(&led_dat->cdev);
>   }
>
> -static int
> -create_netxbig_led(struct platform_device *pdev,
> -		   struct netxbig_led_data *led_dat,
> -		   const struct netxbig_led *template)
> +static int create_netxbig_led(struct platform_device *pdev, int led,
> +			      struct netxbig_led_priv *priv)
>   {
> -	struct netxbig_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct netxbig_led_platform_data *pdata = priv->pdata;
> +	struct netxbig_led_data *led_dat = &priv->leds_data[led];
> +	const struct netxbig_led *template = &priv->pdata->leds[led];
>
>   	spin_lock_init(&led_dat->lock);
>   	led_dat->gpio_ext = pdata->gpio_ext;
> @@ -346,38 +352,249 @@ create_netxbig_led(struct platform_device *pdev,
>   	return led_classdev_register(&pdev->dev, &led_dat->cdev);
>   }
>
> +#ifdef CONFIG_OF_GPIO
> +static int gpio_ext_get_of_pdata(struct device *dev, struct device_node *np,
> +				 struct netxbig_gpio_ext *gpio_ext)
> +{
> +	int *addr, *data;
> +	int num_addr, num_data;
> +	int ret;
> +	int i;
> +
> +	ret = of_gpio_named_count(np, "addr-gpios");
> +	if (ret < 0) {
> +		dev_err(dev,
> +			"Failed to count GPIOs in DT property addr-gpios\n");
> +		return ret;
> +	}
> +	num_addr = ret;
> +	addr = devm_kzalloc(dev, num_addr * sizeof(*addr), GFP_KERNEL);
> +	if (!addr)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_addr; i++) {
> +		ret = of_get_named_gpio(np, "addr-gpios", i);
> +		if (ret < 0)
> +			return ret;
> +		addr[i] = ret;
> +	}
> +	gpio_ext->addr = addr;
> +	gpio_ext->num_addr = num_addr;
> +
> +	ret = of_gpio_named_count(np, "data-gpios");
> +	if (ret < 0) {
> +		dev_err(dev,
> +			"Failed to count GPIOs in DT property data-gpios\n");
> +		return ret;
> +	}
> +	num_data = ret;
> +	data = devm_kzalloc(dev, num_data * sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_data; i++) {
> +		ret = of_get_named_gpio(np, "data-gpios", i);
> +		if (ret < 0)
> +			return ret;
> +		data[i] = ret;
> +	}
> +	gpio_ext->data = data;
> +	gpio_ext->num_data = num_data;
> +
> +	ret = of_get_named_gpio(np, "enable-gpio", 0);
> +	if (ret < 0) {
> +		dev_err(dev,
> +			"Failed to get GPIO from DT property enable-gpio\n");
> +		return ret;
> +	}
> +	gpio_ext->enable = ret;
> +
> +	return 0;
> +}

Since netxbig-gpio-ext extension bus can be also used to control other
device, and the related DT bindings are meant for GPIO subsystem, then
the above parser should also be placed there to make it available
for reuse by other drivers. The gpio_ext_init function should be also
moved to GPIO subsystem, presumably to the same module.

Moreover, if you switched to using devm* prefixed version of
gpio_request_one and led_classdev_reqister, you could simplify
the error paths in the driver.

> +static int netxbig_leds_get_of_pdata(struct device *dev,
> +				     struct netxbig_led_platform_data *pdata)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct device_node *gpio_ext_np;
> +	struct device_node *child;
> +	struct netxbig_gpio_ext *gpio_ext;
> +	struct netxbig_led_timer *timers;
> +	struct netxbig_led *leds, *led;
> +	int num_timers;
> +	int num_leds = 0;
> +	int ret;
> +	int i;
> +
> +	/* GPIO extension */
> +	gpio_ext_np = of_parse_phandle(np, "gpio-ext", 0);
> +	if (!gpio_ext_np) {
> +		dev_err(dev, "Failed to get DT handle gpio-ext\n");
> +		return -EINVAL;
> +	}
> +
> +	gpio_ext = devm_kzalloc(dev, sizeof(*gpio_ext), GFP_KERNEL);
> +	if (!gpio_ext)
> +		return -ENOMEM;
> +	ret = gpio_ext_get_of_pdata(dev, gpio_ext_np, gpio_ext);
> +	if (ret)
> +		return ret;
> +	of_node_put(gpio_ext_np);
> +	pdata->gpio_ext = gpio_ext;
> +
> +	/* Timers (optional) */
> +	ret = of_property_count_u32_elems(np, "timers");
> +	if (ret > 0) {
> +		if (ret % 3)
> +			return -EINVAL;
> +		num_timers = ret / 3;
> +		timers = devm_kzalloc(dev, num_timers * sizeof(*timers),
> +				      GFP_KERNEL);
> +		if (!timers)
> +			return -ENOMEM;
> +		for (i = 0; i < num_timers; i++) {
> +			u32 tmp;
> +
> +			of_property_read_u32_index(np, "timers", 3 * i,
> +						   &timers[i].mode);
> +			if (timers[i].mode >= NETXBIG_LED_MODE_NUM)
> +				return -EINVAL;
> +			of_property_read_u32_index(np, "timers",
> +						   3 * i + 1, &tmp);
> +			timers[i].delay_on = tmp;
> +			of_property_read_u32_index(np, "timers",
> +						   3 * i + 2, &tmp);
> +			timers[i].delay_off = tmp;
> +		}
> +		pdata->timer = timers;
> +		pdata->num_timer = num_timers;
> +	}
> +
> +	/* LEDs */
> +	num_leds = of_get_child_count(np);
> +	if (!num_leds) {
> +		dev_err(dev, "No LED subnodes found in DT\n");
> +		return -ENODEV;
> +	}
> +
> +	leds = devm_kzalloc(dev, num_leds * sizeof(*leds), GFP_KERNEL);
> +	if (!leds)
> +		return -ENOMEM;
> +
> +	led = leds;
> +	for_each_child_of_node(np, child) {
> +		const char *string;
> +		int *mode_val;
> +		int num_modes;
> +
> +		if (of_property_read_u32(child, "mode-addr",
> +					 &led->mode_addr))
> +			return -EINVAL;
> +
> +		if (of_property_read_u32(child, "bright-addr",
> +					 &led->bright_addr))
> +			return -EINVAL;

You don't parse bright-max DT property anywhere, AFAICT.

> +		mode_val =
> +			devm_kzalloc(dev,
> +				     NETXBIG_LED_MODE_NUM * sizeof(*mode_val),
> +				     GFP_KERNEL);
> +		if (!mode_val)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < NETXBIG_LED_MODE_NUM; i++)
> +			mode_val[i] = NETXBIG_LED_INVALID_MODE;
> +
> +		ret = of_property_count_u32_elems(child, "mode-val");
> +		if (ret < 0 || ret % 2)
> +			return -EINVAL;
> +		num_modes = ret / 2;
> +		if (num_modes > NETXBIG_LED_MODE_NUM)
> +			return -EINVAL;
> +
> +		for (i = 0; i < num_modes; i++) {
> +			int mode;
> +			int val;
> +
> +			of_property_read_u32_index(child,
> +						   "mode-val", 2 * i, &mode);
> +			of_property_read_u32_index(child,
> +						   "mode-val", 2 * i + 1, &val);
> +			if (mode >= NETXBIG_LED_MODE_NUM)
> +				return -EINVAL;
> +			mode_val[mode] = val;
> +		}
> +		led->mode_val = mode_val;
> +
> +		if (!of_property_read_string(child, "label", &string))
> +			led->name = string;
> +		else
> +			led->name = child->name;
> +
> +		if (!of_property_read_string(child,
> +					     "linux,default-trigger", &string))
> +			led->default_trigger = string;
> +
> +		led++;
> +	}
> +
> +	pdata->leds = leds;
> +	pdata->num_leds = num_leds;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id of_netxbig_leds_match[] = {
> +	{ .compatible = "lacie,netxbig-leds", },
> +	{},
> +};
> +#else
> +static int netxbig_leds_get_of_pdata(struct device *dev,
> +				     struct netxbig_led_platform_data *pdata)

s/static int/static inline int/

> +{
> +	return -ENODEV;
> +}
> +#endif /* CONFIG_OF_GPIO */
> +
>   static int netxbig_led_probe(struct platform_device *pdev)
>   {
>   	struct netxbig_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
> -	struct netxbig_led_data *leds_data;
> +	struct netxbig_led_priv *priv;
>   	int i;
>   	int ret;
>
> -	if (!pdata)
> -		return -EINVAL;
> +	if (!pdata) {
> +		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			return -ENOMEM;
> +		ret = netxbig_leds_get_of_pdata(&pdev->dev, pdata);
> +		if (ret)
> +			return ret;
> +	}
>
> -	leds_data = devm_kzalloc(&pdev->dev,
> -		sizeof(struct netxbig_led_data) * pdata->num_leds, GFP_KERNEL);
> -	if (!leds_data)
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv) +
> +			    pdata->num_leds * sizeof(struct netxbig_led_data),
> +			    GFP_KERNEL);
> +	if (!priv)
>   		return -ENOMEM;
> +	priv->pdata = pdata;
>
>   	ret = gpio_ext_init(pdata->gpio_ext);
>   	if (ret < 0)
>   		return ret;
>
>   	for (i = 0; i < pdata->num_leds; i++) {
> -		ret = create_netxbig_led(pdev, &leds_data[i], &pdata->leds[i]);
> +		ret = create_netxbig_led(pdev, i, priv);
>   		if (ret < 0)
>   			goto err_free_leds;
>   	}
> -
> -	platform_set_drvdata(pdev, leds_data);
> +	platform_set_drvdata(pdev, priv);
>
>   	return 0;
>
>   err_free_leds:
>   	for (i = i - 1; i >= 0; i--)
> -		delete_netxbig_led(&leds_data[i]);
> +		delete_netxbig_led(&priv->leds_data[i]);
>
>   	gpio_ext_free(pdata->gpio_ext);
>   	return ret;
> @@ -385,14 +602,12 @@ err_free_leds:
>
>   static int netxbig_led_remove(struct platform_device *pdev)
>   {
> -	struct netxbig_led_platform_data *pdata = dev_get_platdata(&pdev->dev);
> -	struct netxbig_led_data *leds_data;
> +	struct netxbig_led_priv *priv = platform_get_drvdata(pdev);
> +	struct netxbig_led_platform_data *pdata = priv->pdata;
>   	int i;
>
> -	leds_data = platform_get_drvdata(pdev);
> -
>   	for (i = 0; i < pdata->num_leds; i++)
> -		delete_netxbig_led(&leds_data[i]);
> +		delete_netxbig_led(&priv->leds_data[i]);
>
>   	gpio_ext_free(pdata->gpio_ext);
>
> @@ -403,7 +618,8 @@ static struct platform_driver netxbig_led_driver = {
>   	.probe		= netxbig_led_probe,
>   	.remove		= netxbig_led_remove,
>   	.driver		= {
> -		.name	= "leds-netxbig",
> +		.name		= "leds-netxbig",
> +		.of_match_table	= of_match_ptr(of_netxbig_leds_match),
>   	},
>   };
>
> diff --git a/include/dt-bindings/leds/leds-netxbig.h b/include/dt-bindings/leds/leds-netxbig.h
> new file mode 100644
> index 000000000000..92658b0310b2
> --- /dev/null
> +++ b/include/dt-bindings/leds/leds-netxbig.h
> @@ -0,0 +1,18 @@
> +/*
> + * This header provides constants for netxbig LED bindings.
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#ifndef _DT_BINDINGS_LEDS_NETXBIG_H
> +#define _DT_BINDINGS_LEDS_NETXBIG_H
> +
> +#define NETXBIG_LED_OFF		0
> +#define NETXBIG_LED_ON		1
> +#define NETXBIG_LED_SATA	2
> +#define NETXBIG_LED_TIMER1	3
> +#define NETXBIG_LED_TIMER2	4
> +
> +#endif /* _DT_BINDINGS_LEDS_NETXBIG_H */
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v4 1/3] leds: netxbig: add device tree binding

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 7970 bytes --]

On Fri, Sep 18, 2015 at 11:16:41AM +0200, Jacek Anaszewski wrote:
> Hi Simon,
> 
> Please find my comments in the code below.

Hi Jacek,

Thanks for the quick review.

> 
> On 09/17/2015 05:59 PM, Simon Guinot wrote:
> >This patch adds device tree support for the netxbig LEDs.
> >
> >This also introduces a additionnal DT binding for the GPIO extension bus
> >(netxbig-gpio-ext) used to configure the LEDs. Since this bus could also
> >be used to control other devices, then it seems more suitable to have it
> >in a separate DT binding.
> >
> >Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> >Acked-by: Linus Walleij <linus.walleij@linaro.org>
> >---
> >  .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
> >  .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++++
> >  drivers/leds/leds-netxbig.c                        | 258 +++++++++++++++++++--
> >  include/dt-bindings/leds/leds-netxbig.h            |  18 ++
> >  4 files changed, 369 insertions(+), 21 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> >  create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt
> >  create mode 100644 include/dt-bindings/leds/leds-netxbig.h
> >
> >Changes for v2:
> >- Check timer mode value retrieved from DT.
> >- In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
> >   timer delay values from DT with function of_property_read_u32_index.
> >   Instead, use a temporary u32 variable. This allows to silence a static
> >   checker warning.
> >- Make timer property optional in the binding documentation. It is now
> >   aligned with the driver code.
> >
> >Changes for v3:
> >- Fix pointer usage with the temporary u32 variable while calling
> >   of_property_read_u32_index.
> >
> >Changes for v4:
> >- In DT binding document netxbig-gpio-ext.txt, detail byte order for
> >   registers and latch mechanism for "enable-gpio".
> >- In leds-netxbig.c, add some error messages.
> >- In leds-netxbig.c, fix some "sizeof" style issues.
> >- In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
> >   of_property_read_string() calls after the error-prone checks.
> >- Add some Acked-by.

...

> >+#ifdef CONFIG_OF_GPIO
> >+static int gpio_ext_get_of_pdata(struct device *dev, struct device_node *np,
> >+				 struct netxbig_gpio_ext *gpio_ext)
> >+{
> >+	int *addr, *data;
> >+	int num_addr, num_data;
> >+	int ret;
> >+	int i;
> >+
> >+	ret = of_gpio_named_count(np, "addr-gpios");
> >+	if (ret < 0) {
> >+		dev_err(dev,
> >+			"Failed to count GPIOs in DT property addr-gpios\n");
> >+		return ret;
> >+	}
> >+	num_addr = ret;
> >+	addr = devm_kzalloc(dev, num_addr * sizeof(*addr), GFP_KERNEL);
> >+	if (!addr)
> >+		return -ENOMEM;
> >+
> >+	for (i = 0; i < num_addr; i++) {
> >+		ret = of_get_named_gpio(np, "addr-gpios", i);
> >+		if (ret < 0)
> >+			return ret;
> >+		addr[i] = ret;
> >+	}
> >+	gpio_ext->addr = addr;
> >+	gpio_ext->num_addr = num_addr;
> >+
> >+	ret = of_gpio_named_count(np, "data-gpios");
> >+	if (ret < 0) {
> >+		dev_err(dev,
> >+			"Failed to count GPIOs in DT property data-gpios\n");
> >+		return ret;
> >+	}
> >+	num_data = ret;
> >+	data = devm_kzalloc(dev, num_data * sizeof(*data), GFP_KERNEL);
> >+	if (!data)
> >+		return -ENOMEM;
> >+
> >+	for (i = 0; i < num_data; i++) {
> >+		ret = of_get_named_gpio(np, "data-gpios", i);
> >+		if (ret < 0)
> >+			return ret;
> >+		data[i] = ret;
> >+	}
> >+	gpio_ext->data = data;
> >+	gpio_ext->num_data = num_data;
> >+
> >+	ret = of_get_named_gpio(np, "enable-gpio", 0);
> >+	if (ret < 0) {
> >+		dev_err(dev,
> >+			"Failed to get GPIO from DT property enable-gpio\n");
> >+		return ret;
> >+	}
> >+	gpio_ext->enable = ret;
> >+
> >+	return 0;
> >+}
> 
> Since netxbig-gpio-ext extension bus can be also used to control other
> device, and the related DT bindings are meant for GPIO subsystem, then
> the above parser should also be placed there to make it available
> for reuse by other drivers. The gpio_ext_init function should be also
> moved to GPIO subsystem, presumably to the same module.

Yes, this patch series is a first step. I am planning to move the
netxbig-gpio-ext extension bus into a dedicated driver as soon as I need
need it for an another device/driver. But for the time being, I was
planning to keep it here. If you agree with that of course.

> 
> Moreover, if you switched to using devm* prefixed version of
> gpio_request_one and led_classdev_reqister, you could simplify
> the error paths in the driver.

Yes, I have a pending patch for this conversion. But since it is not
really related with the subject of this patch series (add DT support),
I was planning to send it next.

Do you want me to include this patch into this series.

> 
> >+static int netxbig_leds_get_of_pdata(struct device *dev,
> >+				     struct netxbig_led_platform_data *pdata)
> >+{
> >+	struct device_node *np = dev->of_node;
> >+	struct device_node *gpio_ext_np;
> >+	struct device_node *child;
> >+	struct netxbig_gpio_ext *gpio_ext;
> >+	struct netxbig_led_timer *timers;
> >+	struct netxbig_led *leds, *led;
> >+	int num_timers;
> >+	int num_leds = 0;
> >+	int ret;
> >+	int i;

...

> >+	led = leds;
> >+	for_each_child_of_node(np, child) {
> >+		const char *string;
> >+		int *mode_val;
> >+		int num_modes;
> >+
> >+		if (of_property_read_u32(child, "mode-addr",
> >+					 &led->mode_addr))
> >+			return -EINVAL;
> >+
> >+		if (of_property_read_u32(child, "bright-addr",
> >+					 &led->bright_addr))
> >+			return -EINVAL;
> 
> You don't parse bright-max DT property anywhere, AFAICT.

Yes I don't. I have added this bright-max property thinking ahead of
moving the netxbig-gpio-ext stuff into a dedicated driver. For now, it
is possible to compute bright-max from the number of data GPIOs of the
extension bus. But once it will be removed something else will be
needed. That's why I introduced this property.

But now, I am thinking I should have used this property right now. It
will be more convenient when moving the netxbig-gpio-ext code out.

I'll do that for the next round.

> 
> >+		mode_val =
> >+			devm_kzalloc(dev,
> >+				     NETXBIG_LED_MODE_NUM * sizeof(*mode_val),
> >+				     GFP_KERNEL);
> >+		if (!mode_val)
> >+			return -ENOMEM;
> >+
> >+		for (i = 0; i < NETXBIG_LED_MODE_NUM; i++)
> >+			mode_val[i] = NETXBIG_LED_INVALID_MODE;
> >+
> >+		ret = of_property_count_u32_elems(child, "mode-val");
> >+		if (ret < 0 || ret % 2)
> >+			return -EINVAL;
> >+		num_modes = ret / 2;
> >+		if (num_modes > NETXBIG_LED_MODE_NUM)
> >+			return -EINVAL;
> >+
> >+		for (i = 0; i < num_modes; i++) {
> >+			int mode;
> >+			int val;
> >+
> >+			of_property_read_u32_index(child,
> >+						   "mode-val", 2 * i, &mode);
> >+			of_property_read_u32_index(child,
> >+						   "mode-val", 2 * i + 1, &val);
> >+			if (mode >= NETXBIG_LED_MODE_NUM)
> >+				return -EINVAL;
> >+			mode_val[mode] = val;
> >+		}
> >+		led->mode_val = mode_val;
> >+
> >+		if (!of_property_read_string(child, "label", &string))
> >+			led->name = string;
> >+		else
> >+			led->name = child->name;
> >+
> >+		if (!of_property_read_string(child,
> >+					     "linux,default-trigger", &string))
> >+			led->default_trigger = string;
> >+
> >+		led++;
> >+	}
> >+
> >+	pdata->leds = leds;
> >+	pdata->num_leds = num_leds;
> >+
> >+	return 0;
> >+}
> >+
> >+static const struct of_device_id of_netxbig_leds_match[] = {
> >+	{ .compatible = "lacie,netxbig-leds", },
> >+	{},
> >+};
> >+#else
> >+static int netxbig_leds_get_of_pdata(struct device *dev,
> >+				     struct netxbig_led_platform_data *pdata)
> 
> s/static int/static inline int/

Is that not already the case with modern compiler ?

I'll add it anyway.

Thanks,

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4 1/3] leds: netxbig: add device tree binding

[Jacek Anaszewski]

On 09/18/2015 12:30 PM, Simon Guinot wrote:
> On Fri, Sep 18, 2015 at 11:16:41AM +0200, Jacek Anaszewski wrote:
>> Hi Simon,
>>
>> Please find my comments in the code below.
>
> Hi Jacek,
>
> Thanks for the quick review.

You're welcome.

>>
>> On 09/17/2015 05:59 PM, Simon Guinot wrote:
>>> This patch adds device tree support for the netxbig LEDs.
>>>
>>> This also introduces a additionnal DT binding for the GPIO extension bus
>>> (netxbig-gpio-ext) used to configure the LEDs. Since this bus could also
>>> be used to control other devices, then it seems more suitable to have it
>>> in a separate DT binding.
>>>
>>> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
>>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>>> ---
>>>   .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
>>>   .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++++
>>>   drivers/leds/leds-netxbig.c                        | 258 +++++++++++++++++++--
>>>   include/dt-bindings/leds/leds-netxbig.h            |  18 ++
>>>   4 files changed, 369 insertions(+), 21 deletions(-)
>>>   create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
>>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt
>>>   create mode 100644 include/dt-bindings/leds/leds-netxbig.h
>>>
>>> Changes for v2:
>>> - Check timer mode value retrieved from DT.
>>> - In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
>>>    timer delay values from DT with function of_property_read_u32_index.
>>>    Instead, use a temporary u32 variable. This allows to silence a static
>>>    checker warning.
>>> - Make timer property optional in the binding documentation. It is now
>>>    aligned with the driver code.
>>>
>>> Changes for v3:
>>> - Fix pointer usage with the temporary u32 variable while calling
>>>    of_property_read_u32_index.
>>>
>>> Changes for v4:
>>> - In DT binding document netxbig-gpio-ext.txt, detail byte order for
>>>    registers and latch mechanism for "enable-gpio".
>>> - In leds-netxbig.c, add some error messages.
>>> - In leds-netxbig.c, fix some "sizeof" style issues.
>>> - In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
>>>    of_property_read_string() calls after the error-prone checks.
>>> - Add some Acked-by.
>
> ...
>
>>> +#ifdef CONFIG_OF_GPIO
>>> +static int gpio_ext_get_of_pdata(struct device *dev, struct device_node *np,
>>> +				 struct netxbig_gpio_ext *gpio_ext)
>>> +{
>>> +	int *addr, *data;
>>> +	int num_addr, num_data;
>>> +	int ret;
>>> +	int i;
>>> +
>>> +	ret = of_gpio_named_count(np, "addr-gpios");
>>> +	if (ret < 0) {
>>> +		dev_err(dev,
>>> +			"Failed to count GPIOs in DT property addr-gpios\n");
>>> +		return ret;
>>> +	}
>>> +	num_addr = ret;
>>> +	addr = devm_kzalloc(dev, num_addr * sizeof(*addr), GFP_KERNEL);
>>> +	if (!addr)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < num_addr; i++) {
>>> +		ret = of_get_named_gpio(np, "addr-gpios", i);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		addr[i] = ret;
>>> +	}
>>> +	gpio_ext->addr = addr;
>>> +	gpio_ext->num_addr = num_addr;
>>> +
>>> +	ret = of_gpio_named_count(np, "data-gpios");
>>> +	if (ret < 0) {
>>> +		dev_err(dev,
>>> +			"Failed to count GPIOs in DT property data-gpios\n");
>>> +		return ret;
>>> +	}
>>> +	num_data = ret;
>>> +	data = devm_kzalloc(dev, num_data * sizeof(*data), GFP_KERNEL);
>>> +	if (!data)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < num_data; i++) {
>>> +		ret = of_get_named_gpio(np, "data-gpios", i);
>>> +		if (ret < 0)
>>> +			return ret;
>>> +		data[i] = ret;
>>> +	}
>>> +	gpio_ext->data = data;
>>> +	gpio_ext->num_data = num_data;
>>> +
>>> +	ret = of_get_named_gpio(np, "enable-gpio", 0);
>>> +	if (ret < 0) {
>>> +		dev_err(dev,
>>> +			"Failed to get GPIO from DT property enable-gpio\n");
>>> +		return ret;
>>> +	}
>>> +	gpio_ext->enable = ret;
>>> +
>>> +	return 0;
>>> +}
>>
>> Since netxbig-gpio-ext extension bus can be also used to control other
>> device, and the related DT bindings are meant for GPIO subsystem, then
>> the above parser should also be placed there to make it available
>> for reuse by other drivers. The gpio_ext_init function should be also
>> moved to GPIO subsystem, presumably to the same module.
>
> Yes, this patch series is a first step. I am planning to move the
> netxbig-gpio-ext extension bus into a dedicated driver as soon as I need
> need it for an another device/driver. But for the time being, I was
> planning to keep it here. If you agree with that of course.

ack.

>>
>> Moreover, if you switched to using devm* prefixed version of
>> gpio_request_one and led_classdev_reqister, you could simplify
>> the error paths in the driver.
>
> Yes, I have a pending patch for this conversion. But since it is not
> really related with the subject of this patch series (add DT support),
> I was planning to send it next.
>
> Do you want me to include this patch into this series.

Why not, if you have it ready to go. If it needs some polishing,
we can live with what we have now.

>>
>>> +static int netxbig_leds_get_of_pdata(struct device *dev,
>>> +				     struct netxbig_led_platform_data *pdata)
>>> +{
>>> +	struct device_node *np = dev->of_node;
>>> +	struct device_node *gpio_ext_np;
>>> +	struct device_node *child;
>>> +	struct netxbig_gpio_ext *gpio_ext;
>>> +	struct netxbig_led_timer *timers;
>>> +	struct netxbig_led *leds, *led;
>>> +	int num_timers;
>>> +	int num_leds = 0;
>>> +	int ret;
>>> +	int i;
>
> ...
>
>>> +	led = leds;
>>> +	for_each_child_of_node(np, child) {
>>> +		const char *string;
>>> +		int *mode_val;
>>> +		int num_modes;
>>> +
>>> +		if (of_property_read_u32(child, "mode-addr",
>>> +					 &led->mode_addr))
>>> +			return -EINVAL;
>>> +
>>> +		if (of_property_read_u32(child, "bright-addr",
>>> +					 &led->bright_addr))
>>> +			return -EINVAL;
>>
>> You don't parse bright-max DT property anywhere, AFAICT.
>
> Yes I don't. I have added this bright-max property thinking ahead of
> moving the netxbig-gpio-ext stuff into a dedicated driver. For now, it
> is possible to compute bright-max from the number of data GPIOs of the
> extension bus. But once it will be removed something else will be
> needed. That's why I introduced this property.
>
> But now, I am thinking I should have used this property right now. It
> will be more convenient when moving the netxbig-gpio-ext code out.
>
> I'll do that for the next round.

Thanks.

>>
>>> +		mode_val =
>>> +			devm_kzalloc(dev,
>>> +				     NETXBIG_LED_MODE_NUM * sizeof(*mode_val),
>>> +				     GFP_KERNEL);
>>> +		if (!mode_val)
>>> +			return -ENOMEM;
>>> +
>>> +		for (i = 0; i < NETXBIG_LED_MODE_NUM; i++)
>>> +			mode_val[i] = NETXBIG_LED_INVALID_MODE;
>>> +
>>> +		ret = of_property_count_u32_elems(child, "mode-val");
>>> +		if (ret < 0 || ret % 2)
>>> +			return -EINVAL;
>>> +		num_modes = ret / 2;
>>> +		if (num_modes > NETXBIG_LED_MODE_NUM)
>>> +			return -EINVAL;
>>> +
>>> +		for (i = 0; i < num_modes; i++) {
>>> +			int mode;
>>> +			int val;
>>> +
>>> +			of_property_read_u32_index(child,
>>> +						   "mode-val", 2 * i, &mode);
>>> +			of_property_read_u32_index(child,
>>> +						   "mode-val", 2 * i + 1, &val);
>>> +			if (mode >= NETXBIG_LED_MODE_NUM)
>>> +				return -EINVAL;
>>> +			mode_val[mode] = val;
>>> +		}
>>> +		led->mode_val = mode_val;
>>> +
>>> +		if (!of_property_read_string(child, "label", &string))
>>> +			led->name = string;
>>> +		else
>>> +			led->name = child->name;
>>> +
>>> +		if (!of_property_read_string(child,
>>> +					     "linux,default-trigger", &string))
>>> +			led->default_trigger = string;
>>> +
>>> +		led++;
>>> +	}
>>> +
>>> +	pdata->leds = leds;
>>> +	pdata->num_leds = num_leds;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id of_netxbig_leds_match[] = {
>>> +	{ .compatible = "lacie,netxbig-leds", },
>>> +	{},
>>> +};
>>> +#else
>>> +static int netxbig_leds_get_of_pdata(struct device *dev,
>>> +				     struct netxbig_led_platform_data *pdata)
>>
>> s/static int/static inline int/
>
> Is that not already the case with modern compiler ?

Could you elaborate on this?

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v4 1/3] leds: netxbig: add device tree binding

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1388 bytes --]

On Fri, Sep 18, 2015 at 12:49:28PM +0200, Jacek Anaszewski wrote:
> >>
> >>Moreover, if you switched to using devm* prefixed version of
> >>gpio_request_one and led_classdev_reqister, you could simplify
> >>the error paths in the driver.
> >
> >Yes, I have a pending patch for this conversion. But since it is not
> >really related with the subject of this patch series (add DT support),
> >I was planning to send it next.
> >
> >Do you want me to include this patch into this series.
> 
> Why not, if you have it ready to go. If it needs some polishing,
> we can live with what we have now.

No, the patch is ready. I'll add it.

> >>>+static const struct of_device_id of_netxbig_leds_match[] = {
> >>>+	{ .compatible = "lacie,netxbig-leds", },
> >>>+	{},
> >>>+};
> >>>+#else
> >>>+static int netxbig_leds_get_of_pdata(struct device *dev,
> >>>+				     struct netxbig_led_platform_data *pdata)
> >>
> >>s/static int/static inline int/
> >
> >Is that not already the case with modern compiler ?
> 
> Could you elaborate on this?

There is not much to say. netxbig_leds_get_of_pdata is a single line of
code. I am not a gcc expert but I am quite confident that this function
will be inlined anyway (given the optimisation level used to compile the
Linux kernel). That's it.

But there is nothing wrong by adding "inline" neither.

Thanks,

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4 0/3] Add DT support for netxbig LEDs

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 1868 bytes --]

On Fri, Sep 18, 2015 at 11:15:45AM +0200, Jacek Anaszewski wrote:
> Hi Simon,
> 
> Thanks for the update.
> 
> On 09/17/2015 05:59 PM, Simon Guinot wrote:
> >Hello,
> >
> >This patch series adds DT support for the LEDs found on the Kirkwood-based
> >LaCie boards 2Big and 5Big Network v2.
> >
> >Changes for v2:
> >- Check timer mode value retrieved from DT.
> >- In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
> >   timer delay values from DT with function of_property_read_u32_index.
> >   Instead, use a temporary u32 variable. This allows to silence a static
> >   checker warning.
> >- Make timer property optional in the binding documentation. It is now
> >   aligned with the driver code.
> >
> >Changes for v3:
> >- Fix pointer usage with the temporary u32 variable while calling
> >   of_property_read_u32_index.
> >
> >Changes for v4:
> >- In DT binding document netxbig-gpio-ext.txt, detail byte order for
> >   registers and latch mechanism for "enable-gpio".
> >- In leds-netxbig.c, add some error messages.
> >- In leds-netxbig.c, fix some "sizeof" style issues.
> >- In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
> >   of_property_read_string() calls after the error-prone checks.
> >- Add some Acked-by.
> >
> >Jacek,
> >
> >I did not convert the bright-max DT property into led-max-microamp.
> >The reason is that I can't translate the bright-max values into
> >microamperes.
> 
> Doesn't specification of your device say what current value given
> brightness level reflects?

I double-checked and I can confirm that I don't have the current values
for the LEDs. Although this feature is nice to have, it has not been
used by the LaCie stock firmware. LEDs are only enabled at their maximum
level. I believe it is the reason why it has not been specified...

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4 1/3] leds: netxbig: add device tree binding

[Jacek Anaszewski]

On 09/18/2015 03:10 PM, Simon Guinot wrote:
> On Fri, Sep 18, 2015 at 12:49:28PM +0200, Jacek Anaszewski wrote:
>>>>
>>>> Moreover, if you switched to using devm* prefixed version of
>>>> gpio_request_one and led_classdev_reqister, you could simplify
>>>> the error paths in the driver.
>>>
>>> Yes, I have a pending patch for this conversion. But since it is not
>>> really related with the subject of this patch series (add DT support),
>>> I was planning to send it next.
>>>
>>> Do you want me to include this patch into this series.
>>
>> Why not, if you have it ready to go. If it needs some polishing,
>> we can live with what we have now.
>
> No, the patch is ready. I'll add it.
>
>>>>> +static const struct of_device_id of_netxbig_leds_match[] = {
>>>>> +	{ .compatible = "lacie,netxbig-leds", },
>>>>> +	{},
>>>>> +};
>>>>> +#else
>>>>> +static int netxbig_leds_get_of_pdata(struct device *dev,
>>>>> +				     struct netxbig_led_platform_data *pdata)
>>>>
>>>> s/static int/static inline int/
>>>
>>> Is that not already the case with modern compiler ?
>>
>> Could you elaborate on this?
>
> There is not much to say. netxbig_leds_get_of_pdata is a single line of
> code. I am not a gcc expert but I am quite confident that this function
> will be inlined anyway (given the optimisation level used to compile the
> Linux kernel). That's it.
>
> But there is nothing wrong by adding "inline" neither.

Right. All the more, this is a common pattern for no-ops, also in *.c
files.


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v4 0/3] Add DT support for netxbig LEDs

[Jacek Anaszewski]

On 09/18/2015 03:23 PM, Simon Guinot wrote:
> On Fri, Sep 18, 2015 at 11:15:45AM +0200, Jacek Anaszewski wrote:
>> Hi Simon,
>>
>> Thanks for the update.
>>
>> On 09/17/2015 05:59 PM, Simon Guinot wrote:
>>> Hello,
>>>
>>> This patch series adds DT support for the LEDs found on the Kirkwood-based
>>> LaCie boards 2Big and 5Big Network v2.
>>>
>>> Changes for v2:
>>> - Check timer mode value retrieved from DT.
>>> - In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
>>>    timer delay values from DT with function of_property_read_u32_index.
>>>    Instead, use a temporary u32 variable. This allows to silence a static
>>>    checker warning.
>>> - Make timer property optional in the binding documentation. It is now
>>>    aligned with the driver code.
>>>
>>> Changes for v3:
>>> - Fix pointer usage with the temporary u32 variable while calling
>>>    of_property_read_u32_index.
>>>
>>> Changes for v4:
>>> - In DT binding document netxbig-gpio-ext.txt, detail byte order for
>>>    registers and latch mechanism for "enable-gpio".
>>> - In leds-netxbig.c, add some error messages.
>>> - In leds-netxbig.c, fix some "sizeof" style issues.
>>> - In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
>>>    of_property_read_string() calls after the error-prone checks.
>>> - Add some Acked-by.
>>>
>>> Jacek,
>>>
>>> I did not convert the bright-max DT property into led-max-microamp.
>>> The reason is that I can't translate the bright-max values into
>>> microamperes.
>>
>> Doesn't specification of your device say what current value given
>> brightness level reflects?
>
> I double-checked and I can confirm that I don't have the current values
> for the LEDs. Although this feature is nice to have, it has not been
> used by the LaCie stock firmware. LEDs are only enabled at their maximum
> level. I believe it is the reason why it has not been specified...

OK, so we have to use levels. Could you rename bright-max to
max-brightness? We have already this in leds-pwm bindings and
probably we should make it a common optional DT property for all LEDs.
I'll add this to my TODO list.

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v4 1/3] leds: netxbig: add device tree binding

[Rob Herring]

On 09/17/2015 10:59 AM, Simon Guinot wrote:
> This patch adds device tree support for the netxbig LEDs.
> 
> This also introduces a additionnal DT binding for the GPIO extension bus
> (netxbig-gpio-ext) used to configure the LEDs. Since this bus could also
> be used to control other devices, then it seems more suitable to have it
> in a separate DT binding.
> 
> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
>  .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++++
>  drivers/leds/leds-netxbig.c                        | 258 +++++++++++++++++++--
>  include/dt-bindings/leds/leds-netxbig.h            |  18 ++
>  4 files changed, 369 insertions(+), 21 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt
>  create mode 100644 include/dt-bindings/leds/leds-netxbig.h
> 
> Changes for v2:
> - Check timer mode value retrieved from DT.
> - In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
>   timer delay values from DT with function of_property_read_u32_index.
>   Instead, use a temporary u32 variable. This allows to silence a static
>   checker warning.
> - Make timer property optional in the binding documentation. It is now
>   aligned with the driver code.
> 
> Changes for v3:
> - Fix pointer usage with the temporary u32 variable while calling
>   of_property_read_u32_index.
> 
> Changes for v4:
> - In DT binding document netxbig-gpio-ext.txt, detail byte order for
>   registers and latch mechanism for "enable-gpio".
> - In leds-netxbig.c, add some error messages.
> - In leds-netxbig.c, fix some "sizeof" style issues.
> - In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
>   of_property_read_string() calls after the error-prone checks.
> - Add some Acked-by.
> 
> diff --git a/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> new file mode 100644
> index 000000000000..50ec2e690701
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> @@ -0,0 +1,22 @@
> +Binding for the GPIO extension bus found on some LaCie/Seagate boards
> +(Example: 2Big/5Big Network v2, 2Big NAS).
> +
> +Required properties:
> +- compatible: "lacie,netxbig-gpio-ext".
> +- addr-gpios: GPIOs representing the address register (LSB -> MSB).
> +- data-gpios: GPIOs representing the data register (LSB -> MSB).
> +- enable-gpio: latches the new configuration (address, data) on raising edge.
> +
> +Example:
> +
> +netxbig_gpio_ext: netxbig-gpio-ext {
> +	compatible = "lacie,netxbig-gpio-ext";
> +
> +	addr-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH
> +		      &gpio1 16 GPIO_ACTIVE_HIGH
> +		      &gpio1 17 GPIO_ACTIVE_HIGH>;
> +	data-gpios = <&gpio1 12 GPIO_ACTIVE_HIGH
> +		      &gpio1 13 GPIO_ACTIVE_HIGH
> +		      &gpio1 14 GPIO_ACTIVE_HIGH>;
> +	enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
> +};
> diff --git a/Documentation/devicetree/bindings/leds/leds-netxbig.txt b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
> new file mode 100644
> index 000000000000..efadbecbfeb9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
> @@ -0,0 +1,92 @@
> +Binding for the CPLD LEDs (GPIO extension bus) found on some LaCie/Seagate
> +boards (Example: 2Big/5Big Network v2, 2Big NAS).
> +
> +Required properties:
> +- compatible: "lacie,netxbig-leds".
> +- gpio-ext: Phandle for the gpio-ext bus.

Would being a subnode of gpio-ext work instead?

> +
> +Optional properties:
> +- timers: Timer array. Each timer entry is represented by three integers:
> +  Mode (gpio-ext bus), delay_on and delay_off.
> +
> +Each LED is represented as a sub-node of the netxbig-leds device.
> +
> +Required sub-node properties:
> +- mode-addr: Mode register address on gpio-ext bus.
> +- mode-val: Mode to value mapping. Each entry is represented by two integers:
> +  A mode and the corresponding value on the gpio-ext bus.

I somewhat wonder if this is too low level and should just be in the
driver instead. I guess with only 3 addr and data lines, it is okay. It
wouldn't scale to a large number of registers though.

> +- bright-addr: Brightness register address on gpio-ext bus.
> +- bright-max: Maximum brightness value.

We already have a somewhat standard max-brightness property.

Re: [PATCH v4 1/3] leds: netxbig: add device tree binding

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 5774 bytes --]

On Mon, Sep 21, 2015 at 10:38:45AM -0500, Rob Herring wrote:

Hi Rob,

Thanks for your feedback. Please, see my answers below.

> On 09/17/2015 10:59 AM, Simon Guinot wrote:
> > This patch adds device tree support for the netxbig LEDs.
> > 
> > This also introduces a additionnal DT binding for the GPIO extension bus
> > (netxbig-gpio-ext) used to configure the LEDs. Since this bus could also
> > be used to control other devices, then it seems more suitable to have it
> > in a separate DT binding.
> > 
> > Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> >  .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
> >  .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++++
> >  drivers/leds/leds-netxbig.c                        | 258 +++++++++++++++++++--
> >  include/dt-bindings/leds/leds-netxbig.h            |  18 ++
> >  4 files changed, 369 insertions(+), 21 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> >  create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt
> >  create mode 100644 include/dt-bindings/leds/leds-netxbig.h
> > 
> > Changes for v2:
> > - Check timer mode value retrieved from DT.
> > - In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
> >   timer delay values from DT with function of_property_read_u32_index.
> >   Instead, use a temporary u32 variable. This allows to silence a static
> >   checker warning.
> > - Make timer property optional in the binding documentation. It is now
> >   aligned with the driver code.
> > 
> > Changes for v3:
> > - Fix pointer usage with the temporary u32 variable while calling
> >   of_property_read_u32_index.
> > 
> > Changes for v4:
> > - In DT binding document netxbig-gpio-ext.txt, detail byte order for
> >   registers and latch mechanism for "enable-gpio".
> > - In leds-netxbig.c, add some error messages.
> > - In leds-netxbig.c, fix some "sizeof" style issues.
> > - In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
> >   of_property_read_string() calls after the error-prone checks.
> > - Add some Acked-by.
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> > new file mode 100644
> > index 000000000000..50ec2e690701
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> > @@ -0,0 +1,22 @@
> > +Binding for the GPIO extension bus found on some LaCie/Seagate boards
> > +(Example: 2Big/5Big Network v2, 2Big NAS).
> > +
> > +Required properties:
> > +- compatible: "lacie,netxbig-gpio-ext".
> > +- addr-gpios: GPIOs representing the address register (LSB -> MSB).
> > +- data-gpios: GPIOs representing the data register (LSB -> MSB).
> > +- enable-gpio: latches the new configuration (address, data) on raising edge.
> > +
> > +Example:
> > +
> > +netxbig_gpio_ext: netxbig-gpio-ext {
> > +	compatible = "lacie,netxbig-gpio-ext";
> > +
> > +	addr-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH
> > +		      &gpio1 16 GPIO_ACTIVE_HIGH
> > +		      &gpio1 17 GPIO_ACTIVE_HIGH>;
> > +	data-gpios = <&gpio1 12 GPIO_ACTIVE_HIGH
> > +		      &gpio1 13 GPIO_ACTIVE_HIGH
> > +		      &gpio1 14 GPIO_ACTIVE_HIGH>;
> > +	enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
> > +};
> > diff --git a/Documentation/devicetree/bindings/leds/leds-netxbig.txt b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
> > new file mode 100644
> > index 000000000000..efadbecbfeb9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
> > @@ -0,0 +1,92 @@
> > +Binding for the CPLD LEDs (GPIO extension bus) found on some LaCie/Seagate
> > +boards (Example: 2Big/5Big Network v2, 2Big NAS).
> > +
> > +Required properties:
> > +- compatible: "lacie,netxbig-leds".
> > +- gpio-ext: Phandle for the gpio-ext bus.
> 
> Would being a subnode of gpio-ext work instead?

Well, it is really unclear to me. I think that the GPIO extension bus
can be seen as a "kind of" GPIO chip. And devices which are using a GPIO
resource are not represented in DT as sub-nodes of the GPIO node. That's
why I chose to use a phandle here. 

Let me know if this representation is not correct.

> 
> > +
> > +Optional properties:
> > +- timers: Timer array. Each timer entry is represented by three integers:
> > +  Mode (gpio-ext bus), delay_on and delay_off.
> > +
> > +Each LED is represented as a sub-node of the netxbig-leds device.
> > +
> > +Required sub-node properties:
> > +- mode-addr: Mode register address on gpio-ext bus.
> > +- mode-val: Mode to value mapping. Each entry is represented by two integers:
> > +  A mode and the corresponding value on the gpio-ext bus.
> 
> I somewhat wonder if this is too low level and should just be in the
> driver instead. I guess with only 3 addr and data lines, it is okay. It
> wouldn't scale to a large number of registers though.

Even if it is not the case now, note that this values could be different
for an another board. For example, we already have some x86-based boards
(still no mainlined yet) which are using the leds-netxbig driver but
with a different gpio-ext configuration. This could happen with some
ARM-based boards too.

That's why I think it is better to have this properties in the DT rather
than hardcoded values in the driver.

> 
> > +- bright-addr: Brightness register address on gpio-ext bus.
> > +- bright-max: Maximum brightness value.
> 
> We already have a somewhat standard max-brightness property.

Yes, Jacek mentioned that too. It made the change in the v5.

Thanks,

Simon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v4 1/3] leds: netxbig: add device tree binding

[Jacek Anaszewski]

Hi Rob.

If you are satisfied with Simon's explanation, could you confirm that 
with your ack, please?

Best Regards,
Jacek Anaszewski

On 09/22/2015 11:30 AM, Simon Guinot wrote:
> On Mon, Sep 21, 2015 at 10:38:45AM -0500, Rob Herring wrote:
>
> Hi Rob,
>
> Thanks for your feedback. Please, see my answers below.
>
>> On 09/17/2015 10:59 AM, Simon Guinot wrote:
>>> This patch adds device tree support for the netxbig LEDs.
>>>
>>> This also introduces a additionnal DT binding for the GPIO extension bus
>>> (netxbig-gpio-ext) used to configure the LEDs. Since this bus could also
>>> be used to control other devices, then it seems more suitable to have it
>>> in a separate DT binding.
>>>
>>> Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
>>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>>> ---
>>>   .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
>>>   .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++++
>>>   drivers/leds/leds-netxbig.c                        | 258 +++++++++++++++++++--
>>>   include/dt-bindings/leds/leds-netxbig.h            |  18 ++
>>>   4 files changed, 369 insertions(+), 21 deletions(-)
>>>   create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
>>>   create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt
>>>   create mode 100644 include/dt-bindings/leds/leds-netxbig.h
>>>
>>> Changes for v2:
>>> - Check timer mode value retrieved from DT.
>>> - In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
>>>    timer delay values from DT with function of_property_read_u32_index.
>>>    Instead, use a temporary u32 variable. This allows to silence a static
>>>    checker warning.
>>> - Make timer property optional in the binding documentation. It is now
>>>    aligned with the driver code.
>>>
>>> Changes for v3:
>>> - Fix pointer usage with the temporary u32 variable while calling
>>>    of_property_read_u32_index.
>>>
>>> Changes for v4:
>>> - In DT binding document netxbig-gpio-ext.txt, detail byte order for
>>>    registers and latch mechanism for "enable-gpio".
>>> - In leds-netxbig.c, add some error messages.
>>> - In leds-netxbig.c, fix some "sizeof" style issues.
>>> - In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
>>>    of_property_read_string() calls after the error-prone checks.
>>> - Add some Acked-by.
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
>>> new file mode 100644
>>> index 000000000000..50ec2e690701
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
>>> @@ -0,0 +1,22 @@
>>> +Binding for the GPIO extension bus found on some LaCie/Seagate boards
>>> +(Example: 2Big/5Big Network v2, 2Big NAS).
>>> +
>>> +Required properties:
>>> +- compatible: "lacie,netxbig-gpio-ext".
>>> +- addr-gpios: GPIOs representing the address register (LSB -> MSB).
>>> +- data-gpios: GPIOs representing the data register (LSB -> MSB).
>>> +- enable-gpio: latches the new configuration (address, data) on raising edge.
>>> +
>>> +Example:
>>> +
>>> +netxbig_gpio_ext: netxbig-gpio-ext {
>>> +	compatible = "lacie,netxbig-gpio-ext";
>>> +
>>> +	addr-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH
>>> +		      &gpio1 16 GPIO_ACTIVE_HIGH
>>> +		      &gpio1 17 GPIO_ACTIVE_HIGH>;
>>> +	data-gpios = <&gpio1 12 GPIO_ACTIVE_HIGH
>>> +		      &gpio1 13 GPIO_ACTIVE_HIGH
>>> +		      &gpio1 14 GPIO_ACTIVE_HIGH>;
>>> +	enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
>>> +};
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-netxbig.txt b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
>>> new file mode 100644
>>> index 000000000000..efadbecbfeb9
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
>>> @@ -0,0 +1,92 @@
>>> +Binding for the CPLD LEDs (GPIO extension bus) found on some LaCie/Seagate
>>> +boards (Example: 2Big/5Big Network v2, 2Big NAS).
>>> +
>>> +Required properties:
>>> +- compatible: "lacie,netxbig-leds".
>>> +- gpio-ext: Phandle for the gpio-ext bus.
>>
>> Would being a subnode of gpio-ext work instead?
>
> Well, it is really unclear to me. I think that the GPIO extension bus
> can be seen as a "kind of" GPIO chip. And devices which are using a GPIO
> resource are not represented in DT as sub-nodes of the GPIO node. That's
> why I chose to use a phandle here.
>
> Let me know if this representation is not correct.
>
>>
>>> +
>>> +Optional properties:
>>> +- timers: Timer array. Each timer entry is represented by three integers:
>>> +  Mode (gpio-ext bus), delay_on and delay_off.
>>> +
>>> +Each LED is represented as a sub-node of the netxbig-leds device.
>>> +
>>> +Required sub-node properties:
>>> +- mode-addr: Mode register address on gpio-ext bus.
>>> +- mode-val: Mode to value mapping. Each entry is represented by two integers:
>>> +  A mode and the corresponding value on the gpio-ext bus.
>>
>> I somewhat wonder if this is too low level and should just be in the
>> driver instead. I guess with only 3 addr and data lines, it is okay. It
>> wouldn't scale to a large number of registers though.
>
> Even if it is not the case now, note that this values could be different
> for an another board. For example, we already have some x86-based boards
> (still no mainlined yet) which are using the leds-netxbig driver but
> with a different gpio-ext configuration. This could happen with some
> ARM-based boards too.
>
> That's why I think it is better to have this properties in the DT rather
> than hardcoded values in the driver.
>
>>
>>> +- bright-addr: Brightness register address on gpio-ext bus.
>>> +- bright-max: Maximum brightness value.
>>
>> We already have a somewhat standard max-brightness property.
>
> Yes, Jacek mentioned that too. It made the change in the v5.
>
> Thanks,
>
> Simon
>

Re: [PATCH v4 1/3] leds: netxbig: add device tree binding

[Simon Guinot]

[-- Attachment #1: Type: text/plain, Size: 6506 bytes --]

On Mon, Sep 28, 2015 at 09:58:44AM +0200, Jacek Anaszewski wrote:
> Hi Rob.
> 
> If you are satisfied with Simon's explanation, could you confirm
> that with your ack, please?

Hi Rob,

Please let us know your answer.

Thanks in advance.

Simon

> On 09/22/2015 11:30 AM, Simon Guinot wrote:
> >On Mon, Sep 21, 2015 at 10:38:45AM -0500, Rob Herring wrote:
> >
> >Hi Rob,
> >
> >Thanks for your feedback. Please, see my answers below.
> >
> >>On 09/17/2015 10:59 AM, Simon Guinot wrote:
> >>>This patch adds device tree support for the netxbig LEDs.
> >>>
> >>>This also introduces a additionnal DT binding for the GPIO extension bus
> >>>(netxbig-gpio-ext) used to configure the LEDs. Since this bus could also
> >>>be used to control other devices, then it seems more suitable to have it
> >>>in a separate DT binding.
> >>>
> >>>Signed-off-by: Simon Guinot <simon.guinot@sequanux.org>
> >>>Acked-by: Linus Walleij <linus.walleij@linaro.org>
> >>>---
> >>>  .../devicetree/bindings/gpio/netxbig-gpio-ext.txt  |  22 ++
> >>>  .../devicetree/bindings/leds/leds-netxbig.txt      |  92 ++++++++
> >>>  drivers/leds/leds-netxbig.c                        | 258 +++++++++++++++++++--
> >>>  include/dt-bindings/leds/leds-netxbig.h            |  18 ++
> >>>  4 files changed, 369 insertions(+), 21 deletions(-)
> >>>  create mode 100644 Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> >>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-netxbig.txt
> >>>  create mode 100644 include/dt-bindings/leds/leds-netxbig.h
> >>>
> >>>Changes for v2:
> >>>- Check timer mode value retrieved from DT.
> >>>- In netxbig_leds_get_of_pdata, don't use unsigned long variables to get
> >>>   timer delay values from DT with function of_property_read_u32_index.
> >>>   Instead, use a temporary u32 variable. This allows to silence a static
> >>>   checker warning.
> >>>- Make timer property optional in the binding documentation. It is now
> >>>   aligned with the driver code.
> >>>
> >>>Changes for v3:
> >>>- Fix pointer usage with the temporary u32 variable while calling
> >>>   of_property_read_u32_index.
> >>>
> >>>Changes for v4:
> >>>- In DT binding document netxbig-gpio-ext.txt, detail byte order for
> >>>   registers and latch mechanism for "enable-gpio".
> >>>- In leds-netxbig.c, add some error messages.
> >>>- In leds-netxbig.c, fix some "sizeof" style issues.
> >>>- In leds-netxbig.c, in netxbig_leds_get_of_pdata(), move the
> >>>   of_property_read_string() calls after the error-prone checks.
> >>>- Add some Acked-by.
> >>>
> >>>diff --git a/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> >>>new file mode 100644
> >>>index 000000000000..50ec2e690701
> >>>--- /dev/null
> >>>+++ b/Documentation/devicetree/bindings/gpio/netxbig-gpio-ext.txt
> >>>@@ -0,0 +1,22 @@
> >>>+Binding for the GPIO extension bus found on some LaCie/Seagate boards
> >>>+(Example: 2Big/5Big Network v2, 2Big NAS).
> >>>+
> >>>+Required properties:
> >>>+- compatible: "lacie,netxbig-gpio-ext".
> >>>+- addr-gpios: GPIOs representing the address register (LSB -> MSB).
> >>>+- data-gpios: GPIOs representing the data register (LSB -> MSB).
> >>>+- enable-gpio: latches the new configuration (address, data) on raising edge.
> >>>+
> >>>+Example:
> >>>+
> >>>+netxbig_gpio_ext: netxbig-gpio-ext {
> >>>+	compatible = "lacie,netxbig-gpio-ext";
> >>>+
> >>>+	addr-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH
> >>>+		      &gpio1 16 GPIO_ACTIVE_HIGH
> >>>+		      &gpio1 17 GPIO_ACTIVE_HIGH>;
> >>>+	data-gpios = <&gpio1 12 GPIO_ACTIVE_HIGH
> >>>+		      &gpio1 13 GPIO_ACTIVE_HIGH
> >>>+		      &gpio1 14 GPIO_ACTIVE_HIGH>;
> >>>+	enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
> >>>+};
> >>>diff --git a/Documentation/devicetree/bindings/leds/leds-netxbig.txt b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
> >>>new file mode 100644
> >>>index 000000000000..efadbecbfeb9
> >>>--- /dev/null
> >>>+++ b/Documentation/devicetree/bindings/leds/leds-netxbig.txt
> >>>@@ -0,0 +1,92 @@
> >>>+Binding for the CPLD LEDs (GPIO extension bus) found on some LaCie/Seagate
> >>>+boards (Example: 2Big/5Big Network v2, 2Big NAS).
> >>>+
> >>>+Required properties:
> >>>+- compatible: "lacie,netxbig-leds".
> >>>+- gpio-ext: Phandle for the gpio-ext bus.
> >>
> >>Would being a subnode of gpio-ext work instead?
> >
> >Well, it is really unclear to me. I think that the GPIO extension bus
> >can be seen as a "kind of" GPIO chip. And devices which are using a GPIO
> >resource are not represented in DT as sub-nodes of the GPIO node. That's
> >why I chose to use a phandle here.
> >
> >Let me know if this representation is not correct.
> >
> >>
> >>>+
> >>>+Optional properties:
> >>>+- timers: Timer array. Each timer entry is represented by three integers:
> >>>+  Mode (gpio-ext bus), delay_on and delay_off.
> >>>+
> >>>+Each LED is represented as a sub-node of the netxbig-leds device.
> >>>+
> >>>+Required sub-node properties:
> >>>+- mode-addr: Mode register address on gpio-ext bus.
> >>>+- mode-val: Mode to value mapping. Each entry is represented by two integers:
> >>>+  A mode and the corresponding value on the gpio-ext bus.
> >>
> >>I somewhat wonder if this is too low level and should just be in the
> >>driver instead. I guess with only 3 addr and data lines, it is okay. It
> >>wouldn't scale to a large number of registers though.
> >
> >Even if it is not the case now, note that this values could be different
> >for an another board. For example, we already have some x86-based boards
> >(still no mainlined yet) which are using the leds-netxbig driver but
> >with a different gpio-ext configuration. This could happen with some
> >ARM-based boards too.
> >
> >That's why I think it is better to have this properties in the DT rather
> >than hardcoded values in the driver.
> >
> >>
> >>>+- bright-addr: Brightness register address on gpio-ext bus.
> >>>+- bright-max: Maximum brightness value.
> >>
> >>We already have a somewhat standard max-brightness property.
> >
> >Yes, Jacek mentioned that too. It made the change in the v5.
> >
> >Thanks,
> >
> >Simon
> >
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]