[PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Dan Murphy]

This adds the devicetree bindings for the LM3692x
I2C LED string driver.

Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---

v3 - No changes
v2 - No changes - https://patchwork.kernel.org/patch/10056677/

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

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
new file mode 100644
index 000000000000..cfef67bd4100
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
@@ -0,0 +1,28 @@
+* Texas Instruments - LM3692x Highly Efficient Dual-String White LED Driver
+
+The LM36922 is an ultra-compact, highly efficient,
+two string white-LED driver designed for LCD display
+backlighting.
+
+Required properties:
+	- compatible:
+		"ti,lm3692x"
+	- reg -  I2C slave address
+
+Optional properties:
+	- label - Used for naming LEDs
+	- enable-gpio - gpio pin to enable/disable the device.
+	- supply - "vled" - LED supply
+
+Example:
+
+leds: leds@6 {
+	compatible = "ti,lm3692x";
+	reg = <0x36>;
+	label = "backlight_cluster";
+	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+	vled-supply = <&vbatt>;
+}
+
+For more product information please see the link below:
+http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
-- 
2.15.0.124.g7668cbc60

[PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string driver

[Dan Murphy]

Introducing the LM3692x Dual-String white LED driver.

Data sheet is located
http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf

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

v3 - Add missing Makefile and Kconfig from v1 and v2 - https://patchwork.kernel.org/patch/10060075/

v2 - Added data sheet link, fixed linuxdoc format, returned on failure in init
routine, return on fault_check failure, updated brightness calculation and
fixed capitalization issue - https://patchwork.kernel.org/patch/10056675/

 drivers/leds/Kconfig        |   7 +
 drivers/leds/Makefile       |   1 +
 drivers/leds/leds-lm3692x.c | 390 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 398 insertions(+)
 create mode 100644 drivers/leds/leds-lm3692x.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 318a28fd58fe..1363e77a7d5f 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -137,6 +137,13 @@ config LEDS_LM3642
 	  converter plus 1.5A constant current driver for a high-current
 	  white LED.
 
+config LEDS_LM3692X
+	tristate "LED support for LM3692x Chips"
+	depends on LEDS_CLASS && I2C
+	select REGMAP_I2C
+	help
+	  This option enables support for the TI LM3692x family
+	  of LED drivers.
 
 config LEDS_LOCOMO
 	tristate "LED Support for Locomo device"
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index a2a6b5a4f86d..987884a5b9a5 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -74,6 +74,7 @@ obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
 obj-$(CONFIG_LEDS_MLXCPLD)		+= leds-mlxcpld.o
 obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
 obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
+obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
new file mode 100644
index 000000000000..9fa7f5a20466
--- /dev/null
+++ b/drivers/leds/leds-lm3692x.c
@@ -0,0 +1,390 @@
+/*
+ * TI lm3692x LED Driver
+ *
+ * Copyright (C) 2017 Texas Instruments
+ *
+ * Author: Dan Murphy <dmurphy@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * Data sheet is located
+ * http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
+ */
+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/slab.h>
+
+#define LM3692X_LED_NAME	"lm3692x_led"
+
+#define LM3692X_REV		0x0
+#define LM3692X_RESET		0x1
+#define LM3692X_EN		0x10
+#define LM3692X_BRT_CTRL	0x11
+#define LM3692X_PWM_CTRL	0x12
+#define LM3692X_BOOST_CTRL	0x13
+#define LM3692X_AUTO_FREQ_HI	0x15
+#define LM3692X_AUTO_FREQ_LO	0x16
+#define LM3692X_BL_ADJ_THRESH	0x17
+#define LM3692X_BRT_LSB		0x18
+#define LM3692X_BRT_MSB		0x19
+#define LM3692X_FAULT_CTRL	0x1e
+#define LM3692X_FAULT_FLAGS	0x1f
+
+#define LM3692X_SW_RESET	BIT(0)
+#define LM3692X_DEVICE_EN	BIT(0)
+#define LM3692X_LED1_EN		BIT(1)
+#define LM3692X_LED2_EN		BIT(2)
+
+/* Brightness Control Bits */
+#define LM3692X_BL_ADJ_POL	BIT(0)
+#define LM3692X_RAMP_RATE_125us	0x00
+#define LM3692X_RAMP_RATE_250us	BIT(1)
+#define LM3692X_RAMP_RATE_500us BIT(2)
+#define LM3692X_RAMP_RATE_1ms	(BIT(1) | BIT(2))
+#define LM3692X_RAMP_RATE_2ms	BIT(3)
+#define LM3692X_RAMP_RATE_4ms	(BIT(3) | BIT(1))
+#define LM3692X_RAMP_RATE_8ms	(BIT(2) | BIT(3))
+#define LM3692X_RAMP_RATE_16ms	(BIT(1) | BIT(2) | BIT(3))
+#define LM3692X_RAMP_EN		BIT(4)
+#define LM3692X_BRHT_MODE_REG	0x00
+#define LM3692X_BRHT_MODE_PWM	BIT(5)
+#define LM3692X_BRHT_MODE_MULTI_RAMP BIT(6)
+#define LM3692X_BRHT_MODE_RAMP_MULTI (BIT(5) | BIT(6))
+#define LM3692X_MAP_MODE_EXP	BIT(7)
+
+/* PWM Register Bits */
+#define LM3692X_PWM_FILTER_100	BIT(0)
+#define LM3692X_PWM_FILTER_150	BIT(1)
+#define LM3692X_PWM_FILTER_200	(BIT(0) | BIT(1))
+#define LM3692X_PWM_HYSTER_1LSB BIT(2)
+#define LM3692X_PWM_HYSTER_2LSB	BIT(3)
+#define LM3692X_PWM_HYSTER_3LSB (BIT(3) | BIT(2))
+#define LM3692X_PWM_HYSTER_4LSB BIT(4)
+#define LM3692X_PWM_HYSTER_5LSB (BIT(4) | BIT(2))
+#define LM3692X_PWM_HYSTER_6LSB (BIT(4) | BIT(3))
+#define LM3692X_PWM_POLARITY	BIT(5)
+#define LM3692X_PWM_SAMP_4MHZ	BIT(6)
+#define LM3692X_PWM_SAMP_24MHZ	BIT(7)
+
+/* Boost Control Bits */
+#define LM3692X_OCP_PROT_1A	BIT(0)
+#define LM3692X_OCP_PROT_1_25A	BIT(1)
+#define LM3692X_OCP_PROT_1_5A	(BIT(0) | BIT(1))
+#define LM3692X_OVP_21V		BIT(2)
+#define LM3692X_OVP_25V		BIT(3)
+#define LM3692X_OVP_29V		(BIT(2) | BIT(3))
+#define LM3692X_MIN_IND_22UH	BIT(4)
+#define LM3692X_BOOST_SW_1MHZ	BIT(5)
+#define LM3692X_BOOST_SW_NO_SHIFT	BIT(6)
+
+/* Fault Control Bits */
+#define LM3692X_FAULT_CTRL_OVP BIT(0)
+#define LM3692X_FAULT_CTRL_OCP BIT(1)
+#define LM3692X_FAULT_CTRL_TSD BIT(2)
+#define LM3692X_FAULT_CTRL_OPEN BIT(3)
+
+/* Fault Flag Bits */
+#define LM3692X_FAULT_FLAG_OVP BIT(0)
+#define LM3692X_FAULT_FLAG_OCP BIT(1)
+#define LM3692X_FAULT_FLAG_TSD BIT(2)
+#define LM3692X_FAULT_FLAG_SHRT BIT(3)
+#define LM3692X_FAULT_FLAG_OPEN BIT(4)
+
+/**
+ * struct lm3692x_led -
+ * @lock - Lock for reading/writing the device
+ * @client - Pointer to the I2C client
+ * @led_dev - led class device pointer
+ * @regmap - Devices register map
+ * @enable_gpio - VDDIO/EN gpio to enable communication interface
+ * @regulator - LED supply regulator pointer
+ * @label - LED label
+ */
+struct lm3692x_led {
+	struct mutex lock;
+	struct i2c_client *client;
+	struct led_classdev led_dev;
+	struct regmap *regmap;
+	struct gpio_desc *enable_gpio;
+	struct regulator *regulator;
+	const char *label;
+};
+
+static const struct reg_default lm3692x_reg_defs[] = {
+	{LM3692X_EN, 0xf},
+	{LM3692X_BRT_CTRL, 0x61},
+	{LM3692X_PWM_CTRL, 0x73},
+	{LM3692X_BOOST_CTRL, 0x6f},
+	{LM3692X_AUTO_FREQ_HI, 0x0},
+	{LM3692X_AUTO_FREQ_LO, 0x0},
+	{LM3692X_BL_ADJ_THRESH, 0x0},
+	{LM3692X_BRT_LSB, 0x7},
+	{LM3692X_BRT_MSB, 0xff},
+	{LM3692X_FAULT_CTRL, 0x7},
+};
+
+static const struct regmap_config lm3692x_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = LM3692X_FAULT_FLAGS,
+	.reg_defaults = lm3692x_reg_defs,
+	.num_reg_defaults = ARRAY_SIZE(lm3692x_reg_defs),
+	.cache_type = REGCACHE_RBTREE,
+};
+
+static int lm3692x_fault_check(struct lm3692x_led *led)
+{
+	int ret, fault;
+	unsigned int read_buf;
+
+	ret = regmap_read(led->regmap, LM3692X_FAULT_FLAGS, &read_buf);
+	if (ret)
+		return ret;
+
+	fault = read_buf;
+
+	if (fault)
+		dev_err(&led->client->dev, "Detected a fault 0x%X\n",
+			fault);
+
+	return ret;
+}
+
+static int lm3692x_brightness_set(struct led_classdev *led_cdev,
+				enum led_brightness brt_val)
+{
+	struct lm3692x_led *led =
+			container_of(led_cdev, struct lm3692x_led, led_dev);
+	int ret;
+	int led_brightness_lsb = (brt_val >> 5);
+
+	mutex_lock(&led->lock);
+
+	ret = lm3692x_fault_check(led);
+	if (ret) {
+		dev_err(&led->client->dev, "Cannot read/clear faults\n");
+		goto out;
+	}
+
+	ret = regmap_write(led->regmap, LM3692X_BRT_MSB, brt_val);
+	if (ret) {
+		dev_err(&led->client->dev, "Cannot write MSB\n");
+		goto out;
+	}
+
+	ret = regmap_write(led->regmap, LM3692X_BRT_LSB, led_brightness_lsb);
+	if (ret) {
+		dev_err(&led->client->dev, "Cannot write LSB\n");
+		goto out;
+	}
+out:
+	mutex_unlock(&led->lock);
+	return ret;
+}
+
+static int lm3692x_init(struct lm3692x_led *led)
+{
+	int ret;
+
+	if (led->regulator) {
+		ret = regulator_enable(led->regulator);
+		if (ret)
+			dev_err(&led->client->dev,
+				"Failed to enable regulator\n");
+			goto out;
+	}
+
+	if (led->enable_gpio)
+		gpiod_direction_output(led->enable_gpio, 1);
+
+	ret = lm3692x_fault_check(led);
+	if (ret) {
+		dev_err(&led->client->dev, "Cannot read/clear faults\n");
+		goto out;
+	}
+
+	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL, 0x00);
+	if (ret) {
+		dev_err(&led->client->dev, "Fail writing BRT CTRL\n");
+		goto out;
+	}
+
+	/*
+	 * For glitch free operation, the following data should
+	 * only be written while device enable bit is 0
+	 * per Section 7.5.14 of the data sheet
+	 */
+	ret = regmap_write(led->regmap, LM3692X_PWM_CTRL,
+		LM3692X_PWM_FILTER_100 | LM3692X_PWM_SAMP_24MHZ);
+	if (ret) {
+		dev_err(&led->client->dev, "Failed programming PWM CTRL\n");
+		goto out;
+	}
+
+	ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL,
+			LM3692X_BRHT_MODE_RAMP_MULTI |
+			LM3692X_BL_ADJ_POL |
+			LM3692X_RAMP_RATE_250us);
+	if (ret) {
+		dev_err(&led->client->dev, "Failed programming BOOST CTRL\n");
+		goto out;
+	}
+
+	ret = regmap_write(led->regmap, LM3692X_AUTO_FREQ_HI, 0x00);
+	if (ret) {
+		dev_err(&led->client->dev, "Failed programming AUTO HI FREQ\n");
+		goto out;
+	}
+
+	ret = regmap_write(led->regmap, LM3692X_AUTO_FREQ_LO, 0x00);
+	if (ret) {
+		dev_err(&led->client->dev, "Failed programming AUTO LO FREQ\n");
+		goto out;
+	}
+
+	ret = regmap_write(led->regmap, LM3692X_BL_ADJ_THRESH, 0x00);
+	if (ret) {
+		dev_err(&led->client->dev, "Failed programming BL Threshold\n");
+		goto out;
+	}
+
+	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL,
+			LM3692X_BL_ADJ_POL | LM3692X_PWM_HYSTER_4LSB);
+	if (ret)
+		dev_err(&led->client->dev, "Failed programming BRT CTRL\n");
+
+	return ret;
+out:
+
+	if (led->enable_gpio)
+		gpiod_direction_output(led->enable_gpio, 0);
+
+	if (led->regulator) {
+		ret = regulator_disable(led->regulator);
+		if (ret)
+			dev_err(&led->client->dev,
+				"Failed to disable regulator\n");
+	}
+
+	return ret;
+}
+
+static int lm3692x_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	int ret;
+	struct lm3692x_led *led;
+	struct device_node *np = client->dev.of_node;
+
+	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	if (client->dev.of_node) {
+		ret = of_property_read_string(np, "label", &led->label);
+		if (ret)
+			led->label = LM3692X_LED_NAME;
+	}
+
+	led->enable_gpio = devm_gpiod_get_optional(&client->dev,
+						   "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(led->enable_gpio)) {
+		ret = PTR_ERR(led->enable_gpio);
+		dev_err(&client->dev, "Failed to get enable gpio: %d\n", ret);
+		return ret;
+	}
+
+	led->regulator = devm_regulator_get(&client->dev, "vled");
+	if (IS_ERR(led->regulator))
+		led->regulator = NULL;
+
+	led->client = client;
+	led->led_dev.name = led->label;
+	led->led_dev.max_brightness = LED_FULL;
+	led->led_dev.brightness_set_blocking = lm3692x_brightness_set;
+
+	mutex_init(&led->lock);
+
+	i2c_set_clientdata(client, led);
+
+	led->regmap = devm_regmap_init_i2c(client, &lm3692x_regmap_config);
+	if (IS_ERR(led->regmap)) {
+		ret = PTR_ERR(led->regmap);
+		dev_err(&client->dev, "Failed to allocate register map: %d\n",
+			ret);
+		return ret;
+	}
+
+	ret = lm3692x_init(led);
+	if (ret)
+		return ret;
+
+	ret = led_classdev_register(&client->dev, &led->led_dev);
+	if (ret) {
+		dev_err(&client->dev, "led register err: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int lm3692x_remove(struct i2c_client *client)
+{
+	struct lm3692x_led *led = i2c_get_clientdata(client);
+	int ret;
+
+	led_classdev_unregister(&led->led_dev);
+
+	if (led->enable_gpio)
+		gpiod_direction_output(led->enable_gpio, 0);
+
+	if (led->regulator) {
+		ret = regulator_disable(led->regulator);
+		if (ret)
+			dev_err(&led->client->dev,
+				"Failed to disable regulator\n");
+	}
+
+	return 0;
+}
+
+static const struct i2c_device_id lm3692x_id[] = {
+	{ "lm3692x", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lm3692x_id);
+
+#ifdef CONFIG_OF
+static const struct of_device_id of_lm3692x_leds_match[] = {
+	{ .compatible = "ti,lm3692x", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_lm3692x_leds_match);
+#endif
+
+static struct i2c_driver lm3692x_driver = {
+	.driver = {
+		.name	= "lm3692x",
+		.of_match_table = of_match_ptr(of_lm3692x_leds_match),
+	},
+	.probe		= lm3692x_probe,
+	.remove		= lm3692x_remove,
+	.id_table	= lm3692x_id,
+};
+module_i2c_driver(lm3692x_driver);
+
+MODULE_DESCRIPTION("Texas Instruments LM3692X LED driver");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
+MODULE_LICENSE("GPL");
-- 
2.15.0.124.g7668cbc60

Re: [PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2016 bytes --]

Hi!

> Introducing the LM3692x Dual-String white LED driver.
> 
> Data sheet is located

"located at"? (Twice.)

> http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> +config LEDS_LM3692X
> +	tristate "LED support for LM3692x Chips"
> +	depends on LEDS_CLASS && I2C
> +	select REGMAP_I2C
> +	help
> +	  This option enables support for the TI LM3692x family
> +	  of LED drivers.

"If unsure ..., module will be named..."

Might want to say this is for backlight LEDs here.

> +static int lm3692x_fault_check(struct lm3692x_led *led)
> +{
> +	int ret, fault;
> +	unsigned int read_buf;
> +
> +	ret = regmap_read(led->regmap, LM3692X_FAULT_FLAGS, &read_buf);
> +	if (ret)
> +		return ret;
> +
> +	fault = read_buf;
> +
> +	if (fault)
> +		dev_err(&led->client->dev, "Detected a fault 0x%X\n",
> +			fault);
> +
> +	return ret;
> +}

Get rid of "fault" variable?

Does fault need to be propagated to the caller?


> +static int lm3692x_init(struct lm3692x_led *led)
> +{
> +	int ret;
> +
> +	if (led->regulator) {
> +		ret = regulator_enable(led->regulator);
> +		if (ret)
> +			dev_err(&led->client->dev,
> +				"Failed to enable regulator\n");
> +			goto out;
> +	}
> +
> +	if (led->enable_gpio)
> +		gpiod_direction_output(led->enable_gpio, 1);
> +
> +	ret = lm3692x_fault_check(led);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Cannot read/clear faults\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL, 0x00);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Fail writing BRT CTRL\n");
> +		goto out;
> +	}

How often are those fails reached? Maybe regmap wrapper that would
print "reading/writing register XY failed" would be enough?

Otherwise looks good,

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

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string driver

[Dan Murphy]

Pavel

On 11/15/2017 03:12 PM, Pavel Machek wrote:
> Hi!
> 
>> Introducing the LM3692x Dual-String white LED driver.
>>
>> Data sheet is located
> 
> "located at"? (Twice.)

Actually it is 3x since I call it out in the dt binding as well.

So what to eliminate?

> 
>> http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> 
>> +config LEDS_LM3692X
>> +	tristate "LED support for LM3692x Chips"
>> +	depends on LEDS_CLASS && I2C
>> +	select REGMAP_I2C
>> +	help
>> +	  This option enables support for the TI LM3692x family
>> +	  of LED drivers.
> 
> "If unsure ..., module will be named..."
> 
> Might want to say this is for backlight LEDs here.

AcK

> 
>> +static int lm3692x_fault_check(struct lm3692x_led *led)
>> +{
>> +	int ret, fault;
>> +	unsigned int read_buf;
>> +
>> +	ret = regmap_read(led->regmap, LM3692X_FAULT_FLAGS, &read_buf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	fault = read_buf;
>> +
>> +	if (fault)
>> +		dev_err(&led->client->dev, "Detected a fault 0x%X\n",
>> +			fault);
>> +
>> +	return ret;
>> +}
> 
> Get rid of "fault" variable?
> 
> Does fault need to be propagated to the caller?

I should probably set ret to fail if I see a fault or as you suggest propagate the fault

> 
> 
>> +static int lm3692x_init(struct lm3692x_led *led)
>> +{
>> +	int ret;
>> +
>> +	if (led->regulator) {
>> +		ret = regulator_enable(led->regulator);
>> +		if (ret)
>> +			dev_err(&led->client->dev,
>> +				"Failed to enable regulator\n");
>> +			goto out;
>> +	}
>> +
>> +	if (led->enable_gpio)
>> +		gpiod_direction_output(led->enable_gpio, 1);
>> +
>> +	ret = lm3692x_fault_check(led);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Cannot read/clear faults\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL, 0x00);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Fail writing BRT CTRL\n");
>> +		goto out;
>> +	}
> 
> How often are those fails reached? Maybe regmap wrapper that would
> print "reading/writing register XY failed" would be enough?

Or maybe in the out label I can just dev_err once saying initializing the device failed.
These are really only called at probe time.  Its the only time init is called.

Dan

> 
> Otherwise looks good,
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> 									Pavel
> 


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

Re: [PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string driver

[Jacek Anaszewski]

Hi Dan,

Please find my comments below.

On 11/15/2017 08:42 PM, Dan Murphy wrote:
> Introducing the LM3692x Dual-String white LED driver.
> 
> Data sheet is located
> http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v3 - Add missing Makefile and Kconfig from v1 and v2 - https://patchwork.kernel.org/patch/10060075/
> 
> v2 - Added data sheet link, fixed linuxdoc format, returned on failure in init
> routine, return on fault_check failure, updated brightness calculation and
> fixed capitalization issue - https://patchwork.kernel.org/patch/10056675/
> 
>  drivers/leds/Kconfig        |   7 +
>  drivers/leds/Makefile       |   1 +
>  drivers/leds/leds-lm3692x.c | 390 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 398 insertions(+)
>  create mode 100644 drivers/leds/leds-lm3692x.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 318a28fd58fe..1363e77a7d5f 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -137,6 +137,13 @@ config LEDS_LM3642
>  	  converter plus 1.5A constant current driver for a high-current
>  	  white LED.
>  
> +config LEDS_LM3692X
> +	tristate "LED support for LM3692x Chips"
> +	depends on LEDS_CLASS && I2C
> +	select REGMAP_I2C
> +	help
> +	  This option enables support for the TI LM3692x family
> +	  of LED drivers.
>  
>  config LEDS_LOCOMO
>  	tristate "LED Support for Locomo device"
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index a2a6b5a4f86d..987884a5b9a5 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -74,6 +74,7 @@ obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
>  obj-$(CONFIG_LEDS_MLXCPLD)		+= leds-mlxcpld.o
>  obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
>  obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
> +obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
>  
>  # LED SPI Drivers
>  obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
> diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
> new file mode 100644
> index 000000000000..9fa7f5a20466
> --- /dev/null
> +++ b/drivers/leds/leds-lm3692x.c
> @@ -0,0 +1,390 @@
> +/*
> + * TI lm3692x LED Driver
> + *
> + * Copyright (C) 2017 Texas Instruments
> + *
> + * Author: Dan Murphy <dmurphy@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * Data sheet is located
> + * http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/leds.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/slab.h>
> +
> +#define LM3692X_LED_NAME	"lm3692x_led"
> +
> +#define LM3692X_REV		0x0
> +#define LM3692X_RESET		0x1
> +#define LM3692X_EN		0x10
> +#define LM3692X_BRT_CTRL	0x11
> +#define LM3692X_PWM_CTRL	0x12
> +#define LM3692X_BOOST_CTRL	0x13
> +#define LM3692X_AUTO_FREQ_HI	0x15
> +#define LM3692X_AUTO_FREQ_LO	0x16
> +#define LM3692X_BL_ADJ_THRESH	0x17
> +#define LM3692X_BRT_LSB		0x18
> +#define LM3692X_BRT_MSB		0x19
> +#define LM3692X_FAULT_CTRL	0x1e
> +#define LM3692X_FAULT_FLAGS	0x1f
> +
> +#define LM3692X_SW_RESET	BIT(0)
> +#define LM3692X_DEVICE_EN	BIT(0)
> +#define LM3692X_LED1_EN		BIT(1)
> +#define LM3692X_LED2_EN		BIT(2)
> +
> +/* Brightness Control Bits */
> +#define LM3692X_BL_ADJ_POL	BIT(0)
> +#define LM3692X_RAMP_RATE_125us	0x00
> +#define LM3692X_RAMP_RATE_250us	BIT(1)
> +#define LM3692X_RAMP_RATE_500us BIT(2)
> +#define LM3692X_RAMP_RATE_1ms	(BIT(1) | BIT(2))
> +#define LM3692X_RAMP_RATE_2ms	BIT(3)
> +#define LM3692X_RAMP_RATE_4ms	(BIT(3) | BIT(1))
> +#define LM3692X_RAMP_RATE_8ms	(BIT(2) | BIT(3))
> +#define LM3692X_RAMP_RATE_16ms	(BIT(1) | BIT(2) | BIT(3))
> +#define LM3692X_RAMP_EN		BIT(4)
> +#define LM3692X_BRHT_MODE_REG	0x00
> +#define LM3692X_BRHT_MODE_PWM	BIT(5)
> +#define LM3692X_BRHT_MODE_MULTI_RAMP BIT(6)
> +#define LM3692X_BRHT_MODE_RAMP_MULTI (BIT(5) | BIT(6))
> +#define LM3692X_MAP_MODE_EXP	BIT(7)
> +
> +/* PWM Register Bits */
> +#define LM3692X_PWM_FILTER_100	BIT(0)
> +#define LM3692X_PWM_FILTER_150	BIT(1)
> +#define LM3692X_PWM_FILTER_200	(BIT(0) | BIT(1))
> +#define LM3692X_PWM_HYSTER_1LSB BIT(2)
> +#define LM3692X_PWM_HYSTER_2LSB	BIT(3)
> +#define LM3692X_PWM_HYSTER_3LSB (BIT(3) | BIT(2))
> +#define LM3692X_PWM_HYSTER_4LSB BIT(4)
> +#define LM3692X_PWM_HYSTER_5LSB (BIT(4) | BIT(2))
> +#define LM3692X_PWM_HYSTER_6LSB (BIT(4) | BIT(3))
> +#define LM3692X_PWM_POLARITY	BIT(5)
> +#define LM3692X_PWM_SAMP_4MHZ	BIT(6)
> +#define LM3692X_PWM_SAMP_24MHZ	BIT(7)
> +
> +/* Boost Control Bits */
> +#define LM3692X_OCP_PROT_1A	BIT(0)
> +#define LM3692X_OCP_PROT_1_25A	BIT(1)
> +#define LM3692X_OCP_PROT_1_5A	(BIT(0) | BIT(1))
> +#define LM3692X_OVP_21V		BIT(2)
> +#define LM3692X_OVP_25V		BIT(3)
> +#define LM3692X_OVP_29V		(BIT(2) | BIT(3))
> +#define LM3692X_MIN_IND_22UH	BIT(4)
> +#define LM3692X_BOOST_SW_1MHZ	BIT(5)
> +#define LM3692X_BOOST_SW_NO_SHIFT	BIT(6)
> +
> +/* Fault Control Bits */
> +#define LM3692X_FAULT_CTRL_OVP BIT(0)
> +#define LM3692X_FAULT_CTRL_OCP BIT(1)
> +#define LM3692X_FAULT_CTRL_TSD BIT(2)
> +#define LM3692X_FAULT_CTRL_OPEN BIT(3)
> +
> +/* Fault Flag Bits */
> +#define LM3692X_FAULT_FLAG_OVP BIT(0)
> +#define LM3692X_FAULT_FLAG_OCP BIT(1)
> +#define LM3692X_FAULT_FLAG_TSD BIT(2)
> +#define LM3692X_FAULT_FLAG_SHRT BIT(3)
> +#define LM3692X_FAULT_FLAG_OPEN BIT(4)
> +
> +/**
> + * struct lm3692x_led -
> + * @lock - Lock for reading/writing the device
> + * @client - Pointer to the I2C client
> + * @led_dev - led class device pointer
> + * @regmap - Devices register map
> + * @enable_gpio - VDDIO/EN gpio to enable communication interface
> + * @regulator - LED supply regulator pointer
> + * @label - LED label
> + */
> +struct lm3692x_led {
> +	struct mutex lock;
> +	struct i2c_client *client;
> +	struct led_classdev led_dev;
> +	struct regmap *regmap;
> +	struct gpio_desc *enable_gpio;
> +	struct regulator *regulator;
> +	const char *label;
> +};
> +
> +static const struct reg_default lm3692x_reg_defs[] = {
> +	{LM3692X_EN, 0xf},
> +	{LM3692X_BRT_CTRL, 0x61},
> +	{LM3692X_PWM_CTRL, 0x73},
> +	{LM3692X_BOOST_CTRL, 0x6f},
> +	{LM3692X_AUTO_FREQ_HI, 0x0},
> +	{LM3692X_AUTO_FREQ_LO, 0x0},
> +	{LM3692X_BL_ADJ_THRESH, 0x0},
> +	{LM3692X_BRT_LSB, 0x7},
> +	{LM3692X_BRT_MSB, 0xff},
> +	{LM3692X_FAULT_CTRL, 0x7},
> +};
> +
> +static const struct regmap_config lm3692x_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = LM3692X_FAULT_FLAGS,
> +	.reg_defaults = lm3692x_reg_defs,
> +	.num_reg_defaults = ARRAY_SIZE(lm3692x_reg_defs),
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
> +static int lm3692x_fault_check(struct lm3692x_led *led)
> +{
> +	int ret, fault;
> +	unsigned int read_buf;
> +
> +	ret = regmap_read(led->regmap, LM3692X_FAULT_FLAGS, &read_buf);
> +	if (ret)
> +		return ret;
> +
> +	fault = read_buf;
> +
> +	if (fault)
> +		dev_err(&led->client->dev, "Detected a fault 0x%X\n",
> +			fault);
> +
> +	return ret;
> +}
> +
> +static int lm3692x_brightness_set(struct led_classdev *led_cdev,
> +				enum led_brightness brt_val)
> +{
> +	struct lm3692x_led *led =
> +			container_of(led_cdev, struct lm3692x_led, led_dev);
> +	int ret;
> +	int led_brightness_lsb = (brt_val >> 5);
> +
> +	mutex_lock(&led->lock);
> +
> +	ret = lm3692x_fault_check(led);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Cannot read/clear faults\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BRT_MSB, brt_val);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Cannot write MSB\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BRT_LSB, led_brightness_lsb);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Cannot write LSB\n");
> +		goto out;
> +	}
> +out:
> +	mutex_unlock(&led->lock);
> +	return ret;
> +}
> +
> +static int lm3692x_init(struct lm3692x_led *led)
> +{
> +	int ret;
> +
> +	if (led->regulator) {
> +		ret = regulator_enable(led->regulator);
> +		if (ret)
> +			dev_err(&led->client->dev,
> +				"Failed to enable regulator\n");
> +			goto out;
> +	}
> +
> +	if (led->enable_gpio)
> +		gpiod_direction_output(led->enable_gpio, 1);
> +
> +	ret = lm3692x_fault_check(led);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Cannot read/clear faults\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL, 0x00);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Fail writing BRT CTRL\n");
> +		goto out;
> +	}
> +
> +	/*
> +	 * For glitch free operation, the following data should
> +	 * only be written while device enable bit is 0
> +	 * per Section 7.5.14 of the data sheet
> +	 */
> +	ret = regmap_write(led->regmap, LM3692X_PWM_CTRL,
> +		LM3692X_PWM_FILTER_100 | LM3692X_PWM_SAMP_24MHZ);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Failed programming PWM CTRL\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL,
> +			LM3692X_BRHT_MODE_RAMP_MULTI |
> +			LM3692X_BL_ADJ_POL |
> +			LM3692X_RAMP_RATE_250us);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Failed programming BOOST CTRL\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_AUTO_FREQ_HI, 0x00);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Failed programming AUTO HI FREQ\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_AUTO_FREQ_LO, 0x00);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Failed programming AUTO LO FREQ\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BL_ADJ_THRESH, 0x00);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Failed programming BL Threshold\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL,
> +			LM3692X_BL_ADJ_POL | LM3692X_PWM_HYSTER_4LSB);
> +	if (ret)
> +		dev_err(&led->client->dev, "Failed programming BRT CTRL\n");
> +
> +	return ret;
> +out:
> +
> +	if (led->enable_gpio)
> +		gpiod_direction_output(led->enable_gpio, 0);
> +
> +	if (led->regulator) {
> +		ret = regulator_disable(led->regulator);
> +		if (ret)
> +			dev_err(&led->client->dev,
> +				"Failed to disable regulator\n");
> +	}
> +
> +	return ret;
> +}
> +
> +static int lm3692x_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	int ret;
> +	struct lm3692x_led *led;
> +	struct device_node *np = client->dev.of_node;
> +
> +	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	if (client->dev.of_node) {

We should fail in the opposite case.

> +		ret = of_property_read_string(np, "label", &led->label);
> +		if (ret)
> +			led->label = LM3692X_LED_NAME;

LED label should be taken from the child node name if label property is
not present. See Documentation/devicetree/bindings/leds/common.txt.

We have some mess in this area, but I'm going to sort it out soon
(added you on CC to the related discussion).

Generally LED class device name should be composed by the LED class
driver by taking label and prefixing it with "[devicename]:" string.

> +	}
> +
> +	led->enable_gpio = devm_gpiod_get_optional(&client->dev,
> +						   "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(led->enable_gpio)) {
> +		ret = PTR_ERR(led->enable_gpio);
> +		dev_err(&client->dev, "Failed to get enable gpio: %d\n", ret);
> +		return ret;
> +	}
> +
> +	led->regulator = devm_regulator_get(&client->dev, "vled");
> +	if (IS_ERR(led->regulator))
> +		led->regulator = NULL;
> +
> +	led->client = client;
> +	led->led_dev.name = led->label;
> +	led->led_dev.max_brightness = LED_FULL;

You can skip this line, it will be initialized to LED_FULL
in of_led_classdev_register in this case.


> +	led->led_dev.brightness_set_blocking = lm3692x_brightness_set;
> +
> +	mutex_init(&led->lock);
> +
> +	i2c_set_clientdata(client, led);
> +
> +	led->regmap = devm_regmap_init_i2c(client, &lm3692x_regmap_config);
> +	if (IS_ERR(led->regmap)) {
> +		ret = PTR_ERR(led->regmap);
> +		dev_err(&client->dev, "Failed to allocate register map: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	ret = lm3692x_init(led);
> +	if (ret)
> +		return ret;
> +
> +	ret = led_classdev_register(&client->dev, &led->led_dev);

Please use devm_* prefixed version.

> +	if (ret) {
> +		dev_err(&client->dev, "led register err: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int lm3692x_remove(struct i2c_client *client)
> +{
> +	struct lm3692x_led *led = i2c_get_clientdata(client);
> +	int ret;
> +
> +	led_classdev_unregister(&led->led_dev);
> +
> +	if (led->enable_gpio)
> +		gpiod_direction_output(led->enable_gpio, 0);
> +
> +	if (led->regulator) {
> +		ret = regulator_disable(led->regulator);
> +		if (ret)
> +			dev_err(&led->client->dev,
> +				"Failed to disable regulator\n");
> +	}

mutex_destroy(&led->lock)

> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id lm3692x_id[] = {
> +	{ "lm3692x", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, lm3692x_id);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id of_lm3692x_leds_match[] = {
> +	{ .compatible = "ti,lm3692x", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_lm3692x_leds_match);
> +#endif
> +
> +static struct i2c_driver lm3692x_driver = {
> +	.driver = {
> +		.name	= "lm3692x",
> +		.of_match_table = of_match_ptr(of_lm3692x_leds_match),
> +	},
> +	.probe		= lm3692x_probe,
> +	.remove		= lm3692x_remove,
> +	.id_table	= lm3692x_id,
> +};
> +module_i2c_driver(lm3692x_driver);
> +
> +MODULE_DESCRIPTION("Texas Instruments LM3692X LED driver");
> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
> +MODULE_LICENSE("GPL");

According to your license info it should be "GPL v2"

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 2/2] leds: lm3692x: Introduce LM3692x dual string driver

[Dan Murphy]

Jacek

Thanks

On 11/18/2017 08:19 AM, Jacek Anaszewski wrote:
> Hi Dan,
> 
> Please find my comments below.
> 
> On 11/15/2017 08:42 PM, Dan Murphy wrote:
>> Introducing the LM3692x Dual-String white LED driver.
>>
>> Data sheet is located
>> http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v3 - Add missing Makefile and Kconfig from v1 and v2 - https://patchwork.kernel.org/patch/10060075/
>>
>> v2 - Added data sheet link, fixed linuxdoc format, returned on failure in init
>> routine, return on fault_check failure, updated brightness calculation and
>> fixed capitalization issue - https://patchwork.kernel.org/patch/10056675/
>>
>>  drivers/leds/Kconfig        |   7 +
>>  drivers/leds/Makefile       |   1 +
>>  drivers/leds/leds-lm3692x.c | 390 ++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 398 insertions(+)
>>  create mode 100644 drivers/leds/leds-lm3692x.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 318a28fd58fe..1363e77a7d5f 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -137,6 +137,13 @@ config LEDS_LM3642
>>  	  converter plus 1.5A constant current driver for a high-current
>>  	  white LED.
>>  
>> +config LEDS_LM3692X
>> +	tristate "LED support for LM3692x Chips"
>> +	depends on LEDS_CLASS && I2C
>> +	select REGMAP_I2C
>> +	help
>> +	  This option enables support for the TI LM3692x family
>> +	  of LED drivers.
>>  
>>  config LEDS_LOCOMO
>>  	tristate "LED Support for Locomo device"
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index a2a6b5a4f86d..987884a5b9a5 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -74,6 +74,7 @@ obj-$(CONFIG_LEDS_PM8058)		+= leds-pm8058.o
>>  obj-$(CONFIG_LEDS_MLXCPLD)		+= leds-mlxcpld.o
>>  obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
>>  obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
>> +obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
>>  
>>  # LED SPI Drivers
>>  obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o
>> diff --git a/drivers/leds/leds-lm3692x.c b/drivers/leds/leds-lm3692x.c
>> new file mode 100644
>> index 000000000000..9fa7f5a20466
>> --- /dev/null
>> +++ b/drivers/leds/leds-lm3692x.c
>> @@ -0,0 +1,390 @@
>> +/*
>> + * TI lm3692x LED Driver
>> + *
>> + * Copyright (C) 2017 Texas Instruments
>> + *
>> + * Author: Dan Murphy <dmurphy@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + *
>> + * Data sheet is located
>> + * http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/init.h>
>> +#include <linux/leds.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/slab.h>
>> +
>> +#define LM3692X_LED_NAME	"lm3692x_led"
>> +
>> +#define LM3692X_REV		0x0
>> +#define LM3692X_RESET		0x1
>> +#define LM3692X_EN		0x10
>> +#define LM3692X_BRT_CTRL	0x11
>> +#define LM3692X_PWM_CTRL	0x12
>> +#define LM3692X_BOOST_CTRL	0x13
>> +#define LM3692X_AUTO_FREQ_HI	0x15
>> +#define LM3692X_AUTO_FREQ_LO	0x16
>> +#define LM3692X_BL_ADJ_THRESH	0x17
>> +#define LM3692X_BRT_LSB		0x18
>> +#define LM3692X_BRT_MSB		0x19
>> +#define LM3692X_FAULT_CTRL	0x1e
>> +#define LM3692X_FAULT_FLAGS	0x1f
>> +
>> +#define LM3692X_SW_RESET	BIT(0)
>> +#define LM3692X_DEVICE_EN	BIT(0)
>> +#define LM3692X_LED1_EN		BIT(1)
>> +#define LM3692X_LED2_EN		BIT(2)
>> +
>> +/* Brightness Control Bits */
>> +#define LM3692X_BL_ADJ_POL	BIT(0)
>> +#define LM3692X_RAMP_RATE_125us	0x00
>> +#define LM3692X_RAMP_RATE_250us	BIT(1)
>> +#define LM3692X_RAMP_RATE_500us BIT(2)
>> +#define LM3692X_RAMP_RATE_1ms	(BIT(1) | BIT(2))
>> +#define LM3692X_RAMP_RATE_2ms	BIT(3)
>> +#define LM3692X_RAMP_RATE_4ms	(BIT(3) | BIT(1))
>> +#define LM3692X_RAMP_RATE_8ms	(BIT(2) | BIT(3))
>> +#define LM3692X_RAMP_RATE_16ms	(BIT(1) | BIT(2) | BIT(3))
>> +#define LM3692X_RAMP_EN		BIT(4)
>> +#define LM3692X_BRHT_MODE_REG	0x00
>> +#define LM3692X_BRHT_MODE_PWM	BIT(5)
>> +#define LM3692X_BRHT_MODE_MULTI_RAMP BIT(6)
>> +#define LM3692X_BRHT_MODE_RAMP_MULTI (BIT(5) | BIT(6))
>> +#define LM3692X_MAP_MODE_EXP	BIT(7)
>> +
>> +/* PWM Register Bits */
>> +#define LM3692X_PWM_FILTER_100	BIT(0)
>> +#define LM3692X_PWM_FILTER_150	BIT(1)
>> +#define LM3692X_PWM_FILTER_200	(BIT(0) | BIT(1))
>> +#define LM3692X_PWM_HYSTER_1LSB BIT(2)
>> +#define LM3692X_PWM_HYSTER_2LSB	BIT(3)
>> +#define LM3692X_PWM_HYSTER_3LSB (BIT(3) | BIT(2))
>> +#define LM3692X_PWM_HYSTER_4LSB BIT(4)
>> +#define LM3692X_PWM_HYSTER_5LSB (BIT(4) | BIT(2))
>> +#define LM3692X_PWM_HYSTER_6LSB (BIT(4) | BIT(3))
>> +#define LM3692X_PWM_POLARITY	BIT(5)
>> +#define LM3692X_PWM_SAMP_4MHZ	BIT(6)
>> +#define LM3692X_PWM_SAMP_24MHZ	BIT(7)
>> +
>> +/* Boost Control Bits */
>> +#define LM3692X_OCP_PROT_1A	BIT(0)
>> +#define LM3692X_OCP_PROT_1_25A	BIT(1)
>> +#define LM3692X_OCP_PROT_1_5A	(BIT(0) | BIT(1))
>> +#define LM3692X_OVP_21V		BIT(2)
>> +#define LM3692X_OVP_25V		BIT(3)
>> +#define LM3692X_OVP_29V		(BIT(2) | BIT(3))
>> +#define LM3692X_MIN_IND_22UH	BIT(4)
>> +#define LM3692X_BOOST_SW_1MHZ	BIT(5)
>> +#define LM3692X_BOOST_SW_NO_SHIFT	BIT(6)
>> +
>> +/* Fault Control Bits */
>> +#define LM3692X_FAULT_CTRL_OVP BIT(0)
>> +#define LM3692X_FAULT_CTRL_OCP BIT(1)
>> +#define LM3692X_FAULT_CTRL_TSD BIT(2)
>> +#define LM3692X_FAULT_CTRL_OPEN BIT(3)
>> +
>> +/* Fault Flag Bits */
>> +#define LM3692X_FAULT_FLAG_OVP BIT(0)
>> +#define LM3692X_FAULT_FLAG_OCP BIT(1)
>> +#define LM3692X_FAULT_FLAG_TSD BIT(2)
>> +#define LM3692X_FAULT_FLAG_SHRT BIT(3)
>> +#define LM3692X_FAULT_FLAG_OPEN BIT(4)
>> +
>> +/**
>> + * struct lm3692x_led -
>> + * @lock - Lock for reading/writing the device
>> + * @client - Pointer to the I2C client
>> + * @led_dev - led class device pointer
>> + * @regmap - Devices register map
>> + * @enable_gpio - VDDIO/EN gpio to enable communication interface
>> + * @regulator - LED supply regulator pointer
>> + * @label - LED label
>> + */
>> +struct lm3692x_led {
>> +	struct mutex lock;
>> +	struct i2c_client *client;
>> +	struct led_classdev led_dev;
>> +	struct regmap *regmap;
>> +	struct gpio_desc *enable_gpio;
>> +	struct regulator *regulator;
>> +	const char *label;
>> +};
>> +
>> +static const struct reg_default lm3692x_reg_defs[] = {
>> +	{LM3692X_EN, 0xf},
>> +	{LM3692X_BRT_CTRL, 0x61},
>> +	{LM3692X_PWM_CTRL, 0x73},
>> +	{LM3692X_BOOST_CTRL, 0x6f},
>> +	{LM3692X_AUTO_FREQ_HI, 0x0},
>> +	{LM3692X_AUTO_FREQ_LO, 0x0},
>> +	{LM3692X_BL_ADJ_THRESH, 0x0},
>> +	{LM3692X_BRT_LSB, 0x7},
>> +	{LM3692X_BRT_MSB, 0xff},
>> +	{LM3692X_FAULT_CTRL, 0x7},
>> +};
>> +
>> +static const struct regmap_config lm3692x_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +
>> +	.max_register = LM3692X_FAULT_FLAGS,
>> +	.reg_defaults = lm3692x_reg_defs,
>> +	.num_reg_defaults = ARRAY_SIZE(lm3692x_reg_defs),
>> +	.cache_type = REGCACHE_RBTREE,
>> +};
>> +
>> +static int lm3692x_fault_check(struct lm3692x_led *led)
>> +{
>> +	int ret, fault;
>> +	unsigned int read_buf;
>> +
>> +	ret = regmap_read(led->regmap, LM3692X_FAULT_FLAGS, &read_buf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	fault = read_buf;
>> +
>> +	if (fault)
>> +		dev_err(&led->client->dev, "Detected a fault 0x%X\n",
>> +			fault);
>> +
>> +	return ret;
>> +}
>> +
>> +static int lm3692x_brightness_set(struct led_classdev *led_cdev,
>> +				enum led_brightness brt_val)
>> +{
>> +	struct lm3692x_led *led =
>> +			container_of(led_cdev, struct lm3692x_led, led_dev);
>> +	int ret;
>> +	int led_brightness_lsb = (brt_val >> 5);
>> +
>> +	mutex_lock(&led->lock);
>> +
>> +	ret = lm3692x_fault_check(led);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Cannot read/clear faults\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->regmap, LM3692X_BRT_MSB, brt_val);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Cannot write MSB\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->regmap, LM3692X_BRT_LSB, led_brightness_lsb);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Cannot write LSB\n");
>> +		goto out;
>> +	}
>> +out:
>> +	mutex_unlock(&led->lock);
>> +	return ret;
>> +}
>> +
>> +static int lm3692x_init(struct lm3692x_led *led)
>> +{
>> +	int ret;
>> +
>> +	if (led->regulator) {
>> +		ret = regulator_enable(led->regulator);
>> +		if (ret)
>> +			dev_err(&led->client->dev,
>> +				"Failed to enable regulator\n");
>> +			goto out;
>> +	}
>> +
>> +	if (led->enable_gpio)
>> +		gpiod_direction_output(led->enable_gpio, 1);
>> +
>> +	ret = lm3692x_fault_check(led);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Cannot read/clear faults\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL, 0x00);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Fail writing BRT CTRL\n");
>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * For glitch free operation, the following data should
>> +	 * only be written while device enable bit is 0
>> +	 * per Section 7.5.14 of the data sheet
>> +	 */
>> +	ret = regmap_write(led->regmap, LM3692X_PWM_CTRL,
>> +		LM3692X_PWM_FILTER_100 | LM3692X_PWM_SAMP_24MHZ);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Failed programming PWM CTRL\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->regmap, LM3692X_BOOST_CTRL,
>> +			LM3692X_BRHT_MODE_RAMP_MULTI |
>> +			LM3692X_BL_ADJ_POL |
>> +			LM3692X_RAMP_RATE_250us);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Failed programming BOOST CTRL\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->regmap, LM3692X_AUTO_FREQ_HI, 0x00);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Failed programming AUTO HI FREQ\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->regmap, LM3692X_AUTO_FREQ_LO, 0x00);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Failed programming AUTO LO FREQ\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->regmap, LM3692X_BL_ADJ_THRESH, 0x00);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Failed programming BL Threshold\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->regmap, LM3692X_BRT_CTRL,
>> +			LM3692X_BL_ADJ_POL | LM3692X_PWM_HYSTER_4LSB);
>> +	if (ret)
>> +		dev_err(&led->client->dev, "Failed programming BRT CTRL\n");
>> +
>> +	return ret;
>> +out:
>> +
>> +	if (led->enable_gpio)
>> +		gpiod_direction_output(led->enable_gpio, 0);
>> +
>> +	if (led->regulator) {
>> +		ret = regulator_disable(led->regulator);
>> +		if (ret)
>> +			dev_err(&led->client->dev,
>> +				"Failed to disable regulator\n");
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int lm3692x_probe(struct i2c_client *client,
>> +			const struct i2c_device_id *id)
>> +{
>> +	int ret;
>> +	struct lm3692x_led *led;
>> +	struct device_node *np = client->dev.of_node;
>> +
>> +	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
>> +	if (!led)
>> +		return -ENOMEM;
>> +
>> +	if (client->dev.of_node) {
> 
> We should fail in the opposite case.

OK

> 
>> +		ret = of_property_read_string(np, "label", &led->label);
>> +		if (ret)
>> +			led->label = LM3692X_LED_NAME;
> 
> LED label should be taken from the child node name if label property is
> not present. See Documentation/devicetree/bindings/leds/common.txt.
> 
> We have some mess in this area, but I'm going to sort it out soon
> (added you on CC to the related discussion).
> 
> Generally LED class device name should be composed by the LED class
> driver by taking label and prefixing it with "[devicename]:" string.

OK

> 
>> +	}
>> +
>> +	led->enable_gpio = devm_gpiod_get_optional(&client->dev,
>> +						   "enable", GPIOD_OUT_LOW);
>> +	if (IS_ERR(led->enable_gpio)) {
>> +		ret = PTR_ERR(led->enable_gpio);
>> +		dev_err(&client->dev, "Failed to get enable gpio: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	led->regulator = devm_regulator_get(&client->dev, "vled");
>> +	if (IS_ERR(led->regulator))
>> +		led->regulator = NULL;
>> +
>> +	led->client = client;
>> +	led->led_dev.name = led->label;
>> +	led->led_dev.max_brightness = LED_FULL;
> 
> You can skip this line, it will be initialized to LED_FULL
> in of_led_classdev_register in this case.

OK

> 
> 
>> +	led->led_dev.brightness_set_blocking = lm3692x_brightness_set;
>> +
>> +	mutex_init(&led->lock);
>> +
>> +	i2c_set_clientdata(client, led);
>> +
>> +	led->regmap = devm_regmap_init_i2c(client, &lm3692x_regmap_config);
>> +	if (IS_ERR(led->regmap)) {
>> +		ret = PTR_ERR(led->regmap);
>> +		dev_err(&client->dev, "Failed to allocate register map: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = lm3692x_init(led);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = led_classdev_register(&client->dev, &led->led_dev);
> 
> Please use devm_* prefixed version.

OK

> 
>> +	if (ret) {
>> +		dev_err(&client->dev, "led register err: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int lm3692x_remove(struct i2c_client *client)
>> +{
>> +	struct lm3692x_led *led = i2c_get_clientdata(client);
>> +	int ret;
>> +
>> +	led_classdev_unregister(&led->led_dev);
>> +
>> +	if (led->enable_gpio)
>> +		gpiod_direction_output(led->enable_gpio, 0);
>> +
>> +	if (led->regulator) {
>> +		ret = regulator_disable(led->regulator);
>> +		if (ret)
>> +			dev_err(&led->client->dev,
>> +				"Failed to disable regulator\n");
>> +	}
> 
> mutex_destroy(&led->lock)

OK

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct i2c_device_id lm3692x_id[] = {
>> +	{ "lm3692x", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, lm3692x_id);
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_lm3692x_leds_match[] = {
>> +	{ .compatible = "ti,lm3692x", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, of_lm3692x_leds_match);
>> +#endif
>> +
>> +static struct i2c_driver lm3692x_driver = {
>> +	.driver = {
>> +		.name	= "lm3692x",
>> +		.of_match_table = of_match_ptr(of_lm3692x_leds_match),
>> +	},
>> +	.probe		= lm3692x_probe,
>> +	.remove		= lm3692x_remove,
>> +	.id_table	= lm3692x_id,
>> +};
>> +module_i2c_driver(lm3692x_driver);
>> +
>> +MODULE_DESCRIPTION("Texas Instruments LM3692X LED driver");
>> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
>> +MODULE_LICENSE("GPL");
> 
> According to your license info it should be "GPL v2"
> 

OK


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

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Jacek Anaszewski]

Hi Dan,

Thanks for the patch.

On 11/15/2017 08:42 PM, Dan Murphy wrote:
> This adds the devicetree bindings for the LM3692x
> I2C LED string driver.
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v3 - No changes
> v2 - No changes - https://patchwork.kernel.org/patch/10056677/
> 
>  .../devicetree/bindings/leds/leds-lm3692x.txt      | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3692x.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
> new file mode 100644
> index 000000000000..cfef67bd4100
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
> @@ -0,0 +1,28 @@
> +* Texas Instruments - LM3692x Highly Efficient Dual-String White LED Driver
> +
> +The LM36922 is an ultra-compact, highly efficient,
> +two string white-LED driver designed for LCD display
> +backlighting.

Shouldn't the driver be targeted for backlight subsystem then?

Adding backlight maintainers.

Best regards,
Jacek Anaszewski

> +Required properties:
> +	- compatible:
> +		"ti,lm3692x"
> +	- reg -  I2C slave address
> +
> +Optional properties:
> +	- label - Used for naming LEDs
> +	- enable-gpio - gpio pin to enable/disable the device.
> +	- supply - "vled" - LED supply
> +
> +Example:
> +
> +leds: leds@6 {
> +	compatible = "ti,lm3692x";
> +	reg = <0x36>;
> +	label = "backlight_cluster";
> +	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> +	vled-supply = <&vbatt>;
> +}
> +
> +For more product information please see the link below:
> +http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
> 

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Dan Murphy]

Jacek

On 11/15/2017 02:15 PM, Jacek Anaszewski wrote:
> Hi Dan,
> 
> Thanks for the patch.
> 
> On 11/15/2017 08:42 PM, Dan Murphy wrote:
>> This adds the devicetree bindings for the LM3692x
>> I2C LED string driver.
>>
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v3 - No changes
>> v2 - No changes - https://patchwork.kernel.org/patch/10056677/
>>
>>  .../devicetree/bindings/leds/leds-lm3692x.txt      | 28 ++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>> new file mode 100644
>> index 000000000000..cfef67bd4100
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>> @@ -0,0 +1,28 @@
>> +* Texas Instruments - LM3692x Highly Efficient Dual-String White LED Driver
>> +
>> +The LM36922 is an ultra-compact, highly efficient,
>> +two string white-LED driver designed for LCD display
>> +backlighting.
> 
> Shouldn't the driver be targeted for backlight subsystem then?
> 
> Adding backlight maintainers.
> 

Great point!  I was not aware of the backlight subsystem.

Looks like I need to create a back light version as well.

Like the lp8788 did since this can be used as a LED driver beyond display back lighting.

Unless the maintainers don't want it.

Dan


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

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 879 bytes --]

Hi!

> > Shouldn't the driver be targeted for backlight subsystem then?
> > 
> > Adding backlight maintainers.
> > 
> 
> Great point!  I was not aware of the backlight subsystem.
> 
> Looks like I need to create a back light version as well.
> 
> Like the lp8788 did since this can be used as a LED driver beyond
> display back lighting.

No, definitely not two drivers for lp8788 hardware.

If that does not yet exist... you want to create glue layer to be able
to use LED as a display backlight. (It may already exist, no idea).

...

Actually or maybe a LED trigger. Just set LED's trigger to "this is
display backlight".

Other solutions are acceptable, but not two hardware drivers.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Jacek Anaszewski]

Hi Dan and Pavel,

On 11/15/2017 11:23 PM, Pavel Machek wrote:
> Hi!
> 
>>> Shouldn't the driver be targeted for backlight subsystem then?
>>>
>>> Adding backlight maintainers.
>>>
>>
>> Great point!  I was not aware of the backlight subsystem.
>>
>> Looks like I need to create a back light version as well.
>>
>> Like the lp8788 did since this can be used as a LED driver beyond
>> display back lighting.
> 
> No, definitely not two drivers for lp8788 hardware.

I agree.

> If that does not yet exist... you want to create glue layer to be able
> to use LED as a display backlight. (It may already exist, no idea).
> 
> ...
> 
> Actually or maybe a LED trigger. Just set LED's trigger to "this is
> display backlight".

There is one already:

drivers/leds/trigger/ledtrig-backlight.c

It adds a LED class device to the fb_notifier_list
(drivers/video/fbdev/core/fb_notify.c)

using fb_register_client(). The same is used in
backlight_device_register (drivers/video/backlight/backlight.c).

Actually why do you want to have this driver in the LED subsystem,
if it is advertised as "designed for LCD display backlighting"?

As a side note I can say that I've been always wondering why the two
subsystems for similar type of hardware.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Dan Murphy]

Jacek

On 11/16/2017 02:14 PM, Jacek Anaszewski wrote:
> Hi Dan and Pavel,
> 
> On 11/15/2017 11:23 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> Shouldn't the driver be targeted for backlight subsystem then?
>>>>
>>>> Adding backlight maintainers.
>>>>
>>>
>>> Great point!  I was not aware of the backlight subsystem.
>>>
>>> Looks like I need to create a back light version as well.
>>>
>>> Like the lp8788 did since this can be used as a LED driver beyond
>>> display back lighting.
>>
>> No, definitely not two drivers for lp8788 hardware.
> 
> I agree.
> 
>> If that does not yet exist... you want to create glue layer to be able
>> to use LED as a display backlight. (It may already exist, no idea).
>>
>> ...
>>
>> Actually or maybe a LED trigger. Just set LED's trigger to "this is
>> display backlight".
> 
> There is one already:
> 
> drivers/leds/trigger/ledtrig-backlight.c
> 
> It adds a LED class device to the fb_notifier_list
> (drivers/video/fbdev/core/fb_notify.c)
> 
> using fb_register_client(). The same is used in
> backlight_device_register (drivers/video/backlight/backlight.c).
> 
> Actually why do you want to have this driver in the LED subsystem,
> if it is advertised as "designed for LCD display backlighting"?

Well this is also advertised as a driver for Smart phone and tablet devices.  And having worked with the Android lighting
solutions this is the preferred subsystem for Android.  The Android OS manages the led brightness based on ALS values and in
turn calls into the driver to control the brightness register through the vendor provided lighting HAL.

I am going to look at the backlight source to figure out how to get the same functionality using the backlight subsystem.
Otherwise I will plug in this driver to the backlight subsystem through the notifier.

> 
> As a side note I can say that I've been always wondering why the two
> subsystems for similar type of hardware.

This is my worry too.  Why do we need both subsystems to do the same thing?

I don't see either having one advantage over the other.


Dan
> 


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

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Jingoo Han]

On Thursday, November 16, 2017 4:42 PM, Dan Murphy wrote:
> 
> Jacek
> 
> On 11/16/2017 02:14 PM, Jacek Anaszewski wrote:
> > Hi Dan and Pavel,
> >
> > On 11/15/2017 11:23 PM, Pavel Machek wrote:
> >> Hi!
> >>
> >>>> Shouldn't the driver be targeted for backlight subsystem then?
> >>>>
> >>>> Adding backlight maintainers.
> >>>>
> >>>
> >>> Great point!  I was not aware of the backlight subsystem.
> >>>
> >>> Looks like I need to create a back light version as well.
> >>>
> >>> Like the lp8788 did since this can be used as a LED driver beyond
> >>> display back lighting.
> >>
> >> No, definitely not two drivers for lp8788 hardware.
> >
> > I agree.
> >
> >> If that does not yet exist... you want to create glue layer to be able
> >> to use LED as a display backlight. (It may already exist, no idea).
> >>
> >> ...
> >>
> >> Actually or maybe a LED trigger. Just set LED's trigger to "this is
> >> display backlight".
> >
> > There is one already:
> >
> > drivers/leds/trigger/ledtrig-backlight.c
> >
> > It adds a LED class device to the fb_notifier_list
> > (drivers/video/fbdev/core/fb_notify.c)
> >
> > using fb_register_client(). The same is used in
> > backlight_device_register (drivers/video/backlight/backlight.c).
> >
> > Actually why do you want to have this driver in the LED subsystem,
> > if it is advertised as "designed for LCD display backlighting"?
> 
> Well this is also advertised as a driver for Smart phone and tablet
> devices.  And having worked with the Android lighting
> solutions this is the preferred subsystem for Android.  The Android OS
> manages the led brightness based on ALS values and in
> turn calls into the driver to control the brightness register through the
> vendor provided lighting HAL.
> 
> I am going to look at the backlight source to figure out how to get the
> same functionality using the backlight subsystem.
> Otherwise I will plug in this driver to the backlight subsystem through
> the notifier.


I also developed Android devices for a long time.
I think that modifying HAL will not be difficult.

Also, backlight subsystem will be similar with LED subsystem.
You can grasp it easily.

Best regards,
Jingoo Han

> 
> >
> > As a side note I can say that I've been always wondering why the two
> > subsystems for similar type of hardware.
> 
> This is my worry too.  Why do we need both subsystems to do the same
thing?
> 
> I don't see either having one advantage over the other.
> 
> 
> Dan
> >
> 
> 
> --
> ------------------
> Dan Murphy

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1651 bytes --]

Hi!

> >> If that does not yet exist... you want to create glue layer to be able
> >> to use LED as a display backlight. (It may already exist, no idea).
> >>
> >> ...
> >>
> >> Actually or maybe a LED trigger. Just set LED's trigger to "this is
> >> display backlight".
> > 
> > There is one already:
> > 
> > drivers/leds/trigger/ledtrig-backlight.c
> > 
> > It adds a LED class device to the fb_notifier_list
> > (drivers/video/fbdev/core/fb_notify.c)
> > 
> > using fb_register_client(). The same is used in
> > backlight_device_register (drivers/video/backlight/backlight.c).
> > 
> > Actually why do you want to have this driver in the LED subsystem,
> > if it is advertised as "designed for LCD display backlighting"?
> 
> Well this is also advertised as a driver for Smart phone and tablet devices.  And having worked with the Android lighting
> solutions this is the preferred subsystem for Android.  The Android OS manages the led brightness based on ALS values and in
> turn calls into the driver to control the brightness register through the vendor provided lighting HAL.
> 

Well.. if it can control other LEDs than just backlight, I believe it
can stay in the LED subsystem.

> I am going to look at the backlight source to figure out how to get the same functionality using the backlight subsystem.
> Otherwise I will plug in this driver to the backlight subsystem through the notifier.
> 

The backlight trigger should be ok.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Jingoo Han]

On Friday, November 17, 2017 6:21 AM, Pavel Machek wrote:
> 
> Hi!
> 
> > >> If that does not yet exist... you want to create glue layer to be
> able
> > >> to use LED as a display backlight. (It may already exist, no idea).
> > >>
> > >> ...
> > >>
> > >> Actually or maybe a LED trigger. Just set LED's trigger to "this is
> > >> display backlight".
> > >
> > > There is one already:
> > >
> > > drivers/leds/trigger/ledtrig-backlight.c
> > >
> > > It adds a LED class device to the fb_notifier_list
> > > (drivers/video/fbdev/core/fb_notify.c)
> > >
> > > using fb_register_client(). The same is used in
> > > backlight_device_register (drivers/video/backlight/backlight.c).
> > >
> > > Actually why do you want to have this driver in the LED subsystem,
> > > if it is advertised as "designed for LCD display backlighting"?
> >
> > Well this is also advertised as a driver for Smart phone and tablet
> devices.  And having worked with the Android lighting
> > solutions this is the preferred subsystem for Android.  The Android OS
> manages the led brightness based on ALS values and in
> > turn calls into the driver to control the brightness register through
> the vendor provided lighting HAL.
> >
> 
> Well.. if it can control other LEDs than just backlight, I believe it
> can stay in the LED subsystem.

I also agree with your opinion.

Best regards,
Jingoo Han

> 
> > I am going to look at the backlight source to figure out how to get the
> same functionality using the backlight subsystem.
> > Otherwise I will plug in this driver to the backlight subsystem through
> the notifier.
> >
> 
> The backlight trigger should be ok.
> 
> Best regards,
>
Pavel
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Dan Murphy]

Pavel and Jacek

On 11/17/2017 10:30 AM, Jingoo Han wrote:
> On Friday, November 17, 2017 6:21 AM, Pavel Machek wrote:
>>
>> Hi!
>>
>>>>> If that does not yet exist... you want to create glue layer to be
>> able
>>>>> to use LED as a display backlight. (It may already exist, no idea).
>>>>>
>>>>> ...
>>>>>
>>>>> Actually or maybe a LED trigger. Just set LED's trigger to "this is
>>>>> display backlight".
>>>>
>>>> There is one already:
>>>>
>>>> drivers/leds/trigger/ledtrig-backlight.c
>>>>
>>>> It adds a LED class device to the fb_notifier_list
>>>> (drivers/video/fbdev/core/fb_notify.c)
>>>>
>>>> using fb_register_client(). The same is used in
>>>> backlight_device_register (drivers/video/backlight/backlight.c).
>>>>
>>>> Actually why do you want to have this driver in the LED subsystem,
>>>> if it is advertised as "designed for LCD display backlighting"?
>>>
>>> Well this is also advertised as a driver for Smart phone and tablet
>> devices.  And having worked with the Android lighting
>>> solutions this is the preferred subsystem for Android.  The Android OS
>> manages the led brightness based on ALS values and in
>>> turn calls into the driver to control the brightness register through
>> the vendor provided lighting HAL.
>>>
>>
>> Well.. if it can control other LEDs than just backlight, I believe it
>> can stay in the LED subsystem.
> 
> I also agree with your opinion.

I will make the necessary changes for v4.

Just a heads up I won't be posting v4 until after the US holiday so you
don't think I abandoned everything.

Dan

> 
> Best regards,
> Jingoo Han
> 
>>
>>> I am going to look at the backlight source to figure out how to get the
>> same functionality using the backlight subsystem.
>>> Otherwise I will plug in this driver to the backlight subsystem through
>> the notifier.
>>>
>>
>> The backlight trigger should be ok.
>>
>> Best regards,
>>
> Pavel
>> --
>> (english) http://www.livejournal.com/~pavelmachek
>> (cesky, pictures)
>> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> 


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

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

Hi!

> >> Well.. if it can control other LEDs than just backlight, I believe it
> >> can stay in the LED subsystem.
> > 
> > I also agree with your opinion.
> 
> I will make the necessary changes for v4.

I'm not sure if you need to make any changes. Just add default trigger
to the dts and you should be done.

> Just a heads up I won't be posting v4 until after the US holiday so you
> don't think I abandoned everything.

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Dan Murphy]

Pavel

On 11/17/2017 05:58 PM, Pavel Machek wrote:
> Hi!
> 
>>>> Well.. if it can control other LEDs than just backlight, I believe it
>>>> can stay in the LED subsystem.
>>>
>>> I also agree with your opinion.
>>
>> I will make the necessary changes for v4.
> 
> I'm not sure if you need to make any changes. Just add default trigger
> to the dts and you should be done.
> 

Sorry what I meant was changes for the additional v3 comments.

Dan

>> Just a heads up I won't be posting v4 until after the US holiday so you
>> don't think I abandoned everything.
> 
> :-)
> 									Pavel
> 


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

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Rob Herring]

