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

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

[Tomi Valkeinen]

Add DT binding for led-backlight.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: devicetree@vger.kernel.org
---
 .../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..d4621d7414bc
--- /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)
+
+Optional properties:
+  - power-supply: regulator for supply voltage
+  - 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: [PATCHv4 3/3] devicetree: Add led-backlight binding

[Lee Jones]

On Wed, 30 Sep 2015, Tomi Valkeinen wrote:

> Add DT binding for led-backlight.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../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..d4621d7414bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/video/backlight/led-backlight.txt
> @@ -0,0 +1,30 @@
> +led-backlight bindings

Make this look like a heading, rather than just another binding.

I would expect to see "LED Backlight Bindings" or similar.

> +Required properties:
> +  - compatible: "led-backlight"
> +  - leds: phandle to a led OF node [0]

s/phandle/Phandle/

s/led/LED/

s/[0]/(See: ../leds/common.txt)

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

s/the/The/

> +      array defined by the "brightness-levels" property)

Tab this out, so:

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

Etc.

> +Optional properties:
> +  - power-supply: regulator for supply voltage

s/regulator/Regulator/

> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables

s/contains/Contains/

> +                  and disables the backlight (see GPIO binding[1])

s/[1]/(See: ../gpio/gpio.txt)

> +[0]: Documentation/devicetree/bindings/leds/common.txt
> +[1]: Documentation/devicetree/bindings/gpio/gpio.txt

Remove these.

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

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Rob Herring]

On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Add DT binding for led-backlight.

Please use get_maintainers.pl.

>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../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..d4621d7414bc
> --- /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]

Why do we need 2 levels of LED nodes?

> +  - 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)
> +
> +Optional properties:
> +  - power-supply: regulator for supply voltage
> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
> +                  and disables the backlight (see GPIO binding[1])

Why are all of these not part of the LED node pointed to by leds?

Describe the h/w, not what you want for a driver.

Rob

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

[Tomi Valkeinen]

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

Hi Rob,

On 13/10/15 17:21, Rob Herring wrote:
> On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> Add DT binding for led-backlight.
> 
> Please use get_maintainers.pl.

At some point I got feedback that the DT maintainers don't have time to
look at each individual driver binding, but rely on the subsystem
maintainers to handle them. Maybe I misunderstood that.

>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> Cc: devicetree@vger.kernel.org
>> ---
>>  .../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..d4621d7414bc
>> --- /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]
> 
> Why do we need 2 levels of LED nodes?

Sorry, didn't get that. What do you mean with 2 levels?

>> +  - 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)
>> +
>> +Optional properties:
>> +  - power-supply: regulator for supply voltage
>> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>> +                  and disables the backlight (see GPIO binding[1])
> 
> Why are all of these not part of the LED node pointed to by leds?

These are for the backlight, not for the LED chip. So "LED" here is a
chip that produces (most likely) a PWM signal, and "backlight" is the
collection of components that use the PWM to produce the backlight
itself, and use the power-supply and gpios.

> Describe the h/w, not what you want for a driver.

I think this describes the HW quite well. The LED chip works fine
without any of the properties here, and these are specific to the
backlight part of the board.

 Tomi


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

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

[Rob Herring]

On Thu, Oct 15, 2015 at 7:17 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 13/10/15 17:21, Rob Herring wrote:
>> On Wed, Sep 30, 2015 at 4:32 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> Add DT binding for led-backlight.
>>
>> Please use get_maintainers.pl.
>
> At some point I got feedback that the DT maintainers don't have time to
> look at each individual driver binding, but rely on the subsystem
> maintainers to handle them. Maybe I misunderstood that.

True, but that doesn't mean to not copy us. If we didn't want to be
copied, we would update MAINTAINERS.

I wouldn't call this one an individual driver either. This is very
much a generic binding which we do want to review.

