mbox series

[v2,0/6] Updated lp8860 led driver

Message ID 20171205204327.12111-1-dmurphy@ti.com
Headers show
Series Updated lp8860 led driver | expand

Message

Dan Murphy Dec. 5, 2017, 8:43 p.m. UTC 
All

v2 - Added an initial patch to bring the DT binding up to standard prior to adding
the changes for the label and triggers.

v1 Cover letter repeat below

After creating a new LED driver for the LM3692x device I went back to the
LP8860 driver that I authored and found some updates that need to be applied.

First the way the LP8860 retrieved the label from the DT was incorrect as the
label should have been from a child node as opposed to the parent.  This is now
fixed with this series.

Second, since that device can be used to as either a backlight driver or as a
string agnostic driver a trigger to the backlight needed to be added.

Finally there are changes to the driver that need to be made as either
unnoticed bugs or updates to the driver to align with the current LED
framework.  For instance moving to the devm LED class registration, destroying
the mutex upon driver removal and removing the in driver dependency on CONFIG_OF
and moving it to the Kconfig.

With these changes this should at least bring the driver into a better shape.

There are additional changes coming for this driver but I wanted to get the
driver up to snuff before adding a feature to it.


Dan

Dan Murphy (6):
  dt: bindings: lp8860: Update bindings for lp8860
  dt: bindings: lp8860: Update DT label binding
  leds: lp8860: Update the dt parsing for LED labeling
  dt: bindings: lp8860: Add trigger binding to the lp8860
  leds: lp8860: Add DT parsing to retrieve the trigger node
  leds: lp8860: Various fixes to align with LED framework

 .../devicetree/bindings/leds/leds-lp8860.txt       | 39 ++++++++++++++-------
 drivers/leds/Kconfig                               |  2 +-
 drivers/leds/leds-lp8860.c                         | 40 ++++++++++++----------
 3 files changed, 49 insertions(+), 32 deletions(-)

-- 
2.15.0.124.g7668cbc60

Comments

Rob Herring (Arm) Dec. 7, 2017, 10:43 p.m. UTC | #1 
On Tue, Dec 05, 2017 at 02:43:22PM -0600, Dan Murphy wrote:
> Update the lp8860 bindings to fix various issues

> found.  Add address-cells and size-cells, rename

> enable-gpio to enable-gpios, update the node name

> to the device name and indent the node example.

> 

> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> ---

> 

> v2 - New patch

> 

>  .../devicetree/bindings/leds/leds-lp8860.txt       | 28 ++++++++++++----------

>  1 file changed, 16 insertions(+), 12 deletions(-)

> 

> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

> index aad38dd94d4b..1b2fab05ec6a 100644

> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt

> +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

> @@ -6,24 +6,28 @@ current sinks that can be controlled by a PWM input

>  signal, a SPI/I2C master, or both.

>  

>  Required properties:

> -	- compatible:

> +	- compatible :

>  		"ti,lp8860"

> -	- reg -  I2C slave address

> -	- label - Used for naming LEDs

> +	- reg : I2C slave address

> +	- label : Used for naming LEDs

> +	- #address-cells : 1

> +	- #size-cells : 0


This should be added in the next patch when you have child nodes.

Rob
Rob Herring (Arm) Dec. 7, 2017, 10:45 p.m. UTC | #2 
On Tue, Dec 05, 2017 at 02:43:23PM -0600, Dan Murphy wrote:
> Update the lp8860 label binding to the LED

> standard as documented in

> 

> Documentation/devicetree/bindings/leds/common.txt

> 

> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> ---

> 

> v2 - Added reg to child node and made it required

> 

>  Documentation/devicetree/bindings/leds/leds-lp8860.txt | 12 ++++++++++--

>  1 file changed, 10 insertions(+), 2 deletions(-)

> 

> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

> index 1b2fab05ec6a..22b594d95162 100644

> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt

> +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

> @@ -9,7 +9,6 @@ Required properties:

>  	- compatible :

>  		"ti,lp8860"

>  	- reg : I2C slave address

> -	- label : Used for naming LEDs

>  	- #address-cells : 1

>  	- #size-cells : 0

>  

> @@ -17,6 +16,12 @@ Optional properties:

>  	- enable-gpios : gpio pin to enable/disable the device.

>  	- supply : "vled" - LED supply

