[PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[PATCH v2 1/2] dt: bindings: lm3697: 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>
---

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

 .../devicetree/bindings/leds/leds-lm3697.txt  | 64 +++++++++++++++++++
 1 file changed, 64 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..7b8e490f1ea1
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
@@ -0,0 +1,64 @@
+* 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
+	- control-bank-cfg - : Indicates which sink is connected to which control bank
+		0 - All HVLED outputs are controlled by bank A
+		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
+		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
+		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
+		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
+		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
+		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
+		7 - All HVLED outputs are controlled by bank B
+
+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
+
+Optional child properties:
+	- label : see Documentation/devicetree/bindings/leds/common.txt
+	- linux,default-trigger :
+	   see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+led-controller@36 {
+	compatible = "ti,lm3967";
+	reg = <0x36>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+	vled-supply = <&vbatt>;
+	control-bank-cfg = <0>;
+
+	led@0 {
+		reg = <0>;
+		label = "white:first_backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
+
+	led@1 {
+		reg = <1>;
+		label = "white:second_backlight_cluster";
+		linux,default-trigger = "frontlight";
+	};
+}
+
+For more product information please see the link below:
+http://www.ti.com/lit/ds/symlink/lm3697.pdf
-- 
2.17.0.582.gccdcbd54c

[PATCH v2 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>
---

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 | 368 +++++++++++++++++++++++++++++++++++++
 3 files changed, 378 insertions(+)
 create mode 100644 drivers/leds/leds-lm3697.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 6e3a998f3370..efa8be180934 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -150,6 +150,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..6384f1d0280e
--- /dev/null
+++ b/drivers/leds/leds-lm3697.c
@@ -0,0 +1,368 @@
+// 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_CONTROL_A	0
+#define LM3697_CONTROL_B	1
+
+#define LM3697_HVLED1_2_3_A		0
+#define LM3697_HVLED1_B_HVLED2_3_A	1
+#define LM3697_HVLED2_B_HVLED1_3_A	2
+#define LM3697_HVLED1_2_B_HVLED3_A	3
+#define LM3697_HVLED3_B_HVLED1_2_A	4
+#define LM3697_HVLED1_3_B_HVLED2_A	5
+#define LM3697_HVLED1_A_HVLED2_3_B	6
+#define LM3697_HVLED1_2_3_B		7
+
+/**
+ * struct lm3697_led -
+ * @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.
+ * @label - LED label
+ */
+struct lm3697_led {
+	struct led_classdev led_dev;
+	struct lm3697 *priv;
+	int control_bank;
+	char label[LED_MAX_NAME_SIZE];
+};
+
+/**
+ * struct lm3697 -
+ * @lock - Lock for reading/writing the device
+ * @client - Pointer to the I2C client
+ * @dev - Pointer to the devices device struct
+ * @regmap - Devices register map
+ * @enable_gpio - Hardware enable gpio
+ * @regulator - LED supply regulator pointer
+ * @control_bank_config - Control bank configuration
+ * @leds - Array of LED strings.
+ */
+struct lm3697 {
+	struct mutex lock;
+	struct i2c_client *client;
+	struct device *dev;
+	struct regmap *regmap;
+	struct gpio_desc *enable_gpio;
+	struct regulator *regulator;
+	int control_bank_config;
+	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_RBTREE,
+};
+
+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_init(struct lm3697 *priv)
+{
+	int ret;
+
+	if (priv->enable_gpio)
+		gpiod_direction_output(priv->enable_gpio, 1);
+	else
+		regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET);
+
+	ret = regmap_write(priv->regmap, LM3697_OUTPUT_CONFIG,
+			   priv->control_bank_config);
+	if (ret) {
+		dev_err(&priv->client->dev, "Cannot write MSB\n");
+		goto out;
+	}
+
+	ret = regmap_write(priv->regmap, LM3697_CTRL_ENABLE, 0x0);
+	if (ret) {
+		dev_err(&priv->client->dev, "Cannot write LSB\n");
+		goto out;
+	}
+
+out:
+	return ret;
+}
+
+static int lm3697_probe_dt(struct lm3697 *priv)
+{
+	struct fwnode_handle *child = NULL;
+	struct fwnode_handle *fwnode;
+	struct lm3697_led *led;
+	const char *name;
+	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;
+
+	fwnode = dev_fwnode(&priv->client->dev);
+	ret = fwnode_property_read_u32(fwnode, "control-bank-cfg",
+				       &priv->control_bank_config);
+	if (ret) {
+		dev_err(&priv->client->dev, "reg DT property missing\n");
+		goto out;
+	}
+
+	if (priv->control_bank_config < LM3697_HVLED1_2_3_A ||
+	    priv->control_bank_config > LM3697_HVLED1_2_3_B) {
+		dev_err(&priv->client->dev, "Control bank configuration is out of range\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	device_for_each_child_node(priv->dev, child) {
+		led = &priv->leds[i];
+
+		ret = fwnode_property_read_u32(child, "reg", &led->control_bank);
+		if (ret) {
+			dev_err(&priv->client->dev, "reg DT property missing\n");
+			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);
+			goto child_out;
+		}
+
+		if (priv->control_bank_config == LM3697_HVLED1_2_3_A ||
+		    priv->control_bank_config == LM3697_HVLED1_2_3_B)
+			goto child_out;
+
+		i++;
+		fwnode_handle_put(child);
+	}
+
+child_out:
+	fwnode_handle_put(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);
+	if (ret)
+		return ret;
+
+	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 regulator\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 v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Michal Vokáč]

On 7.8.2018 18:04, Dan Murphy wrote:
> Add the device tree bindings for the lm3697
> led driver for backlighting and display.

Hi Dan,
it seems like you missed to fix part of the subject as Jacek suggested.

I think it should be: "dt-bindings: leds: Add bindings for lm3697 driver"

Best regards,
Michal
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Jacek Anaszewski]

Hi Michal.

Thanks for the heads-up.
"dt-" prefix is indeed more preferred than "dt: ".

Dan - I will fix it by myself, no need to resend.

Best regards,
Jacek Anaszewski

On 08/08/2018 09:56 AM, Michal Vokáč wrote:
> On 7.8.2018 18:04, Dan Murphy wrote:
>> Add the device tree bindings for the lm3697
>> led driver for backlighting and display.
> 
> Hi Dan,
> it seems like you missed to fix part of the subject as Jacek suggested.
> 
> I think it should be: "dt-bindings: leds: Add bindings for lm3697 driver"
> 
> Best regards,
> Michal
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v2 - Fixed subject and patch commit message -
>> https://lore.kernel.org/patchwork/patch/971326/
> 
> 

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Pavel Machek]

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

