Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v8 0/5] Add a generic driver for LED-based backlight
@ 2019-10-03  8:28 Jean-Jacques Hiblot
  2019-10-03  8:28 ` [PATCH v8 1/5] leds: populate the device's of_node when possible Jean-Jacques Hiblot
                   ` (5 more replies)
  0 siblings, 6 replies; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-03  8:28 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot

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

A few years ago (2015), Tomi Valkeinen posted a series implementing a
backlight driver on top of a LED device:
https://patchwork.kernel.org/patch/7293991/
https://patchwork.kernel.org/patch/7294001/
https://patchwork.kernel.org/patch/7293981/

The discussion stopped because Tomi lacked the time to work on it.

changes in v8:
- use class_find_device_by_of_node() instead of class_find_device()
- renamed devm_led_get() as devm_of_led_get()

changes in v7:
- rebased on top of linux-leds/for-next
- populate the of_node member of the LED device if fwnode is a of_node
  (this is a new patch and the first of the series).
- devm_led_get() works only when the device tree is used. Add a few checks
  for that.  

changes in v6:
- trim the list of included headers
- remove useless definition of BKL_FULL_BRIGHTNESS

changes in v5:
- removed LED brightness scaling
- disable sysfs the interface of the LEDs when used by the backlight device

changes in v4:
- fix dev_err() messages and commit logs following the advices of Pavel
- cosmetic changes (indents, getting rid of  "? 1 : 0" in
  led_match_led_node())

changes in v3:
- dt binding: don't limit the brightness range to 0-255. Use the range of
  the underlying LEDs. as a side-effect, all LEDs must now have the same
  range
- driver: Adapt to dt binding update.
- driver: rework probe() for clarity and remove the remaining goto.

changes in v2:
- handle more than one LED.
- don't make the backlight device a child of the LED controller.
- make brightness-levels and default-brightness-level optional
- removed the option to use a GPIO enable.
- removed the option to use a regulator. It should be handled by the LED
  core
- don't make any change to the LED core (not needed anymore)

Jean-Jacques Hiblot (3):
  leds: populate the device's of_node when possible
  leds: Add managed API to get a LED from a device driver
  dt-bindings: backlight: Add led-backlight binding

Tomi Valkeinen (2):
  leds: Add of_led_get() and led_put()
  backlight: add led-backlight driver

 .../bindings/leds/backlight/led-backlight.txt |  28 ++
 drivers/leds/led-class.c                      |  98 ++++++-
 drivers/video/backlight/Kconfig               |   7 +
 drivers/video/backlight/Makefile              |   1 +
 drivers/video/backlight/led_bl.c              | 260 ++++++++++++++++++
 include/linux/leds.h                          |   6 +
 6 files changed, 399 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
 create mode 100644 drivers/video/backlight/led_bl.c

-- 
2.17.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v8 1/5] leds: populate the device's of_node when possible
  2019-10-03  8:28 [PATCH v8 0/5] Add a generic driver for LED-based backlight Jean-Jacques Hiblot
@ 2019-10-03  8:28 ` Jean-Jacques Hiblot
  2019-10-04 21:08   ` Jacek Anaszewski
  2019-10-03  8:28 ` [PATCH v8 2/5] leds: Add of_led_get() and led_put() Jean-Jacques Hiblot
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-03  8:28 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot

If initialization data is available and its fwnode is actually a of_node,
store this information in the led device's structure. This will allow the
device to use or provide OF-based API such (devm_xxx).

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
 drivers/leds/led-class.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 647b1263c579..c2167b66b61f 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -276,8 +276,11 @@ int led_classdev_register_ext(struct device *parent,
 		mutex_unlock(&led_cdev->led_access);
 		return PTR_ERR(led_cdev->dev);
 	}
-	if (init_data && init_data->fwnode)
+	if (init_data && init_data->fwnode) {
 		led_cdev->dev->fwnode = init_data->fwnode;
+		if (is_of_node(init_data->fwnode))
+			led_cdev->dev->of_node = to_of_node(init_data->fwnode);
+	}
 
 	if (ret)
 		dev_warn(parent, "Led %s renamed to %s due to name collision",
-- 
2.17.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v8 2/5] leds: Add of_led_get() and led_put()
  2019-10-03  8:28 [PATCH v8 0/5] Add a generic driver for LED-based backlight Jean-Jacques Hiblot
  2019-10-03  8:28 ` [PATCH v8 1/5] leds: populate the device's of_node when possible Jean-Jacques Hiblot
@ 2019-10-03  8:28 ` Jean-Jacques Hiblot
  2019-10-03 10:42   ` Sebastian Reichel
  2019-10-03  8:28 ` [PATCH v8 3/5] leds: Add managed API to get a LED from a device driver Jean-Jacques Hiblot
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-03  8:28 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

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>
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/leds/led-class.c | 44 ++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h     |  4 ++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index c2167b66b61f..455545f5d663 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -19,6 +19,7 @@
 #include <linux/spinlock.h>
 #include <linux/timer.h>
 #include <uapi/linux/uleds.h>
+#include <linux/of.h>
 #include "leds.h"
 
 static struct class *leds_class;
@@ -214,6 +215,49 @@ static int led_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
 
+/**
+ * of_led_get() - request a LED device via the LED framework
+ * @np: device node to get the LED device from
+ * @index: the index of the LED
+ *
+ * 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.
+ */
+struct led_classdev *of_led_get(struct device_node *np, int index)
+{
+	struct device *led_dev;
+	struct led_classdev *led_cdev;
+	struct device_node *led_node;
+
+	led_node = of_parse_phandle(np, "leds", index);
+	if (!led_node)
+		return ERR_PTR(-ENOENT);
+
+	led_dev = class_find_device_by_of_node(leds_class, led_node);
+	of_node_put(led_node);
+
+	if (!led_dev)
+		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);
+
+/**
+ * led_put() - release a LED device
+ * @led_cdev: LED device
+ */
+void led_put(struct led_classdev *led_cdev)
+{
+	module_put(led_cdev->dev->parent->driver->owner);
+}
+EXPORT_SYMBOL_GPL(led_put);
+
 static int led_classdev_next_name(const char *init_name, char *name,
 				  size_t len)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b8df71193329..6f7371bc7757 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -20,6 +20,7 @@
 
 struct device;
 struct led_pattern;
+struct device_node;
 /*
  * LED Core
  */
@@ -196,6 +197,9 @@ 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 struct led_classdev *of_led_get(struct device_node *np, int index);
+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.17.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v8 3/5] leds: Add managed API to get a LED from a device driver
  2019-10-03  8:28 [PATCH v8 0/5] Add a generic driver for LED-based backlight Jean-Jacques Hiblot
  2019-10-03  8:28 ` [PATCH v8 1/5] leds: populate the device's of_node when possible Jean-Jacques Hiblot
  2019-10-03  8:28 ` [PATCH v8 2/5] leds: Add of_led_get() and led_put() Jean-Jacques Hiblot
@ 2019-10-03  8:28 ` Jean-Jacques Hiblot
  2019-10-03 10:47   ` Sebastian Reichel
  2019-10-03  8:28 ` [PATCH v8 4/5] dt-bindings: backlight: Add led-backlight binding Jean-Jacques Hiblot
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-03  8:28 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot

If the LED is acquired by a consumer device with devm_led_get(), it is
automatically released when the device is detached.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/leds/led-class.c | 49 ++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h     |  2 ++
 2 files changed, 51 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 455545f5d663..80c96dd96afc 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -258,6 +258,55 @@ void led_put(struct led_classdev *led_cdev)
 }
 EXPORT_SYMBOL_GPL(led_put);
 