>
>>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>> Cc: devicetree@vger.kernel.org
>>> ---
>>>  .../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..d4621d7414bc
>>> --- /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]
>>
>> Why do we need 2 levels of LED nodes?
>
> Sorry, didn't get that. What do you mean with 2 levels?

You have the node the "leds" phandle points to which is the actual LED
device and then this node which is just backlight properties. And then
presumably another phandle in the panel device to point to the
backlight device.

>>> +  - 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)
>>> +
>>> +Optional properties:
>>> +  - power-supply: regulator for supply voltage
>>> +  - enable-gpios: contains a single GPIO specifier for the GPIO which enables
>>> +                  and disables the backlight (see GPIO binding[1])
>>
>> Why are all of these not part of the LED node pointed to by leds?
>
> These are for the backlight, not for the LED chip. So "LED" here is a
> chip that produces (most likely) a PWM signal, and "backlight" is the
> collection of components that use the PWM to produce the backlight
> itself, and use the power-supply and gpios.

Okay, it wasn't clear that leds points to the LED controller node. The
example made it seem as it was the device. We already have a way to
describe LEDs and that is as child nodes of the LED controller node.
Please follow what was done for flash LEDs (leds/common.txt).

What's wrong with the existing pwm-backlight binding in the PWM case?

>
>> Describe the h/w, not what you want for a driver.
>
> I think this describes the HW quite well. The LED chip works fine
> without any of the properties here, and these are specific to the
> backlight part of the board.

A more complete example would be helpful here.

Rob

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

[Tomi Valkeinen]

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

Hi Rob,

On 15/10/15 16:46, Rob Herring wrote:

>> At some point I got feedback that the DT maintainers don't have time to
>> look at each individual driver binding, but rely on the subsystem
>> maintainers to handle them. Maybe I misunderstood that.
> 
> True, but that doesn't mean to not copy us. If we didn't want to be
> copied, we would update MAINTAINERS.

Ok.

>>> Why do we need 2 levels of LED nodes?
>>
>> Sorry, didn't get that. What do you mean with 2 levels?
> 
> You have the node the "leds" phandle points to which is the actual LED
> device and then this node which is just backlight properties. And then
> presumably another phandle in the panel device to point to the
> backlight device.

Ok, I see what you mean.

Well, I have to say this is far from perfect. I initially pushed for a
PWM driver for the LED chip we use (tlc591xx), which would have allowed
us to use pwm-backlight driver. But Andrew was using the same chip for
more LED-ish use cases, for which a LED driver was more suitable.

But what I think we really should have is a more generic way to
represent output pins, so that GPIOs (well, GPOs really), PWMs and
current controlled outputs would all be done the same way.

It was rather difficult to use the LED driver and LED bindings for this,
as (afaics) they were really never designed to be used for anything else
than for simple LEDs (i.e. a LED connected directly to the LED chip).
The flash support was added later, but that's almost as simple as the
first case.

>> These are for the backlight, not for the LED chip. So "LED" here is a
>> chip that produces (most likely) a PWM signal, and "backlight" is the
>> collection of components that use the PWM to produce the backlight
>> itself, and use the power-supply and gpios.
> 
> Okay, it wasn't clear that leds points to the LED controller node. The

No, it doesn't point to the main LED node (the one having 'compatible').
It points to a child node.

> example made it seem as it was the device. We already have a way to
> describe LEDs and that is as child nodes of the LED controller node.

True, but those child nodes are very limited. As I see it, those child
nodes really describe the outputs of the LED chip, not what's on the
other end of the lines.

If on the other end of the lines is a more complex device, we need a
proper device driver for it, with a proper DT node with compatible
property etc.

Now, one could argue that a "backlight" that gets the LED signal from a
LED chip is really just a simple LED. But there are complications:

- Our board needs a GPIO to enable the backlight. I can't say what
exactly the GPIO does as my HW skills don't go far enough, but all this
is after the LED chip. I also see the circuitry using powers, which in
our case happen to be always on so we don't need to enable them explicitly.

- We need a backlight device/driver (because of the Linux SW stack).

