[PATCHv4 0/3] backlight: led-backlight driver

[PATCHv4 0/3] backlight: led-backlight driver

[Tomi Valkeinen]

This series aims to add a led-backlight driver, similar to pwm-backlight, but
using a LED class device underneath.

LED framework has no support for DT or getting a LED class driver from another
kernel driver, so I added minimal functionality to led-class to get
led-backlight working.

Changes to v3:
- Change a comment to refer to of_led_get()

Changes to v2:
- power supply is now optional
- cosmetic changes
- no-op function for led_put() when !CONFIG_LEDS_CLASS

Changes to v1:
- Split LED OF parts into separate .h and .c files
- Check for CONFIG_OF and CONFIG_LEDS_CLASS where relevant to leave unused code
  out.
- Improved error prints and comments a bit
- Added put_device() into led_put(), as the device was gotten from
  class_find_device() which requires a put_device() call.

 Tomi

Tomi Valkeinen (3):
  leds: Add of_led_get() and led_put()
  backlight: add led-backlight driver
  devicetree: Add led-backlight binding

 .../bindings/video/backlight/led-backlight.txt     |  30 +++
 drivers/leds/Makefile                              |   6 +-
 drivers/leds/led-class.c                           |  13 +-
 drivers/leds/led-of.c                              |  85 +++++++
 drivers/leds/leds.h                                |   1 +
 drivers/video/backlight/Kconfig                    |   7 +
 drivers/video/backlight/Makefile                   |   1 +
 drivers/video/backlight/led_bl.c                   | 246 +++++++++++++++++++++
 include/linux/leds.h                               |  10 +
 include/linux/of_leds.h                            |  26 +++
 10 files changed, 423 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
 create mode 100644 drivers/leds/led-of.c
 create mode 100644 drivers/video/backlight/led_bl.c
 create mode 100644 include/linux/of_leds.h

-- 
2.1.4

[PATCHv4 0/3] backlight: led-backlight driver

[Tomi Valkeinen]

This series aims to add a led-backlight driver, similar to pwm-backlight, but
using a LED class device underneath.

LED framework has no support for DT or getting a LED class driver from another
kernel driver, so I added minimal functionality to led-class to get
led-backlight working.

Changes to v3:
- Change a comment to refer to of_led_get()

Changes to v2:
- power supply is now optional
- cosmetic changes
- no-op function for led_put() when !CONFIG_LEDS_CLASS

Changes to v1:
- Split LED OF parts into separate .h and .c files
- Check for CONFIG_OF and CONFIG_LEDS_CLASS where relevant to leave unused code
  out.
- Improved error prints and comments a bit
- Added put_device() into led_put(), as the device was gotten from
  class_find_device() which requires a put_device() call.

 Tomi

Tomi Valkeinen (3):
  leds: Add of_led_get() and led_put()
  backlight: add led-backlight driver
  devicetree: Add led-backlight binding

 .../bindings/video/backlight/led-backlight.txt     |  30 +++
 drivers/leds/Makefile                              |   6 +-
 drivers/leds/led-class.c                           |  13 +-
 drivers/leds/led-of.c                              |  85 +++++++
 drivers/leds/leds.h                                |   1 +
 drivers/video/backlight/Kconfig                    |   7 +
 drivers/video/backlight/Makefile                   |   1 +
 drivers/video/backlight/led_bl.c                   | 246 +++++++++++++++++++++
 include/linux/leds.h                               |  10 +
 include/linux/of_leds.h                            |  26 +++
 10 files changed, 423 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
 create mode 100644 drivers/leds/led-of.c
 create mode 100644 drivers/video/backlight/led_bl.c
 create mode 100644 include/linux/of_leds.h

-- 
2.1.4

[PATCHv4 1/3] leds: Add of_led_get() and led_put()

[Tomi Valkeinen]

This patch adds basic support for a kernel driver to get a LED device.
This will be used by the led-backlight driver.

Only OF version is implemented for now, and the behavior is similar to
PWM's of_pwm_get() and pwm_put().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/leds/Makefile    |  6 +++-
 drivers/leds/led-class.c | 13 +++++++-
 drivers/leds/led-of.c    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/leds/leds.h      |  1 +
 include/linux/leds.h     | 10 ++++++
 include/linux/of_leds.h  | 26 +++++++++++++++
 6 files changed, 139 insertions(+), 2 deletions(-)
 create mode 100644 drivers/leds/led-of.c
 create mode 100644 include/linux/of_leds.h

diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 8d6a24a2f513..6fd22e411810 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -1,7 +1,11 @@
 
 # LED Core
 obj-$(CONFIG_NEW_LEDS)			+= led-core.o
-obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
+
+obj-$(CONFIG_LEDS_CLASS)		+= led-class-objs.o
+led-class-objs-y			:= led-class.o
+led-class-objs-$(CONFIG_OF)		+= led-of.o
+
 obj-$(CONFIG_LEDS_CLASS_FLASH)		+= led-class-flash.o
 obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
 
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index beabfbc6f7cd..ff0c27654358 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -22,7 +22,7 @@
 #include <linux/timer.h>
 #include "leds.h"
 
-static struct class *leds_class;
+struct class *leds_class;
 
 static ssize_t brightness_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
@@ -216,6 +216,17 @@ static int led_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
 
