[PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Jacek Anaszewski]

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:
-- 
1.7.9.5

[PATCH v4 04/12] DT: Add documentation for the mfd Maxim max77693

[Jacek Anaszewski]

This patch adds device tree binding documentation for
the flash cell of the Maxim max77693 multifunctional device.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Chanwoo Choi <cw00.choi@samsung.com>
Cc: Bryan Wu <cooloney@gmail.com>
Cc: Richard Purdie <rpurdie@rpsys.net>
Cc: devicetree@vger.kernel.org
---
 Documentation/devicetree/bindings/mfd/max77693.txt |   61 ++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt b/Documentation/devicetree/bindings/mfd/max77693.txt
index 38e6440..deb832f 100644
--- a/Documentation/devicetree/bindings/mfd/max77693.txt
+++ b/Documentation/devicetree/bindings/mfd/max77693.txt
@@ -76,7 +76,54 @@ Optional properties:
     Valid values: 4300000, 4700000, 4800000, 4900000
     Default: 4300000
 
+- led : the LED submodule device node
+
+There are two LED outputs available - FLED1 and FLED2. Each of them can
+control a separate LED or they can be connected together to double
+the maximum current for a single connected LED. One LED is represented
+by one child node.
+
+Required properties:
+- compatible : Must be "maxim,max77693-led".
+
+Optional properties:
+- maxim,boost-mode :
+	In boost mode the device can produce up to 1.2A of total current
+	on both outputs. The maximum current on each output is reduced
+	to 625mA then. If not enabled explicitly, boost setting defaults to
+	LEDS_BOOST_FIXED in case both current sources are used.
+	Possible values:
+		LEDS_BOOST_OFF (0) - no boost,
+		LEDS_BOOST_ADAPTIVE (1) - adaptive mode,
+		LEDS_BOOST_FIXED (2) - fixed mode.
+- maxim,boost-mvout : Output voltage of the boost module in millivolts.
+	Valid values: 3300 - 5500, step by 25 (rounded down)
+	Default: 3300
+- maxim,mvsys-min : Low input voltage level in millivolts. Flash is not fired
+	if chip estimates that system voltage could drop below this level due
+	to flash power consumption.
+	Valid values: 2400 - 3400, step by 33 (rounded down)
+	Default: 2400
+
+Required properties of the LED child node:
+- led-sources : see Documentation/devicetree/bindings/leds/common.txt;
+		device current output identifiers: 0 - FLED1, 1 - FLED2
+
+Optional properties of the LED child node:
+- label : see Documentation/devicetree/bindings/leds/common.txt
+- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+	Valid values: 15625 - 250000, step by 15625 (rounded down)
+	Default: 250000
+- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
+	Valid values: 15625 - 1000000, step by 15625 (rounded down)
+	Default: 1000000 when boost mode is off and 625000 otherwise
+- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
+	Valid values: 62500 - 1000000, step by 62500 (rounded down)
+	Default: 1000000
+
 Example:
+#include <dt-bindings/leds/common.h>
+
 	max77693@66 {
 		compatible = "maxim,max77693";
 		reg = <0x66>;
@@ -117,5 +164,19 @@ Example:
 			maxim,thermal-regulation-celsius = <75>;
 			maxim,battery-overcurrent-microamp = <3000000>;
 			maxim,charge-input-threshold-microvolt = <4300000>;
+
+		led {
+			compatible = "maxim,max77693-led";
+			maxim,boost-mode = <LEDS_BOOST_FIXED>;
+			maxim,boost-mvout = <5000>;
+			maxim,mvsys-min = <2400>;
+
+			camera_flash: flash-led {
+				label = "max77693-flash";
+				led-sources = <0>, <1>;
+				max-microamp = <500000>;
+				flash-max-microamp = <1250000>;
+				flash-timeout-us = <1000000>;
+			};
 		};
 	};
-- 
1.7.9.5

[PATCH v4 06/12] of: Add Skyworks Solutions, Inc. vendor prefix

[Jacek Anaszewski]

Use "skyworks" as the vendor prefix for the Skyworks Solutions, Inc.

Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: devicetree@vger.kernel.org
---
 .../devicetree/bindings/vendor-prefixes.txt        |    1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 42b3dab..4cd18bb 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -163,6 +163,7 @@ ricoh	Ricoh Co. Ltd.
 rockchip	Fuzhou Rockchip Electronics Co., Ltd
 samsung	Samsung Semiconductor
 sandisk	Sandisk Corporation
+skyworks	Skyworks Solutions, Inc.
 sbs	Smart Battery System
 schindler	Schindler
 seagate	Seagate Technology PLC
-- 
1.7.9.5