>  

> +Required child properties:

> +	- reg : 0

> +

> +Optional child properties:

> +	- label : see Documentation/devicetree/bindings/leds/common.txt

> +

>  Example:

>  

>  	lp8860@2d {

> @@ -24,9 +29,12 @@ Example:

>  		#address-cells: 1

>  		#size-cells: 0

>  		reg = <0x2d>;

> -		label = "display_cluster";

>  		enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;

>  		vled-supply = <&vbatt>;

> +		display_cluster: display_cluster@0 {


led@0 {

Do you really need a (dts) label here?

> +			reg=<0>;


spaces around the '='.

> +			label = "display_cluster";

> +		};

>  	}

>  

>  For more product information please see the link below:

> -- 

> 2.15.0.124.g7668cbc60

>
Rob Herring (Arm) Dec. 7, 2017, 10:46 p.m. UTC | #3 
On Tue, Dec 05, 2017 at 02:43:25PM -0600, Dan Murphy wrote:
> Add a default trigger optional node to the child node.

> This will allow the driver to set the trigger for a backlight.

> 

> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> ---

> 

> v2 - Moved binding changes to first patch in the series.

> 

>  Documentation/devicetree/bindings/leds/leds-lp8860.txt | 3 +++

>  1 file changed, 3 insertions(+)

> 

> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

> index 22b594d95162..3452c9c10499 100644

> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt

> +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

> @@ -21,6 +21,8 @@ Required child properties:

>  

>  Optional child properties:

>  	- label : see Documentation/devicetree/bindings/leds/common.txt

> +	- linux,default-trigger : (optional)


You already said this is optional.

> +	   see Documentation/devicetree/bindings/leds/common.txt

>  

>  Example:

>  

> @@ -34,6 +36,7 @@ Example:

>  		display_cluster: display_cluster@0 {

>  			reg=<0>;

>  			label = "display_cluster";

> +			linux,default-trigger = "backlight";

>  		};

>  	}

>  

> -- 

> 2.15.0.124.g7668cbc60

>
Dan Murphy Dec. 7, 2017, 11:07 p.m. UTC | #4 
Rob

Thanks

On 12/07/2017 04:42 PM, Rob Herring wrote:
> gOn Tue, Dec 05, 2017 at 02:43:22PM -0600, Dan Murphy wrote:

>> Update the lp8860 bindings to fix various issues

>> found.  Add address-cells and size-cells, rename

>> enable-gpio to enable-gpios, update the node name

>> to the device name and indent the node example.

>>

>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>> ---

>>

>> v2 - New patch

>>

>>  .../devicetree/bindings/leds/leds-lp8860.txt       | 28 ++++++++++++----------

>>  1 file changed, 16 insertions(+), 12 deletions(-)

>>

>> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

>> index aad38dd94d4b..1b2fab05ec6a 100644

>> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt

>> +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

>> @@ -6,24 +6,28 @@ current sinks that can be controlled by a PWM input

>>  signal, a SPI/I2C master, or both.

>>  

>>  Required properties:

>> -	- compatible:

>> +	- compatible :

>>  		"ti,lp8860"

>> -	- reg -  I2C slave address

>> -	- label - Used for naming LEDs

>> +	- reg : I2C slave address

>> +	- label : Used for naming LEDs

>> +	- #address-cells : 1

>> +	- #size-cells : 0

>>  

>>  Optional properties:

>> -	- enable-gpio - gpio pin to enable/disable the device.

>> -	- supply - "vled" - LED supply

>> +	- enable-gpios : gpio pin to enable/disable the device.

> 

> Needs to state active high or low.


Ack

> 

>> +	- supply : "vled" - LED supply

> 

> "vled-supply : ..."

> 


Ack

>>  

>>  Example:

>>  

>> -leds: leds@6 {

>> -	compatible = "ti,lp8860";

>> -	reg = <0x2d>;

>> -	label = "display_cluster";

>> -	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;

>> -	vled-supply = <&vbatt>;

>> -}

