* [PATCH v5] DT: leds: Improve description of flash LEDs related properties @ 2015-04-08 14:31 Jacek Anaszewski 2015-04-08 15:15 ` Sylwester Nawrocki 0 siblings, 1 reply; 8+ messages in thread From: Jacek Anaszewski @ 2015-04-08 14:31 UTC (permalink / raw) To: linux-leds Cc: Jacek Anaszewski, Bryan Wu, Richard Purdie, Pavel Machek, Sakari Ailus, Sylwester Nawrocki, devicetree Properties defining maximum values for LED currents and timeout should be mandatory to avoid the risk of hardware damage. This patch fixes the issue. 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: Pavel Machek <pavel@ucw.cz> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Sylwester Nawrocki <s.nawrocki@samsung.com> Cc: devicetree@vger.kernel.org --- Documentation/devicetree/bindings/leds/common.txt | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index 747c538..e478ac6 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -10,6 +10,17 @@ 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 by child nodes of the parent LED device binding. +Required properties for child nodes: +- led-max-microamp : Maximum LED supply current in microamperes + (torch LED for flash devices). Controllers that have no + configurable current can omit this property. +- flash-max-microamp : Maximum flash LED supply current in microamperes. +- flash-timeout-us : Timeout in microseconds after which the flash + LED is turned off. + +Above properties determine a LED driver IC settings required for safe +operation. They should be also used as the initial settings for the IC. + Optional properties for child nodes: - led-sources : List of device current outputs the LED is connected to. The outputs are identified by the numbers that must be defined @@ -29,14 +40,6 @@ Optional properties for child nodes: "ide-disk" - LED indicates disk activity "timer" - LED flashes at a fixed, configurable rate -- max-microamp : maximum intensity in microamperes of the LED - (torch LED for flash devices) -- flash-max-microamp : maximum intensity in microamperes of the - flash LED; it is mandatory if the LED should - support the flash mode -- flash-timeout-us : timeout in microseconds after which the flash - LED is turned off - Examples: -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v5] DT: leds: Improve description of flash LEDs related properties 2015-04-08 14:31 [PATCH v5] DT: leds: Improve description of flash LEDs related properties Jacek Anaszewski @ 2015-04-08 15:15 ` Sylwester Nawrocki 2015-04-09 7:23 ` Jacek Anaszewski 0 siblings, 1 reply; 8+ messages in thread From: Sylwester Nawrocki @ 2015-04-08 15:15 UTC (permalink / raw) To: Jacek Anaszewski Cc: linux-leds, Bryan Wu, Richard Purdie, Pavel Machek, Sakari Ailus, devicetree On 08/04/15 16:31, Jacek Anaszewski wrote: > Properties defining maximum values for LED currents and timeout should > be mandatory to avoid the risk of hardware damage. This patch fixes > the issue. > > Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> > Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > --- > Documentation/devicetree/bindings/leds/common.txt | 19 +++++++++++-------- > 1 file changed, 11 insertions(+), 8 deletions(-) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 747c538..e478ac6 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -10,6 +10,17 @@ 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 > by child nodes of the parent LED device binding. > > +Required properties for child nodes: These properties are mandatory only for LEDs with Flash/Torch capabilities, aren't they? Requiring those properties for all led nodes would make all current dtses not compliant with the DT binding specification AFAICT. How about: "Required properties for child nodes for LEDs with Flash/Torch capabilities:" ? > +- led-max-microamp : Maximum LED supply current in microamperes > + (torch LED for flash devices). Controllers that have no > + configurable current can omit this property. > +- flash-max-microamp : Maximum flash LED supply current in microamperes. > +- flash-timeout-us : Timeout in microseconds after which the flash > + LED is turned off. > + > +Above properties determine a LED driver IC settings required for safe > +operation. They should be also used as the initial settings for the IC. Shouldn't "Controllers that have no configurable current can omit this property" refer to both led-max-microamp and flash-max-microamp? I would drop the "Above...for the IC." paragraph and instead add something like: "For controllers that have no configurable current the led-max-microamp, flash-max-microamp properties respectively can be omitted. For controllers that have no configurable timeout the flash-timeout-us property can be omitted." -- Regards, Sylwester ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] DT: leds: Improve description of flash LEDs related properties 2015-04-08 15:15 ` Sylwester Nawrocki @ 2015-04-09 7:23 ` Jacek Anaszewski 2015-04-09 8:03 ` Sakari Ailus 2015-04-09 9:45 ` Sylwester Nawrocki 0 siblings, 2 replies; 8+ messages in thread From: Jacek Anaszewski @ 2015-04-09 7:23 UTC (permalink / raw) To: Sylwester Nawrocki Cc: linux-leds, Bryan Wu, Richard Purdie, Pavel Machek, Sakari Ailus, devicetree On 04/08/2015 05:15 PM, Sylwester Nawrocki wrote: > On 08/04/15 16:31, Jacek Anaszewski wrote: >> Properties defining maximum values for LED currents and timeout should >> be mandatory to avoid the risk of hardware damage. This patch fixes >> the issue. >> >> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> >> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> > >> --- >> Documentation/devicetree/bindings/leds/common.txt | 19 +++++++++++-------- >> 1 file changed, 11 insertions(+), 8 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt >> index 747c538..e478ac6 100644 >> --- a/Documentation/devicetree/bindings/leds/common.txt >> +++ b/Documentation/devicetree/bindings/leds/common.txt >> @@ -10,6 +10,17 @@ 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 >> by child nodes of the parent LED device binding. >> >> +Required properties for child nodes: > > These properties are mandatory only for LEDs with Flash/Torch capabilities, > aren't they? flash-max-microamp and flash-timeout-us properties description mention that they refer to flash LEDs. Perhaps this should be made indeed more explicit. > Requiring those properties for all led nodes would make all > current dtses not compliant with the DT binding specification AFAICT. I was also worrying about making led-max-microamp required, but others didn't share my objections. I think that we have to reexamine this. Please refer to the discussion under [PATCH v4]. The role of this led-max-microamp property would be preventing hardware damage. > How about: > > "Required properties for child nodes for LEDs with Flash/Torch capabilities:" ? > >> +- led-max-microamp : Maximum LED supply current in microamperes >> + (torch LED for flash devices). Controllers that have no >> + configurable current can omit this property. >> +- flash-max-microamp : Maximum flash LED supply current in microamperes. >> +- flash-timeout-us : Timeout in microseconds after which the flash >> + LED is turned off. >> + > >> +Above properties determine a LED driver IC settings required for safe >> +operation. They should be also used as the initial settings for the IC. > > Shouldn't "Controllers that have no configurable current can omit this > property" refer to both led-max-microamp and flash-max-microamp? > > I would drop the "Above...for the IC." paragraph and instead add something like: > > "For controllers that have no configurable current the led-max-microamp, > flash-max-microamp properties respectively can be omitted. For controllers that > have no configurable timeout the flash-timeout-us property can be omitted." Are we going to avoid mentioning about their role in preventing hardware damage (former "for safe operation" sequence)? -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] DT: leds: Improve description of flash LEDs related properties 2015-04-09 7:23 ` Jacek Anaszewski @ 2015-04-09 8:03 ` Sakari Ailus 2015-04-09 10:03 ` Sylwester Nawrocki 2015-04-09 9:45 ` Sylwester Nawrocki 1 sibling, 1 reply; 8+ messages in thread From: Sakari Ailus @ 2015-04-09 8:03 UTC (permalink / raw) To: Jacek Anaszewski, Sylwester Nawrocki Cc: linux-leds, Bryan Wu, Richard Purdie, Pavel Machek, devicetree Hi Jacek and Sylwester, Jacek Anaszewski wrote: > On 04/08/2015 05:15 PM, Sylwester Nawrocki wrote: >> On 08/04/15 16:31, Jacek Anaszewski wrote: >>> Properties defining maximum values for LED currents and timeout should >>> be mandatory to avoid the risk of hardware damage. This patch fixes >>> the issue. >>> >>> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com> >>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com> >> >>> --- >>> Documentation/devicetree/bindings/leds/common.txt | 19 >>> +++++++++++-------- >>> 1 file changed, 11 insertions(+), 8 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/leds/common.txt >>> b/Documentation/devicetree/bindings/leds/common.txt >>> index 747c538..e478ac6 100644 >>> --- a/Documentation/devicetree/bindings/leds/common.txt >>> +++ b/Documentation/devicetree/bindings/leds/common.txt >>> @@ -10,6 +10,17 @@ 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 >>> by child nodes of the parent LED device binding. >>> >>> +Required properties for child nodes: >> >> These properties are mandatory only for LEDs with Flash/Torch >> capabilities, >> aren't they? > > flash-max-microamp and flash-timeout-us properties description mention > that they refer to flash LEDs. Perhaps this should be made indeed more > explicit. Sounds good to me, after all these shouldn't be defined for devices they don't make sense for. Alternatively, you could add another section for "required properties for flash LEDs". > >> Requiring those properties for all led nodes would make all >> current dtses not compliant with the DT binding specification AFAICT. Some drivers also define these explicitly as optional, the behaviour for those obviously can't be changed. This should apply to new drivers only, and I think documenting the matter in this file would be the best way to do that. > > I was also worrying about making led-max-microamp required, but others > didn't share my objections. I think that we have to reexamine this. > > Please refer to the discussion under [PATCH v4]. The role of this > led-max-microamp property would be preventing hardware damage. > > >> How about: >> >> "Required properties for child nodes for LEDs with Flash/Torch >> capabilities:" ? >> >>> +- led-max-microamp : Maximum LED supply current in microamperes >>> + (torch LED for flash devices). Controllers that Do you think we can just rename max-microamp as led-max-microamp? It's been there since v3.19. Or is it because no driver has been using it? >>> have no >>> + configurable current can omit this property. >>> +- flash-max-microamp : Maximum flash LED supply current in >>> microamperes. >>> +- flash-timeout-us : Timeout in microseconds after which the flash >>> + LED is turned off. >>> + >> >>> +Above properties determine a LED driver IC settings required for safe >>> +operation. They should be also used as the initial settings for the IC. >> >> Shouldn't "Controllers that have no configurable current can omit this >> property" refer to both led-max-microamp and flash-max-microamp? >> >> I would drop the "Above...for the IC." paragraph and instead add >> something like: >> >> "For controllers that have no configurable current the led-max-microamp, >> flash-max-microamp properties respectively can be omitted. For >> controllers that >> have no configurable timeout the flash-timeout-us property can be >> omitted." > > Are we going to avoid mentioning about their role in preventing > hardware damage (former "for safe operation" sequence)? That'd make it understandable why the properties are mandatory. Sounds good to me. -- Kind regards, Sakari Ailus sakari.ailus@linux.intel.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] DT: leds: Improve description of flash LEDs related properties 2015-04-09 8:03 ` Sakari Ailus @ 2015-04-09 10:03 ` Sylwester Nawrocki 2015-04-09 14:39 ` Jacek Anaszewski 0 siblings, 1 reply; 8+ messages in thread From: Sylwester Nawrocki @ 2015-04-09 10:03 UTC (permalink / raw) To: Sakari Ailus Cc: Jacek Anaszewski, linux-leds, Bryan Wu, Richard Purdie, Pavel Machek, devicetree, Rob Herring, Mark Rutland On 09/04/15 10:03, Sakari Ailus wrote: > Jacek Anaszewski wrote: >> On 04/08/2015 05:15 PM, Sylwester Nawrocki wrote: >>> On 08/04/15 16:31, Jacek Anaszewski wrote: >>>> Properties defining maximum values for LED currents and timeout should >>>> be mandatory to avoid the risk of hardware damage. This patch fixes >>>> the issue. >>>> --- a/Documentation/devicetree/bindings/leds/common.txt >>>> +++ b/Documentation/devicetree/bindings/leds/common.txt >>>> @@ -10,6 +10,17 @@ 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 >>>> by child nodes of the parent LED device binding. >>>> >>>> +Required properties for child nodes: >>> >>> These properties are mandatory only for LEDs with Flash/Torch >>> capabilities, aren't they? >> >> flash-max-microamp and flash-timeout-us properties description mention >> that they refer to flash LEDs. Perhaps this should be made indeed more >> explicit. > > Sounds good to me, after all these shouldn't be defined for devices they > don't make sense for. Alternatively, you could add another section for > "required properties for flash LEDs". Sounds good to me. >>> Requiring those properties for all led nodes would make all >>> current dtses not compliant with the DT binding specification AFAICT. > > Some drivers also define these explicitly as optional, the behaviour for > those obviously can't be changed. This should apply to new drivers > only, and I think documenting the matter in this file would be the best > way to do that. Unfortunately this file is not a proper place for information regarding Linux drivers. I can't see any drivers in -next using those DT properties. Could you point to any examples? Perhaps we should stay with the below properties specified as optional in general and add a note they can be mandatory for some LEDs (controllers)? >> I was also worrying about making led-max-microamp required, but others >> didn't share my objections. I think that we have to reexamine this. >> >> Please refer to the discussion under [PATCH v4]. The role of this >> led-max-microamp property would be preventing hardware damage. >> >> >>> How about: >>> >>> "Required properties for child nodes for LEDs with Flash/Torch >>> capabilities:" ? >>> >>>> +- led-max-microamp : Maximum LED supply current in microamperes >>>> + (torch LED for flash devices). Controllers that > > Do you think we can just rename max-microamp as led-max-microamp? It's > been there since v3.19. Or is it because no driver has been using it? I'd say it could be renamed if there is no users in mainline. But the DT maintainers could disagree here (adding Rob, Mark at Cc). >>>> have no >>>> + configurable current can omit this property. >>>> +- flash-max-microamp : Maximum flash LED supply current in >>>> microamperes. >>>> +- flash-timeout-us : Timeout in microseconds after which the flash >>>> + LED is turned off. >>>> + >>> >>>> +Above properties determine a LED driver IC settings required for safe >>>> +operation. They should be also used as the initial settings for the IC. >>> >>> Shouldn't "Controllers that have no configurable current can omit this >>> property" refer to both led-max-microamp and flash-max-microamp? >>> >>> I would drop the "Above...for the IC." paragraph and instead add >>> something like: >>> >>> "For controllers that have no configurable current the led-max-microamp, >>> flash-max-microamp properties respectively can be omitted. For >>> controllers that >>> have no configurable timeout the flash-timeout-us property can be >>> omitted." >> >> Are we going to avoid mentioning about their role in preventing >> hardware damage (former "for safe operation" sequence)? > > That'd make it understandable why the properties are mandatory. Sounds > good to me. -- Regards, Sylwester ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] DT: leds: Improve description of flash LEDs related properties 2015-04-09 10:03 ` Sylwester Nawrocki @ 2015-04-09 14:39 ` Jacek Anaszewski 2015-04-09 23:25 ` Sakari Ailus 0 siblings, 1 reply; 8+ messages in thread From: Jacek Anaszewski @ 2015-04-09 14:39 UTC (permalink / raw) To: Sylwester Nawrocki, Sakari Ailus Cc: linux-leds, Bryan Wu, Richard Purdie, Pavel Machek, devicetree, Rob Herring, Mark Rutland Hi Sylwester, Sakari, On 04/09/2015 12:03 PM, Sylwester Nawrocki wrote: > On 09/04/15 10:03, Sakari Ailus wrote: >> Jacek Anaszewski wrote: >>> On 04/08/2015 05:15 PM, Sylwester Nawrocki wrote: >>>> On 08/04/15 16:31, Jacek Anaszewski wrote: >>>>> Properties defining maximum values for LED currents and timeout should >>>>> be mandatory to avoid the risk of hardware damage. This patch fixes >>>>> the issue. > >>>>> --- a/Documentation/devicetree/bindings/leds/common.txt >>>>> +++ b/Documentation/devicetree/bindings/leds/common.txt >>>>> @@ -10,6 +10,17 @@ 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 >>>>> by child nodes of the parent LED device binding. >>>>> >>>>> +Required properties for child nodes: >>>> >>>> These properties are mandatory only for LEDs with Flash/Torch >>>> capabilities, aren't they? >>> >>> flash-max-microamp and flash-timeout-us properties description mention >>> that they refer to flash LEDs. Perhaps this should be made indeed more >>> explicit. >> >> Sounds good to me, after all these shouldn't be defined for devices they >> don't make sense for. Alternatively, you could add another section for >> "required properties for flash LEDs". > > Sounds good to me. > >>>> Requiring those properties for all led nodes would make all >>>> current dtses not compliant with the DT binding specification AFAICT. >> >> Some drivers also define these explicitly as optional, the behaviour for >> those obviously can't be changed. This should apply to new drivers >> only, and I think documenting the matter in this file would be the best >> way to do that. > > Unfortunately this file is not a proper place for information regarding > Linux drivers. > I can't see any drivers in -next using those DT properties. Could you > point to any examples? > Perhaps we should stay with the below properties specified as optional > in general and add a note they can be mandatory for some LEDs (controllers)? I've analyzed existing led bindings. Four bindings define brightness related properties: leds-lp55xx.txt Each child has own specific current settings - led-cur: Current setting at each led channel (mA x10, 0 if led is not connected) - max-cur: Maximun current at each led channel. leds-netxbig.txt Required sub-node properties: - bright-max: Maximum brightness value leds-pwm LED sub-node properties: - max-brightness : Maximum brightness possible for the LED leds-pm8941-wled.txt Optional properties: - qcom,current-limit: mA; per-string current limit; value from 0 to 25 default: 20mA We can summarize this as follows: - only four devices define max brightness - one of them defines also default current setting - some of them define current in mA, some in brightness levels It has to be stressed that LED subsystem operates on brightness levels, not microamperes. max-microamp property was expressed in microamperes, because it was initially designed only for flash LEDs. I think that if we want to add generic property for limiting maximum brightness for non-flash LEDs, then it should be called brightness-max and expressed in brightness levels. It should apply only to non-flash devices. We could have also brightness-default. For flash devices we would have torch-max-microamp and flash-max-microamp properties. We could also have their counterparts: torch-default-microamp, flash-default-microamp. Should we also need - flash-timeout-max-us - flash-timeout-default-us ? max-brightness and default-brightness would be expressed in brightness levels and would be optional. Flash related properties would be required. >>> I was also worrying about making led-max-microamp required, but others >>> didn't share my objections. I think that we have to reexamine this. >>> >>> Please refer to the discussion under [PATCH v4]. The role of this >>> led-max-microamp property would be preventing hardware damage. >>> >>> >>>> How about: >>>> >>>> "Required properties for child nodes for LEDs with Flash/Torch >>>> capabilities:" ? >>>> >>>>> +- led-max-microamp : Maximum LED supply current in microamperes >>>>> + (torch LED for flash devices). Controllers that >> >> Do you think we can just rename max-microamp as led-max-microamp? It's >> been there since v3.19. Or is it because no driver has been using it? > > I'd say it could be renamed if there is no users in mainline. But the DT > maintainers could disagree here (adding Rob, Mark at Cc). > >>>>> have no >>>>> + configurable current can omit this property. >>>>> +- flash-max-microamp : Maximum flash LED supply current in >>>>> microamperes. >>>>> +- flash-timeout-us : Timeout in microseconds after which the flash >>>>> + LED is turned off. >>>>> + >>>> >>>>> +Above properties determine a LED driver IC settings required for safe >>>>> +operation. They should be also used as the initial settings for the IC. >>>> >>>> Shouldn't "Controllers that have no configurable current can omit this >>>> property" refer to both led-max-microamp and flash-max-microamp? >>>> >>>> I would drop the "Above...for the IC." paragraph and instead add >>>> something like: >>>> >>>> "For controllers that have no configurable current the led-max-microamp, >>>> flash-max-microamp properties respectively can be omitted. For >>>> controllers that >>>> have no configurable timeout the flash-timeout-us property can be >>>> omitted." >>> >>> Are we going to avoid mentioning about their role in preventing >>> hardware damage (former "for safe operation" sequence)? >> >> That'd make it understandable why the properties are mandatory. Sounds >> good to me. > -- Best Regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] DT: leds: Improve description of flash LEDs related properties 2015-04-09 14:39 ` Jacek Anaszewski @ 2015-04-09 23:25 ` Sakari Ailus 0 siblings, 0 replies; 8+ messages in thread From: Sakari Ailus @ 2015-04-09 23:25 UTC (permalink / raw) To: Jacek Anaszewski, Sylwester Nawrocki Cc: linux-leds, Bryan Wu, Richard Purdie, Pavel Machek, devicetree, Rob Herring, Mark Rutland Hi Jacek, Jacek Anaszewski wrote: > Hi Sylwester, Sakari, > > On 04/09/2015 12:03 PM, Sylwester Nawrocki wrote: >> On 09/04/15 10:03, Sakari Ailus wrote: >>> Jacek Anaszewski wrote: >>>> On 04/08/2015 05:15 PM, Sylwester Nawrocki wrote: >>>>> On 08/04/15 16:31, Jacek Anaszewski wrote: >>>>>> Properties defining maximum values for LED currents and timeout >>>>>> should >>>>>> be mandatory to avoid the risk of hardware damage. This patch fixes >>>>>> the issue. >> >>>>>> --- a/Documentation/devicetree/bindings/leds/common.txt >>>>>> +++ b/Documentation/devicetree/bindings/leds/common.txt >>>>>> @@ -10,6 +10,17 @@ 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 >>>>>> by child nodes of the parent LED device binding. >>>>>> >>>>>> +Required properties for child nodes: >>>>> >>>>> These properties are mandatory only for LEDs with Flash/Torch >>>>> capabilities, aren't they? >>>> >>>> flash-max-microamp and flash-timeout-us properties description mention >>>> that they refer to flash LEDs. Perhaps this should be made indeed more >>>> explicit. >>> >>> Sounds good to me, after all these shouldn't be defined for devices they >>> don't make sense for. Alternatively, you could add another section for >>> "required properties for flash LEDs". >> >> Sounds good to me. >> >>>>> Requiring those properties for all led nodes would make all >>>>> current dtses not compliant with the DT binding specification AFAICT. >>> >>> Some drivers also define these explicitly as optional, the behaviour for >>> those obviously can't be changed. This should apply to new drivers >>> only, and I think documenting the matter in this file would be the best >>> way to do that. >> >> Unfortunately this file is not a proper place for information regarding >> Linux drivers. >> I can't see any drivers in -next using those DT properties. Could you >> point to any examples? >> Perhaps we should stay with the below properties specified as optional >> in general and add a note they can be mandatory for some LEDs >> (controllers)? > > I've analyzed existing led bindings. Four bindings define brightness > related properties: > > leds-lp55xx.txt > Each child has own specific current settings > - led-cur: Current setting at each led channel (mA x10, 0 if led is not > connected) > - max-cur: Maximun current at each led channel. > > leds-netxbig.txt > Required sub-node properties: > - bright-max: Maximum brightness value > > leds-pwm > LED sub-node properties: > - max-brightness : Maximum brightness possible for the LED > > leds-pm8941-wled.txt > Optional properties: > - qcom,current-limit: mA; per-string current limit; value from 0 to 25 > default: 20mA > > > We can summarize this as follows: > - only four devices define max brightness > - one of them defines also default current setting > - some of them define current in mA, some in brightness levels > > It has to be stressed that LED subsystem operates on brightness levels, > not microamperes. max-microamp property was expressed in microamperes, > because it was initially designed only for flash LEDs. The DT should describe the hardware, not reflect particular choices made in the interfaces the Linux kernel offers to the user space. I don't think we can change existing bindings, but new bindings should respect what has been discussed in this thread and others previously. The driver should be responsible for converting the current to brightness levels if the hardware deals with current. > > I think that if we want to add generic property for limiting maximum > brightness for non-flash LEDs, then it should be called brightness-max > and expressed in brightness levels. It should apply only to non-flash > devices. We could have also brightness-default. I personally don't know of any device for which brightness level would make sense from hardware point of view. > For flash devices we would have torch-max-microamp and > flash-max-microamp properties. We could also have their counterparts: > torch-default-microamp, flash-default-microamp. > > Should we also need > > - flash-timeout-max-us We have "flash-timeout-us". The documentation isn't very clear on this; it should be the maximum obviously. > - flash-timeout-default-us ? The default is hardly a property of the hardware. Drivers could default to maximum but this could be driver dependent. -- Regards, Sakari Ailus sakari.ailus@linux.intel.com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v5] DT: leds: Improve description of flash LEDs related properties 2015-04-09 7:23 ` Jacek Anaszewski 2015-04-09 8:03 ` Sakari Ailus @ 2015-04-09 9:45 ` Sylwester Nawrocki 1 sibling, 0 replies; 8+ messages in thread From: Sylwester Nawrocki @ 2015-04-09 9:45 UTC (permalink / raw) To: Jacek Anaszewski Cc: linux-leds, Bryan Wu, Richard Purdie, Pavel Machek, Sakari Ailus, devicetree On 09/04/15 09:23, Jacek Anaszewski wrote: > On 04/08/2015 05:15 PM, Sylwester Nawrocki wrote: >> > On 08/04/15 16:31, Jacek Anaszewski wrote: >>> >> Properties defining maximum values for LED currents and timeout should >>> >> be mandatory to avoid the risk of hardware damage. This patch fixes >>> >> the issue. >>> >> --- a/Documentation/devicetree/bindings/leds/common.txt >>> >> +++ b/Documentation/devicetree/bindings/leds/common.txt >>> >> @@ -10,6 +10,17 @@ 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 >>> >> by child nodes of the parent LED device binding. >>> >> >>> >> +Required properties for child nodes: >> > >> > These properties are mandatory only for LEDs with Flash/Torch capabilities, >> > aren't they? > > flash-max-microamp and flash-timeout-us properties description mention > that they refer to flash LEDs. Perhaps this should be made indeed more > explicit. I understood it as if we wanted to make those 3 new properties mandatory for all LEDs. I think it's better to make it explicit to avoid confusion. > > Requiring those properties for all led nodes would make all >> > current dtses not compliant with the DT binding specification AFAICT. > > I was also worrying about making led-max-microamp required, but others > didn't share my objections. I think that we have to reexamine this. > > Please refer to the discussion under [PATCH v4]. The role of this > led-max-microamp property would be preventing hardware damage. Well, we can't simply add such a required property now to all led device nodes. It would cause dtb/kernel compatibility issues. So far led-max-microamp was not required for simple LEDs, why would we have to add it now? >> > How about: >> > >> > "Required properties for child nodes for LEDs with Flash/Torch capabilities:" ? >> > >>> >> +- led-max-microamp : Maximum LED supply current in microamperes >>> >> + (torch LED for flash devices). Controllers that have no >>> >> + configurable current can omit this property. >>> >> +- flash-max-microamp : Maximum flash LED supply current in microamperes. >>> >> +- flash-timeout-us : Timeout in microseconds after which the flash >>> >> + LED is turned off. >>> >> + >> > >>> >> +Above properties determine a LED driver IC settings required for safe >>> >> +operation. They should be also used as the initial settings for the IC. >> > >> > Shouldn't "Controllers that have no configurable current can omit this >> > property" refer to both led-max-microamp and flash-max-microamp? > > >> > I would drop the "Above...for the IC." paragraph and instead add something like: >> > >> > "For controllers that have no configurable current the led-max-microamp, >> > flash-max-microamp properties respectively can be omitted. For controllers that >> > have no configurable timeout the flash-timeout-us property can be omitted." > > Are we going to avoid mentioning about their role in preventing > hardware damage (former "for safe operation" sequence)? I don't have strong opinion, up to you. I'd say this additional note is not needed when we explained those properties specify limits for controllers with configurable current. -- Regards, Sylwester ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-04-09 23:25 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-04-08 14:31 [PATCH v5] DT: leds: Improve description of flash LEDs related properties Jacek Anaszewski 2015-04-08 15:15 ` Sylwester Nawrocki 2015-04-09 7:23 ` Jacek Anaszewski 2015-04-09 8:03 ` Sakari Ailus 2015-04-09 10:03 ` Sylwester Nawrocki 2015-04-09 14:39 ` Jacek Anaszewski 2015-04-09 23:25 ` Sakari Ailus 2015-04-09 9:45 ` Sylwester Nawrocki
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).