[PATCH v4 1/2] dt-bindings: leds: Add bindings for lm3697 driver

[PATCH v4 1/2] dt-bindings: leds: Add bindings for lm3697 driver

[Dan Murphy]

Add the device tree bindings for the lm3697
LED driver for backlighting and display.

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

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

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
new file mode 100644
index 000000000000..bcd67d53c889
--- /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 2 controlled by control bank A and HVLED string 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
-- 
2.17.0.582.gccdcbd54c

[PATCH v4 2/2] leds: lm3697: Introduce the lm3697 driver

[Dan Murphy]

Introduce the lm3697 LED driver for
backlighting and display.

Datasheet location:
http://www.ti.com/lit/ds/symlink/lm3697.pdf

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

v4 - Made modifications to the control bank routine to accomodate the DT changes
https://lore.kernel.org/patchwork/patch/974811/
v3 - Add code to support led-sources and control bank configuration also fix
other miscellaneous issues reported- https://lore.kernel.org/patchwork/patch/972336/
v2 - Removed unneed 'of' calls in dt_parse, fixed comment, fixed checkpatch
error, and change led registration - https://lore.kernel.org/patchwork/patch/971327/

 drivers/leds/Kconfig       |   9 +
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-lm3697.c | 381 +++++++++++++++++++++++++++++++++++++
 3 files changed, 391 insertions(+)
 create mode 100644 drivers/leds/leds-lm3697.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 44097a3e0fcc..784cbe375724 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -151,6 +151,15 @@ config LEDS_LM3642
 	  converter plus 1.5A constant current driver for a high-current
 	  white LED.
 
+config LEDS_LM3697
+	tristate "LED support for LM3697 Chip"
+	depends on LEDS_CLASS && I2C
+	select REGMAP_I2C
+	help
+	  This option enables support for LEDs connected to LM3697.
+	  The LM3697 is an 11 bit high performance backlight driver for
+	  keypad and display lighting.
+
 config LEDS_LM3692X
 	tristate "LED support for LM3692x Chips"
 	depends on LEDS_CLASS && I2C && OF
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 420b5d2cfa62..2813f089f3db 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_LEDS_LOCOMO)		+= leds-locomo.o
 obj-$(CONFIG_LEDS_LM3530)		+= leds-lm3530.o
 obj-$(CONFIG_LEDS_LM3533)		+= leds-lm3533.o
 obj-$(CONFIG_LEDS_LM3642)		+= leds-lm3642.o
+obj-$(CONFIG_LEDS_LM3697)		+= leds-lm3697.o
 obj-$(CONFIG_LEDS_MIKROTIK_RB532)	+= leds-rb532.o
 obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
 obj-$(CONFIG_LEDS_NET48XX)		+= leds-net48xx.o
diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
new file mode 100644
index 000000000000..390c6a928899
--- /dev/null
+++ b/drivers/leds/leds-lm3697.c
@@ -0,0 +1,381 @@
+// SPDX-License-Identifier: GPL-2.0
+// TI LM3697 LED chip family driver
+// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <uapi/linux/uleds.h>
+
+#define LM3697_REV			0x0
+#define LM3697_RESET			0x1
+#define LM3697_OUTPUT_CONFIG		0x10
+#define LM3697_CTRL_A_RAMP		0x11
+#define LM3697_CTRL_B_RAMP		0x12
+#define LM3697_CTRL_A_B_RT_RAMP		0x13
+#define LM3697_CTRL_A_B_RAMP_CFG	0x14
+#define LM3697_CTRL_A_B_BRT_CFG		0x16
+#define LM3697_CTRL_A_FS_CURR_CFG	0x17
+#define LM3697_CTRL_B_FS_CURR_CFG	0x18
+#define LM3697_PWM_CFG			0x1c
+#define LM3697_CTRL_A_BRT_LSB		0x20
+#define LM3697_CTRL_A_BRT_MSB		0x21
+#define LM3697_CTRL_B_BRT_LSB		0x22
+#define LM3697_CTRL_B_BRT_MSB		0x23
+#define LM3697_CTRL_ENABLE		0x24
+
+#define LM3697_SW_RESET		BIT(0)
+
+#define LM3697_CTRL_A_EN	BIT(0)
+#define LM3697_CTRL_B_EN	BIT(1)
+#define LM3697_CTRL_A_B_EN	(LM3697_CTRL_A_EN | LM3697_CTRL_B_EN)
+
+#define LM3697_MAX_LED_STRINGS	3
+
+#define LM3697_CONTROL_A	0
+#define LM3697_CONTROL_B	1
+#define LM3697_HVLED		1
+
+/**
+ * struct lm3697_led -
+ * @hvled_strings - Array of LED strings associated with a control bank
+ * @label - LED label
+ * @led_dev - LED class device
+ * @priv - Pointer to the device struct
+ * @control_bank - Control bank the LED is associated to. 0 is control bank A
+ *		   1 is control bank B.
+ */
+struct lm3697_led {
+	u32 hvled_strings[LM3697_MAX_LED_STRINGS];
+	char label[LED_MAX_NAME_SIZE];
+	struct led_classdev led_dev;
+	struct lm3697 *priv;
+	int control_bank;
+};
+
+/**
+ * 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.
+ */
+struct lm3697 {
+	struct gpio_desc *enable_gpio;
+	struct regulator *regulator;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct device *dev;
+	struct mutex lock;
+	struct lm3697_led leds[];
+};
+
+static const struct reg_default lm3697_reg_defs[] = {
+	{LM3697_OUTPUT_CONFIG, 0x6},
+	{LM3697_CTRL_A_RAMP, 0x0},
+	{LM3697_CTRL_B_RAMP, 0x0},
+	{LM3697_CTRL_A_B_RT_RAMP, 0x0},
+	{LM3697_CTRL_A_B_RAMP_CFG, 0x0},
+	{LM3697_CTRL_A_B_BRT_CFG, 0x0},
+	{LM3697_CTRL_A_FS_CURR_CFG, 0x13},
+	{LM3697_CTRL_B_FS_CURR_CFG, 0x13},
+	{LM3697_PWM_CFG, 0xc},
+	{LM3697_CTRL_A_BRT_LSB, 0x0},
+	{LM3697_CTRL_A_BRT_MSB, 0x0},
+	{LM3697_CTRL_B_BRT_LSB, 0x0},
+	{LM3697_CTRL_B_BRT_MSB, 0x0},
+	{LM3697_CTRL_ENABLE, 0x0},
+};
+
+static const struct regmap_config lm3697_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = LM3697_CTRL_ENABLE,
+	.reg_defaults = lm3697_reg_defs,
+	.num_reg_defaults = ARRAY_SIZE(lm3697_reg_defs),
+	.cache_type = REGCACHE_FLAT,
+};
+
+static int lm3697_brightness_set(struct led_classdev *led_cdev,
+				enum led_brightness brt_val)
+{
+	struct lm3697_led *led = container_of(led_cdev, struct lm3697_led,
+					      led_dev);
+	int brt_msb_reg, brt_lsb_reg, ctrl_en_val;
+	int led_brightness_lsb = (brt_val >> 5);
+	int ret;
+
+	mutex_lock(&led->priv->lock);
+
+	if (led->control_bank == LM3697_CONTROL_A) {
+		brt_msb_reg = LM3697_CTRL_A_BRT_MSB;
+		brt_lsb_reg = LM3697_CTRL_A_BRT_LSB;
+		ctrl_en_val = LM3697_CTRL_A_EN;
+	} else {
+		brt_msb_reg = LM3697_CTRL_B_BRT_MSB;
+		brt_lsb_reg = LM3697_CTRL_B_BRT_LSB;
+		ctrl_en_val = LM3697_CTRL_B_EN;
+	}
+
+	if (brt_val == LED_OFF)
+		ret = regmap_update_bits(led->priv->regmap, LM3697_CTRL_ENABLE,
+					 ctrl_en_val, ~ctrl_en_val);
+	else
+		ret = regmap_update_bits(led->priv->regmap, LM3697_CTRL_ENABLE,
+					 ctrl_en_val, ctrl_en_val);
+
+	if (ret) {
+		dev_err(&led->priv->client->dev, "Cannot write CTRL enable\n");
+		goto out;
+	}
+
+	ret = regmap_write(led->priv->regmap, brt_lsb_reg, led_brightness_lsb);
+	if (ret) {
+		dev_err(&led->priv->client->dev, "Cannot write LSB\n");
+		goto out;
+	}
+
+	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:
+	mutex_unlock(&led->priv->lock);
+	return ret;
+}
+
+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];
+
+	for (i = 0; i < LM3697_MAX_LED_STRINGS; i++) {
+		if (led->hvled_strings[i] == LM3697_HVLED)
+			control_bank_config |= 1 << i;
+	}
+
+	ret = regmap_write(priv->regmap, LM3697_OUTPUT_CONFIG,
+			   control_bank_config);
+	if (ret)
+		dev_err(&priv->client->dev, "Cannot write OUTPUT config\n");
+
+	return ret;
+}
+
+static int lm3697_init(struct lm3697 *priv)
+{
+	int ret;
+
+	if (priv->enable_gpio) {
+		gpiod_direction_output(priv->enable_gpio, 1);
+	} else {
+		ret = regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET);
+		if (ret) {
+			dev_err(&priv->client->dev, "Cannot reset the device\n");
+			goto out;
+		}
+	}
+
+	ret = regmap_write(priv->regmap, LM3697_CTRL_ENABLE, 0x0);
+	if (ret) {
+		dev_err(&priv->client->dev, "Cannot write ctrl enable\n");
+		goto out;
+	}
+
+	ret = lm3697_set_control_bank(priv);
+	if (ret)
+		dev_err(&priv->client->dev, "Setting the CRTL bank failed\n");
+
+out:
+	return ret;
+}
+
+static int lm3697_probe_dt(struct lm3697 *priv)
+{
+	struct fwnode_handle *child = NULL;
+	struct lm3697_led *led;
+	const char *name;
+	int control_bank;
+	size_t i = 0;
+	int ret;
+
+	priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev,
+						   "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(priv->enable_gpio)) {
+		ret = PTR_ERR(priv->enable_gpio);
+		dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n",
+			ret);
+		return ret;
+	}
+
+	priv->regulator = devm_regulator_get(&priv->client->dev, "vled");
+	if (IS_ERR(priv->regulator))
+		priv->regulator = NULL;
+
+	device_for_each_child_node(priv->dev, child) {
+		ret = fwnode_property_read_u32(child, "reg", &control_bank);
+		if (ret) {
+			dev_err(&priv->client->dev, "reg property missing\n");
+			fwnode_handle_put(child);
+			goto child_out;
+		}
+
+		if (control_bank > LM3697_CONTROL_B) {
+			dev_err(&priv->client->dev, "reg property is invalid\n");
+			ret = -EINVAL;
+			fwnode_handle_put(child);
+			goto child_out;
+		}
+
+		led = &priv->leds[i];
+		led->control_bank = control_bank;
+		ret = fwnode_property_read_u32_array(child, "led-sources",
+						    led->hvled_strings,
+						    LM3697_MAX_LED_STRINGS);
+		if (ret) {
+			dev_err(&priv->client->dev, "led-sources property missing\n");
+			fwnode_handle_put(child);
+			goto child_out;
+		}
+
+		fwnode_property_read_string(child, "linux,default-trigger",
+					    &led->led_dev.default_trigger);
+
+		ret = fwnode_property_read_string(child, "label", &name);
+		if (ret)
+			snprintf(led->label, sizeof(led->label),
+				"%s::", priv->client->name);
+		else
+			snprintf(led->label, sizeof(led->label),
+				 "%s:%s", priv->client->name, name);
+
+		led->priv = priv;
+		led->led_dev.name = led->label;
+		led->led_dev.brightness_set_blocking = lm3697_brightness_set;
+
+		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
+		if (ret) {
+			dev_err(&priv->client->dev, "led register err: %d\n",
+				ret);
+			fwnode_handle_put(child);
+			goto child_out;
+		}
+
+		i++;
+	}
+
+child_out:
+	return ret;
+}
+
+static int lm3697_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct lm3697 *led;
+	int count;
+	int ret;
+
+	count = device_get_child_node_count(&client->dev);
+	if (!count) {
+		dev_err(&client->dev, "LEDs are not defined in device tree!");
+		return -ENODEV;
+	}
+
+	led = devm_kzalloc(&client->dev, struct_size(led, leds, count),
+			   GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	mutex_init(&led->lock);
+	i2c_set_clientdata(client, led);
+
+	led->client = client;
+	led->dev = &client->dev;
+	led->regmap = devm_regmap_init_i2c(client, &lm3697_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 = lm3697_probe_dt(led);
+	if (ret)
+		return ret;
+
+	ret = lm3697_init(led);
+
+	return ret;
+}
+
+static int lm3697_remove(struct i2c_client *client)
+{
+	struct lm3697 *led = i2c_get_clientdata(client);
+	int ret;
+
+	ret = regmap_update_bits(led->regmap, LM3697_CTRL_ENABLE,
+				 LM3697_CTRL_A_B_EN, 0);
+	if (ret) {
+		dev_err(&led->client->dev, "Failed to disable the device\n");
+		return ret;
+	}
+
+	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 lm3697_id[] = {
+	{ "lm3697", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lm3697_id);
+
+static const struct of_device_id of_lm3697_leds_match[] = {
+	{ .compatible = "ti,lm3697", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_lm3697_leds_match);
+
+static struct i2c_driver lm3697_driver = {
+	.driver = {
+		.name	= "lm3697",
+		.of_match_table = of_lm3697_leds_match,
+	},
+	.probe		= lm3697_probe,
+	.remove		= lm3697_remove,
+	.id_table	= lm3697_id,
+};
+module_i2c_driver(lm3697_driver);
+
+MODULE_DESCRIPTION("Texas Instruments LM3697 LED driver");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.0.582.gccdcbd54c

Re: [PATCH v4 1/2] dt-bindings: leds: Add bindings for lm3697 driver

[Jacek Anaszewski]

Hi Dan,

Thanks for the update.

I have one remark below.

On 08/16/2018 07:20 PM, 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>
> ---
> 
> 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
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
> new file mode 100644
> index 000000000000..bcd67d53c889
> --- /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 2 controlled by control bank A and HVLED string controlled by

s/1 and 2/1 and 3/
s/HVLED string controller/HVLED string 2 controlled/

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

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 1/2] dt-bindings: leds: Add bindings for lm3697 driver

[Dan Murphy]

Jacek

On 08/16/2018 01:37 PM, Jacek Anaszewski wrote:
> Hi Dan,
> 
> Thanks for the update.
> 
> I have one remark below.
> 
> On 08/16/2018 07:20 PM, 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>
>> ---
>>
>> 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
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
>> new file mode 100644
>> index 000000000000..bcd67d53c889
>> --- /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 2 controlled by control bank A and HVLED string controlled by
> 
> s/1 and 2/1 and 3/
> s/HVLED string controller/HVLED string 2 controlled/
> 

Thanks I will fix this up in v5 after the code review.

Dan

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


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

Re: [PATCH v4 2/2] leds: lm3697: Introduce the lm3697 driver

[Jacek Anaszewski]

Dan,

Thank you for the patch.

I didn't review DT parsing details in v3, but now I've produced
diff between v3 and v4 to check what has changed.

I'm quite surprised realizing that you're not validating
HVLED and control banks assignment, having in mind earlier
discussions and your concerns about numerous DT configurations
to check.

Is it on purpose?

Regardless of that - please find two nits below.

On 08/16/2018 07:20 PM, Dan Murphy wrote:
> Introduce the lm3697 LED driver for
> backlighting and display.
> 
> Datasheet location:
> http://www.ti.com/lit/ds/symlink/lm3697.pdf
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v4 - Made modifications to the control bank routine to accomodate the DT changes
> https://lore.kernel.org/patchwork/patch/974811/
> v3 - Add code to support led-sources and control bank configuration also fix
> other miscellaneous issues reported- https://lore.kernel.org/patchwork/patch/972336/
> v2 - Removed unneed 'of' calls in dt_parse, fixed comment, fixed checkpatch
> error, and change led registration - https://lore.kernel.org/patchwork/patch/971327/
> 
>  drivers/leds/Kconfig       |   9 +
>  drivers/leds/Makefile      |   1 +
>  drivers/leds/leds-lm3697.c | 381 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 391 insertions(+)
>  create mode 100644 drivers/leds/leds-lm3697.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 44097a3e0fcc..784cbe375724 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -151,6 +151,15 @@ config LEDS_LM3642
>  	  converter plus 1.5A constant current driver for a high-current
>  	  white LED.
>  
> +config LEDS_LM3697
> +	tristate "LED support for LM3697 Chip"
> +	depends on LEDS_CLASS && I2C
> +	select REGMAP_I2C
> +	help
> +	  This option enables support for LEDs connected to LM3697.
> +	  The LM3697 is an 11 bit high performance backlight driver for
> +	  keypad and display lighting.
> +
>  config LEDS_LM3692X
>  	tristate "LED support for LM3692x Chips"
>  	depends on LEDS_CLASS && I2C && OF
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 420b5d2cfa62..2813f089f3db 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_LEDS_LOCOMO)		+= leds-locomo.o
>  obj-$(CONFIG_LEDS_LM3530)		+= leds-lm3530.o
>  obj-$(CONFIG_LEDS_LM3533)		+= leds-lm3533.o
>  obj-$(CONFIG_LEDS_LM3642)		+= leds-lm3642.o
> +obj-$(CONFIG_LEDS_LM3697)		+= leds-lm3697.o
>  obj-$(CONFIG_LEDS_MIKROTIK_RB532)	+= leds-rb532.o
>  obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
>  obj-$(CONFIG_LEDS_NET48XX)		+= leds-net48xx.o
> diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
> new file mode 100644
> index 000000000000..390c6a928899
> --- /dev/null
> +++ b/drivers/leds/leds-lm3697.c
> @@ -0,0 +1,381 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// TI LM3697 LED chip family driver
> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/init.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +#include <uapi/linux/uleds.h>
> +
> +#define LM3697_REV			0x0
> +#define LM3697_RESET			0x1
> +#define LM3697_OUTPUT_CONFIG		0x10
> +#define LM3697_CTRL_A_RAMP		0x11
> +#define LM3697_CTRL_B_RAMP		0x12
> +#define LM3697_CTRL_A_B_RT_RAMP		0x13
> +#define LM3697_CTRL_A_B_RAMP_CFG	0x14
> +#define LM3697_CTRL_A_B_BRT_CFG		0x16
> +#define LM3697_CTRL_A_FS_CURR_CFG	0x17
> +#define LM3697_CTRL_B_FS_CURR_CFG	0x18
> +#define LM3697_PWM_CFG			0x1c
> +#define LM3697_CTRL_A_BRT_LSB		0x20
> +#define LM3697_CTRL_A_BRT_MSB		0x21
> +#define LM3697_CTRL_B_BRT_LSB		0x22
> +#define LM3697_CTRL_B_BRT_MSB		0x23
> +#define LM3697_CTRL_ENABLE		0x24
> +
> +#define LM3697_SW_RESET		BIT(0)
> +
> +#define LM3697_CTRL_A_EN	BIT(0)
> +#define LM3697_CTRL_B_EN	BIT(1)
> +#define LM3697_CTRL_A_B_EN	(LM3697_CTRL_A_EN | LM3697_CTRL_B_EN)
> +
> +#define LM3697_MAX_LED_STRINGS	3
> +
> +#define LM3697_CONTROL_A	0
> +#define LM3697_CONTROL_B	1
> +#define LM3697_HVLED		1

s/LM3697_HVLED/LM3697_HVLED_ASSIGNED/

It will be more informative in this form.

> +
> +/**
> + * struct lm3697_led -
> + * @hvled_strings - Array of LED strings associated with a control bank
> + * @label - LED label
> + * @led_dev - LED class device
> + * @priv - Pointer to the device struct
> + * @control_bank - Control bank the LED is associated to. 0 is control bank A
> + *		   1 is control bank B.
> + */
> +struct lm3697_led {
> +	u32 hvled_strings[LM3697_MAX_LED_STRINGS];
> +	char label[LED_MAX_NAME_SIZE];
> +	struct led_classdev led_dev;
> +	struct lm3697 *priv;
> +	int control_bank;
> +};
> +
> +/**
> + * 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.
> + */
> +struct lm3697 {
> +	struct gpio_desc *enable_gpio;
> +	struct regulator *regulator;
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct mutex lock;
> +	struct lm3697_led leds[];
> +};
> +
> +static const struct reg_default lm3697_reg_defs[] = {
> +	{LM3697_OUTPUT_CONFIG, 0x6},
> +	{LM3697_CTRL_A_RAMP, 0x0},
> +	{LM3697_CTRL_B_RAMP, 0x0},
> +	{LM3697_CTRL_A_B_RT_RAMP, 0x0},
> +	{LM3697_CTRL_A_B_RAMP_CFG, 0x0},
> +	{LM3697_CTRL_A_B_BRT_CFG, 0x0},
> +	{LM3697_CTRL_A_FS_CURR_CFG, 0x13},
> +	{LM3697_CTRL_B_FS_CURR_CFG, 0x13},
> +	{LM3697_PWM_CFG, 0xc},
> +	{LM3697_CTRL_A_BRT_LSB, 0x0},
> +	{LM3697_CTRL_A_BRT_MSB, 0x0},
> +	{LM3697_CTRL_B_BRT_LSB, 0x0},
> +	{LM3697_CTRL_B_BRT_MSB, 0x0},
> +	{LM3697_CTRL_ENABLE, 0x0},
> +};
> +
> +static const struct regmap_config lm3697_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +
> +	.max_register = LM3697_CTRL_ENABLE,
> +	.reg_defaults = lm3697_reg_defs,
> +	.num_reg_defaults = ARRAY_SIZE(lm3697_reg_defs),
> +	.cache_type = REGCACHE_FLAT,
> +};
> +
> +static int lm3697_brightness_set(struct led_classdev *led_cdev,
> +				enum led_brightness brt_val)
> +{
> +	struct lm3697_led *led = container_of(led_cdev, struct lm3697_led,
> +					      led_dev);
> +	int brt_msb_reg, brt_lsb_reg, ctrl_en_val;
> +	int led_brightness_lsb = (brt_val >> 5);
> +	int ret;
> +
> +	mutex_lock(&led->priv->lock);
> +
> +	if (led->control_bank == LM3697_CONTROL_A) {
> +		brt_msb_reg = LM3697_CTRL_A_BRT_MSB;
> +		brt_lsb_reg = LM3697_CTRL_A_BRT_LSB;
> +		ctrl_en_val = LM3697_CTRL_A_EN;
> +	} else {
> +		brt_msb_reg = LM3697_CTRL_B_BRT_MSB;
> +		brt_lsb_reg = LM3697_CTRL_B_BRT_LSB;
> +		ctrl_en_val = LM3697_CTRL_B_EN;
> +	}
> +
> +	if (brt_val == LED_OFF)
> +		ret = regmap_update_bits(led->priv->regmap, LM3697_CTRL_ENABLE,
> +					 ctrl_en_val, ~ctrl_en_val);
> +	else
> +		ret = regmap_update_bits(led->priv->regmap, LM3697_CTRL_ENABLE,
> +					 ctrl_en_val, ctrl_en_val);
> +
> +	if (ret) {
> +		dev_err(&led->priv->client->dev, "Cannot write CTRL enable\n");
> +		goto out;
> +	}
> +
> +	ret = regmap_write(led->priv->regmap, brt_lsb_reg, led_brightness_lsb);
> +	if (ret) {
> +		dev_err(&led->priv->client->dev, "Cannot write LSB\n");
> +		goto out;
> +	}
> +
> +	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:
> +	mutex_unlock(&led->priv->lock);
> +	return ret;
> +}
> +
> +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];
> +
> +	for (i = 0; i < LM3697_MAX_LED_STRINGS; i++) {
> +		if (led->hvled_strings[i] == LM3697_HVLED)
> +			control_bank_config |= 1 << i;
> +	}
> +
> +	ret = regmap_write(priv->regmap, LM3697_OUTPUT_CONFIG,
> +			   control_bank_config);
> +	if (ret)
> +		dev_err(&priv->client->dev, "Cannot write OUTPUT config\n");
> +
> +	return ret;
> +}
> +
> +static int lm3697_init(struct lm3697 *priv)
> +{
> +	int ret;
> +
> +	if (priv->enable_gpio) {
> +		gpiod_direction_output(priv->enable_gpio, 1);
> +	} else {
> +		ret = regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET);
> +		if (ret) {
> +			dev_err(&priv->client->dev, "Cannot reset the device\n");
> +			goto out;
> +		}
> +	}
> +
> +	ret = regmap_write(priv->regmap, LM3697_CTRL_ENABLE, 0x0);
> +	if (ret) {
> +		dev_err(&priv->client->dev, "Cannot write ctrl enable\n");
> +		goto out;
> +	}
> +
> +	ret = lm3697_set_control_bank(priv);
> +	if (ret)
> +		dev_err(&priv->client->dev, "Setting the CRTL bank failed\n");
> +
> +out:
> +	return ret;
> +}
> +
> +static int lm3697_probe_dt(struct lm3697 *priv)
> +{
> +	struct fwnode_handle *child = NULL;
> +	struct lm3697_led *led;
> +	const char *name;
> +	int control_bank;
> +	size_t i = 0;
> +	int ret;
> +
> +	priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev,
> +						   "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(priv->enable_gpio)) {
> +		ret = PTR_ERR(priv->enable_gpio);
> +		dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	priv->regulator = devm_regulator_get(&priv->client->dev, "vled");
> +	if (IS_ERR(priv->regulator))
> +		priv->regulator = NULL;
> +
> +	device_for_each_child_node(priv->dev, child) {
> +		ret = fwnode_property_read_u32(child, "reg", &control_bank);
> +		if (ret) {
> +			dev_err(&priv->client->dev, "reg property missing\n");
> +			fwnode_handle_put(child);
> +			goto child_out;
> +		}
> +
> +		if (control_bank > LM3697_CONTROL_B) {
> +			dev_err(&priv->client->dev, "reg property is invalid\n");
> +			ret = -EINVAL;
> +			fwnode_handle_put(child);
> +			goto child_out;
> +		}
> +
> +		led = &priv->leds[i];
> +		led->control_bank = control_bank;
> +		ret = fwnode_property_read_u32_array(child, "led-sources",
> +						    led->hvled_strings,
> +						    LM3697_MAX_LED_STRINGS);
> +		if (ret) {
> +			dev_err(&priv->client->dev, "led-sources property missing\n");
> +			fwnode_handle_put(child);
> +			goto child_out;
> +		}
> +
> +		fwnode_property_read_string(child, "linux,default-trigger",
> +					    &led->led_dev.default_trigger);
> +
> +		ret = fwnode_property_read_string(child, "label", &name);
> +		if (ret)
> +			snprintf(led->label, sizeof(led->label),
> +				"%s::", priv->client->name);
> +		else
> +			snprintf(led->label, sizeof(led->label),
> +				 "%s:%s", priv->client->name, name);
> +
> +		led->priv = priv;
> +		led->led_dev.name = led->label;
> +		led->led_dev.brightness_set_blocking = lm3697_brightness_set;
> +
> +		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
> +		if (ret) {
> +			dev_err(&priv->client->dev, "led register err: %d\n",
> +				ret);
> +			fwnode_handle_put(child);
> +			goto child_out;
> +		}
> +
> +		i++;
> +	}
> +
> +child_out:
> +	return ret;
> +}
> +
> +static int lm3697_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct lm3697 *led;
> +	int count;
> +	int ret;
> +
> +	count = device_get_child_node_count(&client->dev);
> +	if (!count) {
> +		dev_err(&client->dev, "LEDs are not defined in device tree!");
> +		return -ENODEV;
> +	}
> +
> +	led = devm_kzalloc(&client->dev, struct_size(led, leds, count),
> +			   GFP_KERNEL);
> +	if (!led)
> +		return -ENOMEM;
> +
> +	mutex_init(&led->lock);
> +	i2c_set_clientdata(client, led);
> +
> +	led->client = client;
> +	led->dev = &client->dev;
> +	led->regmap = devm_regmap_init_i2c(client, &lm3697_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 = lm3697_probe_dt(led);
> +	if (ret)
> +		return ret;
> +
> +	ret = lm3697_init(led);

Let's simplify it:

return lm3697_init(led);

> +
> +	return ret;
> +}
> +
> +static int lm3697_remove(struct i2c_client *client)
> +{
> +	struct lm3697 *led = i2c_get_clientdata(client);
> +	int ret;
> +
> +	ret = regmap_update_bits(led->regmap, LM3697_CTRL_ENABLE,
> +				 LM3697_CTRL_A_B_EN, 0);
> +	if (ret) {
> +		dev_err(&led->client->dev, "Failed to disable the device\n");
> +		return ret;
> +	}
> +
> +	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 lm3697_id[] = {
> +	{ "lm3697", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, lm3697_id);
> +
> +static const struct of_device_id of_lm3697_leds_match[] = {
> +	{ .compatible = "ti,lm3697", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_lm3697_leds_match);
> +
> +static struct i2c_driver lm3697_driver = {
> +	.driver = {
> +		.name	= "lm3697",
> +		.of_match_table = of_lm3697_leds_match,
> +	},
> +	.probe		= lm3697_probe,
> +	.remove		= lm3697_remove,
> +	.id_table	= lm3697_id,
> +};
> +module_i2c_driver(lm3697_driver);
> +
> +MODULE_DESCRIPTION("Texas Instruments LM3697 LED driver");
> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
> +MODULE_LICENSE("GPL v2");
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 2/2] leds: lm3697: Introduce the lm3697 driver

[Dan Murphy]

Jacek

On 08/16/2018 02:58 PM, Jacek Anaszewski wrote:
> Dan,
> 
> Thank you for the patch.
> 
> I didn't review DT parsing details in v3, but now I've produced
> diff between v3 and v4 to check what has changed.
> 
> I'm quite surprised realizing that you're not validating
> HVLED and control banks assignment, having in mind earlier
> discussions and your concerns about numerous DT configurations
> to check.
> 
> Is it on purpose?
> 

Yes.  It was on purpose.  After sleeping on it and going through the overall
control to HVLED assignments I realized the user will know quite quickly
that their configuration is messed up.

The suggestions actually simplified the code quite nicely which I am happier to have


> Regardless of that - please find two nits below.
> 
> On 08/16/2018 07:20 PM, Dan Murphy wrote:
>> Introduce the lm3697 LED driver for
>> backlighting and display.
>>
>> Datasheet location:
>> http://www.ti.com/lit/ds/symlink/lm3697.pdf
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v4 - Made modifications to the control bank routine to accomodate the DT changes
>> https://lore.kernel.org/patchwork/patch/974811/
>> v3 - Add code to support led-sources and control bank configuration also fix
>> other miscellaneous issues reported- https://lore.kernel.org/patchwork/patch/972336/
>> v2 - Removed unneed 'of' calls in dt_parse, fixed comment, fixed checkpatch
>> error, and change led registration - https://lore.kernel.org/patchwork/patch/971327/
>>
>>  drivers/leds/Kconfig       |   9 +
>>  drivers/leds/Makefile      |   1 +
>>  drivers/leds/leds-lm3697.c | 381 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 391 insertions(+)
>>  create mode 100644 drivers/leds/leds-lm3697.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index 44097a3e0fcc..784cbe375724 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -151,6 +151,15 @@ config LEDS_LM3642
>>  	  converter plus 1.5A constant current driver for a high-current
>>  	  white LED.
>>  
>> +config LEDS_LM3697
>> +	tristate "LED support for LM3697 Chip"
>> +	depends on LEDS_CLASS && I2C
>> +	select REGMAP_I2C
>> +	help
>> +	  This option enables support for LEDs connected to LM3697.
>> +	  The LM3697 is an 11 bit high performance backlight driver for
>> +	  keypad and display lighting.
>> +
>>  config LEDS_LM3692X
>>  	tristate "LED support for LM3692x Chips"
>>  	depends on LEDS_CLASS && I2C && OF
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index 420b5d2cfa62..2813f089f3db 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -19,6 +19,7 @@ obj-$(CONFIG_LEDS_LOCOMO)		+= leds-locomo.o
>>  obj-$(CONFIG_LEDS_LM3530)		+= leds-lm3530.o
>>  obj-$(CONFIG_LEDS_LM3533)		+= leds-lm3533.o
>>  obj-$(CONFIG_LEDS_LM3642)		+= leds-lm3642.o
>> +obj-$(CONFIG_LEDS_LM3697)		+= leds-lm3697.o
>>  obj-$(CONFIG_LEDS_MIKROTIK_RB532)	+= leds-rb532.o
>>  obj-$(CONFIG_LEDS_S3C24XX)		+= leds-s3c24xx.o
>>  obj-$(CONFIG_LEDS_NET48XX)		+= leds-net48xx.o
>> diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
>> new file mode 100644
>> index 000000000000..390c6a928899
>> --- /dev/null
>> +++ b/drivers/leds/leds-lm3697.c
>> @@ -0,0 +1,381 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// TI LM3697 LED chip family driver
>> +// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/init.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>> +#include <uapi/linux/uleds.h>
>> +
>> +#define LM3697_REV			0x0
>> +#define LM3697_RESET			0x1
>> +#define LM3697_OUTPUT_CONFIG		0x10
>> +#define LM3697_CTRL_A_RAMP		0x11
>> +#define LM3697_CTRL_B_RAMP		0x12
>> +#define LM3697_CTRL_A_B_RT_RAMP		0x13
>> +#define LM3697_CTRL_A_B_RAMP_CFG	0x14
>> +#define LM3697_CTRL_A_B_BRT_CFG		0x16
>> +#define LM3697_CTRL_A_FS_CURR_CFG	0x17
>> +#define LM3697_CTRL_B_FS_CURR_CFG	0x18
>> +#define LM3697_PWM_CFG			0x1c
>> +#define LM3697_CTRL_A_BRT_LSB		0x20
>> +#define LM3697_CTRL_A_BRT_MSB		0x21
>> +#define LM3697_CTRL_B_BRT_LSB		0x22
>> +#define LM3697_CTRL_B_BRT_MSB		0x23
>> +#define LM3697_CTRL_ENABLE		0x24
>> +
>> +#define LM3697_SW_RESET		BIT(0)
>> +
>> +#define LM3697_CTRL_A_EN	BIT(0)
>> +#define LM3697_CTRL_B_EN	BIT(1)
>> +#define LM3697_CTRL_A_B_EN	(LM3697_CTRL_A_EN | LM3697_CTRL_B_EN)
>> +
>> +#define LM3697_MAX_LED_STRINGS	3
>> +
>> +#define LM3697_CONTROL_A	0
>> +#define LM3697_CONTROL_B	1
>> +#define LM3697_HVLED		1
> 
> s/LM3697_HVLED/LM3697_HVLED_ASSIGNED/
> 
> It will be more informative in this form.

Ack

> 
>> +
>> +/**
>> + * struct lm3697_led -
>> + * @hvled_strings - Array of LED strings associated with a control bank
>> + * @label - LED label
>> + * @led_dev - LED class device
>> + * @priv - Pointer to the device struct
>> + * @control_bank - Control bank the LED is associated to. 0 is control bank A
>> + *		   1 is control bank B.
>> + */
>> +struct lm3697_led {
>> +	u32 hvled_strings[LM3697_MAX_LED_STRINGS];
>> +	char label[LED_MAX_NAME_SIZE];
>> +	struct led_classdev led_dev;
>> +	struct lm3697 *priv;
>> +	int control_bank;
>> +};
>> +
>> +/**
>> + * 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.
>> + */
>> +struct lm3697 {
>> +	struct gpio_desc *enable_gpio;
>> +	struct regulator *regulator;
>> +	struct i2c_client *client;
>> +	struct regmap *regmap;
>> +	struct device *dev;
>> +	struct mutex lock;
>> +	struct lm3697_led leds[];
>> +};
>> +
>> +static const struct reg_default lm3697_reg_defs[] = {
>> +	{LM3697_OUTPUT_CONFIG, 0x6},
>> +	{LM3697_CTRL_A_RAMP, 0x0},
>> +	{LM3697_CTRL_B_RAMP, 0x0},
>> +	{LM3697_CTRL_A_B_RT_RAMP, 0x0},
>> +	{LM3697_CTRL_A_B_RAMP_CFG, 0x0},
>> +	{LM3697_CTRL_A_B_BRT_CFG, 0x0},
>> +	{LM3697_CTRL_A_FS_CURR_CFG, 0x13},
>> +	{LM3697_CTRL_B_FS_CURR_CFG, 0x13},
>> +	{LM3697_PWM_CFG, 0xc},
>> +	{LM3697_CTRL_A_BRT_LSB, 0x0},
>> +	{LM3697_CTRL_A_BRT_MSB, 0x0},
>> +	{LM3697_CTRL_B_BRT_LSB, 0x0},
>> +	{LM3697_CTRL_B_BRT_MSB, 0x0},
>> +	{LM3697_CTRL_ENABLE, 0x0},
>> +};
>> +
>> +static const struct regmap_config lm3697_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +
>> +	.max_register = LM3697_CTRL_ENABLE,
>> +	.reg_defaults = lm3697_reg_defs,
>> +	.num_reg_defaults = ARRAY_SIZE(lm3697_reg_defs),
>> +	.cache_type = REGCACHE_FLAT,
>> +};
>> +
>> +static int lm3697_brightness_set(struct led_classdev *led_cdev,
>> +				enum led_brightness brt_val)
>> +{
>> +	struct lm3697_led *led = container_of(led_cdev, struct lm3697_led,
>> +					      led_dev);
>> +	int brt_msb_reg, brt_lsb_reg, ctrl_en_val;
>> +	int led_brightness_lsb = (brt_val >> 5);
>> +	int ret;
>> +
>> +	mutex_lock(&led->priv->lock);
>> +
>> +	if (led->control_bank == LM3697_CONTROL_A) {
>> +		brt_msb_reg = LM3697_CTRL_A_BRT_MSB;
>> +		brt_lsb_reg = LM3697_CTRL_A_BRT_LSB;
>> +		ctrl_en_val = LM3697_CTRL_A_EN;
>> +	} else {
>> +		brt_msb_reg = LM3697_CTRL_B_BRT_MSB;
>> +		brt_lsb_reg = LM3697_CTRL_B_BRT_LSB;
>> +		ctrl_en_val = LM3697_CTRL_B_EN;
>> +	}
>> +
>> +	if (brt_val == LED_OFF)
>> +		ret = regmap_update_bits(led->priv->regmap, LM3697_CTRL_ENABLE,
>> +					 ctrl_en_val, ~ctrl_en_val);
>> +	else
>> +		ret = regmap_update_bits(led->priv->regmap, LM3697_CTRL_ENABLE,
>> +					 ctrl_en_val, ctrl_en_val);
>> +
>> +	if (ret) {
>> +		dev_err(&led->priv->client->dev, "Cannot write CTRL enable\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = regmap_write(led->priv->regmap, brt_lsb_reg, led_brightness_lsb);
>> +	if (ret) {
>> +		dev_err(&led->priv->client->dev, "Cannot write LSB\n");
>> +		goto out;
>> +	}
>> +
>> +	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:
>> +	mutex_unlock(&led->priv->lock);
>> +	return ret;
>> +}
>> +
>> +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];
>> +
>> +	for (i = 0; i < LM3697_MAX_LED_STRINGS; i++) {
>> +		if (led->hvled_strings[i] == LM3697_HVLED)
>> +			control_bank_config |= 1 << i;
>> +	}
>> +
>> +	ret = regmap_write(priv->regmap, LM3697_OUTPUT_CONFIG,
>> +			   control_bank_config);
>> +	if (ret)
>> +		dev_err(&priv->client->dev, "Cannot write OUTPUT config\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static int lm3697_init(struct lm3697 *priv)
>> +{
>> +	int ret;
>> +
>> +	if (priv->enable_gpio) {
>> +		gpiod_direction_output(priv->enable_gpio, 1);
>> +	} else {
>> +		ret = regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET);
>> +		if (ret) {
>> +			dev_err(&priv->client->dev, "Cannot reset the device\n");
>> +			goto out;
>> +		}
>> +	}
>> +
>> +	ret = regmap_write(priv->regmap, LM3697_CTRL_ENABLE, 0x0);
>> +	if (ret) {
>> +		dev_err(&priv->client->dev, "Cannot write ctrl enable\n");
>> +		goto out;
>> +	}
>> +
>> +	ret = lm3697_set_control_bank(priv);
>> +	if (ret)
>> +		dev_err(&priv->client->dev, "Setting the CRTL bank failed\n");
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>> +static int lm3697_probe_dt(struct lm3697 *priv)
>> +{
>> +	struct fwnode_handle *child = NULL;
>> +	struct lm3697_led *led;
>> +	const char *name;
>> +	int control_bank;
>> +	size_t i = 0;
>> +	int ret;
>> +
>> +	priv->enable_gpio = devm_gpiod_get_optional(&priv->client->dev,
>> +						   "enable", GPIOD_OUT_LOW);
>> +	if (IS_ERR(priv->enable_gpio)) {
>> +		ret = PTR_ERR(priv->enable_gpio);
>> +		dev_err(&priv->client->dev, "Failed to get enable gpio: %d\n",
>> +			ret);
>> +		return ret;
>> +	}
>> +
>> +	priv->regulator = devm_regulator_get(&priv->client->dev, "vled");
>> +	if (IS_ERR(priv->regulator))
>> +		priv->regulator = NULL;
>> +
>> +	device_for_each_child_node(priv->dev, child) {
>> +		ret = fwnode_property_read_u32(child, "reg", &control_bank);
>> +		if (ret) {
>> +			dev_err(&priv->client->dev, "reg property missing\n");
>> +			fwnode_handle_put(child);
>> +			goto child_out;
>> +		}
>> +
>> +		if (control_bank > LM3697_CONTROL_B) {
>> +			dev_err(&priv->client->dev, "reg property is invalid\n");
>> +			ret = -EINVAL;
>> +			fwnode_handle_put(child);
>> +			goto child_out;
>> +		}
>> +
>> +		led = &priv->leds[i];
>> +		led->control_bank = control_bank;
>> +		ret = fwnode_property_read_u32_array(child, "led-sources",
>> +						    led->hvled_strings,
>> +						    LM3697_MAX_LED_STRINGS);
>> +		if (ret) {
>> +			dev_err(&priv->client->dev, "led-sources property missing\n");
>> +			fwnode_handle_put(child);
>> +			goto child_out;
>> +		}
>> +
>> +		fwnode_property_read_string(child, "linux,default-trigger",
>> +					    &led->led_dev.default_trigger);
>> +
>> +		ret = fwnode_property_read_string(child, "label", &name);
>> +		if (ret)
>> +			snprintf(led->label, sizeof(led->label),
>> +				"%s::", priv->client->name);
>> +		else
>> +			snprintf(led->label, sizeof(led->label),
>> +				 "%s:%s", priv->client->name, name);
>> +
>> +		led->priv = priv;
>> +		led->led_dev.name = led->label;
>> +		led->led_dev.brightness_set_blocking = lm3697_brightness_set;
>> +
>> +		ret = devm_led_classdev_register(priv->dev, &led->led_dev);
>> +		if (ret) {
>> +			dev_err(&priv->client->dev, "led register err: %d\n",
>> +				ret);
>> +			fwnode_handle_put(child);
>> +			goto child_out;
>> +		}
>> +
>> +		i++;
>> +	}
>> +
>> +child_out:
>> +	return ret;
>> +}
>> +
>> +static int lm3697_probe(struct i2c_client *client,
>> +			const struct i2c_device_id *id)
>> +{
>> +	struct lm3697 *led;
>> +	int count;
>> +	int ret;
>> +
>> +	count = device_get_child_node_count(&client->dev);
>> +	if (!count) {
>> +		dev_err(&client->dev, "LEDs are not defined in device tree!");
>> +		return -ENODEV;
>> +	}
>> +
>> +	led = devm_kzalloc(&client->dev, struct_size(led, leds, count),
>> +			   GFP_KERNEL);
>> +	if (!led)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&led->lock);
>> +	i2c_set_clientdata(client, led);
>> +
>> +	led->client = client;
>> +	led->dev = &client->dev;
>> +	led->regmap = devm_regmap_init_i2c(client, &lm3697_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 = lm3697_probe_dt(led);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = lm3697_init(led);
> 
> Let's simplify it:
> 
> return lm3697_init(led);

ACK

> 
>> +
>> +	return ret;
>> +}
>> +
>> +static int lm3697_remove(struct i2c_client *client)
>> +{
>> +	struct lm3697 *led = i2c_get_clientdata(client);
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(led->regmap, LM3697_CTRL_ENABLE,
>> +				 LM3697_CTRL_A_B_EN, 0);
>> +	if (ret) {
>> +		dev_err(&led->client->dev, "Failed to disable the device\n");
>> +		return ret;
>> +	}
>> +
>> +	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 lm3697_id[] = {
>> +	{ "lm3697", 0 },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, lm3697_id);
>> +
>> +static const struct of_device_id of_lm3697_leds_match[] = {
>> +	{ .compatible = "ti,lm3697", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, of_lm3697_leds_match);
>> +
>> +static struct i2c_driver lm3697_driver = {
>> +	.driver = {
>> +		.name	= "lm3697",
>> +		.of_match_table = of_lm3697_leds_match,
>> +	},
>> +	.probe		= lm3697_probe,
>> +	.remove		= lm3697_remove,
>> +	.id_table	= lm3697_id,
>> +};
>> +module_i2c_driver(lm3697_driver);
>> +
>> +MODULE_DESCRIPTION("Texas Instruments LM3697 LED driver");
>> +MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
>> +MODULE_LICENSE("GPL v2");
>>
> 


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

Re: [PATCH v4 2/2] leds: lm3697: Introduce the lm3697 driver

[Jacek Anaszewski]

Dan,

On 08/16/2018 10:44 PM, Dan Murphy wrote:
> Jacek
> 
> On 08/16/2018 02:58 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> Thank you for the patch.
>>
>> I didn't review DT parsing details in v3, but now I've produced
>> diff between v3 and v4 to check what has changed.
>>
>> I'm quite surprised realizing that you're not validating
>> HVLED and control banks assignment, having in mind earlier
>> discussions and your concerns about numerous DT configurations
>> to check.
>>
>> Is it on purpose?
>>
> 
> Yes.  It was on purpose.  After sleeping on it and going through the overall
> control to HVLED assignments I realized the user will know quite quickly
> that their configuration is messed up.
> 
> The suggestions actually simplified the code quite nicely which I am happier to have

Ack. Thanks for the v5 - I'll let it sit on the lists for a week anyway,
until the merge window gets closed.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v4 2/2] leds: lm3697: Introduce the lm3697 driver

[Dan Murphy]

Jacek

On 08/17/2018 02:37 PM, Jacek Anaszewski wrote:
> Dan,
> 
> On 08/16/2018 10:44 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 08/16/2018 02:58 PM, Jacek Anaszewski wrote:
>>> Dan,
>>>
>>> Thank you for the patch.
>>>
>>> I didn't review DT parsing details in v3, but now I've produced
>>> diff between v3 and v4 to check what has changed.
>>>
>>> I'm quite surprised realizing that you're not validating
>>> HVLED and control banks assignment, having in mind earlier
>>> discussions and your concerns about numerous DT configurations
>>> to check.
>>>
>>> Is it on purpose?
>>>
>>
>> Yes.  It was on purpose.  After sleeping on it and going through the overall
>> control to HVLED assignments I realized the user will know quite quickly
>> that their configuration is messed up.
>>
>> The suggestions actually simplified the code quite nicely which I am happier to have
> 
> Ack. Thanks for the v5 - I'll let it sit on the lists for a week anyway,
> until the merge window gets closed.
> 

Sounds good to me.

Dan

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