>> +	lp8860@2d {

> 

> leds@2d

> 

> There's not really any point in adding the indentation.


OK I was just following convention of other child node LED bindings

I can remove the indents.

> 

>> +		compatible = "ti,lp8860";

> 

>> +		#address-cells: 1

>> +		#size-cells: 0

> 

> s/:/=/

> 

> Though, these aren't necessary without any child nodes. Is there more 

> than 1 LED channel/driver?

> 


Yes this device can be used to drive at least 4 different LED strings as well
I will be adding this feature once I get this driver and binding cleaned up.

Dan

>> +		reg = <0x2d>;

>> +		label = "display_cluster";

>> +		enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;

>> +		vled-supply = <&vbatt>;

>> +	}

>>  

>>  For more product information please see the link below:

>>  http://www.ti.com/product/lp8860-q1

>> -- 

>> 2.15.0.124.g7668cbc60

>>



-- 
------------------
Dan Murphy
Dan Murphy Dec. 7, 2017, 11:08 p.m. UTC | #5 
Rob
On 12/07/2017 04:43 PM, Rob Herring wrote:
> On Tue, Dec 05, 2017 at 02:43:22PM -0600, Dan Murphy wrote:

>> Update the lp8860 bindings to fix various issues

>> found.  Add address-cells and size-cells, rename

>> enable-gpio to enable-gpios, update the node name

>> to the device name and indent the node example.

>>

>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>> ---

>>

>> v2 - New patch

>>

>>  .../devicetree/bindings/leds/leds-lp8860.txt       | 28 ++++++++++++----------

>>  1 file changed, 16 insertions(+), 12 deletions(-)

>>

>> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

>> index aad38dd94d4b..1b2fab05ec6a 100644

>> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt

>> +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

>> @@ -6,24 +6,28 @@ current sinks that can be controlled by a PWM input

>>  signal, a SPI/I2C master, or both.

>>  

>>  Required properties:

>> -	- compatible:

>> +	- compatible :

>>  		"ti,lp8860"

>> -	- reg -  I2C slave address

>> -	- label - Used for naming LEDs

>> +	- reg : I2C slave address

>> +	- label : Used for naming LEDs

>> +	- #address-cells : 1

>> +	- #size-cells : 0

> 

> This should be added in the next patch when you have child nodes.

> 


Ack

> Rob

> 



-- 
------------------
Dan Murphy
Dan Murphy Dec. 7, 2017, 11:09 p.m. UTC | #6 
Rob

On 12/07/2017 04:45 PM, Rob Herring wrote:
> On Tue, Dec 05, 2017 at 02:43:23PM -0600, Dan Murphy wrote:

>> Update the lp8860 label binding to the LED

>> standard as documented in

>>

>> Documentation/devicetree/bindings/leds/common.txt

>>

>> Signed-off-by: Dan Murphy <dmurphy@ti.com>

>> ---

>>

>> v2 - Added reg to child node and made it required

>>

>>  Documentation/devicetree/bindings/leds/leds-lp8860.txt | 12 ++++++++++--

>>  1 file changed, 10 insertions(+), 2 deletions(-)

>>

>> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

>> index 1b2fab05ec6a..22b594d95162 100644

>> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt

>> +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt

>> @@ -9,7 +9,6 @@ Required properties:

>>  	- compatible :

>>  		"ti,lp8860"

>>  	- reg : I2C slave address

>> -	- label : Used for naming LEDs

>>  	- #address-cells : 1

>>  	- #size-cells : 0

>>  

>> @@ -17,6 +16,12 @@ Optional properties:

>>  	- enable-gpios : gpio pin to enable/disable the device.

>>  	- supply : "vled" - LED supply

>>  

>> +Required child properties:

>> +	- reg : 0

>> +

>> +Optional child properties:

>> +	- label : see Documentation/devicetree/bindings/leds/common.txt

>> +

>>  Example:

>>  

>>  	lp8860@2d {

>> @@ -24,9 +29,12 @@ Example:

>>  		#address-cells: 1

>>  		#size-cells: 0

>>  		reg = <0x2d>;

>> -		label = "display_cluster";

>>  		enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;

>>  		vled-supply = <&vbatt>;

>> +		display_cluster: display_cluster@0 {

> 

> led@0 {

> 

> Do you really need a (dts) label here?


We will when I add LED string support.

> 

>> +			reg=<0>;

> 

> spaces around the '='.


Ack

> 

>> +			label = "display_cluster";

>> +		};

>>  	}

>>  

>>  For more product information please see the link below:

>> -- 

>> 2.15.0.124.g7668cbc60

>>



-- 
------------------
Dan Murphy