On Wed, Nov 15, 2017 at 01:42:02PM -0600, Dan Murphy wrote:
> This adds the devicetree bindings for the LM3692x
> I2C LED string driver.
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v3 - No changes
> v2 - No changes - https://patchwork.kernel.org/patch/10056677/
> 
>  .../devicetree/bindings/leds/leds-lm3692x.txt      | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3692x.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
> new file mode 100644
> index 000000000000..cfef67bd4100
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
> @@ -0,0 +1,28 @@
> +* Texas Instruments - LM3692x Highly Efficient Dual-String White LED Driver
> +
> +The LM36922 is an ultra-compact, highly efficient,
> +two string white-LED driver designed for LCD display
> +backlighting.
> +
> +Required properties:
> +	- compatible:
> +		"ti,lm3692x"

Don't use wildcards in compatible strings.

> +	- reg -  I2C slave address
> +
> +Optional properties:
> +	- label - Used for naming LEDs
> +	- enable-gpio - gpio pin to enable/disable the device.

enable-gpios is the preferred form.

> +	- supply - "vled" - LED supply

vled-supply - ...

> +
> +Example:
> +
> +leds: leds@6 {
> +	compatible = "ti,lm3692x";
> +	reg = <0x36>;
> +	label = "backlight_cluster";
> +	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> +	vled-supply = <&vbatt>;
> +}
> +
> +For more product information please see the link below:
> +http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
> -- 
> 2.15.0.124.g7668cbc60
> 

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Dan Murphy]