+/**
+ * led_put() - release a LED device, reserved with of_led_get()
+ * @led_cdev: LED device
+ */
+void led_put(struct led_classdev *led_cdev)
+{
+	put_device(led_cdev->dev);
+	module_put(led_cdev->dev->parent->driver->owner);
+}
+EXPORT_SYMBOL_GPL(led_put);
+
 static int match_name(struct device *dev, const void *data)
 {
 	if (!dev_name(dev))
diff --git a/drivers/leds/led-of.c b/drivers/leds/led-of.c
new file mode 100644
index 000000000000..6e96fee9adf1
--- /dev/null
+++ b/drivers/leds/led-of.c
@@ -0,0 +1,85 @@
+/*
+ * LED Class Core OF support
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_leds.h>
+
+#include "leds.h"
+
+/* find OF node for the given led_cdev */
+static struct device_node *find_led_of_node(struct led_classdev *led_cdev)
+{
+	struct device *led_dev = led_cdev->dev;
+	struct device_node *child;
+
+	for_each_child_of_node(led_dev->parent->of_node, child) {
+		int idx;
+
+		idx = of_property_match_string(child, "label", led_cdev->name);
+		if (idx == 0)
+			return child;
+	}
+
+	return NULL;
+}
+
+static int led_match_led_node(struct device *led_dev, const void *data)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(led_dev);
+	const struct device_node *target_node = data;
+	struct device_node *led_node;
+
+	led_node = find_led_of_node(led_cdev);
+	if (!led_node)
+		return 0;
+
+	of_node_put(led_node);
+
+	return led_node == target_node;
+}
+
+/**
+ * of_led_get() - request a LED device via the LED framework
+ * @np: device node to get the LED device from
+ *
+ * Returns the LED device parsed from the phandle specified in the "leds"
+ * property of a device tree node or a negative error-code on failure.
+ *
+ * The caller must use led_put() to release the device after use.
+ */
+struct led_classdev *of_led_get(struct device_node *np)
+{
+	struct device *led_dev;
+	struct led_classdev *led_cdev;
+	struct device_node *led_node;
+
+	led_node = of_parse_phandle(np, "leds", 0);
+	if (!led_node)
+		return ERR_PTR(-ENODEV);
+
+	led_dev = class_find_device(leds_class, NULL, led_node,
+		led_match_led_node);
+
+	of_node_put(led_node);
+
+	if (!led_dev) {
+		pr_err("failed to find led device for node %s, deferring probe\n",
+			of_node_full_name(led_node));
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	led_cdev = dev_get_drvdata(led_dev);
+
+	if (!try_module_get(led_cdev->dev->parent->driver->owner))
+		return ERR_PTR(-ENODEV);
+
+	return led_cdev;
+}
+EXPORT_SYMBOL_GPL(of_led_get);
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index bc89d7ace2c4..ccc3abb417d4 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -46,6 +46,7 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
 
 void led_stop_software_blink(struct led_classdev *led_cdev);
 
+extern struct class *leds_class;
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eeafb5dc..fbad4ce78e6e 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -113,6 +113,16 @@ extern void devm_led_classdev_unregister(struct device *parent,
 extern void led_classdev_suspend(struct led_classdev *led_cdev);
 extern void led_classdev_resume(struct led_classdev *led_cdev);
 
+#if IS_ENABLED(CONFIG_LEDS_CLASS)
+
+extern void led_put(struct led_classdev *led_cdev);
+
+#else
+
+static inline void led_put(struct led_classdev *led_cdev) { }
+
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS) */
+
 /**
  * led_blink_set - set blinking with software fallback
  * @led_cdev: the LED to start blinking
diff --git a/include/linux/of_leds.h b/include/linux/of_leds.h
new file mode 100644
index 000000000000..7e8e64bd9811
--- /dev/null
+++ b/include/linux/of_leds.h
@@ -0,0 +1,26 @@
+/*
+ * OF support for leds
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __LINUX_LEDS_OF_H_INCLUDED
+#define __LINUX_LEDS_OF_H_INCLUDED
+
+#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_LEDS_CLASS)
+
+extern struct led_classdev *of_led_get(struct device_node *np);
+
+#else
+
+static inline struct led_classdev *of_led_get(struct device_node *np)
+{
+	return -ENODEV;
+}
+
+#endif
+
+#endif /* __LINUX_LEDS_OF_H_INCLUDED */
-- 
2.1.4

[PATCHv4 1/3] leds: Add of_led_get() and led_put()

[Tomi Valkeinen]

This patch adds basic support for a kernel driver to get a LED device.
This will be used by the led-backlight driver.

Only OF version is implemented for now, and the behavior is similar to
PWM's of_pwm_get() and pwm_put().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/leds/Makefile    |  6 +++-
 drivers/leds/led-class.c | 13 +++++++-
 drivers/leds/led-of.c    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/leds/leds.h      |  1 +
 include/linux/leds.h     | 10 ++++++
 include/linux/of_leds.h  | 26 +++++++++++++++
 6 files changed, 139 insertions(+), 2 deletions(-)
 create mode 100644 drivers/leds/led-of.c
 create mode 100644 include/linux/of_leds.h

diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 8d6a24a2f513..6fd22e411810 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -1,7 +1,11 @@
 
 # LED Core
 obj-$(CONFIG_NEW_LEDS)			+= led-core.o
-obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
+
+obj-$(CONFIG_LEDS_CLASS)		+= led-class-objs.o
+led-class-objs-y			:= led-class.o
+led-class-objs-$(CONFIG_OF)		+= led-of.o
+
 obj-$(CONFIG_LEDS_CLASS_FLASH)		+= led-class-flash.o
 obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
 
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index beabfbc6f7cd..ff0c27654358 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -22,7 +22,7 @@
 #include <linux/timer.h>
 #include "leds.h"
 
-static struct class *leds_class;
+struct class *leds_class;
 
 static ssize_t brightness_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
@@ -216,6 +216,17 @@ static int led_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
 
+/**
+ * led_put() - release a LED device, reserved with of_led_get()
+ * @led_cdev: LED device
+ */
+void led_put(struct led_classdev *led_cdev)
+{
+	put_device(led_cdev->dev);
+	module_put(led_cdev->dev->parent->driver->owner);
+}
+EXPORT_SYMBOL_GPL(led_put);
+
 static int match_name(struct device *dev, const void *data)
 {
 	if (!dev_name(dev))
diff --git a/drivers/leds/led-of.c b/drivers/leds/led-of.c
new file mode 100644
index 000000000000..6e96fee9adf1
--- /dev/null
+++ b/drivers/leds/led-of.c
@@ -0,0 +1,85 @@
+/*
+ * LED Class Core OF support
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_leds.h>
+
+#include "leds.h"
+
+/* find OF node for the given led_cdev */
+static struct device_node *find_led_of_node(struct led_classdev *led_cdev)
+{
+	struct device *led_dev = led_cdev->dev;
+	struct device_node *child;
+
+	for_each_child_of_node(led_dev->parent->of_node, child) {
+		int idx;
+
+		idx = of_property_match_string(child, "label", led_cdev->name);
+		if (idx = 0)
+			return child;
+	}
+
+	return NULL;
+}
+
+static int led_match_led_node(struct device *led_dev, const void *data)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(led_dev);
+	const struct device_node *target_node = data;
+	struct device_node *led_node;
+
+	led_node = find_led_of_node(led_cdev);
+	if (!led_node)
+		return 0;
+
+	of_node_put(led_node);
+
+	return led_node = target_node;
+}
+
+/**
+ * of_led_get() - request a LED device via the LED framework
+ * @np: device node to get the LED device from
+ *
+ * Returns the LED device parsed from the phandle specified in the "leds"
+ * property of a device tree node or a negative error-code on failure.
+ *
+ * The caller must use led_put() to release the device after use.
+ */
+struct led_classdev *of_led_get(struct device_node *np)
+{
+	struct device *led_dev;
+	struct led_classdev *led_cdev;
+	struct device_node *led_node;
+
+	led_node = of_parse_phandle(np, "leds", 0);
+	if (!led_node)
+		return ERR_PTR(-ENODEV);
+
+	led_dev = class_find_device(leds_class, NULL, led_node,
+		led_match_led_node);
+
+	of_node_put(led_node);
+
+	if (!led_dev) {
+		pr_err("failed to find led device for node %s, deferring probe\n",
+			of_node_full_name(led_node));
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	led_cdev = dev_get_drvdata(led_dev);
+
+	if (!try_module_get(led_cdev->dev->parent->driver->owner))
+		return ERR_PTR(-ENODEV);
+
+	return led_cdev;
+}
+EXPORT_SYMBOL_GPL(of_led_get);
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index bc89d7ace2c4..ccc3abb417d4 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -46,6 +46,7 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
 
 void led_stop_software_blink(struct led_classdev *led_cdev);
 
+extern struct class *leds_class;
 extern struct rw_semaphore leds_list_lock;
 extern struct list_head leds_list;
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eeafb5dc..fbad4ce78e6e 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -113,6 +113,16 @@ extern void devm_led_classdev_unregister(struct device *parent,
 extern void led_classdev_suspend(struct led_classdev *led_cdev);
 extern void led_classdev_resume(struct led_classdev *led_cdev);
 
+#if IS_ENABLED(CONFIG_LEDS_CLASS)
+
+extern void led_put(struct led_classdev *led_cdev);
+
+#else
+
+static inline void led_put(struct led_classdev *led_cdev) { }
+
+#endif /* IS_ENABLED(CONFIG_LEDS_CLASS) */
+
 /**
  * led_blink_set - set blinking with software fallback
  * @led_cdev: the LED to start blinking
diff --git a/include/linux/of_leds.h b/include/linux/of_leds.h
new file mode 100644
index 000000000000..7e8e64bd9811
--- /dev/null
+++ b/include/linux/of_leds.h
@@ -0,0 +1,26 @@
+/*
+ * OF support for leds
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#ifndef __LINUX_LEDS_OF_H_INCLUDED
+#define __LINUX_LEDS_OF_H_INCLUDED
+
+#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_LEDS_CLASS)
+
+extern struct led_classdev *of_led_get(struct device_node *np);
+
+#else
+
+static inline struct led_classdev *of_led_get(struct device_node *np)
+{
+	return -ENODEV;
+}
+
+#endif
+
+#endif /* __LINUX_LEDS_OF_H_INCLUDED */
-- 
2.1.4

[PATCHv4 2/3] backlight: add led-backlight driver

[Tomi Valkeinen]

This patch adds a led-backlight driver (led_bl), which is similar to
pwm_bl except the driver uses a LED class driver to adjust the
brightness in the HW.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/Kconfig  |   7 ++
 drivers/video/backlight/Makefile |   1 +
 drivers/video/backlight/led_bl.c | 246 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 254 insertions(+)
 create mode 100644 drivers/video/backlight/led_bl.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 0505b796d743..d1336196aba2 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -453,6 +453,13 @@ config BACKLIGHT_BD6107
 	help
 	  If you have a Rohm BD6107 say Y to enable the backlight driver.
 
+config BACKLIGHT_LED
+	tristate "Generic LED based Backlight Driver"
+	depends on LEDS_CLASS && OF
+	help
+	  If you have a LCD backlight adjustable by LED class driver, say Y
+	  to enable this driver.
+
 endif # BACKLIGHT_CLASS_DEVICE
 
 endif # BACKLIGHT_LCD_SUPPORT
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index d67073f9d421..ecd321daee21 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
 obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
 obj-$(CONFIG_BACKLIGHT_TPS65217)	+= tps65217_bl.o
 obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
+obj-$(CONFIG_BACKLIGHT_LED)		+= led_bl.o
diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
new file mode 100644
index 000000000000..1befc8ce5964
--- /dev/null
+++ b/drivers/video/backlight/led_bl.c
@@ -0,0 +1,246 @@
+/*
+ * Copyright 2015 Texas Instruments
+ *
+ * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
+ *
+ * Based on pwm_bl.c
+ *
+ * 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; version 2 of the License.
+ */
+#include <linux/backlight.h>
+#include <linux/gpio/consumer.h>
+#include <linux/leds.h>
+#include <linux/of_leds.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+struct led_bl_data {
+	struct device		*dev;
+	struct backlight_device	*bl_dev;
+
+	unsigned int		*levels;
+	bool			enabled;
+	struct regulator	*power_supply;
+	struct gpio_desc	*enable_gpio;
+
+	struct led_classdev *led_cdev;
+
+	unsigned int max_brightness;
+	unsigned int default_brightness;
+};
+
+static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
+{
+	int err;
+
+	if (!priv->enabled) {
+		if (priv->power_supply) {
+			err = regulator_enable(priv->power_supply);
+
+			if (err < 0)
+				dev_err(priv->dev,
+					"failed to enable power supply\n");
+		}
+
+		if (priv->enable_gpio)
+			gpiod_set_value_cansleep(priv->enable_gpio, 1);
+	}
+
+	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
+
+	priv->enabled = true;
+}
+
+static void led_bl_power_off(struct led_bl_data *priv)
+{
+	if (!priv->enabled)
+		return;
+
+	led_set_brightness(priv->led_cdev, LED_OFF);
+
+	if (priv->enable_gpio)
+		gpiod_set_value_cansleep(priv->enable_gpio, 0);
+
+	if (priv->power_supply)
+		regulator_disable(priv->power_supply);
+
+	priv->enabled = false;
+}
+
+static int led_bl_update_status(struct backlight_device *bl)
+{
+	struct led_bl_data *priv = bl_get_data(bl);
+	int brightness = bl->props.brightness;
+
+	if (bl->props.power != FB_BLANK_UNBLANK ||
+	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
+	    bl->props.state & BL_CORE_FBBLANK)
+		brightness = 0;
+
+	if (brightness > 0)
+		led_bl_set_brightness(priv, brightness);
+	else
+		led_bl_power_off(priv);
+
+	return 0;
+}
+
+static const struct backlight_ops led_bl_ops = {
+	.update_status	= led_bl_update_status,
+};
+
+static int led_bl_parse_dt(struct device *dev,
+			   struct led_bl_data *priv)
+{
+	struct device_node *node = dev->of_node;
+	int num_levels;
+	u32 *levels;
+	u32 value;
+	int ret;
+
+	if (!node)
+		return -ENODEV;
+
+	num_levels = of_property_count_u32_elems(node, "brightness-levels");
+	if (num_levels < 0) {
+		dev_err(dev, "failed to find 'brightness-levels'\n");
+		return num_levels;
+	}
+
+	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
+	if (!levels)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(node, "brightness-levels",
+					 levels,
+					 num_levels);
+	if (ret < 0) {
+		dev_err(dev, "failed to parse 'brightness-levels'\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(node, "default-brightness-level", &value);
+	if (ret < 0) {
+		dev_err(dev, "failed to parse 'default-brightness-level'\n");
+		return ret;
+	}
+
+	if (value >= num_levels) {
+		dev_err(dev, "invalid default-brightness-level\n");
+		return -EINVAL;
+	}
+
+	priv->levels = levels;
+	priv->max_brightness = num_levels - 1;
+	priv->default_brightness = value;
+
+	priv->led_cdev = of_led_get(node);
+	if (IS_ERR(priv->led_cdev)) {
+		dev_err(dev, "failed to get LED device\n");
+		return PTR_ERR(priv->led_cdev);
+	}
+
+	return 0;
+}
+
+static int led_bl_probe(struct platform_device *pdev)
+{
+	struct backlight_properties props;
+	struct led_bl_data *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->dev = &pdev->dev;
+
+	ret = led_bl_parse_dt(&pdev->dev, priv);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to parse DT data\n");
+		return ret;
+	}
+
+	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
+			    GPIOD_OUT_LOW);
+	if (IS_ERR(priv->enable_gpio)) {
+		ret = PTR_ERR(priv->enable_gpio);
+		goto err;
+	}
+
+	priv->power_supply = devm_regulator_get_optional(&pdev->dev, "power");
+	if (IS_ERR(priv->power_supply)) {
+		ret = PTR_ERR(priv->power_supply);
+
+		if (ret == -ENODEV) {
+			priv->power_supply = NULL;
+		} else {
+			ret = PTR_ERR(priv->power_supply);
+			goto err;
+		}
+	}
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_RAW;
+	props.max_brightness = priv->max_brightness;
+	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
+			&pdev->dev, priv, &led_bl_ops, &props);
+	if (IS_ERR(priv->bl_dev)) {
+		dev_err(&pdev->dev, "failed to register backlight\n");
+		ret = PTR_ERR(priv->bl_dev);
+		goto err;
+	}
+
+	priv->bl_dev->props.brightness = priv->default_brightness;
+	backlight_update_status(priv->bl_dev);
+
+	return 0;
+
+err:
+	if (priv->led_cdev)
+		led_put(priv->led_cdev);
+
+	return ret;
+}
+
+static int led_bl_remove(struct platform_device *pdev)
+{
+	struct led_bl_data *priv = platform_get_drvdata(pdev);
+	struct backlight_device *bl = priv->bl_dev;
+
+	backlight_device_unregister(bl);
+
+	led_bl_power_off(priv);
+
+	led_put(priv->led_cdev);
+
+	return 0;
+}
+
+static const struct of_device_id led_bl_of_match[] = {
+	{ .compatible = "led-backlight" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, led_bl_of_match);
+
+static struct platform_driver led_bl_driver = {
+	.driver		= {
+		.name		= "led-backlight",
+		.of_match_table	= of_match_ptr(led_bl_of_match),
+	},
+	.probe		= led_bl_probe,
+	.remove		= led_bl_remove,
+};
+
+module_platform_driver(led_bl_driver);
+
+MODULE_DESCRIPTION("LED based Backlight Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:led-backlight");
-- 
2.1.4

[PATCHv4 2/3] backlight: add led-backlight driver

[Tomi Valkeinen]

This patch adds a led-backlight driver (led_bl), which is similar to
pwm_bl except the driver uses a LED class driver to adjust the
brightness in the HW.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/Kconfig  |   7 ++
 drivers/video/backlight/Makefile |   1 +
 drivers/video/backlight/led_bl.c | 246 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 254 insertions(+)
 create mode 100644 drivers/video/backlight/led_bl.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 0505b796d743..d1336196aba2 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -453,6 +453,13 @@ config BACKLIGHT_BD6107
 	help
 	  If you have a Rohm BD6107 say Y to enable the backlight driver.
 
+config BACKLIGHT_LED
+	tristate "Generic LED based Backlight Driver"
+	depends on LEDS_CLASS && OF
+	help
+	  If you have a LCD backlight adjustable by LED class driver, say Y
+	  to enable this driver.
+
 endif # BACKLIGHT_CLASS_DEVICE
 
 endif # BACKLIGHT_LCD_SUPPORT
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index d67073f9d421..ecd321daee21 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
 obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
 obj-$(CONFIG_BACKLIGHT_TPS65217)	+= tps65217_bl.o
 obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
+obj-$(CONFIG_BACKLIGHT_LED)		+= led_bl.o
diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
new file mode 100644
index 000000000000..1befc8ce5964
--- /dev/null
+++ b/drivers/video/backlight/led_bl.c
@@ -0,0 +1,246 @@
+/*
+ * Copyright 2015 Texas Instruments
+ *
+ * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
+ *
+ * Based on pwm_bl.c
+ *
+ * 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; version 2 of the License.
+ */
+#include <linux/backlight.h>
+#include <linux/gpio/consumer.h>
+#include <linux/leds.h>
+#include <linux/of_leds.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+struct led_bl_data {
+	struct device		*dev;
+	struct backlight_device	*bl_dev;
+
+	unsigned int		*levels;
+	bool			enabled;
+	struct regulator	*power_supply;
+	struct gpio_desc	*enable_gpio;
+
+	struct led_classdev *led_cdev;
+
+	unsigned int max_brightness;
+	unsigned int default_brightness;
+};
+
+static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
+{
+	int err;
+
+	if (!priv->enabled) {
+		if (priv->power_supply) {
+			err = regulator_enable(priv->power_supply);
+
+			if (err < 0)
+				dev_err(priv->dev,
+					"failed to enable power supply\n");
+		}
+
+		if (priv->enable_gpio)
+			gpiod_set_value_cansleep(priv->enable_gpio, 1);
+	}
+
+	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
+
+	priv->enabled = true;
+}
+
+static void led_bl_power_off(struct led_bl_data *priv)
+{
+	if (!priv->enabled)
+		return;
+
+	led_set_brightness(priv->led_cdev, LED_OFF);
+
+	if (priv->enable_gpio)
+		gpiod_set_value_cansleep(priv->enable_gpio, 0);
+
+	if (priv->power_supply)
+		regulator_disable(priv->power_supply);
+
+	priv->enabled = false;
+}
+
+static int led_bl_update_status(struct backlight_device *bl)
+{
+	struct led_bl_data *priv = bl_get_data(bl);
+	int brightness = bl->props.brightness;
+
+	if (bl->props.power != FB_BLANK_UNBLANK ||
+	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
+	    bl->props.state & BL_CORE_FBBLANK)
+		brightness = 0;
+
+	if (brightness > 0)
+		led_bl_set_brightness(priv, brightness);
+	else
+		led_bl_power_off(priv);
+
+	return 0;
+}
+
+static const struct backlight_ops led_bl_ops = {
+	.update_status	= led_bl_update_status,
+};
+
+static int led_bl_parse_dt(struct device *dev,
+			   struct led_bl_data *priv)
+{
+	struct device_node *node = dev->of_node;
+	int num_levels;
+	u32 *levels;
+	u32 value;
+	int ret;
+
+	if (!node)
+		return -ENODEV;
+
+	num_levels = of_property_count_u32_elems(node, "brightness-levels");
+	if (num_levels < 0) {
+		dev_err(dev, "failed to find 'brightness-levels'\n");
+		return num_levels;
+	}
+
+	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
+	if (!levels)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(node, "brightness-levels",
+					 levels,
+					 num_levels);
+	if (ret < 0) {
+		dev_err(dev, "failed to parse 'brightness-levels'\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(node, "default-brightness-level", &value);
+	if (ret < 0) {
+		dev_err(dev, "failed to parse 'default-brightness-level'\n");
+		return ret;
+	}
+
+	if (value >= num_levels) {
+		dev_err(dev, "invalid default-brightness-level\n");
+		return -EINVAL;
+	}
+
+	priv->levels = levels;
+	priv->max_brightness = num_levels - 1;
+	priv->default_brightness = value;
+
+	priv->led_cdev = of_led_get(node);
+	if (IS_ERR(priv->led_cdev)) {
+		dev_err(dev, "failed to get LED device\n");
+		return PTR_ERR(priv->led_cdev);
+	}
+
+	return 0;
+}
+
+static int led_bl_probe(struct platform_device *pdev)
+{
+	struct backlight_properties props;
+	struct led_bl_data *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->dev = &pdev->dev;
+
+	ret = led_bl_parse_dt(&pdev->dev, priv);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to parse DT data\n");
+		return ret;
+	}
+
+	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
+			    GPIOD_OUT_LOW);
+	if (IS_ERR(priv->enable_gpio)) {
+		ret = PTR_ERR(priv->enable_gpio);
+		goto err;
+	}
+
+	priv->power_supply = devm_regulator_get_optional(&pdev->dev, "power");
+	if (IS_ERR(priv->power_supply)) {
+		ret = PTR_ERR(priv->power_supply);
+
+		if (ret = -ENODEV) {
+			priv->power_supply = NULL;
+		} else {
+			ret = PTR_ERR(priv->power_supply);
+			goto err;
+		}
+	}
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_RAW;
+	props.max_brightness = priv->max_brightness;
+	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
+			&pdev->dev, priv, &led_bl_ops, &props);
+	if (IS_ERR(priv->bl_dev)) {
+		dev_err(&pdev->dev, "failed to register backlight\n");
+		ret = PTR_ERR(priv->bl_dev);
+		goto err;
+	}
+
+	priv->bl_dev->props.brightness = priv->default_brightness;
+	backlight_update_status(priv->bl_dev);
+
+	return 0;
+
+err:
+	if (priv->led_cdev)
+		led_put(priv->led_cdev);
+
+	return ret;
+}
+
+static int led_bl_remove(struct platform_device *pdev)
+{
+	struct led_bl_data *priv = platform_get_drvdata(pdev);
+	struct backlight_device *bl = priv->bl_dev;
+
+	backlight_device_unregister(bl);
+
+	led_bl_power_off(priv);
+
+	led_put(priv->led_cdev);
+
+	return 0;
+}
+
+static const struct of_device_id led_bl_of_match[] = {
+	{ .compatible = "led-backlight" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, led_bl_of_match);
+
+static struct platform_driver led_bl_driver = {
+	.driver		= {
+		.name		= "led-backlight",
+		.of_match_table	= of_match_ptr(led_bl_of_match),
+	},
+	.probe		= led_bl_probe,
+	.remove		= led_bl_remove,
+};
+
+module_platform_driver(led_bl_driver);
+
+MODULE_DESCRIPTION("LED based Backlight Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:led-backlight");
-- 
2.1.4

[PATCHv4 3/3] devicetree: Add led-backlight binding

[Tomi Valkeinen]

Add DT binding for led-backlight.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: devicetree@vger.kernel.org
---
 .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt

diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
new file mode 100644
index 000000000000..d4621d7414bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
@@ -0,0 +1,30 @@
+led-backlight bindings
+
+Required properties:
+  - compatible: "led-backlight"
+  - leds: phandle to a led OF node [0]
+  - brightness-levels: Array of distinct LED brightness levels. These
+      are in the range from 0 to 255, passed to the LED class driver.
+  - default-brightness-level: the default brightness level (index into the
+      array defined by the "brightness-levels" property)
+
+Optional properties:
+  - power-supply: regulator for supply voltage
+  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
+                  and disables the backlight (see GPIO binding[1])
+
+[0]: Documentation/devicetree/bindings/leds/common.txt
+[1]: Documentation/devicetree/bindings/gpio/gpio.txt
+
+Example:
+
+	backlight {
+		compatible = "led-backlight";
+		leds = <&backlight_led>;
+
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <6>;
+
+		power-supply = <&vdd_bl_reg>;
+		enable-gpios = <&gpio 58 0>;
+	};
-- 
2.1.4

[PATCHv4 3/3] devicetree: Add led-backlight binding

[Tomi Valkeinen]

Add DT binding for led-backlight.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: devicetree@vger.kernel.org
---
 .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt

diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
new file mode 100644
index 000000000000..d4621d7414bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
@@ -0,0 +1,30 @@
+led-backlight bindings
+
+Required properties:
+  - compatible: "led-backlight"
+  - leds: phandle to a led OF node [0]
+  - brightness-levels: Array of distinct LED brightness levels. These
+      are in the range from 0 to 255, passed to the LED class driver.
+  - default-brightness-level: the default brightness level (index into the
+      array defined by the "brightness-levels" property)
+
+Optional properties:
+  - power-supply: regulator for supply voltage
+  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
+                  and disables the backlight (see GPIO binding[1])
+
+[0]: Documentation/devicetree/bindings/leds/common.txt
+[1]: Documentation/devicetree/bindings/gpio/gpio.txt
+
+Example:
+
+	backlight {
+		compatible = "led-backlight";
+		leds = <&backlight_led>;
+
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <6>;
+
+		power-supply = <&vdd_bl_reg>;
+		enable-gpios = <&gpio 58 0>;
+	};
-- 
2.1.4

Re: [PATCHv4 1/3] leds: Add of_led_get() and led_put()

[Jacek Anaszewski]

Hi Tomi,

Thanks for the update.

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

On 09/30/2015 11:32 AM, Tomi Valkeinen wrote:
> This patch adds basic support for a kernel driver to get a LED device.
> This will be used by the led-backlight driver.
>
> Only OF version is implemented for now, and the behavior is similar to
> PWM's of_pwm_get() and pwm_put().
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/leds/Makefile    |  6 +++-
>   drivers/leds/led-class.c | 13 +++++++-
>   drivers/leds/led-of.c    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/leds/leds.h      |  1 +
>   include/linux/leds.h     | 10 ++++++
>   include/linux/of_leds.h  | 26 +++++++++++++++
>   6 files changed, 139 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/leds/led-of.c
>   create mode 100644 include/linux/of_leds.h
>
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8d6a24a2f513..6fd22e411810 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -1,7 +1,11 @@
>
>   # LED Core
>   obj-$(CONFIG_NEW_LEDS)			+= led-core.o
> -obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
> +
> +obj-$(CONFIG_LEDS_CLASS)		+= led-class-objs.o
> +led-class-objs-y			:= led-class.o
> +led-class-objs-$(CONFIG_OF)		+= led-of.o
> +
>   obj-$(CONFIG_LEDS_CLASS_FLASH)		+= led-class-flash.o
>   obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index beabfbc6f7cd..ff0c27654358 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -22,7 +22,7 @@
>   #include <linux/timer.h>
>   #include "leds.h"
>
> -static struct class *leds_class;
> +struct class *leds_class;
>
>   static ssize_t brightness_show(struct device *dev,
>   		struct device_attribute *attr, char *buf)
> @@ -216,6 +216,17 @@ static int led_resume(struct device *dev)
>
>   static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
>
> +/**
> + * led_put() - release a LED device, reserved with of_led_get()
> + * @led_cdev: LED device
> + */
> +void led_put(struct led_classdev *led_cdev)
> +{
> +	put_device(led_cdev->dev);
> +	module_put(led_cdev->dev->parent->driver->owner);
> +}
> +EXPORT_SYMBOL_GPL(led_put);
> +
>   static int match_name(struct device *dev, const void *data)
>   {
>   	if (!dev_name(dev))
> diff --git a/drivers/leds/led-of.c b/drivers/leds/led-of.c
> new file mode 100644
> index 000000000000..6e96fee9adf1
> --- /dev/null
> +++ b/drivers/leds/led-of.c
> @@ -0,0 +1,85 @@
> +/*
> + * LED Class Core OF support
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_leds.h>
> +
> +#include "leds.h"
> +
> +/* find OF node for the given led_cdev */
> +static struct device_node *find_led_of_node(struct led_classdev *led_cdev)
> +{
> +	struct device *led_dev = led_cdev->dev;
> +	struct device_node *child;
> +
> +	for_each_child_of_node(led_dev->parent->of_node, child) {
> +		int idx;
> +
> +		idx = of_property_match_string(child, "label", led_cdev->name);
> +		if (idx == 0)
> +			return child;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int led_match_led_node(struct device *led_dev, const void *data)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(led_dev);
> +	const struct device_node *target_node = data;
> +	struct device_node *led_node;
> +
> +	led_node = find_led_of_node(led_cdev);
> +	if (!led_node)
> +		return 0;
> +
> +	of_node_put(led_node);
> +
> +	return led_node == target_node;
> +}
> +
> +/**
> + * of_led_get() - request a LED device via the LED framework
> + * @np: device node to get the LED device from
> + *
> + * Returns the LED device parsed from the phandle specified in the "leds"
> + * property of a device tree node or a negative error-code on failure.
> + *
> + * The caller must use led_put() to release the device after use.
> + */
> +struct led_classdev *of_led_get(struct device_node *np)
> +{
> +	struct device *led_dev;
> +	struct led_classdev *led_cdev;
> +	struct device_node *led_node;
> +
> +	led_node = of_parse_phandle(np, "leds", 0);
> +	if (!led_node)
> +		return ERR_PTR(-ENODEV);
> +
> +	led_dev = class_find_device(leds_class, NULL, led_node,
> +		led_match_led_node);
> +
> +	of_node_put(led_node);
> +
> +	if (!led_dev) {
> +		pr_err("failed to find led device for node %s, deferring probe\n",
> +			of_node_full_name(led_node));
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	led_cdev = dev_get_drvdata(led_dev);
> +
> +	if (!try_module_get(led_cdev->dev->parent->driver->owner))
> +		return ERR_PTR(-ENODEV);
> +
> +	return led_cdev;
> +}
> +EXPORT_SYMBOL_GPL(of_led_get);
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index bc89d7ace2c4..ccc3abb417d4 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -46,6 +46,7 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
>
>   void led_stop_software_blink(struct led_classdev *led_cdev);
>
> +extern struct class *leds_class;
>   extern struct rw_semaphore leds_list_lock;
>   extern struct list_head leds_list;
>
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b122eeafb5dc..fbad4ce78e6e 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -113,6 +113,16 @@ extern void devm_led_classdev_unregister(struct device *parent,
>   extern void led_classdev_suspend(struct led_classdev *led_cdev);
>   extern void led_classdev_resume(struct led_classdev *led_cdev);
>
> +#if IS_ENABLED(CONFIG_LEDS_CLASS)
> +
> +extern void led_put(struct led_classdev *led_cdev);
> +
> +#else
> +
> +static inline void led_put(struct led_classdev *led_cdev) { }
> +
> +#endif /* IS_ENABLED(CONFIG_LEDS_CLASS) */
> +
>   /**
>    * led_blink_set - set blinking with software fallback
>    * @led_cdev: the LED to start blinking
> diff --git a/include/linux/of_leds.h b/include/linux/of_leds.h
> new file mode 100644
> index 000000000000..7e8e64bd9811
> --- /dev/null
> +++ b/include/linux/of_leds.h
> @@ -0,0 +1,26 @@
> +/*
> + * OF support for leds
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#ifndef __LINUX_LEDS_OF_H_INCLUDED
> +#define __LINUX_LEDS_OF_H_INCLUDED
> +
> +#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_LEDS_CLASS)
> +
> +extern struct led_classdev *of_led_get(struct device_node *np);
> +
> +#else
> +
> +static inline struct led_classdev *of_led_get(struct device_node *np)
> +{
> +	return -ENODEV;
> +}
> +
> +#endif
> +
> +#endif /* __LINUX_LEDS_OF_H_INCLUDED */
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCHv4 1/3] leds: Add of_led_get() and led_put()

[Jacek Anaszewski]

Hi Tomi,

Thanks for the update.

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

On 09/30/2015 11:32 AM, Tomi Valkeinen wrote:
> This patch adds basic support for a kernel driver to get a LED device.
> This will be used by the led-backlight driver.
>
> Only OF version is implemented for now, and the behavior is similar to
> PWM's of_pwm_get() and pwm_put().
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/leds/Makefile    |  6 +++-
>   drivers/leds/led-class.c | 13 +++++++-
>   drivers/leds/led-of.c    | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
>   drivers/leds/leds.h      |  1 +
>   include/linux/leds.h     | 10 ++++++
>   include/linux/of_leds.h  | 26 +++++++++++++++
>   6 files changed, 139 insertions(+), 2 deletions(-)
>   create mode 100644 drivers/leds/led-of.c
>   create mode 100644 include/linux/of_leds.h
>
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 8d6a24a2f513..6fd22e411810 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -1,7 +1,11 @@
>
>   # LED Core
>   obj-$(CONFIG_NEW_LEDS)			+= led-core.o
> -obj-$(CONFIG_LEDS_CLASS)		+= led-class.o
> +
> +obj-$(CONFIG_LEDS_CLASS)		+= led-class-objs.o
> +led-class-objs-y			:= led-class.o
> +led-class-objs-$(CONFIG_OF)		+= led-of.o
> +
>   obj-$(CONFIG_LEDS_CLASS_FLASH)		+= led-class-flash.o
>   obj-$(CONFIG_LEDS_TRIGGERS)		+= led-triggers.o
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index beabfbc6f7cd..ff0c27654358 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -22,7 +22,7 @@
>   #include <linux/timer.h>
>   #include "leds.h"
>
> -static struct class *leds_class;
> +struct class *leds_class;
>
>   static ssize_t brightness_show(struct device *dev,
>   		struct device_attribute *attr, char *buf)
> @@ -216,6 +216,17 @@ static int led_resume(struct device *dev)
>
>   static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
>
> +/**
> + * led_put() - release a LED device, reserved with of_led_get()
> + * @led_cdev: LED device
> + */
> +void led_put(struct led_classdev *led_cdev)
> +{
> +	put_device(led_cdev->dev);
> +	module_put(led_cdev->dev->parent->driver->owner);
> +}
> +EXPORT_SYMBOL_GPL(led_put);
> +
>   static int match_name(struct device *dev, const void *data)
>   {
>   	if (!dev_name(dev))
> diff --git a/drivers/leds/led-of.c b/drivers/leds/led-of.c
> new file mode 100644
> index 000000000000..6e96fee9adf1
> --- /dev/null
> +++ b/drivers/leds/led-of.c
> @@ -0,0 +1,85 @@
> +/*
> + * LED Class Core OF support
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_leds.h>
> +
> +#include "leds.h"
> +
> +/* find OF node for the given led_cdev */
> +static struct device_node *find_led_of_node(struct led_classdev *led_cdev)
> +{
> +	struct device *led_dev = led_cdev->dev;
> +	struct device_node *child;
> +
> +	for_each_child_of_node(led_dev->parent->of_node, child) {
> +		int idx;
> +
> +		idx = of_property_match_string(child, "label", led_cdev->name);
> +		if (idx = 0)
> +			return child;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int led_match_led_node(struct device *led_dev, const void *data)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(led_dev);
> +	const struct device_node *target_node = data;
> +	struct device_node *led_node;
> +
> +	led_node = find_led_of_node(led_cdev);
> +	if (!led_node)
> +		return 0;
> +
> +	of_node_put(led_node);
> +
> +	return led_node = target_node;
> +}
> +
> +/**
> + * of_led_get() - request a LED device via the LED framework
> + * @np: device node to get the LED device from
> + *
> + * Returns the LED device parsed from the phandle specified in the "leds"
> + * property of a device tree node or a negative error-code on failure.
> + *
> + * The caller must use led_put() to release the device after use.
> + */
> +struct led_classdev *of_led_get(struct device_node *np)
> +{
> +	struct device *led_dev;
> +	struct led_classdev *led_cdev;
> +	struct device_node *led_node;
> +
> +	led_node = of_parse_phandle(np, "leds", 0);
> +	if (!led_node)
> +		return ERR_PTR(-ENODEV);
> +
> +	led_dev = class_find_device(leds_class, NULL, led_node,
> +		led_match_led_node);
> +
> +	of_node_put(led_node);
> +
> +	if (!led_dev) {
> +		pr_err("failed to find led device for node %s, deferring probe\n",
> +			of_node_full_name(led_node));
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	led_cdev = dev_get_drvdata(led_dev);
> +
> +	if (!try_module_get(led_cdev->dev->parent->driver->owner))
> +		return ERR_PTR(-ENODEV);
> +
> +	return led_cdev;
> +}
> +EXPORT_SYMBOL_GPL(of_led_get);
> diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
> index bc89d7ace2c4..ccc3abb417d4 100644
> --- a/drivers/leds/leds.h
> +++ b/drivers/leds/leds.h
> @@ -46,6 +46,7 @@ static inline int led_get_brightness(struct led_classdev *led_cdev)
>
>   void led_stop_software_blink(struct led_classdev *led_cdev);
>
> +extern struct class *leds_class;
>   extern struct rw_semaphore leds_list_lock;
>   extern struct list_head leds_list;
>
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b122eeafb5dc..fbad4ce78e6e 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -113,6 +113,16 @@ extern void devm_led_classdev_unregister(struct device *parent,
>   extern void led_classdev_suspend(struct led_classdev *led_cdev);
>   extern void led_classdev_resume(struct led_classdev *led_cdev);
>
> +#if IS_ENABLED(CONFIG_LEDS_CLASS)
> +
> +extern void led_put(struct led_classdev *led_cdev);
> +
> +#else
> +
> +static inline void led_put(struct led_classdev *led_cdev) { }
> +
> +#endif /* IS_ENABLED(CONFIG_LEDS_CLASS) */
> +
>   /**
>    * led_blink_set - set blinking with software fallback
>    * @led_cdev: the LED to start blinking
> diff --git a/include/linux/of_leds.h b/include/linux/of_leds.h
> new file mode 100644
> index 000000000000..7e8e64bd9811
> --- /dev/null
> +++ b/include/linux/of_leds.h
> @@ -0,0 +1,26 @@
> +/*
> + * OF support for leds
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#ifndef __LINUX_LEDS_OF_H_INCLUDED
> +#define __LINUX_LEDS_OF_H_INCLUDED
> +
> +#if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_LEDS_CLASS)
> +
> +extern struct led_classdev *of_led_get(struct device_node *np);
> +
> +#else
> +
> +static inline struct led_classdev *of_led_get(struct device_node *np)
> +{
> +	return -ENODEV;
> +}
> +
> +#endif
> +
> +#endif /* __LINUX_LEDS_OF_H_INCLUDED */
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCHv4 0/3] backlight: led-backlight driver

[Tomi Valkeinen]

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

Hi,

On 30/09/15 12:31, Tomi Valkeinen wrote:
> This series aims to add a led-backlight driver, similar to pwm-backlight, but
> using a LED class device underneath.
> 
> LED framework has no support for DT or getting a LED class driver from another
> kernel driver, so I added minimal functionality to led-class to get
> led-backlight working.
> 
> Changes to v3:
> - Change a comment to refer to of_led_get()
> 
> Changes to v2:
> - power supply is now optional
> - cosmetic changes
> - no-op function for led_put() when !CONFIG_LEDS_CLASS
> 
> Changes to v1:
> - Split LED OF parts into separate .h and .c files
> - Check for CONFIG_OF and CONFIG_LEDS_CLASS where relevant to leave unused code
>   out.
> - Improved error prints and comments a bit
> - Added put_device() into led_put(), as the device was gotten from
>   class_find_device() which requires a put_device() call.
> 
>  Tomi
> 
> Tomi Valkeinen (3):
>   leds: Add of_led_get() and led_put()
>   backlight: add led-backlight driver
>   devicetree: Add led-backlight binding
> 
>  .../bindings/video/backlight/led-backlight.txt     |  30 +++
>  drivers/leds/Makefile                              |   6 +-
>  drivers/leds/led-class.c                           |  13 +-
>  drivers/leds/led-of.c                              |  85 +++++++
>  drivers/leds/leds.h                                |   1 +
>  drivers/video/backlight/Kconfig                    |   7 +
>  drivers/video/backlight/Makefile                   |   1 +
>  drivers/video/backlight/led_bl.c                   | 246 +++++++++++++++++++++
>  include/linux/leds.h                               |  10 +
>  include/linux/of_leds.h                            |  26 +++
>  10 files changed, 423 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>  create mode 100644 drivers/leds/led-of.c
>  create mode 100644 drivers/video/backlight/led_bl.c
>  create mode 100644 include/linux/of_leds.h

There's been no more comments on this. Should this be merged via led or
backlight trees?

Possible conflicts probably happen on the led side, as this changes the
led core files, so perhaps that's easier way?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv4 0/3] backlight: led-backlight driver

[Tomi Valkeinen]

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

Hi,

On 30/09/15 12:31, Tomi Valkeinen wrote:
> This series aims to add a led-backlight driver, similar to pwm-backlight, but
> using a LED class device underneath.
> 
> LED framework has no support for DT or getting a LED class driver from another
> kernel driver, so I added minimal functionality to led-class to get
> led-backlight working.
> 
> Changes to v3:
> - Change a comment to refer to of_led_get()
> 
> Changes to v2:
> - power supply is now optional
> - cosmetic changes
> - no-op function for led_put() when !CONFIG_LEDS_CLASS
> 
> Changes to v1:
> - Split LED OF parts into separate .h and .c files
> - Check for CONFIG_OF and CONFIG_LEDS_CLASS where relevant to leave unused code
>   out.
> - Improved error prints and comments a bit
> - Added put_device() into led_put(), as the device was gotten from
>   class_find_device() which requires a put_device() call.
> 
>  Tomi
> 
> Tomi Valkeinen (3):
>   leds: Add of_led_get() and led_put()
>   backlight: add led-backlight driver
>   devicetree: Add led-backlight binding
> 
>  .../bindings/video/backlight/led-backlight.txt     |  30 +++
>  drivers/leds/Makefile                              |   6 +-
>  drivers/leds/led-class.c                           |  13 +-
>  drivers/leds/led-of.c                              |  85 +++++++
>  drivers/leds/leds.h                                |   1 +
>  drivers/video/backlight/Kconfig                    |   7 +
>  drivers/video/backlight/Makefile                   |   1 +
>  drivers/video/backlight/led_bl.c                   | 246 +++++++++++++++++++++
>  include/linux/leds.h                               |  10 +
>  include/linux/of_leds.h                            |  26 +++
>  10 files changed, 423 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>  create mode 100644 drivers/leds/led-of.c
>  create mode 100644 drivers/video/backlight/led_bl.c
>  create mode 100644 include/linux/of_leds.h

There's been no more comments on this. Should this be merged via led or
backlight trees?

Possible conflicts probably happen on the led side, as this changes the
led core files, so perhaps that's easier way?

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv4 0/3] backlight: led-backlight driver

[Jacek Anaszewski]

On 10/08/2015 11:35 AM, Tomi Valkeinen wrote:
> Hi,
>
> On 30/09/15 12:31, Tomi Valkeinen wrote:
>> This series aims to add a led-backlight driver, similar to pwm-backlight, but
>> using a LED class device underneath.
>>
>> LED framework has no support for DT or getting a LED class driver from another
>> kernel driver, so I added minimal functionality to led-class to get
>> led-backlight working.
>>
>> Changes to v3:
>> - Change a comment to refer to of_led_get()
>>
>> Changes to v2:
>> - power supply is now optional
>> - cosmetic changes
>> - no-op function for led_put() when !CONFIG_LEDS_CLASS
>>
>> Changes to v1:
>> - Split LED OF parts into separate .h and .c files
>> - Check for CONFIG_OF and CONFIG_LEDS_CLASS where relevant to leave unused code
>>    out.
>> - Improved error prints and comments a bit
>> - Added put_device() into led_put(), as the device was gotten from
>>    class_find_device() which requires a put_device() call.
>>
>>   Tomi
>>
>> Tomi Valkeinen (3):
>>    leds: Add of_led_get() and led_put()
>>    backlight: add led-backlight driver
>>    devicetree: Add led-backlight binding
>>
>>   .../bindings/video/backlight/led-backlight.txt     |  30 +++
>>   drivers/leds/Makefile                              |   6 +-
>>   drivers/leds/led-class.c                           |  13 +-
>>   drivers/leds/led-of.c                              |  85 +++++++
>>   drivers/leds/leds.h                                |   1 +
>>   drivers/video/backlight/Kconfig                    |   7 +
>>   drivers/video/backlight/Makefile                   |   1 +
>>   drivers/video/backlight/led_bl.c                   | 246 +++++++++++++++++++++
>>   include/linux/leds.h                               |  10 +
>>   include/linux/of_leds.h                            |  26 +++
>>   10 files changed, 423 insertions(+), 2 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>   create mode 100644 drivers/leds/led-of.c
>>   create mode 100644 drivers/video/backlight/led_bl.c
>>   create mode 100644 include/linux/of_leds.h
>
> There's been no more comments on this. Should this be merged via led or
> backlight trees?
>
> Possible conflicts probably happen on the led side, as this changes the
> led core files, so perhaps that's easier way?

Yes, it would be better to merge it via LED tree, also because the
driver being added depends on drivers/leds/led-of.c which is introduced
in this set. Obviously I need backlight maintainer's ack. Jingoo, Lee,
are you ok with this changes?

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCHv4 0/3] backlight: led-backlight driver

[Jacek Anaszewski]

On 10/08/2015 11:35 AM, Tomi Valkeinen wrote:
> Hi,
>
> On 30/09/15 12:31, Tomi Valkeinen wrote:
>> This series aims to add a led-backlight driver, similar to pwm-backlight, but
>> using a LED class device underneath.
>>
>> LED framework has no support for DT or getting a LED class driver from another
>> kernel driver, so I added minimal functionality to led-class to get
>> led-backlight working.
>>
>> Changes to v3:
>> - Change a comment to refer to of_led_get()
>>
>> Changes to v2:
>> - power supply is now optional
>> - cosmetic changes
>> - no-op function for led_put() when !CONFIG_LEDS_CLASS
>>
>> Changes to v1:
>> - Split LED OF parts into separate .h and .c files
>> - Check for CONFIG_OF and CONFIG_LEDS_CLASS where relevant to leave unused code
>>    out.
>> - Improved error prints and comments a bit
>> - Added put_device() into led_put(), as the device was gotten from
>>    class_find_device() which requires a put_device() call.
>>
>>   Tomi
>>
>> Tomi Valkeinen (3):
>>    leds: Add of_led_get() and led_put()
>>    backlight: add led-backlight driver
>>    devicetree: Add led-backlight binding
>>
>>   .../bindings/video/backlight/led-backlight.txt     |  30 +++
>>   drivers/leds/Makefile                              |   6 +-
>>   drivers/leds/led-class.c                           |  13 +-
>>   drivers/leds/led-of.c                              |  85 +++++++
>>   drivers/leds/leds.h                                |   1 +
>>   drivers/video/backlight/Kconfig                    |   7 +
>>   drivers/video/backlight/Makefile                   |   1 +
>>   drivers/video/backlight/led_bl.c                   | 246 +++++++++++++++++++++
>>   include/linux/leds.h                               |  10 +
>>   include/linux/of_leds.h                            |  26 +++
>>   10 files changed, 423 insertions(+), 2 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>   create mode 100644 drivers/leds/led-of.c
>>   create mode 100644 drivers/video/backlight/led_bl.c
>>   create mode 100644 include/linux/of_leds.h
>
> There's been no more comments on this. Should this be merged via led or
> backlight trees?
>
> Possible conflicts probably happen on the led side, as this changes the
> led core files, so perhaps that's easier way?

Yes, it would be better to merge it via LED tree, also because the
driver being added depends on drivers/leds/led-of.c which is introduced
in this set. Obviously I need backlight maintainer's ack. Jingoo, Lee,
are you ok with this changes?

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Lee Jones]

On Wed, 30 Sep 2015, Tomi Valkeinen wrote:

> Add DT binding for led-backlight.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> new file mode 100644
> index 000000000000..d4621d7414bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> @@ -0,0 +1,30 @@
> +led-backlight bindings

Make this look like a heading, rather than just another binding.

I would expect to see "LED Backlight Bindings" or similar.

> +Required properties:
> +  - compatible: "led-backlight"
> +  - leds: phandle to a led OF node [0]

s/phandle/Phandle/

s/led/LED/

s/[0]/(See: ../leds/common.txt)

> +  - brightness-levels: Array of distinct LED brightness levels. These
> +      are in the range from 0 to 255, passed to the LED class driver.
> +  - default-brightness-level: the default brightness level (index into the

s/the/The/

> +      array defined by the "brightness-levels" property)

Tab this out, so:

- compatible			: "led-backlight"
- leds				: phandle to a led OF node [0]
- brightness-levels		: Array of distinct LED brightness levels. These
				  are in the range from 0 to 255, passed to the LED class driver.
- default-brightness-level	: the default brightness level (index into the
				  array defined by the "brightness-levels" property)

Etc.

> +Optional properties:
> +  - power-supply: regulator for supply voltage

s/regulator/Regulator/

> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables

s/contains/Contains/

> +                  and disables the backlight (see GPIO binding[1])

s/[1]/(See: ../gpio/gpio.txt)

> +[0]: Documentation/devicetree/bindings/leds/common.txt
> +[1]: Documentation/devicetree/bindings/gpio/gpio.txt

Remove these.

> +Example:
> +
> +	backlight {
> +		compatible = "led-backlight";
> +		leds = <&backlight_led>;
> +
> +		brightness-levels = <0 4 8 16 32 64 128 255>;
> +		default-brightness-level = <6>;
> +
> +		power-supply = <&vdd_bl_reg>;
> +		enable-gpios = <&gpio 58 0>;
> +	};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Lee Jones]

On Wed, 30 Sep 2015, Tomi Valkeinen wrote:

> Add DT binding for led-backlight.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> 
> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> new file mode 100644
> index 000000000000..d4621d7414bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> @@ -0,0 +1,30 @@
> +led-backlight bindings

Make this look like a heading, rather than just another binding.

I would expect to see "LED Backlight Bindings" or similar.

> +Required properties:
> +  - compatible: "led-backlight"
> +  - leds: phandle to a led OF node [0]

s/phandle/Phandle/

s/led/LED/

s/[0]/(See: ../leds/common.txt)

> +  - brightness-levels: Array of distinct LED brightness levels. These
> +      are in the range from 0 to 255, passed to the LED class driver.
> +  - default-brightness-level: the default brightness level (index into the

s/the/The/

> +      array defined by the "brightness-levels" property)

Tab this out, so:

- compatible			: "led-backlight"
- leds				: phandle to a led OF node [0]
- brightness-levels		: Array of distinct LED brightness levels. These
				  are in the range from 0 to 255, passed to the LED class driver.
- default-brightness-level	: the default brightness level (index into the
				  array defined by the "brightness-levels" property)

Etc.

> +Optional properties:
> +  - power-supply: regulator for supply voltage

s/regulator/Regulator/

> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables

s/contains/Contains/

> +                  and disables the backlight (see GPIO binding[1])

s/[1]/(See: ../gpio/gpio.txt)

> +[0]: Documentation/devicetree/bindings/leds/common.txt
> +[1]: Documentation/devicetree/bindings/gpio/gpio.txt

Remove these.

> +Example:
> +
> +	backlight {
> +		compatible = "led-backlight";
> +		leds = <&backlight_led>;
> +
> +		brightness-levels = <0 4 8 16 32 64 128 255>;
> +		default-brightness-level = <6>;
> +
> +		power-supply = <&vdd_bl_reg>;
> +		enable-gpios = <&gpio 58 0>;
> +	};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCHv4 2/3] backlight: add led-backlight driver

[Lee Jones]

Jingoo?

On Wed, 30 Sep 2015, Tomi Valkeinen wrote:
> This patch adds a led-backlight driver (led_bl), which is similar to
> pwm_bl except the driver uses a LED class driver to adjust the
> brightness in the HW.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/video/backlight/Kconfig  |   7 ++
>  drivers/video/backlight/Makefile |   1 +
>  drivers/video/backlight/led_bl.c | 246 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 254 insertions(+)
>  create mode 100644 drivers/video/backlight/led_bl.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 0505b796d743..d1336196aba2 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -453,6 +453,13 @@ config BACKLIGHT_BD6107
>  	help
>  	  If you have a Rohm BD6107 say Y to enable the backlight driver.
>  
> +config BACKLIGHT_LED
> +	tristate "Generic LED based Backlight Driver"
> +	depends on LEDS_CLASS && OF
> +	help
> +	  If you have a LCD backlight adjustable by LED class driver, say Y
> +	  to enable this driver.
> +
>  endif # BACKLIGHT_CLASS_DEVICE
>  
>  endif # BACKLIGHT_LCD_SUPPORT
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index d67073f9d421..ecd321daee21 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -54,3 +54,4 @@ obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
>  obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
>  obj-$(CONFIG_BACKLIGHT_TPS65217)	+= tps65217_bl.o
>  obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
> +obj-$(CONFIG_BACKLIGHT_LED)		+= led_bl.o
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> new file mode 100644
> index 000000000000..1befc8ce5964
> --- /dev/null
> +++ b/drivers/video/backlight/led_bl.c
> @@ -0,0 +1,246 @@
> +/*
> + * Copyright 2015 Texas Instruments
> + *
> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> + *
> + * Based on pwm_bl.c
> + *
> + * 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; version 2 of the License.
> + */
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/leds.h>
> +#include <linux/of_leds.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +struct led_bl_data {
> +	struct device		*dev;
> +	struct backlight_device	*bl_dev;
> +
> +	unsigned int		*levels;
> +	bool			enabled;
> +	struct regulator	*power_supply;
> +	struct gpio_desc	*enable_gpio;
> +
> +	struct led_classdev *led_cdev;
> +
> +	unsigned int max_brightness;
> +	unsigned int default_brightness;
> +};
> +
> +static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
> +{
> +	int err;
> +
> +	if (!priv->enabled) {
> +		if (priv->power_supply) {
> +			err = regulator_enable(priv->power_supply);
> +
> +			if (err < 0)
> +				dev_err(priv->dev,
> +					"failed to enable power supply\n");
> +		}
> +
> +		if (priv->enable_gpio)
> +			gpiod_set_value_cansleep(priv->enable_gpio, 1);
> +	}
> +
> +	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
> +
> +	priv->enabled = true;
> +}
> +
> +static void led_bl_power_off(struct led_bl_data *priv)
> +{
> +	if (!priv->enabled)
> +		return;
> +
> +	led_set_brightness(priv->led_cdev, LED_OFF);
> +
> +	if (priv->enable_gpio)
> +		gpiod_set_value_cansleep(priv->enable_gpio, 0);
> +
> +	if (priv->power_supply)
> +		regulator_disable(priv->power_supply);
> +
> +	priv->enabled = false;
> +}
> +
> +static int led_bl_update_status(struct backlight_device *bl)
> +{
> +	struct led_bl_data *priv = bl_get_data(bl);
> +	int brightness = bl->props.brightness;
> +
> +	if (bl->props.power != FB_BLANK_UNBLANK ||
> +	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
> +	    bl->props.state & BL_CORE_FBBLANK)
> +		brightness = 0;
> +
> +	if (brightness > 0)
> +		led_bl_set_brightness(priv, brightness);
> +	else
> +		led_bl_power_off(priv);
> +
> +	return 0;
> +}
> +
> +static const struct backlight_ops led_bl_ops = {
> +	.update_status	= led_bl_update_status,
> +};
> +
> +static int led_bl_parse_dt(struct device *dev,
> +			   struct led_bl_data *priv)
> +{
> +	struct device_node *node = dev->of_node;
> +	int num_levels;
> +	u32 *levels;
> +	u32 value;
> +	int ret;
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	num_levels = of_property_count_u32_elems(node, "brightness-levels");
> +	if (num_levels < 0) {
> +		dev_err(dev, "failed to find 'brightness-levels'\n");
> +		return num_levels;
> +	}
> +
> +	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
> +	if (!levels)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(node, "brightness-levels",
> +					 levels,
> +					 num_levels);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to parse 'brightness-levels'\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "default-brightness-level", &value);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to parse 'default-brightness-level'\n");
> +		return ret;
> +	}
> +
> +	if (value >= num_levels) {
> +		dev_err(dev, "invalid default-brightness-level\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->levels = levels;
> +	priv->max_brightness = num_levels - 1;
> +	priv->default_brightness = value;
> +
> +	priv->led_cdev = of_led_get(node);
> +	if (IS_ERR(priv->led_cdev)) {
> +		dev_err(dev, "failed to get LED device\n");
> +		return PTR_ERR(priv->led_cdev);
> +	}
> +
> +	return 0;
> +}
> +
> +static int led_bl_probe(struct platform_device *pdev)
> +{
> +	struct backlight_properties props;
> +	struct led_bl_data *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->dev = &pdev->dev;
> +
> +	ret = led_bl_parse_dt(&pdev->dev, priv);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to parse DT data\n");
> +		return ret;
> +	}
> +
> +	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> +			    GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->enable_gpio)) {
> +		ret = PTR_ERR(priv->enable_gpio);
> +		goto err;
> +	}
> +
> +	priv->power_supply = devm_regulator_get_optional(&pdev->dev, "power");
> +	if (IS_ERR(priv->power_supply)) {
> +		ret = PTR_ERR(priv->power_supply);
> +
> +		if (ret == -ENODEV) {
> +			priv->power_supply = NULL;
> +		} else {
> +			ret = PTR_ERR(priv->power_supply);
> +			goto err;
> +		}
> +	}
> +
> +	memset(&props, 0, sizeof(struct backlight_properties));
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = priv->max_brightness;
> +	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
> +			&pdev->dev, priv, &led_bl_ops, &props);
> +	if (IS_ERR(priv->bl_dev)) {
> +		dev_err(&pdev->dev, "failed to register backlight\n");
> +		ret = PTR_ERR(priv->bl_dev);
> +		goto err;
> +	}
> +
> +	priv->bl_dev->props.brightness = priv->default_brightness;
> +	backlight_update_status(priv->bl_dev);
> +
> +	return 0;
> +
> +err:
> +	if (priv->led_cdev)
> +		led_put(priv->led_cdev);
> +
> +	return ret;
> +}
> +
> +static int led_bl_remove(struct platform_device *pdev)
> +{
> +	struct led_bl_data *priv = platform_get_drvdata(pdev);
> +	struct backlight_device *bl = priv->bl_dev;
> +
> +	backlight_device_unregister(bl);
> +
> +	led_bl_power_off(priv);
> +
> +	led_put(priv->led_cdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id led_bl_of_match[] = {
> +	{ .compatible = "led-backlight" },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, led_bl_of_match);
> +
> +static struct platform_driver led_bl_driver = {
> +	.driver		= {
> +		.name		= "led-backlight",
> +		.of_match_table	= of_match_ptr(led_bl_of_match),
> +	},
> +	.probe		= led_bl_probe,
> +	.remove		= led_bl_remove,
> +};
> +
> +module_platform_driver(led_bl_driver);
> +
> +MODULE_DESCRIPTION("LED based Backlight Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:led-backlight");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCHv4 2/3] backlight: add led-backlight driver

[Lee Jones]

Jingoo?

On Wed, 30 Sep 2015, Tomi Valkeinen wrote:
> This patch adds a led-backlight driver (led_bl), which is similar to
> pwm_bl except the driver uses a LED class driver to adjust the
> brightness in the HW.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/video/backlight/Kconfig  |   7 ++
>  drivers/video/backlight/Makefile |   1 +
>  drivers/video/backlight/led_bl.c | 246 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 254 insertions(+)
>  create mode 100644 drivers/video/backlight/led_bl.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 0505b796d743..d1336196aba2 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -453,6 +453,13 @@ config BACKLIGHT_BD6107
>  	help
>  	  If you have a Rohm BD6107 say Y to enable the backlight driver.
>  
> +config BACKLIGHT_LED
> +	tristate "Generic LED based Backlight Driver"
> +	depends on LEDS_CLASS && OF
> +	help
> +	  If you have a LCD backlight adjustable by LED class driver, say Y
> +	  to enable this driver.
> +
>  endif # BACKLIGHT_CLASS_DEVICE
>  
>  endif # BACKLIGHT_LCD_SUPPORT
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index d67073f9d421..ecd321daee21 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -54,3 +54,4 @@ obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
>  obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
>  obj-$(CONFIG_BACKLIGHT_TPS65217)	+= tps65217_bl.o
>  obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
> +obj-$(CONFIG_BACKLIGHT_LED)		+= led_bl.o
> diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
> new file mode 100644
> index 000000000000..1befc8ce5964
> --- /dev/null
> +++ b/drivers/video/backlight/led_bl.c
> @@ -0,0 +1,246 @@
> +/*
> + * Copyright 2015 Texas Instruments
> + *
> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> + *
> + * Based on pwm_bl.c
> + *
> + * 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; version 2 of the License.
> + */
> +#include <linux/backlight.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/leds.h>
> +#include <linux/of_leds.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +struct led_bl_data {
> +	struct device		*dev;
> +	struct backlight_device	*bl_dev;
> +
> +	unsigned int		*levels;
> +	bool			enabled;
> +	struct regulator	*power_supply;
> +	struct gpio_desc	*enable_gpio;
> +
> +	struct led_classdev *led_cdev;
> +
> +	unsigned int max_brightness;
> +	unsigned int default_brightness;
> +};
> +
> +static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
> +{
> +	int err;
> +
> +	if (!priv->enabled) {
> +		if (priv->power_supply) {
> +			err = regulator_enable(priv->power_supply);
> +
> +			if (err < 0)
> +				dev_err(priv->dev,
> +					"failed to enable power supply\n");
> +		}
> +
> +		if (priv->enable_gpio)
> +			gpiod_set_value_cansleep(priv->enable_gpio, 1);
> +	}
> +
> +	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
> +
> +	priv->enabled = true;
> +}
> +
> +static void led_bl_power_off(struct led_bl_data *priv)
> +{
> +	if (!priv->enabled)
> +		return;
> +
> +	led_set_brightness(priv->led_cdev, LED_OFF);
> +
> +	if (priv->enable_gpio)
> +		gpiod_set_value_cansleep(priv->enable_gpio, 0);
> +
> +	if (priv->power_supply)
> +		regulator_disable(priv->power_supply);
> +
> +	priv->enabled = false;
> +}
> +
> +static int led_bl_update_status(struct backlight_device *bl)
> +{
> +	struct led_bl_data *priv = bl_get_data(bl);
> +	int brightness = bl->props.brightness;
> +
> +	if (bl->props.power != FB_BLANK_UNBLANK ||
> +	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
> +	    bl->props.state & BL_CORE_FBBLANK)
> +		brightness = 0;
> +
> +	if (brightness > 0)
> +		led_bl_set_brightness(priv, brightness);
> +	else
> +		led_bl_power_off(priv);
> +
> +	return 0;
> +}
> +
> +static const struct backlight_ops led_bl_ops = {
> +	.update_status	= led_bl_update_status,
> +};
> +
> +static int led_bl_parse_dt(struct device *dev,
> +			   struct led_bl_data *priv)
> +{
> +	struct device_node *node = dev->of_node;
> +	int num_levels;
> +	u32 *levels;
> +	u32 value;
> +	int ret;
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	num_levels = of_property_count_u32_elems(node, "brightness-levels");
> +	if (num_levels < 0) {
> +		dev_err(dev, "failed to find 'brightness-levels'\n");
> +		return num_levels;
> +	}
> +
> +	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
> +	if (!levels)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_array(node, "brightness-levels",
> +					 levels,
> +					 num_levels);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to parse 'brightness-levels'\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(node, "default-brightness-level", &value);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to parse 'default-brightness-level'\n");
> +		return ret;
> +	}
> +
> +	if (value >= num_levels) {
> +		dev_err(dev, "invalid default-brightness-level\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->levels = levels;
> +	priv->max_brightness = num_levels - 1;
> +	priv->default_brightness = value;
> +
> +	priv->led_cdev = of_led_get(node);
> +	if (IS_ERR(priv->led_cdev)) {
> +		dev_err(dev, "failed to get LED device\n");
> +		return PTR_ERR(priv->led_cdev);
> +	}
> +
> +	return 0;
> +}
> +
> +static int led_bl_probe(struct platform_device *pdev)
> +{
> +	struct backlight_properties props;
> +	struct led_bl_data *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->dev = &pdev->dev;
> +
> +	ret = led_bl_parse_dt(&pdev->dev, priv);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to parse DT data\n");
> +		return ret;
> +	}
> +
> +	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
> +			    GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->enable_gpio)) {
> +		ret = PTR_ERR(priv->enable_gpio);
> +		goto err;
> +	}
> +
> +	priv->power_supply = devm_regulator_get_optional(&pdev->dev, "power");
> +	if (IS_ERR(priv->power_supply)) {
> +		ret = PTR_ERR(priv->power_supply);
> +
> +		if (ret = -ENODEV) {
> +			priv->power_supply = NULL;
> +		} else {
> +			ret = PTR_ERR(priv->power_supply);
> +			goto err;
> +		}
> +	}
> +
> +	memset(&props, 0, sizeof(struct backlight_properties));
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = priv->max_brightness;
> +	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
> +			&pdev->dev, priv, &led_bl_ops, &props);
> +	if (IS_ERR(priv->bl_dev)) {
> +		dev_err(&pdev->dev, "failed to register backlight\n");
> +		ret = PTR_ERR(priv->bl_dev);
> +		goto err;
> +	}
> +
> +	priv->bl_dev->props.brightness = priv->default_brightness;
> +	backlight_update_status(priv->bl_dev);
> +
> +	return 0;
> +
> +err:
> +	if (priv->led_cdev)
> +		led_put(priv->led_cdev);
> +
> +	return ret;
> +}
> +
> +static int led_bl_remove(struct platform_device *pdev)
> +{
> +	struct led_bl_data *priv = platform_get_drvdata(pdev);
> +	struct backlight_device *bl = priv->bl_dev;
> +
> +	backlight_device_unregister(bl);
> +
> +	led_bl_power_off(priv);
> +
> +	led_put(priv->led_cdev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id led_bl_of_match[] = {
> +	{ .compatible = "led-backlight" },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, led_bl_of_match);
> +
> +static struct platform_driver led_bl_driver = {
> +	.driver		= {
> +		.name		= "led-backlight",
> +		.of_match_table	= of_match_ptr(led_bl_of_match),
> +	},
> +	.probe		= led_bl_probe,
> +	.remove		= led_bl_remove,
> +};
> +
> +module_platform_driver(led_bl_driver);
> +
> +MODULE_DESCRIPTION("LED based Backlight Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:led-backlight");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Rob Herring]

On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Add DT binding for led-backlight.

Please use get_maintainers.pl.

>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>
> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> new file mode 100644
> index 000000000000..d4621d7414bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> @@ -0,0 +1,30 @@
> +led-backlight bindings
> +
> +Required properties:
> +  - compatible: "led-backlight"
> +  - leds: phandle to a led OF node [0]

Why do we need 2 levels of LED nodes?

> +  - brightness-levels: Array of distinct LED brightness levels. These
> +      are in the range from 0 to 255, passed to the LED class driver.
> +  - default-brightness-level: the default brightness level (index into the
> +      array defined by the "brightness-levels" property)
> +
> +Optional properties:
> +  - power-supply: regulator for supply voltage
> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
> +                  and disables the backlight (see GPIO binding[1])

Why are all of these not part of the LED node pointed to by leds?

Describe the h/w, not what you want for a driver.

Rob

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Rob Herring]