+static void devm_led_release(struct device *dev, void *res)
+{
+	struct led_classdev **p = res;
+
+	led_put(*p);
+}
+
+/**
+ * devm_of_led_get - Resource-managed request of a LED device
+ * @dev:	LED consumer
+ * @index:	index of the LED to obtain in the consumer
+ *
+ * The device node of the device is parse to find the request LED device.
+ * The LED device returned from this function is automatically released
+ * on driver detach.
+ *
+ * @return a pointer to a LED device or ERR_PTR(errno) on failure.
+ */
+struct led_classdev *__must_check devm_of_led_get(struct device *dev,
+						  int index)
+{
+	struct led_classdev *led;
+	struct led_classdev **dr;
+
+	if (!dev)
+		return ERR_PTR(-EINVAL);
+
+	/* Not using device tree? */
+	if (!IS_ENABLED(CONFIG_OF) || !dev->of_node)
+		return ERR_PTR(-ENOTSUPP);
+
+	led = of_led_get(dev->of_node, index);
+	if (IS_ERR(led))
+		return led;
+
+	dr = devres_alloc(devm_led_release, sizeof(struct led_classdev *),
+			  GFP_KERNEL);
+	if (!dr) {
+		led_put(led);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	*dr = led;
+	devres_add(dev, dr);
+
+	return led;
+}
+EXPORT_SYMBOL_GPL(devm_of_led_get);
+
 static int led_classdev_next_name(const char *init_name, char *name,
 				  size_t len)
 {
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 6f7371bc7757..9b94cf752012 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -199,6 +199,8 @@ extern void led_classdev_resume(struct led_classdev *led_cdev);
 
 extern struct led_classdev *of_led_get(struct device_node *np, int index);
 extern void led_put(struct led_classdev *led_cdev);
+struct led_classdev *__must_check devm_of_led_get(struct device *dev,
+						  int index);
 
 /**
  * led_blink_set - set blinking with software fallback
-- 
2.17.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v8 4/5] dt-bindings: backlight: Add led-backlight binding
  2019-10-03  8:28 [PATCH v8 0/5] Add a generic driver for LED-based backlight Jean-Jacques Hiblot
                   ` (2 preceding siblings ...)
  2019-10-03  8:28 ` [PATCH v8 3/5] leds: Add managed API to get a LED from a device driver Jean-Jacques Hiblot
@ 2019-10-03  8:28 ` Jean-Jacques Hiblot
  2019-10-03 11:17   ` Sebastian Reichel
  2019-10-03  8:28 ` [PATCH v8 5/5] backlight: add led-backlight driver Jean-Jacques Hiblot
  2019-10-03 11:40 ` [PATCH v8 0/5] Add a generic driver for LED-based backlight Lee Jones
  5 siblings, 1 reply; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-03  8:28 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot

Add DT binding for led-backlight.

Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 .../bindings/leds/backlight/led-backlight.txt | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/backlight/led-backlight.txt

diff --git a/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
new file mode 100644
index 000000000000..4c7dfbe7f67a
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
@@ -0,0 +1,28 @@
+led-backlight bindings
+
+This binding is used to describe a basic backlight device made of LEDs.
+It can also be used to describe a backlight device controlled by the output of
+a LED driver.
+
+Required properties:
+  - compatible: "led-backlight"
+  - leds: a list of LEDs
+
+Optional properties:
+  - brightness-levels: Array of distinct brightness levels. The levels must be
+                       in the range accepted by the underlying LED devices.
+                       This is used to translate a backlight brightness level
+                       into a LED brightness level. If it is not provided, the
+                       identity mapping is used.
+
+  - default-brightness-level: The default brightness level.
+
+Example:
+
+	backlight {
+		compatible = "led-backlight";
+
+		leds = <&led1>, <&led2>;
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <6>;
+	};
-- 
2.17.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH v8 5/5] backlight: add led-backlight driver
  2019-10-03  8:28 [PATCH v8 0/5] Add a generic driver for LED-based backlight Jean-Jacques Hiblot
                   ` (3 preceding siblings ...)
  2019-10-03  8:28 ` [PATCH v8 4/5] dt-bindings: backlight: Add led-backlight binding Jean-Jacques Hiblot
@ 2019-10-03  8:28 ` Jean-Jacques Hiblot
  2019-10-03 11:47   ` Sebastian Reichel
  2019-10-03 11:40 ` [PATCH v8 0/5] Add a generic driver for LED-based backlight Lee Jones
  5 siblings, 1 reply; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-03  8:28 UTC (permalink / raw)
  To: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen,
	Jean-Jacques Hiblot

From: Tomi Valkeinen <tomi.valkeinen@ti.com>

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. Multiple LEDs can be used for a single backlight.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
Acked-by: Pavel Machek <pavel@ucw.cz>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 drivers/video/backlight/Kconfig  |   7 +
 drivers/video/backlight/Makefile |   1 +
 drivers/video/backlight/led_bl.c | 260 +++++++++++++++++++++++++++++++
 3 files changed, 268 insertions(+)
 create mode 100644 drivers/video/backlight/led_bl.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 8b081d61773e..585a1787618c 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -458,6 +458,13 @@ config BACKLIGHT_RAVE_SP
 	help
 	  Support for backlight control on RAVE SP device.
 
+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
 
 endmenu
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index 63c507c07437..2a67642966a5 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -57,3 +57,4 @@ obj-$(CONFIG_BACKLIGHT_TPS65217)	+= tps65217_bl.o
 obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
 obj-$(CONFIG_BACKLIGHT_ARCXCNN) 	+= arcxcnn_bl.o
 obj-$(CONFIG_BACKLIGHT_RAVE_SP)		+= rave-sp-backlight.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..3f66549997c8
--- /dev/null
+++ b/drivers/video/backlight/led_bl.c
@@ -0,0 +1,260 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2015-2019 Texas Instruments Incorporated -  http://www.ti.com/
+ * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
+ *
+ * Based on pwm_bl.c
+ */
+
+#include <linux/backlight.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+struct led_bl_data {
+	struct device		*dev;
+	struct backlight_device	*bl_dev;
+	struct led_classdev	**leds;
+	bool			enabled;
+	int			nb_leds;
+	unsigned int		*levels;
+	unsigned int		default_brightness;
+	unsigned int		max_brightness;
+};
+
+static void led_bl_set_brightness(struct led_bl_data *priv, int level)
+{
+	int i;
+	int bkl_brightness;
+
+	if (priv->levels)
+		bkl_brightness = priv->levels[level];
+	else
+		bkl_brightness = level;
+
+	for (i = 0; i < priv->nb_leds; i++)
+		led_set_brightness(priv->leds[i], bkl_brightness);
+
+	priv->enabled = true;
+}
+
+static void led_bl_power_off(struct led_bl_data *priv)
+{
+	int i;
+
+	if (!priv->enabled)
+		return;
+
+	for (i = 0; i < priv->nb_leds; i++)
+		led_set_brightness(priv->leds[i], LED_OFF);
+
+	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_get_leds(struct device *dev,
+			   struct led_bl_data *priv)
+{
+	int i, nb_leds, ret;
+	struct device_node *node = dev->of_node;
+	struct led_classdev **leds;
+	unsigned int max_brightness;
+	unsigned int default_brightness;
+
+	ret = of_count_phandle_with_args(node, "leds", NULL);
+	if (ret < 0) {
+		dev_err(dev, "Unable to get led count\n");
+		return -EINVAL;
+	}
+
+	nb_leds = ret;
+	if (nb_leds < 1) {
+		dev_err(dev, "At least one LED must be specified!\n");
+		return -EINVAL;
+	}
+
+	leds = devm_kzalloc(dev, sizeof(struct led_classdev *) * nb_leds,
+			    GFP_KERNEL);
+	if (!leds)
+		return -ENOMEM;
+
+	for (i = 0; i < nb_leds; i++) {
+		leds[i] = devm_of_led_get(dev, i);
+		if (IS_ERR(leds[i]))
+			return PTR_ERR(leds[i]);
+	}
+
+	/* check that the LEDs all have the same brightness range */
+	max_brightness = leds[0]->max_brightness;
+	for (i = 1; i < nb_leds; i++) {
+		if (max_brightness != leds[i]->max_brightness) {
+			dev_err(dev, "LEDs must have identical ranges\n");
+			return -EINVAL;
+		}
+	}
+
+	/* get the default brightness from the first LED from the list */
+	default_brightness = leds[0]->brightness;
+
+	priv->nb_leds = nb_leds;
+	priv->leds = leds;
+	priv->max_brightness = max_brightness;
+	priv->default_brightness = default_brightness;
+
+	return 0;
+}
+
+static int led_bl_parse_levels(struct device *dev,
+			   struct led_bl_data *priv)
+{
+	struct device_node *node = dev->of_node;
+	int num_levels;
+	u32 value;
+	int ret;
+
+	if (!node)
+		return -ENODEV;
+
+	num_levels = of_property_count_u32_elems(node, "brightness-levels");
+	if (num_levels > 1) {
+		int i;
+		unsigned int db;
+		u32 *levels = NULL;
+
+		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)
+			return ret;
+
+		/*
+		 * Try to map actual LED brightness to backlight brightness
+		 * level
+		 */
+		db = priv->default_brightness;
+		for (i = 0 ; i < num_levels; i++) {
+			if ((i && db > levels[i-1]) && db <= levels[i])
+				break;
+		}
+		priv->default_brightness = i;
+		priv->max_brightness = num_levels - 1;
+		priv->levels = levels;
+	} else if (num_levels >= 0)
+		dev_warn(dev, "Not enough levels defined\n");
+
+	ret = of_property_read_u32(node, "default-brightness-level", &value);
+	if (!ret && value <= priv->max_brightness)
+		priv->default_brightness = value;
+	else if (!ret  && value > priv->max_brightness)
+		dev_warn(dev, "Invalid default brightness. Ignoring it\n");
+
+	return 0;
+}
+
+static int led_bl_probe(struct platform_device *pdev)
+{
+	struct backlight_properties props;
+	struct led_bl_data *priv;
+	int ret, i;
+
+	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_get_leds(&pdev->dev, priv);
+	if (ret)
+		return ret;
+
+	ret = led_bl_parse_levels(&pdev->dev, priv);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to parse DT data\n");
+		return ret;
+	}
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_RAW;
+	props.max_brightness = priv->max_brightness;
+	props.brightness = priv->default_brightness;
+	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
+		      FB_BLANK_UNBLANK;
+	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");
+		return PTR_ERR(priv->bl_dev);
+	}
+
+	for (i = 0; i < priv->nb_leds; i++)
+		led_sysfs_disable(priv->leds[i]);
+
+	backlight_update_status(priv->bl_dev);
+
+	return 0;
+}
+
+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;
+	int i;
+
+	backlight_device_unregister(bl);
+
+	led_bl_power_off(priv);
+	for (i = 0; i < priv->nb_leds; i++)
+		led_sysfs_enable(priv->leds[i]);
+
+	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.17.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()
  2019-10-03  8:28 ` [PATCH v8 2/5] leds: Add of_led_get() and led_put() Jean-Jacques Hiblot
@ 2019-10-03 10:42   ` Sebastian Reichel
  2019-10-03 12:47     ` Jean-Jacques Hiblot
  0 siblings, 1 reply; 29+ messages in thread
From: Sebastian Reichel @ 2019-10-03 10:42 UTC (permalink / raw)
  To: Jean-Jacques Hiblot
  Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, linux-kernel, dri-devel, tomi.valkeinen,
	dmurphy, linux-leds

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

Hi,

On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> 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>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> ---
>  drivers/leds/led-class.c | 44 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/leds.h     |  4 ++++
>  2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index c2167b66b61f..455545f5d663 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -19,6 +19,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/timer.h>
>  #include <uapi/linux/uleds.h>
> +#include <linux/of.h>
>  #include "leds.h"
>  
>  static struct class *leds_class;
> @@ -214,6 +215,49 @@ static int led_resume(struct device *dev)
>  
>  static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
>  
> +/**
> + * of_led_get() - request a LED device via the LED framework
> + * @np: device node to get the LED device from
> + * @index: the index of the LED
> + *
> + * 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.
> + */
> +struct led_classdev *of_led_get(struct device_node *np, int index)
> +{
> +	struct device *led_dev;
> +	struct led_classdev *led_cdev;
> +	struct device_node *led_node;
> +
> +	led_node = of_parse_phandle(np, "leds", index);
> +	if (!led_node)
> +		return ERR_PTR(-ENOENT);
> +
> +	led_dev = class_find_device_by_of_node(leds_class, led_node);

If you convert led_node into a fwnode, you can use
class_find_device_by_fwnode() instead. That way the
first patch can just be dropped.

-- Sebastian

> +	of_node_put(led_node);
> +
> +	if (!led_dev)
> +		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);
> +
> +/**
> + * led_put() - release a LED device
> + * @led_cdev: LED device
> + */
> +void led_put(struct led_classdev *led_cdev)
> +{
> +	module_put(led_cdev->dev->parent->driver->owner);
> +}
> +EXPORT_SYMBOL_GPL(led_put);
> +
>  static int led_classdev_next_name(const char *init_name, char *name,
>  				  size_t len)
>  {
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b8df71193329..6f7371bc7757 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -20,6 +20,7 @@
>  
>  struct device;
>  struct led_pattern;
> +struct device_node;
>  /*
>   * LED Core
>   */
> @@ -196,6 +197,9 @@ 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 struct led_classdev *of_led_get(struct device_node *np, int index);
> +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.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 3/5] leds: Add managed API to get a LED from a device driver
  2019-10-03  8:28 ` [PATCH v8 3/5] leds: Add managed API to get a LED from a device driver Jean-Jacques Hiblot
