linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
@ 2018-09-06 13:50 Dan Murphy
  2018-09-06 13:50 ` [PATCH v6 2/2] leds: lm3697: Introduce the " Dan Murphy
  2018-09-06 21:16 ` [PATCH v6 1/2] dt-bindings: leds: Add bindings for " Pavel Machek
  0 siblings, 2 replies; 15+ messages in thread
From: Dan Murphy @ 2018-09-06 13:50 UTC (permalink / raw)
  To: robh+dt, jacek.anaszewski, pavel
  Cc: devicetree, linux-kernel, linux-leds, Dan Murphy

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

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

v6 - Fix minor issues - https://lore.kernel.org/patchwork/patch/975387/

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

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

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v6 2/2] leds: lm3697: Introduce the lm3697 driver
  2018-09-06 13:50 [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver Dan Murphy
@ 2018-09-06 13:50 ` Dan Murphy
  2018-09-06 21:16 ` [PATCH v6 1/2] dt-bindings: leds: Add bindings for " Pavel Machek
  1 sibling, 0 replies; 15+ messages in thread
From: Dan Murphy @ 2018-09-06 13:50 UTC (permalink / raw)
  To: robh+dt, jacek.anaszewski, pavel
  Cc: devicetree, linux-kernel, linux-leds, 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>
---

v6 - Fix nitpicks - https://lore.kernel.org/patchwork/patch/975388/

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


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

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


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-06 13:50 [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver Dan Murphy
  2018-09-06 13:50 ` [PATCH v6 2/2] leds: lm3697: Introduce the " Dan Murphy
@ 2018-09-06 21:16 ` Pavel Machek
  2018-09-07 13:20   ` Dan Murphy
  1 sibling, 1 reply; 15+ messages in thread
From: Pavel Machek @ 2018-09-06 21:16 UTC (permalink / raw)
  To: Dan Murphy
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds

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

Hi!

> Add the device tree bindings for the lm3697
> LED driver for backlighting and display.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v6 - Fix minor issues - https://lore.kernel.org/patchwork/patch/975387/
> 
> v5 - Fix the comment for the example - https://lore.kernel.org/patchwork/patch/975060/
> v4 - Removed HVLED definition in favor of HVLED place definition - https://lore.kernel.org/patchwork/patch/974812/
> v3 - Updated subject with prefered title - https://lore.kernel.org/patchwork/patch/972337/
> v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/
> 
>  .../devicetree/bindings/leds/leds-lm3697.txt  | 86 +++++++++++++++++++
>  1 file changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt
> 
> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
> new file mode 100644
> index 000000000000..3256dec21075
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
> @@ -0,0 +1,86 @@
> +* Texas Instruments - LM3697 Highly Efficient White LED Driver
> +
> +The LM3697 11-bit LED driver provides high-
> +performance backlight dimming for 1, 2, or 3 series
> +LED strings while delivering up to 90% efficiency.

Hmm. We already have second set of bindings for 3697:

./devicetree/bindings/mfd/ti-lmu.txt

(Sorry for not noticing that earlier). Advantage is that those have
had discussion with device tree people and have acks:

What to do there?

								Pavel
commit 287cce719d85311f61d1b6b7f7b0d93f7907cd46
Author: Milo Kim <milo.kim@ti.com>
Date:   Tue Feb 28 15:45:14 2017 +0900

    dt-bindings: mfd: Add TI LMU device binding information

    This patch describes overall binding for TI LMU MFD devices.

    Signed-off-by: Milo Kim <milo.kim@ti.com>
        Acked-by: Rob Herring <robh+dt@kernel.org>
	    Acked-by: Tony Lindgren <tony@atomide.com>
	        Signed-off-by: Lee Jones <lee.jones@linaro.org>
		

commit d774c7e447ac911e73a1b3c775e6d89f0422218c
Author: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
Date:   Mon Aug 27 09:15:08 2018 -0700

    dt-bindings: mfd: ti-lmu: update for backlight
    
    Update binding to integrate the backlight feature directly into
    the main node, as suggested by Rob Herring.
    
    Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
index c885cf8..b3433e9 100644
--- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
+++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
@@ -28,10 +28,9 @@ Required properties:
 
 Optional property:
   - enable-gpios: A GPIO specifier for hardware enable pin.
-
-Required node:
-  - backlight: All LMU devices have backlight child nodes.
-               For the properties, please refer to [1].
+  - pwm-names: Should be either "lmu-backlight" or unset
+  - pwm: This should be a PWM specifier following ../pwm/pwm.txt and must
+         only be specified, if the backlight should be used in PWM mode.
 
 Optional nodes:
   - fault-monitor: Hardware fault monitoring driver for LM3633 and LM3697.
@@ -42,8 +41,31 @@ Optional nodes:
   - leds: LED properties for LM3633. Please refer to [2].
   - regulators: Regulator properties for LM3631 and LM3632.
                 Please refer to [3].
+  - bank0, bank1, bank2: This contains the backlight configuration
+                         for each backlight control bank.
+
+Required properties in the bank subnodes:
+  - label: A string describing the backlight. Should contain "keyboard"
+           for a keyboard backlight and "lcd" for LCD panel backlights.
+  - ti,led-sources: A list of channels, that should be driven. Each channel
+                    should only be driven by one bank.
+
+Optional properties in the bank subnodes:
+  - default-brightness-level: Backlight initial brightness value.
+                              Type is <u32>. It is set as soon as backlight
+			      device is created.
+			      0 ~ 2047 = LM3631, LM3632, LM3633, LM3695 and
+					 LM3697
+			      0 ~ 255  = LM3532
+  - ti,ramp-up-ms, ti,ramp-down-ms: Light dimming effect properties.
+				    Type is <u32>. Unit is millisecond.
+					0 ~ 65 ms    = LM3532
+					0 ~ 4000 ms  = LM3631
+					0 ~ 16000 ms = LM3633 and LM3697
+  - pwm-period: PWM period. Only valid in PWM brightness mode.
+                Type is <u32>. If this property is missing, then control
+		mode is set to I2C by default.
 
-[1] ../leds/backlight/ti-lmu-backlight.txt
 [2] ../leds/leds-lm3633.txt
 [3] ../regulator/lm363x-regulator.txt
 
@@ -53,14 +75,11 @@ lm3532@38 {
 
 	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
 
-	backlight {
-		compatible = "ti,lm3532-backlight";
-
-		lcd {
-			led-sources = <0 1 2>;
-			ramp-up-msec = <30>;
-			ramp-down-msec = <0>;
-		};
+	bank0 {
+		label = "lcd";
+		led-sources = <0 1 2>;
+		ramp-up-msec = <30>;
+		ramp-down-msec = <0>;
 	};
 };
 
@@ -105,13 +124,10 @@ lm3631@29 {
 		};
 	};
 
-	backlight {
-		compatible = "ti,lm3631-backlight";
-
-		lcd_bl {
-			led-sources = <0 1>;
-			ramp-up-msec = <300>;
-		};
+	bank0 {
+		label = "lcd_bl";
+		led-sources = <0 1>;
+		ramp-up-msec = <300>;
 	};
 };
 
@@ -147,16 +163,13 @@ lm3632@11 {
 		};
 	};
 
-	backlight {
-		compatible = "ti,lm3632-backlight";
-
-		pwms = <&pwm0 0 10000 0>; /* pwm number, period, polarity */
-		pwm-names = "lmu-backlight";
+	pwms = <&pwm0 0 10000 0>; /* pwm number, period, polarity */
+	pwm-names = "lmu-backlight";
 
-		lcd {
-			led-sources = <0 1>;
-			pwm-period = <10000>;
-		};
+	bank0 {
+		label = "lcd";
+		led-sources = <0 1>;
+		pwm-period = <10000>;
 	};
 };
 
@@ -166,22 +179,18 @@ lm3633@36 {
 
 	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
 
-	backlight {
-		compatible = "ti,lm3633-backlight";
-
-		main {
-			label = "main_lcd";
-			led-sources = <1 2>;
-			ramp-up-msec = <500>;
-			ramp-down-msec = <500>;
-		};
+	bank0 {
+		label = "main_lcd";
+		led-sources = <1 2>;
+		ramp-up-msec = <500>;
+		ramp-down-msec = <500>;
+	};
 
-		front {
-			label = "front_lcd";
-			led-sources = <0>;
-			ramp-up-msec = <1000>;
-			ramp-down-msec = <0>;
-		};
+	bank1 {
+		label = "front_lcd";
+		led-sources = <0>;
+		ramp-up-msec = <1000>;
+		ramp-down-msec = <0>;
 	};
 
 	leds {
@@ -211,13 +220,9 @@ lm3695@63 {
 
 	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
 
-	backlight {
-		compatible = "ti,lm3695-backlight";
-
-		lcd {
-			label = "bl";
-			led-sources = <0 1>;
-		};
+	bank0 {
+		label = "lcd_bl";
+		led-sources = <0 1>;
 	};
 };
 
@@ -227,14 +232,10 @@ lm3697@36 {
 
 	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
 
-	backlight {
-		compatible = "ti,lm3697-backlight";
-
-		lcd {
-			led-sources = <0 1 2>;
-			ramp-up-msec = <200>;
-			ramp-down-msec = <200>;
-		};
+	bank0 {
+		led-sources = <0 1 2>;
+		ramp-up-msec = <200>;
+		ramp-down-msec = <200>;
 	};
 
 	fault-monitor {

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

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-06 21:16 ` [PATCH v6 1/2] dt-bindings: leds: Add bindings for " Pavel Machek
@ 2018-09-07 13:20   ` Dan Murphy
  2018-09-07 13:32     ` Pavel Machek
  0 siblings, 1 reply; 15+ messages in thread
From: Dan Murphy @ 2018-09-07 13:20 UTC (permalink / raw)
  To: Pavel Machek
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds

Pavel

On 09/06/2018 04:16 PM, Pavel Machek wrote:
> Hi!
> 
>> Add the device tree bindings for the lm3697
>> LED driver for backlighting and display.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v6 - Fix minor issues - https://lore.kernel.org/patchwork/patch/975387/
>>
>> v5 - Fix the comment for the example - https://lore.kernel.org/patchwork/patch/975060/
>> v4 - Removed HVLED definition in favor of HVLED place definition - https://lore.kernel.org/patchwork/patch/974812/
>> v3 - Updated subject with prefered title - https://lore.kernel.org/patchwork/patch/972337/
>> v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/
>>
>>  .../devicetree/bindings/leds/leds-lm3697.txt  | 86 +++++++++++++++++++
>>  1 file changed, 86 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt
>>
>> diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
>> new file mode 100644
>> index 000000000000..3256dec21075
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
>> @@ -0,0 +1,86 @@
>> +* Texas Instruments - LM3697 Highly Efficient White LED Driver
>> +
>> +The LM3697 11-bit LED driver provides high-
>> +performance backlight dimming for 1, 2, or 3 series
>> +LED strings while delivering up to 90% efficiency.
> 
> Hmm. We already have second set of bindings for 3697:
> 
> ./devicetree/bindings/mfd/ti-lmu.txt
> 
> (Sorry for not noticing that earlier). Advantage is that those have
> had discussion with device tree people and have acks:
> 
> What to do there?

UGH!  IMO this should have not been accepted without the support code.
I think this MFD driver is complete over kill to try to commonize features
into a MFD device.  The LM3631 and LM3632 seem to be the only true MFD devices
here since they provide regulator support.

The LM3697 fault monitoring is only for test purposes according to the data
sheet.  Not sure the customer can trust this or they should be warned at boot
that the Fault monitoring is for test purposes only.

And I think Jacek pointed out that the bindings references in this bindings
don't even exist.

I am thinking we need to deprecate this MFD driver and consolidate these drivers
in the LED directory as we indicated before.  I did not find any ti-lmu support
code.

ti-lmu common core code and then the LED children appending the feature differentiation.

Need some maintainer weigh in here.

Dan

> 
> 								Pavel
> commit 287cce719d85311f61d1b6b7f7b0d93f7907cd46
> Author: Milo Kim <milo.kim@ti.com>
> Date:   Tue Feb 28 15:45:14 2017 +0900
> 
>     dt-bindings: mfd: Add TI LMU device binding information
> 
>     This patch describes overall binding for TI LMU MFD devices.
> 
>     Signed-off-by: Milo Kim <milo.kim@ti.com>
>         Acked-by: Rob Herring <robh+dt@kernel.org>
> 	    Acked-by: Tony Lindgren <tony@atomide.com>
> 	        Signed-off-by: Lee Jones <lee.jones@linaro.org>
> 		
> 
> commit d774c7e447ac911e73a1b3c775e6d89f0422218c
> Author: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Date:   Mon Aug 27 09:15:08 2018 -0700
> 
>     dt-bindings: mfd: ti-lmu: update for backlight
>     
>     Update binding to integrate the backlight feature directly into
>     the main node, as suggested by Rob Herring.
>     
>     Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> 
> diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> index c885cf8..b3433e9 100644
> --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> @@ -28,10 +28,9 @@ Required properties:
>  
>  Optional property:
>    - enable-gpios: A GPIO specifier for hardware enable pin.
> -
> -Required node:
> -  - backlight: All LMU devices have backlight child nodes.
> -               For the properties, please refer to [1].
> +  - pwm-names: Should be either "lmu-backlight" or unset
> +  - pwm: This should be a PWM specifier following ../pwm/pwm.txt and must
> +         only be specified, if the backlight should be used in PWM mode.
>  
>  Optional nodes:
>    - fault-monitor: Hardware fault monitoring driver for LM3633 and LM3697.
> @@ -42,8 +41,31 @@ Optional nodes:
>    - leds: LED properties for LM3633. Please refer to [2].
>    - regulators: Regulator properties for LM3631 and LM3632.
>                  Please refer to [3].
> +  - bank0, bank1, bank2: This contains the backlight configuration
> +                         for each backlight control bank.
> +
> +Required properties in the bank subnodes:
> +  - label: A string describing the backlight. Should contain "keyboard"
> +           for a keyboard backlight and "lcd" for LCD panel backlights.
> +  - ti,led-sources: A list of channels, that should be driven. Each channel
> +                    should only be driven by one bank.
> +
> +Optional properties in the bank subnodes:
> +  - default-brightness-level: Backlight initial brightness value.
> +                              Type is <u32>. It is set as soon as backlight
> +			      device is created.
> +			      0 ~ 2047 = LM3631, LM3632, LM3633, LM3695 and
> +					 LM3697
> +			      0 ~ 255  = LM3532
> +  - ti,ramp-up-ms, ti,ramp-down-ms: Light dimming effect properties.
> +				    Type is <u32>. Unit is millisecond.
> +					0 ~ 65 ms    = LM3532
> +					0 ~ 4000 ms  = LM3631
> +					0 ~ 16000 ms = LM3633 and LM3697
> +  - pwm-period: PWM period. Only valid in PWM brightness mode.
> +                Type is <u32>. If this property is missing, then control
> +		mode is set to I2C by default.
>  
> -[1] ../leds/backlight/ti-lmu-backlight.txt
>  [2] ../leds/leds-lm3633.txt
>  [3] ../regulator/lm363x-regulator.txt
>  
> @@ -53,14 +75,11 @@ lm3532@38 {
>  
>  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
>  
> -	backlight {
> -		compatible = "ti,lm3532-backlight";
> -
> -		lcd {
> -			led-sources = <0 1 2>;
> -			ramp-up-msec = <30>;
> -			ramp-down-msec = <0>;
> -		};
> +	bank0 {
> +		label = "lcd";
> +		led-sources = <0 1 2>;
> +		ramp-up-msec = <30>;
> +		ramp-down-msec = <0>;
>  	};
>  };
>  
> @@ -105,13 +124,10 @@ lm3631@29 {
>  		};
>  	};
>  
> -	backlight {
> -		compatible = "ti,lm3631-backlight";
> -
> -		lcd_bl {
> -			led-sources = <0 1>;
> -			ramp-up-msec = <300>;
> -		};
> +	bank0 {
> +		label = "lcd_bl";
> +		led-sources = <0 1>;
> +		ramp-up-msec = <300>;
>  	};
>  };
>  
> @@ -147,16 +163,13 @@ lm3632@11 {
>  		};
>  	};
>  
> -	backlight {
> -		compatible = "ti,lm3632-backlight";
> -
> -		pwms = <&pwm0 0 10000 0>; /* pwm number, period, polarity */
> -		pwm-names = "lmu-backlight";
> +	pwms = <&pwm0 0 10000 0>; /* pwm number, period, polarity */
> +	pwm-names = "lmu-backlight";
>  
> -		lcd {
> -			led-sources = <0 1>;
> -			pwm-period = <10000>;
> -		};
> +	bank0 {
> +		label = "lcd";
> +		led-sources = <0 1>;
> +		pwm-period = <10000>;
>  	};
>  };
>  
> @@ -166,22 +179,18 @@ lm3633@36 {
>  
>  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
>  
> -	backlight {
> -		compatible = "ti,lm3633-backlight";
> -
> -		main {
> -			label = "main_lcd";
> -			led-sources = <1 2>;
> -			ramp-up-msec = <500>;
> -			ramp-down-msec = <500>;
> -		};
> +	bank0 {
> +		label = "main_lcd";
> +		led-sources = <1 2>;
> +		ramp-up-msec = <500>;
> +		ramp-down-msec = <500>;
> +	};
>  
> -		front {
> -			label = "front_lcd";
> -			led-sources = <0>;
> -			ramp-up-msec = <1000>;
> -			ramp-down-msec = <0>;
> -		};
> +	bank1 {
> +		label = "front_lcd";
> +		led-sources = <0>;
> +		ramp-up-msec = <1000>;
> +		ramp-down-msec = <0>;
>  	};
>  
>  	leds {
> @@ -211,13 +220,9 @@ lm3695@63 {
>  
>  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
>  
> -	backlight {
> -		compatible = "ti,lm3695-backlight";
> -
> -		lcd {
> -			label = "bl";
> -			led-sources = <0 1>;
> -		};
> +	bank0 {
> +		label = "lcd_bl";
> +		led-sources = <0 1>;
>  	};
>  };
>  
> @@ -227,14 +232,10 @@ lm3697@36 {
>  
>  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
>  
> -	backlight {
> -		compatible = "ti,lm3697-backlight";
> -
> -		lcd {
> -			led-sources = <0 1 2>;
> -			ramp-up-msec = <200>;
> -			ramp-down-msec = <200>;
> -		};
> +	bank0 {
> +		led-sources = <0 1 2>;
> +		ramp-up-msec = <200>;
> +		ramp-down-msec = <200>;
>  	};
>  
>  	fault-monitor {
> 


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-07 13:20   ` Dan Murphy
@ 2018-09-07 13:32     ` Pavel Machek
  2018-09-07 13:52       ` Dan Murphy
  0 siblings, 1 reply; 15+ messages in thread
From: Pavel Machek @ 2018-09-07 13:32 UTC (permalink / raw)
  To: Dan Murphy
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds

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

Hi!

> >> index 000000000000..3256dec21075
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
> >> @@ -0,0 +1,86 @@
> >> +* Texas Instruments - LM3697 Highly Efficient White LED Driver
> >> +
> >> +The LM3697 11-bit LED driver provides high-
> >> +performance backlight dimming for 1, 2, or 3 series
> >> +LED strings while delivering up to 90% efficiency.
> > 
> > Hmm. We already have second set of bindings for 3697:
> > 
> > ./devicetree/bindings/mfd/ti-lmu.txt
> > 
> > (Sorry for not noticing that earlier). Advantage is that those have
> > had discussion with device tree people and have acks:
> > 
> > What to do there?
> 
> UGH!  IMO this should have not been accepted without the support code.
> I think this MFD driver is complete over kill to try to commonize features
> into a MFD device.  The LM3631 and LM3632 seem to be the only true MFD devices
> here since they provide regulator support.

They are not yet in mainline; but they have Rob's acknowledges.

...but I see there are improvements possible.

> The LM3697 fault monitoring is only for test purposes according to the data
> sheet.  Not sure the customer can trust this or they should be warned at boot
> that the Fault monitoring is for test purposes only.

Ok, but that's topic for the driver, not for binding, right?

> And I think Jacek pointed out that the bindings references in this bindings
> don't even exist.
> 
> I am thinking we need to deprecate this MFD driver and consolidate these drivers
> in the LED directory as we indicated before.  I did not find any ti-lmu support
> code.
> 
> ti-lmu common core code and then the LED children appending the feature differentiation.

> Need some maintainer weigh in here.

Hehe. I'm maintnainer. Fun.

Is there something obviously wrong with
287cce719d85311f61d1b6b7f7b0d93f7907cd46 +
d774c7e447ac911e73a1b3c775e6d89f0422218c ?

If not, as it already had discussion/Acks so I'd prefer that one. We
may move it to LEDs directory if neccessary, or something like that...

Best regards,

								Pavel

> > commit 287cce719d85311f61d1b6b7f7b0d93f7907cd46
> > Author: Milo Kim <milo.kim@ti.com>
> > Date:   Tue Feb 28 15:45:14 2017 +0900
> > 
> >     dt-bindings: mfd: Add TI LMU device binding information
> > 
> >     This patch describes overall binding for TI LMU MFD devices.
> > 
> >     Signed-off-by: Milo Kim <milo.kim@ti.com>
> >         Acked-by: Rob Herring <robh+dt@kernel.org>
> > 	    Acked-by: Tony Lindgren <tony@atomide.com>
> > 	        Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > 		
> > 
> > commit d774c7e447ac911e73a1b3c775e6d89f0422218c
> > Author: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > Date:   Mon Aug 27 09:15:08 2018 -0700
> > 
> >     dt-bindings: mfd: ti-lmu: update for backlight
> >     
> >     Update binding to integrate the backlight feature directly into
> >     the main node, as suggested by Rob Herring.
> >     
> >     Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > index c885cf8..b3433e9 100644
> > --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > @@ -28,10 +28,9 @@ Required properties:
> >  
> >  Optional property:
> >    - enable-gpios: A GPIO specifier for hardware enable pin.
> > -
> > -Required node:
> > -  - backlight: All LMU devices have backlight child nodes.
> > -               For the properties, please refer to [1].
> > +  - pwm-names: Should be either "lmu-backlight" or unset
> > +  - pwm: This should be a PWM specifier following ../pwm/pwm.txt and must
> > +         only be specified, if the backlight should be used in PWM mode.
> >  
> >  Optional nodes:
> >    - fault-monitor: Hardware fault monitoring driver for LM3633 and LM3697.
> > @@ -42,8 +41,31 @@ Optional nodes:
> >    - leds: LED properties for LM3633. Please refer to [2].
> >    - regulators: Regulator properties for LM3631 and LM3632.
> >                  Please refer to [3].
> > +  - bank0, bank1, bank2: This contains the backlight configuration
> > +                         for each backlight control bank.
> > +
> > +Required properties in the bank subnodes:
> > +  - label: A string describing the backlight. Should contain "keyboard"
> > +           for a keyboard backlight and "lcd" for LCD panel backlights.
> > +  - ti,led-sources: A list of channels, that should be driven. Each channel
> > +                    should only be driven by one bank.
> > +
> > +Optional properties in the bank subnodes:
> > +  - default-brightness-level: Backlight initial brightness value.
> > +                              Type is <u32>. It is set as soon as backlight
> > +			      device is created.
> > +			      0 ~ 2047 = LM3631, LM3632, LM3633, LM3695 and
> > +					 LM3697
> > +			      0 ~ 255  = LM3532
> > +  - ti,ramp-up-ms, ti,ramp-down-ms: Light dimming effect properties.
> > +				    Type is <u32>. Unit is millisecond.
> > +					0 ~ 65 ms    = LM3532
> > +					0 ~ 4000 ms  = LM3631
> > +					0 ~ 16000 ms = LM3633 and LM3697
> > +  - pwm-period: PWM period. Only valid in PWM brightness mode.
> > +                Type is <u32>. If this property is missing, then control
> > +		mode is set to I2C by default.
> >  
> > -[1] ../leds/backlight/ti-lmu-backlight.txt
> >  [2] ../leds/leds-lm3633.txt
> >  [3] ../regulator/lm363x-regulator.txt
> >  
> > @@ -53,14 +75,11 @@ lm3532@38 {
> >  
> >  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
> >  
> > -	backlight {
> > -		compatible = "ti,lm3532-backlight";
> > -
> > -		lcd {
> > -			led-sources = <0 1 2>;
> > -			ramp-up-msec = <30>;
> > -			ramp-down-msec = <0>;
> > -		};
> > +	bank0 {
> > +		label = "lcd";
> > +		led-sources = <0 1 2>;
> > +		ramp-up-msec = <30>;
> > +		ramp-down-msec = <0>;
> >  	};
> >  };
> >  
> > @@ -105,13 +124,10 @@ lm3631@29 {
> >  		};
> >  	};
> >  
> > -	backlight {
> > -		compatible = "ti,lm3631-backlight";
> > -
> > -		lcd_bl {
> > -			led-sources = <0 1>;
> > -			ramp-up-msec = <300>;
> > -		};
> > +	bank0 {
> > +		label = "lcd_bl";
> > +		led-sources = <0 1>;
> > +		ramp-up-msec = <300>;
> >  	};
> >  };
> >  
> > @@ -147,16 +163,13 @@ lm3632@11 {
> >  		};
> >  	};
> >  
> > -	backlight {
> > -		compatible = "ti,lm3632-backlight";
> > -
> > -		pwms = <&pwm0 0 10000 0>; /* pwm number, period, polarity */
> > -		pwm-names = "lmu-backlight";
> > +	pwms = <&pwm0 0 10000 0>; /* pwm number, period, polarity */
> > +	pwm-names = "lmu-backlight";
> >  
> > -		lcd {
> > -			led-sources = <0 1>;
> > -			pwm-period = <10000>;
> > -		};
> > +	bank0 {
> > +		label = "lcd";
> > +		led-sources = <0 1>;
> > +		pwm-period = <10000>;
> >  	};
> >  };
> >  
> > @@ -166,22 +179,18 @@ lm3633@36 {
> >  
> >  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
> >  
> > -	backlight {
> > -		compatible = "ti,lm3633-backlight";
> > -
> > -		main {
> > -			label = "main_lcd";
> > -			led-sources = <1 2>;
> > -			ramp-up-msec = <500>;
> > -			ramp-down-msec = <500>;
> > -		};
> > +	bank0 {
> > +		label = "main_lcd";
> > +		led-sources = <1 2>;
> > +		ramp-up-msec = <500>;
> > +		ramp-down-msec = <500>;
> > +	};
> >  
> > -		front {
> > -			label = "front_lcd";
> > -			led-sources = <0>;
> > -			ramp-up-msec = <1000>;
> > -			ramp-down-msec = <0>;
> > -		};
> > +	bank1 {
> > +		label = "front_lcd";
> > +		led-sources = <0>;
> > +		ramp-up-msec = <1000>;
> > +		ramp-down-msec = <0>;
> >  	};
> >  
> >  	leds {
> > @@ -211,13 +220,9 @@ lm3695@63 {
> >  
> >  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
> >  
> > -	backlight {
> > -		compatible = "ti,lm3695-backlight";
> > -
> > -		lcd {
> > -			label = "bl";
> > -			led-sources = <0 1>;
> > -		};
> > +	bank0 {
> > +		label = "lcd_bl";
> > +		led-sources = <0 1>;
> >  	};
> >  };
> >  
> > @@ -227,14 +232,10 @@ lm3697@36 {
> >  
> >  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
> >  
> > -	backlight {
> > -		compatible = "ti,lm3697-backlight";
> > -
> > -		lcd {
> > -			led-sources = <0 1 2>;
> > -			ramp-up-msec = <200>;
> > -			ramp-down-msec = <200>;
> > -		};
> > +	bank0 {
> > +		led-sources = <0 1 2>;
> > +		ramp-up-msec = <200>;
> > +		ramp-down-msec = <200>;
> >  	};
> >  
> >  	fault-monitor {
> > 
> 
> 

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-07 13:32     ` Pavel Machek
@ 2018-09-07 13:52       ` Dan Murphy
  2018-09-08 19:53         ` Jacek Anaszewski
  0 siblings, 1 reply; 15+ messages in thread
From: Dan Murphy @ 2018-09-07 13:52 UTC (permalink / raw)
  To: Pavel Machek
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds

Pavel

On 09/07/2018 08:32 AM, Pavel Machek wrote:
> Hi!
> 
>>>> index 000000000000..3256dec21075
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
>>>> @@ -0,0 +1,86 @@
>>>> +* Texas Instruments - LM3697 Highly Efficient White LED Driver
>>>> +
>>>> +The LM3697 11-bit LED driver provides high-
>>>> +performance backlight dimming for 1, 2, or 3 series
>>>> +LED strings while delivering up to 90% efficiency.
>>>
>>> Hmm. We already have second set of bindings for 3697:
>>>
>>> ./devicetree/bindings/mfd/ti-lmu.txt
>>>
>>> (Sorry for not noticing that earlier). Advantage is that those have
>>> had discussion with device tree people and have acks:
>>>
>>> What to do there?
>>
>> UGH!  IMO this should have not been accepted without the support code.
>> I think this MFD driver is complete over kill to try to commonize features
>> into a MFD device.  The LM3631 and LM3632 seem to be the only true MFD devices
>> here since they provide regulator support.
> 
> They are not yet in mainline; but they have Rob's acknowledges.
> 
> ...but I see there are improvements possible.
> 
>> The LM3697 fault monitoring is only for test purposes according to the data
>> sheet.  Not sure the customer can trust this or they should be warned at boot
>> that the Fault monitoring is for test purposes only.
> 
> Ok, but that's topic for the driver, not for binding, right?

Well the binding indicates that there is fault monitoring for this device.
If fault monitoring is for test only then it probably should not be
exposed in the binding

	fault-monitor {
		compatible = "ti,lm3697-fault-monitor";
	};


> 
>> And I think Jacek pointed out that the bindings references in this bindings
>> don't even exist.
>>
>> I am thinking we need to deprecate this MFD driver and consolidate these drivers
>> in the LED directory as we indicated before.  I did not find any ti-lmu support
>> code.
>>
>> ti-lmu common core code and then the LED children appending the feature differentiation.
> 
>> Need some maintainer weigh in here.
> 
> Hehe. I'm maintnainer. Fun.

I know.  I want to see if there was any other opinion.  Especially for the LED driver.

> 
> Is there something obviously wrong with
> 287cce719d85311f61d1b6b7f7b0d93f7907cd46 +
> d774c7e447ac911e73a1b3c775e6d89f0422218c ?
> 
> If not, as it already had discussion/Acks so I'd prefer that one. We
> may move it to LEDs directory if neccessary, or something like that...

Not finding d774c7e447ac911e73a1b3c775e6d89f0422218c in the tree.
I am still not in favor of this ti-lmu driver in the MFD directory.

It really does not do much.  It seems like a waste.

I will work on a new architecture and try to RFC next week.

Dan


> 
> Best regards,
> 
> 								Pavel
> 
>>> commit 287cce719d85311f61d1b6b7f7b0d93f7907cd46
>>> Author: Milo Kim <milo.kim@ti.com>
>>> Date:   Tue Feb 28 15:45:14 2017 +0900
>>>
>>>     dt-bindings: mfd: Add TI LMU device binding information
>>>
>>>     This patch describes overall binding for TI LMU MFD devices.
>>>
>>>     Signed-off-by: Milo Kim <milo.kim@ti.com>
>>>         Acked-by: Rob Herring <robh+dt@kernel.org>
>>> 	    Acked-by: Tony Lindgren <tony@atomide.com>
>>> 	        Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>> 		
>>>
>>> commit d774c7e447ac911e73a1b3c775e6d89f0422218c
>>> Author: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
>>> Date:   Mon Aug 27 09:15:08 2018 -0700
>>>
>>>     dt-bindings: mfd: ti-lmu: update for backlight
>>>     
>>>     Update binding to integrate the backlight feature directly into
>>>     the main node, as suggested by Rob Herring.
>>>     
>>>     Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
>>> index c885cf8..b3433e9 100644
>>> --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
>>> +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
>>> @@ -28,10 +28,9 @@ Required properties:
>>>  
>>>  Optional property:
>>>    - enable-gpios: A GPIO specifier for hardware enable pin.
>>> -
>>> -Required node:
>>> -  - backlight: All LMU devices have backlight child nodes.
>>> -               For the properties, please refer to [1].
>>> +  - pwm-names: Should be either "lmu-backlight" or unset
>>> +  - pwm: This should be a PWM specifier following ../pwm/pwm.txt and must
>>> +         only be specified, if the backlight should be used in PWM mode.
>>>  
>>>  Optional nodes:
>>>    - fault-monitor: Hardware fault monitoring driver for LM3633 and LM3697.
>>> @@ -42,8 +41,31 @@ Optional nodes:
>>>    - leds: LED properties for LM3633. Please refer to [2].
>>>    - regulators: Regulator properties for LM3631 and LM3632.
>>>                  Please refer to [3].
>>> +  - bank0, bank1, bank2: This contains the backlight configuration
>>> +                         for each backlight control bank.
>>> +
>>> +Required properties in the bank subnodes:
>>> +  - label: A string describing the backlight. Should contain "keyboard"
>>> +           for a keyboard backlight and "lcd" for LCD panel backlights.
>>> +  - ti,led-sources: A list of channels, that should be driven. Each channel
>>> +                    should only be driven by one bank.
>>> +
>>> +Optional properties in the bank subnodes:
>>> +  - default-brightness-level: Backlight initial brightness value.
>>> +                              Type is <u32>. It is set as soon as backlight
>>> +			      device is created.
>>> +			      0 ~ 2047 = LM3631, LM3632, LM3633, LM3695 and
>>> +					 LM3697
>>> +			      0 ~ 255  = LM3532
>>> +  - ti,ramp-up-ms, ti,ramp-down-ms: Light dimming effect properties.
>>> +				    Type is <u32>. Unit is millisecond.
>>> +					0 ~ 65 ms    = LM3532
>>> +					0 ~ 4000 ms  = LM3631
>>> +					0 ~ 16000 ms = LM3633 and LM3697
>>> +  - pwm-period: PWM period. Only valid in PWM brightness mode.
>>> +                Type is <u32>. If this property is missing, then control
>>> +		mode is set to I2C by default.
>>>  
>>> -[1] ../leds/backlight/ti-lmu-backlight.txt
>>>  [2] ../leds/leds-lm3633.txt
>>>  [3] ../regulator/lm363x-regulator.txt
>>>  
>>> @@ -53,14 +75,11 @@ lm3532@38 {
>>>  
>>>  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
>>>  
>>> -	backlight {
>>> -		compatible = "ti,lm3532-backlight";
>>> -
>>> -		lcd {
>>> -			led-sources = <0 1 2>;
>>> -			ramp-up-msec = <30>;
>>> -			ramp-down-msec = <0>;
>>> -		};
>>> +	bank0 {
>>> +		label = "lcd";
>>> +		led-sources = <0 1 2>;
>>> +		ramp-up-msec = <30>;
>>> +		ramp-down-msec = <0>;
>>>  	};
>>>  };
>>>  
>>> @@ -105,13 +124,10 @@ lm3631@29 {
>>>  		};
>>>  	};
>>>  
>>> -	backlight {
>>> -		compatible = "ti,lm3631-backlight";
>>> -
>>> -		lcd_bl {
>>> -			led-sources = <0 1>;
>>> -			ramp-up-msec = <300>;
>>> -		};
>>> +	bank0 {
>>> +		label = "lcd_bl";
>>> +		led-sources = <0 1>;
>>> +		ramp-up-msec = <300>;
>>>  	};
>>>  };
>>>  
>>> @@ -147,16 +163,13 @@ lm3632@11 {
>>>  		};
>>>  	};
>>>  
>>> -	backlight {
>>> -		compatible = "ti,lm3632-backlight";
>>> -
>>> -		pwms = <&pwm0 0 10000 0>; /* pwm number, period, polarity */
>>> -		pwm-names = "lmu-backlight";
>>> +	pwms = <&pwm0 0 10000 0>; /* pwm number, period, polarity */
>>> +	pwm-names = "lmu-backlight";
>>>  
>>> -		lcd {
>>> -			led-sources = <0 1>;
>>> -			pwm-period = <10000>;
>>> -		};
>>> +	bank0 {
>>> +		label = "lcd";
>>> +		led-sources = <0 1>;
>>> +		pwm-period = <10000>;
>>>  	};
>>>  };
>>>  
>>> @@ -166,22 +179,18 @@ lm3633@36 {
>>>  
>>>  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
>>>  
>>> -	backlight {
>>> -		compatible = "ti,lm3633-backlight";
>>> -
>>> -		main {
>>> -			label = "main_lcd";
>>> -			led-sources = <1 2>;
>>> -			ramp-up-msec = <500>;
>>> -			ramp-down-msec = <500>;
>>> -		};
>>> +	bank0 {
>>> +		label = "main_lcd";
>>> +		led-sources = <1 2>;
>>> +		ramp-up-msec = <500>;
>>> +		ramp-down-msec = <500>;
>>> +	};
>>>  
>>> -		front {
>>> -			label = "front_lcd";
>>> -			led-sources = <0>;
>>> -			ramp-up-msec = <1000>;
>>> -			ramp-down-msec = <0>;
>>> -		};
>>> +	bank1 {
>>> +		label = "front_lcd";
>>> +		led-sources = <0>;
>>> +		ramp-up-msec = <1000>;
>>> +		ramp-down-msec = <0>;
>>>  	};
>>>  
>>>  	leds {
>>> @@ -211,13 +220,9 @@ lm3695@63 {
>>>  
>>>  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
>>>  
>>> -	backlight {
>>> -		compatible = "ti,lm3695-backlight";
>>> -
>>> -		lcd {
>>> -			label = "bl";
>>> -			led-sources = <0 1>;
>>> -		};
>>> +	bank0 {
>>> +		label = "lcd_bl";
>>> +		led-sources = <0 1>;
>>>  	};
>>>  };
>>>  
>>> @@ -227,14 +232,10 @@ lm3697@36 {
>>>  
>>>  	enable-gpios = <&pioC 2 GPIO_ACTIVE_HIGH>;
>>>  
>>> -	backlight {
>>> -		compatible = "ti,lm3697-backlight";
>>> -
>>> -		lcd {
>>> -			led-sources = <0 1 2>;
>>> -			ramp-up-msec = <200>;
>>> -			ramp-down-msec = <200>;
>>> -		};
>>> +	bank0 {
>>> +		led-sources = <0 1 2>;
>>> +		ramp-up-msec = <200>;
>>> +		ramp-down-msec = <200>;
>>>  	};
>>>  
>>>  	fault-monitor {
>>>
>>
>>
> 


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-07 13:52       ` Dan Murphy
@ 2018-09-08 19:53         ` Jacek Anaszewski
  2018-09-10 14:37           ` Dan Murphy
  2018-09-10 15:41           ` Pavel Machek
  0 siblings, 2 replies; 15+ messages in thread
From: Jacek Anaszewski @ 2018-09-08 19:53 UTC (permalink / raw)
  To: Dan Murphy, Pavel Machek; +Cc: robh+dt, devicetree, linux-kernel, linux-leds

Dan,

On 09/07/2018 03:52 PM, Dan Murphy wrote:
[...]
>>
>>> And I think Jacek pointed out that the bindings references in this bindings
>>> don't even exist.
>>>
>>> I am thinking we need to deprecate this MFD driver and consolidate these drivers
>>> in the LED directory as we indicated before.  I did not find any ti-lmu support
>>> code.
>>>
>>> ti-lmu common core code and then the LED children appending the feature differentiation.
>>
>>> Need some maintainer weigh in here.
>>
>> Hehe. I'm maintnainer. Fun.
> 
> I know.  I want to see if there was any other opinion.  Especially for the LED driver.
> 
[...]

I have a question - is this lm3697 LED controller a cell of some MFD
device? Or is it a self-contained chip?

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-08 19:53         ` Jacek Anaszewski
@ 2018-09-10 14:37           ` Dan Murphy
  2018-09-10 19:07             ` Jacek Anaszewski
  2018-09-10 15:41           ` Pavel Machek
  1 sibling, 1 reply; 15+ messages in thread
From: Dan Murphy @ 2018-09-10 14:37 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: robh+dt, devicetree, linux-kernel, linux-leds

Jacek

On 09/08/2018 02:53 PM, Jacek Anaszewski wrote:
> Dan,
> 
> On 09/07/2018 03:52 PM, Dan Murphy wrote:
> [...]
>>>
>>>> And I think Jacek pointed out that the bindings references in this bindings
>>>> don't even exist.
>>>>
>>>> I am thinking we need to deprecate this MFD driver and consolidate these drivers
>>>> in the LED directory as we indicated before.  I did not find any ti-lmu support
>>>> code.
>>>>
>>>> ti-lmu common core code and then the LED children appending the feature differentiation.
>>>
>>>> Need some maintainer weigh in here.
>>>
>>> Hehe. I'm maintnainer. Fun.
>>
>> I know.  I want to see if there was any other opinion.  Especially for the LED driver.
>>
> [...]
> 
> I have a question - is this lm3697 LED controller a cell of some MFD
> device? Or is it a self-contained chip?
> 

This is a self contained chip.  And the LM3697 only function is a LED driver.
It does not have any other special functions like the LM363X drivers for GPIO and Regulator support.


Dan

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-08 19:53         ` Jacek Anaszewski
  2018-09-10 14:37           ` Dan Murphy
@ 2018-09-10 15:41           ` Pavel Machek
  1 sibling, 0 replies; 15+ messages in thread
From: Pavel Machek @ 2018-09-10 15:41 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Dan Murphy, robh+dt, devicetree, linux-kernel, linux-leds

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

On Sat 2018-09-08 21:53:00, Jacek Anaszewski wrote:
> Dan,
> 
> On 09/07/2018 03:52 PM, Dan Murphy wrote:
> [...]
> >>
> >>> And I think Jacek pointed out that the bindings references in this bindings
> >>> don't even exist.
> >>>
> >>> I am thinking we need to deprecate this MFD driver and consolidate these drivers
> >>> in the LED directory as we indicated before.  I did not find any ti-lmu support
> >>> code.
> >>>
> >>> ti-lmu common core code and then the LED children appending the feature differentiation.
> >>
> >>> Need some maintainer weigh in here.
> >>
> >> Hehe. I'm maintnainer. Fun.
> > 
> > I know.  I want to see if there was any other opinion.  Especially for the LED driver.
> > 
> [...]
> 
> I have a question - is this lm3697 LED controller a cell of some MFD
> device? Or is it a self-contained chip?

lm3697 is number for stand-alone device, but same hardware seems to be
embedded into multi-functional chips, with other numbers in lm36xx
series.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-10 14:37           ` Dan Murphy
@ 2018-09-10 19:07             ` Jacek Anaszewski
  2018-09-10 19:51               ` Dan Murphy
  0 siblings, 1 reply; 15+ messages in thread
From: Jacek Anaszewski @ 2018-09-10 19:07 UTC (permalink / raw)
  To: Dan Murphy, Pavel Machek; +Cc: robh+dt, devicetree, linux-kernel, linux-leds

Dan, Pavel,

On 09/10/2018 04:37 PM, Dan Murphy wrote:
> Jacek
> 
> On 09/08/2018 02:53 PM, Jacek Anaszewski wrote:
>> Dan,
>>
>> On 09/07/2018 03:52 PM, Dan Murphy wrote:
>> [...]
>>>>
>>>>> And I think Jacek pointed out that the bindings references in this bindings
>>>>> don't even exist.
>>>>>
>>>>> I am thinking we need to deprecate this MFD driver and consolidate these drivers
>>>>> in the LED directory as we indicated before.  I did not find any ti-lmu support
>>>>> code.
>>>>>
>>>>> ti-lmu common core code and then the LED children appending the feature differentiation.
>>>>
>>>>> Need some maintainer weigh in here.
>>>>
>>>> Hehe. I'm maintnainer. Fun.
>>>
>>> I know.  I want to see if there was any other opinion.  Especially for the LED driver.
>>>
>> [...]
>>
>> I have a question - is this lm3697 LED controller a cell of some MFD
>> device? Or is it a self-contained chip?
>>
> 
> This is a self contained chip.  And the LM3697 only function is a LED driver.
> It does not have any other special functions like the LM363X drivers for GPIO and Regulator support.

This is an argument for merging it as a standalone LED class driver
then. It is even more justifiable, taking into account uncertainties
related to the proper way of adding the support for it to the existing
MFD driver, whereas the code reuse would be the only advantage of having
thus support in MFD subsystem.

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-10 19:07             ` Jacek Anaszewski
@ 2018-09-10 19:51               ` Dan Murphy
  2018-09-11 18:27                 ` Jacek Anaszewski
  2018-09-11 20:55                 ` Pavel Machek
  0 siblings, 2 replies; 15+ messages in thread
From: Dan Murphy @ 2018-09-10 19:51 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: robh+dt, devicetree, linux-kernel, linux-leds

Jacek

On 09/10/2018 02:07 PM, Jacek Anaszewski wrote:
> Dan, Pavel,
> 
> On 09/10/2018 04:37 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 09/08/2018 02:53 PM, Jacek Anaszewski wrote:
>>> Dan,
>>>
>>> On 09/07/2018 03:52 PM, Dan Murphy wrote:
>>> [...]
>>>>>
>>>>>> And I think Jacek pointed out that the bindings references in this bindings
>>>>>> don't even exist.
>>>>>>
>>>>>> I am thinking we need to deprecate this MFD driver and consolidate these drivers
>>>>>> in the LED directory as we indicated before.  I did not find any ti-lmu support
>>>>>> code.
>>>>>>
>>>>>> ti-lmu common core code and then the LED children appending the feature differentiation.
>>>>>
>>>>>> Need some maintainer weigh in here.
>>>>>
>>>>> Hehe. I'm maintnainer. Fun.
>>>>
>>>> I know.  I want to see if there was any other opinion.  Especially for the LED driver.
>>>>
>>> [...]
>>>
>>> I have a question - is this lm3697 LED controller a cell of some MFD
>>> device? Or is it a self-contained chip?
>>>
>>
>> This is a self contained chip.  And the LM3697 only function is a LED driver.
>> It does not have any other special functions like the LM363X drivers for GPIO and Regulator support.
> 
> This is an argument for merging it as a standalone LED class driver
> then. It is even more justifiable, taking into account uncertainties
> related to the proper way of adding the support for it to the existing
> MFD driver, whereas the code reuse would be the only advantage of having
> thus support in MFD subsystem.
> 

Does the argument carry over to the other devices?

Like the LM3632 (part of the ti-lmu) has flash and torch and no other special functions
so it would look like the lm3601x family with different register mappings.

The LM3631 seems to also be just a LED driver with no extra functionality

I could go buy an EVM and put together a driver for that device as well using the lm3601x as
reference.

Dan

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-10 19:51               ` Dan Murphy
@ 2018-09-11 18:27                 ` Jacek Anaszewski
  2018-09-11 18:37                   ` Dan Murphy
  2018-09-11 20:55                 ` Pavel Machek
  1 sibling, 1 reply; 15+ messages in thread
From: Jacek Anaszewski @ 2018-09-11 18:27 UTC (permalink / raw)
  To: Dan Murphy, Pavel Machek; +Cc: robh+dt, devicetree, linux-kernel, linux-leds

Dan,

On 09/10/2018 09:51 PM, Dan Murphy wrote:
> Jacek
> 
> On 09/10/2018 02:07 PM, Jacek Anaszewski wrote:
>> Dan, Pavel,
>>
>> On 09/10/2018 04:37 PM, Dan Murphy wrote:
>>> Jacek
>>>
>>> On 09/08/2018 02:53 PM, Jacek Anaszewski wrote:
>>>> Dan,
>>>>
>>>> On 09/07/2018 03:52 PM, Dan Murphy wrote:
>>>> [...]
>>>>>>
>>>>>>> And I think Jacek pointed out that the bindings references in this bindings
>>>>>>> don't even exist.
>>>>>>>
>>>>>>> I am thinking we need to deprecate this MFD driver and consolidate these drivers
>>>>>>> in the LED directory as we indicated before.  I did not find any ti-lmu support
>>>>>>> code.
>>>>>>>
>>>>>>> ti-lmu common core code and then the LED children appending the feature differentiation.
>>>>>>
>>>>>>> Need some maintainer weigh in here.
>>>>>>
>>>>>> Hehe. I'm maintnainer. Fun.
>>>>>
>>>>> I know.  I want to see if there was any other opinion.  Especially for the LED driver.
>>>>>
>>>> [...]
>>>>
>>>> I have a question - is this lm3697 LED controller a cell of some MFD
>>>> device? Or is it a self-contained chip?
>>>>
>>>
>>> This is a self contained chip.  And the LM3697 only function is a LED driver.
>>> It does not have any other special functions like the LM363X drivers for GPIO and Regulator support.
>>
>> This is an argument for merging it as a standalone LED class driver
>> then. It is even more justifiable, taking into account uncertainties
>> related to the proper way of adding the support for it to the existing
>> MFD driver, whereas the code reuse would be the only advantage of having
>> thus support in MFD subsystem.
>>
> 
> Does the argument carry over to the other devices?

If we want to be consequent - yes.

> Like the LM3632 (part of the ti-lmu) has flash and torch and no other special functions
> so it would look like the lm3601x family with different register mappings.

Yes, this is obvious candidate for LED class flash driver.

> The LM3631 seems to also be just a LED driver with no extra functionality
> 
> I could go buy an EVM and put together a driver for that device as well using the lm3601x as
> reference.

I'm not going to encourage you to make this expense, but to put it
politically - I'd happily welcome those drivers in the LED subsystem ;-)

-- 
Best regards,
Jacek Anaszewski

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-11 18:27                 ` Jacek Anaszewski
@ 2018-09-11 18:37                   ` Dan Murphy
  0 siblings, 0 replies; 15+ messages in thread
From: Dan Murphy @ 2018-09-11 18:37 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: robh+dt, devicetree, linux-kernel, linux-leds

Jacek

On 09/11/2018 01:27 PM, Jacek Anaszewski wrote:
> Dan,
> 
> On 09/10/2018 09:51 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 09/10/2018 02:07 PM, Jacek Anaszewski wrote:
>>> Dan, Pavel,
>>>
>>> On 09/10/2018 04:37 PM, Dan Murphy wrote:
>>>> Jacek
>>>>
>>>> On 09/08/2018 02:53 PM, Jacek Anaszewski wrote:
>>>>> Dan,
>>>>>
>>>>> On 09/07/2018 03:52 PM, Dan Murphy wrote:
>>>>> [...]
>>>>>>>
>>>>>>>> And I think Jacek pointed out that the bindings references in this bindings
>>>>>>>> don't even exist.
>>>>>>>>
>>>>>>>> I am thinking we need to deprecate this MFD driver and consolidate these drivers
>>>>>>>> in the LED directory as we indicated before.  I did not find any ti-lmu support
>>>>>>>> code.
>>>>>>>>
>>>>>>>> ti-lmu common core code and then the LED children appending the feature differentiation.
>>>>>>>
>>>>>>>> Need some maintainer weigh in here.
>>>>>>>
>>>>>>> Hehe. I'm maintnainer. Fun.
>>>>>>
>>>>>> I know.  I want to see if there was any other opinion.  Especially for the LED driver.
>>>>>>
>>>>> [...]
>>>>>
>>>>> I have a question - is this lm3697 LED controller a cell of some MFD
>>>>> device? Or is it a self-contained chip?
>>>>>
>>>>
>>>> This is a self contained chip.  And the LM3697 only function is a LED driver.
>>>> It does not have any other special functions like the LM363X drivers for GPIO and Regulator support.
>>>
>>> This is an argument for merging it as a standalone LED class driver
>>> then. It is even more justifiable, taking into account uncertainties
>>> related to the proper way of adding the support for it to the existing
>>> MFD driver, whereas the code reuse would be the only advantage of having
>>> thus support in MFD subsystem.
>>>
>>
>> Does the argument carry over to the other devices?
> 
> If we want to be consequent - yes.
> 
>> Like the LM3632 (part of the ti-lmu) has flash and torch and no other special functions
>> so it would look like the lm3601x family with different register mappings.
> 
> Yes, this is obvious candidate for LED class flash driver.
> 
>> The LM3631 seems to also be just a LED driver with no extra functionality
>>
>> I could go buy an EVM and put together a driver for that device as well using the lm3601x as
>> reference.
> 
> I'm not going to encourage you to make this expense, but to put it
> politically - I'd happily welcome those drivers in the LED subsystem ;-)
> 

Understood.  I am waiting on hardware to test.

Dan

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-10 19:51               ` Dan Murphy
  2018-09-11 18:27                 ` Jacek Anaszewski
@ 2018-09-11 20:55                 ` Pavel Machek
  2018-09-11 21:05                   ` Dan Murphy
  1 sibling, 1 reply; 15+ messages in thread
From: Pavel Machek @ 2018-09-11 20:55 UTC (permalink / raw)
  To: Dan Murphy
  Cc: Jacek Anaszewski, robh+dt, devicetree, linux-kernel, linux-leds

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

Hi!

> >>>>>> And I think Jacek pointed out that the bindings references in this bindings
> >>>>>> don't even exist.
> >>>>>>
> >>>>>> I am thinking we need to deprecate this MFD driver and consolidate these drivers
> >>>>>> in the LED directory as we indicated before.  I did not find any ti-lmu support
> >>>>>> code.
> >>>>>>
> >>>>>> ti-lmu common core code and then the LED children appending the feature differentiation.
> >>>>>
> >>>>>> Need some maintainer weigh in here.
> >>>>>
> >>>>> Hehe. I'm maintnainer. Fun.
> >>>>
> >>>> I know.  I want to see if there was any other opinion.  Especially for the LED driver.
> >>>>
> >>> [...]
> >>>
> >>> I have a question - is this lm3697 LED controller a cell of some MFD
> >>> device? Or is it a self-contained chip?
> >>>
> >>
> >> This is a self contained chip.  And the LM3697 only function is a LED driver.
> >> It does not have any other special functions like the LM363X drivers for GPIO and Regulator support.
> > 
> > This is an argument for merging it as a standalone LED class driver
> > then. It is even more justifiable, taking into account uncertainties
> > related to the proper way of adding the support for it to the existing
> > MFD driver, whereas the code reuse would be the only advantage of having
> > thus support in MFD subsystem.
> 
> Does the argument carry over to the other devices?

We really need something reasonable, that works for stand-alone LEDs,
and also works for LEDs that are part of MFD when the hardware is similar.

> Like the LM3632 (part of the ti-lmu) has flash and torch and no other special functions
> so it would look like the lm3601x family with different register mappings.
> 
> The LM3631 seems to also be just a LED driver with no extra functionality
> 
> I could go buy an EVM and put together a driver for that device as well using the lm3601x as
> reference.

I do have hardware with lm3532. I can test patches, and I guess I can
port driver easily if it is obvious how to do that.

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver
  2018-09-11 20:55                 ` Pavel Machek
@ 2018-09-11 21:05                   ` Dan Murphy
  0 siblings, 0 replies; 15+ messages in thread
From: Dan Murphy @ 2018-09-11 21:05 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Jacek Anaszewski, robh+dt, devicetree, linux-kernel, linux-leds

On 09/11/2018 03:55 PM, Pavel Machek wrote:
> Hi!
> 
>>>>>>>> And I think Jacek pointed out that the bindings references in this bindings
>>>>>>>> don't even exist.
>>>>>>>>
>>>>>>>> I am thinking we need to deprecate this MFD driver and consolidate these drivers
>>>>>>>> in the LED directory as we indicated before.  I did not find any ti-lmu support
>>>>>>>> code.
>>>>>>>>
>>>>>>>> ti-lmu common core code and then the LED children appending the feature differentiation.
>>>>>>>
>>>>>>>> Need some maintainer weigh in here.
>>>>>>>
>>>>>>> Hehe. I'm maintnainer. Fun.
>>>>>>
>>>>>> I know.  I want to see if there was any other opinion.  Especially for the LED driver.
>>>>>>
>>>>> [...]
>>>>>
>>>>> I have a question - is this lm3697 LED controller a cell of some MFD
>>>>> device? Or is it a self-contained chip?
>>>>>
>>>>
>>>> This is a self contained chip.  And the LM3697 only function is a LED driver.
>>>> It does not have any other special functions like the LM363X drivers for GPIO and Regulator support.
>>>
>>> This is an argument for merging it as a standalone LED class driver
>>> then. It is even more justifiable, taking into account uncertainties
>>> related to the proper way of adding the support for it to the existing
>>> MFD driver, whereas the code reuse would be the only advantage of having
>>> thus support in MFD subsystem.
>>
>> Does the argument carry over to the other devices?
> 
> We really need something reasonable, that works for stand-alone LEDs,
> and also works for LEDs that are part of MFD when the hardware is similar.

I agree that LED drivers that have other functional blocks beyond driving a LED
chain belongs in the MFD space.  The amount of code that is similar is very small.

And like I pointed out Droid 4 may be only one use case where it makes sense to combine
all the LED code into a central place.  But most customers will just want a LED specific
driver to maintain.

> 
>> Like the LM3632 (part of the ti-lmu) has flash and torch and no other special functions
>> so it would look like the lm3601x family with different register mappings.
>>
>> The LM3631 seems to also be just a LED driver with no extra functionality
>>
>> I could go buy an EVM and put together a driver for that device as well using the lm3601x as
>> reference.
> 
> I do have hardware with lm3532. I can test patches, and I guess I can
> port driver easily if it is obvious how to do that.


I can get the LM3532 EVM.  I wrote a similar driver for the original Droid 10 years ago.
Upstreaming was not a priority for that company.

Here is a reference to the LM3530 code from back in the day on Google OMAP kernel.
https://android.googlesource.com/kernel/omap/+/android-omap-3.0/drivers/leds/leds-lm3530.c

Otherwise I can create the LM3532 driver as well and look at the LM3530

Dan

> 
> 									Pavel
> 


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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2018-09-11 21:06 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-06 13:50 [PATCH v6 1/2] dt-bindings: leds: Add bindings for lm3697 driver Dan Murphy
2018-09-06 13:50 ` [PATCH v6 2/2] leds: lm3697: Introduce the " Dan Murphy
2018-09-06 21:16 ` [PATCH v6 1/2] dt-bindings: leds: Add bindings for " Pavel Machek
2018-09-07 13:20   ` Dan Murphy
2018-09-07 13:32     ` Pavel Machek
2018-09-07 13:52       ` Dan Murphy
2018-09-08 19:53         ` Jacek Anaszewski
2018-09-10 14:37           ` Dan Murphy
2018-09-10 19:07             ` Jacek Anaszewski
2018-09-10 19:51               ` Dan Murphy
2018-09-11 18:27                 ` Jacek Anaszewski
2018-09-11 18:37                   ` Dan Murphy
2018-09-11 20:55                 ` Pavel Machek
2018-09-11 21:05                   ` Dan Murphy
2018-09-10 15:41           ` Pavel Machek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).