So, maybe it would be possible to construct all that in a LED child
node, and the LED driver would create a child device for the nodes which
have 'compatible' property. But then, that would be very different from
pwm-backlight, and the parent-child relationships are usually used to
indicate a control relationship, right?

The led-backlight in these patches is very much similar to pwm-backlight.

> Please follow what was done for flash LEDs (leds/common.txt).

The flash support is quite simple. I'm not sure how I could do the same
for the backlight, as I described above.

> What's wrong with the existing pwm-backlight binding in the PWM case?

Nothing, if there's a PWM driver. But if the LED chip is modelled as a
LED driver, pwm-backlight is out. I think there are two kinds of LED
chips, PWM ones and current-controlling ones. And then there are the PWM
devices which are clearly PWM ones.

>>> Describe the h/w, not what you want for a driver.
>>
>> I think this describes the HW quite well. The LED chip works fine
>> without any of the properties here, and these are specific to the
>> backlight part of the board.
> 
> A more complete example would be helpful here.

Of our HW? I can't give the schematics but I hope I described it enough
above.

 Tomi


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

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

[Rob Herring]

On Thu, Oct 15, 2015 at 9:46 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 15/10/15 16:46, Rob Herring wrote:
>
>>> At some point I got feedback that the DT maintainers don't have time to
>>> look at each individual driver binding, but rely on the subsystem
>>> maintainers to handle them. Maybe I misunderstood that.
>>
>> True, but that doesn't mean to not copy us. If we didn't want to be
>> copied, we would update MAINTAINERS.
>
> Ok.
>
>>>> Why do we need 2 levels of LED nodes?
>>>
>>> Sorry, didn't get that. What do you mean with 2 levels?
>>
>> You have the node the "leds" phandle points to which is the actual LED
>> device and then this node which is just backlight properties. And then
>> presumably another phandle in the panel device to point to the
>> backlight device.
>
> Ok, I see what you mean.
>
> Well, I have to say this is far from perfect. I initially pushed for a
> PWM driver for the LED chip we use (tlc591xx), which would have allowed
> us to use pwm-backlight driver. But Andrew was using the same chip for
> more LED-ish use cases, for which a LED driver was more suitable.
>
> But what I think we really should have is a more generic way to
> represent output pins, so that GPIOs (well, GPOs really), PWMs and
> current controlled outputs would all be done the same way.
>
> It was rather difficult to use the LED driver and LED bindings for this,
> as (afaics) they were really never designed to be used for anything else
> than for simple LEDs (i.e. a LED connected directly to the LED chip).
> The flash support was added later, but that's almost as simple as the
> first case.

There's still room to extend it though.

>>> These are for the backlight, not for the LED chip. So "LED" here is a
>>> chip that produces (most likely) a PWM signal, and "backlight" is the
>>> collection of components that use the PWM to produce the backlight
>>> itself, and use the power-supply and gpios.
>>
>> Okay, it wasn't clear that leds points to the LED controller node. The
>
> No, it doesn't point to the main LED node (the one having 'compatible').
> It points to a child node.
>
>> example made it seem as it was the device. We already have a way to
>> describe LEDs and that is as child nodes of the LED controller node.
>
> True, but those child nodes are very limited. As I see it, those child
> nodes really describe the outputs of the LED chip, not what's on the
> other end of the lines.

The child nodes are supposed to be the other end. In the flash case,
the properties are constraints on the flash LED (i.e. different flash
LEDs will have different max currents).

>
> If on the other end of the lines is a more complex device, we need a
> proper device driver for it, with a proper DT node with compatible
> property etc.
>
> Now, one could argue that a "backlight" that gets the LED signal from a
> LED chip is really just a simple LED. But there are complications:

I would say backlights are a complex example of LEDs. Of course, there
are backlights not based on LEDs, but we're not talking about those
here.

> - Our board needs a GPIO to enable the backlight. I can't say what
> exactly the GPIO does as my HW skills don't go far enough, but all this
> is after the LED chip. I also see the circuitry using powers, which in
> our case happen to be always on so we don't need to enable them explicitly.

