| Message ID | 1420816989-1808-4-git-send-email-j.anaszewski@samsung.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri 2015-01-09 16:22:53, Jacek Anaszewski wrote: > Add a property for defining the device outputs the LED > represented by the DT child node is connected to. > > Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Bryan Wu <cooloney@gmail.com> > Cc: Richard Purdie <rpurdie@rpsys.net> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > Cc: Kumar Gala <galak@codeaurora.org> Acked-by: Pavel Machek <pavel@ucw.cz>
On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote: > Add a property for defining the device outputs the LED > represented by the DT child node is connected to. > > Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Bryan Wu <cooloney@gmail.com> > Cc: Richard Purdie <rpurdie@rpsys.net> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Pawel Moll <pawel.moll@arm.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> > Cc: Kumar Gala <galak@codeaurora.org> > --- > Documentation/devicetree/bindings/leds/common.txt | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt 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. Rob > - label : The label for this LED. If omitted, the label is > taken from the node name (excluding the unit address). > > @@ -33,6 +37,7 @@ system-status { > > camera-flash { > label = "Flash"; > + led-sources = <1 0>; > max-microamp = <50000>; > flash-max-microamp = <320000>; > flash-timeout-us = <500000>; > -- > 1.7.9.5 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/09/2015 07:33 PM, Rob Herring wrote: > On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski > <j.anaszewski@samsung.com> wrote: >> Add a property for defining the device outputs the LED >> represented by the DT child node is connected to. >> >> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> >> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> >> Cc: Bryan Wu <cooloney@gmail.com> >> Cc: Richard Purdie <rpurdie@rpsys.net> >> Cc: Rob Herring <robh+dt@kernel.org> >> Cc: Pawel Moll <pawel.moll@arm.com> >> Cc: Mark Rutland <mark.rutland@arm.com> >> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> >> Cc: Kumar Gala <galak@codeaurora.org> >> --- >> Documentation/devicetree/bindings/leds/common.txt | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/leds/common.txt 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. Do your doubts stem from the ambiguity of the word "current" or the form of the description itself is unclear? Probably there should be explicit explanation added that the size of the array depends on the number of current outputs of the flash LED device. >> - label : The label for this LED. If omitted, the label is >> taken from the node name (excluding the unit address). >> >> @@ -33,6 +37,7 @@ system-status { >> >> camera-flash { >> label = "Flash"; >> + led-sources = <1 0>; >> max-microamp = <50000>; >> flash-max-microamp = <320000>; >> flash-timeout-us = <500000>; >> -- >> 1.7.9.5 >> >
On Mon, Jan 12, 2015 at 2:32 AM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote: > On 01/09/2015 07:33 PM, Rob Herring wrote: >> >> On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski >> <j.anaszewski@samsung.com> wrote: >>> >>> Add a property for defining the device outputs the LED >>> represented by the DT child node is connected to. >>> >>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> >>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> >>> Cc: Bryan Wu <cooloney@gmail.com> >>> Cc: Richard Purdie <rpurdie@rpsys.net> >>> Cc: Rob Herring <robh+dt@kernel.org> >>> Cc: Pawel Moll <pawel.moll@arm.com> >>> Cc: Mark Rutland <mark.rutland@arm.com> >>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> >>> Cc: Kumar Gala <galak@codeaurora.org> >>> --- >>> Documentation/devicetree/bindings/leds/common.txt | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/leds/common.txt >>> 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). Please use the regulator binding. > Do your doubts stem from the ambiguity of the word "current" or the > form of the description itself is unclear? Probably there should be > explicit explanation added that the size of the array depends on the > number of current outputs of the flash LED device. The size of the array and meaning of array indexes was not clear. Rob > > >>> - label : The label for this LED. If omitted, the label is >>> taken from the node name (excluding the unit address). >>> >>> @@ -33,6 +37,7 @@ system-status { >>> >>> camera-flash { >>> label = "Flash"; >>> + led-sources = <1 0>; >>> max-microamp = <50000>; >>> flash-max-microamp = <320000>; >>> flash-timeout-us = <500000>; >>> -- >>> 1.7.9.5 >>> >> > > -- > Best Regards, > Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/12/2015 02:52 PM, Rob Herring wrote: > On Mon, Jan 12, 2015 at 2:32 AM, Jacek Anaszewski > <j.anaszewski@samsung.com> wrote: >> On 01/09/2015 07:33 PM, Rob Herring wrote: >>> >>> On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski >>> <j.anaszewski@samsung.com> wrote: >>>> >>>> Add a property for defining the device outputs the LED >>>> represented by the DT child node is connected to. >>>> >>>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> >>>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> >>>> Cc: Bryan Wu <cooloney@gmail.com> >>>> Cc: Richard Purdie <rpurdie@rpsys.net> >>>> Cc: Rob Herring <robh+dt@kernel.org> >>>> Cc: Pawel Moll <pawel.moll@arm.com> >>>> Cc: Mark Rutland <mark.rutland@arm.com> >>>> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk> >>>> Cc: Kumar Gala <galak@codeaurora.org> >>>> --- >>>> Documentation/devicetree/bindings/leds/common.txt | 5 +++++ >>>> 1 file changed, 5 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/leds/common.txt >>>> 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. > Please use the regulator binding. >> Do your doubts stem from the ambiguity of the word "current" or the >> form of the description itself is unclear? Probably there should be >> explicit explanation added that the size of the array depends on the >> number of current outputs of the flash LED device. > > The size of the array and meaning of array indexes was not clear. What about this: 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.
Adding Mark B and Liam... On Mon, Jan 12, 2015 at 10:10 AM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote: > On 01/12/2015 02:52 PM, Rob Herring wrote: >> >> On Mon, Jan 12, 2015 at 2:32 AM, Jacek Anaszewski >> <j.anaszewski@samsung.com> wrote: >>> On 01/09/2015 07:33 PM, Rob Herring wrote: >>>> On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski >>>> <j.anaszewski@samsung.com> 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. Rob > >> Please use the regulator binding. > > >>> Do your doubts stem from the ambiguity of the word "current" or the >>> form of the description itself is unclear? Probably there should be >>> explicit explanation added that the size of the array depends on the >>> number of current outputs of the flash LED device. >> >> >> The size of the array and meaning of array indexes was not clear. > > > What about this: > > 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. > > > -- > Best Regards, > Jacek Anaszewski -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 12, 2015 at 10:55:29AM -0600, Rob Herring wrote: > On Mon, Jan 12, 2015 at 10:10 AM, Jacek Anaszewski > > 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. What is the reasoning for this? If a single supply is being supplied by multiple regulators then in general those regulators will all know about each other at a hardware level and so from a functional and software point of view will effectively be one regulator. If they don't/aren't then they tend to interfere with each other.
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@samsung.com> wrote: >> On 01/12/2015 02:52 PM, Rob Herring wrote: >>> >>> On Mon, Jan 12, 2015 at 2:32 AM, Jacek Anaszewski >>> <j.anaszewski@samsung.com> wrote: >>>> On 01/09/2015 07:33 PM, Rob Herring wrote: >>>>> On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski >>>>> <j.anaszewski@samsung.com> 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.
On 12/01/15 18:06, Mark Brown wrote: > On Mon, Jan 12, 2015 at 10:55:29AM -0600, Rob Herring wrote: >> > On Mon, Jan 12, 2015 at 10:10 AM, Jacek Anaszewski >>> > > 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. > > What is the reasoning for this? If a single supply is being supplied by > multiple regulators then in general those regulators will all know about > each other at a hardware level and so from a functional and software > point of view will effectively be one regulator. If they don't/aren't > then they tend to interfere with each other. For LED current regulators like this one [1] we want to be able to communicate to the software the hardware wiring, e.g. if a single LED is connected to only one or both the current regulators. The device needs to be programmed differently for each configuration, as shown on page 36 of the datasheet [2]. Now, the LED DT binding describes the LEDs (current consumers) as child nodes of the LED driver IC (current supplier), e.g. (from [3]): pca9632@62 { compatible = "nxp,pca9632"; #address-cells = <1>; #size-cells = <0>; reg = <0x62>; red@0 { label = "red"; reg = <0>; linux,default-trigger = "none"; }; green@1 { label = "green"; reg = <1>; linux,default-trigger = "none"; }; ... }; What is missing in this binding is the ability to tell that a single LED is connected to more than one current source. We could, for example adopt the multiple phandle in the supply property scheme, but not use the kernel regulator API, e.g. flash-led { compatible = "maxim,max77387"; current-reg1 { // FLED1 led-output-id = <0>; }; current-reg2 { // FLED2 led-output-id = <1>; }; red_led { led-supply = <¤t-reg1>, <¤t-reg2>; }; }; However my feeling is that it is unnecessarily complicated that way. Perhaps we could use the 'reg' property to describe actual connections, I'm not sure if it's better than a LED specific property, e.g. max77387@52 { compatible = "nxp,max77387"; #address-cells = <2>; #size-cells = <0>; reg = <0x52>; flash_led { reg = <1 1>; ... }; }; [1] http://www.maximintegrated.com/en/products/power/led-drivers/MAX77387.html [2] http://datasheets.maximintegrated.com/en/ds/MAX77387.pdf [3] Documentation/devicetree/bindings/leds/pca963x.txt -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 13, 2015 at 2:42 AM, Jacek Anaszewski <j.anaszewski@samsung.com> 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@samsung.com> wrote: >>> >>> On 01/12/2015 02:52 PM, Rob Herring wrote: >>>> >>>> >>>> On Mon, Jan 12, 2015 at 2:32 AM, Jacek Anaszewski >>>> <j.anaszewski@samsung.com> wrote: >>>>> >>>>> On 01/09/2015 07:33 PM, Rob Herring wrote: >>>>>> >>>>>> On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski >>>>>> <j.anaszewski@samsung.com> 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. Or are there cases where you need more information than just the connection? Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 15, 2015 at 6:33 AM, Sylwester Nawrocki <s.nawrocki@samsung.com> wrote: > On 12/01/15 18:06, Mark Brown wrote: >> On Mon, Jan 12, 2015 at 10:55:29AM -0600, Rob Herring wrote: >>> > On Mon, Jan 12, 2015 at 10:10 AM, Jacek Anaszewski >>>> > > 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. >> >> What is the reasoning for this? If a single supply is being supplied by >> multiple regulators then in general those regulators will all know about >> each other at a hardware level and so from a functional and software >> point of view will effectively be one regulator. If they don't/aren't >> then they tend to interfere with each other. > > For LED current regulators like this one [1] we want to be able to > communicate to the software the hardware wiring, e.g. if a single LED is > connected to only one or both the current regulators. The device needs > to be programmed differently for each configuration, as shown on page 36 > of the datasheet [2]. > > Now, the LED DT binding describes the LEDs (current consumers) as child > nodes of the LED driver IC (current supplier), e.g. (from [3]): > > pca9632@62 { > compatible = "nxp,pca9632"; > #address-cells = <1>; > #size-cells = <0>; > reg = <0x62>; > > red@0 { > label = "red"; > reg = <0>; This only works if you don't have sub blocks or different functions to describe. I suppose you could add yet another level of nodes. This feels like abuse of the reg property even though to use the reg property is a frequent review comment. OTOH, we don't need 2 ways to describe this. > linux,default-trigger = "none"; > }; > green@1 { > label = "green"; > reg = <1>; > linux,default-trigger = "none"; > }; > ... > }; > > What is missing in this binding is the ability to tell that a single LED > is connected to more than one current source. > > We could, for example adopt the multiple phandle in the supply property > scheme, but not use the kernel regulator API, e.g. > > flash-led { > compatible = "maxim,max77387"; > > current-reg1 { // FLED1 > led-output-id = <0>; > }; > > current-reg2 { // FLED2 > led-output-id = <1>; > }; > > red_led { > led-supply = <¤t-reg1>, <¤t-reg2>; > }; > }; > > However my feeling is that it is unnecessarily complicated that way. This example is not so complicated, but I already agreed on not using regulators on the basis there are other properties of the driver unique to LEDs. > Perhaps we could use the 'reg' property to describe actual connections, > I'm not sure if it's better than a LED specific property, e.g. > > max77387@52 { > compatible = "nxp,max77387"; > #address-cells = <2>; > #size-cells = <0>; > reg = <0x52>; > > flash_led { > reg = <1 1>; Don't you mean <0 1> as the values are the "address" which in this case are the LED driver output indexes. Rob > ... > }; > }; > > [1] http://www.maximintegrated.com/en/products/power/led-drivers/MAX77387.html > [2] http://datasheets.maximintegrated.com/en/ds/MAX77387.pdf > [3] Documentation/devicetree/bindings/leds/pca963x.txt -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 15, 2015 at 08:24:26AM -0600, Rob Herring wrote: > On Tue, Jan 13, 2015 at 2:42 AM, Jacek Anaszewski > > 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. Right, the other thing here is that these chips are not normally designed with the goal that the various components be used separately - I've seen devices where the startup and shutdown procedures involve interleaved actions to the boost regulator and current sink for example. Trying to insert an abstraction layer typically just makes life more complex.
Hi! > Perhaps we could use the 'reg' property to describe actual connections, > I'm not sure if it's better than a LED specific property, e.g. > > max77387@52 { > compatible = "nxp,max77387"; > #address-cells = <2>; > #size-cells = <0>; > reg = <0x52>; > > flash_led { > reg = <1 1>; > ... > }; > }; Normally, reg property is <start length>, if I understand things correctly? Would that be enough here, or would we be doing list of outputs? Thanks, Pavel
On 01/15/2015 03:24 PM, Rob Herring wrote: > On Tue, Jan 13, 2015 at 2:42 AM, Jacek Anaszewski > <j.anaszewski@samsung.com> 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@samsung.com> wrote: >>>> >>>> On 01/12/2015 02:52 PM, Rob Herring wrote: >>>>> >>>>> >>>>> On Mon, Jan 12, 2015 at 2:32 AM, Jacek Anaszewski >>>>> <j.anaszewski@samsung.com> wrote: >>>>>> >>>>>> On 01/09/2015 07:33 PM, Rob Herring wrote: >>>>>>> >>>>>>> On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski >>>>>>> <j.anaszewski@samsung.com> 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): +- 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.
Hi, On 15/01/15 22:03, Pavel Machek wrote: >> Perhaps we could use the 'reg' property to describe actual connections, >> > I'm not sure if it's better than a LED specific property, e.g. >> > >> > max77387@52 { >> > compatible = "nxp,max77387"; >> > #address-cells = <2>; >> > #size-cells = <0>; >> > reg = <0x52>; >> > >> > flash_led { >> > reg = <1 1>; >> > ... >> > }; >> > }; > > Normally, reg property is <start length>, if I understand things > correctly? Would that be enough here, or would we be doing list of > outputs? In general the exact meaning depends on value of the #address-cells and #size-cells properties in the parent node. In case as above #size-cells is 0. You can find exact explanation in [1], at page 25. Anyway, the above example might an abuse of the simple bus. I thought more about a list of outputs, but the indexes wouldn't be contiguous, e.g. curr. reg. outputs | "addres" value -------------------------------- FLED2 FLED1 | reg --------------------+----------- 0 1 | 0x00000001 1 0 | 0x00010000 1 1 | 0x00010001 But it might be not a good idea as Rob pointed out. OTOH we could simply assign indices 1,2,3 to the above FLED1/2 output combinations. [1] https://www.power.org/wp-content/uploads/2012/06/Power_ePAPR_APPROVED_v1.1.pdf -- Regards, Sylwester -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 16, 2015 at 3:07 AM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote: > On 01/15/2015 03:24 PM, Rob Herring wrote: >> >> On Tue, Jan 13, 2015 at 2:42 AM, Jacek Anaszewski >> <j.anaszewski@samsung.com> 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@samsung.com> wrote: >>>>> >>>>> >>>>> On 01/12/2015 02:52 PM, Rob Herring wrote: >>>>>> >>>>>> >>>>>> >>>>>> On Mon, Jan 12, 2015 at 2:32 AM, Jacek Anaszewski >>>>>> <j.anaszewski@samsung.com> wrote: >>>>>>> >>>>>>> >>>>>>> On 01/09/2015 07:33 PM, Rob Herring wrote: >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski >>>>>>>> <j.anaszewski@samsung.com> 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. > +- 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. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/16/2015 02:48 PM, Rob Herring wrote: > On Fri, Jan 16, 2015 at 3:07 AM, Jacek Anaszewski > <j.anaszewski@samsung.com> wrote: >> On 01/15/2015 03:24 PM, Rob Herring wrote: >>> >>> On Tue, Jan 13, 2015 at 2:42 AM, Jacek Anaszewski >>> <j.anaszewski@samsung.com> 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@samsung.com> wrote: >>>>>> >>>>>> >>>>>> On 01/12/2015 02:52 PM, Rob Herring wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Mon, Jan 12, 2015 at 2:32 AM, Jacek Anaszewski >>>>>>> <j.anaszewski@samsung.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 01/09/2015 07:33 PM, Rob Herring wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski >>>>>>>>> <j.anaszewski@samsung.com> 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.
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@samsung.com> wrote: >>> On 01/15/2015 03:24 PM, Rob Herring wrote: >>>> >>>> On Tue, Jan 13, 2015 at 2:42 AM, Jacek Anaszewski >>>> <j.anaszewski@samsung.com> 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@samsung.com> wrote: >>>>>>> >>>>>>> >>>>>>> On 01/12/2015 02:52 PM, Rob Herring wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Mon, Jan 12, 2015 at 2:32 AM, Jacek Anaszewski >>>>>>>> <j.anaszewski@samsung.com> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 01/09/2015 07:33 PM, Rob Herring wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Fri, Jan 9, 2015 at 9:22 AM, Jacek Anaszewski >>>>>>>>>> <j.anaszewski@samsung.com> 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>; -} +};
On Tue, Jan 20, 2015 at 10:09 AM, Jacek Anaszewski <j.anaszewski@samsung.com> wrote: > On 01/16/2015 04:52 PM, Jacek Anaszewski wrote: >> >> On 01/16/2015 02:48 PM, Rob Herring wrote: [...] >>> 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. Sorry, I meant this for the parent node meaning how many outputs the driver IC has. I did say maybe because you may always know this. It can make it easier to allocate memory for led-sources knowing the max size up front. Rob > > 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 2015-01-20 11:29:16, Rob Herring wrote: > On Tue, Jan 20, 2015 at 10:09 AM, Jacek Anaszewski > <j.anaszewski@samsung.com> wrote: > > On 01/16/2015 04:52 PM, Jacek Anaszewski wrote: > >> > >> On 01/16/2015 02:48 PM, Rob Herring wrote: > > [...] > > >>> 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. > > Sorry, I meant this for the parent node meaning how many outputs the > driver IC has. I did say maybe because you may always know this. It > can make it easier to allocate memory for led-sources knowing the max > size up front. Umm. Not sure if that kind of "help" should go to the device tree. Pavel
On 01/20/2015 06:40 PM, Pavel Machek wrote: > On Tue 2015-01-20 11:29:16, Rob Herring wrote: >> On Tue, Jan 20, 2015 at 10:09 AM, Jacek Anaszewski >> <j.anaszewski@samsung.com> wrote: >>> On 01/16/2015 04:52 PM, Jacek Anaszewski wrote: >>>> >>>> On 01/16/2015 02:48 PM, Rob Herring wrote: >> >> [...] >> >>>>> 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. >> >> Sorry, I meant this for the parent node meaning how many outputs the >> driver IC has. I did say maybe because you may always know this. It >> can make it easier to allocate memory for led-sources knowing the max >> size up front. > > Umm. Not sure if that kind of "help" should go to the device > tree. > Pavel > I agree. I think that led-sources-cnt is redundant. A list property can be read using of_prop_next_u32 function. I missed that and thus proposed putting led-sources-cnt in each sub-node to be able to read led-sources list with of_property_read_u32_array. Effectively, I propose to avoid adding led-sources-cnt property.
On Wed, Jan 21, 2015 at 10:39:05AM +0100, Jacek Anaszewski wrote: > I agree. I think that led-sources-cnt is redundant. A list property > can be read using of_prop_next_u32 function. I missed that and thus > proposed putting led-sources-cnt in each sub-node to be able to read > led-sources list with of_property_read_u32_array. > > Effectively, I propose to avoid adding led-sources-cnt property. You can also read the array size using of_get_property().
diff --git a/Documentation/devicetree/bindings/leds/common.txt 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). - label : The label for this LED. If omitted, the label is taken from the node name (excluding the unit address). @@ -33,6 +37,7 @@ system-status { camera-flash { label = "Flash"; + led-sources = <1 0>; max-microamp = <50000>; flash-max-microamp = <320000>; flash-timeout-us = <500000>;