@ 2019-10-03 10:47   ` Sebastian Reichel
  0 siblings, 0 replies; 29+ messages in thread
From: Sebastian Reichel @ 2019-10-03 10:47 UTC (permalink / raw)
  To: Jean-Jacques Hiblot
  Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, linux-kernel, dri-devel, tomi.valkeinen,
	dmurphy, linux-leds

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

Hi,

On Thu, Oct 03, 2019 at 10:28:10AM +0200, Jean-Jacques Hiblot wrote:
> If the LED is acquired by a consumer device with devm_led_get(), it is
> automatically released when the device is detached.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> ---
>  drivers/leds/led-class.c | 49 ++++++++++++++++++++++++++++++++++++++++
>  include/linux/leds.h     |  2 ++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 455545f5d663..80c96dd96afc 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -258,6 +258,55 @@ void led_put(struct led_classdev *led_cdev)
>  }
>  EXPORT_SYMBOL_GPL(led_put);
>  
> +static void devm_led_release(struct device *dev, void *res)
> +{
> +	struct led_classdev **p = res;
> +
> +	led_put(*p);
> +}
> +
> +/**
> + * devm_of_led_get - Resource-managed request of a LED device
> + * @dev:	LED consumer
> + * @index:	index of the LED to obtain in the consumer
> + *
> + * The device node of the device is parse to find the request LED device.
> + * The LED device returned from this function is automatically released
> + * on driver detach.
> + *
> + * @return a pointer to a LED device or ERR_PTR(errno) on failure.
> + */
> +struct led_classdev *__must_check devm_of_led_get(struct device *dev,
> +						  int index)
> +{
> +	struct led_classdev *led;
> +	struct led_classdev **dr;
> +
> +	if (!dev)
> +		return ERR_PTR(-EINVAL);
> +
> +	/* Not using device tree? */
> +	if (!IS_ENABLED(CONFIG_OF) || !dev->of_node)
> +		return ERR_PTR(-ENOTSUPP);

I suggested to move the CONFIG_OF check and the NULL check for
of_node into of_led_get(). Otherwise

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

> +
> +	led = of_led_get(dev->of_node, index);
> +	if (IS_ERR(led))
> +		return led;
> +
> +	dr = devres_alloc(devm_led_release, sizeof(struct led_classdev *),
> +			  GFP_KERNEL);
> +	if (!dr) {
> +		led_put(led);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	*dr = led;
> +	devres_add(dev, dr);
> +
> +	return led;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_led_get);
> +
>  static int led_classdev_next_name(const char *init_name, char *name,
>  				  size_t len)
>  {
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 6f7371bc7757..9b94cf752012 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -199,6 +199,8 @@ extern void led_classdev_resume(struct led_classdev *led_cdev);
>  
>  extern struct led_classdev *of_led_get(struct device_node *np, int index);
>  extern void led_put(struct led_classdev *led_cdev);
> +struct led_classdev *__must_check devm_of_led_get(struct device *dev,
> +						  int index);
>  
>  /**
>   * led_blink_set - set blinking with software fallback
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 4/5] dt-bindings: backlight: Add led-backlight binding
  2019-10-03  8:28 ` [PATCH v8 4/5] dt-bindings: backlight: Add led-backlight binding Jean-Jacques Hiblot
@ 2019-10-03 11:17   ` Sebastian Reichel
  0 siblings, 0 replies; 29+ messages in thread
From: Sebastian Reichel @ 2019-10-03 11:17 UTC (permalink / raw)
  To: Jean-Jacques Hiblot
  Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, linux-kernel, dri-devel, tomi.valkeinen,
	dmurphy, linux-leds

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

Hi,

On Thu, Oct 03, 2019 at 10:28:11AM +0200, Jean-Jacques Hiblot wrote:
> Add DT binding for led-backlight.
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

> ---
>  .../bindings/leds/backlight/led-backlight.txt | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
> new file mode 100644
> index 000000000000..4c7dfbe7f67a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
> @@ -0,0 +1,28 @@
> +led-backlight bindings
> +
> +This binding is used to describe a basic backlight device made of LEDs.
> +It can also be used to describe a backlight device controlled by the output of
> +a LED driver.
> +
> +Required properties:
> +  - compatible: "led-backlight"
> +  - leds: a list of LEDs
> +
> +Optional properties:
> +  - brightness-levels: Array of distinct brightness levels. The levels must be
> +                       in the range accepted by the underlying LED devices.
> +                       This is used to translate a backlight brightness level
> +                       into a LED brightness level. If it is not provided, the
> +                       identity mapping is used.
> +
> +  - default-brightness-level: The default brightness level.
> +
> +Example:
> +
> +	backlight {
> +		compatible = "led-backlight";
> +
> +		leds = <&led1>, <&led2>;
> +		brightness-levels = <0 4 8 16 32 64 128 255>;
> +		default-brightness-level = <6>;
> +	};
> -- 
> 2.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 0/5] Add a generic driver for LED-based backlight
  2019-10-03  8:28 [PATCH v8 0/5] Add a generic driver for LED-based backlight Jean-Jacques Hiblot
                   ` (4 preceding siblings ...)
  2019-10-03  8:28 ` [PATCH v8 5/5] backlight: add led-backlight driver Jean-Jacques Hiblot
@ 2019-10-03 11:40 ` Lee Jones
  2019-10-08 19:55   ` Pavel Machek
  5 siblings, 1 reply; 29+ messages in thread
From: Lee Jones @ 2019-10-03 11:40 UTC (permalink / raw)
  To: Jean-Jacques Hiblot
  Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson,
	dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen

On Thu, 03 Oct 2019, Jean-Jacques Hiblot wrote:

> This series aims to add a led-backlight driver, similar to pwm-backlight,
> but using a LED class device underneath.
> 
> A few years ago (2015), Tomi Valkeinen posted a series implementing a
> backlight driver on top of a LED device:
> https://patchwork.kernel.org/patch/7293991/
> https://patchwork.kernel.org/patch/7294001/
> https://patchwork.kernel.org/patch/7293981/
> 
> The discussion stopped because Tomi lacked the time to work on it.

[...]

>  .../bindings/leds/backlight/led-backlight.txt |  28 ++
>  drivers/leds/led-class.c                      |  98 ++++++-
>  drivers/video/backlight/Kconfig               |   7 +
>  drivers/video/backlight/Makefile              |   1 +
>  drivers/video/backlight/led_bl.c              | 260 ++++++++++++++++++
>  include/linux/leds.h                          |   6 +
>  6 files changed, 399 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
>  create mode 100644 drivers/video/backlight/led_bl.c

How should this set be processed?  I'm happy to take it through
Backlight via an immutable branch and PR to be consumed by LED.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 5/5] backlight: add led-backlight driver
  2019-10-03  8:28 ` [PATCH v8 5/5] backlight: add led-backlight driver Jean-Jacques Hiblot
@ 2019-10-03 11:47   ` Sebastian Reichel
  2019-10-03 17:23     ` Jean-Jacques Hiblot
  0 siblings, 1 reply; 29+ messages in thread
From: Sebastian Reichel @ 2019-10-03 11:47 UTC (permalink / raw)
  To: Jean-Jacques Hiblot
  Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, linux-kernel, dri-devel, tomi.valkeinen,
	dmurphy, linux-leds

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

Hi,

On Thu, Oct 03, 2019 at 10:28:12AM +0200, Jean-Jacques Hiblot wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> 
> 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. Multiple LEDs can be used for a single backlight.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

(with some suggestions below)