Rob

Thanks for the review

On 11/16/2017 09:41 AM, Rob Herring wrote:
> On Wed, Nov 15, 2017 at 01:42:02PM -0600, Dan Murphy wrote:
>> This adds the devicetree bindings for the LM3692x
>> I2C LED string driver.
>>
>> Acked-by: Pavel Machek <pavel@ucw.cz>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v3 - No changes
>> v2 - No changes - https://patchwork.kernel.org/patch/10056677/
>>
>>  .../devicetree/bindings/leds/leds-lm3692x.txt      | 28 ++++++++++++++++++++++
>>  1 file changed, 28 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>> new file mode 100644
>> index 000000000000..cfef67bd4100
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>> @@ -0,0 +1,28 @@
>> +* Texas Instruments - LM3692x Highly Efficient Dual-String White LED Driver
>> +
>> +The LM36922 is an ultra-compact, highly efficient,
>> +two string white-LED driver designed for LCD display
>> +backlighting.
>> +
>> +Required properties:
>> +	- compatible:
>> +		"ti,lm3692x"
> 
> Don't use wildcards in compatible strings.

Do you mean to remove the x?  How do we denote a family of parts then?

> 
>> +	- reg -  I2C slave address
>> +
>> +Optional properties:
>> +	- label - Used for naming LEDs
>> +	- enable-gpio - gpio pin to enable/disable the device.
> 
> enable-gpios is the preferred form.

