diff mbox series

[v5,1/2] dt-bindings: leds: Add bindings for lm3697 driver

Message ID 20180817151528.21623-1-dmurphy@ti.com
State Superseded
Headers show
Series [v5,1/2] dt-bindings: leds: Add bindings for lm3697 driver | expand

Commit Message

Dan Murphy Aug. 17, 2018, 3:15 p.m. UTC
Add the device tree bindings for the lm3697
LED driver for backlighting and display.

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

---

v5 - Fix the comment for the example - https://lore.kernel.org/patchwork/patch/975060/

v4 - Removed HVLED definition in favor of HVLED place definition - https://lore.kernel.org/patchwork/patch/974812/
v3 - Updated subject with prefered title - https://lore.kernel.org/patchwork/patch/972337/
v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/

 .../devicetree/bindings/leds/leds-lm3697.txt  | 86 +++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt

-- 
2.17.0.582.gccdcbd54c

Comments

Rob Herring (Arm) Aug. 20, 2018, 5:57 p.m. UTC | #1
On Fri, 17 Aug 2018 10:15:27 -0500, Dan Murphy wrote:
> Add the device tree bindings for the lm3697

> LED driver for backlighting and display.

> 

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

> ---

> 

> v5 - Fix the comment for the example - https://lore.kernel.org/patchwork/patch/975060/

> 

> v4 - Removed HVLED definition in favor of HVLED place definition - https://lore.kernel.org/patchwork/patch/974812/

> v3 - Updated subject with prefered title - https://lore.kernel.org/patchwork/patch/972337/

> v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/

> 

>  .../devicetree/bindings/leds/leds-lm3697.txt  | 86 +++++++++++++++++++

>  1 file changed, 86 insertions(+)

>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt

> 


Reviewed-by: Rob Herring <robh@kernel.org>
Pavel Machek Aug. 24, 2018, 10:05 a.m. UTC | #2
Hi!

> +/**

> + * struct lm3697 -

> + * @enable_gpio - Hardware enable gpio

> + * @regulator - LED supply regulator pointer

> + * @client - Pointer to the I2C client

> + * @regmap - Devices register map

> + * @dev - Pointer to the devices device struct

> + * @lock - Lock for reading/writing the device

> + * @leds - Array of LED strings.

> + */


extra .

> +	ret = regmap_write(led->priv->regmap, brt_msb_reg, brt_val);

> +	if (ret) {

> +		dev_err(&led->priv->client->dev, "Cannot write MSB\n");

> +		goto out;

> +	}

> +out:


I'd avoid this goto.

> +static int lm3697_set_control_bank(struct lm3697 *priv)