On Tue 2018-08-07 11:04:41, 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>
> ---
> 
> v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/
> 
>  .../devicetree/bindings/leds/leds-lm3697.txt  | 64 +++++++++++++++++++
>  1 file changed, 64 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..7b8e490f1ea1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
> @@ -0,0 +1,64 @@
> +* 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
> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
> +		0 - All HVLED outputs are controlled by bank A
> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
> +		7 - All HVLED outputs are controlled by bank B

This is quite long way to describe a bitmask, no? Could we make
it so that control-bank-cfg is not needed?

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

Can we compute control-bank-cfg from this?

> +Optional child properties:
> +	- label : see Documentation/devicetree/bindings/leds/common.txt
> +	- linux,default-trigger :
> +	   see Documentation/devicetree/bindings/leds/common.txt
> +
> +Example:
> +
> +led-controller@36 {
> +	compatible = "ti,lm3967";
> +	reg = <0x36>;
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> +	vled-supply = <&vbatt>;
> +	control-bank-cfg = <0>;
> +
> +	led@0 {
> +		reg = <0>;
> +		label = "white:first_backlight_cluster";
> +		linux,default-trigger = "backlight";
> +	};
> +
> +	led@1 {
> +		reg = <1>;
> +		label = "white:second_backlight_cluster";
> +		linux,default-trigger = "frontlight";
> +	};
> +}

Does the example show correct config? AFAICT all controls go to bank
A according to control-bank-cfg, yet led@1 describes bank B...

									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 v2 2/2] leds: lm3697: Introduce the lm3697 driver

[Pavel Machek]

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

Hi!

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

> +
> +#define LM3697_HVLED1_2_3_A		0
> +#define LM3697_HVLED1_B_HVLED2_3_A	1
> +#define LM3697_HVLED2_B_HVLED1_3_A	2
> +#define LM3697_HVLED1_2_B_HVLED3_A	3
> +#define LM3697_HVLED3_B_HVLED1_2_A	4
> +#define LM3697_HVLED1_3_B_HVLED2_A	5
> +#define LM3697_HVLED1_A_HVLED2_3_B	6
> +#define LM3697_HVLED1_2_3_B		7

That's rather long and verbose way to describe a bitmap, right?

> +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_RBTREE,
> +};

Is rbtree good idea? You don't have that many registers.

> +static int lm3697_init(struct lm3697 *priv)
> +{
> +	int ret;
> +
....
> +		regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET);

No error checking required here?

> +	if (priv->control_bank_config < LM3697_HVLED1_2_3_A ||
> +	    priv->control_bank_config > LM3697_HVLED1_2_3_B) {
> +		dev_err(&priv->client->dev, "Control bank configuration is out of range\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	device_for_each_child_node(priv->dev, child) {
> +		led = &priv->leds[i];
> +
> +		ret = fwnode_property_read_u32(child, "reg", &led->control_bank);
> +		if (ret) {
> +			dev_err(&priv->client->dev, "reg DT property missing\n");
> +			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);
> +			goto child_out;
> +		}
> +
> +		if (priv->control_bank_config == LM3697_HVLED1_2_3_A ||
> +		    priv->control_bank_config == LM3697_HVLED1_2_3_B)
> +			goto child_out;

This checks if we have just one bank, I see it. Should it also check
the led actually uses the correct bank?

> +		i++;
> +		fwnode_handle_put(child);
> +	}
> +
> +child_out:
> +	fwnode_handle_put(child);

Is not the fwnode_handle_put() done twice for non-error case?

> +	ret = lm3697_init(led);
> +	if (ret)
> +		return ret;
> +
> +	return ret;
> +}

The if is not needed here.

> +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 regulator\n");
> +		return ret;

Misleading, this does nothing with regulators.

									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 v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Dan Murphy]

Pavel

Thanks for the review

On 08/08/2018 02:59 PM, Pavel Machek wrote:
> On Tue 2018-08-07 11:04:41, 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>
>> ---
>>
>> v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/
>>
>>  .../devicetree/bindings/leds/leds-lm3697.txt  | 64 +++++++++++++++++++
>>  1 file changed, 64 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..7b8e490f1ea1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
>> @@ -0,0 +1,64 @@
>> +* 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
>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
>> +		0 - All HVLED outputs are controlled by bank A
>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
>> +		7 - All HVLED outputs are controlled by bank B
> 
> This is quite long way to describe a bitmask, no? Could we make
> it so that control-bank-cfg is not needed?

The problem we have here is there is a potential to control
3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
bank b can control 1 LED string.  

These values represent device level control and configuration of the LED strings to a specific control bank.

I racked my brain trying to figure out how to configure the control banks and associated LED strings.
These values are for the device configuration itself and the reg below indicates which control bank the LED
node is assigned to.

> 
>> +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
> 
> Can we compute control-bank-cfg from this?

Don't see how you could compute this.  There is no easy way to give indication to the driver which LED
node belongs to which control bank.  The control-bank-cfg is a device level property and the reg under the
child is a LED string level property denoting the Class node to control bank mapping.

Furthermore there are 2 device configurations that can be configured to only use 1 bank for all 3 LED strings.
This will be answered in your comments in the code.