>  drivers/video/backlight/Kconfig  |   7 +
>  drivers/video/backlight/Makefile |   1 +
>  drivers/video/backlight/led_bl.c | 260 +++++++++++++++++++++++++++++++
>  3 files changed, 268 insertions(+)
>  create mode 100644 drivers/video/backlight/led_bl.c
> 
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 8b081d61773e..585a1787618c 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -458,6 +458,13 @@ config BACKLIGHT_RAVE_SP
>  	help
>  	  Support for backlight control on RAVE SP device.
>  
> +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
>  
>  endmenu
> diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
> index 63c507c07437..2a67642966a5 100644
> --- a/drivers/video/backlight/Makefile
> +++ b/drivers/video/backlight/Makefile
> @@ -57,3 +57,4 @@ obj-$(CONFIG_BACKLIGHT_TPS65217)	+= tps65217_bl.o
>  obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
>  obj-$(CONFIG_BACKLIGHT_ARCXCNN) 	+= arcxcnn_bl.o
>  obj-$(CONFIG_BACKLIGHT_RAVE_SP)		+= rave-sp-backlight.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..3f66549997c8
> --- /dev/null
> +++ b/drivers/video/backlight/led_bl.c
> @@ -0,0 +1,260 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2015-2019 Texas Instruments Incorporated -  http://www.ti.com/
> + * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> + *
> + * Based on pwm_bl.c
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +struct led_bl_data {
> +	struct device		*dev;
> +	struct backlight_device	*bl_dev;
> +	struct led_classdev	**leds;
> +	bool			enabled;
> +	int			nb_leds;
> +	unsigned int		*levels;
> +	unsigned int		default_brightness;
> +	unsigned int		max_brightness;
> +};
> +
> +static void led_bl_set_brightness(struct led_bl_data *priv, int level)
> +{
> +	int i;
> +	int bkl_brightness;
> +
> +	if (priv->levels)
> +		bkl_brightness = priv->levels[level];
> +	else
> +		bkl_brightness = level;
> +
> +	for (i = 0; i < priv->nb_leds; i++)
> +		led_set_brightness(priv->leds[i], bkl_brightness);
> +
> +	priv->enabled = true;
> +}
> +
> +static void led_bl_power_off(struct led_bl_data *priv)
> +{
> +	int i;
> +
> +	if (!priv->enabled)
> +		return;
> +
> +	for (i = 0; i < priv->nb_leds; i++)
> +		led_set_brightness(priv->leds[i], LED_OFF);
> +
> +	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_get_leds(struct device *dev,
> +			   struct led_bl_data *priv)
> +{
> +	int i, nb_leds, ret;
> +	struct device_node *node = dev->of_node;
> +	struct led_classdev **leds;
> +	unsigned int max_brightness;
> +	unsigned int default_brightness;
> +
> +	ret = of_count_phandle_with_args(node, "leds", NULL);
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to get led count\n");
> +		return -EINVAL;
> +	}
> +
> +	nb_leds = ret;
> +	if (nb_leds < 1) {
> +		dev_err(dev, "At least one LED must be specified!\n");
> +		return -EINVAL;
> +	}
> +
> +	leds = devm_kzalloc(dev, sizeof(struct led_classdev *) * nb_leds,
> +			    GFP_KERNEL);
> +	if (!leds)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nb_leds; i++) {
> +		leds[i] = devm_of_led_get(dev, i);
> +		if (IS_ERR(leds[i]))
> +			return PTR_ERR(leds[i]);
> +	}
> +
> +	/* check that the LEDs all have the same brightness range */
> +	max_brightness = leds[0]->max_brightness;
> +	for (i = 1; i < nb_leds; i++) {
> +		if (max_brightness != leds[i]->max_brightness) {
> +			dev_err(dev, "LEDs must have identical ranges\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/* get the default brightness from the first LED from the list */
> +	default_brightness = leds[0]->brightness;
> +
> +	priv->nb_leds = nb_leds;
> +	priv->leds = leds;
> +	priv->max_brightness = max_brightness;
> +	priv->default_brightness = default_brightness;
> +
> +	return 0;
> +}
> +
> +static int led_bl_parse_levels(struct device *dev,
> +			   struct led_bl_data *priv)
> +{
> +	struct device_node *node = dev->of_node;
> +	int num_levels;
> +	u32 value;
> +	int ret;
> +
> +	if (!node)
> +		return -ENODEV;
> +
> +	num_levels = of_property_count_u32_elems(node, "brightness-levels");
> +	if (num_levels > 1) {
> +		int i;
> +		unsigned int db;
> +		u32 *levels = NULL;
> +
> +		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)
> +			return ret;
> +
> +		/*
> +		 * Try to map actual LED brightness to backlight brightness
> +		 * level
> +		 */
> +		db = priv->default_brightness;
> +		for (i = 0 ; i < num_levels; i++) {
> +			if ((i && db > levels[i-1]) && db <= levels[i])
> +				break;
> +		}
> +		priv->default_brightness = i;
> +		priv->max_brightness = num_levels - 1;
> +		priv->levels = levels;
> +	} else if (num_levels >= 0)
> +		dev_warn(dev, "Not enough levels defined\n");
> +
> +	ret = of_property_read_u32(node, "default-brightness-level", &value);
> +	if (!ret && value <= priv->max_brightness)
> +		priv->default_brightness = value;
> +	else if (!ret  && value > priv->max_brightness)
> +		dev_warn(dev, "Invalid default brightness. Ignoring it\n");
> +
> +	return 0;
> +}
> +
> +static int led_bl_probe(struct platform_device *pdev)
> +{
> +	struct backlight_properties props;
> +	struct led_bl_data *priv;
> +	int ret, i;
> +
> +	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_get_leds(&pdev->dev, priv);
> +	if (ret)
> +		return ret;
> +
> +	ret = led_bl_parse_levels(&pdev->dev, priv);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to parse DT data\n");
> +		return ret;
> +	}
> +
> +	memset(&props, 0, sizeof(struct backlight_properties));
> +	props.type = BACKLIGHT_RAW;
> +	props.max_brightness = priv->max_brightness;
> +	props.brightness = priv->default_brightness;
> +	props.power = (priv->default_brightness > 0) ? FB_BLANK_POWERDOWN :
> +		      FB_BLANK_UNBLANK;
> +	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");
> +		return PTR_ERR(priv->bl_dev);
> +	}
> +
> +	for (i = 0; i < priv->nb_leds; i++)
> +		led_sysfs_disable(priv->leds[i]);

I suggest to restructure:

1. call led_sysfs_disable

2. use devm_add_action_or_reset() to register the
   led_sysfs_enable loop

3. use devm_backlight_device_register() to register BL

4. drop the remove function

> +
> +	backlight_update_status(priv->bl_dev);
> +
> +	return 0;
> +}
> +
> +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;
> +	int i;
> +
> +	backlight_device_unregister(bl);
> +
> +	led_bl_power_off(priv);
> +	for (i = 0; i < priv->nb_leds; i++)
> +		led_sysfs_enable(priv->leds[i]);
> +
> +	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),

You should drop of_match_ptr(). Since the driver depends on OF,
it will always simply return led_bl_of_match.

(Also after removing the OF dependency from the driver it would
either require led_bl_of_match to be marked __maybe_unused or
moving it into a #if CONFIG_OF area to avoid warnings.)

-- Sebastian

> +	},
> +	.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.17.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()
  2019-10-03 10:42   ` Sebastian Reichel
@ 2019-10-03 12:47     ` Jean-Jacques Hiblot
  2019-10-03 17:43       ` Jacek Anaszewski
  0 siblings, 1 reply; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-03 12:47 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, linux-kernel, dri-devel, tomi.valkeinen,
	dmurphy, linux-leds

Hi Sebastian,

On 03/10/2019 12:42, Sebastian Reichel wrote:
> Hi,
>
> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:
>> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>
>> 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>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>> ---
>>   drivers/leds/led-class.c | 44 ++++++++++++++++++++++++++++++++++++++++
>>   include/linux/leds.h     |  4 ++++
>>   2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index c2167b66b61f..455545f5d663 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -19,6 +19,7 @@
>>   #include <linux/spinlock.h>
>>   #include <linux/timer.h>
>>   #include <uapi/linux/uleds.h>
>> +#include <linux/of.h>
>>   #include "leds.h"
>>   
>>   static struct class *leds_class;
>> @@ -214,6 +215,49 @@ static int led_resume(struct device *dev)
>>   
>>   static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
>>   
>> +/**
>> + * of_led_get() - request a LED device via the LED framework
>> + * @np: device node to get the LED device from
>> + * @index: the index of the LED
>> + *
>> + * 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.
>> + */
>> +struct led_classdev *of_led_get(struct device_node *np, int index)
>> +{
>> +	struct device *led_dev;
>> +	struct led_classdev *led_cdev;
>> +	struct device_node *led_node;
>> +
>> +	led_node = of_parse_phandle(np, "leds", index);
>> +	if (!led_node)
>> +		return ERR_PTR(-ENOENT);
>> +
>> +	led_dev = class_find_device_by_of_node(leds_class, led_node);
> If you convert led_node into a fwnode, you can use
> class_find_device_by_fwnode() instead. That way the
> first patch can just be dropped.

Thanks for the reviews.

The first patch could be dropped  indeed, but it would break something 
else I'm working on: getting regulator support in the LED core.

This has been discussed during the v7 iteration of this series.

JJ


>
> -- Sebastian
>
>> +	of_node_put(led_node);
>> +
>> +	if (!led_dev)
>> +		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);
>> +
>> +/**
>> + * led_put() - release a LED device
>> + * @led_cdev: LED device
>> + */
>> +void led_put(struct led_classdev *led_cdev)
>> +{
>> +	module_put(led_cdev->dev->parent->driver->owner);
>> +}
>> +EXPORT_SYMBOL_GPL(led_put);
>> +
>>   static int led_classdev_next_name(const char *init_name, char *name,
>>   				  size_t len)
>>   {
>> diff --git a/include/linux/leds.h b/include/linux/leds.h
>> index b8df71193329..6f7371bc7757 100644
>> --- a/include/linux/leds.h
>> +++ b/include/linux/leds.h
>> @@ -20,6 +20,7 @@
>>   
>>   struct device;
>>   struct led_pattern;
>> +struct device_node;
>>   /*
>>    * LED Core
>>    */
>> @@ -196,6 +197,9 @@ 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 struct led_classdev *of_led_get(struct device_node *np, int index);
>> +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.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 5/5] backlight: add led-backlight driver
  2019-10-03 11:47   ` Sebastian Reichel