On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Add DT binding for led-backlight.

Please use get_maintainers.pl.

>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>
> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> new file mode 100644
> index 000000000000..d4621d7414bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> @@ -0,0 +1,30 @@
> +led-backlight bindings
> +
> +Required properties:
> +  - compatible: "led-backlight"
> +  - leds: phandle to a led OF node [0]

Why do we need 2 levels of LED nodes?

> +  - brightness-levels: Array of distinct LED brightness levels. These
> +      are in the range from 0 to 255, passed to the LED class driver.
> +  - default-brightness-level: the default brightness level (index into the
> +      array defined by the "brightness-levels" property)
> +
> +Optional properties:
> +  - power-supply: regulator for supply voltage
> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
> +                  and disables the backlight (see GPIO binding[1])

Why are all of these not part of the LED node pointed to by leds?

Describe the h/w, not what you want for a driver.

Rob

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Tomi Valkeinen]

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

Hi Rob,

On 13/10/15 17:21, Rob Herring wrote:
> On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> Add DT binding for led-backlight.
> 
> Please use get_maintainers.pl.

At some point I got feedback that the DT maintainers don't have time to
look at each individual driver binding, but rely on the subsystem
maintainers to handle them. Maybe I misunderstood that.

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: devicetree@vger.kernel.org
>> ---
>>  .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>> new file mode 100644
>> index 000000000000..d4621d7414bc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>> @@ -0,0 +1,30 @@
>> +led-backlight bindings
>> +
>> +Required properties:
>> +  - compatible: "led-backlight"
>> +  - leds: phandle to a led OF node [0]
> 
> Why do we need 2 levels of LED nodes?