> 
>> +Optional child properties:
>> +	- label : see Documentation/devicetree/bindings/leds/common.txt
>> +	- linux,default-trigger :
>> +	   see Documentation/devicetree/bindings/leds/common.txt
>> +
>> +Example:
>> +
>> +led-controller@36 {
>> +	compatible = "ti,lm3967";
>> +	reg = <0x36>;
>> +	#address-cells = <1>;
>> +	#size-cells = <0>;
>> +
>> +	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>> +	vled-supply = <&vbatt>;
>> +	control-bank-cfg = <0>;
>> +
>> +	led@0 {
>> +		reg = <0>;
>> +		label = "white:first_backlight_cluster";
>> +		linux,default-trigger = "backlight";
>> +	};
>> +
>> +	led@1 {
>> +		reg = <1>;
>> +		label = "white:second_backlight_cluster";
>> +		linux,default-trigger = "frontlight";
>> +	};
>> +}
> 
> Does the example show correct config? AFAICT all controls go to bank
> A according to control-bank-cfg, yet led@1 describes bank B...

This I can fix it should be a value between 1 and 6

> 
> 									Pavel
> 


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

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Pavel Machek]

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

Hi!

> >> +	- #size-cells : 0
> >> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
> >> +		0 - All HVLED outputs are controlled by bank A
> >> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
> >> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
> >> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
> >> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
> >> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
> >> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
> >> +		7 - All HVLED outputs are controlled by bank B
> > 
> > This is quite long way to describe a bitmask, no? Could we make
> > it so that control-bank-cfg is not needed?
> 
> The problem we have here is there is a potential to control
> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
> bank b can control 1 LED string.  
> 

Can we forget about the LED strings, and just expose the sinks as
Linux LED devices?
									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 v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Dan Murphy]

On 08/08/2018 04:02 PM, Pavel Machek wrote:
> Hi!
> 
>>>> +	- #size-cells : 0
>>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
>>>> +		0 - All HVLED outputs are controlled by bank A
>>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
>>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
>>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
>>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
>>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
>>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
>>>> +		7 - All HVLED outputs are controlled by bank B
>>>
>>> This is quite long way to describe a bitmask, no? Could we make
>>> it so that control-bank-cfg is not needed?
>>
>> The problem we have here is there is a potential to control
>> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
>> bank b can control 1 LED string.  
>>
> 
> Can we forget about the LED strings, and just expose the sinks as
> Linux LED devices?

2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
to when setting the brightness.  Each Bank has a separate register for brightness control.

Dan

> 									Pavel
> 


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

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Pavel Machek]

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

On Wed 2018-08-08 16:04:43, Dan Murphy wrote:
> On 08/08/2018 04:02 PM, Pavel Machek wrote:
> > Hi!
> > 
> >>>> +	- #size-cells : 0
> >>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
> >>>> +		0 - All HVLED outputs are controlled by bank A
> >>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
> >>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
> >>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
> >>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
> >>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
> >>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
> >>>> +		7 - All HVLED outputs are controlled by bank B
> >>>
> >>> This is quite long way to describe a bitmask, no? Could we make
> >>> it so that control-bank-cfg is not needed?
> >>
> >> The problem we have here is there is a potential to control
> >> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
> >> bank b can control 1 LED string.  
> >>
> > 
> > Can we forget about the LED strings, and just expose the sinks as
> > Linux LED devices?
> 
> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
> to when setting the brightness.  Each Bank has a separate register for brightness control.

Yes, and LED strings are statically assigned to banks, right?

So why not simply forget about LED strings for sake of hw
abstractions, and work just with banks?
									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 v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Jacek Anaszewski]

Hi Dan,

On 08/08/2018 11:04 PM, Dan Murphy wrote:
> On 08/08/2018 04:02 PM, Pavel Machek wrote:
>> Hi!
>>
>>>>> +	- #size-cells : 0
>>>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
>>>>> +		0 - All HVLED outputs are controlled by bank A
>>>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
>>>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
>>>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
>>>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
>>>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
>>>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
>>>>> +		7 - All HVLED outputs are controlled by bank B
>>>>
>>>> This is quite long way to describe a bitmask, no? Could we make
>>>> it so that control-bank-cfg is not needed?
>>>
>>> The problem we have here is there is a potential to control
>>> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
>>> bank b can control 1 LED string.  
>>>
>>
>> Can we forget about the LED strings, and just expose the sinks as
>> Linux LED devices?
> 
> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
> to when setting the brightness.  Each Bank has a separate register for brightness control.

Just a blind shot, without going into details - could you please check
if led-sources property documented in the common LED bindings couldn't
help here?

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Dan Murphy]

On 08/08/2018 04:09 PM, Pavel Machek wrote:
> On Wed 2018-08-08 16:04:43, Dan Murphy wrote:
>> On 08/08/2018 04:02 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>> +	- #size-cells : 0
>>>>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
>>>>>> +		0 - All HVLED outputs are controlled by bank A
>>>>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
>>>>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
>>>>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
>>>>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
>>>>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
>>>>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
>>>>>> +		7 - All HVLED outputs are controlled by bank B
>>>>>
>>>>> This is quite long way to describe a bitmask, no? Could we make
>>>>> it so that control-bank-cfg is not needed?
>>>>
>>>> The problem we have here is there is a potential to control
>>>> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
>>>> bank b can control 1 LED string.  
>>>>
>>>
>>> Can we forget about the LED strings, and just expose the sinks as
>>> Linux LED devices?
>>
>> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
>> to when setting the brightness.  Each Bank has a separate register for brightness control.
> 
> Yes, and LED strings are statically assigned to banks, right?
> 
> So why not simply forget about LED strings for sake of hw
> abstractions, and work just with banks?

How would you set the control bank register for the correct LED string configuration?

Dan

> 									Pavel
> 


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

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Dan Murphy]

Jacek

