* [PATCH v3 0/3] Add DT support for netxbig LEDs
@ 2015-06-18 14:59 ` Simon Guinot
0 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-18 14:59 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
This patch series adds DT support for the LEDs found on the Kirkwood-based
LaCie boards 2Big and 5Big Network v2.
Changes since v1:
- 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 since v2:
- Fix pointer usage with the temporary u32 variable while calling
of_property_read_u32_index.
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 | 250 +++++++++++++++++++--
include/dt-bindings/leds/leds-netxbig.h | 18 ++
11 files changed, 501 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 0/3] Add DT support for netxbig LEDs
@ 2015-06-18 14:59 ` Simon Guinot
0 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-18 14:59 UTC (permalink / raw)
To: Bryan Wu, Richard Purdie, Jason Cooper, Andrew Lunn,
Gregory Clement, Sebastian Hesselbarth
Cc: linux-leds, linux-arm-kernel, Dan Carpenter, Jacek Anaszewski,
Vincent Donnefort
Hello,
This patch series adds DT support for the LEDs found on the Kirkwood-based
LaCie boards 2Big and 5Big Network v2.
Changes since v1:
- 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 since v2:
- Fix pointer usage with the temporary u32 variable while calling
of_property_read_u32_index.
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 | 250 +++++++++++++++++++--
include/dt-bindings/leds/leds-netxbig.h | 18 ++
11 files changed, 501 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/3] leds: netxbig: add device tree binding
2015-06-18 14:59 ` Simon Guinot
@ 2015-06-18 14:59 ` Simon Guinot
-1 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-18 14:59 UTC (permalink / raw)
To: linux-arm-kernel
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>
---
Changes since v1:
- 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 since v2:
- Fix pointer usage with the temporary u32 variable while calling
of_property_read_u32_index.
.../devicetree/bindings/gpio/netxbig-gpio-ext.txt | 22 ++
.../devicetree/bindings/leds/leds-netxbig.txt | 92 ++++++++
drivers/leds/leds-netxbig.c | 250 +++++++++++++++++++--
include/dt-bindings/leds/leds-netxbig.h | 18 ++
4 files changed, 361 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
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..e4fb2fe11086
--- /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.
+- data-gpios: GPIOs representing the data register.
+- enable-gpio: GPIO used to enable the new configuration (address, data).
+
+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..3649dfebd1d3 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,241 @@ 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)
+ return ret;
+ num_addr = ret;
+ addr = devm_kzalloc(dev, num_addr * sizeof(unsigned int), 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)
+ return ret;
+ num_data = ret;
+ data = devm_kzalloc(dev, num_data * sizeof(unsigned int), 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)
+ 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)
+ return -EINVAL;
+
+ gpio_ext = devm_kzalloc(dev, sizeof(struct netxbig_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(struct netxbig_led_timer),
+ 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)
+ return -ENODEV;
+
+ leds = devm_kzalloc(dev, num_leds * sizeof(struct netxbig_led),
+ 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_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;
+
+ 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(int),
+ 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;
+
+ 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(struct netxbig_led_platform_data),
+ 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(struct netxbig_led_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 +594,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 +610,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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 1/3] leds: netxbig: add device tree binding
@ 2015-06-18 14:59 ` Simon Guinot
0 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-18 14:59 UTC (permalink / raw)
To: Bryan Wu, Richard Purdie, Jason Cooper, Andrew Lunn,
Gregory Clement, Sebastian Hesselbarth
Cc: linux-leds, linux-arm-kernel, Dan Carpenter, Jacek Anaszewski,
Vincent Donnefort
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>
---
Changes since v1:
- 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 since v2:
- Fix pointer usage with the temporary u32 variable while calling
of_property_read_u32_index.
.../devicetree/bindings/gpio/netxbig-gpio-ext.txt | 22 ++
.../devicetree/bindings/leds/leds-netxbig.txt | 92 ++++++++
drivers/leds/leds-netxbig.c | 250 +++++++++++++++++++--
include/dt-bindings/leds/leds-netxbig.h | 18 ++
4 files changed, 361 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
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..e4fb2fe11086
--- /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.
+- data-gpios: GPIOs representing the data register.
+- enable-gpio: GPIO used to enable the new configuration (address, data).
+
+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..3649dfebd1d3 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,241 @@ 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)
+ return ret;
+ num_addr = ret;
+ addr = devm_kzalloc(dev, num_addr * sizeof(unsigned int), 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)
+ return ret;
+ num_data = ret;
+ data = devm_kzalloc(dev, num_data * sizeof(unsigned int), 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)
+ 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)
+ return -EINVAL;
+
+ gpio_ext = devm_kzalloc(dev, sizeof(struct netxbig_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(struct netxbig_led_timer),
+ 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)
+ return -ENODEV;
+
+ leds = devm_kzalloc(dev, num_leds * sizeof(struct netxbig_led),
+ 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_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;
+
+ 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(int),
+ 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;
+
+ 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(struct netxbig_led_platform_data),
+ 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(struct netxbig_led_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 +594,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 +610,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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 2/3] ARM: Kirkwood: add LED DT entries for netxbig boards
2015-06-18 14:59 ` Simon Guinot
@ 2015-06-18 14:59 ` Simon Guinot
-1 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-18 14:59 UTC (permalink / raw)
To: linux-arm-kernel
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>
---
arch/arm/boot/dts/kirkwood-net5big.dts | 60 +++++++++++++++++++++++++
arch/arm/boot/dts/kirkwood-netxbig.dtsi | 80 +++++++++++++++++++++++++++++++++
2 files changed, 140 insertions(+)
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 b0cfb7cd30b9..5e7ed1c7e1b2 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 2/3] ARM: Kirkwood: add LED DT entries for netxbig boards
@ 2015-06-18 14:59 ` Simon Guinot
0 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-18 14:59 UTC (permalink / raw)
To: Bryan Wu, Richard Purdie, Jason Cooper, Andrew Lunn,
Gregory Clement, Sebastian Hesselbarth
Cc: linux-leds, linux-arm-kernel, Dan Carpenter, Jacek Anaszewski,
Vincent Donnefort
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>
---
arch/arm/boot/dts/kirkwood-net5big.dts | 60 +++++++++++++++++++++++++
arch/arm/boot/dts/kirkwood-netxbig.dtsi | 80 +++++++++++++++++++++++++++++++++
2 files changed, 140 insertions(+)
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 b0cfb7cd30b9..5e7ed1c7e1b2 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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 3/3] ARM: mvebu: remove static LED setup for netxbig boards
2015-06-18 14:59 ` Simon Guinot
@ 2015-06-18 14:59 ` Simon Guinot
-1 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-18 14:59 UTC (permalink / raw)
To: linux-arm-kernel
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>
---
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
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v3 3/3] ARM: mvebu: remove static LED setup for netxbig boards
@ 2015-06-18 14:59 ` Simon Guinot
0 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-18 14:59 UTC (permalink / raw)
To: Bryan Wu, Richard Purdie, Jason Cooper, Andrew Lunn,
Gregory Clement, Sebastian Hesselbarth
Cc: linux-leds, linux-arm-kernel, Dan Carpenter, Jacek Anaszewski,
Vincent Donnefort
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>
---
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
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/3] leds: netxbig: add device tree binding
2015-06-18 14:59 ` Simon Guinot
@ 2015-06-23 12:11 ` Jacek Anaszewski
-1 siblings, 0 replies; 18+ messages in thread
From: Jacek Anaszewski @ 2015-06-23 12:11 UTC (permalink / raw)
To: Simon Guinot
Cc: Bryan Wu, Richard Purdie, Jason Cooper, Andrew Lunn,
Gregory Clement, Sebastian Hesselbarth, linux-leds,
linux-arm-kernel, Dan Carpenter, Vincent Donnefort, devicetree,
Linus Walleij, Alexandre Courbot
Hi Simon,
On 06/18/2015 04: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>
> ---
> Changes since v1:
> - 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 since v2:
> - Fix pointer usage with the temporary u32 variable while calling
> of_property_read_u32_index.
>
> .../devicetree/bindings/gpio/netxbig-gpio-ext.txt | 22 ++
> .../devicetree/bindings/leds/leds-netxbig.txt | 92 ++++++++
> drivers/leds/leds-netxbig.c | 250 +++++++++++++++++++--
> include/dt-bindings/leds/leds-netxbig.h | 18 ++
> 4 files changed, 361 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
>
> 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..e4fb2fe11086
> --- /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.
> +- data-gpios: GPIOs representing the data register.
> +- enable-gpio: GPIO used to enable the new configuration (address, data).
Please mention that enable-gpio latches the settings 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>;
You should indicate which element is MSB/LSB.
> + enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
> +};
This needs gpio maintainer ack. Adding Linus and Alexandre.
Please also cc devicetree@vger.kernel.org always when modifying
DT bindings.
> 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.
We have a property led-max-microamp for this. Since LED subsystem
brightness level is not suitable for describing hardware properties,
we've switched to using microamperes. There is pending patch [1] that
makes the things more precise.
> +
> +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..3649dfebd1d3 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,241 @@ 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)
> + return ret;
Please add error messages when parsing fails somewhere. There is a bit
to parse in this driver and the messages would make debugging easier.
> + num_addr = ret;
> + addr = devm_kzalloc(dev, num_addr * sizeof(unsigned int), 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)
> + return ret;
> + num_data = ret;
> + data = devm_kzalloc(dev, num_data * sizeof(unsigned int), 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)
> + 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)
> + return -EINVAL;
Please add error message here.
> + gpio_ext = devm_kzalloc(dev, sizeof(struct netxbig_gpio_ext),
> + GFP_KERNEL);
Kernel code style would be: sizeof(*gpio_ext). The same is relevant
to all other occurrences of sizeof in this file.
> + 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(struct netxbig_led_timer),
> + 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;
You could get rid of tmp by reformatting above as follows:
of_property_read_u32_index(np, "timers", 3 * i + 1,
&timers[i].delay_on);
of_property_read_u32_index(np, "timers", 3 * i + 2,
&timers[i].delay_off);
> + }
> + pdata->timer = timers;
> + pdata->num_timer = num_timers;
> + }
> +
> + /* LEDs */
> + num_leds = of_get_child_count(np);
> + if (!num_leds)
> + return -ENODEV;
> +
> + leds = devm_kzalloc(dev, num_leds * sizeof(struct netxbig_led),
> + 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_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;
Please put above calls to of_property_read_string after all the
below conditions that can end up with returning an error.
> +
> + 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(int),
> + 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;
> +
> + 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(struct netxbig_led_platform_data),
> + GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> + ret = netxbig_leds_get_of_pdata(&pdev->dev, pdata);
> + if (ret)
> + return ret;
> + }
You're removing board file for the device later in this patch set, but
in the same time you seem to expect pdata from board file as the
first choice and resort to parsing DT only if pdata is not available.
This is inconsistent. I'd leave the board file intact for now, also
because there can be some users of it.
BTW how do you compile the driver? I've tried to use
multi_v5_defconfig and mvebu_v5_defconfig and I have build breaks
with both of them on the recent linux-next.
>
> - 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(struct netxbig_led_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 +594,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 +610,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 */
>
[1] https://patchwork.ozlabs.org/patch/459991/
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/3] leds: netxbig: add device tree binding
@ 2015-06-23 12:11 ` Jacek Anaszewski
0 siblings, 0 replies; 18+ messages in thread
From: Jacek Anaszewski @ 2015-06-23 12:11 UTC (permalink / raw)
To: linux-arm-kernel
Hi Simon,
On 06/18/2015 04: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>
> ---
> Changes since v1:
> - 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 since v2:
> - Fix pointer usage with the temporary u32 variable while calling
> of_property_read_u32_index.
>
> .../devicetree/bindings/gpio/netxbig-gpio-ext.txt | 22 ++
> .../devicetree/bindings/leds/leds-netxbig.txt | 92 ++++++++
> drivers/leds/leds-netxbig.c | 250 +++++++++++++++++++--
> include/dt-bindings/leds/leds-netxbig.h | 18 ++
> 4 files changed, 361 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
>
> 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..e4fb2fe11086
> --- /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.
> +- data-gpios: GPIOs representing the data register.
> +- enable-gpio: GPIO used to enable the new configuration (address, data).
Please mention that enable-gpio latches the settings 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>;
You should indicate which element is MSB/LSB.
> + enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
> +};
This needs gpio maintainer ack. Adding Linus and Alexandre.
Please also cc devicetree at vger.kernel.org always when modifying
DT bindings.
> 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.
We have a property led-max-microamp for this. Since LED subsystem
brightness level is not suitable for describing hardware properties,
we've switched to using microamperes. There is pending patch [1] that
makes the things more precise.
> +
> +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..3649dfebd1d3 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,241 @@ 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)
> + return ret;
Please add error messages when parsing fails somewhere. There is a bit
to parse in this driver and the messages would make debugging easier.
> + num_addr = ret;
> + addr = devm_kzalloc(dev, num_addr * sizeof(unsigned int), 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)
> + return ret;
> + num_data = ret;
> + data = devm_kzalloc(dev, num_data * sizeof(unsigned int), 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)
> + 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)
> + return -EINVAL;
Please add error message here.
> + gpio_ext = devm_kzalloc(dev, sizeof(struct netxbig_gpio_ext),
> + GFP_KERNEL);
Kernel code style would be: sizeof(*gpio_ext). The same is relevant
to all other occurrences of sizeof in this file.
> + 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(struct netxbig_led_timer),
> + 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;
You could get rid of tmp by reformatting above as follows:
of_property_read_u32_index(np, "timers", 3 * i + 1,
&timers[i].delay_on);
of_property_read_u32_index(np, "timers", 3 * i + 2,
&timers[i].delay_off);
> + }
> + pdata->timer = timers;
> + pdata->num_timer = num_timers;
> + }
> +
> + /* LEDs */
> + num_leds = of_get_child_count(np);
> + if (!num_leds)
> + return -ENODEV;
> +
> + leds = devm_kzalloc(dev, num_leds * sizeof(struct netxbig_led),
> + 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_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;
Please put above calls to of_property_read_string after all the
below conditions that can end up with returning an error.
> +
> + 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(int),
> + 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;
> +
> + 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(struct netxbig_led_platform_data),
> + GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> + ret = netxbig_leds_get_of_pdata(&pdev->dev, pdata);
> + if (ret)
> + return ret;
> + }
You're removing board file for the device later in this patch set, but
in the same time you seem to expect pdata from board file as the
first choice and resort to parsing DT only if pdata is not available.
This is inconsistent. I'd leave the board file intact for now, also
because there can be some users of it.
BTW how do you compile the driver? I've tried to use
multi_v5_defconfig and mvebu_v5_defconfig and I have build breaks
with both of them on the recent linux-next.
>
> - 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(struct netxbig_led_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 +594,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 +610,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 */
>
[1] https://patchwork.ozlabs.org/patch/459991/
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/3] leds: netxbig: add device tree binding
2015-06-23 12:11 ` Jacek Anaszewski
@ 2015-06-23 21:17 ` Simon Guinot
-1 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-23 21:17 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Bryan Wu, Richard Purdie, Jason Cooper, Andrew Lunn,
Gregory Clement, Sebastian Hesselbarth, linux-leds,
linux-arm-kernel, Dan Carpenter, Vincent Donnefort, devicetree,
Linus Walleij, Alexandre Courbot
[-- Attachment #1: Type: text/plain, Size: 17273 bytes --]
On Tue, Jun 23, 2015 at 02:11:54PM +0200, Jacek Anaszewski wrote:
> Hi Simon,
Hi Jacek,
Thanks again for taking care of this patch set.
>
> On 06/18/2015 04: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>
> >---
> >Changes since v1:
> >- 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 since v2:
> >- Fix pointer usage with the temporary u32 variable while calling
> > of_property_read_u32_index.
> >
> > .../devicetree/bindings/gpio/netxbig-gpio-ext.txt | 22 ++
> > .../devicetree/bindings/leds/leds-netxbig.txt | 92 ++++++++
> > drivers/leds/leds-netxbig.c | 250 +++++++++++++++++++--
> > include/dt-bindings/leds/leds-netxbig.h | 18 ++
> > 4 files changed, 361 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
> >
> >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..e4fb2fe11086
> >--- /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.
> >+- data-gpios: GPIOs representing the data register.
> >+- enable-gpio: GPIO used to enable the new configuration (address, data).
>
> Please mention that enable-gpio latches the settings on raising edge.
OK.
>
> >+
> >+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>;
>
> You should indicate which element is MSB/LSB.
OK.
>
> >+ enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
> >+};
>
> This needs gpio maintainer ack. Adding Linus and Alexandre.
>
> Please also cc devicetree@vger.kernel.org always when modifying
> DT bindings.
OK.
>
> >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.
>
> We have a property led-max-microamp for this. Since LED subsystem
> brightness level is not suitable for describing hardware properties,
> we've switched to using microamperes. There is pending patch [1] that
> makes the things more precise.
Thanks. I'll have a look at this.
>
> >+
> >+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..3649dfebd1d3 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,241 @@ 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)
> >+ return ret;
>
> Please add error messages when parsing fails somewhere. There is a bit
> to parse in this driver and the messages would make debugging easier.
I am not sure it makes sense to add an error message here. It is very
unlikely we will get an error here. The only purpose would be debugging
while writing a DT node for this driver. With the DT binding document,
I think it is quite hard to make it wrong. Moreover, while writing this
code, I didn't need messages here.
But if you think that error messages are needed anyway, I'll be glad to
add them.
>
> >+ num_addr = ret;
> >+ addr = devm_kzalloc(dev, num_addr * sizeof(unsigned int), 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)
> >+ return ret;
> >+ num_data = ret;
> >+ data = devm_kzalloc(dev, num_data * sizeof(unsigned int), 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)
> >+ 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)
> >+ return -EINVAL;
>
> Please add error message here.
>
> >+ gpio_ext = devm_kzalloc(dev, sizeof(struct netxbig_gpio_ext),
> >+ GFP_KERNEL);
>
> Kernel code style would be: sizeof(*gpio_ext). The same is relevant
> to all other occurrences of sizeof in this file.
OK.
>
> >+ 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(struct netxbig_led_timer),
> >+ 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;
>
> You could get rid of tmp by reformatting above as follows:
>
> of_property_read_u32_index(np, "timers", 3 * i + 1,
> &timers[i].delay_on);
> of_property_read_u32_index(np, "timers", 3 * i + 2,
> &timers[i].delay_off);
No we can't. This was the main purpose for the v2. delay_{on,off} are
unsigned long and of_property_read_u32_index expects u32. On an 64-bit
big endian machine, this code is broken. Even if this driver will never
be used on a such architecture, it is still a mistake. Moreover, it has
been reported by Dan Carpenter that the static code checker was not
happy about that.
That's why we are using an intermediary variable here.
>
> >+ }
> >+ pdata->timer = timers;
> >+ pdata->num_timer = num_timers;
> >+ }
> >+
> >+ /* LEDs */
> >+ num_leds = of_get_child_count(np);
> >+ if (!num_leds)
> >+ return -ENODEV;
> >+
> >+ leds = devm_kzalloc(dev, num_leds * sizeof(struct netxbig_led),
> >+ 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_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;
>
> Please put above calls to of_property_read_string after all the
> below conditions that can end up with returning an error.
OK.
>
> >+
> >+ 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(int),
> >+ 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;
> >+
> >+ 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(struct netxbig_led_platform_data),
> >+ GFP_KERNEL);
> >+ if (!pdata)
> >+ return -ENOMEM;
> >+ ret = netxbig_leds_get_of_pdata(&pdev->dev, pdata);
> >+ if (ret)
> >+ return ret;
> >+ }
>
> You're removing board file for the device later in this patch set, but
> in the same time you seem to expect pdata from board file as the
> first choice and resort to parsing DT only if pdata is not available.
> This is inconsistent. I'd leave the board file intact for now, also
> because there can be some users of it.
It is probably true that the next patch makes it inconsistent. But even
if there is no more "board setup" code to fill pdata, I think it may be
convenient to still support this mechanism.
About the board file removal, it is safe. AFAIK, for this boards, users
will update the Linux and DTB images together. Backward compatibility
between Linux and the earlier DTB version is not supported. Else we
would have to keep this board file forever and I am pretty sure that
the mvebu maintainers will disagree about that :)
>
> BTW how do you compile the driver? I've tried to use
> multi_v5_defconfig and mvebu_v5_defconfig and I have build breaks
> with both of them on the recent linux-next.
For the original patch set version (Linux 4.0 at the time), I have used
the mvebu_v5_defconfig. Now I am using a lighter configuration because
I don't need to build the whole mvebu stuff.
What kind of error do you get ? I remember something about setting
CONFIG_HZ to 250.
Thanks,
Simon
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/3] leds: netxbig: add device tree binding
@ 2015-06-23 21:17 ` Simon Guinot
0 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-23 21:17 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 23, 2015 at 02:11:54PM +0200, Jacek Anaszewski wrote:
> Hi Simon,
Hi Jacek,
Thanks again for taking care of this patch set.
>
> On 06/18/2015 04: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>
> >---
> >Changes since v1:
> >- 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 since v2:
> >- Fix pointer usage with the temporary u32 variable while calling
> > of_property_read_u32_index.
> >
> > .../devicetree/bindings/gpio/netxbig-gpio-ext.txt | 22 ++
> > .../devicetree/bindings/leds/leds-netxbig.txt | 92 ++++++++
> > drivers/leds/leds-netxbig.c | 250 +++++++++++++++++++--
> > include/dt-bindings/leds/leds-netxbig.h | 18 ++
> > 4 files changed, 361 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
> >
> >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..e4fb2fe11086
> >--- /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.
> >+- data-gpios: GPIOs representing the data register.
> >+- enable-gpio: GPIO used to enable the new configuration (address, data).
>
> Please mention that enable-gpio latches the settings on raising edge.
OK.
>
> >+
> >+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>;
>
> You should indicate which element is MSB/LSB.
OK.
>
> >+ enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
> >+};
>
> This needs gpio maintainer ack. Adding Linus and Alexandre.
>
> Please also cc devicetree at vger.kernel.org always when modifying
> DT bindings.
OK.
>
> >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.
>
> We have a property led-max-microamp for this. Since LED subsystem
> brightness level is not suitable for describing hardware properties,
> we've switched to using microamperes. There is pending patch [1] that
> makes the things more precise.
Thanks. I'll have a look at this.
>
> >+
> >+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..3649dfebd1d3 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,241 @@ 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)
> >+ return ret;
>
> Please add error messages when parsing fails somewhere. There is a bit
> to parse in this driver and the messages would make debugging easier.
I am not sure it makes sense to add an error message here. It is very
unlikely we will get an error here. The only purpose would be debugging
while writing a DT node for this driver. With the DT binding document,
I think it is quite hard to make it wrong. Moreover, while writing this
code, I didn't need messages here.
But if you think that error messages are needed anyway, I'll be glad to
add them.
>
> >+ num_addr = ret;
> >+ addr = devm_kzalloc(dev, num_addr * sizeof(unsigned int), 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)
> >+ return ret;
> >+ num_data = ret;
> >+ data = devm_kzalloc(dev, num_data * sizeof(unsigned int), 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)
> >+ 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)
> >+ return -EINVAL;
>
> Please add error message here.
>
> >+ gpio_ext = devm_kzalloc(dev, sizeof(struct netxbig_gpio_ext),
> >+ GFP_KERNEL);
>
> Kernel code style would be: sizeof(*gpio_ext). The same is relevant
> to all other occurrences of sizeof in this file.
OK.
>
> >+ 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(struct netxbig_led_timer),
> >+ 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;
>
> You could get rid of tmp by reformatting above as follows:
>
> of_property_read_u32_index(np, "timers", 3 * i + 1,
> &timers[i].delay_on);
> of_property_read_u32_index(np, "timers", 3 * i + 2,
> &timers[i].delay_off);
No we can't. This was the main purpose for the v2. delay_{on,off} are
unsigned long and of_property_read_u32_index expects u32. On an 64-bit
big endian machine, this code is broken. Even if this driver will never
be used on a such architecture, it is still a mistake. Moreover, it has
been reported by Dan Carpenter that the static code checker was not
happy about that.
That's why we are using an intermediary variable here.
>
> >+ }
> >+ pdata->timer = timers;
> >+ pdata->num_timer = num_timers;
> >+ }
> >+
> >+ /* LEDs */
> >+ num_leds = of_get_child_count(np);
> >+ if (!num_leds)
> >+ return -ENODEV;
> >+
> >+ leds = devm_kzalloc(dev, num_leds * sizeof(struct netxbig_led),
> >+ 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_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;
>
> Please put above calls to of_property_read_string after all the
> below conditions that can end up with returning an error.
OK.
>
> >+
> >+ 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(int),
> >+ 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;
> >+
> >+ 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(struct netxbig_led_platform_data),
> >+ GFP_KERNEL);
> >+ if (!pdata)
> >+ return -ENOMEM;
> >+ ret = netxbig_leds_get_of_pdata(&pdev->dev, pdata);
> >+ if (ret)
> >+ return ret;
> >+ }
>
> You're removing board file for the device later in this patch set, but
> in the same time you seem to expect pdata from board file as the
> first choice and resort to parsing DT only if pdata is not available.
> This is inconsistent. I'd leave the board file intact for now, also
> because there can be some users of it.
It is probably true that the next patch makes it inconsistent. But even
if there is no more "board setup" code to fill pdata, I think it may be
convenient to still support this mechanism.
About the board file removal, it is safe. AFAIK, for this boards, users
will update the Linux and DTB images together. Backward compatibility
between Linux and the earlier DTB version is not supported. Else we
would have to keep this board file forever and I am pretty sure that
the mvebu maintainers will disagree about that :)
>
> BTW how do you compile the driver? I've tried to use
> multi_v5_defconfig and mvebu_v5_defconfig and I have build breaks
> with both of them on the recent linux-next.
For the original patch set version (Linux 4.0 at the time), I have used
the mvebu_v5_defconfig. Now I am using a lighter configuration because
I don't need to build the whole mvebu stuff.
What kind of error do you get ? I remember something about setting
CONFIG_HZ to 250.
Thanks,
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150623/a619ffdd/attachment-0001.sig>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/3] leds: netxbig: add device tree binding
2015-06-23 21:17 ` Simon Guinot
@ 2015-06-24 9:07 ` Jacek Anaszewski
-1 siblings, 0 replies; 18+ messages in thread
From: Jacek Anaszewski @ 2015-06-24 9:07 UTC (permalink / raw)
To: Simon Guinot
Cc: Bryan Wu, Richard Purdie, Jason Cooper, Andrew Lunn,
Gregory Clement, Sebastian Hesselbarth, linux-leds,
linux-arm-kernel, Dan Carpenter, Vincent Donnefort, devicetree,
Linus Walleij, Alexandre Courbot
Hi Simon,
On 06/23/2015 11:17 PM, Simon Guinot wrote:
> On Tue, Jun 23, 2015 at 02:11:54PM +0200, Jacek Anaszewski wrote:
>> Hi Simon,
>
> Hi Jacek,
>
> Thanks again for taking care of this patch set.
>
>>
>> On 06/18/2015 04: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>
>>> ---
>>> Changes since v1:
>>> - 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 since v2:
>>> - Fix pointer usage with the temporary u32 variable while calling
>>> of_property_read_u32_index.
>>>
>>> .../devicetree/bindings/gpio/netxbig-gpio-ext.txt | 22 ++
>>> .../devicetree/bindings/leds/leds-netxbig.txt | 92 ++++++++
>>> drivers/leds/leds-netxbig.c | 250 +++++++++++++++++++--
>>> include/dt-bindings/leds/leds-netxbig.h | 18 ++
>>> 4 files changed, 361 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
>>>
>>> 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..e4fb2fe11086
>>> --- /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.
>>> +- data-gpios: GPIOs representing the data register.
>>> +- enable-gpio: GPIO used to enable the new configuration (address, data).
>>
>> Please mention that enable-gpio latches the settings on raising edge.
>
> OK.
>
>>
>>> +
>>> +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>;
>>
>> You should indicate which element is MSB/LSB.
>
> OK.
>
>>
>>> + enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
>>> +};
>>
>> This needs gpio maintainer ack. Adding Linus and Alexandre.
>>
>> Please also cc devicetree@vger.kernel.org always when modifying
>> DT bindings.
>
> OK.
>
>>
>>> 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.
>>
>> We have a property led-max-microamp for this. Since LED subsystem
>> brightness level is not suitable for describing hardware properties,
>> we've switched to using microamperes. There is pending patch [1] that
>> makes the things more precise.
>
> Thanks. I'll have a look at this.
>
>>
>>> +
>>> +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..3649dfebd1d3 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,241 @@ 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)
>>> + return ret;
>>
>> Please add error messages when parsing fails somewhere. There is a bit
>> to parse in this driver and the messages would make debugging easier.
>
> I am not sure it makes sense to add an error message here. It is very
> unlikely we will get an error here. The only purpose would be debugging
> while writing a DT node for this driver. With the DT binding document,
> I think it is quite hard to make it wrong. Moreover, while writing this
> code, I didn't need messages here.
You knew the code and you was a designer. If you looked at this from the
perspective of a person who doesn't have an access to the driver source
code or isn't fluent in code analysis the things look different.
> But if you think that error messages are needed anyway, I'll be glad to
> add them.
It's up to you. If you looked through the other kernel drivers, many of
them provide error messages when DT parsing fails.
>>
>>> + num_addr = ret;
>>> + addr = devm_kzalloc(dev, num_addr * sizeof(unsigned int), 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)
>>> + return ret;
>>> + num_data = ret;
>>> + data = devm_kzalloc(dev, num_data * sizeof(unsigned int), 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)
>>> + 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)
>>> + return -EINVAL;
>>
>> Please add error message here.
>>
>>> + gpio_ext = devm_kzalloc(dev, sizeof(struct netxbig_gpio_ext),
>>> + GFP_KERNEL);
>>
>> Kernel code style would be: sizeof(*gpio_ext). The same is relevant
>> to all other occurrences of sizeof in this file.
>
> OK.
>
>>
>>> + 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(struct netxbig_led_timer),
>>> + 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;
>>
>> You could get rid of tmp by reformatting above as follows:
>>
>> of_property_read_u32_index(np, "timers", 3 * i + 1,
>> &timers[i].delay_on);
>> of_property_read_u32_index(np, "timers", 3 * i + 2,
>> &timers[i].delay_off);
>
> No we can't. This was the main purpose for the v2. delay_{on,off} are
> unsigned long and of_property_read_u32_index expects u32. On an 64-bit
> big endian machine, this code is broken. Even if this driver will never
> be used on a such architecture, it is still a mistake. Moreover, it has
> been reported by Dan Carpenter that the static code checker was not
> happy about that.
>
> That's why we are using an intermediary variable here.
OK, I had to refresh my memory and it turned out that I myself
also took part in that discussion. tmp is indeed required.
>>
>>> + }
>>> + pdata->timer = timers;
>>> + pdata->num_timer = num_timers;
>>> + }
>>> +
>>> + /* LEDs */
>>> + num_leds = of_get_child_count(np);
>>> + if (!num_leds)
>>> + return -ENODEV;
>>> +
>>> + leds = devm_kzalloc(dev, num_leds * sizeof(struct netxbig_led),
>>> + 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_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;
>>
>> Please put above calls to of_property_read_string after all the
>> below conditions that can end up with returning an error.
>
> OK.
>
>>
>>> +
>>> + 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(int),
>>> + 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;
>>> +
>>> + 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(struct netxbig_led_platform_data),
>>> + GFP_KERNEL);
>>> + if (!pdata)
>>> + return -ENOMEM;
>>> + ret = netxbig_leds_get_of_pdata(&pdev->dev, pdata);
>>> + if (ret)
>>> + return ret;
>>> + }
>>
>> You're removing board file for the device later in this patch set, but
>> in the same time you seem to expect pdata from board file as the
>> first choice and resort to parsing DT only if pdata is not available.
>> This is inconsistent. I'd leave the board file intact for now, also
>> because there can be some users of it.
>
> It is probably true that the next patch makes it inconsistent. But even
> if there is no more "board setup" code to fill pdata, I think it may be
> convenient to still support this mechanism.
>
> About the board file removal, it is safe. AFAIK, for this boards, users
> will update the Linux and DTB images together. Backward compatibility
> between Linux and the earlier DTB version is not supported. Else we
> would have to keep this board file forever and I am pretty sure that
> the mvebu maintainers will disagree about that :)
Ack.
>>
>> BTW how do you compile the driver? I've tried to use
>> multi_v5_defconfig and mvebu_v5_defconfig and I have build breaks
>> with both of them on the recent linux-next.
>
> For the original patch set version (Linux 4.0 at the time), I have used
> the mvebu_v5_defconfig. Now I am using a lighter configuration because
> I don't need to build the whole mvebu stuff.
>
> What kind of error do you get ? I remember something about setting
> CONFIG_HZ to 250.
I am getting:
drivers/crypto/mv_cesa.c:1037:2: error: implicit declaration of function
‘of_get_named_gen_pool’ [-Werror=implicit-function-declaration]
Compilation succeeded after disabling CONFIG_CRYPTO_DEV_MV_CESA.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/3] leds: netxbig: add device tree binding
@ 2015-06-24 9:07 ` Jacek Anaszewski
0 siblings, 0 replies; 18+ messages in thread
From: Jacek Anaszewski @ 2015-06-24 9:07 UTC (permalink / raw)
To: linux-arm-kernel
Hi Simon,
On 06/23/2015 11:17 PM, Simon Guinot wrote:
> On Tue, Jun 23, 2015 at 02:11:54PM +0200, Jacek Anaszewski wrote:
>> Hi Simon,
>
> Hi Jacek,
>
> Thanks again for taking care of this patch set.
>
>>
>> On 06/18/2015 04: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>
>>> ---
>>> Changes since v1:
>>> - 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 since v2:
>>> - Fix pointer usage with the temporary u32 variable while calling
>>> of_property_read_u32_index.
>>>
>>> .../devicetree/bindings/gpio/netxbig-gpio-ext.txt | 22 ++
>>> .../devicetree/bindings/leds/leds-netxbig.txt | 92 ++++++++
>>> drivers/leds/leds-netxbig.c | 250 +++++++++++++++++++--
>>> include/dt-bindings/leds/leds-netxbig.h | 18 ++
>>> 4 files changed, 361 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
>>>
>>> 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..e4fb2fe11086
>>> --- /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.
>>> +- data-gpios: GPIOs representing the data register.
>>> +- enable-gpio: GPIO used to enable the new configuration (address, data).
>>
>> Please mention that enable-gpio latches the settings on raising edge.
>
> OK.
>
>>
>>> +
>>> +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>;
>>
>> You should indicate which element is MSB/LSB.
>
> OK.
>
>>
>>> + enable-gpio = <&gpio0 29 GPIO_ACTIVE_HIGH>;
>>> +};
>>
>> This needs gpio maintainer ack. Adding Linus and Alexandre.
>>
>> Please also cc devicetree at vger.kernel.org always when modifying
>> DT bindings.
>
> OK.
>
>>
>>> 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.
>>
>> We have a property led-max-microamp for this. Since LED subsystem
>> brightness level is not suitable for describing hardware properties,
>> we've switched to using microamperes. There is pending patch [1] that
>> makes the things more precise.
>
> Thanks. I'll have a look at this.
>
>>
>>> +
>>> +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..3649dfebd1d3 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,241 @@ 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)
>>> + return ret;
>>
>> Please add error messages when parsing fails somewhere. There is a bit
>> to parse in this driver and the messages would make debugging easier.
>
> I am not sure it makes sense to add an error message here. It is very
> unlikely we will get an error here. The only purpose would be debugging
> while writing a DT node for this driver. With the DT binding document,
> I think it is quite hard to make it wrong. Moreover, while writing this
> code, I didn't need messages here.
You knew the code and you was a designer. If you looked at this from the
perspective of a person who doesn't have an access to the driver source
code or isn't fluent in code analysis the things look different.
> But if you think that error messages are needed anyway, I'll be glad to
> add them.
It's up to you. If you looked through the other kernel drivers, many of
them provide error messages when DT parsing fails.
>>
>>> + num_addr = ret;
>>> + addr = devm_kzalloc(dev, num_addr * sizeof(unsigned int), 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)
>>> + return ret;
>>> + num_data = ret;
>>> + data = devm_kzalloc(dev, num_data * sizeof(unsigned int), 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)
>>> + 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)
>>> + return -EINVAL;
>>
>> Please add error message here.
>>
>>> + gpio_ext = devm_kzalloc(dev, sizeof(struct netxbig_gpio_ext),
>>> + GFP_KERNEL);
>>
>> Kernel code style would be: sizeof(*gpio_ext). The same is relevant
>> to all other occurrences of sizeof in this file.
>
> OK.
>
>>
>>> + 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(struct netxbig_led_timer),
>>> + 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;
>>
>> You could get rid of tmp by reformatting above as follows:
>>
>> of_property_read_u32_index(np, "timers", 3 * i + 1,
>> &timers[i].delay_on);
>> of_property_read_u32_index(np, "timers", 3 * i + 2,
>> &timers[i].delay_off);
>
> No we can't. This was the main purpose for the v2. delay_{on,off} are
> unsigned long and of_property_read_u32_index expects u32. On an 64-bit
> big endian machine, this code is broken. Even if this driver will never
> be used on a such architecture, it is still a mistake. Moreover, it has
> been reported by Dan Carpenter that the static code checker was not
> happy about that.
>
> That's why we are using an intermediary variable here.
OK, I had to refresh my memory and it turned out that I myself
also took part in that discussion. tmp is indeed required.
>>
>>> + }
>>> + pdata->timer = timers;
>>> + pdata->num_timer = num_timers;
>>> + }
>>> +
>>> + /* LEDs */
>>> + num_leds = of_get_child_count(np);
>>> + if (!num_leds)
>>> + return -ENODEV;
>>> +
>>> + leds = devm_kzalloc(dev, num_leds * sizeof(struct netxbig_led),
>>> + 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_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;
>>
>> Please put above calls to of_property_read_string after all the
>> below conditions that can end up with returning an error.
>
> OK.
>
>>
>>> +
>>> + 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(int),
>>> + 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;
>>> +
>>> + 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(struct netxbig_led_platform_data),
>>> + GFP_KERNEL);
>>> + if (!pdata)
>>> + return -ENOMEM;
>>> + ret = netxbig_leds_get_of_pdata(&pdev->dev, pdata);
>>> + if (ret)
>>> + return ret;
>>> + }
>>
>> You're removing board file for the device later in this patch set, but
>> in the same time you seem to expect pdata from board file as the
>> first choice and resort to parsing DT only if pdata is not available.
>> This is inconsistent. I'd leave the board file intact for now, also
>> because there can be some users of it.
>
> It is probably true that the next patch makes it inconsistent. But even
> if there is no more "board setup" code to fill pdata, I think it may be
> convenient to still support this mechanism.
>
> About the board file removal, it is safe. AFAIK, for this boards, users
> will update the Linux and DTB images together. Backward compatibility
> between Linux and the earlier DTB version is not supported. Else we
> would have to keep this board file forever and I am pretty sure that
> the mvebu maintainers will disagree about that :)
Ack.
>>
>> BTW how do you compile the driver? I've tried to use
>> multi_v5_defconfig and mvebu_v5_defconfig and I have build breaks
>> with both of them on the recent linux-next.
>
> For the original patch set version (Linux 4.0 at the time), I have used
> the mvebu_v5_defconfig. Now I am using a lighter configuration because
> I don't need to build the whole mvebu stuff.
>
> What kind of error do you get ? I remember something about setting
> CONFIG_HZ to 250.
I am getting:
drivers/crypto/mv_cesa.c:1037:2: error: implicit declaration of function
?of_get_named_gen_pool? [-Werror=implicit-function-declaration]
Compilation succeeded after disabling CONFIG_CRYPTO_DEV_MV_CESA.
--
Best Regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/3] leds: netxbig: add device tree binding
2015-06-24 9:07 ` Jacek Anaszewski
@ 2015-06-25 8:46 ` Simon Guinot
-1 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-25 8:46 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Andrew Lunn, Jason Cooper, devicetree, Linus Walleij, Bryan Wu,
Vincent Donnefort, Richard Purdie, linux-arm-kernel,
Gregory Clement, Dan Carpenter, Alexandre Courbot, linux-leds,
Sebastian Hesselbarth
[-- Attachment #1: Type: text/plain, Size: 976 bytes --]
On Wed, Jun 24, 2015 at 11:07:57AM +0200, Jacek Anaszewski wrote:
> >>BTW how do you compile the driver? I've tried to use
> >>multi_v5_defconfig and mvebu_v5_defconfig and I have build breaks
> >>with both of them on the recent linux-next.
> >
> >For the original patch set version (Linux 4.0 at the time), I have used
> >the mvebu_v5_defconfig. Now I am using a lighter configuration because
> >I don't need to build the whole mvebu stuff.
> >
> >What kind of error do you get ? I remember something about setting
> >CONFIG_HZ to 250.
>
> I am getting:
>
> drivers/crypto/mv_cesa.c:1037:2: error: implicit declaration of
> function ‘of_get_named_gen_pool’
> [-Werror=implicit-function-declaration]
>
> Compilation succeeded after disabling CONFIG_CRYPTO_DEV_MV_CESA.
It is because the merge in linux-next missed the renaming of
of_get_named_gen_pool() in of_gen_pool_get() for the mv_cesa driver.
I have sent a patch, just in case.
Simon
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/3] leds: netxbig: add device tree binding
@ 2015-06-25 8:46 ` Simon Guinot
0 siblings, 0 replies; 18+ messages in thread
From: Simon Guinot @ 2015-06-25 8:46 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jun 24, 2015 at 11:07:57AM +0200, Jacek Anaszewski wrote:
> >>BTW how do you compile the driver? I've tried to use
> >>multi_v5_defconfig and mvebu_v5_defconfig and I have build breaks
> >>with both of them on the recent linux-next.
> >
> >For the original patch set version (Linux 4.0 at the time), I have used
> >the mvebu_v5_defconfig. Now I am using a lighter configuration because
> >I don't need to build the whole mvebu stuff.
> >
> >What kind of error do you get ? I remember something about setting
> >CONFIG_HZ to 250.
>
> I am getting:
>
> drivers/crypto/mv_cesa.c:1037:2: error: implicit declaration of
> function ?of_get_named_gen_pool?
> [-Werror=implicit-function-declaration]
>
> Compilation succeeded after disabling CONFIG_CRYPTO_DEV_MV_CESA.
It is because the merge in linux-next missed the renaming of
of_get_named_gen_pool() in of_gen_pool_get() for the mv_cesa driver.
I have sent a patch, just in case.
Simon
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150625/19613b06/attachment.sig>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v3 1/3] leds: netxbig: add device tree binding
2015-06-23 21:17 ` Simon Guinot
@ 2015-07-16 8:25 ` Linus Walleij
-1 siblings, 0 replies; 18+ messages in thread
From: Linus Walleij @ 2015-07-16 8:25 UTC (permalink / raw)
To: Simon Guinot
Cc: Jacek Anaszewski, Bryan Wu, Richard Purdie, Jason Cooper,
Andrew Lunn, Gregory Clement, Sebastian Hesselbarth,
linux-leds@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Dan Carpenter, Vincent Donnefort, devicetree@vger.kernel.org,
Alexandre Courbot
On Tue, Jun 23, 2015 at 11:17 PM, Simon Guinot
<simon.guinot@sequanux.org> wrote:
> On Tue, Jun 23, 2015 at 02:11:54PM +0200, Jacek Anaszewski wrote:
>> >+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.
>> >+- data-gpios: GPIOs representing the data register.
>> >+- enable-gpio: GPIO used to enable the new configuration (address, data).
(...)
>> >+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>;
>> >+};
>>
>> This needs gpio maintainer ack. Adding Linus and Alexandre.
Looks pretty straight-forward, a lot of GPIOs for LEDs, well such is life.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v3 1/3] leds: netxbig: add device tree binding
@ 2015-07-16 8:25 ` Linus Walleij
0 siblings, 0 replies; 18+ messages in thread
From: Linus Walleij @ 2015-07-16 8:25 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 23, 2015 at 11:17 PM, Simon Guinot
<simon.guinot@sequanux.org> wrote:
> On Tue, Jun 23, 2015 at 02:11:54PM +0200, Jacek Anaszewski wrote:
>> >+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.
>> >+- data-gpios: GPIOs representing the data register.
>> >+- enable-gpio: GPIO used to enable the new configuration (address, data).
(...)
>> >+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>;
>> >+};
>>
>> This needs gpio maintainer ack. Adding Linus and Alexandre.
Looks pretty straight-forward, a lot of GPIOs for LEDs, well such is life.
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2015-07-16 8:25 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-18 14:59 [PATCH v3 0/3] Add DT support for netxbig LEDs Simon Guinot
2015-06-18 14:59 ` Simon Guinot
2015-06-18 14:59 ` [PATCH v3 1/3] leds: netxbig: add device tree binding Simon Guinot
2015-06-18 14:59 ` Simon Guinot
2015-06-23 12:11 ` Jacek Anaszewski
2015-06-23 12:11 ` Jacek Anaszewski
2015-06-23 21:17 ` Simon Guinot
2015-06-23 21:17 ` Simon Guinot
2015-06-24 9:07 ` Jacek Anaszewski
2015-06-24 9:07 ` Jacek Anaszewski
2015-06-25 8:46 ` Simon Guinot
2015-06-25 8:46 ` Simon Guinot
2015-07-16 8:25 ` Linus Walleij
2015-07-16 8:25 ` Linus Walleij
2015-06-18 14:59 ` [PATCH v3 2/3] ARM: Kirkwood: add LED DT entries for netxbig boards Simon Guinot
2015-06-18 14:59 ` Simon Guinot
2015-06-18 14:59 ` [PATCH v3 3/3] ARM: mvebu: remove static LED setup " Simon Guinot
2015-06-18 14:59 ` Simon Guinot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.