Sorry, didn't get that. What do you mean with 2 levels?

>> +  - brightness-levels: Array of distinct LED brightness levels. These
>> +      are in the range from 0 to 255, passed to the LED class driver.
>> +  - default-brightness-level: the default brightness level (index into the
>> +      array defined by the "brightness-levels" property)
>> +
>> +Optional properties:
>> +  - power-supply: regulator for supply voltage
>> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>> +                  and disables the backlight (see GPIO binding[1])
> 
> Why are all of these not part of the LED node pointed to by leds?

These are for the backlight, not for the LED chip. So "LED" here is a
chip that produces (most likely) a PWM signal, and "backlight" is the
collection of components that use the PWM to produce the backlight
itself, and use the power-supply and gpios.

> Describe the h/w, not what you want for a driver.

I think this describes the HW quite well. The LED chip works fine
without any of the properties here, and these are specific to the
backlight part of the board.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Tomi Valkeinen]

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

Hi Rob,

On 13/10/15 17:21, Rob Herring wrote:
> On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> Add DT binding for led-backlight.
> 
> Please use get_maintainers.pl.

At some point I got feedback that the DT maintainers don't have time to
look at each individual driver binding, but rely on the subsystem
maintainers to handle them. Maybe I misunderstood that.

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: devicetree@vger.kernel.org
>> ---
>>  .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>
>> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>> new file mode 100644
>> index 000000000000..d4621d7414bc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>> @@ -0,0 +1,30 @@
>> +led-backlight bindings
>> +
>> +Required properties:
>> +  - compatible: "led-backlight"
>> +  - leds: phandle to a led OF node [0]
> 
> Why do we need 2 levels of LED nodes?