On 08/08/2018 04:09 PM, Jacek Anaszewski wrote:
> Hi Dan,
> 
> On 08/08/2018 11:04 PM, Dan Murphy wrote:
>> On 08/08/2018 04:02 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>>> +	- #size-cells : 0
>>>>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
>>>>>> +		0 - All HVLED outputs are controlled by bank A
>>>>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
>>>>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
>>>>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
>>>>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
>>>>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
>>>>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
>>>>>> +		7 - All HVLED outputs are controlled by bank B
>>>>>
>>>>> This is quite long way to describe a bitmask, no? Could we make
>>>>> it so that control-bank-cfg is not needed?
>>>>
>>>> The problem we have here is there is a potential to control
>>>> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
>>>> bank b can control 1 LED string.  
>>>>
>>>
>>> Can we forget about the LED strings, and just expose the sinks as
>>> Linux LED devices?
>>
>> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
>> to when setting the brightness.  Each Bank has a separate register for brightness control.
> 
> Just a blind shot, without going into details - could you please check
> if led-sources property documented in the common LED bindings couldn't
> help here?
> 

I could change the name to led-sources.  But this part does not really follow the 1 output to a
1 LED string topology.


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

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Pavel Machek]

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

On Wed 2018-08-08 16:41:16, Dan Murphy wrote:
> On 08/08/2018 04:09 PM, Pavel Machek wrote:
> > On Wed 2018-08-08 16:04:43, Dan Murphy wrote:
> >> On 08/08/2018 04:02 PM, Pavel Machek wrote:
> >>> Hi!
> >>>
> >>>>>> +	- #size-cells : 0
> >>>>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
> >>>>>> +		0 - All HVLED outputs are controlled by bank A
> >>>>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
> >>>>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
> >>>>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
> >>>>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
> >>>>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
> >>>>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
> >>>>>> +		7 - All HVLED outputs are controlled by bank B
> >>>>>
> >>>>> This is quite long way to describe a bitmask, no? Could we make
> >>>>> it so that control-bank-cfg is not needed?
> >>>>
> >>>> The problem we have here is there is a potential to control
> >>>> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
> >>>> bank b can control 1 LED string.  
> >>>>
> >>>
> >>> Can we forget about the LED strings, and just expose the sinks as
> >>> Linux LED devices?
> >>
> >> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
> >> to when setting the brightness.  Each Bank has a separate register for brightness control.
> > 
> > Yes, and LED strings are statically assigned to banks, right?
> > 
> > So why not simply forget about LED strings for sake of hw
> > abstractions, and work just with banks?
> 
> How would you set the control bank register for the correct LED string configuration?

Have property at each LED saying which which HVLEDs it controls?

									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 v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Dan Murphy]

Pavel

On 08/08/2018 04:45 PM, Pavel Machek wrote:
> On Wed 2018-08-08 16:41:16, Dan Murphy wrote:
>> On 08/08/2018 04:09 PM, Pavel Machek wrote:
>>> On Wed 2018-08-08 16:04:43, Dan Murphy wrote:
>>>> On 08/08/2018 04:02 PM, Pavel Machek wrote:
>>>>> Hi!
>>>>>
>>>>>>>> +	- #size-cells : 0
>>>>>>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
>>>>>>>> +		0 - All HVLED outputs are controlled by bank A
>>>>>>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
>>>>>>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
>>>>>>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
>>>>>>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
>>>>>>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
>>>>>>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
>>>>>>>> +		7 - All HVLED outputs are controlled by bank B
>>>>>>>
>>>>>>> This is quite long way to describe a bitmask, no? Could we make
>>>>>>> it so that control-bank-cfg is not needed?
>>>>>>
>>>>>> The problem we have here is there is a potential to control
>>>>>> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
>>>>>> bank b can control 1 LED string.  
>>>>>>
>>>>>
>>>>> Can we forget about the LED strings, and just expose the sinks as
>>>>> Linux LED devices?
>>>>
>>>> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
>>>> to when setting the brightness.  Each Bank has a separate register for brightness control.
>>>
>>> Yes, and LED strings are statically assigned to banks, right?
>>>
>>> So why not simply forget about LED strings for sake of hw
>>> abstractions, and work just with banks?
>>
>> How would you set the control bank register for the correct LED string configuration?
> 
> Have property at each LED saying which which HVLEDs it controls?

Isn't that what I have already using the reg property?

Then we would have to aggregate the configuration and make a determination in the driver.

But that does not follow the LED child node ideology.
Each output of the LED driver should have a child node.

In this case the outputs are the sinks(inputs) and there are only 2 sinks so having 3 LED child nodes would be confusing
and there are required properties for each child like label.

Each child node would then need to present 1 LED node to the user space to control the LED string.  Which would
be technically incorrect because you would have 2 LED nodes controlling the same control bank sink.

> 
> 									Pavel
> 


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

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Pavel Machek]

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

Hi!

> >>> Yes, and LED strings are statically assigned to banks, right?
> >>>
> >>> So why not simply forget about LED strings for sake of hw
> >>> abstractions, and work just with banks?
> >>
> >> How would you set the control bank register for the correct LED string configuration?
> > 
> > Have property at each LED saying which which HVLEDs it controls?
> 
> Isn't that what I have already using the reg property?

> Then we would have to aggregate the configuration and make a determination in the driver.

Yes.

> But that does not follow the LED child node ideology.
> Each output of the LED driver should have a child node.
> 
> In this case the outputs are the sinks(inputs) and there are only 2 sinks so having 3 LED child nodes would be confusing
> and there are required properties for each child like label.

No, I don't want that.

I'd like 2 child nodes, each specifying which HVLEDs it controls.

Let me edit the original proposal.

									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 v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Pavel Machek]

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

On Tue 2018-08-07 11:04:41, Dan Murphy wrote:
> Add the device tree bindings for the lm3697
> led driver for backlighting and display.

What I'd like to see:

> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
> new file mode 100644
> index 000000000000..7b8e490f1ea1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
> @@ -0,0 +1,64 @@
> +* 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
> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
> +		0 - All HVLED outputs are controlled by bank A
> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
> +		7 - All HVLED outputs are controlled by bank B

Remove control-bank-cfg.

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

Add required child property:
        - hvleds = <list> -- set of outputs this child controls.

									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 v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Jacek Anaszewski]

Dan,

On 08/08/2018 11:45 PM, Dan Murphy wrote:
> Jacek
> 
> On 08/08/2018 04:09 PM, Jacek Anaszewski wrote:
>> Hi Dan,
>>
>> On 08/08/2018 11:04 PM, Dan Murphy wrote:
>>> On 08/08/2018 04:02 PM, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>>> +	- #size-cells : 0
>>>>>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
>>>>>>> +		0 - All HVLED outputs are controlled by bank A
>>>>>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
>>>>>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
>>>>>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
>>>>>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
>>>>>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
>>>>>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
>>>>>>> +		7 - All HVLED outputs are controlled by bank B
>>>>>>
>>>>>> This is quite long way to describe a bitmask, no? Could we make
>>>>>> it so that control-bank-cfg is not needed?
>>>>>
>>>>> The problem we have here is there is a potential to control
>>>>> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
>>>>> bank b can control 1 LED string.  
>>>>>
>>>>
>>>> Can we forget about the LED strings, and just expose the sinks as
>>>> Linux LED devices?
>>>
>>> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
>>> to when setting the brightness.  Each Bank has a separate register for brightness control.
>>
>> Just a blind shot, without going into details - could you please check
>> if led-sources property documented in the common LED bindings couldn't
>> help here?
>>
> 
> I could change the name to led-sources.  But this part does not really follow the 1 output to a
> 1 LED string topology.

led-sources was designed for describing the topology where one LED can
be connected to more then one output, see bindings of
max77693-led (in Documentation/devicetree/bindings/mfd/max77693.txt).

Here the topology is a bit different - more than one LED (string) can be
connected to a single bank, but this is accomplished inside the chip.
Logically LEDs configured that way can be treated as a single LED
(string) connected to two outputs, and what follows they should be
described by a single DT child node.

led-sources will fit very well for this purpose. You could do
the following mapping:

0 - HVLED1
1 - HVLED2
2 - HVLED3

Then, in the child DT nodes you would use these identifiers to describe
the topology:

Following node would describe strings connected to the outputs
HVLED1 and HVLED2 controlled by bank A.

led@0 {
	reg = <0>;
	led-sources = <0>. <1>;
	label = "white:first_backlight_cluster";
	linux,default-trigger = "backlight";
};


IOW I agree with Pavel, but I propose to use already documented common
DT LED property.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Pavel Machek]

Hi!

> >>> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
> >>> to when setting the brightness.  Each Bank has a separate register for brightness control.
> >>
> >> Just a blind shot, without going into details - could you please check
> >> if led-sources property documented in the common LED bindings couldn't
> >> help here?
> >>
> > 
> > I could change the name to led-sources.  But this part does not really follow the 1 output to a
> > 1 LED string topology.
> 
> led-sources was designed for describing the topology where one LED can
> be connected to more then one output, see bindings of
> max77693-led (in Documentation/devicetree/bindings/mfd/max77693.txt).
> 
> Here the topology is a bit different - more than one LED (string) can be
> connected to a single bank, but this is accomplished inside the chip.
> Logically LEDs configured that way can be treated as a single LED
> (string) connected to two outputs, and what follows they should be
> described by a single DT child node.
> 
> led-sources will fit very well for this purpose. You could do
> the following mapping:
> 
> 0 - HVLED1
> 1 - HVLED2
> 2 - HVLED3
> 
> Then, in the child DT nodes you would use these identifiers to describe
> the topology:
> 
> Following node would describe strings connected to the outputs
> HVLED1 and HVLED2 controlled by bank A.
> 
> led@0 {
> 	reg = <0>;
> 	led-sources = <0>. <1>;
> 	label = "white:first_backlight_cluster";
> 	linux,default-trigger = "backlight";
> };
> 
> 
> IOW I agree with Pavel, but I propose to use already documented common
> DT LED property.

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

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Dan Murphy]

Jacek and Pavel

On 08/09/2018 07:09 AM, Jacek Anaszewski wrote:
> Dan,
> 
> On 08/08/2018 11:45 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 08/08/2018 04:09 PM, Jacek Anaszewski wrote:
>>> Hi Dan,
>>>
>>> On 08/08/2018 11:04 PM, Dan Murphy wrote:
>>>> On 08/08/2018 04:02 PM, Pavel Machek wrote:
>>>>> Hi!
>>>>>
>>>>>>>> +	- #size-cells : 0
>>>>>>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
>>>>>>>> +		0 - All HVLED outputs are controlled by bank A
>>>>>>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
>>>>>>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
>>>>>>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
>>>>>>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
>>>>>>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
>>>>>>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
>>>>>>>> +		7 - All HVLED outputs are controlled by bank B
>>>>>>>
>>>>>>> This is quite long way to describe a bitmask, no? Could we make
>>>>>>> it so that control-bank-cfg is not needed?
>>>>>>
>>>>>> The problem we have here is there is a potential to control
>>>>>> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
>>>>>> bank b can control 1 LED string.  
>>>>>>
>>>>>
>>>>> Can we forget about the LED strings, and just expose the sinks as
>>>>> Linux LED devices?
>>>>
>>>> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
>>>> to when setting the brightness.  Each Bank has a separate register for brightness control.
>>>
>>> Just a blind shot, without going into details - could you please check
>>> if led-sources property documented in the common LED bindings couldn't
>>> help here?
>>>
>>
>> I could change the name to led-sources.  But this part does not really follow the 1 output to a
>> 1 LED string topology.
> 
> led-sources was designed for describing the topology where one LED can
> be connected to more then one output, see bindings of
> max77693-led (in Documentation/devicetree/bindings/mfd/max77693.txt).
> 
> Here the topology is a bit different - more than one LED (string) can be
> connected to a single bank, but this is accomplished inside the chip.
> Logically LEDs configured that way can be treated as a single LED
> (string) connected to two outputs, and what follows they should be
> described by a single DT child node.
> 
> led-sources will fit very well for this purpose. You could do
> the following mapping:
> 
> 0 - HVLED1
> 1 - HVLED2
> 2 - HVLED3
> 
> Then, in the child DT nodes you would use these identifiers to describe
> the topology:
> 
> Following node would describe strings connected to the outputs
> HVLED1 and HVLED2 controlled by bank A.
> 
> led@0 {
> 	reg = <0>;
> 	led-sources = <0>. <1>;
> 	label = "white:first_backlight_cluster";
> 	linux,default-trigger = "backlight";
> };
> 
> 
> IOW I agree with Pavel, but I propose to use already documented common
> DT LED property.
> 