ack

> 
>> +	- supply - "vled" - LED supply
> 
> vled-supply - ...
> 

ack

>> +
>> +Example:
>> +
>> +leds: leds@6 {
>> +	compatible = "ti,lm3692x";
>> +	reg = <0x36>;
>> +	label = "backlight_cluster";
>> +	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>> +	vled-supply = <&vbatt>;
>> +}
>> +
>> +For more product information please see the link below:
>> +http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
>> -- 
>> 2.15.0.124.g7668cbc60
>>


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

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Rob Herring]

On Thu, Nov 16, 2017 at 9:45 AM, Dan Murphy <dmurphy@ti.com> wrote:
> Rob
>
> Thanks for the review
>
> On 11/16/2017 09:41 AM, Rob Herring wrote:
>> On Wed, Nov 15, 2017 at 01:42:02PM -0600, Dan Murphy wrote:
>>> This adds the devicetree bindings for the LM3692x
>>> I2C LED string driver.
>>>
>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>>
>>> v3 - No changes
>>> v2 - No changes - https://patchwork.kernel.org/patch/10056677/
>>>
>>>  .../devicetree/bindings/leds/leds-lm3692x.txt      | 28 ++++++++++++++++++++++
>>>  1 file changed, 28 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>>> new file mode 100644
>>> index 000000000000..cfef67bd4100
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
>>> @@ -0,0 +1,28 @@
>>> +* Texas Instruments - LM3692x Highly Efficient Dual-String White LED Driver
>>> +
>>> +The LM36922 is an ultra-compact, highly efficient,
>>> +two string white-LED driver designed for LCD display
>>> +backlighting.
>>> +
>>> +Required properties:
>>> +    - compatible:
>>> +            "ti,lm3692x"
>>
>> Don't use wildcards in compatible strings.
>
> Do you mean to remove the x?  How do we denote a family of parts then?