[PATCH v4 07/12] DT: Add documentation for the Skyworks AAT1290

[Jacek Anaszewski]

This patch adds device tree binding documentation for
1.5A Step-Up Current Regulator for Flash LEDs.

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: devicetree@vger.kernel.org
---
 .../devicetree/bindings/leds/leds-aat1290.txt      |   40 ++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-aat1290.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-aat1290.txt b/Documentation/devicetree/bindings/leds/leds-aat1290.txt
new file mode 100644
index 0000000..6893eb1
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-aat1290.txt
@@ -0,0 +1,40 @@
+* Skyworks Solutions, Inc. AAT1290 Current Regulator for Flash LEDs
+
+The device is controlled through two pins: FL_EN and EN_SET. The pins when,
+asserted high, enable flash strobe and movie mode (max 1/2 of flash current)
+respectively.
+
+Required properties:
+
+- compatible : Must be "skyworks,aat1290".
+- flen-gpios : Must be device tree identifier of the flash device FL_EN pin.
+- enset-gpios : Must be device tree identifier of the flash device EN_SET pin.
+
+A discrete LED element connected to the device must be represented by a child
+node - see Documentation/devicetree/bindings/leds/common.txt.
+
+Required properties of the LED child node:
+- flash-max-microamp : Maximum intensity in microamperes of the flash LED -
+		       it can be calculated using following formula:
+		       I = 1A * 162kohm / Rset.
+- flash-timeout-us : Maximum flash timeout in microseconds -
+		     it can be calculated using following formula:
+		     T = 8.82 * 10^9 * Ct.
+
+Optional properties of the LED child node:
+- label : see Documentation/devicetree/bindings/leds/common.txt
+
+Example (by Ct = 220nF, Rset = 160kohm and exynos4412-trats2 board with
+a switch that allows for routing strobe signal either from host or from ISP):
+
+aat1290 {
+	compatible = "skyworks,aat1290";
+	flen-gpios = <&gpj1 1 GPIO_ACTIVE_HIGH>;
+	enset-gpios = <&gpj1 2 GPIO_ACTIVE_HIGH>;
+
+	camera_flash: flash-led {
+		label = "aat1290-flash";
+		flash-max-microamp = <1012500>;
+		flash-timeout-us = <1940000>;
+	};
+};
-- 
1.7.9.5

Re: [PATCH v4 04/12] DT: Add documentation for the mfd Maxim max77693

[Jacek Anaszewski]

I've just pushed PATCH v5 - it adds more precise description
of current settings constraints.

On 03/31/2015 03:52 PM, Jacek Anaszewski wrote:
> This patch adds device tree binding documentation for
> the flash cell of the Maxim max77693 multifunctional device.
>
> Signed-off-by: Jacek Anaszewski <j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Andrzej Hajda <a.hajda-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Cc: Lee Jones <lee.jones-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Chanwoo Choi <cw00.choi-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Cc: Bryan Wu <cooloney-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> ---
>   Documentation/devicetree/bindings/mfd/max77693.txt |   61 ++++++++++++++++++++
>   1 file changed, 61 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mfd/max77693.txt b/Documentation/devicetree/bindings/mfd/max77693.txt
> index 38e6440..deb832f 100644
> --- a/Documentation/devicetree/bindings/mfd/max77693.txt
> +++ b/Documentation/devicetree/bindings/mfd/max77693.txt
> @@ -76,7 +76,54 @@ Optional properties:
>       Valid values: 4300000, 4700000, 4800000, 4900000
>       Default: 4300000
>
> +- led : the LED submodule device node
> +
> +There are two LED outputs available - FLED1 and FLED2. Each of them can
> +control a separate LED or they can be connected together to double
> +the maximum current for a single connected LED. One LED is represented
> +by one child node.
> +
> +Required properties:
> +- compatible : Must be "maxim,max77693-led".
> +
> +Optional properties:
> +- maxim,boost-mode :
> +	In boost mode the device can produce up to 1.2A of total current
> +	on both outputs. The maximum current on each output is reduced
> +	to 625mA then. If not enabled explicitly, boost setting defaults to
> +	LEDS_BOOST_FIXED in case both current sources are used.
> +	Possible values:
> +		LEDS_BOOST_OFF (0) - no boost,
> +		LEDS_BOOST_ADAPTIVE (1) - adaptive mode,
> +		LEDS_BOOST_FIXED (2) - fixed mode.
> +- maxim,boost-mvout : Output voltage of the boost module in millivolts.
> +	Valid values: 3300 - 5500, step by 25 (rounded down)
> +	Default: 3300
> +- maxim,mvsys-min : Low input voltage level in millivolts. Flash is not fired
> +	if chip estimates that system voltage could drop below this level due
> +	to flash power consumption.
> +	Valid values: 2400 - 3400, step by 33 (rounded down)
> +	Default: 2400
> +
> +Required properties of the LED child node:
> +- led-sources : see Documentation/devicetree/bindings/leds/common.txt;
> +		device current output identifiers: 0 - FLED1, 1 - FLED2
> +
> +Optional properties of the LED child node:
> +- label : see Documentation/devicetree/bindings/leds/common.txt
> +- max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +	Valid values: 15625 - 250000, step by 15625 (rounded down)
> +	Default: 250000
> +- flash-max-microamp : see Documentation/devicetree/bindings/leds/common.txt
> +	Valid values: 15625 - 1000000, step by 15625 (rounded down)
> +	Default: 1000000 when boost mode is off and 625000 otherwise
> +- flash-timeout-us : see Documentation/devicetree/bindings/leds/common.txt
> +	Valid values: 62500 - 1000000, step by 62500 (rounded down)
> +	Default: 1000000
> +
>   Example:
> +#include <dt-bindings/leds/common.h>
> +
>   	max77693@66 {
>   		compatible = "maxim,max77693";
>   		reg = <0x66>;
> @@ -117,5 +164,19 @@ Example:
>   			maxim,thermal-regulation-celsius = <75>;
>   			maxim,battery-overcurrent-microamp = <3000000>;
>   			maxim,charge-input-threshold-microvolt = <4300000>;
> +
> +		led {
> +			compatible = "maxim,max77693-led";
> +			maxim,boost-mode = <LEDS_BOOST_FIXED>;
> +			maxim,boost-mvout = <5000>;
> +			maxim,mvsys-min = <2400>;
> +
> +			camera_flash: flash-led {
> +				label = "max77693-flash";
> +				led-sources = <0>, <1>;
> +				max-microamp = <500000>;
> +				flash-max-microamp = <1250000>;
> +				flash-timeout-us = <1000000>;
> +			};
>   		};
>   	};
>