@ 2019-10-03 17:23     ` Jean-Jacques Hiblot
  0 siblings, 0 replies; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-03 17:23 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: jacek.anaszewski, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, linux-kernel, dri-devel, tomi.valkeinen,
	dmurphy, linux-leds

Sebastian,

On 03/10/2019 13:47, Sebastian Reichel wrote:
> Hi,
>
> On Thu, Oct 03, 2019 at 10:28:12AM +0200, Jean-Jacques Hiblot wrote:
>> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>
>> 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. Multiple LEDs can be used for a single backlight.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
>> ---
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
>
> (with some suggestions below)

[...]


> I suggest to restructure:
>
> 1. call led_sysfs_disable
>
> 2. use devm_add_action_or_reset() to register the
>     led_sysfs_enable loop
>
> 3. use devm_backlight_device_register() to register BL
>
> 4. drop the remove function
>
>> +
>> +	backlight_update_status(priv->bl_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +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;
>> +	int i;
>> +
>> +	backlight_device_unregister(bl);
>> +
>> +	led_bl_power_off(priv);
>> +	for (i = 0; i < priv->nb_leds; i++)
>> +		led_sysfs_enable(priv->leds[i]);
>> +
>> +	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),
> You should drop of_match_ptr(). Since the driver depends on OF,
> it will always simply return led_bl_of_match.
>
> (Also after removing the OF dependency from the driver it would
> either require led_bl_of_match to be marked __maybe_unused or
> moving it into a #if CONFIG_OF area to avoid warnings.)

Thanks for the suggestions. I think I'll work on them as a separate 
thing and post them after this is merged if there are no others changes 
required.

JJ

>
> -- Sebastian
>
>> +	},
>> +	.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.17.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()
  2019-10-03 12:47     ` Jean-Jacques Hiblot
@ 2019-10-03 17:43       ` Jacek Anaszewski
  2019-10-03 18:35         ` Mark Brown
  0 siblings, 1 reply; 29+ messages in thread
From: Jacek Anaszewski @ 2019-10-03 17:43 UTC (permalink / raw)
  To: Jean-Jacques Hiblot, Sebastian Reichel
  Cc: pavel, robh+dt, mark.rutland, lee.jones, daniel.thompson,
	linux-kernel, dri-devel, tomi.valkeinen, dmurphy, linux-leds,
	Liam Girdwood, Mark Brown

Hi Jean,

On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote:
> Hi Sebastian,
> 
> On 03/10/2019 12:42, Sebastian Reichel wrote:
>> Hi,
>>
>> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:
>>> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>>
>>> 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>
>>> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>> ---
>>>   drivers/leds/led-class.c | 44 ++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/leds.h     |  4 ++++
>>>   2 files changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>>> index c2167b66b61f..455545f5d663 100644
>>> --- a/drivers/leds/led-class.c
>>> +++ b/drivers/leds/led-class.c
>>> @@ -19,6 +19,7 @@
>>>   #include <linux/spinlock.h>
>>>   #include <linux/timer.h>
>>>   #include <uapi/linux/uleds.h>
>>> +#include <linux/of.h>
>>>   #include "leds.h"
>>>     static struct class *leds_class;
>>> @@ -214,6 +215,49 @@ static int led_resume(struct device *dev)
>>>     static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend,
>>> led_resume);
>>>   +/**
>>> + * of_led_get() - request a LED device via the LED framework
>>> + * @np: device node to get the LED device from
>>> + * @index: the index of the LED
>>> + *
>>> + * 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.
>>> + */
>>> +struct led_classdev *of_led_get(struct device_node *np, int index)
>>> +{
>>> +    struct device *led_dev;
>>> +    struct led_classdev *led_cdev;
>>> +    struct device_node *led_node;
>>> +
>>> +    led_node = of_parse_phandle(np, "leds", index);
>>> +    if (!led_node)
>>> +        return ERR_PTR(-ENOENT);
>>> +
>>> +    led_dev = class_find_device_by_of_node(leds_class, led_node);
>> If you convert led_node into a fwnode, you can use
>> class_find_device_by_fwnode() instead. That way the
>> first patch can just be dropped.
> 
> Thanks for the reviews.
> 
> The first patch could be dropped  indeed, but it would break something
> else I'm working on: getting regulator support in the LED core.
> 
> This has been discussed during the v7 iteration of this series.

I wonder if it wouldn't make sense to add support for fwnode
parsing to regulator core. Or maybe it is either somehow supported
or not supported on purpose?

These are questions to regulator maintainers.

Adding Liam and Mark.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()
  2019-10-03 17:43       ` Jacek Anaszewski
@ 2019-10-03 18:35         ` Mark Brown
  2019-10-03 19:21           ` Jacek Anaszewski
  0 siblings, 1 reply; 29+ messages in thread
From: Mark Brown @ 2019-10-03 18:35 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Jean-Jacques Hiblot, Sebastian Reichel, pavel, robh+dt,
	mark.rutland, lee.jones, daniel.thompson, linux-kernel,
	dri-devel, tomi.valkeinen, dmurphy, linux-leds, Liam Girdwood

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

On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote:
> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote:
> > On 03/10/2019 12:42, Sebastian Reichel wrote:
> >> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:

This mail has nothing relevant in the subject line and pages of quotes
before the question for me, it's kind of lucky I noticed it....

> I wonder if it wouldn't make sense to add support for fwnode
> parsing to regulator core. Or maybe it is either somehow supported
> or not supported on purpose?

Anything attempting to use the regulator DT bindings in ACPI has very
serious problems, ACPI has its own power model which isn't compatible
with that used in DT.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()
  2019-10-03 18:35         ` Mark Brown
@ 2019-10-03 19:21           ` Jacek Anaszewski
  2019-10-03 19:41             ` Mark Brown
  0 siblings, 1 reply; 29+ messages in thread
From: Jacek Anaszewski @ 2019-10-03 19:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: Jean-Jacques Hiblot, Sebastian Reichel, pavel, robh+dt,
	mark.rutland, lee.jones, daniel.thompson, linux-kernel,
	dri-devel, tomi.valkeinen, dmurphy, linux-leds, Liam Girdwood

Hi Mark,

On 10/3/19 8:35 PM, Mark Brown wrote:
> On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote:
>> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote:
>>> On 03/10/2019 12:42, Sebastian Reichel wrote:
>>>> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:
> 
> This mail has nothing relevant in the subject line and pages of quotes
> before the question for me, it's kind of lucky I noticed it....

Isn't it all about creating proper filters?

>> I wonder if it wouldn't make sense to add support for fwnode
>> parsing to regulator core. Or maybe it is either somehow supported
>> or not supported on purpose?
> 
> Anything attempting to use the regulator DT bindings in ACPI has very
> serious problems, ACPI has its own power model which isn't compatible
> with that used in DT.

We have a means for checking if fwnode refers to of_node:

is_of_node(const struct fwnode_handle *fwnode)

Couldn't it be employed for OF case?

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()
  2019-10-03 19:21           ` Jacek Anaszewski
@ 2019-10-03 19:41             ` Mark Brown
  2019-10-03 20:27               ` Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()) Jacek Anaszewski
  0 siblings, 1 reply; 29+ messages in thread
From: Mark Brown @ 2019-10-03 19:41 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Jean-Jacques Hiblot, Sebastian Reichel, pavel, robh+dt,
	mark.rutland, lee.jones, daniel.thompson, linux-kernel,
	dri-devel, tomi.valkeinen, dmurphy, linux-leds, Liam Girdwood

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

On Thu, Oct 03, 2019 at 09:21:06PM +0200, Jacek Anaszewski wrote:
> On 10/3/19 8:35 PM, Mark Brown wrote:
> > On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote:
> >> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote:
> >>> On 03/10/2019 12:42, Sebastian Reichel wrote:
> >>>> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:

> > This mail has nothing relevant in the subject line and pages of quotes
> > before the question for me, it's kind of lucky I noticed it....

> Isn't it all about creating proper filters?

My point there is that there's nothing obvious in the mail that suggests
it should get past filters - just being CCed on a mail isn't super
reliable, people often get pulled in due to things like checkpatch or
someone copying a CC list from an earlier patch series where there were
things were relevant.

> >> I wonder if it wouldn't make sense to add support for fwnode
> >> parsing to regulator core. Or maybe it is either somehow supported
> >> or not supported on purpose?

> > Anything attempting to use the regulator DT bindings in ACPI has very
> > serious problems, ACPI has its own power model which isn't compatible
> > with that used in DT.

> We have a means for checking if fwnode refers to of_node:

> is_of_node(const struct fwnode_handle *fwnode)

> Couldn't it be employed for OF case?

Why would we want to do that?  We'd continue to support only DT systems,
just with code that's less obviously DT only and would need to put
checks in.  I'm not seeing an upside here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put())
  2019-10-03 19:41             ` Mark Brown
@ 2019-10-03 20:27               ` Jacek Anaszewski
  2019-10-04 10:12                 ` Should regulator core support parsing OF based fwnode? Jean-Jacques Hiblot
  2019-10-04 11:39                 ` Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()) Mark Brown
  0 siblings, 2 replies; 29+ messages in thread
From: Jacek Anaszewski @ 2019-10-03 20:27 UTC (permalink / raw)
  To: Mark Brown
  Cc: Jean-Jacques Hiblot, Sebastian Reichel, pavel, robh+dt,
	mark.rutland, lee.jones, daniel.thompson, linux-kernel,
	dri-devel, tomi.valkeinen, dmurphy, linux-leds, Liam Girdwood

On 10/3/19 9:41 PM, Mark Brown wrote:
> On Thu, Oct 03, 2019 at 09:21:06PM +0200, Jacek Anaszewski wrote:
>> On 10/3/19 8:35 PM, Mark Brown wrote:
>>> On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote:
>>>> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote:
>>>>> On 03/10/2019 12:42, Sebastian Reichel wrote:
>>>>>> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:
> 
>>> This mail has nothing relevant in the subject line and pages of quotes
>>> before the question for me, it's kind of lucky I noticed it....
> 
>> Isn't it all about creating proper filters?
> 
> My point there is that there's nothing obvious in the mail that suggests
> it should get past filters - just being CCed on a mail isn't super
> reliable, people often get pulled in due to things like checkpatch or
> someone copying a CC list from an earlier patch series where there were
> things were relevant.

OK, updated the subject.

>>>> I wonder if it wouldn't make sense to add support for fwnode
>>>> parsing to regulator core. Or maybe it is either somehow supported
>>>> or not supported on purpose?
> 
>>> Anything attempting to use the regulator DT bindings in ACPI has very
>>> serious problems, ACPI has its own power model which isn't compatible
>>> with that used in DT.
> 
>> We have a means for checking if fwnode refers to of_node:
> 
>> is_of_node(const struct fwnode_handle *fwnode)
> 
>> Couldn't it be employed for OF case?
> 
> Why would we want to do that?  We'd continue to support only DT systems,
> just with code that's less obviously DT only and would need to put
> checks in.  I'm not seeing an upside here.

For instance few weeks ago we had a patch [0] in the LED core switching
from using struct device's of_node property to fwnode for conveying
device property data. And this transition to fwnode property API can be
observed as a frequent pattern across subsystems.

Recently there is an ongoing effort aiming to add generic support for
handling regulators in the LED core [1], but it turns out to require
bringing back initialization of of_node property for
devm_regulator_get_optional() to work properly.

Support for OF related fwnodes in regulator core could help reducing
this noise.

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/leds/led-class.c?id=fd81d7e946c6bdb86dbf0bd88fee3e1a545e7979
[1]
https://lore.kernel.org/linux-leds/20190923102059.17818-4-jjhiblot@ti.com/

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Should regulator core support parsing OF based fwnode?
  2019-10-03 20:27               ` Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()) Jacek Anaszewski
