Re: [PATCH/RFC v10 03/19] DT: leds: Add led-sources property

From: Jacek Anaszewski
Date: Tue Jan 20 2015 - 11:09:26 EST

On 01/16/2015 04:52 PM, Jacek Anaszewski wrote:
On 01/16/2015 02:48 PM, Rob Herring wrote:
On Fri, Jan 16, 2015 at 3:07 AM, Jacek Anaszewski
<j.anaszewski@xxxxxxxxxxx> wrote:
On 01/15/2015 03:24 PM, Rob Herring wrote:

On Tue, Jan 13, 2015 at 2:42 AM, Jacek Anaszewski
<j.anaszewski@xxxxxxxxxxx> wrote:

On 01/12/2015 05:55 PM, Rob Herring wrote:


Adding Mark B and Liam...

On Mon, Jan 12, 2015 at 10:10 AM, Jacek Anaszewski
<j.anaszewski@xxxxxxxxxxx> wrote:


On 01/12/2015 02:52 PM, Rob Herring wrote:



On Mon, Jan 12, 2015 at 2:32 AM, Jacek Anaszewski
<j.anaszewski@xxxxxxxxxxx> wrote:


On 01/09/2015 07:33 PM, Rob Herring wrote:


On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski
<j.anaszewski@xxxxxxxxxxx> wrote:


Add a property for defining the device outputs the LED
represented by the DT child node is connected to.



[...]

b/Documentation/devicetree/bindings/leds/common.txt
index a2c3f7a..29295bf 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -1,6 +1,10 @@
Common leds properties.

Optional properties for child nodes:
+- led-sources : Array of bits signifying the LED current
regulator
outputs the
+ LED represented by the child node is
connected to
(1
-
the LED
+ is connected to the output, 0 - the LED isn't
connected
to the
+ output).





Sorry, I just don't understand this.





In some Flash LED devices one LED can be connected to one or more
electric current outputs, which allows for multiplying the maximum
current allowed for the LED. Each sub-LED is represented by a
child
node in the DT binding of the Flash LED device and it needs to
declare
which outputs it is connected to. In the example below the
led-sources
property is a two element array, which means that the flash LED
device
has two current outputs, and the bits signify if the LED is
connected
to the output.




Sounds like a regulator for which we already have bindings for
and we
have a driver for regulator based LEDs (but no binding for it).




Do you think of drivers/leds/leds-regulator.c driver? This driver
just
allows for registering an arbitrary regulator device as a LED
subsystem
device.

There are however devices that don't fall into this category,
i.e. they
have many outputs, that can be connected to a single LED or to many
LEDs
and the driver has to know what is the actual arrangement.



We may need to extend the regulator binding slightly and allow for
multiple phandles on a supply property, but wouldn't something like
this work:

led-supply = <&led-reg0>, <&led-reg1>, <&led-reg2>, <&led-reg3>;

The shared source is already supported by the regulator binding.



I think that we shouldn't split the LED devices into power supply
providers and consumers as in case of generic regulators. From this
point of view a LED device current output is a provider and a discrete
LED element is a consumer. In this approach each discrete LED element
should have a related driver which is not how LED devices are being
handled in the LED subsystem, where there is a single binding for a
LED
device and there is a single driver for it which creates separate LED
class devices for each LED connected to the LED device output. Each
discrete LED is represented by a child node in the LED device binding.

I am aware that it may be tempting to treat LED devices as common
regulators, but they have their specific features which gave a
reason for introducing LED class for them. Besides, there is already
drivers/leds/leds-regulator.c driver for LED devices which support
only
turning on/off and setting brightness level.

In your proposition a separate regulator provider binding would have
to be created for each current output and a separate binding for
each discrete LED connected to the LED device. It would create
unnecessary noise in a dts file.

Moreover, using regulator binding implies that we want to treat it
as a sheer power supply for our device (which would be a discrete LED
element in this case), whereas LED devices provide more features like
blinking pattern and for flash LED devices - flash timeout, external
strobe and flash faults.


Okay, fair enough. Please include some of this explanation in the
binding description.

I do still have some concerns about led-sources and whether it can
support other scenarios. It is very much tied to the parent node. Are
there any cases where we don't want the LEDs to be sub nodes? Perhaps
the LEDs are on a separate daughterboard from the driver/supply and we
can have different drivers. It's a stretch maybe.


I think it is. Such arrangements would introduce problems also to the
other existing bindings. Probably not only LED subsystem related ones.

Or are there cases
where you need more information than just the connection?


Currently I can't think of any.

Modified rough proposal of the description:


-Optional properties for child nodes:
+LED and flash LED devices provide the same basic functionality as
+current regulators, but extended with LED and flash LED specific
+features
like blinking patterns, flash timeout, flash faults and
+external flash strobe mode.
+
+Many LED devices expose more than one current output that can be
+connected to one or more discrete LED component. Since the arrangement
+of connections can influence the way of the LED device initialization,
+the LED components have to be tightly coupled with the LED device
+binding. They are represented in the form of its child nodes.
+
+Optional properties for child nodes (if a LED device exposes only one
+current output the properties can be placed directly in the LED device
+node):

Why special case 1 output case? Just always require a child node.

OK.

+- led-sources : Array of connection states between all LED current
+ sources exposed by the device and this LED (1 - this LED
+ is connected to the current output with index N, 0 -
+ this LED isn't connected to the current output with
+ index N); the mapping of N-th element of the array to
+ the physical device output should be defined in the LED
+ driver binding.

I think this should be a list of connected output numbers rather than
effectively a bitmask.

You may want to add something like led-output-cnt or led-driver-cnt in
the parent so you know the max list size.

Why should we need this? The number of current outputs exposed by the
device is fixed and can be specified in a LED device bindings
documentation.


OK. The led-output-cnt property should be put in each sub-node, as the
number of the current outputs each LED can be connected to is variable.

New version:

Optional properties for child nodes:
+led-sources-cnt : Number of device current outputs the LED is connected to.
+- led-sources : List of device current outputs the LED is connected to. The
+ outputs are identified by the numbers that must be defined
+ in the LED device binding documentation.
- label : The label for this LED. If omitted, the label is
taken from the node name (excluding the unit address).

@@ -33,7 +47,9 @@ system-status {

camera-flash {
label = "Flash";
+ led-sources-cnt = <2>;
+ led-sources = <0>, <1>;
max-microamp = <50000>;
flash-max-microamp = <320000>;
flash-timeout-us = <500000>;
-}
+};

--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/