LinuxLists.cc - [PATCH v8 0/5] Add a generic driver for LED-based backlight

[permalink] [raw]

Subject: [PATCH v8 2/5] leds: Add of_led_get() and led_put()

From: Tomi Valkeinen <[email protected]>

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 <[email protected]>
Signed-off-by: Jean-Jacques Hiblot <[email protected]>
Acked-by: Pavel Machek <[email protected]>
---
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

2019-10-03 08:32:04

[permalink] [raw]

Subject: [PATCH v8 4/5] dt-bindings: backlight: Add led-backlight binding

Add DT binding for led-backlight.

Signed-off-by: Jean-Jacques Hiblot <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
---
.../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

2019-10-03 08:32:44

[permalink] [raw]

Subject: [PATCH v8 5/5] backlight: add led-backlight driver

From: Tomi Valkeinen <[email protected]>

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 <[email protected]>
Signed-off-by: Jean-Jacques Hiblot <[email protected]>
Acked-by: Pavel Machek <[email protected]>
Reviewed-by: Daniel Thompson <[email protected]>
---
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 <[email protected]>
+ *
+ * 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

2019-10-03 08:33:40

[permalink] [raw]

Subject: [PATCH v8 3/5] leds: Add managed API to get a LED from a device driver

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 <[email protected]>
Acked-by: Pavel Machek <[email protected]>
---
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

2019-10-03 10:45:04

[permalink] [raw]

Subject: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()

Hi,

On Thu, Oct 03, 2019 at 10:28:09AM +0200, Jean-Jacques Hiblot wrote:
> From: Tomi Valkeinen <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> Acked-by: Pavel Machek <[email protected]>
> ---
> 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Attachments:

(No filename) (3.59 kB)
signature.asc (849.00 B)
Download all attachments

2019-10-03 10:48:12

[permalink] [raw]

Subject: Re: [PATCH v8 3/5] leds: Add managed API to get a LED from a device driver

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 <[email protected]>
> Acked-by: Pavel Machek <[email protected]>
> ---
> 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 <[email protected]>

-- 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Attachments:

(No filename) (3.02 kB)
signature.asc (849.00 B)
Download all attachments

2019-10-03 11:20:41

[permalink] [raw]

Subject: Re: [PATCH v8 4/5] dt-bindings: backlight: Add led-backlight binding

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 <[email protected]>
> Reviewed-by: Daniel Thompson <[email protected]>

Reviewed-by: Sebastian Reichel <[email protected]>

-- 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Attachments:

(No filename) (2.00 kB)
signature.asc (849.00 B)
Download all attachments

2019-10-03 11:41:52

by Lee Jones

[permalink] [raw]

Subject: Re: [PATCH v8 0/5] Add a generic driver for LED-based backlight

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

2019-10-03 11:49:07

[permalink] [raw]

Subject: Re: [PATCH v8 5/5] backlight: add led-backlight driver

Hi,

On Thu, Oct 03, 2019 at 10:28:12AM +0200, Jean-Jacques Hiblot wrote:
> From: Tomi Valkeinen <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
> Acked-by: Pavel Machek <[email protected]>
> Reviewed-by: Daniel Thompson <[email protected]>
> ---

Reviewed-by: Sebastian Reichel <[email protected]>

(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 <[email protected]>
> + *
> + * 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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Attachments:

(No filename) (9.82 kB)
signature.asc (849.00 B)
Download all attachments

2019-10-03 14:19:33

[permalink] [raw]

Subject: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()

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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
>> Acked-by: Pavel Machek <[email protected]>
>> ---
>> 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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

2019-10-03 17:57:34

[permalink] [raw]

Subject: Re: [PATCH v8 5/5] backlight: add led-backlight driver

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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
>> Acked-by: Pavel Machek <[email protected]>
>> Reviewed-by: Daniel Thompson <[email protected]>
>> ---
> Reviewed-by: Sebastian Reichel <[email protected]>
>
> (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
>> [email protected]
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

2019-10-03 18:55:47

[permalink] [raw]

Subject: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()

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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Jean-Jacques Hiblot <[email protected]>
>>> Acked-by: Pavel Machek <[email protected]>
>>> ---
>>> ? 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

2019-10-03 19:09:09

[permalink] [raw]

Subject: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()

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.

Attachments:

(No filename) (717.00 B)
signature.asc (499.00 B)
Download all attachments

2019-10-03 19:22:51

[permalink] [raw]

Subject: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()

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

2019-10-03 19:43:13

[permalink] [raw]

Subject: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put()

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.

Attachments:

(No filename) (1.53 kB)
signature.asc (499.00 B)
Download all attachments

2019-10-03 20:29:53

[permalink] [raw]

Subject: Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put())

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/[email protected]/

--
Best regards,
Jacek Anaszewski

2019-10-04 10:37:21

[permalink] [raw]

Subject: Re: Should regulator core support parsing OF based fwnode?

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/[email protected]/
>

2019-10-04 12:03:09

[permalink] [raw]

Subject: Re: Should regulator core support parsing OF based fwnode? (was: Re: [PATCH v8 2/5] leds: Add of_led_get() and led_put())

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.

Attachments:

(No filename) (1.45 kB)
signature.asc (499.00 B)
Download all attachments

2019-10-04 13:35:27

[permalink] [raw]

Subject: Re: Should regulator core support parsing OF based fwnode?

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

2019-10-04 14:41:43

[permalink] [raw]

Subject: Re: Should regulator core support parsing OF based fwnode?

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.

Attachments:

(No filename) (1.91 kB)
signature.asc (499.00 B)
Download all attachments

2019-10-04 15:17:01

[permalink] [raw]

Subject: Re: Should regulator core support parsing OF based fwnode?

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
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

2019-10-04 16:00:22

[permalink] [raw]

Subject: Re: Should regulator core support parsing OF based fwnode?

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.

Attachments:

(No filename) (2.13 kB)
signature.asc (499.00 B)
Download all attachments

2019-10-04 16:15:56

[permalink] [raw]

Subject: Re: Should regulator core support parsing OF based fwnode?

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

2019-10-04 17:45:39

[permalink] [raw]

Subject: Re: Should regulator core support parsing OF based fwnode?

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.

Attachments:

(No filename) (1.18 kB)
signature.asc (499.00 B)
Download all attachments

2019-10-04 20:56:35