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

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 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                              |  82 +++++++
 drivers/leds/leds.h                                |   1 +
 drivers/video/backlight/Kconfig                    |   7 +
 drivers/video/backlight/Makefile                   |   1 +
 drivers/video/backlight/led_bl.c                   | 235 +++++++++++++++++++++
 include/linux/leds-of.h                            |  26 +++
 include/linux/leds.h                               |   2 +
 10 files changed, 401 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/leds-of.h

-- 
2.1.4

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 <redacted>
---
 drivers/leds/Makefile    |  6 +++-
 drivers/leds/led-class.c | 13 +++++++-
 drivers/leds/led-of.c    | 82 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/leds/leds.h      |  1 +
 include/linux/leds-of.h  | 26 +++++++++++++++
 include/linux/leds.h     |  2 ++
 6 files changed, 128 insertions(+), 2 deletions(-)
 create mode 100644 drivers/leds/led-of.c
 create mode 100644 include/linux/leds-of.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..1234f9dc3537 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 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..32631682be07
--- /dev/null
+++ b/drivers/leds/led-of.c

@@ -0,0 +1,82 @@
+/*
+ * 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/of.h>
+#include <linux/leds-of.h>
+#include <linux/module.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) {
+		if (of_property_match_string(child, "label", led_cdev->name) = 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 ? 1 : 0;
+}
+
+/**
+ * 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-of.h b/include/linux/leds-of.h
new file mode 100644
index 000000000000..7e8e64bd9811
--- /dev/null
+++ b/include/linux/leds-of.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 */

diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eeafb5dc..0fce71a06d68 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h

@@ -113,6 +113,8 @@ 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);
 
+extern void led_put(struct led_classdev *led_cdev);
+
 /**
  * led_blink_set - set blinking with software fallback
  * @led_cdev: the LED to start blinking

-- 
2.1.4

Add DT binding for led-backlight.

Signed-off-by: Tomi Valkeinen <redacted>
---
 .../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..fb77051ac230
--- /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)
+  - power-supply: regulator for supply voltage
+
+Optional properties:
+  - 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

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 <redacted>
---
 drivers/video/backlight/Kconfig  |   7 ++
 drivers/video/backlight/Makefile |   1 +
 drivers/video/backlight/led_bl.c | 235 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 243 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..581d545ea36b
--- /dev/null
+++ b/drivers/video/backlight/led_bl.c

@@ -0,0 +1,235 @@
+/*
+ * 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/leds-of.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) {
+		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);
+
+	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(&pdev->dev, "power");
+	if (IS_ERR(priv->power_supply)) {
+		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

Hi Tomi,

Thanks for the update.

On 09/08/2015 01:19 PM, 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 <redacted>
---
  drivers/leds/Makefile    |  6 +++-
  drivers/leds/led-class.c | 13 +++++++-
  drivers/leds/led-of.c    | 82 ++++++++++++++++++++++++++++++++++++++++++++++++
  drivers/leds/leds.h      |  1 +
  include/linux/leds-of.h  | 26 +++++++++++++++

According to existing naming convention this should be "of_leds.h".

Please keep alphabetical order.

+#include <linux/module.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) {
+		if (of_property_match_string(child, "label", led_cdev->name) = 0)

Line over 80 characters.

+			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 ? 1 : 0;

return led_node = target_node;

-- 
Best Regards,
Jacek Anaszewski

On Tue, Sep 08, 2015 at 02:19:36PM +0300, Tomi Valkeinen wrote:

Hi Tomi

Maybe this regulator should be optional? I could imagine blacklights
without one, in none power sensitive cases, e.g. industrial PC with a
touch screen display, train ticket machine, etc.

      Andrew

On 09/08/2015 01:19 PM, Tomi Valkeinen wrote:

quoted

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 <redacted>
---
  drivers/leds/Makefile    |  6 +++-
  drivers/leds/led-class.c | 13 +++++++-
  drivers/leds/led-of.c    | 82
++++++++++++++++++++++++++++++++++++++++++++++++
  drivers/leds/leds.h      |  1 +
  include/linux/leds-of.h  | 26 +++++++++++++++

[...]

quoted

diff --git a/include/linux/leds-of.h b/include/linux/leds-of.h
new file mode 100644
index 000000000000..7e8e64bd9811
--- /dev/null
+++ b/include/linux/leds-of.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 */

diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eeafb5dc..0fce71a06d68 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h

@@ -113,6 +113,8 @@ 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);

+extern void led_put(struct led_classdev *led_cdev);
+

This also needs no-op version.

quoted

  /**
   * led_blink_set - set blinking with software fallback
   * @led_cdev: the LED to start blinking

-- 
Best Regards,
Jacek Anaszewski

On 08/09/15 16:41, Andrew Lunn wrote:

On Tue, Sep 08, 2015 at 02:19:36PM +0300, Tomi Valkeinen wrote:

quoted

Add DT binding for led-backlight.

Signed-off-by: Tomi Valkeinen <redacted>
---
 .../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..fb77051ac230
--- /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)
+  - power-supply: regulator for supply voltage

Hi Tomi

Maybe this regulator should be optional? I could imagine blacklights
without one, in none power sensitive cases, e.g. industrial PC with a
touch screen display, train ticket machine, etc.

Yes, I think so. Especially in this case as this led-backlight device is
not exactly a specific HW device, but more of a virtual device.

 Tomi

On 08/09/15 16:20, Jacek Anaszewski wrote:

Hi Tomi,

Thanks for the update.

On 09/08/2015 01:19 PM, Tomi Valkeinen wrote:

quoted

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 <redacted>
---
  drivers/leds/Makefile    |  6 +++-
  drivers/leds/led-class.c | 13 +++++++-
  drivers/leds/led-of.c    | 82
++++++++++++++++++++++++++++++++++++++++++++++++
  drivers/leds/leds.h      |  1 +
  include/linux/leds-of.h  | 26 +++++++++++++++

According to existing naming convention this should be "of_leds.h".

Right. I was thinking it's "leds" first, and "of" second, but I see
of_*.h is the convention.

quoted

+#include <linux/leds.h>
+#include <linux/of.h>
+#include <linux/leds-of.h>

Please keep alphabetical order.

Yep.

quoted

+#include <linux/module.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) {
+        if (of_property_match_string(child, "label", led_cdev->name)
== 0)

Line over 80 characters.

I don't like to split lines to exact 80 chars, when it makes the code
more difficult to read. In this case it's 3 chars over 80, and splitting
the function call above to two lines doesn't look nice to me.

I'll do the func call separately, then it stays under 80 chars.

quoted

+            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 ? 1 : 0;

return led_node == target_node;

Again a matter of taste, but to me, == returns a bool, whereas the match
function here returns an int.

But I'm fine with plain == here.

 Tomi

On 08/09/15 17:04, Jacek Anaszewski wrote:

quoted

quoted

+#endif /* __LINUX_LEDS_OF_H_INCLUDED */

diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eeafb5dc..0fce71a06d68 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h

@@ -113,6 +113,8 @@ 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);

+extern void led_put(struct led_classdev *led_cdev);
+

This also needs no-op version.

Ok, but... I think other already existing functions need no-ops also. If
there's a driver that uses of_led_get and led_put, it's sure to use some
other led_* functions also.

So if we want that driver to be compilable when LED support is disabled
in the kernel, we need to provide no-ops for all those functions.

Probably:

led_set_brightness
led_blink_set_oneshot
led_blink_set

 Tomi

On 09/09/2015 02:16 PM, Tomi Valkeinen wrote:

On 08/09/15 17:04, Jacek Anaszewski wrote:

quoted

quoted

quoted

+#endif /* __LINUX_LEDS_OF_H_INCLUDED */

diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eeafb5dc..0fce71a06d68 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h

@@ -113,6 +113,8 @@ 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);

+extern void led_put(struct led_classdev *led_cdev);
+

This also needs no-op version.

Ok, but... I think other already existing functions need no-ops also. If
there's a driver that uses of_led_get and led_put, it's sure to use some
other led_* functions also.

So if we want that driver to be compilable when LED support is disabled
in the kernel, we need to provide no-ops for all those functions.

Probably:

led_set_brightness
led_blink_set_oneshot
led_blink_set

That's right. It needs to be addressed soon too. Potentially
this could show up by breaking randconfig build.

-- 
Best Regards,
Jacek Anaszewski