Sorry, didn't get that. What do you mean with 2 levels?

>> +  - brightness-levels: Array of distinct LED brightness levels. These
>> +      are in the range from 0 to 255, passed to the LED class driver.
>> +  - default-brightness-level: the default brightness level (index into the
>> +      array defined by the "brightness-levels" property)
>> +
>> +Optional properties:
>> +  - power-supply: regulator for supply voltage
>> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>> +                  and disables the backlight (see GPIO binding[1])
> 
> Why are all of these not part of the LED node pointed to by leds?

These are for the backlight, not for the LED chip. So "LED" here is a
chip that produces (most likely) a PWM signal, and "backlight" is the
collection of components that use the PWM to produce the backlight
itself, and use the power-supply and gpios.

> Describe the h/w, not what you want for a driver.

I think this describes the HW quite well. The LED chip works fine
without any of the properties here, and these are specific to the
backlight part of the board.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Rob Herring]

On Thu, Oct 15, 2015 at 7:17 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 13/10/15 17:21, Rob Herring wrote:
>> On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> Add DT binding for led-backlight.
>>
>> Please use get_maintainers.pl.
>
> At some point I got feedback that the DT maintainers don't have time to
> look at each individual driver binding, but rely on the subsystem
> maintainers to handle them. Maybe I misunderstood that.