Yes. With a compatible string for each one. You can have multiple
compatible strings if some are a superset of others.

Rob

dts: fun with chip names Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

Hi!

> >> +Required properties:
> >> +	- compatible:
> >> +		"ti,lm3692x"
> > 
> > Don't use wildcards in compatible strings.
> 
> Do you mean to remove the x?  How do we denote a family of parts
> then?

I guess you should specify the exact chip.

Which will present interesting problem for me on Nokia N9/N950; in one
case, compatible chip is produced by two companies, and it looks like
some machines have one and some have the other; but we'd like to share
the dts as user has no chance telling them apart (and it is not
important, anyway).

In second case, chip is refered as APDS990X and I don't know where to
get more exact data.

									Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: dts: fun with chip names Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Rob Herring]

On Thu, Nov 16, 2017 at 2:11 PM, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
>> >> +Required properties:
>> >> +  - compatible:
>> >> +          "ti,lm3692x"
>> >
>> > Don't use wildcards in compatible strings.
>>
>> Do you mean to remove the x?  How do we denote a family of parts
>> then?
>
> I guess you should specify the exact chip.
>
> Which will present interesting problem for me on Nokia N9/N950; in one
> case, compatible chip is produced by two companies, and it looks like
> some machines have one and some have the other; but we'd like to share
> the dts as user has no chance telling them apart (and it is not
> important, anyway).
>
> In second case, chip is refered as APDS990X and I don't know where to
> get more exact data.