I agree to use the led-sources but I still believe this approach may be confusing to other sw devs
and will lead to configuration issues by users.

This implementation requires the sw dev to know which strings are controlled by which bank.
And this method may produce a misconfiguration like something below where HVLED2 is declared in
both bank A and bank B

led@0 {
	reg = <0>;
	led-sources = <0>. <1>;
	label = "white:first_backlight_cluster";
	linux,default-trigger = "backlight";
};

led@1 {
	reg = <1>;
	led-sources = <1>. <2>;
	label = "white:keypad_cluster";
	linux,default-trigger = "backlight";
};

The driver will need to be intelligent and declare a miss configuration on the above.
Not saying this cannot be done but I am not sure why we want to add all of these extra LoC and intelligence
in the kernel driver.
The driver cannot make assumptions on the intention.  And the device tree documentation will need to
pretty much need a lengthy explanation on how to configure the child nodes.

The implementation I suggested removes that ambiguity.  It is a simple integer that is written to the device
as part of the device configuration, which the config is a setting for the device.

The child nodes denote which bank the exposed LED node will control.  Removing any need
for the sw developers new or old to know the specific device configurations.


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

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Jacek Anaszewski]

Dan,

On 08/09/2018 03:30 PM, Dan Murphy wrote:
> Jacek and Pavel
> 
> On 08/09/2018 07:09 AM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 08/08/2018 11:45 PM, Dan Murphy wrote:
>>> Jacek
>>>
>>> On 08/08/2018 04:09 PM, Jacek Anaszewski wrote:
>>>> Hi Dan,
>>>>
>>>> On 08/08/2018 11:04 PM, Dan Murphy wrote:
>>>>> On 08/08/2018 04:02 PM, Pavel Machek wrote:
>>>>>> Hi!
>>>>>>
>>>>>>>>> +	- #size-cells : 0
>>>>>>>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
>>>>>>>>> +		0 - All HVLED outputs are controlled by bank A
>>>>>>>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
>>>>>>>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
>>>>>>>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
>>>>>>>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
>>>>>>>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
>>>>>>>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
>>>>>>>>> +		7 - All HVLED outputs are controlled by bank B
>>>>>>>>
>>>>>>>> This is quite long way to describe a bitmask, no? Could we make
>>>>>>>> it so that control-bank-cfg is not needed?
>>>>>>>
>>>>>>> The problem we have here is there is a potential to control
>>>>>>> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
>>>>>>> bank b can control 1 LED string.  
>>>>>>>
>>>>>>
>>>>>> Can we forget about the LED strings, and just expose the sinks as
>>>>>> Linux LED devices?
>>>>>
>>>>> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
>>>>> to when setting the brightness.  Each Bank has a separate register for brightness control.
>>>>
>>>> Just a blind shot, without going into details - could you please check
>>>> if led-sources property documented in the common LED bindings couldn't
>>>> help here?
>>>>
>>>
>>> I could change the name to led-sources.  But this part does not really follow the 1 output to a
>>> 1 LED string topology.
>>
>> led-sources was designed for describing the topology where one LED can
>> be connected to more then one output, see bindings of
>> max77693-led (in Documentation/devicetree/bindings/mfd/max77693.txt).
>>
>> Here the topology is a bit different - more than one LED (string) can be
>> connected to a single bank, but this is accomplished inside the chip.
>> Logically LEDs configured that way can be treated as a single LED
>> (string) connected to two outputs, and what follows they should be
>> described by a single DT child node.
>>
>> led-sources will fit very well for this purpose. You could do
>> the following mapping:
>>
>> 0 - HVLED1
>> 1 - HVLED2
>> 2 - HVLED3
>>
>> Then, in the child DT nodes you would use these identifiers to describe
>> the topology:
>>
>> Following node would describe strings connected to the outputs
>> HVLED1 and HVLED2 controlled by bank A.
>>
>> led@0 {
>> 	reg = <0>;
>> 	led-sources = <0>. <1>;
>> 	label = "white:first_backlight_cluster";
>> 	linux,default-trigger = "backlight";
>> };
>>
>>
>> IOW I agree with Pavel, but I propose to use already documented common
>> DT LED property.
>>
> 
> I agree to use the led-sources but I still believe this approach may be confusing to other sw devs
> and will lead to configuration issues by users.
> 
> This implementation requires the sw dev to know which strings are controlled by which bank.
> And this method may produce a misconfiguration like something below where HVLED2 is declared in
> both bank A and bank B
> 
> led@0 {
> 	reg = <0>;
> 	led-sources = <0>. <1>;
> 	label = "white:first_backlight_cluster";
> 	linux,default-trigger = "backlight";
> };
> 
> led@1 {
> 	reg = <1>;
> 	led-sources = <1>. <2>;
> 	label = "white:keypad_cluster";
> 	linux,default-trigger = "backlight";
> };
> 
> The driver will need to be intelligent and declare a miss configuration on the above.
> Not saying this cannot be done but I am not sure why we want to add all of these extra LoC and intelligence
> in the kernel driver.