True, but that doesn't mean to not copy us. If we didn't want to be
copied, we would update MAINTAINERS.

I wouldn't call this one an individual driver either. This is very
much a generic binding which we do want to review.

>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> Cc: devicetree@vger.kernel.org
>>> ---
>>>  .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>> new file mode 100644
>>> index 000000000000..d4621d7414bc
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>> @@ -0,0 +1,30 @@
>>> +led-backlight bindings
>>> +
>>> +Required properties:
>>> +  - compatible: "led-backlight"
>>> +  - leds: phandle to a led OF node [0]
>>
>> Why do we need 2 levels of LED nodes?
>
> Sorry, didn't get that. What do you mean with 2 levels?

You have the node the "leds" phandle points to which is the actual LED
device and then this node which is just backlight properties. And then
presumably another phandle in the panel device to point to the
backlight device.

>>> +  - brightness-levels: Array of distinct LED brightness levels. These
>>> +      are in the range from 0 to 255, passed to the LED class driver.
>>> +  - default-brightness-level: the default brightness level (index into the
>>> +      array defined by the "brightness-levels" property)
>>> +
>>> +Optional properties:
>>> +  - power-supply: regulator for supply voltage
>>> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>>> +                  and disables the backlight (see GPIO binding[1])
>>
>> Why are all of these not part of the LED node pointed to by leds?
>
> These are for the backlight, not for the LED chip. So "LED" here is a
> chip that produces (most likely) a PWM signal, and "backlight" is the
> collection of components that use the PWM to produce the backlight
> itself, and use the power-supply and gpios.

Okay, it wasn't clear that leds points to the LED controller node. The
example made it seem as it was the device. We already have a way to
describe LEDs and that is as child nodes of the LED controller node.
Please follow what was done for flash LEDs (leds/common.txt).

What's wrong with the existing pwm-backlight binding in the PWM case?

>
>> Describe the h/w, not what you want for a driver.
>
> I think this describes the HW quite well. The LED chip works fine
> without any of the properties here, and these are specific to the
> backlight part of the board.

A more complete example would be helpful here.

Rob

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Rob Herring]

On Thu, Oct 15, 2015 at 7:17 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 13/10/15 17:21, Rob Herring wrote:
>> On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> Add DT binding for led-backlight.
>>
>> Please use get_maintainers.pl.
>
> At some point I got feedback that the DT maintainers don't have time to
> look at each individual driver binding, but rely on the subsystem
> maintainers to handle them. Maybe I misunderstood that.

True, but that doesn't mean to not copy us. If we didn't want to be
copied, we would update MAINTAINERS.

I wouldn't call this one an individual driver either. This is very
much a generic binding which we do want to review.

>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> Cc: devicetree@vger.kernel.org
>>> ---
>>>  .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
>>>  1 file changed, 30 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>> new file mode 100644
>>> index 000000000000..d4621d7414bc
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>>> @@ -0,0 +1,30 @@
>>> +led-backlight bindings
>>> +
>>> +Required properties:
>>> +  - compatible: "led-backlight"
>>> +  - leds: phandle to a led OF node [0]
>>
>> Why do we need 2 levels of LED nodes?
>
> Sorry, didn't get that. What do you mean with 2 levels?

You have the node the "leds" phandle points to which is the actual LED
device and then this node which is just backlight properties. And then
presumably another phandle in the panel device to point to the
backlight device.

>>> +  - brightness-levels: Array of distinct LED brightness levels. These
>>> +      are in the range from 0 to 255, passed to the LED class driver.
>>> +  - default-brightness-level: the default brightness level (index into the
>>> +      array defined by the "brightness-levels" property)
>>> +
>>> +Optional properties:
>>> +  - power-supply: regulator for supply voltage
>>> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>>> +                  and disables the backlight (see GPIO binding[1])
>>
>> Why are all of these not part of the LED node pointed to by leds?
>
> These are for the backlight, not for the LED chip. So "LED" here is a
> chip that produces (most likely) a PWM signal, and "backlight" is the
> collection of components that use the PWM to produce the backlight
> itself, and use the power-supply and gpios.

Okay, it wasn't clear that leds points to the LED controller node. The
example made it seem as it was the device. We already have a way to
describe LEDs and that is as child nodes of the LED controller node.
Please follow what was done for flash LEDs (leds/common.txt).

What's wrong with the existing pwm-backlight binding in the PWM case?

>
>> Describe the h/w, not what you want for a driver.
>
> I think this describes the HW quite well. The LED chip works fine
> without any of the properties here, and these are specific to the
> backlight part of the board.

A more complete example would be helpful here.

Rob

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Tomi Valkeinen]

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

Hi Rob,

On 15/10/15 16:46, Rob Herring wrote:

>> At some point I got feedback that the DT maintainers don't have time to
>> look at each individual driver binding, but rely on the subsystem
>> maintainers to handle them. Maybe I misunderstood that.
> 
> True, but that doesn't mean to not copy us. If we didn't want to be
> copied, we would update MAINTAINERS.

Ok.

>>> Why do we need 2 levels of LED nodes?
>>
>> Sorry, didn't get that. What do you mean with 2 levels?
> 
> You have the node the "leds" phandle points to which is the actual LED
> device and then this node which is just backlight properties. And then
> presumably another phandle in the panel device to point to the
> backlight device.

Ok, I see what you mean.

Well, I have to say this is far from perfect. I initially pushed for a
PWM driver for the LED chip we use (tlc591xx), which would have allowed
us to use pwm-backlight driver. But Andrew was using the same chip for
more LED-ish use cases, for which a LED driver was more suitable.

But what I think we really should have is a more generic way to
represent output pins, so that GPIOs (well, GPOs really), PWMs and
current controlled outputs would all be done the same way.

It was rather difficult to use the LED driver and LED bindings for this,
as (afaics) they were really never designed to be used for anything else
than for simple LEDs (i.e. a LED connected directly to the LED chip).
The flash support was added later, but that's almost as simple as the
first case.

>> These are for the backlight, not for the LED chip. So "LED" here is a
>> chip that produces (most likely) a PWM signal, and "backlight" is the
>> collection of components that use the PWM to produce the backlight
>> itself, and use the power-supply and gpios.
> 
> Okay, it wasn't clear that leds points to the LED controller node. The

No, it doesn't point to the main LED node (the one having 'compatible').
It points to a child node.

> example made it seem as it was the device. We already have a way to
> describe LEDs and that is as child nodes of the LED controller node.

True, but those child nodes are very limited. As I see it, those child
nodes really describe the outputs of the LED chip, not what's on the
other end of the lines.

If on the other end of the lines is a more complex device, we need a
proper device driver for it, with a proper DT node with compatible
property etc.

Now, one could argue that a "backlight" that gets the LED signal from a
LED chip is really just a simple LED. But there are complications:

- Our board needs a GPIO to enable the backlight. I can't say what
exactly the GPIO does as my HW skills don't go far enough, but all this
is after the LED chip. I also see the circuitry using powers, which in
our case happen to be always on so we don't need to enable them explicitly.

- We need a backlight device/driver (because of the Linux SW stack).

So, maybe it would be possible to construct all that in a LED child
node, and the LED driver would create a child device for the nodes which
have 'compatible' property. But then, that would be very different from
pwm-backlight, and the parent-child relationships are usually used to
indicate a control relationship, right?

The led-backlight in these patches is very much similar to pwm-backlight.

> Please follow what was done for flash LEDs (leds/common.txt).

The flash support is quite simple. I'm not sure how I could do the same
for the backlight, as I described above.

> What's wrong with the existing pwm-backlight binding in the PWM case?

Nothing, if there's a PWM driver. But if the LED chip is modelled as a
LED driver, pwm-backlight is out. I think there are two kinds of LED
chips, PWM ones and current-controlling ones. And then there are the PWM
devices which are clearly PWM ones.

>>> Describe the h/w, not what you want for a driver.
>>
>> I think this describes the HW quite well. The LED chip works fine
>> without any of the properties here, and these are specific to the
>> backlight part of the board.
> 
> A more complete example would be helpful here.

Of our HW? I can't give the schematics but I hope I described it enough
above.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Tomi Valkeinen]

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

Hi Rob,

On 15/10/15 16:46, Rob Herring wrote:

>> At some point I got feedback that the DT maintainers don't have time to
>> look at each individual driver binding, but rely on the subsystem
>> maintainers to handle them. Maybe I misunderstood that.
> 
> True, but that doesn't mean to not copy us. If we didn't want to be
> copied, we would update MAINTAINERS.

Ok.

>>> Why do we need 2 levels of LED nodes?
>>
>> Sorry, didn't get that. What do you mean with 2 levels?
> 
> You have the node the "leds" phandle points to which is the actual LED
> device and then this node which is just backlight properties. And then
> presumably another phandle in the panel device to point to the
> backlight device.

Ok, I see what you mean.

Well, I have to say this is far from perfect. I initially pushed for a
PWM driver for the LED chip we use (tlc591xx), which would have allowed
us to use pwm-backlight driver. But Andrew was using the same chip for
more LED-ish use cases, for which a LED driver was more suitable.

But what I think we really should have is a more generic way to
represent output pins, so that GPIOs (well, GPOs really), PWMs and
current controlled outputs would all be done the same way.

It was rather difficult to use the LED driver and LED bindings for this,
as (afaics) they were really never designed to be used for anything else
than for simple LEDs (i.e. a LED connected directly to the LED chip).
The flash support was added later, but that's almost as simple as the
first case.

>> These are for the backlight, not for the LED chip. So "LED" here is a
>> chip that produces (most likely) a PWM signal, and "backlight" is the
>> collection of components that use the PWM to produce the backlight
>> itself, and use the power-supply and gpios.
> 
> Okay, it wasn't clear that leds points to the LED controller node. The

No, it doesn't point to the main LED node (the one having 'compatible').
It points to a child node.

> example made it seem as it was the device. We already have a way to
> describe LEDs and that is as child nodes of the LED controller node.

True, but those child nodes are very limited. As I see it, those child
nodes really describe the outputs of the LED chip, not what's on the
other end of the lines.

If on the other end of the lines is a more complex device, we need a
proper device driver for it, with a proper DT node with compatible
property etc.

Now, one could argue that a "backlight" that gets the LED signal from a
LED chip is really just a simple LED. But there are complications:

- Our board needs a GPIO to enable the backlight. I can't say what
exactly the GPIO does as my HW skills don't go far enough, but all this
is after the LED chip. I also see the circuitry using powers, which in
our case happen to be always on so we don't need to enable them explicitly.

- We need a backlight device/driver (because of the Linux SW stack).

So, maybe it would be possible to construct all that in a LED child
node, and the LED driver would create a child device for the nodes which
have 'compatible' property. But then, that would be very different from
pwm-backlight, and the parent-child relationships are usually used to
indicate a control relationship, right?

The led-backlight in these patches is very much similar to pwm-backlight.

> Please follow what was done for flash LEDs (leds/common.txt).

The flash support is quite simple. I'm not sure how I could do the same
for the backlight, as I described above.

