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
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",
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
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
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
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.
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.
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?
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.
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/dr... [1] https://lore.kernel.org/linux-leds/20190923102059.17818-4-jjhiblot@ti.com/
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/dr... [1] https://lore.kernel.org/linux-leds/20190923102059.17818-4-jjhiblot@ti.com/
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.
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
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.
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 = <®_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
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.
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
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.
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
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
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>; + };
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 levelinto a LED brightness level. If it is not provided, theidentity 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
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");
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 Yto 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
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:
call led_sysfs_disable
use devm_add_action_or_reset() to register the led_sysfs_enable loop
use devm_backlight_device_register() to register BL
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
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.
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
dri-devel@lists.freedesktop.org
-
Jacek Anaszewski -
Jean-Jacques Hiblot -
Lee Jones -
Mark Brown -
Pavel Machek -
Sebastian Reichel -
Sebastian Reichel