It is better do add some complexity to the driver than to the
user configurable settings like DT. Besides - you will only need to
check if given led-source is already taken by another node.

> The driver cannot make assumptions on the intention.  And the device tree documentation will need to
> pretty much need a lengthy explanation on how to configure the child nodes.

Some description will be needed for sure, but I don't expect it
to be overwhelmingly lengthy.

> The implementation I suggested removes that ambiguity.  It is a simple integer that is written to the device
> as part of the device configuration, which the config is a setting for the device.

Your control-bank-cfg seemed like having much room for improvement,
and it would for sure raise questions on why it was implemented that
way. Documenting all available combinations of the configuration is
seldom the best solution. It often obscures the issue.

> The child nodes denote which bank the exposed LED node will control.  Removing any need
> for the sw developers new or old to know the specific device configurations.

In your bindings device configuration is scattered among global
control-bank-cfg property and child node's reg property.
In my proposal each child node contains all the needed configuration,
also in the form of two properties - led-sources and reg. IMHO having
all the LED class device related configuration in one place simplifies
the analysis.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Dan Murphy]

On 08/09/2018 09:48 AM, Jacek Anaszewski wrote:
> Dan,
> 
> On 08/09/2018 03:30 PM, Dan Murphy wrote:
>> Jacek and Pavel
>>
>> On 08/09/2018 07:09 AM, Jacek Anaszewski wrote:
>>> Dan,
>>>
>>> On 08/08/2018 11:45 PM, Dan Murphy wrote:
>>>> Jacek
>>>>
>>>> On 08/08/2018 04:09 PM, Jacek Anaszewski wrote:
>>>>> Hi Dan,
>>>>>
>>>>> On 08/08/2018 11:04 PM, Dan Murphy wrote:
>>>>>> On 08/08/2018 04:02 PM, Pavel Machek wrote:
>>>>>>> Hi!
>>>>>>>
>>>>>>>>>> +	- #size-cells : 0
>>>>>>>>>> +	- control-bank-cfg - : Indicates which sink is connected to which control bank
>>>>>>>>>> +		0 - All HVLED outputs are controlled by bank A
>>>>>>>>>> +		1 - HVLED1 is controlled bank B, HVLED2/3 are controlled by bank A
>>>>>>>>>> +		2 - HVLED2 is controlled bank B, HVLED1/3 are controlled by bank A
>>>>>>>>>> +		3 - HVLED1/2 are controlled by bank B, HVLED3 is controlled by bank A
>>>>>>>>>> +		4 - HVLED3 is controlled by bank B, HVLED1/2 are controlled by bank A
>>>>>>>>>> +		5 - HVLED1/3 is controlled by bank B, HVLED2 is controlled by bank A
>>>>>>>>>> +		6 - (default) HVLED1 is controlled by bank A, HVLED2/3 are controlled by bank B
>>>>>>>>>> +		7 - All HVLED outputs are controlled by bank B
>>>>>>>>>
>>>>>>>>> This is quite long way to describe a bitmask, no? Could we make
>>>>>>>>> it so that control-bank-cfg is not needed?
>>>>>>>>
>>>>>>>> The problem we have here is there is a potential to control
>>>>>>>> 3 different LED string but only 2 sinks.  So control bank A can control 2 LED strings and control
>>>>>>>> bank b can control 1 LED string.  
>>>>>>>>
>>>>>>>
>>>>>>> Can we forget about the LED strings, and just expose the sinks as
>>>>>>> Linux LED devices?
>>>>>>
>>>>>> 2 sinks 3 LED strings.  How do you know which LED string is which and what bank it belongs
>>>>>> to when setting the brightness.  Each Bank has a separate register for brightness control.
>>>>>
>>>>> Just a blind shot, without going into details - could you please check
>>>>> if led-sources property documented in the common LED bindings couldn't
>>>>> help here?
>>>>>
>>>>
>>>> I could change the name to led-sources.  But this part does not really follow the 1 output to a
>>>> 1 LED string topology.
>>>
>>> led-sources was designed for describing the topology where one LED can
>>> be connected to more then one output, see bindings of
>>> max77693-led (in Documentation/devicetree/bindings/mfd/max77693.txt).
>>>
>>> Here the topology is a bit different - more than one LED (string) can be
>>> connected to a single bank, but this is accomplished inside the chip.
>>> Logically LEDs configured that way can be treated as a single LED
>>> (string) connected to two outputs, and what follows they should be
>>> described by a single DT child node.
>>>
>>> led-sources will fit very well for this purpose. You could do
>>> the following mapping:
>>>
>>> 0 - HVLED1
>>> 1 - HVLED2
>>> 2 - HVLED3
>>>
>>> Then, in the child DT nodes you would use these identifiers to describe
>>> the topology:
>>>
>>> Following node would describe strings connected to the outputs
>>> HVLED1 and HVLED2 controlled by bank A.
>>>
>>> led@0 {
>>> 	reg = <0>;
>>> 	led-sources = <0>. <1>;
>>> 	label = "white:first_backlight_cluster";
>>> 	linux,default-trigger = "backlight";
>>> };
>>>
>>>
>>> IOW I agree with Pavel, but I propose to use already documented common
>>> DT LED property.
>>>
>>
>> I agree to use the led-sources but I still believe this approach may be confusing to other sw devs
>> and will lead to configuration issues by users.
>>
>> This implementation requires the sw dev to know which strings are controlled by which bank.
>> And this method may produce a misconfiguration like something below where HVLED2 is declared in
>> both bank A and bank B
>>
>> led@0 {
>> 	reg = <0>;
>> 	led-sources = <0>. <1>;
>> 	label = "white:first_backlight_cluster";
>> 	linux,default-trigger = "backlight";
>> };
>>
>> led@1 {
>> 	reg = <1>;
>> 	led-sources = <1>. <2>;
>> 	label = "white:keypad_cluster";
>> 	linux,default-trigger = "backlight";
>> };
>>
>> The driver will need to be intelligent and declare a miss configuration on the above.
>> Not saying this cannot be done but I am not sure why we want to add all of these extra LoC and intelligence
>> in the kernel driver.
> 
> It is better do add some complexity to the driver than to the
> user configurable settings like DT. Besides - you will only need to
> check if given led-source is already taken by another node.

