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

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

[Tomi Valkeinen]

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

LED framework has no support for DT or getting a LED class driver from another
kernel driver, so I added minimal functionality to led-class.c to get
led-backlight working.

 Tomi

Tomi Valkeinen (3):
  leds: Add of_led_get() and led_put()
  backlight: add led-backlight driver
  devicetree: Add led-backlight binding

 .../bindings/video/backlight/led-backlight.txt     |  30 +++
 drivers/leds/led-class.c                           |  75 +++++++
 drivers/video/backlight/Kconfig                    |   7 +
 drivers/video/backlight/Makefile                   |   1 +
 drivers/video/backlight/led_bl.c                   | 226 +++++++++++++++++++++
 include/linux/leds.h                               |   4 +
 6 files changed, 343 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
 create mode 100644 drivers/video/backlight/led_bl.c

-- 
2.1.4

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

[Tomi Valkeinen]

This patch adds basic support for a kernel driver to get a LED device.
This will be used by the led-backlight driver.

Only OF version is implemented for now, and the behavior is similar to
PWM's of_pwm_get() and pwm_put().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/leds/led-class.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/leds.h     |  4 +++
 2 files changed, 79 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index beabfbc6f7cd..0cb4fd7d71d6 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -20,6 +20,7 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
+#include <linux/of.h>
 #include "leds.h"
 
 static struct class *leds_class;
@@ -216,6 +217,80 @@ static int led_resume(struct device *dev)
 
 static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
 
+/* find OF node for the given led_cdev */
+static struct device_node *find_led_of_node(struct led_classdev *led_cdev)
+{
+	struct device *led_dev = led_cdev->dev;
+	struct device_node *child;
+
+	for_each_child_of_node(led_dev->parent->of_node, child) {
+		if (of_property_match_string(child, "label", led_cdev->name) = 0)
+			return child;
+	}
+
+	return NULL;
+}
+
+static int led_match_led_node(struct device *led_dev, const void *data)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(led_dev);
+	const struct device_node *target_node = data;
+	struct device_node *led_node;
+
+	led_node = find_led_of_node(led_cdev);
+	if (!led_node)
+		return 0;
+
+	of_node_put(led_node);
+
+	return led_node = target_node ? 1 : 0;
+}
+
+/**
+ * of_led_get() - request a LED device via the LED framework
+ * @np: device node to get the LED device from
+ *
+ * Returns the LED device parsed from the phandle specified in the "leds"
+ * property of a device tree node or a negative error-code on failure.
+ */
+struct led_classdev *of_led_get(struct device_node *np)
+{
+	struct device *led_dev;
+	struct led_classdev *led_cdev;
+	struct device_node *led_node;
+
+	led_node = of_parse_phandle(np, "leds", 0);
+	if (!led_node)
+		return ERR_PTR(-ENODEV);
+
+	led_dev = class_find_device(leds_class, NULL, led_node,
+		led_match_led_node);
+	if (!led_dev) {
+		of_node_put(led_node);
+		return ERR_PTR(-EPROBE_DEFER);
+	}
+
+	of_node_put(led_node);
+
+	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 match_name(struct device *dev, const void *data)
 {
 	if (!dev_name(dev))
diff --git a/include/linux/leds.h b/include/linux/leds.h
index b122eeafb5dc..efc9b28af564 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -21,6 +21,7 @@
 #include <linux/workqueue.h>
 
 struct device;
+struct device_node;
 /*
  * LED Core
  */
@@ -113,6 +114,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);
+extern void led_put(struct led_classdev *led_cdev);
+
 /**
  * led_blink_set - set blinking with software fallback
  * @led_cdev: the LED to start blinking
-- 
2.1.4

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

[Andrew Lunn]

On Tue, Aug 25, 2015 at 02:34:00PM +0300, Tomi Valkeinen wrote:
> This patch adds basic support for a kernel driver to get a LED device.
> This will be used by the led-backlight driver.
> 
> Only OF version is implemented for now, and the behavior is similar to
> PWM's of_pwm_get() and pwm_put().

Hi Tomi

Is this the correct way to do it?  I would of expected an xlate
function.

I'm also wondering if this is the right place, in the class. Don't you
just need the core?

     Andrew

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

[Tomi Valkeinen]

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



On 25/08/15 15:18, Andrew Lunn wrote:
> On Tue, Aug 25, 2015 at 02:34:00PM +0300, Tomi Valkeinen wrote:
>> This patch adds basic support for a kernel driver to get a LED device.
>> This will be used by the led-backlight driver.
>>
>> Only OF version is implemented for now, and the behavior is similar to
>> PWM's of_pwm_get() and pwm_put().
> 
> Hi Tomi
> 
> Is this the correct way to do it?  I would of expected an xlate
> function.

I don't know. To be honest I haven't worked much with this kind of code,
and I didn't want to start writing full blown "of" support for leds. My
solution cuts the corners a bit... Probably this is more of an RFC than
a ready patch series.

I need to look what the of_xlate functions are doing in other frameworks.

> I'm also wondering if this is the right place, in the class. Don't you
> just need the core?

I'm using the "static struct class *leds_class;" to find the actual
led_classdev, and that variable is local to led-class.c.

 Tomi


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

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

[Tomi Valkeinen]

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

Hi Andrew,

On 25/08/15 15:18, Andrew Lunn wrote:
> On Tue, Aug 25, 2015 at 02:34:00PM +0300, Tomi Valkeinen wrote:
>> This patch adds basic support for a kernel driver to get a LED device.
>> This will be used by the led-backlight driver.
>>
>> Only OF version is implemented for now, and the behavior is similar to
>> PWM's of_pwm_get() and pwm_put().
> 
> Hi Tomi
> 
> Is this the correct way to do it?  I would of expected an xlate
> function.

I just sent v2, but I don't use xlate there.

If I understand the purpose of xlate (in pwm, for example) correctly,
xlate is a function in the pwm chip to allow custom bindings for the pwm
outputs from that pwm chip.

The problem with LEDs is that there's no "LED chip". Each LED is
modelled as individual device, without a well defined parent. Thus
there's no place to add such an xlate function.

Do you have any thoughts on what the xlate for LEDs should look like?

 Tomi


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

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

[Jacek Anaszewski]

Hi Tomi,

Thanks for the patch. Generally, I'd prefer to add files
drivers/leds/of.c and include/linux/of_leds.h and put related functions
there. Those functions' names should begin with "of_". Please provide
also no-op versions of the functions to address configurations when
CONFIG_OF isn't enabled. I have also few comments below.

On 08/25/2015 01:34 PM, Tomi Valkeinen wrote:
> This patch adds basic support for a kernel driver to get a LED device.
> This will be used by the led-backlight driver.
>
> Only OF version is implemented for now, and the behavior is similar to
> PWM's of_pwm_get() and pwm_put().
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/leds/led-class.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/leds.h     |  4 +++
>   2 files changed, 79 insertions(+)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index beabfbc6f7cd..0cb4fd7d71d6 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -20,6 +20,7 @@
>   #include <linux/slab.h>
>   #include <linux/spinlock.h>
>   #include <linux/timer.h>
> +#include <linux/of.h>
>   #include "leds.h"
>
>   static struct class *leds_class;
> @@ -216,6 +217,80 @@ static int led_resume(struct device *dev)
>
>   static SIMPLE_DEV_PM_OPS(leds_class_dev_pm_ops, led_suspend, led_resume);
>
> +/* find OF node for the given led_cdev */
> +static struct device_node *find_led_of_node(struct led_classdev *led_cdev)
> +{
> +	struct device *led_dev = led_cdev->dev;
> +	struct device_node *child;
> +
> +	for_each_child_of_node(led_dev->parent->of_node, child) {
> +		if (of_property_match_string(child, "label", led_cdev->name) = 0)
> +			return child;
> +	}
> +
> +	return NULL;
> +}
> +
> +static int led_match_led_node(struct device *led_dev, const void *data)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(led_dev);
> +	const struct device_node *target_node = data;
> +	struct device_node *led_node;
> +
> +	led_node = find_led_of_node(led_cdev);
> +	if (!led_node)
> +		return 0;
> +
> +	of_node_put(led_node);
> +
> +	return led_node = target_node ? 1 : 0;
> +}
> +
> +/**
> + * of_led_get() - request a LED device via the LED framework
> + * @np: device node to get the LED device from
> + *
> + * Returns the LED device parsed from the phandle specified in the "leds"
> + * property of a device tree node or a negative error-code on failure.
> + */
> +struct led_classdev *of_led_get(struct device_node *np)
> +{
> +	struct device *led_dev;
> +	struct led_classdev *led_cdev;
> +	struct device_node *led_node;
> +
> +	led_node = of_parse_phandle(np, "leds", 0);
> +	if (!led_node)
> +		return ERR_PTR(-ENODEV);
> +
> +	led_dev = class_find_device(leds_class, NULL, led_node,
> +		led_match_led_node);

Single of_node_put(led_node) here will do.

> +	if (!led_dev) {
> +		of_node_put(led_node);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	of_node_put(led_node);

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

Please move it to include/linux/leds.h, make static inline and provide
also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled.

>   static int match_name(struct device *dev, const void *data)
>   {
>   	if (!dev_name(dev))
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index b122eeafb5dc..efc9b28af564 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -21,6 +21,7 @@
>   #include <linux/workqueue.h>
>
>   struct device;
> +struct device_node;
>   /*
>    * LED Core
>    */
> @@ -113,6 +114,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);

We need also no-op version for (!CONFIG_OF || !CONFIG_LEDS_CLASS).

> +extern void led_put(struct led_classdev *led_cdev);
> +
>   /**
>    * led_blink_set - set blinking with software fallback
>    * @led_cdev: the LED to start blinking
>


-- 
Best Regards,
Jacek Anaszewski

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

[Tomi Valkeinen]

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

Hi,

On 25/08/15 16:25, Jacek Anaszewski wrote:
> Hi Tomi,
> 
> Thanks for the patch. Generally, I'd prefer to add files
> drivers/leds/of.c and include/linux/of_leds.h and put related functions
> there. Those functions' names should begin with "of_". Please provide

Ok, I'll do that. I do need to export something from led-class in that
case, so that the of.c gets hold of 'leds_class' pointer, either
directly or indirectly.

> also no-op versions of the functions to address configurations when
> CONFIG_OF isn't enabled. I have also few comments below.

Yep. No-ops for the purpose of making the kernel image smaller? I do
think the current code compiles and works fine with CONFIG_OF disabled
(although I have to say I don't remember if I actually tested it).

>> +struct led_classdev *of_led_get(struct device_node *np)
>> +{
>> +    struct device *led_dev;
>> +    struct led_classdev *led_cdev;
>> +    struct device_node *led_node;
>> +
>> +    led_node = of_parse_phandle(np, "leds", 0);
>> +    if (!led_node)
>> +        return ERR_PTR(-ENODEV);
>> +
>> +    led_dev = class_find_device(leds_class, NULL, led_node,
>> +        led_match_led_node);
> 
> Single of_node_put(led_node) here will do.

Right.

>> +    if (!led_dev) {
>> +        of_node_put(led_node);
>> +        return ERR_PTR(-EPROBE_DEFER);
>> +    }
>> +
>> +    of_node_put(led_node);
> 
>> +    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);
> 
> Please move it to include/linux/leds.h, make static inline and provide
> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled.

Ok. Why do you want it as static inline in the leds.h? I usually like to
keep the matching functions (get and put here) in the same place.

 Tomi


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

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

[Jacek Anaszewski]

On 09/07/2015 02:35 PM, Tomi Valkeinen wrote:
> Hi,
>
> On 25/08/15 16:25, Jacek Anaszewski wrote:
>> Hi Tomi,
>>
>> Thanks for the patch. Generally, I'd prefer to add files
>> drivers/leds/of.c and include/linux/of_leds.h and put related functions
>> there. Those functions' names should begin with "of_". Please provide
>
> Ok, I'll do that. I do need to export something from led-class in that
> case, so that the of.c gets hold of 'leds_class' pointer, either
> directly or indirectly.

What exactly do you need to export?

>
>> also no-op versions of the functions to address configurations when
>> CONFIG_OF isn't enabled. I have also few comments below.
>
> Yep. No-ops for the purpose of making the kernel image smaller?

No, for the case when support for LED subsystem is disabled in the
kernel config. I'd rather backlight driver depended on LED subsystem
than selected it.

> I do
> think the current code compiles and works fine with CONFIG_OF disabled
> (although I have to say I don't remember if I actually tested it).
>
>>> +struct led_classdev *of_led_get(struct device_node *np)
>>> +{
>>> +    struct device *led_dev;
>>> +    struct led_classdev *led_cdev;
>>> +    struct device_node *led_node;
>>> +
>>> +    led_node = of_parse_phandle(np, "leds", 0);
>>> +    if (!led_node)
>>> +        return ERR_PTR(-ENODEV);
>>> +
>>> +    led_dev = class_find_device(leds_class, NULL, led_node,
>>> +        led_match_led_node);
>>
>> Single of_node_put(led_node) here will do.
>
> Right.
>
>>> +    if (!led_dev) {
>>> +        of_node_put(led_node);
>>> +        return ERR_PTR(-EPROBE_DEFER);
>>> +    }
>>> +
>>> +    of_node_put(led_node);
>>
>>> +    led_cdev = dev_get_drvdata(led_dev);
>>> +pinctrl/pinctrl.h"
>>> +    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);
>>
>> Please move it to include/linux/leds.h, make static inline and provide
>> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled.
>
> Ok. Why do you want it as static inline in the leds.h?

This function contains only one instruction, why should we waste
a function call for it?

> I usually like to
> keep the matching functions (get and put here) in the same place.

-- 
Best Regards,
Jacek Anaszewski

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

[Tomi Valkeinen]

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


On 07/09/15 17:11, Jacek Anaszewski wrote:

>>> Thanks for the patch. Generally, I'd prefer to add files
>>> drivers/leds/of.c and include/linux/of_leds.h and put related functions
>>> there. Those functions' names should begin with "of_". Please provide
>>
>> Ok, I'll do that. I do need to export something from led-class in that
>> case, so that the of.c gets hold of 'leds_class' pointer, either
>> directly or indirectly.
> 
> What exactly do you need to export?

The "static struct class *leds_class" from led-class.c, in one way or
another. of_led_get() needs to go through the led devices from the class.

For now I just removed the "static" from it, so that I can use it from of.c.

>>> also no-op versions of the functions to address configurations when
>>> CONFIG_OF isn't enabled. I have also few comments below.
>>
>> Yep. No-ops for the purpose of making the kernel image smaller?
> 
> No, for the case when support for LED subsystem is disabled in the
> kernel config. I'd rather backlight driver depended on LED subsystem
> than selected it.

Sorry, I didn't get that one. How does the backlight driver's
depend/select affect this?

>>> Please move it to include/linux/leds.h, make static inline and provide
>>> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled.
>>
>> Ok. Why do you want it as static inline in the leds.h?
> 
> This function contains only one instruction, why should we waste
> a function call for it?

I like to keep code in .c files and leave .h files for declarations, and
only in special cases have inline code in .h files.

What do you mean with "waste"?

In this case there's no need to get more performance by inlining (which
anyway may not help and needs to be studied separately for every case),
with inlining the resulting kernel image is larger, and inlining forces
the users of led_put to include module.h to compile.

 Tomi


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

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

[Jacek Anaszewski]

On 09/08/2015 09:10 AM, Tomi Valkeinen wrote:
>
> On 07/09/15 17:11, Jacek Anaszewski wrote:
>
>>>> Thanks for the patch. Generally, I'd prefer to add files
>>>> drivers/leds/of.c and include/linux/of_leds.h and put related functions
>>>> there. Those functions' names should begin with "of_". Please provide
>>>
>>> Ok, I'll do that. I do need to export something from led-class in that
>>> case, so that the of.c gets hold of 'leds_class' pointer, either
>>> directly or indirectly.
>>
>> What exactly do you need to export?
>
> The "static struct class *leds_class" from led-class.c, in one way or
> another. of_led_get() needs to go through the led devices from the class.
>
> For now I just removed the "static" from it, so that I can use it from of.c.

I think we can go in this direction. I've skimmed through existing
class drivers and found similar examples (e.g. tty_class, rtc_class).

>>>> also no-op versions of the functions to address configurations when
>>>> CONFIG_OF isn't enabled. I have also few comments below.
>>>
>>> Yep. No-ops for the purpose of making the kernel image smaller?
>>
>> No, for the case when support for LED subsystem is disabled in the
>> kernel config. I'd rather backlight driver depended on LED subsystem
>> than selected it.
>
> Sorry, I didn't get that one. How does the backlight driver's
> depend/select affect this?

OK, I confused something here. Backlight driver should depend on
LEDS_CLASS by defining "depends on LEDS_CLASS" in backlight Kconfig.
It should also depend on OF. In case of this driver the no-ops would be
only for the purpose of making the kernel image smaller, as the driver
probe will fail without them anyway. Nevertheless, there might be added
other drivers in the future, using of_led_get{put} API which would like
to make some decisions basing on whether LEDS_CLASS or/and OF are
turned on. No-ops would be of use then.


>>>> Please move it to include/linux/leds.h, make static inline and provide
>>>> also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled.
>>>
>>> Ok. Why do you want it as static inline in the leds.h?
>>
>> This function contains only one instruction, why should we waste
>> a function call for it?
>
> I like to keep code in .c files and leave .h files for declarations, and
> only in special cases have inline code in .h files.
>
> What do you mean with "waste"?

I used 'waste' because we would be wasting time here for the call which
can be avoided at no cost. Of course if compiler will decide to inline
it.

> In this case there's no need to get more performance by inlining

Why so?

> (which
> anyway may not help and needs to be studied separately for every case),

There is the tradeoff. Readability vs performance.

> with inlining the resulting kernel image is larger,

That's why we inline only small functions.

> and inlining forces the users of led_put to include module.h to compile.

We could include module.h from leds.h.

-- 
Best Regards,
Jacek Anaszewski

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

[Tomi Valkeinen]

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


On 08/09/15 12:21, Jacek Anaszewski wrote:

>> The "static struct class *leds_class" from led-class.c, in one way or
>> another. of_led_get() needs to go through the led devices from the class.
>>
>> For now I just removed the "static" from it, so that I can use it from
>> of.c.
> 
> I think we can go in this direction. I've skimmed through existing
> class drivers and found similar examples (e.g. tty_class, rtc_class).

Yep.

>> Sorry, I didn't get that one. How does the backlight driver's
>> depend/select affect this?
> 
> OK, I confused something here. Backlight driver should depend on
> LEDS_CLASS by defining "depends on LEDS_CLASS" in backlight Kconfig.
> It should also depend on OF. In case of this driver the no-ops would be
> only for the purpose of making the kernel image smaller, as the driver
> probe will fail without them anyway. Nevertheless, there might be added
> other drivers in the future, using of_led_get{put} API which would like
> to make some decisions basing on whether LEDS_CLASS or/and OF are
> turned on. No-ops would be of use then.

I agree.

>> What do you mean with "waste"?
> 
> I used 'waste' because we would be wasting time here for the call which
> can be avoided at no cost. Of course if compiler will decide to inline
> it.
> 
>> In this case there's no need to get more performance by inlining
> 
> Why so?

Reserving and freeing resources are rarely hot paths. The functions in
question are usually called in a driver's probe and remove. Saving a few
CPU cycles there doesn't really matter, so I think readability is the
important part here.

>> and inlining forces the users of led_put to include module.h to compile.
> 
> We could include module.h from leds.h.

Yes we can. And I can do that in my patch. I don't agree with the
solution but neither do I really have a problem with it =).

 Tomi


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

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

[Jacek Anaszewski]

On 09/08/2015 12:41 PM, Tomi Valkeinen wrote:
>
> On 08/09/15 12:21, Jacek Anaszewski wrote:
>
>>> The "static struct class *leds_class" from led-class.c, in one way or
>>> another. of_led_get() needs to go through the led devices from the class.
>>>
>>> For now I just removed the "static" from it, so that I can use it from
>>> of.c.
>>
>> I think we can go in this direction. I've skimmed through existing
>> class drivers and found similar examples (e.g. tty_class, rtc_class).
>
> Yep.
>
>>> Sorry, I didn't get that one. How does the backlight driver's
>>> depend/select affect this?
>>
>> OK, I confused something here. Backlight driver should depend on
>> LEDS_CLASS by defining "depends on LEDS_CLASS" in backlight Kconfig.
>> It should also depend on OF. In case of this driver the no-ops would be
>> only for the purpose of making the kernel image smaller, as the driver
>> probe will fail without them anyway. Nevertheless, there might be added
>> other drivers in the future, using of_led_get{put} API which would like
>> to make some decisions basing on whether LEDS_CLASS or/and OF are
>> turned on. No-ops would be of use then.
>
> I agree.
>
>>> What do you mean with "waste"?
>>
>> I used 'waste' because we would be wasting time here for the call which
>> can be avoided at no cost. Of course if compiler will decide to inline
>> it.
>>
>>> In this case there's no need to get more performance by inlining
>>
>> Why so?
>
> Reserving and freeing resources are rarely hot paths. The functions in
> question are usually called in a driver's probe and remove. Saving a few
> CPU cycles there doesn't really matter, so I think readability is the
> important part here.

OK, I agree.

>>> and inlining forces the users of led_put to include module.h to compile.
>>
>> We could include module.h from leds.h.
>
> Yes we can. And I can do that in my patch. I don't agree with the
> solution but neither do I really have a problem with it =).

Please go ahead as you originally planned, i.e. without inlining.

-- 
Best Regards,
Jacek Anaszewski

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

[Tomi Valkeinen]

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

Hi,

On 25/08/15 16:25, Jacek Anaszewski wrote:
> Hi Tomi,
> 
> Thanks for the patch. Generally, I'd prefer to add files
> drivers/leds/of.c and include/linux/of_leds.h and put related functions
> there. Those functions' names should begin with "of_". Please provide

So I presume leds/of.c should be linked together with led-class.c.
Afaics, that means I need to either rename the resulting led-class.ko or
led-class.c, so that I can do it in the Makefile. Any preferences?

Alternatively I could create a led-of.ko, but that doesn't feel right.

 Tomi


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

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

[Jacek Anaszewski]

On 09/07/2015 03:18 PM, Tomi Valkeinen wrote:
> Hi,
>
> On 25/08/15 16:25, Jacek Anaszewski wrote:
>> Hi Tomi,
>>
>> Thanks for the patch. Generally, I'd prefer to add files
>> drivers/leds/of.c and include/linux/of_leds.h and put related functions
>> there. Those functions' names should begin with "of_". Please provide
>
> So I presume leds/of.c should be linked together with led-class.c.
> Afaics, that means I need to either rename the resulting led-class.ko or
> led-class.c, so that I can do it in the Makefile. Any preferences?

Let's rename led-class.ko to led-class-objs.ko.

> Alternatively I could create a led-of.ko, but that doesn't feel right.


-- 
Best Regards,
Jacek Anaszewski

[PATCH 2/3] backlight: add led-backlight driver

[Tomi Valkeinen]

This patch adds a led-backlight driver (led_bl), which is similar to
pwm_bl except the driver uses a LED class driver to adjust the
brightness in the HW.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/backlight/Kconfig  |   7 ++
 drivers/video/backlight/Makefile |   1 +
 drivers/video/backlight/led_bl.c | 226 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 234 insertions(+)
 create mode 100644 drivers/video/backlight/led_bl.c

diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
index 0505b796d743..d1336196aba2 100644
--- a/drivers/video/backlight/Kconfig
+++ b/drivers/video/backlight/Kconfig
@@ -453,6 +453,13 @@ config BACKLIGHT_BD6107
 	help
 	  If you have a Rohm BD6107 say Y to enable the backlight driver.
 
+config BACKLIGHT_LED
+	tristate "Generic LED based Backlight Driver"
+	depends on LEDS_CLASS && OF
+	help
+	  If you have a LCD backlight adjustable by LED class driver, say Y
+	  to enable this driver.
+
 endif # BACKLIGHT_CLASS_DEVICE
 
 endif # BACKLIGHT_LCD_SUPPORT
diff --git a/drivers/video/backlight/Makefile b/drivers/video/backlight/Makefile
index d67073f9d421..ecd321daee21 100644
--- a/drivers/video/backlight/Makefile
+++ b/drivers/video/backlight/Makefile
@@ -54,3 +54,4 @@ obj-$(CONFIG_BACKLIGHT_SKY81452)	+= sky81452-backlight.o
 obj-$(CONFIG_BACKLIGHT_TOSA)		+= tosa_bl.o
 obj-$(CONFIG_BACKLIGHT_TPS65217)	+= tps65217_bl.o
 obj-$(CONFIG_BACKLIGHT_WM831X)		+= wm831x_bl.o
+obj-$(CONFIG_BACKLIGHT_LED)		+= led_bl.o
diff --git a/drivers/video/backlight/led_bl.c b/drivers/video/backlight/led_bl.c
new file mode 100644
index 000000000000..82cffa39a5b2
--- /dev/null
+++ b/drivers/video/backlight/led_bl.c
@@ -0,0 +1,226 @@
+/*
+ * Copyright 2015 Texas Instruments
+ *
+ * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
+ *
+ * Based on pwm_bl.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ */
+#include <linux/backlight.h>
+#include <linux/gpio/consumer.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+
+struct led_bl_data {
+	struct device		*dev;
+	struct backlight_device	*bl_dev;
+
+	unsigned int		*levels;
+	bool			enabled;
+	struct regulator	*power_supply;
+	struct gpio_desc	*enable_gpio;
+
+	struct led_classdev *led_cdev;
+
+	unsigned int max_brightness;
+	unsigned int default_brightness;
+};
+
+static void led_bl_set_brightness(struct led_bl_data *priv, int brightness)
+{
+	int err;
+
+	if (!priv->enabled) {
+		err = regulator_enable(priv->power_supply);
+		if (err < 0)
+			dev_err(priv->dev, "failed to enable power supply\n");
+
+		if (priv->enable_gpio)
+			gpiod_set_value_cansleep(priv->enable_gpio, 1);
+	}
+
+	led_set_brightness(priv->led_cdev, priv->levels[brightness]);
+
+	priv->enabled = true;
+}
+
+static void led_bl_power_off(struct led_bl_data *priv)
+{
+	if (!priv->enabled)
+		return;
+
+	led_set_brightness(priv->led_cdev, LED_OFF);
+
+	if (priv->enable_gpio)
+		gpiod_set_value_cansleep(priv->enable_gpio, 0);
+
+	regulator_disable(priv->power_supply);
+
+	priv->enabled = false;
+}
+
+static int led_bl_update_status(struct backlight_device *bl)
+{
+	struct led_bl_data *priv = bl_get_data(bl);
+	int brightness = bl->props.brightness;
+
+	if (bl->props.power != FB_BLANK_UNBLANK ||
+	    bl->props.fb_blank != FB_BLANK_UNBLANK ||
+	    bl->props.state & BL_CORE_FBBLANK)
+		brightness = 0;
+
+	if (brightness > 0)
+		led_bl_set_brightness(priv, brightness);
+	else
+		led_bl_power_off(priv);
+
+	return 0;
+}
+
+static const struct backlight_ops led_bl_ops = {
+	.update_status	= led_bl_update_status,
+};
+
+static int led_bl_parse_dt(struct device *dev,
+			   struct led_bl_data *priv)
+{
+	struct device_node *node = dev->of_node;
+	int num_levels;
+	u32 *levels;
+	u32 value;
+	int ret;
+
+	if (!node)
+		return -ENODEV;
+
+	num_levels = of_property_count_u32_elems(node, "brightness-levels");
+	if (num_levels < 0)
+		return num_levels;
+
+	levels = devm_kzalloc(dev, sizeof(u32) * num_levels, GFP_KERNEL);
+	if (!levels)
+		return -ENOMEM;
+
+	ret = of_property_read_u32_array(node, "brightness-levels",
+					 levels,
+					 num_levels);
+	if (ret < 0)
+		return ret;
+
+	ret = of_property_read_u32(node, "default-brightness-level", &value);
+	if (ret < 0)
+		return ret;
+
+	if (value >= num_levels) {
+		dev_err(dev, "invalid default-brightness-level\n");
+		return -EINVAL;
+	}
+
+	priv->levels = levels;
+	priv->max_brightness = num_levels - 1;
+	priv->default_brightness = value;
+
+	priv->led_cdev = of_led_get(node);
+	if (IS_ERR(priv->led_cdev))
+		return PTR_ERR(priv->led_cdev);
+
+	return 0;
+}
+
+static int led_bl_probe(struct platform_device *pdev)
+{
+	struct backlight_properties props;
+	struct led_bl_data *priv;
+	int ret;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+
+	priv->dev = &pdev->dev;
+
+	ret = led_bl_parse_dt(&pdev->dev, priv);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to parse DT data\n");
+		return ret;
+	}
+
+	priv->enable_gpio = devm_gpiod_get_optional(&pdev->dev, "enable",
+			    GPIOD_OUT_LOW);
+	if (IS_ERR(priv->enable_gpio)) {
+		ret = PTR_ERR(priv->enable_gpio);
+		goto err;
+	}
+
+	priv->power_supply = devm_regulator_get(&pdev->dev, "power");
+	if (IS_ERR(priv->power_supply)) {
+		ret = PTR_ERR(priv->power_supply);
+		goto err;
+	}
+
+	memset(&props, 0, sizeof(struct backlight_properties));
+	props.type = BACKLIGHT_RAW;
+	props.max_brightness = priv->max_brightness;
+	priv->bl_dev = backlight_device_register(dev_name(&pdev->dev),
+			&pdev->dev, priv, &led_bl_ops, &props);
+	if (IS_ERR(priv->bl_dev)) {
+		dev_err(&pdev->dev, "failed to register backlight\n");
+		ret = PTR_ERR(priv->bl_dev);
+		goto err;
+	}
+
+	priv->bl_dev->props.brightness = priv->default_brightness;
+	backlight_update_status(priv->bl_dev);
+
+	return 0;
+
+err:
+	if (priv->led_cdev)
+		led_put(priv->led_cdev);
+
+	return ret;
+}
+
+static int led_bl_remove(struct platform_device *pdev)
+{
+	struct led_bl_data *priv = platform_get_drvdata(pdev);
+	struct backlight_device *bl = priv->bl_dev;
+
+	backlight_device_unregister(bl);
+
+	led_bl_power_off(priv);
+
+	led_put(priv->led_cdev);
+
+	return 0;
+}
+
+static const struct of_device_id led_bl_of_match[] = {
+	{ .compatible = "led-backlight" },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, led_bl_of_match);
+
+static struct platform_driver led_bl_driver = {
+	.driver		= {
+		.name		= "led-backlight",
+		.of_match_table	= of_match_ptr(led_bl_of_match),
+	},
+	.probe		= led_bl_probe,
+	.remove		= led_bl_remove,
+};
+
+module_platform_driver(led_bl_driver);
+
+MODULE_DESCRIPTION("LED based Backlight Driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:led-backlight");
-- 
2.1.4

Re: [PATCH 2/3] backlight: add led-backlight driver

[Andrew Lunn]

On Tue, Aug 25, 2015 at 02:34:01PM +0300, Tomi Valkeinen wrote:
> 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.

Shouldn't this be sending an event to drivers/leds/trigger/kedtrig-backlight.c

Calling led_set_brightness() from outside of drivers/leds is pretty
much unheard of. It is normal to have a trigger to do this. So why not
use the exiting backlight trigger?

      Andrew

Re: [PATCH 2/3] backlight: add led-backlight driver

[Tomi Valkeinen]

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



On 25/08/15 15:39, Andrew Lunn wrote:
> On Tue, Aug 25, 2015 at 02:34:01PM +0300, Tomi Valkeinen wrote:
>> 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.
> 
> Shouldn't this be sending an event to drivers/leds/trigger/kedtrig-backlight.c
>
> Calling led_set_brightness() from outside of drivers/leds is pretty
> much unheard of. It is normal to have a trigger to do this. So why not
> use the exiting backlight trigger?

ledtrig-backlight.c is based on fbdev, and I want the backlight to work
also with DRM. And I want the backlight device to be a proper backlight
device, i.e. something I can pass data to from the DT.

So, I could create a new trigger, say, "bldev" trigger, which would
create a backlight device. But then I could not pass any info to it via DT.

This patch was the only working solution I could figure out, but I don't
have much experience with leds, so I might be missing something.

 Tomi


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

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

[Tomi Valkeinen]

Add DT binding for led-backlight.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt

diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
new file mode 100644
index 000000000000..fb77051ac230
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
@@ -0,0 +1,30 @@
+led-backlight bindings
+
+Required properties:
+  - compatible: "led-backlight"
+  - leds: phandle to a led OF node [0]
+  - brightness-levels: Array of distinct LED brightness levels. These
+      are in the range from 0 to 255, passed to the LED class driver.
+  - default-brightness-level: the default brightness level (index into the
+      array defined by the "brightness-levels" property)
+  - power-supply: regulator for supply voltage
+
+Optional properties:
+  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
+                  and disables the backlight (see GPIO binding[1])
+
+[0]: Documentation/devicetree/bindings/leds/common.txt
+[1]: Documentation/devicetree/bindings/gpio/gpio.txt
+
+Example:
+
+	backlight {
+		compatible = "led-backlight";
+		leds = <&backlight_led>;
+
+		brightness-levels = <0 4 8 16 32 64 128 255>;
+		default-brightness-level = <6>;
+
+		power-supply = <&vdd_bl_reg>;
+		enable-gpios = <&gpio 58 0>;
+	};
-- 
2.1.4

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

[Jacek Anaszewski]

Hi Tomi,

On 08/25/2015 01:34 PM, Tomi Valkeinen wrote:
> Add DT binding for led-backlight.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   .../bindings/video/backlight/led-backlight.txt     | 30 ++++++++++++++++++++++
>   1 file changed, 30 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/video/backlight/led-backlight.txt
>
> diff --git a/Documentation/devicetree/bindings/video/backlight/led-backlight.txt b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> new file mode 100644
> index 000000000000..fb77051ac230
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> @@ -0,0 +1,30 @@
> +led-backlight bindings
> +
> +Required properties:
> +  - compatible: "led-backlight"
> +  - leds: phandle to a led OF node [0]
> +  - brightness-levels: Array of distinct LED brightness levels. These
> +      are in the range from 0 to 255, passed to the LED class driver.
> +  - default-brightness-level: the default brightness level (index into the
> +      array defined by the "brightness-levels" property)
> +  - power-supply: regulator for supply voltage
> +
> +Optional properties:
> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
> +                  and disables the backlight (see GPIO binding[1])
> +
> +[0]: Documentation/devicetree/bindings/leds/common.txt
> +[1]: Documentation/devicetree/bindings/gpio/gpio.txt
> +
> +Example:
> +
> +	backlight {
> +		compatible = "led-backlight";
> +		leds = <&backlight_led>;
> +
> +		brightness-levels = <0 4 8 16 32 64 128 255>;

brightness level is not a suitable unit for describing LED brightness
in a Device Tree, as it is not a physical unit. We have led-max-microamp
property for this, expressed in microamperes, please refer to [0] from
linux-next.

> +		default-brightness-level = <6>;

This also should be microamperes.

> +		power-supply = <&vdd_bl_reg>;
> +		enable-gpios = <&gpio 58 0>;
> +	};
>


-- 
Best Regards,
Jacek Anaszewski

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

[Tomi Valkeinen]

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



On 25/08/15 16:39, Jacek Anaszewski wrote:

>> +Example:
>> +
>> +    backlight {
>> +        compatible = "led-backlight";
>> +        leds = <&backlight_led>;
>> +
>> +        brightness-levels = <0 4 8 16 32 64 128 255>;
> 
> brightness level is not a suitable unit for describing LED brightness
> in a Device Tree, as it is not a physical unit. We have led-max-microamp
> property for this, expressed in microamperes, please refer to [0] from
> linux-next.

Hmm, ok, but what should the driver do with microamperes? As far as I
see, "enum led_brightness" (which is between 0-255) is used to set the
brightness to LEDs. I don't see any function accepting microamperes.

>> +        default-brightness-level = <6>;
> 
> This also should be microamperes.

This is an index to the above brightness-levels array. It's not LED
brightness, but backlight brightness, between 0 and
ARRAY_SIZE(brightness-levels) - 1.

 Tomi


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

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

[Jacek Anaszewski]

On 08/25/2015 05:41 PM, Tomi Valkeinen wrote:
>
>
> On 25/08/15 16:39, Jacek Anaszewski wrote:
>
>>> +Example:
>>> +
>>> +    backlight {
>>> +        compatible = "led-backlight";
>>> +        leds = <&backlight_led>;
>>> +
>>> +        brightness-levels = <0 4 8 16 32 64 128 255>;
>>
>> brightness level is not a suitable unit for describing LED brightness
>> in a Device Tree, as it is not a physical unit. We have led-max-microamp
>> property for this, expressed in microamperes, please refer to [0] from
>> linux-next.
>
> Hmm, ok, but what should the driver do with microamperes? As far as I
> see, "enum led_brightness" (which is between 0-255) is used to set the
> brightness to LEDs. I don't see any function accepting microamperes.

This is implementation detail. You can convert microamperes to
enum led_brightness in the driver. Please refer to the discussion [1].

>>> +        default-brightness-level = <6>;
>>
>> This also should be microamperes.
>
> This is an index to the above brightness-levels array. It's not LED
> brightness, but backlight brightness, between 0 and
> ARRAY_SIZE(brightness-levels) - 1.

You could skip "-level" postfix  and have default brightness
in microamperes.

[1] http://www.spinics.net/lists/linux-leds/msg03416.html

-- 
Best Regards,
Jacek Anaszewski

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

[Tomi Valkeinen]

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



On 26/08/15 10:07, Jacek Anaszewski wrote:
> On 08/25/2015 05:41 PM, Tomi Valkeinen wrote:
>>
>>
>> On 25/08/15 16:39, Jacek Anaszewski wrote:
>>
>>>> +Example:
>>>> +
>>>> +    backlight {
>>>> +        compatible = "led-backlight";
>>>> +        leds = <&backlight_led>;
>>>> +
>>>> +        brightness-levels = <0 4 8 16 32 64 128 255>;
>>>
>>> brightness level is not a suitable unit for describing LED brightness
>>> in a Device Tree, as it is not a physical unit. We have led-max-microamp
>>> property for this, expressed in microamperes, please refer to [0] from
>>> linux-next.
>>
>> Hmm, ok, but what should the driver do with microamperes? As far as I
>> see, "enum led_brightness" (which is between 0-255) is used to set the
>> brightness to LEDs. I don't see any function accepting microamperes.
> 
> This is implementation detail. You can convert microamperes to
> enum led_brightness in the driver. Please refer to the discussion [1].

The led_set_brightness() takes "enum led_brightness", so I don't
understand what this driver would do with the microampere value. It
could, of course, do an arbitrary conversion, say, direct mapping of the
mA value to brightness, but that would just confuse things further.

If there was a led_set_microampere(), supported by all led devices, then
yes, the above should be microamperes.

Or are you saying that I should have the binding use microamperes, and
just do a naive microampere -> brightness conversion in this driver, and
hope that in the future this driver can be changes to pass the
micrompere value directly to the underlying LED driver?

And speaking of microamperes for LED drivers, what's a meaningful
microampere conversion for LED drivers that don't deal with microamperes
at all? For example, the LED chip we're using, TLC59108, is basically a
PWM chip. Can we somehow calculate microamperes if we know duty cycle
and period?

 Tomi


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

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

[Jacek Anaszewski]

On 08/26/2015 11:11 AM, Tomi Valkeinen wrote:
>
>
> On 26/08/15 10:07, Jacek Anaszewski wrote:
>> On 08/25/2015 05:41 PM, Tomi Valkeinen wrote:
>>>
>>>
>>> On 25/08/15 16:39, Jacek Anaszewski wrote:
>>>
>>>>> +Example:
>>>>> +
>>>>> +    backlight {
>>>>> +        compatible = "led-backlight";
>>>>> +        leds = <&backlight_led>;
>>>>> +
>>>>> +        brightness-levels = <0 4 8 16 32 64 128 255>;
>>>>
>>>> brightness level is not a suitable unit for describing LED brightness
>>>> in a Device Tree, as it is not a physical unit. We have led-max-microamp
>>>> property for this, expressed in microamperes, please refer to [0] from
>>>> linux-next.
>>>
>>> Hmm, ok, but what should the driver do with microamperes? As far as I
>>> see, "enum led_brightness" (which is between 0-255) is used to set the
>>> brightness to LEDs. I don't see any function accepting microamperes.
>>
>> This is implementation detail. You can convert microamperes to
>> enum led_brightness in the driver. Please refer to the discussion [1].
>
> The led_set_brightness() takes "enum led_brightness", so I don't
> understand what this driver would do with the microampere value. It
> could, of course, do an arbitrary conversion, say, direct mapping of the
> mA value to brightness, but that would just confuse things further.

OK, I was looking at the problem from LED-centric perspective. Indeed,
backlight subsystem has no other way to pass brightness to the LED
subsystem than in the form of levels. However, the last word belongs
to DT maintainer in this matter.

Cc'ing devicetree@vger.kernel.org.

> If there was a led_set_microampere(), supported by all led devices, then
> yes, the above should be microamperes.
>
> Or are you saying that I should have the binding use microamperes, and
> just do a naive microampere -> brightness conversion in this driver, and
> hope that in the future this driver can be changes to pass the
> micrompere value directly to the underlying LED driver?
>
> And speaking of microamperes for LED drivers, what's a meaningful
> microampere conversion for LED drivers that don't deal with microamperes
> at all? For example, the LED chip we're using, TLC59108, is basically a
> PWM chip. Can we somehow calculate microamperes if we know duty cycle
> and period?

For this type of drivers we have to resort to levels in DT.
It shouldn't serve as an excuse for not describing the rest of
hardware in a better way, though.

-- 
Best Regards,
Jacek Anaszewski

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

[Rob Herring]

On Wed, Aug 26, 2015 at 4:56 AM, Jacek Anaszewski
<j.anaszewski@samsung.com> wrote:
> On 08/26/2015 11:11 AM, Tomi Valkeinen wrote:
>>
>>
>>
>> On 26/08/15 10:07, Jacek Anaszewski wrote:
>>>
>>> On 08/25/2015 05:41 PM, Tomi Valkeinen wrote:
>>>>
>>>>
>>>>
>>>> On 25/08/15 16:39, Jacek Anaszewski wrote:
>>>>
>>>>>> +Example:
>>>>>> +
>>>>>> +    backlight {
>>>>>> +        compatible = "led-backlight";
>>>>>> +        leds = <&backlight_led>;
>>>>>> +
>>>>>> +        brightness-levels = <0 4 8 16 32 64 128 255>;
>>>>>
>>>>>
>>>>> brightness level is not a suitable unit for describing LED brightness
>>>>> in a Device Tree, as it is not a physical unit. We have
>>>>> led-max-microamp
>>>>> property for this, expressed in microamperes, please refer to [0] from
>>>>> linux-next.
>>>>
>>>>
>>>> Hmm, ok, but what should the driver do with microamperes? As far as I
>>>> see, "enum led_brightness" (which is between 0-255) is used to set the
>>>> brightness to LEDs. I don't see any function accepting microamperes.
>>>
>>>
>>> This is implementation detail. You can convert microamperes to
>>> enum led_brightness in the driver. Please refer to the discussion [1].
>>
>>
>> The led_set_brightness() takes "enum led_brightness", so I don't
>> understand what this driver would do with the microampere value. It
>> could, of course, do an arbitrary conversion, say, direct mapping of the
>> mA value to brightness, but that would just confuse things further.
>
>
> OK, I was looking at the problem from LED-centric perspective. Indeed,
> backlight subsystem has no other way to pass brightness to the LED
> subsystem than in the form of levels. However, the last word belongs
> to DT maintainer in this matter.
>
> Cc'ing devicetree@vger.kernel.org.

I don't have a simple answer for you...

There was a similar discussion for pm8941-wled and
"default-brightness-level" units[1]. The conclusion was it should be
units matching the h/w so that there is no conversion between
bootloader and OS units to h/w units. That principle probably applies
here.

If the brightness levels are non-linear, then you need a translation
from percent to h/w level. What's needed here for h/w levels depends
on whether the brightness control is PWM, current control or both. For
PWM, units of the PWM control makes sense. For current control, units
of microamps probably makes sense. I don't know what you do with both,
but I have seen that h/w (FSL PMICs).

This all certainly needs some more work on defining some common
binding. We already have some bindings for backlights with the LED and
PWM bindings (perhaps incomplete?). Do we need another way here? This
also introduces possibility of multiple ways to define GPIO controlled
backlights: gpio -> gpio-leds ->  led-backlight or gpio ->
gpio-backlight. We don't want that...

This problem is not really specific at all to backlights, but applies
to all LEDs. Some LEDs you may not have control beyond on/off or
really care about fine-grained control of level, but they are really
no different. The main unique thing about backlights is what display
are they associated with.

Rob

[1] https://lkml.org/lkml/2015/7/30/542

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

[Jacek Anaszewski]

On 09/01/2015 01:12 AM, Rob Herring wrote:
> On Wed, Aug 26, 2015 at 4:56 AM, Jacek Anaszewski
> <j.anaszewski@samsung.com> wrote:
>> On 08/26/2015 11:11 AM, Tomi Valkeinen wrote:
>>>
>>>
>>>
>>> On 26/08/15 10:07, Jacek Anaszewski wrote:
>>>>
>>>> On 08/25/2015 05:41 PM, Tomi Valkeinen wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 25/08/15 16:39, Jacek Anaszewski wrote:
>>>>>
>>>>>>> +Example:
>>>>>>> +
>>>>>>> +    backlight {
>>>>>>> +        compatible = "led-backlight";
>>>>>>> +        leds = <&backlight_led>;
>>>>>>> +
>>>>>>> +        brightness-levels = <0 4 8 16 32 64 128 255>;
>>>>>>
>>>>>>
>>>>>> brightness level is not a suitable unit for describing LED brightness
>>>>>> in a Device Tree, as it is not a physical unit. We have
>>>>>> led-max-microamp
>>>>>> property for this, expressed in microamperes, please refer to [0] from
>>>>>> linux-next.
>>>>>
>>>>>
>>>>> Hmm, ok, but what should the driver do with microamperes? As far as I
>>>>> see, "enum led_brightness" (which is between 0-255) is used to set the
>>>>> brightness to LEDs. I don't see any function accepting microamperes.
>>>>
>>>>
>>>> This is implementation detail. You can convert microamperes to
>>>> enum led_brightness in the driver. Please refer to the discussion [1].
>>>
>>>
>>> The led_set_brightness() takes "enum led_brightness", so I don't
>>> understand what this driver would do with the microampere value. It
>>> could, of course, do an arbitrary conversion, say, direct mapping of the
>>> mA value to brightness, but that would just confuse things further.
>>
>>
>> OK, I was looking at the problem from LED-centric perspective. Indeed,
>> backlight subsystem has no other way to pass brightness to the LED
>> subsystem than in the form of levels. However, the last word belongs
>> to DT maintainer in this matter.
>>
>> Cc'ing devicetree@vger.kernel.org.
>
> I don't have a simple answer for you...
>
> There was a similar discussion for pm8941-wled and
> "default-brightness-level" units[1]. The conclusion was it should be
> units matching the h/w so that there is no conversion between
> bootloader and OS units to h/w units. That principle probably applies
> here.
>
> If the brightness levels are non-linear, then you need a translation
> from percent to h/w level. What's needed here for h/w levels depends
> on whether the brightness control is PWM, current control or both. For
> PWM, units of the PWM control makes sense. For current control, units
> of microamps probably makes sense. I don't know what you do with both,
> but I have seen that h/w (FSL PMICs).
>
> This all certainly needs some more work on defining some common
> binding. We already have some bindings for backlights with the LED and
> PWM bindings (perhaps incomplete?). Do we need another way here? This
> also introduces possibility of multiple ways to define GPIO controlled
> backlights: gpio -> gpio-leds ->  led-backlight or gpio ->
> gpio-backlight. We don't want that...
>
> This problem is not really specific at all to backlights, but applies
> to all LEDs. Some LEDs you may not have control beyond on/off or
> really care about fine-grained control of level, but they are really
> no different. The main unique thing about backlights is what display
> are they associated with.

Some time ago I was trying to add common LEDs DT properties that would
have defined brightness in levels, but other people insisted on
microamperes [1]. The discussion was focused on flash LEDs, where
currents are significantly greater and there is risk of hardware
damage in case underrated LED is connected [2]. Having the property in
microamperes removes the need for consulting documentation to check
current value for given brightness level.

As it was mentioned earlier in this thread, this approach is not
suitable for PWM driven leds, and they have their own bindings,
which allow for describing brightness in levels. Probably we
should add some note to the common LEDs DT bindings that would
state explicitly that such exceptions are allowed.

In case of backlights which are built upon LED class devices we could
stick to brightness levels, as a LED class driver will assert
brightness level to the leds-max-microamp value anyway.


[1] http://www.spinics.net/lists/linux-leds/msg03416.html
[2] http://patchwork.ozlabs.org/patch/456622/

-- 
Best Regards,
Jacek Anaszewski