[2/4] ARM: dts: omap4-droid4: Update backlight dt properties
diff mbox series

Message ID 20190307220947.20057-2-dmurphy@ti.com
State Superseded
Headers show
Series
  • [1/4] dt: lm3532: Add lm3532 dt doc and update ti_lmu doc
Related show

Commit Message

Dan Murphy March 7, 2019, 10:09 p.m. UTC
Update the properties for the lm3532 device node for droid4.
With this change the backlight LED string and the keypad
LED strings will be controlled separately.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 arch/arm/boot/dts/omap4-droid4-xt894.dts | 27 +++++++++++++++++-------
 1 file changed, 19 insertions(+), 8 deletions(-)

Comments

Pavel Machek March 8, 2019, 3:14 p.m. UTC | #1
Hi!

> Update the properties for the lm3532 device node for droid4.
> With this change the backlight LED string and the keypad
> LED strings will be controlled separately.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
>  arch/arm/boot/dts/omap4-droid4-xt894.dts | 27 +++++++++++++++++-------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts
> index e21ec929f096..94e3d53dbcf3 100644
> --- a/arch/arm/boot/dts/omap4-droid4-xt894.dts
> +++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts
> @@ -6,6 +6,7 @@
>  /dts-v1/;
>  
>  #include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/leds-lm3532.h>
>  #include "omap443x.dtsi"
>  #include "motorola-cpcap-mapphone.dtsi"
>  
> @@ -383,20 +384,30 @@
>  };
>  
>  &i2c1 {
> -	lm3532@38 {
> +	led-controller@38 {
>  		compatible = "ti,lm3532";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
>  		reg = <0x38>;
>  
>  		enable-gpios = <&gpio6 12 GPIO_ACTIVE_HIGH>;
>  
> -		lcd_backlight: backlight {
> -			compatible = "ti,lm3532-backlight";
> +		ramp-up-ms = <LM3532_RAMP_1024us>;
> +		ramp-down-ms = <LM3532_RAMP_65536us>;

I guess dt people will have some comments here. I'd expect

ramp-up-us = <1024> would be more natural.

> +		lcd_backlight: led@0 {
> +			reg = <0>;
> +			led-sources = <2>;
> +			ti,led-mode = <LM3532_BL_MODE_MANUAL>;
> +			label = "backlight";

Ok, so we'll have lm3532::backlight. That is not too useful, as it
does not tell userland what kaclight it is.

main_display::backlight ?

OTOH this one is not too important as backlight subsystem should
handle this.

> +		led@1 {
> +			reg = <1>;
> +			led-sources = <1>;
> +			ti,led-mode = <LM3532_BL_MODE_MANUAL>;
> +			label = "keypad";

I guess best variant would be inputX::backlight here, but that might
be tricky to implement.

									Pavel
Dan Murphy March 8, 2019, 3:45 p.m. UTC | #2
Pavel

Thanks for the review.

On 3/8/19 9:14 AM, Pavel Machek wrote:
> Hi!
> 
>> Update the properties for the lm3532 device node for droid4.
>> With this change the backlight LED string and the keypad
>> LED strings will be controlled separately.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>  arch/arm/boot/dts/omap4-droid4-xt894.dts | 27 +++++++++++++++++-------
>>  1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts
>> index e21ec929f096..94e3d53dbcf3 100644
>> --- a/arch/arm/boot/dts/omap4-droid4-xt894.dts
>> +++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts
>> @@ -6,6 +6,7 @@
>>  /dts-v1/;
>>  
>>  #include <dt-bindings/input/input.h>
>> +#include <dt-bindings/leds/leds-lm3532.h>
>>  #include "omap443x.dtsi"
>>  #include "motorola-cpcap-mapphone.dtsi"
>>  
>> @@ -383,20 +384,30 @@
>>  };
>>  
>>  &i2c1 {
>> -	lm3532@38 {
>> +	led-controller@38 {
>>  		compatible = "ti,lm3532";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>>  		reg = <0x38>;
>>  
>>  		enable-gpios = <&gpio6 12 GPIO_ACTIVE_HIGH>;
>>  
>> -		lcd_backlight: backlight {
>> -			compatible = "ti,lm3532-backlight";
>> +		ramp-up-ms = <LM3532_RAMP_1024us>;
>> +		ramp-down-ms = <LM3532_RAMP_65536us>;
> 
> I guess dt people will have some comments here. I'd expect
> 
> ramp-up-us = <1024> would be more natural.
> 

Actually ramp-up-us/ramp-down-us is more correct this is an error in this dt definition
and will be fixed in v2


>> +		lcd_backlight: led@0 {
>> +			reg = <0>;
>> +			led-sources = <2>;
>> +			ti,led-mode = <LM3532_BL_MODE_MANUAL>;
>> +			label = "backlight";
> 
> Ok, so we'll have lm3532::backlight. That is not too useful, as it
> does not tell userland what kaclight it is.
> 
> main_display::backlight ?
> 
> OTOH this one is not too important as backlight subsystem should
> handle this.
> 

For Droid4 I am not particular to any specific label.

And if the series in https://lkml.org/lkml/2018/11/7/144
is ever implemented then the label may change as well.

The driver forces the lm3532 string to be part of the label.

This was a discussion a while back with Jacek when I submitted other drivers.

>> +		led@1 {
>> +			reg = <1>;
>> +			led-sources = <1>;
>> +			ti,led-mode = <LM3532_BL_MODE_MANUAL>;
>> +			label = "keypad";
> 
> I guess best variant would be inputX::backlight here, but that might
> be tricky to implement.
> 

Yeah because we don't know what input the keyboard would be on.
Unless there are APIs to retrieve that info.  I have not looked at the input
framework in a while.

Dan

> 									Pavel
>
Pavel Machek March 8, 2019, 9:06 p.m. UTC | #3
> >> +		led@1 {
> >> +			reg = <1>;
> >> +			led-sources = <1>;
> >> +			ti,led-mode = <LM3532_BL_MODE_MANUAL>;
> >> +			label = "keypad";
> > 
> > I guess best variant would be inputX::backlight here, but that might
> > be tricky to implement.
> > 
> 
> Yeah because we don't know what input the keyboard would be on.
> Unless there are APIs to retrieve that info.  I have not looked at the input
> framework in a while.

Lets just make it "platform::kbd_backlight". *:kbd_backlight is
already used by quite a few drivers.

									Pavel
Dan Murphy March 8, 2019, 9:08 p.m. UTC | #4
Pavel

On 3/8/19 3:06 PM, Pavel Machek wrote:
> 
>>>> +		led@1 {
>>>> +			reg = <1>;
>>>> +			led-sources = <1>;
>>>> +			ti,led-mode = <LM3532_BL_MODE_MANUAL>;
>>>> +			label = "keypad";
>>>
>>> I guess best variant would be inputX::backlight here, but that might
>>> be tricky to implement.
>>>
>>
>> Yeah because we don't know what input the keyboard would be on.
>> Unless there are APIs to retrieve that info.  I have not looked at the input
>> framework in a while.
> 
> Lets just make it "platform::kbd_backlight". *:kbd_backlight is
> already used by quite a few drivers.
> 

Sounds fine to me.  I will update in v3 as I posted v2 with a lot of deltas

Dan

> 									Pavel
>

Patch
diff mbox series

diff --git a/arch/arm/boot/dts/omap4-droid4-xt894.dts b/arch/arm/boot/dts/omap4-droid4-xt894.dts
index e21ec929f096..94e3d53dbcf3 100644
--- a/arch/arm/boot/dts/omap4-droid4-xt894.dts
+++ b/arch/arm/boot/dts/omap4-droid4-xt894.dts
@@ -6,6 +6,7 @@ 
 /dts-v1/;
 
 #include <dt-bindings/input/input.h>
+#include <dt-bindings/leds/leds-lm3532.h>
 #include "omap443x.dtsi"
 #include "motorola-cpcap-mapphone.dtsi"
 
@@ -383,20 +384,30 @@ 
 };
 
 &i2c1 {
-	lm3532@38 {
+	led-controller@38 {
 		compatible = "ti,lm3532";
+		#address-cells = <1>;
+		#size-cells = <0>;
 		reg = <0x38>;
 
 		enable-gpios = <&gpio6 12 GPIO_ACTIVE_HIGH>;
 
-		lcd_backlight: backlight {
-			compatible = "ti,lm3532-backlight";
+		ramp-up-ms = <LM3532_RAMP_1024us>;
+		ramp-down-ms = <LM3532_RAMP_65536us>;
 
-			lcd {
-				led-sources = <0 1 2>;
-				ramp-up-msec = <1>;
-				ramp-down-msec = <0>;
-			};
+		lcd_backlight: led@0 {
+			reg = <0>;
+			led-sources = <2>;
+			ti,led-mode = <LM3532_BL_MODE_MANUAL>;
+			label = "backlight";
+			linux,default-trigger = "backlight";
+		};
+
+		led@1 {
+			reg = <1>;
+			led-sources = <1>;
+			ti,led-mode = <LM3532_BL_MODE_MANUAL>;
+			label = "keypad";
 		};
 	};
 };