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

[PATCHv2 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 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

[PATCHv2 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    | 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

[PATCHv2 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 | 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

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

[Tomi Valkeinen]

Add DT binding for led-backlight.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 .../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

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

[Jacek Anaszewski]

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 <tomi.valkeinen@ti.com>
> ---
>   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".

>   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>

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;

> +}
> +
> +/**
> + * 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
>


-- 
Best Regards,
Jacek Anaszewski

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

[Andrew Lunn]

On Tue, Sep 08, 2015 at 02:19:36PM +0300, Tomi Valkeinen wrote:
> Add DT binding for led-backlight.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  .../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.

      Andrew

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

[Jacek Anaszewski]

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 <tomi.valkeinen@ti.com>
>> ---
>>   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 +++++++++++++++

[...]

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

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


-- 
Best Regards,
Jacek Anaszewski

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

[Tomi Valkeinen]

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



On 08/09/15 16:41, Andrew Lunn wrote:
> On Tue, Sep 08, 2015 at 02:19:36PM +0300, Tomi Valkeinen wrote:
>> Add DT binding for led-backlight.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  .../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


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

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

[Tomi Valkeinen]

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


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:
>> 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    | 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.

>> +#include <linux/leds.h>
>> +#include <linux/of.h>
>> +#include <linux/leds-of.h>
> 
> Please keep alphabetical order.

Yep.

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

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


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

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

[Tomi Valkeinen]

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



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

>>> +#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


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

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

[Jacek Anaszewski]

On 09/09/2015 02:16 PM, Tomi Valkeinen wrote:
>
>
> On 08/09/15 17:04, Jacek Anaszewski wrote:
>
>>>> +#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