-- 
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Pavel Machek]

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.


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Sakari Ailus]

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.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Jacek Anaszewski]

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.
>

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Bryan Wu]

On Fri, Apr 3, 2015 at 1:37 PM, Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org> 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-+ZI9xUNit7I@public.gmane.org>
> To: Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Sakari Ailus]

Hi Bryan,

Bryan Wu wrote:
> On Fri, Apr 3, 2015 at 1:37 PM, Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org> 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-+ZI9xUNit7I@public.gmane.org>
>> To: Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>
>> 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.

-- 
Regards,

Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Jacek Anaszewski]

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.
>

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Sakari Ailus]

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.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Sylwester Nawrocki]

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.

-- 
Regards,
Sylwester

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Jacek Anaszewski]

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.
>


-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Pavel Machek]

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.

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Sakari Ailus]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >>>>Acked-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> >>>>Cc: Bryan Wu <cooloney-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >>>>Cc: Richard Purdie <rpurdie-Fm38FmjxZ/leoWH0uzbU5w@public.gmane.org>
> >>>>Cc: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>
> >>>>Cc: Sakari Ailus <sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >>>>Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.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?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus-X3B1VOXEql0@public.gmane.org	XMPP: sailus-PCDdDYkjdNMDXYZnReoRVg@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Jacek Anaszewski]

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

-- 
Best Regards,
Jacek Anaszewski

Re: [PATCH v4 06/12] of: Add Skyworks Solutions, Inc. vendor prefix

[Sakari Ailus]

On Tue, Mar 31, 2015 at 03:52:42PM +0200, Jacek Anaszewski wrote:
> Use "skyworks" as the vendor prefix for the Skyworks Solutions, Inc.
> 
> Signed-off-by: Jacek Anaszewski <j.anaszewski@samsung.com>
> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: devicetree@vger.kernel.org
> ---
>  .../devicetree/bindings/vendor-prefixes.txt        |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
> index 42b3dab..4cd18bb 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.txt
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
> @@ -163,6 +163,7 @@ ricoh	Ricoh Co. Ltd.
>  rockchip	Fuzhou Rockchip Electronics Co., Ltd
>  samsung	Samsung Semiconductor
>  sandisk	Sandisk Corporation
> +skyworks	Skyworks Solutions, Inc.

Please maintain the alphabetic order. With that fixed,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

>  sbs	Smart Battery System
>  schindler	Schindler
>  seagate	Seagate Technology PLC

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [PATCH v4 01/12] DT: leds: Improve description of flash LEDs related properties

[Sylwester Nawrocki]

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.

-- 
Thanks,
Sylwester