There's always exceptions to rules. Just make the case for it. IIRC,
there was the same case for the BT chip.

Rob

Re: dts: fun with chip names Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Dan Murphy]

On 11/16/2017 02:36 PM, Rob Herring wrote:
> On Thu, Nov 16, 2017 at 2:11 PM, Pavel Machek <pavel@ucw.cz> wrote:
>> Hi!
>>
>>>>> +Required properties:
>>>>> +  - compatible:
>>>>> +          "ti,lm3692x"
>>>>
>>>> Don't use wildcards in compatible strings.
>>>
>>> Do you mean to remove the x?  How do we denote a family of parts
>>> then?
>>
>> I guess you should specify the exact chip.
>>
>> Which will present interesting problem for me on Nokia N9/N950; in one
>> case, compatible chip is produced by two companies, and it looks like
>> some machines have one and some have the other; but we'd like to share
>> the dts as user has no chance telling them apart (and it is not
>> important, anyway).
>>
>> In second case, chip is refered as APDS990X and I don't know where to
>> get more exact data.
> 
> There's always exceptions to rules. Just make the case for it. IIRC,
> there was the same case for the BT chip.
> 

Not sure how I got looped into the dts fun.  But I am going to call out the specific chips out in the
compatible strings.  There are currently only 2 devices supported by this driver.  The delta
between the chips is one supports a third string of LEDs.  1 bit in 1 register.