The GPIO is probably controlling a transistor to connect the LED anode
to ground and therefore turn it on. These have nothing to do with
backlights really, but really are common to LEDs. Every LED needs a
supply rail too. This may come for a regulator or directly from an LED
driver IC.

If the flash LED binding doesn't have these, then it is only a matter of time.

>
> - We need a backlight device/driver (because of the Linux SW stack).

>From a binding perspective, not my problem. The problem with the
driver needs driving the binding definition is the drivers can change
over time. IIRC there has been some discussion of combining the 2
subsystems in the kernel, so we don't want to create something defined
by current kernel needs.

> So, maybe it would be possible to construct all that in a LED child
> node, and the LED driver would create a child device for the nodes which
> have 'compatible' property. But then, that would be very different from
> pwm-backlight, and the parent-child relationships are usually used to
> indicate a control relationship, right?

This is along the lines I was thinking, but don't see how it is very
different at the binding level. The parent-child relationship is
typically control path or just what is downstream from the parent
device. Some bindings like GPIO and PWM don't follow this, but that is
often because they are just additional sideband interfaces on top of
the main control interface. I think simply making the "backlight" node
from the pwm-backlight binding a child works. We probably need a
different compatible string though (led-backlight is as good as
anything). I also think we should require child nodes to have a
compatible string which we didn't do for flash devices. It's probably
not too late to fix that.

I think there are 2 cases of PWM connection to LEDs to consider. The
PWM is an input to a LED driver chip or the PWM directly controls the
LED (attached to the anode). The current pwm-backlight binding covers
the latter. In the former case, pwms should probably be in the parent
(LED driver IC node). Of course, if the driver IC has no s/w
controllable interface beyond PWM, then it probably doesn't need to be
modeled at all in DT.

> The led-backlight in these patches is very much similar to pwm-backlight.

Yes, I think we're really only debating the structure of nodes.

>
>> Please follow what was done for flash LEDs (leds/common.txt).
>
> The flash support is quite simple. I'm not sure how I could do the same
> for the backlight, as I described above.
>
>> What's wrong with the existing pwm-backlight binding in the PWM case?
>
> Nothing, if there's a PWM driver. But if the LED chip is modelled as a
> LED driver, pwm-backlight is out. I think there are two kinds of LED
> chips, PWM ones and current-controlling ones. And then there are the PWM
> devices which are clearly PWM ones.
>
>>>> Describe the h/w, not what you want for a driver.
>>>
>>> I think this describes the HW quite well. The LED chip works fine
>>> without any of the properties here, and these are specific to the
>>> backlight part of the board.
>>
>> A more complete example would be helpful here.
>
> Of our HW? I can't give the schematics but I hope I described it enough
> above.

I just meant the relationship of all the nodes involved.

Rob

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

[Tomi Valkeinen]

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

Hi Rob,

On 15/10/15 21:55, Rob Herring wrote:

>> True, but those child nodes are very limited. As I see it, those child
>> nodes really describe the outputs of the LED chip, not what's on the
>> other end of the lines.
> 
> The child nodes are supposed to be the other end. In the flash case,
> the properties are constraints on the flash LED (i.e. different flash
> LEDs will have different max currents).

Right, but what I meant is that the max current is applied to the LED
controller hardware, so in that sense it can be considered as a property
of the LED controller.

If there was, say, a flash that supports changing the color of the
light, that property would be applied to the flash HW so it'd be a
property of the "other end".

>> If on the other end of the lines is a more complex device, we need a
>> proper device driver for it, with a proper DT node with compatible
>> property etc.
>>
>> Now, one could argue that a "backlight" that gets the LED signal from a
>> LED chip is really just a simple LED. But there are complications:
> 
> I would say backlights are a complex example of LEDs. Of course, there
> are backlights not based on LEDs, but we're not talking about those
> here.