@ 2019-10-04 10:12                 ` Jean-Jacques Hiblot
  2019-10-04 11:39                 ` Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()) Mark Brown
  1 sibling, 0 replies; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-04 10:12 UTC (permalink / raw)
  To: Jacek Anaszewski, Mark Brown
  Cc: Sebastian Reichel, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, linux-kernel, dri-devel, tomi.valkeinen,
	dmurphy, linux-leds, Liam Girdwood


On 03/10/2019 22:27, Jacek Anaszewski wrote:
> On 10/3/19 9:41 PM, Mark Brown wrote:
>> On Thu, Oct 03, 2019 at 09:21:06PM +0200, Jacek Anaszewski wrote:
>>> On 10/3/19 8:35 PM, Mark Brown wrote:
>>>> On Thu, Oct 03, 2019 at 07:43:17PM +0200, Jacek Anaszewski wrote:
>>>>> On 10/3/19 2:47 PM, Jean-Jacques Hiblot wrote:
>>>>>> On 03/10/2019 12:42, Sebastian Reichel wrote:
>>>>>>> On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:
>>>> This mail has nothing relevant in the subject line and pages of quotes
>>>> before the question for me, it's kind of lucky I noticed it....
>>> Isn't it all about creating proper filters?
>> My point there is that there's nothing obvious in the mail that suggests
>> it should get past filters - just being CCed on a mail isn't super
>> reliable, people often get pulled in due to things like checkpatch or
>> someone copying a CC list from an earlier patch series where there were
>> things were relevant.
> OK, updated the subject.
>
>>>>> I wonder if it wouldn't make sense to add support for fwnode
>>>>> parsing to regulator core. Or maybe it is either somehow supported
>>>>> or not supported on purpose?
>>>> Anything attempting to use the regulator DT bindings in ACPI has very
>>>> serious problems, ACPI has its own power model which isn't compatible
>>>> with that used in DT.
>>> We have a means for checking if fwnode refers to of_node:
>>> is_of_node(const struct fwnode_handle *fwnode)
>>> Couldn't it be employed for OF case?
>> Why would we want to do that?  We'd continue to support only DT systems,
>> just with code that's less obviously DT only and would need to put
>> checks in.  I'm not seeing an upside here.
> For instance few weeks ago we had a patch [0] in the LED core switching
> from using struct device's of_node property to fwnode for conveying
> device property data. And this transition to fwnode property API can be
> observed as a frequent pattern across subsystems.
>
> Recently there is an ongoing effort aiming to add generic support for
> handling regulators in the LED core [1], but it turns out to require
> bringing back initialization of of_node property for
> devm_regulator_get_optional() to work properly.
>
> Support for OF related fwnodes in regulator core could help reducing
> this noise.

We could have this done in dev_of_node():

static inline struct device_node *dev_of_node(struct device *dev)
{
     if (!IS_ENABLED(CONFIG_OF) || !dev)
         return NULL;
     return dev->of_node ? dev->of_node : to_of_node(dev->fwnode);
}

Then it will only be a matter of using dev_of_node() instead of 
accessing directly dev->of_node


>
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/leds/led-class.c?id=fd81d7e946c6bdb86dbf0bd88fee3e1a545e7979
> [1]
> https://lore.kernel.org/linux-leds/20190923102059.17818-4-jjhiblot@ti.com/
>

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put())
  2019-10-03 20:27               ` Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()) Jacek Anaszewski
  2019-10-04 10:12                 ` Should regulator core support parsing OF based fwnode? Jean-Jacques Hiblot
@ 2019-10-04 11:39                 ` Mark Brown
  2019-10-04 13:33                   ` Should regulator core support parsing OF based fwnode? Jean-Jacques Hiblot
  1 sibling, 1 reply; 29+ messages in thread
From: Mark Brown @ 2019-10-04 11:39 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Jean-Jacques Hiblot, Sebastian Reichel, pavel, robh+dt,
	mark.rutland, lee.jones, daniel.thompson, linux-kernel,
	dri-devel, tomi.valkeinen, dmurphy, linux-leds, Liam Girdwood

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

On Thu, Oct 03, 2019 at 10:27:26PM +0200, Jacek Anaszewski wrote:
> On 10/3/19 9:41 PM, Mark Brown wrote:

> > Why would we want to do that?  We'd continue to support only DT systems,
> > just with code that's less obviously DT only and would need to put
> > checks in.  I'm not seeing an upside here.

> For instance few weeks ago we had a patch [0] in the LED core switching
> from using struct device's of_node property to fwnode for conveying
> device property data. And this transition to fwnode property API can be
> observed as a frequent pattern across subsystems.

For most subsystems the intent is to reuse DT bindings on embedded ACPI
systems via _DSD.

> Recently there is an ongoing effort aiming to add generic support for
> handling regulators in the LED core [1], but it turns out to require
> bringing back initialization of of_node property for
> devm_regulator_get_optional() to work properly.

Consumers should just be able to request a regulator without having to
worry about how that's being provided - they should have no knowledge at
all of firmware bindings or platform data for defining this.  If they
do that suggests there's an abstraction issue somewhere, what makes you
think that doing something with of_node is required?

Further, unless you have LEDs that work without power you probably
shouldn't be using _get_optional() for their supply.  That interface is
intended only for supplies that may be physically absent.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Should regulator core support parsing OF based fwnode?
  2019-10-04 11:39                 ` Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()) Mark Brown
@ 2019-10-04 13:33                   ` Jean-Jacques Hiblot
  2019-10-04 14:40                     ` Mark Brown
  0 siblings, 1 reply; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-04 13:33 UTC (permalink / raw)
  To: Mark Brown, Jacek Anaszewski
  Cc: Sebastian Reichel, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson, linux-kernel, dri-devel, tomi.valkeinen,
	dmurphy, linux-leds, Liam Girdwood


On 04/10/2019 13:39, Mark Brown wrote:
> On Thu, Oct 03, 2019 at 10:27:26PM +0200, Jacek Anaszewski wrote:
>> On 10/3/19 9:41 PM, Mark Brown wrote:
>>> Why would we want to do that?  We'd continue to support only DT systems,
>>> just with code that's less obviously DT only and would need to put
>>> checks in.  I'm not seeing an upside here.
>> For instance few weeks ago we had a patch [0] in the LED core switching
>> from using struct device's of_node property to fwnode for conveying
>> device property data. And this transition to fwnode property API can be
>> observed as a frequent pattern across subsystems.
> For most subsystems the intent is to reuse DT bindings on embedded ACPI
> systems via _DSD.
>
>> Recently there is an ongoing effort aiming to add generic support for
>> handling regulators in the LED core [1], but it turns out to require
>> bringing back initialization of of_node property for
>> devm_regulator_get_optional() to work properly.
> Consumers should just be able to request a regulator without having to
> worry about how that's being provided - they should have no knowledge at
> all of firmware bindings or platform data for defining this.  If they
> do that suggests there's an abstraction issue somewhere, what makes you
> think that doing something with of_node is required?

The regulator core accesses consumer->of_node to get a phandle to a 
regulator's node. The trouble arises from the fact that the LED core 
does not populate of_node anymore, instead it populates fwnode. This 
allows the LED core to be agnostic of ACPI or OF to get the properties 
of a LED.

IMO it is better to populate both of_node and fwnode in the LED core at 
the moment. It has already been fixed this way for the platform driver 
[0], MTD [1] and PCI-OF [2].

>
> Further, unless you have LEDs that work without power you probably
> shouldn't be using _get_optional() for their supply.  That interface is
> intended only for supplies that may be physically absent.

Not all LEDs have a regulator to provide the power. The power can be 
supplied by the LED controller for example.


[0] f94277af03ead0d3bf2 of/platform: Initialise dev->fwnode appropriately

[1] c176c6d7e932662668 mfd: core: Set fwnode for created devices

[2] 9b099a6c75e4ddceea PCI: OF: Initialize dev->fwnode appropriately


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Should regulator core support parsing OF based fwnode?
  2019-10-04 13:33                   ` Should regulator core support parsing OF based fwnode? Jean-Jacques Hiblot
@ 2019-10-04 14:40                     ` Mark Brown
  2019-10-04 15:13                       ` Jean-Jacques Hiblot
  0 siblings, 1 reply; 29+ messages in thread
From: Mark Brown @ 2019-10-04 14:40 UTC (permalink / raw)
  To: Jean-Jacques Hiblot
  Cc: Jacek Anaszewski, Sebastian Reichel, pavel, robh+dt,
	mark.rutland, lee.jones, daniel.thompson, linux-kernel,
	dri-devel, tomi.valkeinen, dmurphy, linux-leds, Liam Girdwood

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

On Fri, Oct 04, 2019 at 03:33:13PM +0200, Jean-Jacques Hiblot wrote:
> On 04/10/2019 13:39, Mark Brown wrote:

> > Consumers should just be able to request a regulator without having to
> > worry about how that's being provided - they should have no knowledge at
> > all of firmware bindings or platform data for defining this.  If they
> > do that suggests there's an abstraction issue somewhere, what makes you
> > think that doing something with of_node is required?