Dan

> Rob
> 


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

Re: [PATCH v3 1/2] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Jacek Anaszewski]

Hi Dan,

On 11/15/2017 08:42 PM, Dan Murphy wrote:
> This adds the devicetree bindings for the LM3692x
> I2C LED string driver.
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v3 - No changes
> v2 - No changes - https://patchwork.kernel.org/patch/10056677/
> 
>  .../devicetree/bindings/leds/leds-lm3692x.txt      | 28 ++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3692x.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
> new file mode 100644
> index 000000000000..cfef67bd4100
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
> @@ -0,0 +1,28 @@
> +* Texas Instruments - LM3692x Highly Efficient Dual-String White LED Driver
> +
> +The LM36922 is an ultra-compact, highly efficient,
> +two string white-LED driver designed for LCD display
> +backlighting.
> +
> +Required properties:
> +	- compatible:
> +		"ti,lm3692x"
> +	- reg -  I2C slave address
> +
> +Optional properties:
> +	- label - Used for naming LEDs
> +	- enable-gpio - gpio pin to enable/disable the device.
> +	- supply - "vled" - LED supply
> +
> +Example:
> +
> +leds: leds@6 {
> +	compatible = "ti,lm3692x";
> +	reg = <0x36>;
> +	label = "backlight_cluster";

Documentation/devicetree/bindings/leds/common.txt states that
each LED component has to be represented as a child node
(for consistency it should apply also to the arrangements with one
LED).

> +	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> +	vled-supply = <&vbatt>;
> +}
> +
> +For more product information please see the link below:
> +http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
> 

-- 
Best regards,
Jacek Anaszewski