[PATCH v6 1/6] leds: Add new API to derive a LED name

[PATCH v6 1/6] leds: Add new API to derive a LED name

[Dan Murphy]

Create an API that is called to derive the
LED name from either the DT label in the child
node or if that does not exist from the parent
node name and an alternate label that is passed in.

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

v6 - New patch to add the api to generate a LED label

 drivers/leds/led-class.c | 34 ++++++++++++++++++++++++++++++++++
 include/linux/leds.h     |  6 ++++++
 2 files changed, 40 insertions(+)

diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index b0e2d55acbd6..d3e035488737 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -17,6 +17,7 @@
 #include <linux/leds.h>
 #include <linux/list.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/timer.h>
@@ -243,6 +244,39 @@ static int led_classdev_next_name(const char *init_name, char *name,
 	return i;
 }
 
+/**
+ * of_led_compose_name - derive the LED name based on DT or alternate name.
+ *
+ * @parent: parent of LED device
+ * @child: child node of LED device
+ * @alt_name: an alternate name if the label node is not found
+ * @len: length of the alt_name
+ * @led_name: derived name from either DT label or alt_name
+ */
+void of_led_compose_name(struct device_node *parent,
+                     struct device_node *child,
+		     const char *alt_name,
+		     size_t len,
+		     char *led_name)
+{
+	int ret;
+	int length;
+	const char *name;
+
+	if (len == 0 || alt_name == NULL)
+		return;
+
+	ret = of_property_read_string(child, "label", &name);
+	if (!ret) {
+		strlcpy(led_name, name, sizeof(led_name));
+	} else {
+		length = len + strlen(parent->name) + 1;
+		snprintf(led_name, len, "%s:%s", parent->name, alt_name);
+	}
+
+}
+EXPORT_SYMBOL_GPL(of_led_compose_name);
+
 /**
  * of_led_classdev_register - register a new object of led_classdev class.
  *
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 5579c64c8fd6..9e18dbe196e2 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -123,6 +123,12 @@ struct led_classdev {
 	struct mutex		led_access;
 };
 
+extern void of_led_compose_name(struct device_node *parent,
+                     struct device_node *child,
+		     const char *alt_name,
+		     size_t len,
+		     char *led_name);
+
 extern int of_led_classdev_register(struct device *parent,
 				    struct device_node *np,
 				    struct led_classdev *led_cdev);
-- 
2.15.0.124.g7668cbc60

[PATCH v6 2/6] dt: bindings: lm3692x: Add bindings for lm3692x LED driver

[Dan Murphy]

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

Acked-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/leds/leds-lm3692x.txt      | 39 ++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3692x.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3692x.txt b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
new file mode 100644
index 000000000000..c259cde2226f
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3692x.txt
@@ -0,0 +1,39 @@
+* Texas Instruments - LM3692x Highly Efficient White LED Driver
+
+The LM3692x is an ultra-compact, highly efficient,
+white-LED driver designed for LCD display backlighting.
+
+The main difference between the LM36922 and LM36923 is the number of
+LED strings it supports.  The LM36922 supports two strings while the LM36923
+supports three strings.
+
+Required properties:
+	- compatible:
+		"ti,lm36922"
+		"ti,lm36923"
+	- reg :  I2C slave address
+
+Optional properties:
+	- label : see Documentation/devicetree/bindings/leds/common.txt
+	- enable-gpios : gpio pin to enable/disable the device.
+	- vled-supply : LED supply
+	- linux,default-trigger : (optional)
+	   see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+
+lm3692x@36 {
+	compatible = "ti,lm3692x";
+	reg = <0x36>;
+
+	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+	vled-supply = <&vbatt>;
+
+	backlight: backlight@0 {
+		label = "backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
+}
+
+For more product information please see the link below:
+http://www.ti.com/lit/ds/snvsa29/snvsa29.pdf
-- 
2.15.0.124.g7668cbc60

[PATCH v6 3/6] leds: lm3692x: Introduce LM3692x dual string driver

[Dan Murphy]

Introducing the LM3692x Dual-String white LED driver.

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

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

v6 - Use new LED API to compose LED label as opposed to creating it. -
https://patchwork.kernel.org/patch/10085565/

v5 - Added OF dependency in Kconfig, added extra fault flag read to ensure that
if a fault exists and it is not a artifact, fixed LED class label to be derived
from either the DT child "label" node or create a label based on 
parent_node_name:led color:trigger, removed ifdef for CONFIG_OF and removed
of_match_ptr - https://patchwork.kernel.org/patch/10081073/
v4 - Converted to devm led class register, changed MODULE_LICENSE to GPL v2, 
set the led name based on child node name or label entry, removed fault and
returned read_buf for fault checking, added mutex_destroy to remove function,
and removed LED_FULL - https://patchwork.kernel.org/patch/10060109/
v3 - Add missing Makefile and Kconfig from v1 and v2 - https://patchwork.kernel.org/patch/10060075/
v2 - Added data sheet link, fixed linuxdoc format, returned on failure in init
routine, return on fault_check failure, updated brightness calculation and
fixed capitalization issue - https://patchwork.kernel.org/patch/10056675/

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

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

[PATCH v6 4/6] dt: bindings: lp8860: Update the bindings to the standard

[Dan Murphy]

Update the lp8860 dt binding to the LED standard where
the LED should have a child node and also adding a
LED trigger entry.

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

v6 - New patch to fix binding documentation

 Documentation/devicetree/bindings/leds/leds-lp8860.txt | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
index aad38dd94d4b..4cf396de6eba 100644
--- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt
+++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
@@ -12,17 +12,24 @@ Required properties:
 	- label - Used for naming LEDs
 
 Optional properties:
-	- enable-gpio - gpio pin to enable/disable the device.
-	- supply - "vled" - LED supply
+	- enable-gpios : gpio pin to enable/disable the device.
+	- vled-supply : LED supply
+	- label : see Documentation/devicetree/bindings/leds/common.txt
+	- linux,default-trigger : (optional)
+	   see Documentation/devicetree/bindings/leds/common.txt
 
 Example:
 
-leds: leds@6 {
+lp8860@2d {
 	compatible = "ti,lp8860";
 	reg = <0x2d>;
-	label = "display_cluster";
 	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
 	vled-supply = <&vbatt>;
+
+	backlight: backlight@0 {
+		label = "backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
 }
 
 For more product information please see the link below:
-- 
2.15.0.124.g7668cbc60

Re: [PATCH v6 4/6] dt: bindings: lp8860: Update the bindings to the standard

[Jacek Anaszewski]

Dan,

On 12/01/2017 05:56 PM, Dan Murphy wrote:
> Update the lp8860 dt binding to the LED standard where
> the LED should have a child node and also adding a
> LED trigger entry.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v6 - New patch to fix binding documentation
> 
>  Documentation/devicetree/bindings/leds/leds-lp8860.txt | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
> index aad38dd94d4b..4cf396de6eba 100644
> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
> @@ -12,17 +12,24 @@ Required properties:
>  	- label - Used for naming LEDs
>  
>  Optional properties:
> -	- enable-gpio - gpio pin to enable/disable the device.
> -	- supply - "vled" - LED supply
> +	- enable-gpios : gpio pin to enable/disable the device.
> +	- vled-supply : LED supply
> +	- label : see Documentation/devicetree/bindings/leds/common.txt
> +	- linux,default-trigger : (optional)
> +	   see Documentation/devicetree/bindings/leds/common.txt
>  
>  Example:
>  
> -leds: leds@6 {
> +lp8860@2d {
>  	compatible = "ti,lp8860";
>  	reg = <0x2d>;
> -	label = "display_cluster";
>  	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>  	vled-supply = <&vbatt>;
> +
> +	backlight: backlight@0 {
> +		label = "backlight_cluster";

You'll need to change this to:

label = "white:backlight_cluster"

Please always cc your patches with DT bindings to
devicetree@vger.kernel.org and related maintainers.

> +		linux,default-trigger = "backlight";
> +	};
>  }
>  
>  For more product information please see the link below:
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v6 4/6] dt: bindings: lp8860: Update the bindings to the standard

[Rob Herring]

On Sun, Dec 03, 2017 at 02:27:20PM +0100, Jacek Anaszewski wrote:
> Dan,
> 
> On 12/01/2017 05:56 PM, Dan Murphy wrote:
> > Update the lp8860 dt binding to the LED standard where
> > the LED should have a child node and also adding a
> > LED trigger entry.
> > 
> > Signed-off-by: Dan Murphy <dmurphy@ti.com>
> > ---
> > 
> > v6 - New patch to fix binding documentation
> > 
> >  Documentation/devicetree/bindings/leds/leds-lp8860.txt | 15 +++++++++++----
> >  1 file changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
> > index aad38dd94d4b..4cf396de6eba 100644
> > --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt
> > +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
> > @@ -12,17 +12,24 @@ Required properties:
> >  	- label - Used for naming LEDs
> >  
> >  Optional properties:
> > -	- enable-gpio - gpio pin to enable/disable the device.
> > -	- supply - "vled" - LED supply
> > +	- enable-gpios : gpio pin to enable/disable the device.
> > +	- vled-supply : LED supply
> > +	- label : see Documentation/devicetree/bindings/leds/common.txt
> > +	- linux,default-trigger : (optional)
> > +	   see Documentation/devicetree/bindings/leds/common.txt
> >  
> >  Example:
> >  
> > -leds: leds@6 {
> > +lp8860@2d {

leds@2d

> >  	compatible = "ti,lp8860";
> >  	reg = <0x2d>;
> > -	label = "display_cluster";
> >  	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> >  	vled-supply = <&vbatt>;
> > +
> > +	backlight: backlight@0 {

unit-address requires a 'reg' property. Building your dts files with W=1 
will tell you this.

> > +		label = "backlight_cluster";
> 
> You'll need to change this to:
> 
> label = "white:backlight_cluster"
> 
> Please always cc your patches with DT bindings to
> devicetree@vger.kernel.org and related maintainers.
> 
> > +		linux,default-trigger = "backlight";
> > +	};
> >  }
> >  
> >  For more product information please see the link below:
> > 
> 
> -- 
> Best regards,
> Jacek Anaszewski

Re: [PATCH v6 4/6] dt: bindings: lp8860: Update the bindings to the standard

[Dan Murphy]

Rob

On 12/04/2017 04:35 PM, Rob Herring wrote:
> On Sun, Dec 03, 2017 at 02:27:20PM +0100, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 12/01/2017 05:56 PM, Dan Murphy wrote:
>>> Update the lp8860 dt binding to the LED standard where
>>> the LED should have a child node and also adding a
>>> LED trigger entry.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>>
>>> v6 - New patch to fix binding documentation
>>>
>>>  Documentation/devicetree/bindings/leds/leds-lp8860.txt | 15 +++++++++++----
>>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
>>> index aad38dd94d4b..4cf396de6eba 100644
>>> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt
>>> +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
>>> @@ -12,17 +12,24 @@ Required properties:
>>>  	- label - Used for naming LEDs
>>>  
>>>  Optional properties:
>>> -	- enable-gpio - gpio pin to enable/disable the device.
>>> -	- supply - "vled" - LED supply
>>> +	- enable-gpios : gpio pin to enable/disable the device.
>>> +	- vled-supply : LED supply
>>> +	- label : see Documentation/devicetree/bindings/leds/common.txt
>>> +	- linux,default-trigger : (optional)
>>> +	   see Documentation/devicetree/bindings/leds/common.txt
>>>  
>>>  Example:
>>>  
>>> -leds: leds@6 {
>>> +lp8860@2d {
> 
> leds@2d

Ack

> 
>>>  	compatible = "ti,lp8860";
>>>  	reg = <0x2d>;
>>> -	label = "display_cluster";
>>>  	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>>>  	vled-supply = <&vbatt>;
>>> +
>>> +	backlight: backlight@0 {
> 
> unit-address requires a 'reg' property. Building your dts files with W=1 
> will tell you this.

I will add this.  There is so much noise when enabling this option on the dts
I missed the warning.

Dan

<snip>

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

Re: [PATCH v6 4/6] dt: bindings: lp8860: Update the bindings to the standard

[Jacek Anaszewski]

Hi Dan,

Thanks for the patch.

On 12/01/2017 05:56 PM, Dan Murphy wrote:
> Update the lp8860 dt binding to the LED standard where
> the LED should have a child node and also adding a
> LED trigger entry.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v6 - New patch to fix binding documentation
> 
>  Documentation/devicetree/bindings/leds/leds-lp8860.txt | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
> index aad38dd94d4b..4cf396de6eba 100644
> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt
> +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
> @@ -12,17 +12,24 @@ Required properties:
>  	- label - Used for naming LEDs
>  
>  Optional properties:
> -	- enable-gpio - gpio pin to enable/disable the device.
> -	- supply - "vled" - LED supply
> +	- enable-gpios : gpio pin to enable/disable the device.
> +	- vled-supply : LED supply
> +	- label : see Documentation/devicetree/bindings/leds/common.txt
> +	- linux,default-trigger : (optional)
> +	   see Documentation/devicetree/bindings/leds/common.txt

Related driver doesn't parse this DT property AFAICS.

>  Example:
>  
> -leds: leds@6 {
> +lp8860@2d {
>  	compatible = "ti,lp8860";
>  	reg = <0x2d>;
> -	label = "display_cluster";
>  	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;

s/gpio/gpios/

>  	vled-supply = <&vbatt>;
> +
> +	backlight: backlight@0 {
> +		label = "backlight_cluster";
> +		linux,default-trigger = "backlight";
> +	};
>  }
>  
>  For more product information please see the link below:
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v6 4/6] dt: bindings: lp8860: Update the bindings to the standard

[Jacek Anaszewski]

On 12/03/2017 02:49 PM, Jacek Anaszewski wrote:
> Hi Dan,
> 
> Thanks for the patch.
> 
> On 12/01/2017 05:56 PM, Dan Murphy wrote:
>> Update the lp8860 dt binding to the LED standard where
>> the LED should have a child node and also adding a
>> LED trigger entry.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v6 - New patch to fix binding documentation
>>
>>  Documentation/devicetree/bindings/leds/leds-lp8860.txt | 15 +++++++++++----
>>  1 file changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-lp8860.txt b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
>> index aad38dd94d4b..4cf396de6eba 100644
>> --- a/Documentation/devicetree/bindings/leds/leds-lp8860.txt
>> +++ b/Documentation/devicetree/bindings/leds/leds-lp8860.txt
>> @@ -12,17 +12,24 @@ Required properties:
>>  	- label - Used for naming LEDs
>>  
>>  Optional properties:
>> -	- enable-gpio - gpio pin to enable/disable the device.
>> -	- supply - "vled" - LED supply
>> +	- enable-gpios : gpio pin to enable/disable the device.
>> +	- vled-supply : LED supply
>> +	- label : see Documentation/devicetree/bindings/leds/common.txt
>> +	- linux,default-trigger : (optional)
>> +	   see Documentation/devicetree/bindings/leds/common.txt
> 
> Related driver doesn't parse this DT property AFAICS.

Ah, I missed that you're adding it in 5/6.

> 
>>  Example:
>>  
>> -leds: leds@6 {
>> +lp8860@2d {
>>  	compatible = "ti,lp8860";
>>  	reg = <0x2d>;
>> -	label = "display_cluster";
>>  	enable-gpio = <&gpio1 28 GPIO_ACTIVE_HIGH>;
> 
> s/gpio/gpios/
> 
>>  	vled-supply = <&vbatt>;
>> +
>> +	backlight: backlight@0 {
>> +		label = "backlight_cluster";
>> +		linux,default-trigger = "backlight";
>> +	};
>>  }
>>  
>>  For more product information please see the link below:
>>
> 

[PATCH v6 5/6] leds: lp8860: Update the LED label generation

[Dan Murphy]

Fix the LED label generation for the LP8860 to
conform with the

Documentation/devicetree/bindings/leds/common.txt

document indicating the LED label should be part of a
child node to the device parent.  If no label is
in the child node then the LED label is created based
on the parent node name and the alternate name passed in.

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

v6 - New patch to use the new LED class API

 drivers/leds/leds-lp8860.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
index 3e70775a2d54..26bbfa144402 100644
--- a/drivers/leds/leds-lp8860.c
+++ b/drivers/leds/leds-lp8860.c
@@ -22,6 +22,7 @@
 #include <linux/of_gpio.h>
 #include <linux/gpio/consumer.h>
 #include <linux/slab.h>
+#include <uapi/linux/uleds.h>
 
 #define LP8860_DISP_CL1_BRT_MSB		0x00
 #define LP8860_DISP_CL1_BRT_LSB		0x01
@@ -86,8 +87,6 @@
 
 #define LP8860_CLEAR_FAULTS		0x01
 
-#define LP8860_DISP_LED_NAME		"display_cluster"
-
 /**
  * struct lp8860_led -
  * @lock - Lock for reading/writing the device
@@ -107,7 +106,7 @@ struct lp8860_led {
 	struct regmap *eeprom_regmap;
 	struct gpio_desc *enable_gpio;
 	struct regulator *regulator;
-	const char *label;
+	char label[LED_MAX_NAME_SIZE];
 };
 
 struct lp8860_eeprom_reg {
@@ -318,7 +317,7 @@ static const struct regmap_config lp8860_regmap_config = {
 	.max_register = LP8860_EEPROM_UNLOCK,
 	.reg_defaults = lp8860_reg_defs,
 	.num_reg_defaults = ARRAY_SIZE(lp8860_reg_defs),
-	.cache_type = REGCACHE_NONE,
+	.cache_type = REGCACHE_RBTREE,
 };
 
 static const struct reg_default lp8860_eeprom_defs[] = {
@@ -356,7 +355,7 @@ static const struct regmap_config lp8860_eeprom_regmap_config = {
 	.max_register = LP8860_EEPROM_REG_24,
 	.reg_defaults = lp8860_eeprom_defs,
 	.num_reg_defaults = ARRAY_SIZE(lp8860_eeprom_defs),
-	.cache_type = REGCACHE_NONE,
+	.cache_type = REGCACHE_RBTREE,
 };
 
 static int lp8860_probe(struct i2c_client *client,
@@ -365,19 +364,23 @@ static int lp8860_probe(struct i2c_client *client,
 	int ret;
 	struct lp8860_led *led;
 	struct device_node *np = client->dev.of_node;
+	struct device_node *child_node;
+
+	if (!client->dev.of_node)
+		return -ENODEV;
 
 	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
 	if (!led)
 		return -ENOMEM;
 
-	led->label = LP8860_DISP_LED_NAME;
+	for_each_available_child_of_node(np, child_node) {
+		led->led_dev.default_trigger = of_get_property(child_node,
+						    "linux,default-trigger",
+						    NULL);
 
-	if (client->dev.of_node) {
-		ret = of_property_read_string(np, "label", &led->label);
-		if (ret) {
-			dev_err(&client->dev, "Missing label in dt\n");
-			return -EINVAL;
-		}
+		of_led_compose_name(np, child_node, "white:backlight",
+				sizeof("white:backlight"),
+				led->label);
 	}
 
 	led->enable_gpio = devm_gpiod_get_optional(&client->dev,
-- 
2.15.0.124.g7668cbc60

Re: [PATCH v6 5/6] leds: lp8860: Update the LED label generation

[Dan Murphy]

On 12/01/2017 10:56 AM, Dan Murphy wrote:
> Fix the LED label generation for the LP8860 to
> conform with the
> 
> Documentation/devicetree/bindings/leds/common.txt
> 
> document indicating the LED label should be part of a
> child node to the device parent.  If no label is
> in the child node then the LED label is created based
> on the parent node name and the alternate name passed in.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v6 - New patch to use the new LED class API
> 
>  drivers/leds/leds-lp8860.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
> index 3e70775a2d54..26bbfa144402 100644
> --- a/drivers/leds/leds-lp8860.c
> +++ b/drivers/leds/leds-lp8860.c
> @@ -22,6 +22,7 @@
>  #include <linux/of_gpio.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/slab.h>
> +#include <uapi/linux/uleds.h>
>  
>  #define LP8860_DISP_CL1_BRT_MSB		0x00
>  #define LP8860_DISP_CL1_BRT_LSB		0x01
> @@ -86,8 +87,6 @@
>  
>  #define LP8860_CLEAR_FAULTS		0x01
>  
> -#define LP8860_DISP_LED_NAME		"display_cluster"
> -
>  /**
>   * struct lp8860_led -
>   * @lock - Lock for reading/writing the device
> @@ -107,7 +106,7 @@ struct lp8860_led {
>  	struct regmap *eeprom_regmap;
>  	struct gpio_desc *enable_gpio;
>  	struct regulator *regulator;
> -	const char *label;
> +	char label[LED_MAX_NAME_SIZE];
>  };
>  
>  struct lp8860_eeprom_reg {
> @@ -318,7 +317,7 @@ static const struct regmap_config lp8860_regmap_config = {
>  	.max_register = LP8860_EEPROM_UNLOCK,
>  	.reg_defaults = lp8860_reg_defs,
>  	.num_reg_defaults = ARRAY_SIZE(lp8860_reg_defs),
> -	.cache_type = REGCACHE_NONE,
> +	.cache_type = REGCACHE_RBTREE,
>  };
>  
>  static const struct reg_default lp8860_eeprom_defs[] = {
> @@ -356,7 +355,7 @@ static const struct regmap_config lp8860_eeprom_regmap_config = {
>  	.max_register = LP8860_EEPROM_REG_24,
>  	.reg_defaults = lp8860_eeprom_defs,
>  	.num_reg_defaults = ARRAY_SIZE(lp8860_eeprom_defs),
> -	.cache_type = REGCACHE_NONE,
> +	.cache_type = REGCACHE_RBTREE,
>  };
>  
>  static int lp8860_probe(struct i2c_client *client,
> @@ -365,19 +364,23 @@ static int lp8860_probe(struct i2c_client *client,
>  	int ret;
>  	struct lp8860_led *led;
>  	struct device_node *np = client->dev.of_node;
> +	struct device_node *child_node;
> +
> +	if (!client->dev.of_node)
> +		return -ENODEV;

This is actually a bug in the driver.  I can submit this independently or keep this here

>  
>  	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
>  	if (!led)
>  		return -ENOMEM;
>  
> -	led->label = LP8860_DISP_LED_NAME;
> +	for_each_available_child_of_node(np, child_node) {
> +		led->led_dev.default_trigger = of_get_property(child_node,
> +						    "linux,default-trigger",
> +						    NULL);

Not sure if you want me to pull this out into a different patch or just add it to the
commit message

>  
> -	if (client->dev.of_node) {
> -		ret = of_property_read_string(np, "label", &led->label);
> -		if (ret) {
> -			dev_err(&client->dev, "Missing label in dt\n");
> -			return -EINVAL;
> -		}
> +		of_led_compose_name(np, child_node, "white:backlight",
> +				sizeof("white:backlight"),
> +				led->label);
>  	}
>  
>  	led->enable_gpio = devm_gpiod_get_optional(&client->dev,
> 


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

Re: [PATCH v6 5/6] leds: lp8860: Update the LED label generation

[Jacek Anaszewski]

Dan,

On 12/01/2017 05:59 PM, Dan Murphy wrote:
> On 12/01/2017 10:56 AM, Dan Murphy wrote:
>> Fix the LED label generation for the LP8860 to
>> conform with the
>>
>> Documentation/devicetree/bindings/leds/common.txt
>>
>> document indicating the LED label should be part of a
>> child node to the device parent.  If no label is
>> in the child node then the LED label is created based
>> on the parent node name and the alternate name passed in.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v6 - New patch to use the new LED class API
>>
>>  drivers/leds/leds-lp8860.c | 27 +++++++++++++++------------
>>  1 file changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
>> index 3e70775a2d54..26bbfa144402 100644
>> --- a/drivers/leds/leds-lp8860.c
>> +++ b/drivers/leds/leds-lp8860.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/of_gpio.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/slab.h>
>> +#include <uapi/linux/uleds.h>
>>  
>>  #define LP8860_DISP_CL1_BRT_MSB		0x00
>>  #define LP8860_DISP_CL1_BRT_LSB		0x01
>> @@ -86,8 +87,6 @@
>>  
>>  #define LP8860_CLEAR_FAULTS		0x01
>>  
>> -#define LP8860_DISP_LED_NAME		"display_cluster"
>> -
>>  /**
>>   * struct lp8860_led -
>>   * @lock - Lock for reading/writing the device
>> @@ -107,7 +106,7 @@ struct lp8860_led {
>>  	struct regmap *eeprom_regmap;
>>  	struct gpio_desc *enable_gpio;
>>  	struct regulator *regulator;
>> -	const char *label;
>> +	char label[LED_MAX_NAME_SIZE];
>>  };
>>  
>>  struct lp8860_eeprom_reg {
>> @@ -318,7 +317,7 @@ static const struct regmap_config lp8860_regmap_config = {
>>  	.max_register = LP8860_EEPROM_UNLOCK,
>>  	.reg_defaults = lp8860_reg_defs,
>>  	.num_reg_defaults = ARRAY_SIZE(lp8860_reg_defs),
>> -	.cache_type = REGCACHE_NONE,
>> +	.cache_type = REGCACHE_RBTREE,
>>  };
>>  
>>  static const struct reg_default lp8860_eeprom_defs[] = {
>> @@ -356,7 +355,7 @@ static const struct regmap_config lp8860_eeprom_regmap_config = {
>>  	.max_register = LP8860_EEPROM_REG_24,
>>  	.reg_defaults = lp8860_eeprom_defs,
>>  	.num_reg_defaults = ARRAY_SIZE(lp8860_eeprom_defs),
>> -	.cache_type = REGCACHE_NONE,
>> +	.cache_type = REGCACHE_RBTREE,
>>  };
>>  
>>  static int lp8860_probe(struct i2c_client *client,
>> @@ -365,19 +364,23 @@ static int lp8860_probe(struct i2c_client *client,
>>  	int ret;
>>  	struct lp8860_led *led;
>>  	struct device_node *np = client->dev.of_node;
>> +	struct device_node *child_node;
>> +
>> +	if (!client->dev.of_node)
>> +		return -ENODEV;
> 
> This is actually a bug in the driver.  I can submit this independently or keep this here
> 
>>  
>>  	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
>>  	if (!led)
>>  		return -ENOMEM;
>>  
>> -	led->label = LP8860_DISP_LED_NAME;
>> +	for_each_available_child_of_node(np, child_node) {
>> +		led->led_dev.default_trigger = of_get_property(child_node,
>> +						    "linux,default-trigger",
>> +						    NULL);
> 
> Not sure if you want me to pull this out into a different patch or just add it to the
> commit message

Yes, it would be nice. Also related change in DT bindings should be
placed in the same patch.

>>  
>> -	if (client->dev.of_node) {
>> -		ret = of_property_read_string(np, "label", &led->label);
>> -		if (ret) {
>> -			dev_err(&client->dev, "Missing label in dt\n");
>> -			return -EINVAL;
>> -		}
>> +		of_led_compose_name(np, child_node, "white:backlight",
>> +				sizeof("white:backlight"),
>> +				led->label);
>>  	}
>>  
>>  	led->enable_gpio = devm_gpiod_get_optional(&client->dev,
>>
> 
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v6 5/6] leds: lp8860: Update the LED label generation

[Jacek Anaszewski]

Dan,

On 12/01/2017 05:56 PM, Dan Murphy wrote:
> Fix the LED label generation for the LP8860 to
> conform with the
> 
> Documentation/devicetree/bindings/leds/common.txt
> 
> document indicating the LED label should be part of a
> child node to the device parent.  If no label is
> in the child node then the LED label is created based
> on the parent node name and the alternate name passed in.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v6 - New patch to use the new LED class API
> 
>  drivers/leds/leds-lp8860.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
> index 3e70775a2d54..26bbfa144402 100644
> --- a/drivers/leds/leds-lp8860.c
> +++ b/drivers/leds/leds-lp8860.c
> @@ -22,6 +22,7 @@
>  #include <linux/of_gpio.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/slab.h>
> +#include <uapi/linux/uleds.h>
>  
>  #define LP8860_DISP_CL1_BRT_MSB		0x00
>  #define LP8860_DISP_CL1_BRT_LSB		0x01
> @@ -86,8 +87,6 @@
>  
>  #define LP8860_CLEAR_FAULTS		0x01
>  
> -#define LP8860_DISP_LED_NAME		"display_cluster"
> -
>  /**
>   * struct lp8860_led -
>   * @lock - Lock for reading/writing the device
> @@ -107,7 +106,7 @@ struct lp8860_led {
>  	struct regmap *eeprom_regmap;
>  	struct gpio_desc *enable_gpio;
>  	struct regulator *regulator;
> -	const char *label;
> +	char label[LED_MAX_NAME_SIZE];
>  };
>  
>  struct lp8860_eeprom_reg {
> @@ -318,7 +317,7 @@ static const struct regmap_config lp8860_regmap_config = {
>  	.max_register = LP8860_EEPROM_UNLOCK,
>  	.reg_defaults = lp8860_reg_defs,
>  	.num_reg_defaults = ARRAY_SIZE(lp8860_reg_defs),
> -	.cache_type = REGCACHE_NONE,
> +	.cache_type = REGCACHE_RBTREE,

This seems to be an unrelated change.
Please split it to the separate patch and explain its merit.

>  };
>  
>  static const struct reg_default lp8860_eeprom_defs[] = {
> @@ -356,7 +355,7 @@ static const struct regmap_config lp8860_eeprom_regmap_config = {
>  	.max_register = LP8860_EEPROM_REG_24,
>  	.reg_defaults = lp8860_eeprom_defs,
>  	.num_reg_defaults = ARRAY_SIZE(lp8860_eeprom_defs),
> -	.cache_type = REGCACHE_NONE,
> +	.cache_type = REGCACHE_RBTREE,
>  };
>  
>  static int lp8860_probe(struct i2c_client *client,
> @@ -365,19 +364,23 @@ static int lp8860_probe(struct i2c_client *client,
>  	int ret;
>  	struct lp8860_led *led;
>  	struct device_node *np = client->dev.of_node;
> +	struct device_node *child_node;
> +
> +	if (!client->dev.of_node)
> +		return -ENODEV;
>  
>  	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
>  	if (!led)
>  		return -ENOMEM;
>  
> -	led->label = LP8860_DISP_LED_NAME;
> +	for_each_available_child_of_node(np, child_node) {
> +		led->led_dev.default_trigger = of_get_property(child_node,
> +						    "linux,default-trigger",
> +						    NULL);
>  
> -	if (client->dev.of_node) {
> -		ret = of_property_read_string(np, "label", &led->label);
> -		if (ret) {
> -			dev_err(&client->dev, "Missing label in dt\n");
> -			return -EINVAL;
> -		}
> +		of_led_compose_name(np, child_node, "white:backlight",
> +				sizeof("white:backlight"),
> +				led->label);

Let's skip it for now.

Please also CC driver author always when you're modifying it.

>  	}
>  
>  	led->enable_gpio = devm_gpiod_get_optional(&client->dev,
> 

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v6 5/6] leds: lp8860: Update the LED label generation

[Dan Murphy]

Jacek

On 12/03/2017 07:57 AM, Jacek Anaszewski wrote:
> Dan,
> 
> On 12/01/2017 05:56 PM, Dan Murphy wrote:
>> Fix the LED label generation for the LP8860 to
>> conform with the
>>
>> Documentation/devicetree/bindings/leds/common.txt
>>
>> document indicating the LED label should be part of a
>> child node to the device parent.  If no label is
>> in the child node then the LED label is created based
>> on the parent node name and the alternate name passed in.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v6 - New patch to use the new LED class API
>>
>>  drivers/leds/leds-lp8860.c | 27 +++++++++++++++------------
>>  1 file changed, 15 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
>> index 3e70775a2d54..26bbfa144402 100644
>> --- a/drivers/leds/leds-lp8860.c
>> +++ b/drivers/leds/leds-lp8860.c
>> @@ -22,6 +22,7 @@
>>  #include <linux/of_gpio.h>
>>  #include <linux/gpio/consumer.h>
>>  #include <linux/slab.h>
>> +#include <uapi/linux/uleds.h>
>>  
>>  #define LP8860_DISP_CL1_BRT_MSB		0x00
>>  #define LP8860_DISP_CL1_BRT_LSB		0x01
>> @@ -86,8 +87,6 @@
>>  
>>  #define LP8860_CLEAR_FAULTS		0x01
>>  
>> -#define LP8860_DISP_LED_NAME		"display_cluster"
>> -
>>  /**
>>   * struct lp8860_led -
>>   * @lock - Lock for reading/writing the device
>> @@ -107,7 +106,7 @@ struct lp8860_led {
>>  	struct regmap *eeprom_regmap;
>>  	struct gpio_desc *enable_gpio;
>>  	struct regulator *regulator;
>> -	const char *label;
>> +	char label[LED_MAX_NAME_SIZE];
>>  };
>>  
>>  struct lp8860_eeprom_reg {
>> @@ -318,7 +317,7 @@ static const struct regmap_config lp8860_regmap_config = {
>>  	.max_register = LP8860_EEPROM_UNLOCK,
>>  	.reg_defaults = lp8860_reg_defs,
>>  	.num_reg_defaults = ARRAY_SIZE(lp8860_reg_defs),
>> -	.cache_type = REGCACHE_NONE,
>> +	.cache_type = REGCACHE_RBTREE,
> 
> This seems to be an unrelated change.
> Please split it to the separate patch and explain its merit.

ACK.  It will be a separate patch

> 
>>  };
>>  
>>  static const struct reg_default lp8860_eeprom_defs[] = {
>> @@ -356,7 +355,7 @@ static const struct regmap_config lp8860_eeprom_regmap_config = {
>>  	.max_register = LP8860_EEPROM_REG_24,
>>  	.reg_defaults = lp8860_eeprom_defs,
>>  	.num_reg_defaults = ARRAY_SIZE(lp8860_eeprom_defs),
>> -	.cache_type = REGCACHE_NONE,
>> +	.cache_type = REGCACHE_RBTREE,
>>  };
>>  
>>  static int lp8860_probe(struct i2c_client *client,
>> @@ -365,19 +364,23 @@ static int lp8860_probe(struct i2c_client *client,
>>  	int ret;
>>  	struct lp8860_led *led;
>>  	struct device_node *np = client->dev.of_node;
>> +	struct device_node *child_node;
>> +
>> +	if (!client->dev.of_node)
>> +		return -ENODEV;
>>  
>>  	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
>>  	if (!led)
>>  		return -ENOMEM;
>>  
>> -	led->label = LP8860_DISP_LED_NAME;
>> +	for_each_available_child_of_node(np, child_node) {
>> +		led->led_dev.default_trigger = of_get_property(child_node,
>> +						    "linux,default-trigger",
>> +						    NULL);
>>  
>> -	if (client->dev.of_node) {
>> -		ret = of_property_read_string(np, "label", &led->label);
>> -		if (ret) {
>> -			dev_err(&client->dev, "Missing label in dt\n");
>> -			return -EINVAL;
>> -		}
>> +		of_led_compose_name(np, child_node, "white:backlight",
>> +				sizeof("white:backlight"),
>> +				led->label);
> 
> Let's skip it for now.

I will make the same change here as I do for the lm3692x driver.

> 
> Please also CC driver author always when you're modifying it.

The author was on the email.

It is me.  ;)

MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");

Dan



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

Re: [PATCH v6 5/6] leds: lp8860: Update the LED label generation

[Jacek Anaszewski]

Dan,

On 12/04/2017 02:11 PM, Dan Murphy wrote:
> Jacek
> 
> On 12/03/2017 07:57 AM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 12/01/2017 05:56 PM, Dan Murphy wrote:
>>> Fix the LED label generation for the LP8860 to
>>> conform with the
>>>
>>> Documentation/devicetree/bindings/leds/common.txt
>>>
>>> document indicating the LED label should be part of a
>>> child node to the device parent.  If no label is
>>> in the child node then the LED label is created based
>>> on the parent node name and the alternate name passed in.
>>>
>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>> ---
>>>
>>> v6 - New patch to use the new LED class API
>>>
>>>  drivers/leds/leds-lp8860.c | 27 +++++++++++++++------------
>>>  1 file changed, 15 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
>>> index 3e70775a2d54..26bbfa144402 100644
>>> --- a/drivers/leds/leds-lp8860.c
>>> +++ b/drivers/leds/leds-lp8860.c
>>> @@ -22,6 +22,7 @@
>>>  #include <linux/of_gpio.h>
>>>  #include <linux/gpio/consumer.h>
>>>  #include <linux/slab.h>
>>> +#include <uapi/linux/uleds.h>
>>>  
>>>  #define LP8860_DISP_CL1_BRT_MSB		0x00
>>>  #define LP8860_DISP_CL1_BRT_LSB		0x01
>>> @@ -86,8 +87,6 @@
>>>  
>>>  #define LP8860_CLEAR_FAULTS		0x01
>>>  
>>> -#define LP8860_DISP_LED_NAME		"display_cluster"
>>> -
>>>  /**
>>>   * struct lp8860_led -
>>>   * @lock - Lock for reading/writing the device
>>> @@ -107,7 +106,7 @@ struct lp8860_led {
>>>  	struct regmap *eeprom_regmap;
>>>  	struct gpio_desc *enable_gpio;
>>>  	struct regulator *regulator;
>>> -	const char *label;
>>> +	char label[LED_MAX_NAME_SIZE];
>>>  };
>>>  
>>>  struct lp8860_eeprom_reg {
>>> @@ -318,7 +317,7 @@ static const struct regmap_config lp8860_regmap_config = {
>>>  	.max_register = LP8860_EEPROM_UNLOCK,
>>>  	.reg_defaults = lp8860_reg_defs,
>>>  	.num_reg_defaults = ARRAY_SIZE(lp8860_reg_defs),
>>> -	.cache_type = REGCACHE_NONE,
>>> +	.cache_type = REGCACHE_RBTREE,
>>
>> This seems to be an unrelated change.
>> Please split it to the separate patch and explain its merit.
> 
> ACK.  It will be a separate patch
> 
>>
>>>  };
>>>  
>>>  static const struct reg_default lp8860_eeprom_defs[] = {
>>> @@ -356,7 +355,7 @@ static const struct regmap_config lp8860_eeprom_regmap_config = {
>>>  	.max_register = LP8860_EEPROM_REG_24,
>>>  	.reg_defaults = lp8860_eeprom_defs,
>>>  	.num_reg_defaults = ARRAY_SIZE(lp8860_eeprom_defs),
>>> -	.cache_type = REGCACHE_NONE,
>>> +	.cache_type = REGCACHE_RBTREE,
>>>  };
>>>  
>>>  static int lp8860_probe(struct i2c_client *client,
>>> @@ -365,19 +364,23 @@ static int lp8860_probe(struct i2c_client *client,
>>>  	int ret;
>>>  	struct lp8860_led *led;
>>>  	struct device_node *np = client->dev.of_node;
>>> +	struct device_node *child_node;
>>> +
>>> +	if (!client->dev.of_node)
>>> +		return -ENODEV;
>>>  
>>>  	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
>>>  	if (!led)
>>>  		return -ENOMEM;
>>>  
>>> -	led->label = LP8860_DISP_LED_NAME;
>>> +	for_each_available_child_of_node(np, child_node) {
>>> +		led->led_dev.default_trigger = of_get_property(child_node,
>>> +						    "linux,default-trigger",
>>> +						    NULL);
>>>  
>>> -	if (client->dev.of_node) {
>>> -		ret = of_property_read_string(np, "label", &led->label);
>>> -		if (ret) {
>>> -			dev_err(&client->dev, "Missing label in dt\n");
>>> -			return -EINVAL;
>>> -		}
>>> +		of_led_compose_name(np, child_node, "white:backlight",
>>> +				sizeof("white:backlight"),
>>> +				led->label);
>>
>> Let's skip it for now.
> 
> I will make the same change here as I do for the lm3692x driver.
> 
>>
>> Please also CC driver author always when you're modifying it.
> 
> The author was on the email.
> 
> It is me.  ;)
> 
> MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");


OK, but the author of the driver touched by the patch 6/6
is different :-)

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v6 5/6] leds: lp8860: Update the LED label generation

[Dan Murphy]

Jacek

On 12/05/2017 01:56 PM, Jacek Anaszewski wrote:
> Dan,
> 
> On 12/04/2017 02:11 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 12/03/2017 07:57 AM, Jacek Anaszewski wrote:
>>> Dan,
>>>
>>> On 12/01/2017 05:56 PM, Dan Murphy wrote:
>>>> Fix the LED label generation for the LP8860 to
>>>> conform with the
>>>>
>>>> Documentation/devicetree/bindings/leds/common.txt
>>>>
>>>> document indicating the LED label should be part of a
>>>> child node to the device parent.  If no label is
>>>> in the child node then the LED label is created based
>>>> on the parent node name and the alternate name passed in.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>> ---
>>>>
>>>> v6 - New patch to use the new LED class API
>>>>
>>>>  drivers/leds/leds-lp8860.c | 27 +++++++++++++++------------
>>>>  1 file changed, 15 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/leds/leds-lp8860.c b/drivers/leds/leds-lp8860.c
>>>> index 3e70775a2d54..26bbfa144402 100644
>>>> --- a/drivers/leds/leds-lp8860.c
>>>> +++ b/drivers/leds/leds-lp8860.c
>>>> @@ -22,6 +22,7 @@
>>>>  #include <linux/of_gpio.h>
>>>>  #include <linux/gpio/consumer.h>
>>>>  #include <linux/slab.h>
>>>> +#include <uapi/linux/uleds.h>
>>>>  
>>>>  #define LP8860_DISP_CL1_BRT_MSB		0x00
>>>>  #define LP8860_DISP_CL1_BRT_LSB		0x01
>>>> @@ -86,8 +87,6 @@
>>>>  
>>>>  #define LP8860_CLEAR_FAULTS		0x01
>>>>  
>>>> -#define LP8860_DISP_LED_NAME		"display_cluster"
>>>> -
>>>>  /**
>>>>   * struct lp8860_led -
>>>>   * @lock - Lock for reading/writing the device
>>>> @@ -107,7 +106,7 @@ struct lp8860_led {
>>>>  	struct regmap *eeprom_regmap;
>>>>  	struct gpio_desc *enable_gpio;
>>>>  	struct regulator *regulator;
>>>> -	const char *label;
>>>> +	char label[LED_MAX_NAME_SIZE];
>>>>  };
>>>>  
>>>>  struct lp8860_eeprom_reg {
>>>> @@ -318,7 +317,7 @@ static const struct regmap_config lp8860_regmap_config = {
>>>>  	.max_register = LP8860_EEPROM_UNLOCK,
>>>>  	.reg_defaults = lp8860_reg_defs,
>>>>  	.num_reg_defaults = ARRAY_SIZE(lp8860_reg_defs),
>>>> -	.cache_type = REGCACHE_NONE,
>>>> +	.cache_type = REGCACHE_RBTREE,
>>>
>>> This seems to be an unrelated change.
>>> Please split it to the separate patch and explain its merit.
>>
>> ACK.  It will be a separate patch
>>
>>>
>>>>  };
>>>>  
>>>>  static const struct reg_default lp8860_eeprom_defs[] = {
>>>> @@ -356,7 +355,7 @@ static const struct regmap_config lp8860_eeprom_regmap_config = {
>>>>  	.max_register = LP8860_EEPROM_REG_24,
>>>>  	.reg_defaults = lp8860_eeprom_defs,
>>>>  	.num_reg_defaults = ARRAY_SIZE(lp8860_eeprom_defs),
>>>> -	.cache_type = REGCACHE_NONE,
>>>> +	.cache_type = REGCACHE_RBTREE,
>>>>  };
>>>>  
>>>>  static int lp8860_probe(struct i2c_client *client,
>>>> @@ -365,19 +364,23 @@ static int lp8860_probe(struct i2c_client *client,
>>>>  	int ret;
>>>>  	struct lp8860_led *led;
>>>>  	struct device_node *np = client->dev.of_node;
>>>> +	struct device_node *child_node;
>>>> +
>>>> +	if (!client->dev.of_node)
>>>> +		return -ENODEV;
>>>>  
>>>>  	led = devm_kzalloc(&client->dev, sizeof(*led), GFP_KERNEL);
>>>>  	if (!led)
>>>>  		return -ENOMEM;
>>>>  
>>>> -	led->label = LP8860_DISP_LED_NAME;
>>>> +	for_each_available_child_of_node(np, child_node) {
>>>> +		led->led_dev.default_trigger = of_get_property(child_node,
>>>> +						    "linux,default-trigger",
>>>> +						    NULL);
>>>>  
>>>> -	if (client->dev.of_node) {
>>>> -		ret = of_property_read_string(np, "label", &led->label);
>>>> -		if (ret) {
>>>> -			dev_err(&client->dev, "Missing label in dt\n");
>>>> -			return -EINVAL;
>>>> -		}
>>>> +		of_led_compose_name(np, child_node, "white:backlight",
>>>> +				sizeof("white:backlight"),
>>>> +				led->label);
>>>
>>> Let's skip it for now.
>>
>> I will make the same change here as I do for the lm3692x driver.
>>
>>>
>>> Please also CC driver author always when you're modifying it.
>>
>> The author was on the email.
>>
>> It is me.  ;)
>>
>> MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
> 
> 
> OK, but the author of the driver touched by the patch 6/6
> is different :-)
> 

True.  This patch was dropped in v7.



Dan

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

[PATCH v6 6/6] leds: as3645a: Update the LED label generation

[Dan Murphy]

Update the LED label creationg to call the
of_led_compose_name api to generate a label.

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

v6 - New patch to use the new LED class API

 drivers/leds/leds-as3645a.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/leds/leds-as3645a.c b/drivers/leds/leds-as3645a.c
index 9a257f969300..5a29db115277 100644
--- a/drivers/leds/leds-as3645a.c
+++ b/drivers/leds/leds-as3645a.c
@@ -27,6 +27,7 @@
 #include <linux/mutex.h>
 #include <linux/of.h>
 #include <linux/slab.h>
+#include <uapi/linux/uleds.h>
 
 #include <media/v4l2-flash-led-class.h>
 
@@ -133,8 +134,8 @@ struct as3645a_config {
 };
 
 struct as3645a_names {
-	char flash[32];
-	char indicator[32];
+	char flash[LED_MAX_NAME_SIZE];
+	char indicator[LED_MAX_NAME_SIZE];
 };
 
 struct as3645a {
@@ -496,7 +497,6 @@ static int as3645a_parse_node(struct as3645a *flash,
 {
 	struct as3645a_config *cfg = &flash->cfg;
 	struct device_node *child;
-	const char *name;
 	int rval;
 
 	for_each_child_of_node(node, child) {
@@ -523,12 +523,9 @@ static int as3645a_parse_node(struct as3645a *flash,
 		return -ENODEV;
 	}
 
-	rval = of_property_read_string(flash->flash_node, "label", &name);
-	if (!rval)
-		strlcpy(names->flash, name, sizeof(names->flash));
-	else
-		snprintf(names->flash, sizeof(names->flash),
-			 "%s:flash", node->name);
+	of_led_compose_name(node, flash->flash_node, "flash",
+				sizeof("flash"),
+				names->flash);
 
 	rval = of_property_read_u32(flash->flash_node, "flash-timeout-us",
 				    &cfg->flash_timeout_us);
@@ -567,12 +564,9 @@ static int as3645a_parse_node(struct as3645a *flash,
 		goto out_err;
 	}
 
-	rval = of_property_read_string(flash->indicator_node, "label", &name);
-	if (!rval)
-		strlcpy(names->indicator, name, sizeof(names->indicator));
-	else
-		snprintf(names->indicator, sizeof(names->indicator),
-			 "%s:indicator", node->name);
+	of_led_compose_name(node, flash->indicator_node, "indicator",
+				sizeof("indicator"),
+				names->indicator);
 
 	rval = of_property_read_u32(flash->indicator_node, "led-max-microamp",
 				    &cfg->indicator_max_ua);
-- 
2.15.0.124.g7668cbc60

Re: [PATCH v6 6/6] leds: as3645a: Update the LED label generation

[Dan Murphy]

On 12/01/2017 10:56 AM, Dan Murphy wrote:
> Update the LED label creationg to call the

s/creationg/creation

> of_led_compose_name api to generate a label.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v6 - New patch to use the new LED class API
> 
>  drivers/leds/leds-as3645a.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/leds/leds-as3645a.c b/drivers/leds/leds-as3645a.c
> index 9a257f969300..5a29db115277 100644
> --- a/drivers/leds/leds-as3645a.c
> +++ b/drivers/leds/leds-as3645a.c
> @@ -27,6 +27,7 @@
>  #include <linux/mutex.h>
>  #include <linux/of.h>
>  #include <linux/slab.h>
> +#include <uapi/linux/uleds.h>
>  
>  #include <media/v4l2-flash-led-class.h>
>  
> @@ -133,8 +134,8 @@ struct as3645a_config {
>  };
>  
>  struct as3645a_names {
> -	char flash[32];
> -	char indicator[32];
> +	char flash[LED_MAX_NAME_SIZE];
> +	char indicator[LED_MAX_NAME_SIZE];
>  };
>  
>  struct as3645a {
> @@ -496,7 +497,6 @@ static int as3645a_parse_node(struct as3645a *flash,
>  {
>  	struct as3645a_config *cfg = &flash->cfg;
>  	struct device_node *child;
> -	const char *name;
>  	int rval;
>  
>  	for_each_child_of_node(node, child) {
> @@ -523,12 +523,9 @@ static int as3645a_parse_node(struct as3645a *flash,
>  		return -ENODEV;
>  	}
>  
> -	rval = of_property_read_string(flash->flash_node, "label", &name);
> -	if (!rval)
> -		strlcpy(names->flash, name, sizeof(names->flash));
> -	else
> -		snprintf(names->flash, sizeof(names->flash),
> -			 "%s:flash", node->name);
> +	of_led_compose_name(node, flash->flash_node, "flash",
> +				sizeof("flash"),
> +				names->flash);
>  
>  	rval = of_property_read_u32(flash->flash_node, "flash-timeout-us",
>  				    &cfg->flash_timeout_us);
> @@ -567,12 +564,9 @@ static int as3645a_parse_node(struct as3645a *flash,
>  		goto out_err;
>  	}
>  
> -	rval = of_property_read_string(flash->indicator_node, "label", &name);
> -	if (!rval)
> -		strlcpy(names->indicator, name, sizeof(names->indicator));
> -	else
> -		snprintf(names->indicator, sizeof(names->indicator),
> -			 "%s:indicator", node->name);
> +	of_led_compose_name(node, flash->indicator_node, "indicator",
> +				sizeof("indicator"),
> +				names->indicator);
>  
>  	rval = of_property_read_u32(flash->indicator_node, "led-max-microamp",
>  				    &cfg->indicator_max_ua);
> 


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

Re: [PATCH v6 1/6] leds: Add new API to derive a LED name

[Jacek Anaszewski]

Dan,

On 12/01/2017 05:56 PM, Dan Murphy wrote:
> Create an API that is called to derive the
> LED name from either the DT label in the child
> node or if that does not exist from the parent
> node name and an alternate label that is passed in.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v6 - New patch to add the api to generate a LED label
> 
>  drivers/leds/led-class.c | 34 ++++++++++++++++++++++++++++++++++
>  include/linux/leds.h     |  6 ++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index b0e2d55acbd6..d3e035488737 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -17,6 +17,7 @@
>  #include <linux/leds.h>
>  #include <linux/list.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/timer.h>
> @@ -243,6 +244,39 @@ static int led_classdev_next_name(const char *init_name, char *name,
>  	return i;
>  }
>  
> +/**
> + * of_led_compose_name - derive the LED name based on DT or alternate name.
> + *
> + * @parent: parent of LED device
> + * @child: child node of LED device
> + * @alt_name: an alternate name if the label node is not found
> + * @len: length of the alt_name
> + * @led_name: derived name from either DT label or alt_name
> + */
> +void of_led_compose_name(struct device_node *parent,
> +                     struct device_node *child,
> +		     const char *alt_name,
> +		     size_t len,
> +		     char *led_name)
> +{
> +	int ret;
> +	int length;
> +	const char *name;
> +
> +	if (len == 0 || alt_name == NULL)
> +		return;
> +
> +	ret = of_property_read_string(child, "label", &name);
> +	if (!ret) {
> +		strlcpy(led_name, name, sizeof(led_name));
> +	} else {
> +		length = len + strlen(parent->name) + 1;
> +		snprintf(led_name, len, "%s:%s", parent->name, alt_name);
> +	}


It looks different from what I meant, i.e. that devicename
segment shouldn't be present in the label, but derived
from the parent DT node name.

This is however to be decided whether it wouldn't be better to leave
the decision to the driver on how to obtain devicename  - from parent
DT node or from the driver hardcoded string. Some LED class drivers
prefer the latter way, so if their parent node name differs a bit from
the string they currently use for devicename, then the resulting
LED class device name will change after switching to using this
new API. In order to avoid that we would have to modify related
DT node names in *.dts files, which would make unnecessary noise.

Since it may take a while to agree on the final semantics
of this new API, especially after my recent patch [0], I propose
to put below piece of code directly in the driver:


ret = of_property_read_string(child_node, "label", &name);
if (!ret)
    snprintf(led->label, sizeof(led->label), "%s:%s",
			np->name, name)
else
    snprintf(led->label, sizeof(led->label),
             "%s::%s", np->name, name)

Please note that "::" means leaving colour section blank,
because we don't know the LED colour if label DT property was not
provided.

> +}
> +EXPORT_SYMBOL_GPL(of_led_compose_name);
> +
>  /**
>   * of_led_classdev_register - register a new object of led_classdev class.
>   *
> diff --git a/include/linux/leds.h b/include/linux/leds.h
> index 5579c64c8fd6..9e18dbe196e2 100644
> --- a/include/linux/leds.h
> +++ b/include/linux/leds.h
> @@ -123,6 +123,12 @@ struct led_classdev {
>  	struct mutex		led_access;
>  };
>  
> +extern void of_led_compose_name(struct device_node *parent,
> +                     struct device_node *child,
> +		     const char *alt_name,
> +		     size_t len,
> +		     char *led_name);
> +
>  extern int of_led_classdev_register(struct device *parent,
>  				    struct device_node *np,
>  				    struct led_classdev *led_cdev);
> 

[0] https://patchwork.kernel.org/patch/10089047

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v6 1/6] leds: Add new API to derive a LED name

[Dan Murphy]

Jacek

On 12/03/2017 07:27 AM, Jacek Anaszewski wrote:
> Dan,
> 
> On 12/01/2017 05:56 PM, Dan Murphy wrote:
>> Create an API that is called to derive the
>> LED name from either the DT label in the child
>> node or if that does not exist from the parent
>> node name and an alternate label that is passed in.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v6 - New patch to add the api to generate a LED label
>>
>>  drivers/leds/led-class.c | 34 ++++++++++++++++++++++++++++++++++
>>  include/linux/leds.h     |  6 ++++++
>>  2 files changed, 40 insertions(+)
>>
>> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
>> index b0e2d55acbd6..d3e035488737 100644
>> --- a/drivers/leds/led-class.c
>> +++ b/drivers/leds/led-class.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/leds.h>
>>  #include <linux/list.h>
>>  #include <linux/module.h>
>> +#include <linux/of.h>
>>  #include <linux/slab.h>
>>  #include <linux/spinlock.h>
>>  #include <linux/timer.h>
>> @@ -243,6 +244,39 @@ static int led_classdev_next_name(const char *init_name, char *name,
>>  	return i;
>>  }
>>  
>> +/**
>> + * of_led_compose_name - derive the LED name based on DT or alternate name.
>> + *
>> + * @parent: parent of LED device
>> + * @child: child node of LED device
>> + * @alt_name: an alternate name if the label node is not found
>> + * @len: length of the alt_name
>> + * @led_name: derived name from either DT label or alt_name
>> + */
>> +void of_led_compose_name(struct device_node *parent,
>> +                     struct device_node *child,
>> +		     const char *alt_name,
>> +		     size_t len,
>> +		     char *led_name)
>> +{
>> +	int ret;
>> +	int length;
>> +	const char *name;
>> +
>> +	if (len == 0 || alt_name == NULL)
>> +		return;
>> +
>> +	ret = of_property_read_string(child, "label", &name);
>> +	if (!ret) {
>> +		strlcpy(led_name, name, sizeof(led_name));
>> +	} else {
>> +		length = len + strlen(parent->name) + 1;
>> +		snprintf(led_name, len, "%s:%s", parent->name, alt_name);
>> +	}
> 
> 
> It looks different from what I meant, i.e. that devicename
> segment shouldn't be present in the label, but derived
> from the parent DT node name.
> 
> This is however to be decided whether it wouldn't be better to leave
> the decision to the driver on how to obtain devicename  - from parent
> DT node or from the driver hardcoded string. Some LED class drivers
> prefer the latter way, so if their parent node name differs a bit from
> the string they currently use for devicename, then the resulting
> LED class device name will change after switching to using this
> new API. In order to avoid that we would have to modify related
> DT node names in *.dts files, which would make unnecessary noise.
> 
> Since it may take a while to agree on the final semantics
> of this new API, especially after my recent patch [0], I propose
> to put below piece of code directly in the driver:
> 
> 
> ret = of_property_read_string(child_node, "label", &name);
> if (!ret)
>     snprintf(led->label, sizeof(led->label), "%s:%s",
> 			np->name, name)
> else
>     snprintf(led->label, sizeof(led->label),
>              "%s::%s", np->name, name)
> 
> Please note that "::" means leaving colour section blank,
> because we don't know the LED colour if label DT property was not
> provided.
> 

Just for clarification.

You want me to drop the LED class changes and go back to putting the
LED label generation back in the driver as the code you have above.

And separate out the LP8860 changes.

Dan

<snip>
-- 
------------------
Dan Murphy