> The regulator core accesses consumer->of_node to get a phandle to a
> regulator's node. The trouble arises from the fact that the LED core does
> not populate of_node anymore, instead it populates fwnode. This allows the
> LED core to be agnostic of ACPI or OF to get the properties of a LED.

Why is the LED core populating anything?  Is the LED core copying bits
out of the struct device for the actual device into a synthetic device
rather than passing the actual device in?  That really doesn't seem like
a good idea, it's likely to lead to things like this where you don't
copy something that's required (or worse where something directly in the
struct device that can't be copied is needed).

> IMO it is better to populate both of_node and fwnode in the LED core at the
> moment. It has already been fixed this way for the platform driver [0], MTD
> [1] and PCI-OF [2].

Yeah, if you're going to be copying stuff out of the real device I'd
copy the of_node as well.

> > Further, unless you have LEDs that work without power you probably
> > shouldn't be using _get_optional() for their supply.  That interface is
> > intended only for supplies that may be physically absent.

> Not all LEDs have a regulator to provide the power. The power can be
> supplied by the LED controller for example.

This code probably shouldn't be being run at all for LEDs like that, I
was assuming this was just for GPIO LEDs and similar rather than all
LEDs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Should regulator core support parsing OF based fwnode?
  2019-10-04 14:40                     ` Mark Brown
@ 2019-10-04 15:13                       ` Jean-Jacques Hiblot
  2019-10-04 15:58                         ` Mark Brown
  0 siblings, 1 reply; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-04 15:13 UTC (permalink / raw)
  To: Mark Brown
  Cc: mark.rutland, daniel.thompson, Liam Girdwood, tomi.valkeinen,
	Sebastian Reichel, dri-devel, linux-kernel, robh+dt,
	Jacek Anaszewski, pavel, lee.jones, linux-leds, dmurphy


On 04/10/2019 16:40, Mark Brown wrote:
> On Fri, Oct 04, 2019 at 03:33:13PM +0200, Jean-Jacques Hiblot wrote:
>> On 04/10/2019 13:39, Mark Brown wrote:
>>> Consumers should just be able to request a regulator without having to
>>> worry about how that's being provided - they should have no knowledge at
>>> all of firmware bindings or platform data for defining this.  If they
>>> do that suggests there's an abstraction issue somewhere, what makes you
>>> think that doing something with of_node is required?
>> The regulator core accesses consumer->of_node to get a phandle to a
>> regulator's node. The trouble arises from the fact that the LED core does
>> not populate of_node anymore, instead it populates fwnode. This allows the
>> LED core to be agnostic of ACPI or OF to get the properties of a LED.
> Why is the LED core populating anything?  Is the LED core copying bits
> out of the struct device for the actual device into a synthetic device
> rather than passing the actual device in?  That really doesn't seem like
> a good idea, it's likely to lead to things like this where you don't
> copy something that's required (or worse where something directly in the
> struct device that can't be copied is needed).

This is not a copy of a device of parent's of_node or something like that.

You can think of a LED controller as a bus. It 'enumerates' its children 
LED, create the children devices (one per LED) and provides the 
functions to interact with them.

The device node we are talking about here is a per-LED thing, it is a 
child node of the node of the LED controller.

here is an example:

     tlc59108: tlc59116@40 { /* this is the node for the LED controller */
         status = "okay";
         #address-cells = <1>;
         #size-cells = <0>;
         compatible = "ti,tlc59108";
         reg = <0x40>;

         backlight_led: led@2 { /* this is the node of one LED attached 
to pin#2 of the LED controller */
             power-supply = <&bkl_fixed>;
             reg = <0x2>;
         };
         other_led: led@3 { /* this is the node another LED attached to 
pin #3 of the LED controller */
             power-supply = <&reg_3v3>;
             reg = <0x3>;
         };
     };


>
>> IMO it is better to populate both of_node and fwnode in the LED core at the
>> moment. It has already been fixed this way for the platform driver [0], MTD
>> [1] and PCI-OF [2].
> Yeah, if you're going to be copying stuff out of the real device I'd
> copy the of_node as well.
>
>>> Further, unless you have LEDs that work without power you probably
>>> shouldn't be using _get_optional() for their supply.  That interface is
>>> intended only for supplies that may be physically absent.
>> Not all LEDs have a regulator to provide the power. The power can be
>> supplied by the LED controller for example.
> This code probably shouldn't be being run at all for LEDs like that, I
> was assuming this was just for GPIO LEDs and similar rather than all
> LEDs.

>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Should regulator core support parsing OF based fwnode?
  2019-10-04 15:13                       ` Jean-Jacques Hiblot
@ 2019-10-04 15:58                         ` Mark Brown
  2019-10-04 16:12                           ` Jean-Jacques Hiblot
  0 siblings, 1 reply; 29+ messages in thread
From: Mark Brown @ 2019-10-04 15:58 UTC (permalink / raw)
  To: Jean-Jacques Hiblot
  Cc: mark.rutland, daniel.thompson, Liam Girdwood, tomi.valkeinen,
	Sebastian Reichel, dri-devel, linux-kernel, robh+dt,
	Jacek Anaszewski, pavel, lee.jones, linux-leds, dmurphy

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

On Fri, Oct 04, 2019 at 05:13:13PM +0200, Jean-Jacques Hiblot wrote:
> On 04/10/2019 16:40, Mark Brown wrote:

> > Why is the LED core populating anything?  Is the LED core copying bits
> > out of the struct device for the actual device into a synthetic device
> > rather than passing the actual device in?  That really doesn't seem like
> > a good idea, it's likely to lead to things like this where you don't
> > copy something that's required (or worse where something directly in the
> > struct device that can't be copied is needed).

> This is not a copy of a device of parent's of_node or something like that.

> You can think of a LED controller as a bus. It 'enumerates' its children
> LED, create the children devices (one per LED) and provides the functions to
> interact with them.

> The device node we are talking about here is a per-LED thing, it is a child
> node of the node of the LED controller.

> here is an example:
> 
>     tlc59108: tlc59116@40 { /* this is the node for the LED controller */
>         status = "okay";
>         #address-cells = <1>;
>         #size-cells = <0>;
>         compatible = "ti,tlc59108";
>         reg = <0x40>;
> 
>         backlight_led: led@2 { /* this is the node of one LED attached to
> pin#2 of the LED controller */
>             power-supply = <&bkl_fixed>;
>             reg = <0x2>;
>         };

Regulator supplies are supposed to be defined at the chip level rather
than subfunctions with names corresponding to the names on the chip.
This ensures that all chips look similar when you're mapping the
schematic into a DT, you shouldn't need to know about the binding for a
given chip to write a DT for it and if multiple people (perhaps working
on different OSs) write bindings for the same chip there should be a
good chance that they come up with the same mapping.  The supply_alias
interface is there to allow mapping these through to subfunctions if
needed, it looks like the LED framework should be using this.

That said if you are doing the above and the LEDs are appearing as
devices it's extremely surprising that their of_node might not be
initialized.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Should regulator core support parsing OF based fwnode?
  2019-10-04 15:58                         ` Mark Brown
@ 2019-10-04 16:12                           ` Jean-Jacques Hiblot
  2019-10-04 17:42                             ` Mark Brown
  0 siblings, 1 reply; 29+ messages in thread
From: Jean-Jacques Hiblot @ 2019-10-04 16:12 UTC (permalink / raw)
  To: Mark Brown
  Cc: mark.rutland, daniel.thompson, Liam Girdwood, tomi.valkeinen,
	Sebastian Reichel, dri-devel, linux-kernel, robh+dt,
	Jacek Anaszewski, pavel, lee.jones, linux-leds, dmurphy


On 04/10/2019 17:58, Mark Brown wrote:
> On Fri, Oct 04, 2019 at 05:13:13PM +0200, Jean-Jacques Hiblot wrote:
>> On 04/10/2019 16:40, Mark Brown wrote:
>>> Why is the LED core populating anything?  Is the LED core copying bits
>>> out of the struct device for the actual device into a synthetic device
>>> rather than passing the actual device in?  That really doesn't seem like
>>> a good idea, it's likely to lead to things like this where you don't
>>> copy something that's required (or worse where something directly in the
>>> struct device that can't be copied is needed).
>> This is not a copy of a device of parent's of_node or something like that.
>> You can think of a LED controller as a bus. It 'enumerates' its children
>> LED, create the children devices (one per LED) and provides the functions to
>> interact with them.
>> The device node we are talking about here is a per-LED thing, it is a child
>> node of the node of the LED controller.
>> here is an example:
>>
>>      tlc59108: tlc59116@40 { /* this is the node for the LED controller */
>>          status = "okay";
>>          #address-cells = <1>;
>>          #size-cells = <0>;
>>          compatible = "ti,tlc59108";
>>          reg = <0x40>;
>>
>>          backlight_led: led@2 { /* this is the node of one LED attached to
>> pin#2 of the LED controller */
>>              power-supply = <&bkl_fixed>;
>>              reg = <0x2>;
>>          };
> Regulator supplies are supposed to be defined at the chip level rather
> than subfunctions with names corresponding to the names on the chip.
> This ensures that all chips look similar when you're mapping the
> schematic into a DT, you shouldn't need to know about the binding for a
> given chip to write a DT for it and if multiple people (perhaps working
> on different OSs) write bindings for the same chip there should be a
> good chance that they come up with the same mapping.  The supply_alias
> interface is there to allow mapping these through to subfunctions if
> needed, it looks like the LED framework should be using this.

In case of current-sink LED drivers, each LED can be powered by a 
different regulator, because the driver is only a switch between the LED 
cathod and the ground.

>
> That said if you are doing the above and the LEDs are appearing as
> devices it's extremely surprising that their of_node might not be
> initialized.

That is because this is usually done by the platform core which is not 
involved here.

JJ


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Should regulator core support parsing OF based fwnode?
  2019-10-04 16:12                           ` Jean-Jacques Hiblot
@ 2019-10-04 17:42                             ` Mark Brown
  2019-10-04 20:55                               ` Jacek Anaszewski
  0 siblings, 1 reply; 29+ messages in thread
From: Mark Brown @ 2019-10-04 17:42 UTC (permalink / raw)
  To: Jean-Jacques Hiblot
  Cc: mark.rutland, daniel.thompson, Liam Girdwood, tomi.valkeinen,
	Sebastian Reichel, dri-devel, linux-kernel, robh+dt,
	Jacek Anaszewski, pavel, lee.jones, linux-leds, dmurphy

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

On Fri, Oct 04, 2019 at 06:12:52PM +0200, Jean-Jacques Hiblot wrote:
> On 04/10/2019 17:58, Mark Brown wrote:

> > Regulator supplies are supposed to be defined at the chip level rather
> > than subfunctions with names corresponding to the names on the chip.

...

> > good chance that they come up with the same mapping.  The supply_alias
> > interface is there to allow mapping these through to subfunctions if
> > needed, it looks like the LED framework should be using this.

> In case of current-sink LED drivers, each LED can be powered by a different
> regulator, because the driver is only a switch between the LED cathod and
> the ground.

Sure, it's common for devices to have supplies that are only needed by
one part of the chip which is why we have the supply_alias interface for
mapping things through.

> > That said if you are doing the above and the LEDs are appearing as
> > devices it's extremely surprising that their of_node might not be
> > initialized.

> That is because this is usually done by the platform core which is not
> involved here.

The surprise is more that it got instantiated from the DT without
keeping the node around than how it happened.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: Should regulator core support parsing OF based fwnode?
  2019-10-04 17:42                             ` Mark Brown
@ 2019-10-04 20:55                               ` Jacek Anaszewski
  0 siblings, 0 replies; 29+ messages in thread
From: Jacek Anaszewski @ 2019-10-04 20:55 UTC (permalink / raw)
  To: Mark Brown, Jean-Jacques Hiblot
  Cc: mark.rutland, daniel.thompson, Liam Girdwood, tomi.valkeinen,
	Sebastian Reichel, dri-devel, linux-kernel, robh+dt, pavel,
	lee.jones, linux-leds, dmurphy

On 10/4/19 7:42 PM, Mark Brown wrote:
> On Fri, Oct 04, 2019 at 06:12:52PM +0200, Jean-Jacques Hiblot wrote:
>> On 04/10/2019 17:58, Mark Brown wrote:
> 
>>> Regulator supplies are supposed to be defined at the chip level rather
>>> than subfunctions with names corresponding to the names on the chip.
> 
> ...
> 
>>> good chance that they come up with the same mapping.  The supply_alias
>>> interface is there to allow mapping these through to subfunctions if
>>> needed, it looks like the LED framework should be using this.
> 
>> In case of current-sink LED drivers, each LED can be powered by a different
>> regulator, because the driver is only a switch between the LED cathod and
>> the ground.
> 
> Sure, it's common for devices to have supplies that are only needed by
> one part of the chip which is why we have the supply_alias interface for
> mapping things through.
> 
>>> That said if you are doing the above and the LEDs are appearing as
>>> devices it's extremely surprising that their of_node might not be
>>> initialized.
> 
>> That is because this is usually done by the platform core which is not
>> involved here.
> 
> The surprise is more that it got instantiated from the DT without
> keeping the node around than how it happened.

This is LED class driver that is instantiated from DT and it in
turn registers LED class devices - one per corresponding DT child
node found in the parent LED controller node.

LED class device is created via device_create_with_groups() that
returns struct device with uninitialized of_node. This is the point
we're discussing. In order to be able to obtain regulator handle
in the LED core from DT we need to have exactly of_node
(of course provided it contains *-supply property).

Here the question may arise why it is not the LED core that instantiates
LED class devices per child nodes - well it is tricky to implement
due to disparate possible LED channel routings across devices.
We can think of adding it to the TODO list, but this is another story.

This is the background. However, since Jean pointed to the few
other similar cases when of_node is being initialized in addition
to fwnode I deem there is no issue and we can do the same in the
LED core.

No action in regulator core is required then.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 1/5] leds: populate the device's of_node when possible
  2019-10-03  8:28 ` [PATCH v8 1/5] leds: populate the device's of_node when possible Jean-Jacques Hiblot
@ 2019-10-04 21:08   ` Jacek Anaszewski
  0 siblings, 0 replies; 29+ messages in thread