> +{

> +	u8 control_bank_config = 0;

> +	struct lm3697_led *led;

> +	int ret, i;

> +

> +	led = &priv->leds[0];

> +	if (led->control_bank == LM3697_CONTROL_A)

> +		led = &priv->leds[1];


I'd expect CONTROL_A to correspond to leds[0]...?

> +	for (i = 0; i < LM3697_MAX_LED_STRINGS; i++) {

> +		if (led->hvled_strings[i] == LM3697_HVLED_ASSIGNMENT)

> +			control_bank_config |= 1 << i;

> +	}


Extra {}s.

> +	priv->regulator = devm_regulator_get(&priv->client->dev, "vled");

> +	if (IS_ERR(priv->regulator))

> +		priv->regulator = NULL;


If vled regulator is specified in dt and _get fails, is it worth a
warning?

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Dan Murphy Aug. 24, 2018, 11:58 a.m. UTC | #3
Jacek

On 08/24/2018 05:05 AM, Pavel Machek wrote:
> Hi!

> 

>> +/**

>> + * struct lm3697 -

>> + * @enable_gpio - Hardware enable gpio

>> + * @regulator - LED supply regulator pointer

>> + * @client - Pointer to the I2C client

>> + * @regmap - Devices register map

>> + * @dev - Pointer to the devices device struct

>> + * @lock - Lock for reading/writing the device

>> + * @leds - Array of LED strings.

>> + */

> 

> extra .

> 

>> +	ret = regmap_write(led->priv->regmap, brt_msb_reg, brt_val);

>> +	if (ret) {

>> +		dev_err(&led->priv->client->dev, "Cannot write MSB\n");

>> +		goto out;

>> +	}

>> +out:

> 

> I'd avoid this goto.

> 

>> +static int lm3697_set_control_bank(struct lm3697 *priv)

>> +{

>> +	u8 control_bank_config = 0;

>> +	struct lm3697_led *led;

>> +	int ret, i;

>> +

>> +	led = &priv->leds[0];

>> +	if (led->control_bank == LM3697_CONTROL_A)

>> +		led = &priv->leds[1];

> 

> I'd expect CONTROL_A to correspond to leds[0]...?

> 

>> +	for (i = 0; i < LM3697_MAX_LED_STRINGS; i++) {

>> +		if (led->hvled_strings[i] == LM3697_HVLED_ASSIGNMENT)

>> +			control_bank_config |= 1 << i;

>> +	}

> 

> Extra {}s.

> 

>> +	priv->regulator = devm_regulator_get(&priv->client->dev, "vled");

>> +	if (IS_ERR(priv->regulator))

>> +		priv->regulator = NULL;

> 

> If vled regulator is specified in dt and _get fails, is it worth a

> warning?

> 

> 									Pavel

> 


Do you want v6 or a new patch on top?

Dan

-- 
------------------
Dan Murphy
Jacek Anaszewski Aug. 24, 2018, 7:32 p.m. UTC | #4
Hi Pavel,

On 08/24/2018 11:55 AM, Pavel Machek wrote:
> On Fri 2018-08-17 10:15:27, Dan Murphy wrote:

>> Add the device tree bindings for the lm3697

>> LED driver for backlighting and display.

>>

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

> 

> Acked-by: Pavel Machek <pavel@ucw.cz>

> 

> Some nits are below.

> 

>> +The LM3697 11-bit LED driver provides high-

>> +performance backlight dimming for 1, 2, or 3 series

>> +LED strings while delivering up to 90% efficiency.

> 

> LED core is 8-bit only... 


That's not true since this commit:

commit 1bd465e6b0e2b559db47420fea686507a01cfab0
Author: Guennadi Liakhovetski <lg@denx.de>
Date:   Sat Jan 10 18:54:39 2009 +0000

    leds: allow led-drivers to use a variable range of brightness values

    This patch allows drivers to override the default maximum brightness
value
    of 255.  We take care to preserve backwards-compatibility as much as
    possible, so that user-space ABI doesn't change for existing drivers.
    LED trigger code has also been updated to use the per-LED maximum.

    Signed-off-by: Guennadi Liakhovetski <lg@denx.de>

    Signed-off-by: Richard Purdie <rpurdie@linux.intel.com>



but I missed that max_brightness is not initialized in the driver,
so the core will set it to LED_FULL (255) anyway.

> so full dynamic range can not be currently

> used in linux -- right? Is there any plan to change/fix that?

> 

>> +This device is suitable for Display and Keypad Lighting

> 

> "display and keypad lighting."

> 

>> +Optional properties:

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

> 

> Remove "." at end of sentence, for consistency. "GPIO"?

> 

>> +All HVLED strings controlled by control bank A

> 

> ":"?

> 

>> +led-controller@36 {

>> +	compatible = "ti,lm3967";

>> +	reg = <0x36>;

>> +	#address-cells = <1>;

>> +	#size-cells = <0>;

>> +

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

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

>> +

>> +	led@0 {

>> +		reg = <0>;

>> +		led-sources = <1 1 1>;

>> +		label = "white:backlight_cluster";

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

>> +	};

>> +}

>> +

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

>> +http://www.ti.com/lit/ds/symlink/lm3697.pdf

> 


-- 
Best regards,
Jacek Anaszewski
Jacek Anaszewski Aug. 24, 2018, 7:33 p.m. UTC | #5
Dan,

On 08/24/2018 01:58 PM, Dan Murphy wrote:
> Jacek

> 

> On 08/24/2018 05:05 AM, Pavel Machek wrote:

>> Hi!

>>

>>> +/**

>>> + * struct lm3697 -

>>> + * @enable_gpio - Hardware enable gpio

>>> + * @regulator - LED supply regulator pointer

>>> + * @client - Pointer to the I2C client

>>> + * @regmap - Devices register map

>>> + * @dev - Pointer to the devices device struct

>>> + * @lock - Lock for reading/writing the device

>>> + * @leds - Array of LED strings.

>>> + */

>>

>> extra .

>>

>>> +	ret = regmap_write(led->priv->regmap, brt_msb_reg, brt_val);

>>> +	if (ret) {

>>> +		dev_err(&led->priv->client->dev, "Cannot write MSB\n");

>>> +		goto out;

>>> +	}

>>> +out:

>>

>> I'd avoid this goto.

>>

>>> +static int lm3697_set_control_bank(struct lm3697 *priv)

>>> +{

>>> +	u8 control_bank_config = 0;

>>> +	struct lm3697_led *led;

>>> +	int ret, i;

>>> +

>>> +	led = &priv->leds[0];

>>> +	if (led->control_bank == LM3697_CONTROL_A)

>>> +		led = &priv->leds[1];

>>

>> I'd expect CONTROL_A to correspond to leds[0]...?

>>

>>> +	for (i = 0; i < LM3697_MAX_LED_STRINGS; i++) {

>>> +		if (led->hvled_strings[i] == LM3697_HVLED_ASSIGNMENT)

>>> +			control_bank_config |= 1 << i;

>>> +	}

>>

>> Extra {}s.

>>

>>> +	priv->regulator = devm_regulator_get(&priv->client->dev, "vled");

>>> +	if (IS_ERR(priv->regulator))

>>> +		priv->regulator = NULL;

>>

>> If vled regulator is specified in dt and _get fails, is it worth a

>> warning?

>>

>> 									Pavel

>>

> 

> Do you want v6 or a new patch on top?


Please submit v6.

-- 
Best regards,
Jacek Anaszewski
Pavel Machek Sept. 7, 2018, 7:45 a.m. UTC | #6
Hi!

> >> +All HVLED strings controlled by control bank A

> > 

> > ":"?

> 

> Not sure what you are asking for here.


Text looked like missing ":" at end of line.

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

Patch

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
new file mode 100644
index 000000000000..76a529c69438
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
@@ -0,0 +1,86 @@ 
+* Texas Instruments - LM3697 Highly Efficient White LED Driver
+
+The LM3697 11-bit LED driver provides high-
+performance backlight dimming for 1, 2, or 3 series
+LED strings while delivering up to 90% efficiency.
+
+This device is suitable for Display and Keypad Lighting
+
+Required properties:
+	- compatible:
+		"ti,lm3967"
+	- reg :  I2C slave address
+	- #address-cells : 1
+	- #size-cells : 0
+
+Optional properties:
+	- enable-gpios : gpio pin to enable/disable the device.
+	- vled-supply : LED supply
+
+Required child properties:
+	- reg : 0 - LED is Controlled by bank A
+		1 - LED is Controlled by bank B
+	- led-sources : Indicates which HVLED string is associated to which
+			control bank.  Each element in the array is associated
+			with a specific HVLED string.  Element 0 is HVLED1,
+			element 1 is HVLED2 and element 2 HVLED3.
+			Additional information is contained
+			in Documentation/devicetree/bindings/leds/common.txt
+			0 - HVLED is not active in this control bank
+			1 - HVLED string is controlled by this control bank
+
+Optional child properties:
+	- label : see Documentation/devicetree/bindings/leds/common.txt
+	- linux,default-trigger :
+	   see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+HVLED string 1 and 3 are controlled by control bank A and HVLED 2 string is
+controlled by control bank B.
+
+led-controller@36 {
+	compatible = "ti,lm3967";
+	reg = <0x36>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+	vled-supply = <&vbatt>;
+
+	led@0 {
+		reg = <0>;
+		led-sources = <1 0 1>;
+		label = "white:first_backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
+
+	led@1 {
+		reg = <1>;
+		led-sources = <0 1 0>;
+		label = "white:second_backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
+}
+
+All HVLED strings controlled by control bank A
+
+led-controller@36 {
+	compatible = "ti,lm3967";
+	reg = <0x36>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+	vled-supply = <&vbatt>;
+
+	led@0 {
+		reg = <0>;
+		led-sources = <1 1 1>;
+		label = "white:backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
+}
+
+For more product information please see the link below:
+http://www.ti.com/lit/ds/symlink/lm3697.pdf