I like the GPIO/PWM binding model, as it doesn't force any
node-hierarchy to the consumer of the GPIO/PWM. I don't see LED
controller output being any different than PWM, but the current LED
bindings still force the consumers of the LED signal to be a child of
the LED controller.

How about an LCD module, controlled via i2c, which takes a LED PWM
signal as an input, but needs i2c commands to enable the actual
backlight? So kind of i2c controlled smart LED. Or if the LCD module
takes two LED PWM signal inputs to achieve some fancy backlight effects.

Yes, it's theoretical HW, and I know some people don't like to discuss
such things... But the above example is easily accomplished with the
GPIO/PWM style bindings, but I don't have any idea how it could be done
with the current LED bindings.

>> - Our board needs a GPIO to enable the backlight. I can't say what
>> exactly the GPIO does as my HW skills don't go far enough, but all this
>> is after the LED chip. I also see the circuitry using powers, which in
>> our case happen to be always on so we don't need to enable them explicitly.
> 
> The GPIO is probably controlling a transistor to connect the LED anode
> to ground and therefore turn it on. These have nothing to do with
> backlights really, but really are common to LEDs. Every LED needs a
> supply rail too. This may come for a regulator or directly from an LED
> driver IC.

There's something a bit more complex there. The gpio controls TI
TPS6108x (http://www.ti.com/lit/ds/symlink/tps61081.pdf "High-Voltage
DC-DC Boost Converter").

But possibly all that can still be considered as you described.

> If the flash LED binding doesn't have these, then it is only a matter of time.
> 
>>
>> - We need a backlight device/driver (because of the Linux SW stack).
> 
> From a binding perspective, not my problem. The problem with the
> driver needs driving the binding definition is the drivers can change
> over time. IIRC there has been some discussion of combining the 2
> subsystems in the kernel, so we don't want to create something defined
> by current kernel needs.

True. But on the other hand I would also not want to force new drivers
to use the current binding model, if it doesn't quite fit.

So if the PWM/GPIO model is fine, and LED controllers are really very
similar, shouldn't we extend the LED bindings towards PWM/GPIO model
rather than trying to fit everything into the current LED bindings?

Of course, even if everybody would agree with the above, this particular
backlight binding is rather simple, and I think we can fit it in the
current model. But that then raises the question, if led-backlight and
pwm-backlight are about the same, why are the binding model different.

>> So, maybe it would be possible to construct all that in a LED child
>> node, and the LED driver would create a child device for the nodes which
>> have 'compatible' property. But then, that would be very different from
>> pwm-backlight, and the parent-child relationships are usually used to
>> indicate a control relationship, right?
> 
> This is along the lines I was thinking, but don't see how it is very
> different at the binding level. The parent-child relationship is

In my experience, the relationship between the nodes is usually the most
difficult part with bindings, both in the design side and the driver
implementation side.

So if pwm-backlight links to the pwm source using a phandle, and
led-backlight links by child-parent relationship, I see them as quite
different, even if the rest of the properties are the same.

> typically control path or just what is downstream from the parent
> device. Some bindings like GPIO and PWM don't follow this, but that is
> often because they are just additional sideband interfaces on top of
> the main control interface. I think simply making the "backlight" node
> from the pwm-backlight binding a child works. We probably need a
> different compatible string though (led-backlight is as good as

So hmm... You mean using the pwm-backlight node, without the "pwms"
property, as the source is implicit? So:

/* tlc59108 is an i2c device */
tlc59116@40 {
	#address-cells = <1>;
	#size-cells = <0>;
	compatible = "ti,tlc59108";
	reg = <0x40>;

	wan@0 {
		label = "wrt1900ac:amber:wan";
		reg = <0x0>;
	};

	bl@2 {
		label = "backlight";
		reg = <0x2>;

		compatible = "led-backlight";
		brightness-levels = <0 243 245 247 248 249 251 252 255>;
		default-brightness-level = <8>;

		enable-gpios = <&pcf_lcd 13 GPIO_ACTIVE_LOW>;
	};
};

At the moment each LED controller driver does its own DT parsing, and
there's no common code for anything related to DT in the LED framework
and the LED framework is not even aware of DT nodes or such (which is
why I needed a bit hackish approach in my patches to find the nodes).

I have a gut feeling that going into this direction will require quite a
bit of restructuring in the LED drivers, so I think I need to leave this
task for others due to lack of time.

> anything). I also think we should require child nodes to have a
> compatible string which we didn't do for flash devices. It's probably
> not too late to fix that.

That is probably a good idea.

> I think there are 2 cases of PWM connection to LEDs to consider. The
> PWM is an input to a LED driver chip or the PWM directly controls the
> LED (attached to the anode). The current pwm-backlight binding covers
> the latter. In the former case, pwms should probably be in the parent
> (LED driver IC node). Of course, if the driver IC has no s/w
> controllable interface beyond PWM, then it probably doesn't need to be
> modeled at all in DT.

I guess one option would also be to create an MFD of the LED controller,
so that it would offer some of the outputs as plain PWMs. Then
pwm-backlight could be used directly.

 Tomi


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

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

[Rob Herring]

 

On Fri, Oct 16, 2015 at 6:42 AM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> Hi Rob,
>
> On 15/10/15 21:55, Rob Herring wrote:
>
>>> True, but those child nodes are very limited. As I see it, those child
>>> nodes really describe the outputs of the LED chip, not what's on the
>>> other end of the lines.
>>
>> The child nodes are supposed to be the other end. In the flash case,
>> the properties are constraints on the flash LED (i.e. different flash
>> LEDs will have different max currents).
>
> Right, but what I meant is that the max current is applied to the LED
> controller hardware, so in that sense it can be considered as a property
> of the LED controller.
>
> If there was, say, a flash that supports changing the color of the
> light, that property would be applied to the flash HW so it'd be a
> property of the "other end".
>
>>> If on the other end of the lines is a more complex device, we need a
>>> proper device driver for it, with a proper DT node with compatible
>>> property etc.
>>>
>>> Now, one could argue that a "backlight" that gets the LED signal from a
>>> LED chip is really just a simple LED. But there are complications:
>>
>> I would say backlights are a complex example of LEDs. Of course, there
>> are backlights not based on LEDs, but we're not talking about those
>> here.
>
> I like the GPIO/PWM binding model, as it doesn't force any
> node-hierarchy to the consumer of the GPIO/PWM. I don't see LED
> controller output being any different than PWM, but the current LED
> bindings still force the consumers of the LED signal to be a child of
> the LED controller.
>
> How about an LCD module, controlled via i2c, which takes a LED PWM
> signal as an input, but needs i2c commands to enable the actual
> backlight? So kind of i2c controlled smart LED. Or if the LCD module
> takes two LED PWM signal inputs to achieve some fancy backlight effects.
>
> Yes, it's theoretical HW, and I know some people don't like to discuss
> such things... But the above example is easily accomplished with the
> GPIO/PWM style bindings, but I don't have any idea how it could be done
> with the current LED bindings.
>
>>> - Our board needs a GPIO to enable the backlight. I can't say what
>>> exactly the GPIO does as my HW skills don't go far enough, but all this
>>> is after the LED chip. I also see the circuitry using powers, which in
>>> our case happen to be always on so we don't need to enable them explicitly.
>>
>> The GPIO is probably controlling a transistor to connect the LED anode
>> to ground and therefore turn it on. These have nothing to do with
>> backlights really, but really are common to LEDs. Every LED needs a
>> supply rail too. This may come for a regulator or directly from an LED
>> driver IC.
>
> There's something a bit more complex there. The gpio controls TI
> TPS6108x (http://www.ti.com/lit/ds/symlink/tps61081.pdf "High-Voltage
> DC-DC Boost Converter").
>
> But possibly all that can still be considered as you described.
>
>> If the flash LED binding doesn't have these, then it is only a matter of time.
>>
>>>
>>> - We need a backlight device/driver (because of the Linux SW stack).
>>
>> From a binding perspective, not my problem. The problem with the
>> driver needs driving the binding definition is the drivers can change
>> over time. IIRC there has been some discussion of combining the 2
>> subsystems in the kernel, so we don't want to create something defined
>> by current kernel needs.
>
> True. But on the other hand I would also not want to force new drivers
> to use the current binding model, if it doesn't quite fit.
>
> So if the PWM/GPIO model is fine, and LED controllers are really very
> similar, shouldn't we extend the LED bindings towards PWM/GPIO model
> rather than trying to fit everything into the current LED bindings?
>
> Of course, even if everybody would agree with the above, this particular
> backlight binding is rather simple, and I think we can fit it in the
> current model. But that then raises the question, if led-backlight and
> pwm-backlight are about the same, why are the binding model different.
>
>>> So, maybe it would be possible to construct all that in a LED child
>>> node, and the LED driver would create a child device for the nodes which
>>> have 'compatible' property. But then, that would be very different from
>>> pwm-backlight, and the parent-child relationships are usually used to
>>> indicate a control relationship, right?
>>
>> This is along the lines I was thinking, but don't see how it is very
>> different at the binding level. The parent-child relationship is
>
> In my experience, the relationship between the nodes is usually the most
> difficult part with bindings, both in the design side and the driver
> implementation side.
>
> So if pwm-backlight links to the pwm source using a phandle, and
> led-backlight links by child-parent relationship, I see them as quite
> different, even if the rest of the properties are the same.
>
>> typically control path or just what is downstream from the parent
>> device. Some bindings like GPIO and PWM don't follow this, but that is
>> often because they are just additional sideband interfaces on top of
>> the main control interface. I think simply making the "backlight" node
>> from the pwm-backlight binding a child works. We probably need a
>> different compatible string though (led-backlight is as good as
>
> So hmm... You mean using the pwm-backlight node, without the "pwms"
> property, as the source is implicit? So:
>
> /* tlc59108 is an i2c device */
> tlc59116@40 {
>         #address-cells = <1>;
>         #size-cells = <0>;
>         compatible = "ti,tlc59108";
>         reg = <0x40>;
>
>         wan@0 {
>                 label = "wrt1900ac:amber:wan";
>                 reg = <0x0>;
>         };
>
>         bl@2 {
>                 label = "backlight";
>                 reg = <0x2>;
>
>                 compatible = "led-backlight";
>                 brightness-levels = <0 243 245 247 248 249 251 252 255>;
>                 default-brightness-level = <8>;
>
>                 enable-gpios = <&pcf_lcd 13 GPIO_ACTIVE_LOW>;
>         };
> };
>
> At the moment each LED controller driver does its own DT parsing, and
> there's no common code for anything related to DT in the LED framework
> and the LED framework is not even aware of DT nodes or such (which is
> why I needed a bit hackish approach in my patches to find the nodes).
>
> I have a gut feeling that going into this direction will require quite a
> bit of restructuring in the LED drivers, so I think I need to leave this
> task for others due to lack of time.
>
>> anything). I also think we should require child nodes to have a
>> compatible string which we didn't do for flash devices. It's probably
>> not too late to fix that.
>
> That is probably a good idea.
>
>> I think there are 2 cases of PWM connection to LEDs to consider. The
>> PWM is an input to a LED driver chip or the PWM directly controls the
>> LED (attached to the anode). The current pwm-backlight binding covers
>> the latter. In the former case, pwms should probably be in the parent
>> (LED driver IC node). Of course, if the driver IC has no s/w
>> controllable interface beyond PWM, then it probably doesn't need to be
>> modeled at all in DT.
>
> I guess one option would also be to create an MFD of the LED controller,
> so that it would offer some of the outputs as plain PWMs. Then
> pwm-backlight could be used directly.
>
>  Tomi
>