| Message ID | 1427809965-25540-2-git-send-email-j.anaszewski@samsung.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue 2015-03-31 15:52:37, Jacek Anaszewski wrote: > Description of flash LEDs related properties was not precise regarding > the state of corresponding settings in case a property is missing. > Add relevant statements. > Removed is also the requirement making the flash-max-microamp > property obligatory for flash LEDs. It was inconsistent as the property > is defined as optional. Devices which require the property will have > to assert this in their DT bindings. > > 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> Acked-by: Pavel Machek <pavel@ucw.cz> > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 747c538..21a25e4 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -29,13 +29,15 @@ 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 > +- max-microamp : Maximum intensity in microamperes of the LED > + (torch LED for flash devices). If omitted this will default > + to the maximum current allowed by the device. > +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. > + If omitted this will default to the maximum > + current allowed by the device. > +- flash-timeout-us : Timeout in microseconds after which the flash > + LED is turned off. If omitted this will default to the > + maximum timeout allowed by the device.
Hi Jacek, On Tue, Mar 31, 2015 at 03:52:37PM +0200, Jacek Anaszewski wrote: > Description of flash LEDs related properties was not precise regarding > the state of corresponding settings in case a property is missing. > Add relevant statements. > Removed is also the requirement making the flash-max-microamp > property obligatory for flash LEDs. It was inconsistent as the property > is defined as optional. Devices which require the property will have > to assert this in their DT bindings. > > 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: devicetree@vger.kernel.org > --- > Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 747c538..21a25e4 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -29,13 +29,15 @@ 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 > +- max-microamp : Maximum intensity in microamperes of the LED > + (torch LED for flash devices). If omitted this will default > + to the maximum current allowed by the device. > +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. > + If omitted this will default to the maximum > + current allowed by the device. > +- flash-timeout-us : Timeout in microseconds after which the flash > + LED is turned off. If omitted this will default to the > + maximum timeout allowed by the device. > > > Examples: Pavel pointed out that the brightness between maximum current and the maximum *allowed* another current might not be noticeable, leading a potential spelling error to cause the LED being run at too high current. The three drivers I've looked also require these properties, which I think is in the line with the above. How about either dropping the patch, or changing maximum to minimum and will to should? The drivers could also behave this way instead of requiring the properties, but I don't think there's anything wrong with requiring the properties either. I think this is worth considering now as we can't change this later without breaking something.
Hi Sakari, On 04/03/2015 02:09 PM, Sakari Ailus wrote: > Hi Jacek, > > On Tue, Mar 31, 2015 at 03:52:37PM +0200, Jacek Anaszewski wrote: >> Description of flash LEDs related properties was not precise regarding >> the state of corresponding settings in case a property is missing. >> Add relevant statements. >> Removed is also the requirement making the flash-max-microamp >> property obligatory for flash LEDs. It was inconsistent as the property >> is defined as optional. Devices which require the property will have >> to assert this in their DT bindings. >> >> 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: devicetree@vger.kernel.org >> --- >> Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt >> index 747c538..21a25e4 100644 >> --- a/Documentation/devicetree/bindings/leds/common.txt >> +++ b/Documentation/devicetree/bindings/leds/common.txt >> @@ -29,13 +29,15 @@ 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 >> +- max-microamp : Maximum intensity in microamperes of the LED >> + (torch LED for flash devices). If omitted this will default >> + to the maximum current allowed by the device. >> +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. >> + If omitted this will default to the maximum >> + current allowed by the device. >> +- flash-timeout-us : Timeout in microseconds after which the flash >> + LED is turned off. If omitted this will default to the >> + maximum timeout allowed by the device. >> >> >> Examples: > > Pavel pointed out that the brightness between maximum current and the > maximum *allowed* another current might not be noticeable,leading a > potential spelling error to cause the LED being run at too high current. Where did he point this out? Do you think about the current version of the leds/common.txt documentation or there was some other message, that I don't see? Besides, I can't understand your point. Could you express it in other words, please? > The three drivers I've looked also require these properties, which I think > is in the line with the above. These properties were introduced two months ago and there is no merged driver which require them. It means that the drivers that are currently in the mainline assume that they can set the maximum current (please note that max-microamp property refers to non-flash LEDs too). The modifications I proposed just validate the current status. > How about either dropping the patch, or changing maximum to minimum and > will to should? The drivers could also behave this way instead of requiring > the properties, but I don't think there's anything wrong with requiring the > properties either. > > I think this is worth considering now as we can't change this later without > breaking something. >
Hi! > >>+- flash-timeout-us : Timeout in microseconds after which the flash > >>+ LED is turned off. If omitted this will default to the > >>+ maximum timeout allowed by the device. > >> > >> > >> Examples: > > > >Pavel pointed out that the brightness between maximum current and the > >maximum *allowed* another current might not be noticeable,leading a > >potential spelling error to cause the LED being run at too high current. > > Where did he point this out? Do you think about the current version > of the leds/common.txt documentation or there was some other message, > that I don't see? Date: Thu, 2 Apr 2015 22:30:44 +0200 From: Pavel Machek <pavel@ucw.cz> To: Sakari Ailus <sakari.ailus@iki.fi> Subject: Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653 > Besides, I can't understand your point. Could you express it in other > words, please? Typo in device tree would cause hardware damage. But idea. Make the properties mandatory. Pavel
On Fri, Apr 3, 2015 at 1:37 PM, Pavel Machek <pavel@ucw.cz> wrote: > Hi! > >> >>+- flash-timeout-us : Timeout in microseconds after which the flash >> >>+ LED is turned off. If omitted this will default to the >> >>+ maximum timeout allowed by the device. >> >> >> >> >> >> Examples: >> > >> >Pavel pointed out that the brightness between maximum current and the >> >maximum *allowed* another current might not be noticeable,leading a >> >potential spelling error to cause the LED being run at too high current. >> >> Where did he point this out? Do you think about the current version >> of the leds/common.txt documentation or there was some other message, >> that I don't see? > > Date: Thu, 2 Apr 2015 22:30:44 +0200 > From: Pavel Machek <pavel@ucw.cz> > To: Sakari Ailus <sakari.ailus@iki.fi> > Subject: Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653 > >> Besides, I can't understand your point. Could you express it in other >> words, please? > > Typo in device tree would cause hardware damage. But idea. Make the > properties mandatory. > Pavel I don't quite follow there. I think Pavel acked this patch right? So what's left to hold here? Thanks, -Bryan -- 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
Hi Bryan, Bryan Wu wrote: > On Fri, Apr 3, 2015 at 1:37 PM, Pavel Machek <pavel@ucw.cz> wrote: >> Hi! >> >>>>> +- flash-timeout-us : Timeout in microseconds after which the flash >>>>> + LED is turned off. If omitted this will default to the >>>>> + maximum timeout allowed by the device. >>>>> >>>>> >>>>> Examples: >>>> >>>> Pavel pointed out that the brightness between maximum current and the >>>> maximum *allowed* another current might not be noticeable,leading a >>>> potential spelling error to cause the LED being run at too high current. >>> >>> Where did he point this out? Do you think about the current version >>> of the leds/common.txt documentation or there was some other message, >>> that I don't see? >> >> Date: Thu, 2 Apr 2015 22:30:44 +0200 >> From: Pavel Machek <pavel@ucw.cz> >> To: Sakari Ailus <sakari.ailus@iki.fi> >> Subject: Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653 >> >>> Besides, I can't understand your point. Could you express it in other >>> words, please? >> >> Typo in device tree would cause hardware damage. But idea. Make the >> properties mandatory. >> Pavel > > I don't quite follow there. I think Pavel acked this patch right? So > what's left to hold here? LED flash controllers are capable of providing more current than the LEDs attached to them can withstand without hardware damage. This is the reason the maximum current limits lower than the LED controller maximums are there. Pavel, quite rightly so in my opinion, is suggesting these properties are made mandatory. A typo in the DT source could cause the controller maximum current used instead of LED maximum which is often lower. That kind of a problem would easily go unnoticed since there isn't necessarily any perceivable change in the functionality of the board. You'd only notice later on, when the LEDs stop working.
Hi Sakari, On 04/03/2015 02:09 PM, Sakari Ailus wrote: > Hi Jacek, > > On Tue, Mar 31, 2015 at 03:52:37PM +0200, Jacek Anaszewski wrote: >> Description of flash LEDs related properties was not precise regarding >> the state of corresponding settings in case a property is missing. >> Add relevant statements. >> Removed is also the requirement making the flash-max-microamp >> property obligatory for flash LEDs. It was inconsistent as the property >> is defined as optional. Devices which require the property will have >> to assert this in their DT bindings. >> >> 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: devicetree@vger.kernel.org >> --- >> Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt >> index 747c538..21a25e4 100644 >> --- a/Documentation/devicetree/bindings/leds/common.txt >> +++ b/Documentation/devicetree/bindings/leds/common.txt >> @@ -29,13 +29,15 @@ 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 >> +- max-microamp : Maximum intensity in microamperes of the LED >> + (torch LED for flash devices). If omitted this will default >> + to the maximum current allowed by the device. >> +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. >> + If omitted this will default to the maximum >> + current allowed by the device. >> +- flash-timeout-us : Timeout in microseconds after which the flash >> + LED is turned off. If omitted this will default to the >> + maximum timeout allowed by the device. >> >> >> Examples: > > Pavel pointed out that the brightness between maximum current and the > maximum *allowed* another current might not be noticeable, leading a > potential spelling error to cause the LED being run at too high current. I think that a board designed so that it can be damaged because of software bugs should be considered not eligible for commercial use. Any self-esteeming manufacturer will not connect a LED to the output that can produce the current greater than the LED's absolute maximum current. The DT properties could be useful for devices like aat1290 device I was writing a driver for, which has the maximum current and timeout values depending on corresponding capacitor and resistor values respectively. Such devices should make the properties required in their bindings. > The three drivers I've looked also require these properties, which I think > is in the line with the above. > > How about either dropping the patch, or changing maximum to minimum and > will to should? The drivers could also behave this way instead of requiring > the properties, but I don't think there's anything wrong with requiring the > properties either. As I mentioned in the previous message in this subject, the max-microamp property refers also to non-flash LEDs. Since existing LED class devices does not require them, then it should be left optional and default to max. It would however be inconsistent with flash LEDs related properties. > I think this is worth considering now as we can't change this later without > breaking something. >
Hi Jacek, On Wed, Apr 08, 2015 at 10:54:52AM +0200, Jacek Anaszewski wrote: > Hi Sakari, > > On 04/03/2015 02:09 PM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Tue, Mar 31, 2015 at 03:52:37PM +0200, Jacek Anaszewski wrote: > >>Description of flash LEDs related properties was not precise regarding > >>the state of corresponding settings in case a property is missing. > >>Add relevant statements. > >>Removed is also the requirement making the flash-max-microamp > >>property obligatory for flash LEDs. It was inconsistent as the property > >>is defined as optional. Devices which require the property will have > >>to assert this in their DT bindings. > >> > >>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: devicetree@vger.kernel.org > >>--- > >> Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++------- > >> 1 file changed, 9 insertions(+), 7 deletions(-) > >> > >>diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > >>index 747c538..21a25e4 100644 > >>--- a/Documentation/devicetree/bindings/leds/common.txt > >>+++ b/Documentation/devicetree/bindings/leds/common.txt > >>@@ -29,13 +29,15 @@ 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 > >>+- max-microamp : Maximum intensity in microamperes of the LED > >>+ (torch LED for flash devices). If omitted this will default > >>+ to the maximum current allowed by the device. > >>+- flash-max-microamp : Maximum intensity in microamperes of the flash LED. > >>+ If omitted this will default to the maximum > >>+ current allowed by the device. > >>+- flash-timeout-us : Timeout in microseconds after which the flash > >>+ LED is turned off. If omitted this will default to the > >>+ maximum timeout allowed by the device. > >> > >> > >> Examples: > > > >Pavel pointed out that the brightness between maximum current and the > >maximum *allowed* another current might not be noticeable, leading a > >potential spelling error to cause the LED being run at too high current. > > I think that a board designed so that it can be damaged because of > software bugs should be considered not eligible for commercial use. > Any self-esteeming manufacturer will not connect a LED to the output > that can produce the current greater than the LED's absolute maximum > current. The maximum current *is* used to prevent potential hardware damage. This is how mobile phones typically are, probably also the one you're using. :-) I don't believe there's really a difference between vendors in this respect. We still lack a proper way to model the temperature of the flash LED, so what we have now is a bit incomplete, but at least it prevents causing damage unintentionally. > The DT properties could be useful for devices like aat1290 device I was > writing a driver for, which has the maximum current and timeout values > depending on corresponding capacitor and resistor values respectively. > Such devices should make the properties required in their bindings. > > >The three drivers I've looked also require these properties, which I think > >is in the line with the above. > > > >How about either dropping the patch, or changing maximum to minimum and > >will to should? The drivers could also behave this way instead of requiring > >the properties, but I don't think there's anything wrong with requiring the > >properties either. > > As I mentioned in the previous message in this subject, the max-microamp > property refers also to non-flash LEDs. Since existing LED class devices > does not require them, then it should be left optional and default to > max. It would however be inconsistent with flash LEDs related > properties. I do agree with Pavel here, these should be mandatory (at least for new drivers) OR default to minimum.
On Tue 2015-04-07 18:20:08, Bryan Wu wrote: > On Fri, Apr 3, 2015 at 1:37 PM, Pavel Machek <pavel@ucw.cz> wrote: > > Hi! > > > >> >>+- flash-timeout-us : Timeout in microseconds after which the flash > >> >>+ LED is turned off. If omitted this will default to the > >> >>+ maximum timeout allowed by the device. > >> >> > >> >> > >> >> Examples: > >> > > >> >Pavel pointed out that the brightness between maximum current and the > >> >maximum *allowed* another current might not be noticeable,leading a > >> >potential spelling error to cause the LED being run at too high current. > >> > >> Where did he point this out? Do you think about the current version > >> of the leds/common.txt documentation or there was some other message, > >> that I don't see? > > > > Date: Thu, 2 Apr 2015 22:30:44 +0200 > > From: Pavel Machek <pavel@ucw.cz> > > To: Sakari Ailus <sakari.ailus@iki.fi> > > Subject: Re: [PATCHv3] media: i2c/adp1653: devicetree support for adp1653 > > > >> Besides, I can't understand your point. Could you express it in other > >> words, please? > > > > Typo in device tree would cause hardware damage. But idea. Make the > > properties mandatory. > > Pavel > > I don't quite follow there. I think Pavel acked this patch right? So > what's left to hold here? Yeah. Then I realized that patch is wrong/dangerous. Sorry about that. Try to forget about my ACK if you can ;-). Pavel
Hi! > > I think that a board designed so that it can be damaged because of > > software bugs should be considered not eligible for commercial > > use. Hello? It is 2015. Yes, that was nice rule... in 1995 or so :-). > > As I mentioned in the previous message in this subject, the max-microamp > > property refers also to non-flash LEDs. Since existing LED class devices > > does not require them, then it should be left optional and default to > > max. It would however be inconsistent with flash LEDs related > > properties. For non-flash LEDs and backward compatibility, I guess you are right. Inconsistency is fine in this case... Pavel
Hello, On 31/03/15 15:52, Jacek Anaszewski wrote: > Description of flash LEDs related properties was not precise regarding > the state of corresponding settings in case a property is missing. > Add relevant statements. > Removed is also the requirement making the flash-max-microamp > property obligatory for flash LEDs. It was inconsistent as the property > is defined as optional. Devices which require the property will have > to assert this in their DT bindings. > > 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: devicetree@vger.kernel.org > --- > Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > index 747c538..21a25e4 100644 > --- a/Documentation/devicetree/bindings/leds/common.txt > +++ b/Documentation/devicetree/bindings/leds/common.txt > @@ -29,13 +29,15 @@ 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 > +- max-microamp : Maximum intensity in microamperes of the LED > + (torch LED for flash devices). If omitted this will default > + to the maximum current allowed by the device. > +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. > + If omitted this will default to the maximum > + current allowed by the device. > +- flash-timeout-us : Timeout in microseconds after which the flash > + LED is turned off. If omitted this will default to the > + maximum timeout allowed by the device. Sorry about late comments on that, but since we can still change these properties and it seems we're going to do that, I'd like throw in my few preferences on the colour of this bike... IMO "max-microamp" is a poor property name, how about: s/max-microamp/led-max-current-ua, s/flash-max-microamp/flash-max-current-ua, so we have more consistent set of properties like: led-max-current-ua flash-max-current-ua flash-timeout-us Also expressing light intensity in micro-amperes seems technically wrong. I would propose to substitute word "intensity in microamperes" with "LED supply current in microamperes". I also think we should require the maximum current properties and the driver should warn if they are missing and limit current to some potentially safe value, e.g. small fraction of the maximum current. Also from the description it should be clear whether the current limits refer to capabilities of a LED or the desired settings we want to be applied at the LED driver device. We could, for example, add a sentence after the above 3 properties: "Required properties for Flash LEDs: - led-max-current-ua - flash-max-current-ua - flash-timeout-us These properties determine a LED driver IC settings required for safe operation." Or something along these lines.
Hi Sakari, On 04/08/2015 11:11 AM, Sakari Ailus wrote: > Hi Jacek, > > On Wed, Apr 08, 2015 at 10:54:52AM +0200, Jacek Anaszewski wrote: >> Hi Sakari, >> >> On 04/03/2015 02:09 PM, Sakari Ailus wrote: >>> Hi Jacek, >>> >>> On Tue, Mar 31, 2015 at 03:52:37PM +0200, Jacek Anaszewski wrote: >>>> Description of flash LEDs related properties was not precise regarding >>>> the state of corresponding settings in case a property is missing. >>>> Add relevant statements. >>>> Removed is also the requirement making the flash-max-microamp >>>> property obligatory for flash LEDs. It was inconsistent as the property >>>> is defined as optional. Devices which require the property will have >>>> to assert this in their DT bindings. >>>> >>>> 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: devicetree@vger.kernel.org >>>> --- >>>> Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++------- >>>> 1 file changed, 9 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt >>>> index 747c538..21a25e4 100644 >>>> --- a/Documentation/devicetree/bindings/leds/common.txt >>>> +++ b/Documentation/devicetree/bindings/leds/common.txt >>>> @@ -29,13 +29,15 @@ 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 >>>> +- max-microamp : Maximum intensity in microamperes of the LED >>>> + (torch LED for flash devices). If omitted this will default >>>> + to the maximum current allowed by the device. >>>> +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. >>>> + If omitted this will default to the maximum >>>> + current allowed by the device. >>>> +- flash-timeout-us : Timeout in microseconds after which the flash >>>> + LED is turned off. If omitted this will default to the >>>> + maximum timeout allowed by the device. >>>> >>>> >>>> Examples: >>> >>> Pavel pointed out that the brightness between maximum current and the >>> maximum *allowed* another current might not be noticeable, leading a >>> potential spelling error to cause the LED being run at too high current. >> >> I think that a board designed so that it can be damaged because of >> software bugs should be considered not eligible for commercial use. >> Any self-esteeming manufacturer will not connect a LED to the output >> that can produce the current greater than the LED's absolute maximum >> current. > > The maximum current *is* used to prevent potential hardware damage. What hardware are we talking about - LED controller or the discrete LED component attached to the LED controller's current output? The maximum current the LED controller can produce is fixed or depends on external components like resistors. This is at least the case for max77693 and aat1290 device I've been working with. If a LED is rated to max 1A and it will be connected to the output capable of producing 1.2A then it is likely that it will be damaged. I was thinking about this kind of hardware damage. How vendors can protect from it if they connect incompatible LED to the current output? There might be boards that provide sockets for connecting external LEDs though. Such arrangements indeed justify the need for making the properties required. > This is > how mobile phones typically are, probably also the one you're using. :-) I > don't believe there's really a difference between vendors in this respect. > > We still lack a proper way to model the temperature of the flash LED, so > what we have now is a bit incomplete, but at least it prevents causing > damage unintentionally. Are you thinking about flash faults? >> The DT properties could be useful for devices like aat1290 device I was >> writing a driver for, which has the maximum current and timeout values >> depending on corresponding capacitor and resistor values respectively. >> Such devices should make the properties required in their bindings. >> >>> The three drivers I've looked also require these properties, which I think >>> is in the line with the above. >>> >>> How about either dropping the patch, or changing maximum to minimum and >>> will to should? The drivers could also behave this way instead of requiring >>> the properties, but I don't think there's anything wrong with requiring the >>> properties either. >> >> As I mentioned in the previous message in this subject, the max-microamp >> property refers also to non-flash LEDs. Since existing LED class devices >> does not require them, then it should be left optional and default to >> max. It would however be inconsistent with flash LEDs related >> properties. > > I do agree with Pavel here, these should be mandatory (at least for new > drivers) OR default to minimum. >
On Wed 2015-04-08 12:03:27, Sylwester Nawrocki wrote: > Hello, > > On 31/03/15 15:52, Jacek Anaszewski wrote: > > Description of flash LEDs related properties was not precise regarding > > the state of corresponding settings in case a property is missing. > > Add relevant statements. > > Removed is also the requirement making the flash-max-microamp > > property obligatory for flash LEDs. It was inconsistent as the property > > is defined as optional. Devices which require the property will have > > to assert this in their DT bindings. > > > > 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: devicetree@vger.kernel.org > > --- > > Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++------- > > 1 file changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > > index 747c538..21a25e4 100644 > > --- a/Documentation/devicetree/bindings/leds/common.txt > > +++ b/Documentation/devicetree/bindings/leds/common.txt > > @@ -29,13 +29,15 @@ 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 > > +- max-microamp : Maximum intensity in microamperes of the LED > > + (torch LED for flash devices). If omitted this will default > > + to the maximum current allowed by the device. > > +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. > > + If omitted this will default to the maximum > > + current allowed by the device. > > +- flash-timeout-us : Timeout in microseconds after which the flash > > + LED is turned off. If omitted this will default to the > > + maximum timeout allowed by the device. > > Sorry about late comments on that, but since we can still change these > properties and it seems we're going to do that, I'd like throw in my > few preferences on the colour of this bike... Lets not paint bikes here.
Hi Jacek, On Wed, Apr 08, 2015 at 12:23:23PM +0200, Jacek Anaszewski wrote: > Hi Sakari, > > On 04/08/2015 11:11 AM, Sakari Ailus wrote: > >Hi Jacek, > > > >On Wed, Apr 08, 2015 at 10:54:52AM +0200, Jacek Anaszewski wrote: > >>Hi Sakari, > >> > >>On 04/03/2015 02:09 PM, Sakari Ailus wrote: > >>>Hi Jacek, > >>> > >>>On Tue, Mar 31, 2015 at 03:52:37PM +0200, Jacek Anaszewski wrote: > >>>>Description of flash LEDs related properties was not precise regarding > >>>>the state of corresponding settings in case a property is missing. > >>>>Add relevant statements. > >>>>Removed is also the requirement making the flash-max-microamp > >>>>property obligatory for flash LEDs. It was inconsistent as the property > >>>>is defined as optional. Devices which require the property will have > >>>>to assert this in their DT bindings. > >>>> > >>>>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: devicetree@vger.kernel.org > >>>>--- > >>>> Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++------- > >>>> 1 file changed, 9 insertions(+), 7 deletions(-) > >>>> > >>>>diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt > >>>>index 747c538..21a25e4 100644 > >>>>--- a/Documentation/devicetree/bindings/leds/common.txt > >>>>+++ b/Documentation/devicetree/bindings/leds/common.txt > >>>>@@ -29,13 +29,15 @@ 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 > >>>>+- max-microamp : Maximum intensity in microamperes of the LED > >>>>+ (torch LED for flash devices). If omitted this will default > >>>>+ to the maximum current allowed by the device. > >>>>+- flash-max-microamp : Maximum intensity in microamperes of the flash LED. > >>>>+ If omitted this will default to the maximum > >>>>+ current allowed by the device. > >>>>+- flash-timeout-us : Timeout in microseconds after which the flash > >>>>+ LED is turned off. If omitted this will default to the > >>>>+ maximum timeout allowed by the device. > >>>> > >>>> > >>>> Examples: > >>> > >>>Pavel pointed out that the brightness between maximum current and the > >>>maximum *allowed* another current might not be noticeable, leading a > >>>potential spelling error to cause the LED being run at too high current. > >> > >>I think that a board designed so that it can be damaged because of > >>software bugs should be considered not eligible for commercial use. > >>Any self-esteeming manufacturer will not connect a LED to the output > >>that can produce the current greater than the LED's absolute maximum > >>current. > > > >The maximum current *is* used to prevent potential hardware damage. > > What hardware are we talking about - LED controller or the discrete > LED component attached to the LED controller's current output? Generally the LED itself, not the controller. Most controllers have overheating protection while the LEDs do not. > > The maximum current the LED controller can produce is fixed or depends > on external components like resistors. On some controllers perhaps, but not on most of them. > > This is at least the case for max77693 and aat1290 device I've been > working with. If a LED is rated to max 1A and it will be connected > to the output capable of producing 1.2A then it is likely that it > will be damaged. I was thinking about this kind of hardware damage. This is the very reason why the maximum current limit is there: to prevent hardware damage. If the LED could safely be used at the controller's maximum current, there would be no need for the maximum current property (torch/flash). > > How vendors can protect from it if they connect incompatible LED > to the current output? > > There might be boards that provide sockets for connecting external > LEDs though. Such arrangements indeed justify the need for making the > properties required. Pluggable hardware is a completely different matter. If used with DT overlays, the overlay should contain the limits as well. > > >This is > >how mobile phones typically are, probably also the one you're using. :-) I > >don't believe there's really a difference between vendors in this respect. > > > >We still lack a proper way to model the temperature of the flash LED, so > >what we have now is a bit incomplete, but at least it prevents causing > >damage unintentionally. > > Are you thinking about flash faults? The question is: when if it safe to strobe again after a strobe has ended?
Hi Sylwester, On 04/08/2015 12:03 PM, Sylwester Nawrocki wrote: > Hello, > > On 31/03/15 15:52, Jacek Anaszewski wrote: >> Description of flash LEDs related properties was not precise regarding >> the state of corresponding settings in case a property is missing. >> Add relevant statements. >> Removed is also the requirement making the flash-max-microamp >> property obligatory for flash LEDs. It was inconsistent as the property >> is defined as optional. Devices which require the property will have >> to assert this in their DT bindings. >> >> 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: devicetree@vger.kernel.org >> --- >> Documentation/devicetree/bindings/leds/common.txt | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt >> index 747c538..21a25e4 100644 >> --- a/Documentation/devicetree/bindings/leds/common.txt >> +++ b/Documentation/devicetree/bindings/leds/common.txt >> @@ -29,13 +29,15 @@ 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 >> +- max-microamp : Maximum intensity in microamperes of the LED >> + (torch LED for flash devices). If omitted this will default >> + to the maximum current allowed by the device. >> +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. >> + If omitted this will default to the maximum >> + current allowed by the device. >> +- flash-timeout-us : Timeout in microseconds after which the flash >> + LED is turned off. If omitted this will default to the >> + maximum timeout allowed by the device. > > Sorry about late comments on that, but since we can still change these > properties and it seems we're going to do that, I'd like throw in my > few preferences on the colour of this bike... > > IMO "max-microamp" is a poor property name, how about: > > s/max-microamp/led-max-current-ua, > s/flash-max-microamp/flash-max-current-ua, > > so we have more consistent set of properties like: > > led-max-current-ua > flash-max-current-ua > flash-timeout-us The "-microamp' suffix is consistent with regulator bindings. Please refer to [1]. > Also expressing light intensity in micro-amperes seems technically wrong. > I would propose to substitute word "intensity in microamperes" with "LED > supply current in microamperes". OK, I will address this in the next version of the patch. > I also think we should require the maximum current properties and > the driver should warn if they are missing and limit current to some > potentially safe value, e.g. small fraction of the maximum current. TBD. > Also from the description it should be clear whether the current > limits refer to capabilities of a LED or the desired settings we want > to be applied at the LED driver device. > We could, for example, add a sentence after the above 3 properties: > > "Required properties for Flash LEDs: > > - led-max-current-ua > - flash-max-current-ua > - flash-timeout-us > > These properties determine a LED driver IC settings required for > safe operation." > > Or something along these lines. OK. [1] http://www.spinics.net/lists/linux-leds/msg02674.html
Hi Jacek, On 08/04/15 13:58, Jacek Anaszewski wrote: >>> --- a/Documentation/devicetree/bindings/leds/common.txt >>> >> +++ b/Documentation/devicetree/bindings/leds/common.txt >>> >> @@ -29,13 +29,15 @@ 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 >>> >> +- max-microamp : Maximum intensity in microamperes of the LED >>> >> + (torch LED for flash devices). If omitted this will default >>> >> + to the maximum current allowed by the device. >>> >> +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. >>> >> + If omitted this will default to the maximum >>> >> + current allowed by the device. >>> >> +- flash-timeout-us : Timeout in microseconds after which the flash >>> >> + LED is turned off. If omitted this will default to the >>> >> + maximum timeout allowed by the device. >> > >> > Sorry about late comments on that, but since we can still change these >> > properties and it seems we're going to do that, I'd like throw in my >> > few preferences on the colour of this bike... >> > >> > IMO "max-microamp" is a poor property name, how about: >> > >> > s/max-microamp/led-max-current-ua, >> > s/flash-max-microamp/flash-max-current-ua, >> > >> > so we have more consistent set of properties like: >> > >> > led-max-current-ua >> > flash-max-current-ua >> > flash-timeout-us > > The "-microamp' suffix is consistent with regulator bindings. > Please refer to [1]. OK, in a perfect world we would have clean and consistent notation of units. If it's acked let's leave it, I didn't know it was, sorry about that. When I read yesterday Documentation/devicetree/bindings/leds/common.txt the set of new properties looked rather sloppy, especially "max-microamp" looked incomplete to me, as if the subject was missing. Anyway, I'll just get used to it, let's complete this whole Flash/LED integration story.
diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt index 747c538..21a25e4 100644 --- a/Documentation/devicetree/bindings/leds/common.txt +++ b/Documentation/devicetree/bindings/leds/common.txt @@ -29,13 +29,15 @@ 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 +- max-microamp : Maximum intensity in microamperes of the LED + (torch LED for flash devices). If omitted this will default + to the maximum current allowed by the device. +- flash-max-microamp : Maximum intensity in microamperes of the flash LED. + If omitted this will default to the maximum + current allowed by the device. +- flash-timeout-us : Timeout in microseconds after which the flash + LED is turned off. If omitted this will default to the + maximum timeout allowed by the device. Examples: