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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