> What's wrong with the existing pwm-backlight binding in the PWM case?

Nothing, if there's a PWM driver. But if the LED chip is modelled as a
LED driver, pwm-backlight is out. I think there are two kinds of LED
chips, PWM ones and current-controlling ones. And then there are the PWM
devices which are clearly PWM ones.

>>> Describe the h/w, not what you want for a driver.
>>
>> I think this describes the HW quite well. The LED chip works fine
>> without any of the properties here, and these are specific to the
>> backlight part of the board.
> 
> A more complete example would be helpful here.

Of our HW? I can't give the schematics but I hope I described it enough
above.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Rob Herring]

On Thu, Oct 15, 2015 at 9:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 15/10/15 16:46, Rob Herring wrote:
>
>>> At some point I got feedback that the DT maintainers don't have time to
>>> look at each individual driver binding, but rely on the subsystem
>>> maintainers to handle them. Maybe I misunderstood that.
>>
>> True, but that doesn't mean to not copy us. If we didn't want to be
>> copied, we would update MAINTAINERS.
>
> Ok.
>
>>>> Why do we need 2 levels of LED nodes?
>>>
>>> Sorry, didn't get that. What do you mean with 2 levels?
>>
>> You have the node the "leds" phandle points to which is the actual LED
>> device and then this node which is just backlight properties. And then
>> presumably another phandle in the panel device to point to the
>> backlight device.
>
> Ok, I see what you mean.
>
> Well, I have to say this is far from perfect. I initially pushed for a
> PWM driver for the LED chip we use (tlc591xx), which would have allowed
> us to use pwm-backlight driver. But Andrew was using the same chip for
> more LED-ish use cases, for which a LED driver was more suitable.
>
> But what I think we really should have is a more generic way to
> represent output pins, so that GPIOs (well, GPOs really), PWMs and
> current controlled outputs would all be done the same way.
>
> It was rather difficult to use the LED driver and LED bindings for this,
> as (afaics) they were really never designed to be used for anything else
> than for simple LEDs (i.e. a LED connected directly to the LED chip).
> The flash support was added later, but that's almost as simple as the
> first case.

There's still room to extend it though.

>>> These are for the backlight, not for the LED chip. So "LED" here is a
>>> chip that produces (most likely) a PWM signal, and "backlight" is the
>>> collection of components that use the PWM to produce the backlight
>>> itself, and use the power-supply and gpios.
>>
>> Okay, it wasn't clear that leds points to the LED controller node. The
>
> No, it doesn't point to the main LED node (the one having 'compatible').
> It points to a child node.
>
>> example made it seem as it was the device. We already have a way to
>> describe LEDs and that is as child nodes of the LED controller node.
>
> True, but those child nodes are very limited. As I see it, those child
> nodes really describe the outputs of the LED chip, not what's on the
> other end of the lines.

The child nodes are supposed to be the other end. In the flash case,
the properties are constraints on the flash LED (i.e. different flash
LEDs will have different max currents).

>
> If on the other end of the lines is a more complex device, we need a
> proper device driver for it, with a proper DT node with compatible
> property etc.
>
> Now, one could argue that a "backlight" that gets the LED signal from a
> LED chip is really just a simple LED. But there are complications:

I would say backlights are a complex example of LEDs. Of course, there
are backlights not based on LEDs, but we're not talking about those
here.

> - Our board needs a GPIO to enable the backlight. I can't say what
> exactly the GPIO does as my HW skills don't go far enough, but all this
> is after the LED chip. I also see the circuitry using powers, which in
> our case happen to be always on so we don't need to enable them explicitly.

The GPIO is probably controlling a transistor to connect the LED anode
to ground and therefore turn it on. These have nothing to do with
backlights really, but really are common to LEDs. Every LED needs a
supply rail too. This may come for a regulator or directly from an LED
driver IC.

If the flash LED binding doesn't have these, then it is only a matter of time.

>
> - We need a backlight device/driver (because of the Linux SW stack).

>From a binding perspective, not my problem. The problem with the
driver needs driving the binding definition is the drivers can change
over time. IIRC there has been some discussion of combining the 2
subsystems in the kernel, so we don't want to create something defined
by current kernel needs.

> So, maybe it would be possible to construct all that in a LED child
> node, and the LED driver would create a child device for the nodes which
> have 'compatible' property. But then, that would be very different from
> pwm-backlight, and the parent-child relationships are usually used to
> indicate a control relationship, right?

This is along the lines I was thinking, but don't see how it is very
different at the binding level. The parent-child relationship is
typically control path or just what is downstream from the parent
device. Some bindings like GPIO and PWM don't follow this, but that is
often because they are just additional sideband interfaces on top of
the main control interface. I think simply making the "backlight" node
from the pwm-backlight binding a child works. We probably need a
different compatible string though (led-backlight is as good as
anything). I also think we should require child nodes to have a
compatible string which we didn't do for flash devices. It's probably
not too late to fix that.

I think there are 2 cases of PWM connection to LEDs to consider. The
PWM is an input to a LED driver chip or the PWM directly controls the
LED (attached to the anode). The current pwm-backlight binding covers
the latter. In the former case, pwms should probably be in the parent
(LED driver IC node). Of course, if the driver IC has no s/w
controllable interface beyond PWM, then it probably doesn't need to be
modeled at all in DT.

> The led-backlight in these patches is very much similar to pwm-backlight.

Yes, I think we're really only debating the structure of nodes.

>
>> Please follow what was done for flash LEDs (leds/common.txt).
>
> The flash support is quite simple. I'm not sure how I could do the same
> for the backlight, as I described above.
>
>> What's wrong with the existing pwm-backlight binding in the PWM case?
>
> Nothing, if there's a PWM driver. But if the LED chip is modelled as a
> LED driver, pwm-backlight is out. I think there are two kinds of LED
> chips, PWM ones and current-controlling ones. And then there are the PWM
> devices which are clearly PWM ones.
>
>>>> Describe the h/w, not what you want for a driver.
>>>
>>> I think this describes the HW quite well. The LED chip works fine
>>> without any of the properties here, and these are specific to the
>>> backlight part of the board.
>>
>> A more complete example would be helpful here.
>
> Of our HW? I can't give the schematics but I hope I described it enough
> above.

I just meant the relationship of all the nodes involved.

Rob

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Rob Herring]

On Thu, Oct 15, 2015 at 9:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 15/10/15 16:46, Rob Herring wrote:
>
>>> At some point I got feedback that the DT maintainers don't have time to
>>> look at each individual driver binding, but rely on the subsystem
>>> maintainers to handle them. Maybe I misunderstood that.
>>
>> True, but that doesn't mean to not copy us. If we didn't want to be
>> copied, we would update MAINTAINERS.
>
> Ok.
>
>>>> Why do we need 2 levels of LED nodes?
>>>
>>> Sorry, didn't get that. What do you mean with 2 levels?
>>
>> You have the node the "leds" phandle points to which is the actual LED
>> device and then this node which is just backlight properties. And then
>> presumably another phandle in the panel device to point to the
>> backlight device.
>
> Ok, I see what you mean.
>
> Well, I have to say this is far from perfect. I initially pushed for a
> PWM driver for the LED chip we use (tlc591xx), which would have allowed
> us to use pwm-backlight driver. But Andrew was using the same chip for
> more LED-ish use cases, for which a LED driver was more suitable.
>
> But what I think we really should have is a more generic way to
> represent output pins, so that GPIOs (well, GPOs really), PWMs and
> current controlled outputs would all be done the same way.
>
> It was rather difficult to use the LED driver and LED bindings for this,
> as (afaics) they were really never designed to be used for anything else
> than for simple LEDs (i.e. a LED connected directly to the LED chip).
> The flash support was added later, but that's almost as simple as the
> first case.

There's still room to extend it though.

>>> These are for the backlight, not for the LED chip. So "LED" here is a
>>> chip that produces (most likely) a PWM signal, and "backlight" is the
>>> collection of components that use the PWM to produce the backlight
>>> itself, and use the power-supply and gpios.
>>
>> Okay, it wasn't clear that leds points to the LED controller node. The
>
> No, it doesn't point to the main LED node (the one having 'compatible').
> It points to a child node.
>
>> example made it seem as it was the device. We already have a way to
>> describe LEDs and that is as child nodes of the LED controller node.
>
> True, but those child nodes are very limited. As I see it, those child
> nodes really describe the outputs of the LED chip, not what's on the
> other end of the lines.

The child nodes are supposed to be the other end. In the flash case,
the properties are constraints on the flash LED (i.e. different flash
LEDs will have different max currents).

>
> If on the other end of the lines is a more complex device, we need a
> proper device driver for it, with a proper DT node with compatible
> property etc.
>
> Now, one could argue that a "backlight" that gets the LED signal from a
> LED chip is really just a simple LED. But there are complications:

I would say backlights are a complex example of LEDs. Of course, there
are backlights not based on LEDs, but we're not talking about those
here.

> - Our board needs a GPIO to enable the backlight. I can't say what
> exactly the GPIO does as my HW skills don't go far enough, but all this
> is after the LED chip. I also see the circuitry using powers, which in
> our case happen to be always on so we don't need to enable them explicitly.

The GPIO is probably controlling a transistor to connect the LED anode
to ground and therefore turn it on. These have nothing to do with
backlights really, but really are common to LEDs. Every LED needs a
supply rail too. This may come for a regulator or directly from an LED
driver IC.

If the flash LED binding doesn't have these, then it is only a matter of time.

>
> - We need a backlight device/driver (because of the Linux SW stack).

From a binding perspective, not my problem. The problem with the
driver needs driving the binding definition is the drivers can change
over time. IIRC there has been some discussion of combining the 2
subsystems in the kernel, so we don't want to create something defined
by current kernel needs.

> So, maybe it would be possible to construct all that in a LED child
> node, and the LED driver would create a child device for the nodes which
> have 'compatible' property. But then, that would be very different from
> pwm-backlight, and the parent-child relationships are usually used to
> indicate a control relationship, right?

This is along the lines I was thinking, but don't see how it is very
different at the binding level. The parent-child relationship is
typically control path or just what is downstream from the parent
device. Some bindings like GPIO and PWM don't follow this, but that is
often because they are just additional sideband interfaces on top of
the main control interface. I think simply making the "backlight" node
from the pwm-backlight binding a child works. We probably need a
different compatible string though (led-backlight is as good as
anything). I also think we should require child nodes to have a
compatible string which we didn't do for flash devices. It's probably
not too late to fix that.

I think there are 2 cases of PWM connection to LEDs to consider. The
PWM is an input to a LED driver chip or the PWM directly controls the
LED (attached to the anode). The current pwm-backlight binding covers
the latter. In the former case, pwms should probably be in the parent
(LED driver IC node). Of course, if the driver IC has no s/w
controllable interface beyond PWM, then it probably doesn't need to be
modeled at all in DT.

> The led-backlight in these patches is very much similar to pwm-backlight.

Yes, I think we're really only debating the structure of nodes.

>
>> Please follow what was done for flash LEDs (leds/common.txt).
>
> The flash support is quite simple. I'm not sure how I could do the same
> for the backlight, as I described above.
>
>> What's wrong with the existing pwm-backlight binding in the PWM case?
>
> Nothing, if there's a PWM driver. But if the LED chip is modelled as a
> LED driver, pwm-backlight is out. I think there are two kinds of LED
> chips, PWM ones and current-controlling ones. And then there are the PWM
> devices which are clearly PWM ones.
>
>>>> Describe the h/w, not what you want for a driver.
>>>
>>> I think this describes the HW quite well. The LED chip works fine
>>> without any of the properties here, and these are specific to the
>>> backlight part of the board.
>>
>> A more complete example would be helpful here.
>
> Of our HW? I can't give the schematics but I hope I described it enough
> above.

I just meant the relationship of all the nodes involved.

Rob

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Tomi Valkeinen]

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

Hi Rob,

On 15/10/15 21:55, Rob Herring wrote:

>> True, but those child nodes are very limited. As I see it, those child
>> nodes really describe the outputs of the LED chip, not what's on the
>> other end of the lines.
> 
> The child nodes are supposed to be the other end. In the flash case,
> the properties are constraints on the flash LED (i.e. different flash
> LEDs will have different max currents).

Right, but what I meant is that the max current is applied to the LED
controller hardware, so in that sense it can be considered as a property
of the LED controller.

If there was, say, a flash that supports changing the color of the
light, that property would be applied to the flash HW so it'd be a
property of the "other end".

>> If on the other end of the lines is a more complex device, we need a
>> proper device driver for it, with a proper DT node with compatible
>> property etc.
>>
>> Now, one could argue that a "backlight" that gets the LED signal from a
>> LED chip is really just a simple LED. But there are complications:
> 
> I would say backlights are a complex example of LEDs. Of course, there
> are backlights not based on LEDs, but we're not talking about those
> here.

I like the GPIO/PWM binding model, as it doesn't force any
node-hierarchy to the consumer of the GPIO/PWM. I don't see LED
controller output being any different than PWM, but the current LED
bindings still force the consumers of the LED signal to be a child of
the LED controller.

How about an LCD module, controlled via i2c, which takes a LED PWM
signal as an input, but needs i2c commands to enable the actual
backlight? So kind of i2c controlled smart LED. Or if the LCD module
takes two LED PWM signal inputs to achieve some fancy backlight effects.

Yes, it's theoretical HW, and I know some people don't like to discuss
such things... But the above example is easily accomplished with the
GPIO/PWM style bindings, but I don't have any idea how it could be done
with the current LED bindings.

>> - Our board needs a GPIO to enable the backlight. I can't say what
>> exactly the GPIO does as my HW skills don't go far enough, but all this
>> is after the LED chip. I also see the circuitry using powers, which in
>> our case happen to be always on so we don't need to enable them explicitly.
> 
> The GPIO is probably controlling a transistor to connect the LED anode
> to ground and therefore turn it on. These have nothing to do with
> backlights really, but really are common to LEDs. Every LED needs a
> supply rail too. This may come for a regulator or directly from an LED
> driver IC.

There's something a bit more complex there. The gpio controls TI
TPS6108x (http://www.ti.com/lit/ds/symlink/tps61081.pdf "High-Voltage
DC-DC Boost Converter").

But possibly all that can still be considered as you described.

> If the flash LED binding doesn't have these, then it is only a matter of time.
> 
>>
>> - We need a backlight device/driver (because of the Linux SW stack).
> 
> From a binding perspective, not my problem. The problem with the
> driver needs driving the binding definition is the drivers can change
> over time. IIRC there has been some discussion of combining the 2
> subsystems in the kernel, so we don't want to create something defined
> by current kernel needs.

True. But on the other hand I would also not want to force new drivers
to use the current binding model, if it doesn't quite fit.

So if the PWM/GPIO model is fine, and LED controllers are really very
similar, shouldn't we extend the LED bindings towards PWM/GPIO model
rather than trying to fit everything into the current LED bindings?

Of course, even if everybody would agree with the above, this particular
backlight binding is rather simple, and I think we can fit it in the
current model. But that then raises the question, if led-backlight and
pwm-backlight are about the same, why are the binding model different.

>> So, maybe it would be possible to construct all that in a LED child
>> node, and the LED driver would create a child device for the nodes which
>> have 'compatible' property. But then, that would be very different from
>> pwm-backlight, and the parent-child relationships are usually used to
>> indicate a control relationship, right?
> 
> This is along the lines I was thinking, but don't see how it is very
> different at the binding level. The parent-child relationship is

In my experience, the relationship between the nodes is usually the most
difficult part with bindings, both in the design side and the driver
implementation side.

So if pwm-backlight links to the pwm source using a phandle, and
led-backlight links by child-parent relationship, I see them as quite
different, even if the rest of the properties are the same.

> typically control path or just what is downstream from the parent
> device. Some bindings like GPIO and PWM don't follow this, but that is
> often because they are just additional sideband interfaces on top of
> the main control interface. I think simply making the "backlight" node
> from the pwm-backlight binding a child works. We probably need a
> different compatible string though (led-backlight is as good as

So hmm... You mean using the pwm-backlight node, without the "pwms"
property, as the source is implicit? So:

/* tlc59108 is an i2c device */
tlc59116@40 {
	#address-cells = <1>;
	#size-cells = <0>;
	compatible = "ti,tlc59108";
	reg = <0x40>;

	wan@0 {
		label = "wrt1900ac:amber:wan";
		reg = <0x0>;
	};

	bl@2 {
		label = "backlight";
		reg = <0x2>;

		compatible = "led-backlight";
		brightness-levels = <0 243 245 247 248 249 251 252 255>;
		default-brightness-level = <8>;

		enable-gpios = <&pcf_lcd 13 GPIO_ACTIVE_LOW>;
	};
};

At the moment each LED controller driver does its own DT parsing, and
there's no common code for anything related to DT in the LED framework
and the LED framework is not even aware of DT nodes or such (which is
why I needed a bit hackish approach in my patches to find the nodes).

I have a gut feeling that going into this direction will require quite a
bit of restructuring in the LED drivers, so I think I need to leave this
task for others due to lack of time.

> anything). I also think we should require child nodes to have a
> compatible string which we didn't do for flash devices. It's probably
> not too late to fix that.

That is probably a good idea.

> I think there are 2 cases of PWM connection to LEDs to consider. The
> PWM is an input to a LED driver chip or the PWM directly controls the
> LED (attached to the anode). The current pwm-backlight binding covers
> the latter. In the former case, pwms should probably be in the parent
> (LED driver IC node). Of course, if the driver IC has no s/w
> controllable interface beyond PWM, then it probably doesn't need to be
> modeled at all in DT.

I guess one option would also be to create an MFD of the LED controller,
so that it would offer some of the outputs as plain PWMs. Then
pwm-backlight could be used directly.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Tomi Valkeinen]

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

Hi Rob,

On 15/10/15 21:55, Rob Herring wrote:

>> True, but those child nodes are very limited. As I see it, those child
>> nodes really describe the outputs of the LED chip, not what's on the
>> other end of the lines.
> 
> The child nodes are supposed to be the other end. In the flash case,
> the properties are constraints on the flash LED (i.e. different flash
> LEDs will have different max currents).

Right, but what I meant is that the max current is applied to the LED
controller hardware, so in that sense it can be considered as a property
of the LED controller.

If there was, say, a flash that supports changing the color of the
light, that property would be applied to the flash HW so it'd be a
property of the "other end".

>> If on the other end of the lines is a more complex device, we need a
>> proper device driver for it, with a proper DT node with compatible
>> property etc.
>>
>> Now, one could argue that a "backlight" that gets the LED signal from a
>> LED chip is really just a simple LED. But there are complications:
> 
> I would say backlights are a complex example of LEDs. Of course, there
> are backlights not based on LEDs, but we're not talking about those
> here.

I like the GPIO/PWM binding model, as it doesn't force any
node-hierarchy to the consumer of the GPIO/PWM. I don't see LED
controller output being any different than PWM, but the current LED
bindings still force the consumers of the LED signal to be a child of
the LED controller.

How about an LCD module, controlled via i2c, which takes a LED PWM
signal as an input, but needs i2c commands to enable the actual
backlight? So kind of i2c controlled smart LED. Or if the LCD module
takes two LED PWM signal inputs to achieve some fancy backlight effects.

Yes, it's theoretical HW, and I know some people don't like to discuss
such things... But the above example is easily accomplished with the
GPIO/PWM style bindings, but I don't have any idea how it could be done
with the current LED bindings.

>> - Our board needs a GPIO to enable the backlight. I can't say what
>> exactly the GPIO does as my HW skills don't go far enough, but all this
>> is after the LED chip. I also see the circuitry using powers, which in
>> our case happen to be always on so we don't need to enable them explicitly.
> 
> The GPIO is probably controlling a transistor to connect the LED anode
> to ground and therefore turn it on. These have nothing to do with
> backlights really, but really are common to LEDs. Every LED needs a
> supply rail too. This may come for a regulator or directly from an LED
> driver IC.

There's something a bit more complex there. The gpio controls TI
TPS6108x (http://www.ti.com/lit/ds/symlink/tps61081.pdf "High-Voltage
DC-DC Boost Converter").

But possibly all that can still be considered as you described.

> If the flash LED binding doesn't have these, then it is only a matter of time.
> 
>>
>> - We need a backlight device/driver (because of the Linux SW stack).
> 
> From a binding perspective, not my problem. The problem with the
> driver needs driving the binding definition is the drivers can change
> over time. IIRC there has been some discussion of combining the 2
> subsystems in the kernel, so we don't want to create something defined
> by current kernel needs.

True. But on the other hand I would also not want to force new drivers
to use the current binding model, if it doesn't quite fit.

So if the PWM/GPIO model is fine, and LED controllers are really very
similar, shouldn't we extend the LED bindings towards PWM/GPIO model
rather than trying to fit everything into the current LED bindings?

Of course, even if everybody would agree with the above, this particular
backlight binding is rather simple, and I think we can fit it in the
current model. But that then raises the question, if led-backlight and
pwm-backlight are about the same, why are the binding model different.

>> So, maybe it would be possible to construct all that in a LED child
>> node, and the LED driver would create a child device for the nodes which
>> have 'compatible' property. But then, that would be very different from
>> pwm-backlight, and the parent-child relationships are usually used to
>> indicate a control relationship, right?
> 
> This is along the lines I was thinking, but don't see how it is very
> different at the binding level. The parent-child relationship is

In my experience, the relationship between the nodes is usually the most
difficult part with bindings, both in the design side and the driver
implementation side.

So if pwm-backlight links to the pwm source using a phandle, and
led-backlight links by child-parent relationship, I see them as quite
different, even if the rest of the properties are the same.

> typically control path or just what is downstream from the parent
> device. Some bindings like GPIO and PWM don't follow this, but that is
> often because they are just additional sideband interfaces on top of
> the main control interface. I think simply making the "backlight" node
> from the pwm-backlight binding a child works. We probably need a
> different compatible string though (led-backlight is as good as

So hmm... You mean using the pwm-backlight node, without the "pwms"
property, as the source is implicit? So:

/* tlc59108 is an i2c device */
tlc59116@40 {
	#address-cells = <1>;
	#size-cells = <0>;
	compatible = "ti,tlc59108";
	reg = <0x40>;

	wan@0 {
		label = "wrt1900ac:amber:wan";
		reg = <0x0>;
	};

	bl@2 {
		label = "backlight";
		reg = <0x2>;

		compatible = "led-backlight";
		brightness-levels = <0 243 245 247 248 249 251 252 255>;
		default-brightness-level = <8>;

		enable-gpios = <&pcf_lcd 13 GPIO_ACTIVE_LOW>;
	};
};

At the moment each LED controller driver does its own DT parsing, and
there's no common code for anything related to DT in the LED framework
and the LED framework is not even aware of DT nodes or such (which is
why I needed a bit hackish approach in my patches to find the nodes).

I have a gut feeling that going into this direction will require quite a
bit of restructuring in the LED drivers, so I think I need to leave this
task for others due to lack of time.

> anything). I also think we should require child nodes to have a
> compatible string which we didn't do for flash devices. It's probably
> not too late to fix that.

That is probably a good idea.

> I think there are 2 cases of PWM connection to LEDs to consider. The
> PWM is an input to a LED driver chip or the PWM directly controls the
> LED (attached to the anode). The current pwm-backlight binding covers
> the latter. In the former case, pwms should probably be in the parent
> (LED driver IC node). Of course, if the driver IC has no s/w
> controllable interface beyond PWM, then it probably doesn't need to be
> modeled at all in DT.

I guess one option would also be to create an MFD of the LED controller,
so that it would offer some of the outputs as plain PWMs. Then
pwm-backlight could be used directly.

 Tomi


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Rob Herring]

 

On Fri, Oct 16, 2015 at 6:42 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 15/10/15 21:55, Rob Herring wrote:
>
>>> True, but those child nodes are very limited. As I see it, those child
>>> nodes really describe the outputs of the LED chip, not what's on the
>>> other end of the lines.
>>
>> The child nodes are supposed to be the other end. In the flash case,
>> the properties are constraints on the flash LED (i.e. different flash
>> LEDs will have different max currents).
>
> Right, but what I meant is that the max current is applied to the LED
> controller hardware, so in that sense it can be considered as a property
> of the LED controller.
>
> If there was, say, a flash that supports changing the color of the
> light, that property would be applied to the flash HW so it'd be a
> property of the "other end".
>
>>> If on the other end of the lines is a more complex device, we need a
>>> proper device driver for it, with a proper DT node with compatible
>>> property etc.
>>>
>>> Now, one could argue that a "backlight" that gets the LED signal from a
>>> LED chip is really just a simple LED. But there are complications:
>>
>> I would say backlights are a complex example of LEDs. Of course, there
>> are backlights not based on LEDs, but we're not talking about those
>> here.
>
> I like the GPIO/PWM binding model, as it doesn't force any
> node-hierarchy to the consumer of the GPIO/PWM. I don't see LED
> controller output being any different than PWM, but the current LED
> bindings still force the consumers of the LED signal to be a child of
> the LED controller.
>
> How about an LCD module, controlled via i2c, which takes a LED PWM
> signal as an input, but needs i2c commands to enable the actual
> backlight? So kind of i2c controlled smart LED. Or if the LCD module
> takes two LED PWM signal inputs to achieve some fancy backlight effects.
>
> Yes, it's theoretical HW, and I know some people don't like to discuss
> such things... But the above example is easily accomplished with the
> GPIO/PWM style bindings, but I don't have any idea how it could be done
> with the current LED bindings.
>
>>> - Our board needs a GPIO to enable the backlight. I can't say what
>>> exactly the GPIO does as my HW skills don't go far enough, but all this
>>> is after the LED chip. I also see the circuitry using powers, which in
>>> our case happen to be always on so we don't need to enable them explicitly.
>>
>> The GPIO is probably controlling a transistor to connect the LED anode
>> to ground and therefore turn it on. These have nothing to do with
>> backlights really, but really are common to LEDs. Every LED needs a
>> supply rail too. This may come for a regulator or directly from an LED
>> driver IC.
>
> There's something a bit more complex there. The gpio controls TI
> TPS6108x (http://www.ti.com/lit/ds/symlink/tps61081.pdf "High-Voltage
> DC-DC Boost Converter").
>
> But possibly all that can still be considered as you described.
>
>> If the flash LED binding doesn't have these, then it is only a matter of time.
>>
>>>
>>> - We need a backlight device/driver (because of the Linux SW stack).
>>
>> From a binding perspective, not my problem. The problem with the
>> driver needs driving the binding definition is the drivers can change
>> over time. IIRC there has been some discussion of combining the 2
>> subsystems in the kernel, so we don't want to create something defined
>> by current kernel needs.
>
> True. But on the other hand I would also not want to force new drivers
> to use the current binding model, if it doesn't quite fit.
>
> So if the PWM/GPIO model is fine, and LED controllers are really very
> similar, shouldn't we extend the LED bindings towards PWM/GPIO model
> rather than trying to fit everything into the current LED bindings?
>
> Of course, even if everybody would agree with the above, this particular
> backlight binding is rather simple, and I think we can fit it in the
> current model. But that then raises the question, if led-backlight and
> pwm-backlight are about the same, why are the binding model different.
>
>>> So, maybe it would be possible to construct all that in a LED child
>>> node, and the LED driver would create a child device for the nodes which
>>> have 'compatible' property. But then, that would be very different from
>>> pwm-backlight, and the parent-child relationships are usually used to
>>> indicate a control relationship, right?
>>
>> This is along the lines I was thinking, but don't see how it is very
>> different at the binding level. The parent-child relationship is
>
> In my experience, the relationship between the nodes is usually the most
> difficult part with bindings, both in the design side and the driver
> implementation side.
>
> So if pwm-backlight links to the pwm source using a phandle, and
> led-backlight links by child-parent relationship, I see them as quite
> different, even if the rest of the properties are the same.
>
>> typically control path or just what is downstream from the parent
>> device. Some bindings like GPIO and PWM don't follow this, but that is
>> often because they are just additional sideband interfaces on top of
>> the main control interface. I think simply making the "backlight" node
>> from the pwm-backlight binding a child works. We probably need a
>> different compatible string though (led-backlight is as good as
>
> So hmm... You mean using the pwm-backlight node, without the "pwms"
> property, as the source is implicit? So:
>
> /* tlc59108 is an i2c device */
> tlc59116@40 {
>         #address-cells = <1>;
>         #size-cells = <0>;
>         compatible = "ti,tlc59108";
>         reg = <0x40>;
>
>         wan@0 {
>                 label = "wrt1900ac:amber:wan";
>                 reg = <0x0>;
>         };
>
>         bl@2 {
>                 label = "backlight";
>                 reg = <0x2>;
>
>                 compatible = "led-backlight";
>                 brightness-levels = <0 243 245 247 248 249 251 252 255>;
>                 default-brightness-level = <8>;
>
>                 enable-gpios = <&pcf_lcd 13 GPIO_ACTIVE_LOW>;
>         };
> };
>
> At the moment each LED controller driver does its own DT parsing, and
> there's no common code for anything related to DT in the LED framework
> and the LED framework is not even aware of DT nodes or such (which is
> why I needed a bit hackish approach in my patches to find the nodes).
>
> I have a gut feeling that going into this direction will require quite a
> bit of restructuring in the LED drivers, so I think I need to leave this
> task for others due to lack of time.
>
>> anything). I also think we should require child nodes to have a
>> compatible string which we didn't do for flash devices. It's probably
>> not too late to fix that.
>
> That is probably a good idea.
>
>> I think there are 2 cases of PWM connection to LEDs to consider. The
>> PWM is an input to a LED driver chip or the PWM directly controls the
>> LED (attached to the anode). The current pwm-backlight binding covers
>> the latter. In the former case, pwms should probably be in the parent
>> (LED driver IC node). Of course, if the driver IC has no s/w
>> controllable interface beyond PWM, then it probably doesn't need to be
>> modeled at all in DT.
>
> I guess one option would also be to create an MFD of the LED controller,
> so that it would offer some of the outputs as plain PWMs. Then
> pwm-backlight could be used directly.
>
>  Tomi
>