From: Jacek Anaszewski @ 2019-10-04 21:08 UTC (permalink / raw)
  To: Jean-Jacques Hiblot, pavel, robh+dt, mark.rutland, lee.jones,
	daniel.thompson
  Cc: dmurphy, linux-leds, linux-kernel, dri-devel, tomi.valkeinen

Hi Jean,

On 10/3/19 10:28 AM, Jean-Jacques Hiblot wrote:
> If initialization data is available and its fwnode is actually a of_node,
> store this information in the led device's structure. This will allow the
> device to use or provide OF-based API such (devm_xxx).
> 
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
>  drivers/leds/led-class.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 647b1263c579..c2167b66b61f 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -276,8 +276,11 @@ int led_classdev_register_ext(struct device *parent,
>  		mutex_unlock(&led_cdev->led_access);
>  		return PTR_ERR(led_cdev->dev);
>  	}
> -	if (init_data && init_data->fwnode)
> +	if (init_data && init_data->fwnode) {
>  		led_cdev->dev->fwnode = init_data->fwnode;
> +		if (is_of_node(init_data->fwnode))

This check is made inside to_of_node() macro so here we
need only below assignment.

> +			led_cdev->dev->of_node = to_of_node(init_data->fwnode);
> +	}
>  
>  	if (ret)
>  		dev_warn(parent, "Led %s renamed to %s due to name collision",
> 

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH v8 0/5] Add a generic driver for LED-based backlight
  2019-10-03 11:40 ` [PATCH v8 0/5] Add a generic driver for LED-based backlight Lee Jones
@ 2019-10-08 19:55   ` Pavel Machek
  0 siblings, 0 replies; 29+ messages in thread
From: Pavel Machek @ 2019-10-08 19:55 UTC (permalink / raw)
  To: Lee Jones
  Cc: Jean-Jacques Hiblot, jacek.anaszewski, robh+dt, mark.rutland,
	daniel.thompson, dmurphy, linux-leds, linux-kernel, dri-devel,
	tomi.valkeinen

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

On Thu 2019-10-03 12:40:28, Lee Jones wrote:
> On Thu, 03 Oct 2019, Jean-Jacques Hiblot wrote:
> 
> > This series aims to add a led-backlight driver, similar to pwm-backlight,
> > but using a LED class device underneath.
> > 
> > A few years ago (2015), Tomi Valkeinen posted a series implementing a
> > backlight driver on top of a LED device:
> > https://patchwork.kernel.org/patch/7293991/
> > https://patchwork.kernel.org/patch/7294001/
> > https://patchwork.kernel.org/patch/7293981/
> > 
> > The discussion stopped because Tomi lacked the time to work on it.
> 
> [...]
> 
> >  .../bindings/leds/backlight/led-backlight.txt |  28 ++
> >  drivers/leds/led-class.c                      |  98 ++++++-
> >  drivers/video/backlight/Kconfig               |   7 +
> >  drivers/video/backlight/Makefile              |   1 +
> >  drivers/video/backlight/led_bl.c              | 260 ++++++++++++++++++
> >  include/linux/leds.h                          |   6 +
> >  6 files changed, 399 insertions(+), 1 deletion(-)
> >  create mode 100644 Documentation/devicetree/bindings/leds/backlight/led-backlight.txt
> >  create mode 100644 drivers/video/backlight/led_bl.c
> 
> How should this set be processed?  I'm happy to take it through
> Backlight via an immutable branch and PR to be consumed by LED.

That would make sense to me.

For the record, series is Tested-by: Pavel Machek <pavel@ucw.cz> .

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, back to index

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-03  8:28 [PATCH v8 0/5] Add a generic driver for LED-based backlight Jean-Jacques Hiblot
2019-10-03  8:28 ` [PATCH v8 1/5] leds: populate the device's of_node when possible Jean-Jacques Hiblot
2019-10-04 21:08   ` Jacek Anaszewski
2019-10-03  8:28 ` [PATCH v8 2/5] leds: Add of_led_get() and led_put() Jean-Jacques Hiblot
2019-10-03 10:42   ` Sebastian Reichel
2019-10-03 12:47     ` Jean-Jacques Hiblot
2019-10-03 17:43       ` Jacek Anaszewski
2019-10-03 18:35         ` Mark Brown
2019-10-03 19:21           ` Jacek Anaszewski
2019-10-03 19:41             ` Mark Brown
2019-10-03 20:27               ` Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()) Jacek Anaszewski
2019-10-04 10:12                 ` Should regulator core support parsing OF based fwnode? Jean-Jacques Hiblot
2019-10-04 11:39                 ` Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()) Mark Brown
2019-10-04 13:33                   ` Should regulator core support parsing OF based fwnode? Jean-Jacques Hiblot
2019-10-04 14:40                     ` Mark Brown
2019-10-04 15:13                       ` Jean-Jacques Hiblot
2019-10-04 15:58                         ` Mark Brown
2019-10-04 16:12                           ` Jean-Jacques Hiblot
2019-10-04 17:42                             ` Mark Brown
2019-10-04 20:55                               ` Jacek Anaszewski
2019-10-03  8:28 ` [PATCH v8 3/5] leds: Add managed API to get a LED from a device driver Jean-Jacques Hiblot
2019-10-03 10:47   ` Sebastian Reichel
2019-10-03  8:28 ` [PATCH v8 4/5] dt-bindings: backlight: Add led-backlight binding Jean-Jacques Hiblot
2019-10-03 11:17   ` Sebastian Reichel
2019-10-03  8:28 ` [PATCH v8 5/5] backlight: add led-backlight driver Jean-Jacques Hiblot
2019-10-03 11:47   ` Sebastian Reichel
2019-10-03 17:23     ` Jean-Jacques Hiblot
2019-10-03 11:40 ` [PATCH v8 0/5] Add a generic driver for LED-based backlight Lee Jones
2019-10-08 19:55   ` Pavel Machek

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org linux-leds@archiver.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/ public-inbox