Yes I know that the driver can check the string but if the same string is declared by another child then
the driver must exit with -EINVAL.  Again a lot of code for little pay off.
I believe we should keep drivers as simple as possible.

I will add the changes.

> 
>> The driver cannot make assumptions on the intention.  And the device tree documentation will need to
>> pretty much need a lengthy explanation on how to configure the child nodes.
> 
> Some description will be needed for sure, but I don't expect it
> to be overwhelmingly lengthy.
> 
>> The implementation I suggested removes that ambiguity.  It is a simple integer that is written to the device
>> as part of the device configuration, which the config is a setting for the device.
> 
> Your control-bank-cfg seemed like having much room for improvement,
> and it would for sure raise questions on why it was implemented that
> way. Documenting all available combinations of the configuration is
> seldom the best solution. It often obscures the issue.

Unfortunately in either case this high level of documentation will need to be done.
I believe both solutions will raise questions and concerns.

There does not seem to be a good way to describe this device.
Both solutions are wrought with issues and concerns.

But like I said I will re-write the code with the above suggestion.

> 
>> The child nodes denote which bank the exposed LED node will control.  Removing any need
>> for the sw developers new or old to know the specific device configurations.
> 
> In your bindings device configuration is scattered among global
> control-bank-cfg property and child node's reg property.
> In my proposal each child node contains all the needed configuration,
> also in the form of two properties - led-sources and reg. IMHO having
> all the LED class device related configuration in one place simplifies
> the analysis.
> 

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

Re: [PATCH v2 1/2] dt: bindings: lm3697: Add bindings for lm3697 driver

[Pavel Machek]

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

Hi!

> > Following node would describe strings connected to the outputs
> > HVLED1 and HVLED2 controlled by bank A.
> > 
> > led@0 {
> > 	reg = <0>;
> > 	led-sources = <0>. <1>;
> > 	label = "white:first_backlight_cluster";
> > 	linux,default-trigger = "backlight";
> > };
> > 
> > 
> > IOW I agree with Pavel, but I propose to use already documented common
> > DT LED property.
> > 
> 
> I agree to use the led-sources but I still believe this approach may be confusing to other sw devs
> and will lead to configuration issues by users.
> 
> This implementation requires the sw dev to know which strings are controlled by which bank.
> And this method may produce a misconfiguration like something below where HVLED2 is declared in
> both bank A and bank B
> 
> led@0 {
> 	reg = <0>;
> 	led-sources = <0>. <1>;
> 	label = "white:first_backlight_cluster";
> 	linux,default-trigger = "backlight";
> };
> 
> led@1 {
> 	reg = <1>;
> 	led-sources = <1>. <2>;
> 	label = "white:keypad_cluster";
> 	linux,default-trigger = "backlight";
> };
> 
> The driver will need to be intelligent and declare a miss
> configuration on the above.

Yes. But please do it that way, it is still better than being
different from all the other 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 v2 2/2] leds: lm3697: Introduce the lm3697 driver

[Dan Murphy]

Pavel

Thanks for the review

On 08/08/2018 02:59 PM, Pavel Machek wrote:
> Hi!
> 
>> 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>
> 
>> +
>> +#define LM3697_HVLED1_2_3_A		0
>> +#define LM3697_HVLED1_B_HVLED2_3_A	1
>> +#define LM3697_HVLED2_B_HVLED1_3_A	2
>> +#define LM3697_HVLED1_2_B_HVLED3_A	3
>> +#define LM3697_HVLED3_B_HVLED1_2_A	4
>> +#define LM3697_HVLED1_3_B_HVLED2_A	5
>> +#define LM3697_HVLED1_A_HVLED2_3_B	6
>> +#define LM3697_HVLED1_2_3_B		7
> 
> That's rather long and verbose way to describe a bitmap, right?

It will be removed with v3

> 
>> +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_RBTREE,
>> +};
> 
> Is rbtree good idea? You don't have that many registers.

ack

> 
>> +static int lm3697_init(struct lm3697 *priv)
>> +{
>> +	int ret;
>> +
> ....
>> +		regmap_write(priv->regmap, LM3697_RESET, LM3697_SW_RESET);
> 
> No error checking required here?

Ack

> 
>> +	if (priv->control_bank_config < LM3697_HVLED1_2_3_A ||
>> +	    priv->control_bank_config > LM3697_HVLED1_2_3_B) {
>> +		dev_err(&priv->client->dev, "Control bank configuration is out of range\n");
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	device_for_each_child_node(priv->dev, child) {
>> +		led = &priv->leds[i];
>> +
>> +		ret = fwnode_property_read_u32(child, "reg", &led->control_bank);
>> +		if (ret) {
>> +			dev_err(&priv->client->dev, "reg DT property missing\n");
>> +			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);
>> +			goto child_out;
>> +		}
>> +
>> +		if (priv->control_bank_config == LM3697_HVLED1_2_3_A ||
>> +		    priv->control_bank_config == LM3697_HVLED1_2_3_B)
>> +			goto child_out;
> 
> This checks if we have just one bank, I see it. Should it also check
> the led actually uses the correct bank?

It will be removed with v3

> 
>> +		i++;
>> +		fwnode_handle_put(child);
>> +	}
>> +
>> +child_out:
>> +	fwnode_handle_put(child);
> 
> Is not the fwnode_handle_put() done twice for non-error case?

Ack

> 
>> +	ret = lm3697_init(led);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return ret;
>> +}
> 
> The if is not needed here.
> 

Ack

>> +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 regulator\n");
>> +		return ret;
> 
> Misleading, this does nothing with regulators.

Ack

> 
> 									Pavel
> 


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