linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 1/7] leds: add TI LMU backlight driver
@ 2018-10-23 17:06 Dan Murphy
  2018-10-23 17:06 ` [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697 Dan Murphy
                   ` (6 more replies)
  0 siblings, 7 replies; 30+ messages in thread
From: Dan Murphy @ 2018-10-23 17:06 UTC (permalink / raw)
  To: robh+dt, jacek.anaszewski, pavel
  Cc: devicetree, linux-kernel, linux-leds, lee.jones, tony, Milo Kim,
	Sebastian Reichel

From: Pavel Machek <pavel@ucw.cz>

This adds backlight support for the following TI LMU
chips: LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.

It controls LEDs on Droid 4
smartphone, including keyboard and screen backlights.

Signed-off-by: Milo Kim <milo.kim@ti.com>
[add LED subsystem support for keyboard backlight and rework DT
binding according to Rob Herrings feedback]
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
[remove backlight subsystem support for now]
Signed-off-by: Pavel Machek <pavel@ucw.cz>
---
 drivers/leds/Kconfig              |   8 ++
 drivers/leds/Makefile             |   1 +
 drivers/leds/ti-lmu-led-common.c  | 138 ++++++++++++++++++++++++++++++
 include/linux/ti-lmu-led-common.h |  59 +++++++++++++
 4 files changed, 206 insertions(+)
 create mode 100644 drivers/leds/ti-lmu-led-common.c
 create mode 100644 include/linux/ti-lmu-led-common.h

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 44097a3e0fcc..dc717b30d9d3 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -756,6 +756,14 @@ config LEDS_NIC78BX
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-nic78bx.
 
+config LEDS_TI_LMU_COMMON
+	tristate "LED driver for TI LMU"
+	depends on REGMAP
+	help
+          Say Y to enable the LED driver for TI LMU devices.
+          This supports common features between the TI LM3532, LM3631, LM3632,
+	  LM3633, LM3695 and LM3697.
+
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 420b5d2cfa62..e09bb27bc7ea 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -78,6 +78,7 @@ obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
 obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
 obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
+obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= ti-lmu-led-common.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
diff --git a/drivers/leds/ti-lmu-led-common.c b/drivers/leds/ti-lmu-led-common.c
new file mode 100644
index 000000000000..a4435bd1d248
--- /dev/null
+++ b/drivers/leds/ti-lmu-led-common.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2015 Texas Instruments
+ * Copyright 2018 Sebastian Reichel
+ * Copyright 2018 Pavel Machek <pavel@ucw.cz>
+ *
+ * TI LMU Led driver, based on previous work from
+ * Milo Kim <milo.kim@ti.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/notifier.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include <linux/ti-lmu-led-common.h>
+
+const static int ramp_table[16] = { 2, 262, 524, 1049, 2090, 4194, 8389,
+				16780, 33550, 41940, 50330, 58720,
+				67110, 83880, 100660, 117440};
+
+static int ti_lmu_common_update_brightness_register(struct ti_lmu_bank *lmu_bank,
+						       int brightness)
+{
+	struct regmap *regmap = lmu_bank->regmap;
+	u8 reg, val;
+	int ret;
+
+	/*
+	 * Brightness register update
+	 *
+	 * 11 bit dimming: update LSB bits and write MSB byte.
+	 *		   MSB brightness should be shifted.
+	 *  8 bit dimming: write MSB byte.
+	 */
+	if (lmu_bank->max_brightness == MAX_BRIGHTNESS_11BIT) {
+		reg = lmu_bank->lsb_brightness_reg;
+		ret = regmap_update_bits(regmap, reg,
+					 LMU_11BIT_LSB_MASK,
+					 brightness);
+		if (ret)
+			return ret;
+
+		val = brightness >> LMU_11BIT_MSB_SHIFT;
+	} else {
+		val = brightness;
+	}
+
+	reg = lmu_bank->msb_brightness_reg;
+
+	return regmap_write(regmap, reg, val);
+}
+
+int ti_lmu_common_set_brightness(struct ti_lmu_bank *lmu_bank,
+				    int brightness)
+{
+	lmu_bank->current_brightness = brightness;
+
+	return ti_lmu_common_update_brightness_register(lmu_bank, brightness);
+}
+EXPORT_SYMBOL(ti_lmu_common_set_brightness);
+
+static int ti_lmu_common_convert_ramp_to_index(unsigned int msec)
+{
+	int size = ARRAY_SIZE(ramp_table);
+	int i;
+
+	if (msec <= ramp_table[0])
+		return 0;
+
+	if (msec > ramp_table[size - 1])
+		return size - 1;
+
+	for (i = 1; i < size; i++) {
+		if (msec == ramp_table[i])
+			return i;
+
+		/* Find an approximate index by looking up the table */
+		if (msec > ramp_table[i - 1] && msec < ramp_table[i]) {
+			if (msec - ramp_table[i - 1] < ramp_table[i] - msec)
+				return i - 1;
+			else
+				return i;
+		}
+	}
+
+	return -EINVAL;
+}
+
+int ti_lmu_common_set_ramp(struct ti_lmu_bank *lmu_bank)
+{
+	struct regmap *regmap = lmu_bank->regmap;
+	u8 ramp, ramp_up, ramp_down;
+
+	if (lmu_bank->ramp_up_msec == 0 && lmu_bank->ramp_down_msec == 0) {
+		ramp_up = 0;
+		ramp_down = 0;
+	} else {
+		ramp_up = ti_lmu_common_convert_ramp_to_index(lmu_bank->ramp_up_msec);
+		ramp_down = ti_lmu_common_convert_ramp_to_index(lmu_bank->ramp_down_msec);
+	}
+
+	if (ramp_up < 0 || ramp_down < 0)
+		return -EINVAL;
+
+	ramp = (ramp_up << 4) | ramp_down;
+
+	return regmap_write(regmap, lmu_bank->runtime_ramp_reg, ramp);
+
+}
+EXPORT_SYMBOL(ti_lmu_common_set_ramp);
+
+int ti_lmu_common_get_ramp_params(struct device *dev,
+				  struct fwnode_handle *child,
+				  struct ti_lmu_bank *lmu_data)
+{
+	int ret;
+
+	ret = fwnode_property_read_u32(child, "ramp-up-ms",
+				 &lmu_data->ramp_up_msec);
+	if (ret)
+		dev_warn(dev, "ramp-up-ms property missing\n");
+
+
+	ret = fwnode_property_read_u32(child, "ramp-down-ms",
+				 &lmu_data->ramp_down_msec);
+	if (ret)
+		dev_warn(dev, "ramp-down-ms property missing\n");
+
+	return 0;
+}
+EXPORT_SYMBOL(ti_lmu_common_get_ramp_params);
+
+MODULE_DESCRIPTION("TI LMU LED Driver");
+MODULE_AUTHOR("Sebastian Reichel");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:ti-lmu-led");
diff --git a/include/linux/ti-lmu-led-common.h b/include/linux/ti-lmu-led-common.h
new file mode 100644
index 000000000000..b0fe08f05be2
--- /dev/null
+++ b/include/linux/ti-lmu-led-common.h
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0
+// TI LMU Common Core
+// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+
+#ifndef _TI_LMU_COMMON_H_
+#define _TI_LMU_COMMON_H_
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <uapi/linux/uleds.h>
+
+#define LMU_DUAL_CHANNEL_USED	(BIT(0) | BIT(1))
+#define LMU_11BIT_LSB_MASK	(BIT(0) | BIT(1) | BIT(2))
+#define LMU_11BIT_MSB_SHIFT	3
+
+#define MAX_BRIGHTNESS_8BIT	255
+#define MAX_BRIGHTNESS_11BIT	2047
+
+#define NUM_DUAL_CHANNEL	2
+
+struct ti_lmu_bank {
+	struct regmap *regmap;
+
+	int bank_id;
+	int fault_monitor_used;
+
+	u8 enable_reg;
+	unsigned long enable_usec;
+
+	int current_brightness;
+	u32 default_brightness;
+	int max_brightness;
+
+	u8 lsb_brightness_reg;
+	u8 msb_brightness_reg;
+
+	u8 runtime_ramp_reg;
+	u32 ramp_up_msec;
+	u32 ramp_down_msec;
+};
+
+
+int ti_lmu_common_set_brightness(struct ti_lmu_bank *lmu_bank,
+				    int brightness);
+
+int ti_lmu_common_set_ramp(struct ti_lmu_bank *lmu_bank);
+
+int ti_lmu_common_get_ramp_params(struct device *dev,
+				  struct fwnode_handle *child,
+				  struct ti_lmu_bank *lmu_data);
+
+#endif /* _TI_LMU_COMMON_H_ */
-- 
2.19.0


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

* [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-23 17:06 [PATCH v4 1/7] leds: add TI LMU backlight driver Dan Murphy
@ 2018-10-23 17:06 ` Dan Murphy
  2018-10-24  9:04   ` Pavel Machek
  2018-10-24 14:49   ` Rob Herring
  2018-10-23 17:06 ` [PATCH v4 3/7] mfd: ti-lmu: Remove support for LM3697 Dan Murphy
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 30+ messages in thread
From: Dan Murphy @ 2018-10-23 17:06 UTC (permalink / raw)
  To: robh+dt, jacek.anaszewski, pavel
  Cc: devicetree, linux-kernel, linux-leds, lee.jones, tony, Dan Murphy

The LM3697 is a single function LED driver. The single function LED
driver needs to reside in the LED directory as a dedicated LED driver
and not as a MFD device.  The device does have common brightness and ramp
features and those can be accomodated by a TI LMU framework.

The LM3697 dt binding needs to be moved from the ti-lmu.txt and a dedicated
LED dt binding needs to be added.  The new LM3697 LED dt binding will then
reside in the Documentation/devicetree/bindings/leds directory and follow the
current LED and general bindings guidelines.

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

v4 - Squashed removal and addition of the dt bindings into a single patch - https://lore.kernel.org/patchwork/patch/998703/

 .../devicetree/bindings/leds/leds-lm3697.txt  | 98 +++++++++++++++++++
 .../devicetree/bindings/mfd/ti-lmu.txt        | 26 +----
 2 files changed, 99 insertions(+), 25 deletions(-)
 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..4bb2ed51025b
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
@@ -0,0 +1,98 @@
+* 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,lm3697"
+	- 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:
+	- runtime-ramp-up-msec: Current ramping from one brightness level to
+				the a higher brightness level.
+				Range from 2048 us - 117.44 s
+	- runtime-ramp-down-msec: Current ramping from one brightness level to
+				  the a lower brightness level.
+				  Range from 2048 us - 117.44 s
+	- 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,lm3697";
+	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>;
+		runtime-ramp-up-msec = <5000>;
+		runtime-ramp-down-msec = <1000>;
+		label = "white:first_backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
+
+	led@1 {
+		reg = <1>;
+		led-sources = <0 1 0>;
+		runtime-ramp-up-msec = <500>;
+		runtime-ramp-down-msec = <1000>;
+		label = "white:second_backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
+}
+
+All HVLED strings controlled by control bank A
+
+led-controller@36 {
+	compatible = "ti,lm3697";
+	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>;
+		runtime-ramp-up-msec = <500>;
+		runtime-ramp-down-msec = <1000>;
+		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
diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
index c885cf89b8ce..920f910be4e9 100644
--- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
+++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
@@ -9,7 +9,6 @@ TI LMU driver supports lighting devices below.
   LM3632       Backlight and regulator
   LM3633       Backlight, LED and fault monitor
   LM3695       Backlight
-  LM3697       Backlight and fault monitor
 
 Required properties:
   - compatible: Should be one of:
@@ -18,11 +17,10 @@ Required properties:
                 "ti,lm3632"
                 "ti,lm3633"
                 "ti,lm3695"
-                "ti,lm3697"
   - reg: I2C slave address.
          0x11 for LM3632
          0x29 for LM3631
-         0x36 for LM3633, LM3697
+         0x36 for LM3633
          0x38 for LM3532
          0x63 for LM3695
 
@@ -38,7 +36,6 @@ Optional nodes:
     Required properties:
       - compatible: Should be one of:
                     "ti,lm3633-fault-monitor"
-                    "ti,lm3697-fault-monitor"
   - leds: LED properties for LM3633. Please refer to [2].
   - regulators: Regulator properties for LM3631 and LM3632.
                 Please refer to [3].
@@ -220,24 +217,3 @@ lm3695@63 {
 		};
 	};
 };
-
-lm3697@36 {
-	compatible = "ti,lm3697";
-	reg = <0x36>;
-
-	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>;
-		};
-	};
-
-	fault-monitor {
-		compatible = "ti,lm3697-fault-monitor";
-	};
-};
-- 
2.19.0


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

* [PATCH v4 3/7] mfd: ti-lmu: Remove support for LM3697
  2018-10-23 17:06 [PATCH v4 1/7] leds: add TI LMU backlight driver Dan Murphy
  2018-10-23 17:06 ` [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697 Dan Murphy
@ 2018-10-23 17:06 ` Dan Murphy
  2018-10-23 17:06 ` [PATCH v4 4/7] leds: lm3697: Introduce the lm3697 driver Dan Murphy
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 30+ messages in thread
From: Dan Murphy @ 2018-10-23 17:06 UTC (permalink / raw)
  To: robh+dt, jacek.anaszewski, pavel
  Cc: devicetree, linux-kernel, linux-leds, lee.jones, tony, Dan Murphy

Remove support for the LM3697 from the ti-lmu
driver in favor of a dedicated LED driver.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/Kconfig                |  2 +-
 drivers/mfd/Kconfig                 |  2 +-
 drivers/mfd/ti-lmu.c                | 17 -----------
 include/linux/mfd/ti-lmu-register.h | 44 -----------------------------
 include/linux/mfd/ti-lmu.h          |  1 -
 5 files changed, 2 insertions(+), 64 deletions(-)

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index dc717b30d9d3..853014dbb1c2 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -762,7 +762,7 @@ config LEDS_TI_LMU_COMMON
 	help
           Say Y to enable the LED driver for TI LMU devices.
           This supports common features between the TI LM3532, LM3631, LM3632,
-	  LM3633, LM3695 and LM3697.
+	  LM3633, and LM3695.
 
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 11841f4b7b2b..9b04dd527c68 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1293,7 +1293,7 @@ config MFD_TI_LMU
 	help
 	  Say yes here to enable support for TI LMU chips.
 
-	  TI LMU MFD supports LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
+	  TI LMU MFD supports LM3532, LM3631, LM3632, LM3633, and LM3695.
 	  It consists of backlight, LED and regulator driver.
 	  It provides consistent device controls for lighting functions.
 
diff --git a/drivers/mfd/ti-lmu.c b/drivers/mfd/ti-lmu.c
index cfb411cde51c..b6bfa99a29dd 100644
--- a/drivers/mfd/ti-lmu.c
+++ b/drivers/mfd/ti-lmu.c
@@ -128,20 +128,6 @@ static struct mfd_cell lm3695_devices[] = {
 	},
 };
 
-static struct mfd_cell lm3697_devices[] = {
-	{
-		.name          = "ti-lmu-backlight",
-		.id            = LM3697,
-		.of_compatible = "ti,lm3697-backlight",
-	},
-	/* Monitoring driver for open/short circuit detection */
-	{
-		.name          = "ti-lmu-fault-monitor",
-		.id            = LM3697,
-		.of_compatible = "ti,lm3697-fault-monitor",
-	},
-};
-
 #define TI_LMU_DATA(chip, max_reg)		\
 static const struct ti_lmu_data chip##_data =	\
 {						\
@@ -155,7 +141,6 @@ TI_LMU_DATA(lm3631, LM3631_MAX_REG);
 TI_LMU_DATA(lm3632, LM3632_MAX_REG);
 TI_LMU_DATA(lm3633, LM3633_MAX_REG);
 TI_LMU_DATA(lm3695, LM3695_MAX_REG);
-TI_LMU_DATA(lm3697, LM3697_MAX_REG);
 
 static const struct of_device_id ti_lmu_of_match[] = {
 	{ .compatible = "ti,lm3532", .data = &lm3532_data },
@@ -163,7 +148,6 @@ static const struct of_device_id ti_lmu_of_match[] = {
 	{ .compatible = "ti,lm3632", .data = &lm3632_data },
 	{ .compatible = "ti,lm3633", .data = &lm3633_data },
 	{ .compatible = "ti,lm3695", .data = &lm3695_data },
-	{ .compatible = "ti,lm3697", .data = &lm3697_data },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ti_lmu_of_match);
@@ -237,7 +221,6 @@ static const struct i2c_device_id ti_lmu_ids[] = {
 	{ "lm3632", LM3632 },
 	{ "lm3633", LM3633 },
 	{ "lm3695", LM3695 },
-	{ "lm3697", LM3697 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, ti_lmu_ids);
diff --git a/include/linux/mfd/ti-lmu-register.h b/include/linux/mfd/ti-lmu-register.h
index 2125c7c02818..99711ff4b809 100644
--- a/include/linux/mfd/ti-lmu-register.h
+++ b/include/linux/mfd/ti-lmu-register.h
@@ -233,48 +233,4 @@
 #define LM3695_REG_BRT_MSB			0x14
 
 #define LM3695_MAX_REG				0x14
-
-/* LM3697 */
-#define LM3697_REG_HVLED_OUTPUT_CFG		0x10
-#define LM3697_HVLED1_CFG_MASK			BIT(0)
-#define LM3697_HVLED2_CFG_MASK			BIT(1)
-#define LM3697_HVLED3_CFG_MASK			BIT(2)
-#define LM3697_HVLED1_CFG_SHIFT			0
-#define LM3697_HVLED2_CFG_SHIFT			1
-#define LM3697_HVLED3_CFG_SHIFT			2
-
-#define LM3697_REG_BL0_RAMP			0x11
-#define LM3697_REG_BL1_RAMP			0x12
-#define LM3697_RAMPUP_MASK			0xF0
-#define LM3697_RAMPUP_SHIFT			4
-#define LM3697_RAMPDN_MASK			0x0F
-#define LM3697_RAMPDN_SHIFT			0
-
-#define LM3697_REG_RAMP_CONF			0x14
-#define LM3697_RAMP_MASK			0x0F
-#define LM3697_RAMP_EACH			0x05
-
-#define LM3697_REG_PWM_CFG			0x1C
-#define LM3697_PWM_A_MASK			BIT(0)
-#define LM3697_PWM_B_MASK			BIT(1)
-
-#define LM3697_REG_IMAX_A			0x17
-#define LM3697_REG_IMAX_B			0x18
-
-#define LM3697_REG_FEEDBACK_ENABLE		0x19
-
-#define LM3697_REG_BRT_A_LSB			0x20
-#define LM3697_REG_BRT_A_MSB			0x21
-#define LM3697_REG_BRT_B_LSB			0x22
-#define LM3697_REG_BRT_B_MSB			0x23
-
-#define LM3697_REG_ENABLE			0x24
-
-#define LM3697_REG_OPEN_FAULT_STATUS		0xB0
-
-#define LM3697_REG_SHORT_FAULT_STATUS		0xB2
-
-#define LM3697_REG_MONITOR_ENABLE		0xB4
-
-#define LM3697_MAX_REG				0xB4
 #endif
diff --git a/include/linux/mfd/ti-lmu.h b/include/linux/mfd/ti-lmu.h
index 09d5f30384e5..bc9272f08f47 100644
--- a/include/linux/mfd/ti-lmu.h
+++ b/include/linux/mfd/ti-lmu.h
@@ -26,7 +26,6 @@ enum ti_lmu_id {
 	LM3632,
 	LM3633,
 	LM3695,
-	LM3697,
 	LMU_MAX_ID,
 };
 
-- 
2.19.0


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

* [PATCH v4 4/7] leds: lm3697: Introduce the lm3697 driver
  2018-10-23 17:06 [PATCH v4 1/7] leds: add TI LMU backlight driver Dan Murphy
  2018-10-23 17:06 ` [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697 Dan Murphy
  2018-10-23 17:06 ` [PATCH v4 3/7] mfd: ti-lmu: Remove support for LM3697 Dan Murphy
@ 2018-10-23 17:06 ` Dan Murphy
  2018-10-23 17:06 ` [PATCH v4 5/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3633 Dan Murphy
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 30+ messages in thread
From: Dan Murphy @ 2018-10-23 17:06 UTC (permalink / raw)
  To: robh+dt, jacek.anaszewski, pavel
  Cc: devicetree, linux-kernel, linux-leds, lee.jones, tony, 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>
---
 drivers/leds/Kconfig       |   8 +-
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-lm3697.c | 381 +++++++++++++++++++++++++++++++++++++
 3 files changed, 389 insertions(+), 1 deletion(-)
 create mode 100644 drivers/leds/leds-lm3697.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 853014dbb1c2..7d9a98044be4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -756,9 +756,15 @@ config LEDS_NIC78BX
 	  To compile this driver as a module, choose M here: the module
 	  will be called leds-nic78bx.
 
+config LEDS_LM3697
+	tristate "LED driver for LM3697"
+ 	depends on LEDS_TI_LMU_COMMON
+	help
+          Say Y to enable the LED driver for TI LMU devices.
+          This supports the LED device LM3697.
+
 config LEDS_TI_LMU_COMMON
 	tristate "LED driver for TI LMU"
-	depends on REGMAP
 	help
           Say Y to enable the LED driver for TI LMU devices.
           This supports common features between the TI LM3532, LM3631, LM3632,
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index e09bb27bc7ea..6fbce7cfc41c 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -78,6 +78,7 @@ obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
 obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
 obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
+obj-$(CONFIG_LEDS_LM3697)		+= leds-lm3697.o
 obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= ti-lmu-led-common.o
 
 # LED SPI Drivers
diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
new file mode 100644
index 000000000000..1ed724fb0c43
--- /dev/null
+++ b/drivers/leds/leds-lm3697.c
@@ -0,0 +1,381 @@
+// SPDX-License-Identifier: GPL-2.0
+// TI LM3697 LED chip family driver
+// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/regulator/consumer.h>
+#include <linux/ti-lmu-led-common.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_MAX_CONTROL_BANKS 2
+
+#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
+ * @lmu_data: Register and setting values for common code
+ * @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;
+	struct ti_lmu_bank lmu_data;
+	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 ctrl_en_val;
+	int ret;
+
+	mutex_lock(&led->priv->lock);
+
+	if (led->control_bank == LM3697_CONTROL_A) {
+		led->lmu_data.msb_brightness_reg = LM3697_CTRL_A_BRT_MSB;
+		led->lmu_data.lsb_brightness_reg = LM3697_CTRL_A_BRT_LSB;
+		ctrl_en_val = LM3697_CTRL_A_EN;
+	} else {
+		led->lmu_data.msb_brightness_reg = LM3697_CTRL_B_BRT_MSB;
+		led->lmu_data.lsb_brightness_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);
+
+	ret = ti_lmu_common_set_brightness(&led->lmu_data, brt_val);
+	if (ret)
+		dev_err(&led->priv->client->dev, "Cannot write brightness\n");
+
+	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)
+{
+	struct lm3697_led *led;
+	int i, 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");
+
+	for (i = 0; i < LM3697_MAX_CONTROL_BANKS; i++) {
+		led = &priv->leds[i];
+		ti_lmu_common_set_ramp(&led->lmu_data);
+		if (ret)
+			dev_err(&priv->client->dev, "Setting the ramp rate 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;
+		led->lmu_data.bank_id = control_bank;
+		led->lmu_data.enable_reg = LM3697_CTRL_ENABLE;
+		led->lmu_data.regmap = priv->regmap;
+		if (control_bank == LM3697_CONTROL_A)
+			led->lmu_data.runtime_ramp_reg = LM3697_CTRL_A_RAMP;
+		else
+			led->lmu_data.runtime_ramp_reg = LM3697_CTRL_B_RAMP;
+
+		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;
+		}
+
+		ret = ti_lmu_common_get_ramp_params(&priv->client->dev, child, &led->lmu_data);
+		if (ret)
+			dev_warn(&priv->client->dev, "runtime-ramp properties missing\n");
+
+		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.19.0


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

* [PATCH v4 5/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3633
  2018-10-23 17:06 [PATCH v4 1/7] leds: add TI LMU backlight driver Dan Murphy
                   ` (2 preceding siblings ...)
  2018-10-23 17:06 ` [PATCH v4 4/7] leds: lm3697: Introduce the lm3697 driver Dan Murphy
@ 2018-10-23 17:06 ` Dan Murphy
  2018-10-24  9:23   ` Pavel Machek
  2018-10-23 17:06 ` [PATCH v4 6/7] mfd: ti-lmu: Remove support for LM3633 Dan Murphy
                   ` (2 subsequent siblings)
  6 siblings, 1 reply; 30+ messages in thread
From: Dan Murphy @ 2018-10-23 17:06 UTC (permalink / raw)
  To: robh+dt, jacek.anaszewski, pavel
  Cc: devicetree, linux-kernel, linux-leds, lee.jones, tony, Dan Murphy

The LM3633 is a single function LED driver. The single function LED
driver needs to reside in the LED directory as a dedicated LED driver
and not as a MFD device.  The device does have common brightness and ramp
features and those can be accomodated by a TI LMU framework.

The LM3633 dt binding needs to be moved from the ti-lmu.txt and a dedicated
LED dt binding needs to be added.  The new LM3633 LED dt binding will then
reside in the Documentation/devicetree/bindings/leds directory and follow the
current LED and general bindings guidelines.

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

v4 - Squashed removal and addition of the dt bindings into a single patch - https://lore.kernel.org/patchwork/patch/998703/

 .../devicetree/bindings/leds/leds-lm3633.txt  | 102 ++++++++++++++++++
 .../devicetree/bindings/mfd/ti-lmu.txt        |  48 ---------
 2 files changed, 102 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3633.txt

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3633.txt b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
new file mode 100644
index 000000000000..d791b891ea6e
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3633.txt
@@ -0,0 +1,102 @@
+* Texas Instruments - LM3633 Lighting Power Solution for Smartphone Handsets
+
+The LM3633 is a complete power source for
+backlight, keypad, and indicator LEDs in smartphone
+handsets.
+
+This device is suitable for display and keypad Lighting
+
+Required properties:
+	- compatible:
+		"ti,lm3633"
+	- 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 - HVLED is Controlled by bank A
+		1 - HVLED is Controlled by bank B
+		2,3,4 - LVLED1, 2 and 3 are Controlled by bank C, D and E
+		5,6,7 - LVLED4, 5 and 6 are Controlled by bank F, G and H
+	- led-sources : Indicates which LED string is associated to which
+			control bank.
+			0 - LED is not active in this control bank
+			1 - LED string is controlled by this control bank
+
+Optional child properties:
+	- runtime-ramp-up-msec: Current ramping from one brightness level to
+				the a higher brightness level.
+				Range from 2048 us - 117.44 s
+	- runtime-ramp-down-msec: Current ramping from one brightness level to
+				  the a lower brightness level.
+				  Range from 2048 us - 117.44 s
+	- label : see Documentation/devicetree/bindings/leds/common.txt
+	- linux,default-trigger :
+	   see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+Control bank C output to 3 LVLEDs and Control F, G and H have independent
+controls of the LVLEDs.
+
+led-controller@36 {
+	compatible = "ti,lm3633";
+	reg = <0x36>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	enable-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
+	led@0 {
+		reg = <0>;
+		led-sources = < 1 0 1 >;
+		label = "white:backlight";
+		ramp-up-ms = <100000>;
+		ramp-down-ms = <1000>;
+		linux,default-trigger = "backlight";
+	};
+
+	led@1 {
+		reg = <1>;
+		led-sources = < 0 1 0 >;
+		label = "white:light";
+		ramp-up-ms = <100000>;
+		ramp-down-ms = <1000>;
+	};
+
+	led@2 {
+		reg = <2>;
+		led-sources = <1>;
+		ramp-up-ms = <100000>;
+		ramp-down-ms = <1000>;
+		label = "white:indicator1";
+	};
+
+	led@5 {
+		reg = <5>;
+		led-sources = <1>;
+		ramp-up-ms = <100000>;
+		ramp-down-ms = <1000>;
+		label = "red:light";
+	};
+
+	led@6 {
+		reg = <6>;
+		led-sources = <1>;
+		ramp-up-ms = <41940>;
+		ramp-down-ms = <1000>;
+		label = "green:light";
+	};
+
+	led@7 {
+		reg = <7>;
+		led-sources = <1>;
+		ramp-up-ms = <100000>;
+		ramp-down-ms = <1000>;
+		label = "blue:light";
+	};
+};
+For more product information please see the link below:
+http://www.ti.com/lit/ds/symlink/lm3633.pdf
diff --git a/Documentation/devicetree/bindings/mfd/ti-lmu.txt b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
index 920f910be4e9..573e88578d3d 100644
--- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
+++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
@@ -7,7 +7,6 @@ TI LMU driver supports lighting devices below.
   LM3532       Backlight
   LM3631       Backlight and regulator
   LM3632       Backlight and regulator
-  LM3633       Backlight, LED and fault monitor
   LM3695       Backlight
 
 Required properties:
@@ -15,12 +14,10 @@ Required properties:
                 "ti,lm3532"
                 "ti,lm3631"
                 "ti,lm3632"
-                "ti,lm3633"
                 "ti,lm3695"
   - reg: I2C slave address.
          0x11 for LM3632
          0x29 for LM3631
-         0x36 for LM3633
          0x38 for LM3532
          0x63 for LM3695
 
@@ -157,51 +154,6 @@ lm3632@11 {
 	};
 };
 
-lm3633@36 {
-	compatible = "ti,lm3633";
-	reg = <0x36>;
-
-	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>;
-		};
-
-		front {
-			label = "front_lcd";
-			led-sources = <0>;
-			ramp-up-msec = <1000>;
-			ramp-down-msec = <0>;
-		};
-	};
-
-	leds {
-		compatible = "ti,lm3633-leds";
-
-		chan1 {
-			label = "status";
-			led-sources = <1>;
-			led-max-microamp = <6000>;
-		};
-
-		chan345 {
-			label = "rgb";
-			led-sources = <3 4 5>;
-			led-max-microamp = <10000>;
-		};
-	};
-
-	fault-monitor {
-		compatible = "ti,lm3633-fault-monitor";
-	};
-};
-
 lm3695@63 {
 	compatible = "ti,lm3695";
 	reg = <0x63>;
-- 
2.19.0


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

* [PATCH v4 6/7] mfd: ti-lmu: Remove support for LM3633
  2018-10-23 17:06 [PATCH v4 1/7] leds: add TI LMU backlight driver Dan Murphy
                   ` (3 preceding siblings ...)
  2018-10-23 17:06 ` [PATCH v4 5/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3633 Dan Murphy
@ 2018-10-23 17:06 ` Dan Murphy
  2018-10-23 17:06 ` [PATCH v4 7/7] leds: lm3633: Introduce the lm3633 driver Dan Murphy
  2018-10-24  9:14 ` [PATCH v4 1/7] leds: add TI LMU backlight driver Pavel Machek
  6 siblings, 0 replies; 30+ messages in thread
From: Dan Murphy @ 2018-10-23 17:06 UTC (permalink / raw)
  To: robh+dt, jacek.anaszewski, pavel
  Cc: devicetree, linux-kernel, linux-leds, lee.jones, tony, Dan Murphy

Remove support for the LM3633 from the ti-lmu
driver in favor of a dedicated LED driver.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/mfd/Kconfig  |  2 +-
 drivers/mfd/ti-lmu.c | 21 ---------------------
 2 files changed, 1 insertion(+), 22 deletions(-)

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 9b04dd527c68..225cb3be350c 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1293,7 +1293,7 @@ config MFD_TI_LMU
 	help
 	  Say yes here to enable support for TI LMU chips.
 
-	  TI LMU MFD supports LM3532, LM3631, LM3632, LM3633, and LM3695.
+	  TI LMU MFD supports LM3532, LM3631, LM3632, and LM3695.
 	  It consists of backlight, LED and regulator driver.
 	  It provides consistent device controls for lighting functions.
 
diff --git a/drivers/mfd/ti-lmu.c b/drivers/mfd/ti-lmu.c
index b6bfa99a29dd..2cf8a23cba52 100644
--- a/drivers/mfd/ti-lmu.c
+++ b/drivers/mfd/ti-lmu.c
@@ -102,24 +102,6 @@ static struct mfd_cell lm3632_devices[] = {
 	},
 };
 
-static struct mfd_cell lm3633_devices[] = {
-	{
-		.name          = "ti-lmu-backlight",
-		.id            = LM3633,
-		.of_compatible = "ti,lm3633-backlight",
-	},
-	{
-		.name          = "lm3633-leds",
-		.of_compatible = "ti,lm3633-leds",
-	},
-	/* Monitoring driver for open/short circuit detection */
-	{
-		.name          = "ti-lmu-fault-monitor",
-		.id            = LM3633,
-		.of_compatible = "ti,lm3633-fault-monitor",
-	},
-};
-
 static struct mfd_cell lm3695_devices[] = {
 	{
 		.name          = "ti-lmu-backlight",
@@ -139,14 +121,12 @@ static const struct ti_lmu_data chip##_data =	\
 TI_LMU_DATA(lm3532, LM3532_MAX_REG);
 TI_LMU_DATA(lm3631, LM3631_MAX_REG);
 TI_LMU_DATA(lm3632, LM3632_MAX_REG);
-TI_LMU_DATA(lm3633, LM3633_MAX_REG);
 TI_LMU_DATA(lm3695, LM3695_MAX_REG);
 
 static const struct of_device_id ti_lmu_of_match[] = {
 	{ .compatible = "ti,lm3532", .data = &lm3532_data },
 	{ .compatible = "ti,lm3631", .data = &lm3631_data },
 	{ .compatible = "ti,lm3632", .data = &lm3632_data },
-	{ .compatible = "ti,lm3633", .data = &lm3633_data },
 	{ .compatible = "ti,lm3695", .data = &lm3695_data },
 	{ }
 };
@@ -219,7 +199,6 @@ static const struct i2c_device_id ti_lmu_ids[] = {
 	{ "lm3532", LM3532 },
 	{ "lm3631", LM3631 },
 	{ "lm3632", LM3632 },
-	{ "lm3633", LM3633 },
 	{ "lm3695", LM3695 },
 	{ }
 };
-- 
2.19.0


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

* [PATCH v4 7/7] leds: lm3633: Introduce the lm3633 driver
  2018-10-23 17:06 [PATCH v4 1/7] leds: add TI LMU backlight driver Dan Murphy
                   ` (4 preceding siblings ...)
  2018-10-23 17:06 ` [PATCH v4 6/7] mfd: ti-lmu: Remove support for LM3633 Dan Murphy
@ 2018-10-23 17:06 ` Dan Murphy
  2018-10-24  9:14 ` [PATCH v4 1/7] leds: add TI LMU backlight driver Pavel Machek
  6 siblings, 0 replies; 30+ messages in thread
From: Dan Murphy @ 2018-10-23 17:06 UTC (permalink / raw)
  To: robh+dt, jacek.anaszewski, pavel
  Cc: devicetree, linux-kernel, linux-leds, lee.jones, tony, Dan Murphy

Introduce the LED LM3633 driver.  This LED
driver has 9 total LED outputs with runtime
internal ramp configurations.

Data sheet:
http://www.ti.com/lit/ds/symlink/lm3633.pdf

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 drivers/leds/Kconfig       |   8 +
 drivers/leds/Makefile      |   1 +
 drivers/leds/leds-lm3633.c | 556 +++++++++++++++++++++++++++++++++++++
 3 files changed, 565 insertions(+)
 create mode 100644 drivers/leds/leds-lm3633.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7d9a98044be4..1f9093e5d902 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -763,8 +763,16 @@ config LEDS_LM3697
           Say Y to enable the LED driver for TI LMU devices.
           This supports the LED device LM3697.
 
+config LEDS_LM3633
+	tristate "LED driver for LM3633"
+ 	depends on LEDS_TI_LMU_COMMON
+	help
+          Say Y to enable the LED driver for TI LMU devices.
+          This supports the LED device LM3633.
+
 config LEDS_TI_LMU_COMMON
 	tristate "LED driver for TI LMU"
+	depends on REGMAP
 	help
           Say Y to enable the LED driver for TI LMU devices.
           This supports common features between the TI LM3532, LM3631, LM3632,
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 6fbce7cfc41c..6ec006892fc0 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -79,6 +79,7 @@ obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
 obj-$(CONFIG_LEDS_SC27XX_BLTC)		+= leds-sc27xx-bltc.o
 obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
 obj-$(CONFIG_LEDS_LM3697)		+= leds-lm3697.o
+obj-$(CONFIG_LEDS_LM3633)		+= leds-lm3633.o
 obj-$(CONFIG_LEDS_TI_LMU_COMMON)	+= ti-lmu-led-common.o
 
 # LED SPI Drivers
diff --git a/drivers/leds/leds-lm3633.c b/drivers/leds/leds-lm3633.c
new file mode 100644
index 000000000000..de8eb7b83d20
--- /dev/null
+++ b/drivers/leds/leds-lm3633.c
@@ -0,0 +1,556 @@
+// SPDX-License-Identifier: GPL-2.0
+// TI LM3633 LED chip family driver
+// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/ti-lmu-led-common.h>
+#include <linux/regulator/consumer.h>
+
+#define LM3633_REV			0x0
+#define LM3633_RESET			0x1
+#define LM3633_HVLED_OUTPUT_CONFIG	0x10
+#define LM3633_LVLED_OUTPUT_CONFIG	0x11
+
+#define LM3633_CTRL_A_RAMP		0x12
+#define LM3633_CTRL_B_RAMP		0x13
+#define LM3633_CTRL_C_RAMP		0x14
+#define LM3633_CTRL_D_RAMP		0x15
+#define LM3633_CTRL_E_RAMP		0x16
+#define LM3633_CTRL_F_RAMP		0x17
+#define LM3633_CTRL_G_RAMP		0x18
+#define LM3633_CTRL_H_RAMP		0x19
+
+#define LM3633_CTRL_A_B_RT_RAMP		0x1a
+#define LM3633_CTRL_A_B_RAMP_CFG	0x1b
+#define LM3633_CTRL_C_E_RT_RAMP		0x1c
+#define LM3633_CTRL_F_H_RT_RAMP		0x1d
+
+#define LM3633_CTRL_A_B_BRT_CFG		0x16
+#define LM3633_CTRL_A_FS_CURR_CFG	0x17
+#define LM3633_CTRL_B_FS_CURR_CFG	0x18
+#define LM3633_PWM_CFG			0x1c
+
+#define LM3633_CTRL_ENABLE		0x2b
+
+#define LM3633_CTRL_A_BRT_LSB		0x40
+#define LM3633_CTRL_A_BRT_MSB		0x41
+#define LM3633_CTRL_B_BRT_LSB		0x42
+#define LM3633_CTRL_B_BRT_MSB		0x43
+#define LM3633_CTRL_C_BRT		0x44
+#define LM3633_CTRL_D_BRT		0x45
+#define LM3633_CTRL_E_BRT		0x46
+#define LM3633_CTRL_F_BRT		0x47
+#define LM3633_CTRL_G_BRT		0x48
+#define LM3633_CTRL_H_BRT		0x49
+
+#define LM3633_SW_RESET		BIT(0)
+
+#define LM3633_CTRL_A_EN	BIT(0)
+#define LM3633_CTRL_B_EN	BIT(1)
+#define LM3633_CTRL_C_EN	BIT(2)
+#define LM3633_CTRL_D_EN	BIT(3)
+#define LM3633_CTRL_E_EN	BIT(4)
+#define LM3633_CTRL_F_EN	BIT(5)
+#define LM3633_CTRL_G_EN	BIT(6)
+#define LM3633_CTRL_H_EN	BIT(7)
+
+#define LM3633_MAX_HVLED_STRINGS	3
+#define LM3633_MAX_LVLED_STRINGS	6
+
+#define LM3633_CONTROL_A	0
+#define LM3633_CONTROL_B	1
+#define LM3633_CONTROL_C	2
+#define LM3633_CONTROL_D	3
+#define LM3633_CONTROL_E	4
+#define LM3633_CONTROL_F	5
+#define LM3633_CONTROL_G	6
+#define LM3633_CONTROL_H	7
+
+#define LM3633_MAX_CONTROL_BANKS 8
+
+#define LM3633_LED_ASSIGNMENT	1
+
+#define LM3633_CTRL_F_EN_MASK	0x07
+#define LM3633_CTRL_EN_OFFSET	2
+
+/**
+ * struct lm3633_led -
+ * @hvled_strings: Array of high voltage LED strings associated to control bank
+ * @lvled_strings: Array of low voltage LED strings associated to a control bank
+ * @label: LED label
+ * @led_dev: LED class device
+ * @priv: Pointer to the device struct
+ * @lmu_data: Register and setting values for common code
+ * @control_bank: Control bank the LED is associated to
+ */
+struct lm3633_led {
+	u32 hvled_strings[LM3633_MAX_HVLED_STRINGS];
+	u32 lvled_strings[LM3633_MAX_LVLED_STRINGS];
+	char label[LED_MAX_NAME_SIZE];
+	struct led_classdev led_dev;
+	struct lm3633 *priv;
+	struct ti_lmu_bank lmu_data;
+	int control_bank;
+};
+
+/**
+ * struct lm3633 -
+ * @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 lm3633 {
+	struct gpio_desc *enable_gpio;
+	struct regulator *regulator;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct device *dev;
+	struct mutex lock;
+	struct lm3633_led leds[];
+};
+
+static const struct reg_default lm3633_reg_defs[] = {
+	{LM3633_HVLED_OUTPUT_CONFIG, 0x6},
+	{LM3633_LVLED_OUTPUT_CONFIG, 0x36},
+	{LM3633_CTRL_A_RAMP, 0x0},
+	{LM3633_CTRL_B_RAMP, 0x0},
+	{LM3633_CTRL_A_B_RT_RAMP, 0x0},
+	{LM3633_CTRL_A_B_RAMP_CFG, 0x0},
+	{LM3633_CTRL_A_B_BRT_CFG, 0x0},
+	{LM3633_CTRL_A_FS_CURR_CFG, 0x13},
+	{LM3633_CTRL_B_FS_CURR_CFG, 0x13},
+	{LM3633_PWM_CFG, 0xc},
+	{LM3633_CTRL_A_BRT_LSB, 0x0},
+	{LM3633_CTRL_A_BRT_MSB, 0x0},
+	{LM3633_CTRL_B_BRT_LSB, 0x0},
+	{LM3633_CTRL_B_BRT_MSB, 0x0},
+	{LM3633_CTRL_ENABLE, 0x0},
+};
+
+static const struct regmap_config lm3633_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = LM3633_CTRL_H_BRT,
+	.reg_defaults = lm3633_reg_defs,
+	.num_reg_defaults = ARRAY_SIZE(lm3633_reg_defs),
+	.cache_type = REGCACHE_FLAT,
+};
+
+static int lm3633_brightness_set(struct led_classdev *led_cdev,
+				enum led_brightness brt_val)
+{
+	struct lm3633_led *led = container_of(led_cdev, struct lm3633_led,
+					      led_dev);
+	int ctrl_en_val;
+	int ret;
+
+	mutex_lock(&led->priv->lock);
+
+	switch (led->control_bank) {
+	case LM3633_CONTROL_A:
+		ctrl_en_val = LM3633_CTRL_A_EN;
+		break;
+	case LM3633_CONTROL_B:
+		ctrl_en_val = LM3633_CTRL_B_EN;
+		break;
+	case LM3633_CONTROL_C:
+		ctrl_en_val = LM3633_CTRL_C_EN;
+		break;
+	case LM3633_CONTROL_D:
+		ctrl_en_val = LM3633_CTRL_D_EN;
+		break;
+	case LM3633_CONTROL_E:
+		ctrl_en_val = LM3633_CTRL_E_EN;
+		break;
+	case LM3633_CONTROL_F:
+		ctrl_en_val = LM3633_CTRL_F_EN;
+		break;
+	case LM3633_CONTROL_G:
+		ctrl_en_val = LM3633_CTRL_G_EN;
+		break;
+	case LM3633_CONTROL_H:
+		ctrl_en_val = LM3633_CTRL_H_EN;
+		break;
+	default:
+		dev_err(&led->priv->client->dev, "Cannot write brightness\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (brt_val == LED_OFF)
+		ret = regmap_update_bits(led->priv->regmap, LM3633_CTRL_ENABLE,
+					 ctrl_en_val, ~ctrl_en_val);
+	else
+		ret = regmap_update_bits(led->priv->regmap, LM3633_CTRL_ENABLE,
+					 ctrl_en_val, ctrl_en_val);
+
+	ret = ti_lmu_common_set_brightness(&led->lmu_data, brt_val);
+	if (ret)
+		dev_err(&led->priv->client->dev, "Cannot write brightness\n");
+out:
+	mutex_unlock(&led->priv->lock);
+	return ret;
+}
+
+static void lm3633_set_control_bank_regs(struct lm3633_led *led)
+{
+	switch (led->control_bank) {
+	case LM3633_CONTROL_A:
+		led->lmu_data.msb_brightness_reg = LM3633_CTRL_A_BRT_MSB;
+		led->lmu_data.lsb_brightness_reg = LM3633_CTRL_A_BRT_LSB;
+		led->lmu_data.runtime_ramp_reg = LM3633_CTRL_A_RAMP;
+	case LM3633_CONTROL_B:
+		led->lmu_data.msb_brightness_reg = LM3633_CTRL_B_BRT_MSB;
+		led->lmu_data.lsb_brightness_reg = LM3633_CTRL_B_BRT_LSB;
+		led->lmu_data.runtime_ramp_reg = LM3633_CTRL_B_RAMP;
+		break;
+	case LM3633_CONTROL_C:
+		led->lmu_data.msb_brightness_reg = LM3633_CTRL_C_BRT;
+		led->lmu_data.runtime_ramp_reg = LM3633_CTRL_C_E_RT_RAMP;
+		break;
+	case LM3633_CONTROL_D:
+		led->lmu_data.msb_brightness_reg = LM3633_CTRL_D_BRT;
+		led->lmu_data.runtime_ramp_reg = LM3633_CTRL_C_E_RT_RAMP;
+		break;
+	case LM3633_CONTROL_E:
+		led->lmu_data.msb_brightness_reg = LM3633_CTRL_E_BRT;
+		led->lmu_data.runtime_ramp_reg = LM3633_CTRL_C_E_RT_RAMP;
+		break;
+	case LM3633_CONTROL_F:
+		led->lmu_data.msb_brightness_reg = LM3633_CTRL_F_BRT;
+		led->lmu_data.runtime_ramp_reg = LM3633_CTRL_F_H_RT_RAMP;
+		break;
+	case LM3633_CONTROL_G:
+		led->lmu_data.msb_brightness_reg = LM3633_CTRL_G_BRT;
+		led->lmu_data.runtime_ramp_reg = LM3633_CTRL_F_H_RT_RAMP;
+		break;
+	case LM3633_CONTROL_H:
+		led->lmu_data.msb_brightness_reg = LM3633_CTRL_H_BRT;
+		led->lmu_data.runtime_ramp_reg = LM3633_CTRL_F_H_RT_RAMP;
+		break;
+	default:
+		dev_err(&led->priv->client->dev,
+			"Control bank is out of bounds\n");
+	}
+}
+
+static int lm3633_set_control_bank(struct lm3633 *priv)
+{
+	u8 control_bank_config = 0;
+	struct lm3633_led *led;
+	int ret, i;
+
+	led = &priv->leds[0];
+	if (led->control_bank == LM3633_CONTROL_A) {
+		lm3633_set_control_bank_regs(led);
+		led = &priv->leds[1];
+	}
+
+	if (led->control_bank >= LM3633_CONTROL_C)
+		return 0;
+
+	lm3633_set_control_bank_regs(led);
+	for (i = 0; i < LM3633_MAX_HVLED_STRINGS; i++)
+		if (led->hvled_strings[i] == LM3633_LED_ASSIGNMENT)
+			control_bank_config |= 1 << i;
+
+	ret = regmap_write(priv->regmap, LM3633_HVLED_OUTPUT_CONFIG,
+			   control_bank_config);
+	if (ret)
+		dev_err(&priv->client->dev, "Cannot write OUTPUT config\n");
+
+	return ret;
+}
+
+static int lm3633_set_lvled_control_bank(struct lm3633 *priv)
+{
+	u8 control_bank_config = 0;
+	struct lm3633_led *led;
+	int ret, i;
+
+	for (i = 0; i <= LM3633_MAX_CONTROL_BANKS; i++) {
+		led = &priv->leds[i];
+		if (led) {
+			if (led->control_bank < LM3633_CONTROL_C)
+				continue;
+
+			if (led->lvled_strings[0]) {
+				if (led->control_bank == LM3633_CONTROL_C)
+					control_bank_config = 0x0;
+				else if (led->control_bank == LM3633_CONTROL_F)
+					control_bank_config &= LM3633_CTRL_F_EN_MASK;
+				else
+					control_bank_config |= 1 << (led->control_bank - LM3633_CTRL_EN_OFFSET);
+			}
+		} else
+			continue;
+
+		lm3633_set_control_bank_regs(led);
+	}
+
+	ret = regmap_write(priv->regmap, LM3633_LVLED_OUTPUT_CONFIG,
+			   control_bank_config);
+	if (ret)
+		dev_err(&priv->client->dev, "Cannot write OUTPUT config\n");
+
+	return ret;
+}
+
+static int lm3633_init(struct lm3633 *priv)
+{
+	struct lm3633_led *led;
+	int i, ret;
+
+	if (priv->enable_gpio) {
+		gpiod_direction_output(priv->enable_gpio, 1);
+	} else {
+		ret = regmap_write(priv->regmap, LM3633_RESET, LM3633_SW_RESET);
+		if (ret) {
+			dev_err(&priv->client->dev,
+				"Cannot reset the device\n");
+			goto out;
+		}
+	}
+
+	ret = regmap_write(priv->regmap, LM3633_CTRL_ENABLE, 0x0);
+	if (ret) {
+		dev_err(&priv->client->dev, "Cannot write ctrl enable\n");
+		goto out;
+	}
+
+	ret = lm3633_set_control_bank(priv);
+	if (ret)
+		dev_err(&priv->client->dev, "Setting the CRTL bank failed\n");
+
+	ret = lm3633_set_lvled_control_bank(priv);
+	if (ret)
+		dev_err(&priv->client->dev,
+			"Setting the lvled CRTL bank failed\n");
+
+	for (i = 0; i < LM3633_MAX_CONTROL_BANKS; i++) {
+		led = &priv->leds[i];
+		if (led->lmu_data.runtime_ramp_reg) {
+			ti_lmu_common_set_ramp(&led->lmu_data);
+			if (ret)
+				dev_err(&priv->client->dev,
+					"Setting the ramp rate failed\n");
+		}
+	}
+out:
+	return ret;
+}
+
+static int lm3633_parse_hvled_sources(struct fwnode_handle *child,
+			      struct lm3633_led *led)
+{
+	struct lm3633 *priv = led->priv;
+	int ret;
+
+	ret = fwnode_property_read_u32_array(child, "led-sources",
+			    led->hvled_strings,
+			    LM3633_MAX_HVLED_STRINGS);
+
+	if (ret)
+		dev_err(&priv->client->dev, "Cannot write OUTPUT config\n");
+
+	return ret;
+}
+
+static int lm3633_parse_lvled_sources(struct fwnode_handle *child,
+			      struct lm3633_led *led)
+{
+	struct lm3633 *priv = led->priv;
+	int ret;
+
+	ret = fwnode_property_read_u32_array(child, "led-sources",
+			    led->lvled_strings, 1);
+	if (ret)
+		dev_err(&priv->client->dev, "Cannot write OUTPUT config\n");
+
+	led->lmu_data.max_brightness = MAX_BRIGHTNESS_8BIT;
+
+	return 0;
+}
+
+static int lm3633_probe_dt(struct lm3633 *priv)
+{
+	struct fwnode_handle *child = NULL;
+	struct lm3633_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 > LM3633_MAX_CONTROL_BANKS) {
+			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;
+		led->lmu_data.bank_id = control_bank;
+		led->lmu_data.regmap = priv->regmap;
+		led->lmu_data.enable_reg = LM3633_CTRL_ENABLE;
+
+		if (control_bank > LM3633_CONTROL_B)
+			lm3633_parse_lvled_sources(child, led);
+		else
+			lm3633_parse_hvled_sources(child, led);
+
+		ret = ti_lmu_common_get_ramp_params(&priv->client->dev,
+						    child, &led->lmu_data);
+		if (ret)
+			dev_warn(&priv->client->dev,
+				 "runtime-ramp properties missing\n");
+
+		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 = lm3633_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 lm3633_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct lm3633 *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, &lm3633_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 = lm3633_probe_dt(led);
+	if (ret)
+		return ret;
+
+	return lm3633_init(led);
+}
+
+static int lm3633_remove(struct i2c_client *client)
+{
+	struct lm3633 *led = i2c_get_clientdata(client);
+	int ret;
+
+	ret = regmap_write(led->regmap, LM3633_CTRL_ENABLE, 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 lm3633_id[] = {
+	{ "lm3633", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lm3633_id);
+
+static const struct of_device_id of_lm3633_leds_match[] = {
+	{ .compatible = "ti,lm3633", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_lm3633_leds_match);
+
+static struct i2c_driver lm3633_driver = {
+	.driver = {
+		.name	= "lm3633",
+		.of_match_table = of_lm3633_leds_match,
+	},
+	.probe		= lm3633_probe,
+	.remove		= lm3633_remove,
+	.id_table	= lm3633_id,
+};
+module_i2c_driver(lm3633_driver);
+
+MODULE_DESCRIPTION("Texas Instruments LM3633 LED driver");
+MODULE_AUTHOR("Dan Murphy <dmurphy@ti.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.19.0


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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-23 17:06 ` [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697 Dan Murphy
@ 2018-10-24  9:04   ` Pavel Machek
  2018-10-24 12:07     ` Dan Murphy
  2018-10-24 14:49   ` Rob Herring
  1 sibling, 1 reply; 30+ messages in thread
From: Pavel Machek @ 2018-10-24  9:04 UTC (permalink / raw)
  To: Dan Murphy
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds,
	lee.jones, tony

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

Hi!

> The LM3697 is a single function LED driver. The single function LED
> driver needs to reside in the LED directory as a dedicated LED driver
> and not as a MFD device.  The device does have common brightness and ramp

So it is single function LED driver. That does not mean it can not
share bindings with the rest. Where the bindings live is not imporant.

> reside in the Documentation/devicetree/bindings/leds directory and follow the
> current LED and general bindings guidelines.

What you forgot to tell us in the changelog:

> +Optional child properties:
> +	- runtime-ramp-up-msec: Current ramping from one brightness level to
> +				the a higher brightness level.
> +				Range from 2048 us - 117.44 s

The other binding uses "ramp-up-msec". Tell us why you are changing this, or
better don't change things needlessly.

We don't want to be using "runtime-ramp-up-msec" for one device and
"ramp-up-msec" for the other.

I'm not sure what other changes you did, and changelog does not tell
me.

> --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> @@ -9,7 +9,6 @@ TI LMU driver supports lighting devices below.
>    LM3632       Backlight and regulator
>    LM3633       Backlight, LED and fault monitor
>    LM3695       Backlight
> -  LM3697       Backlight and fault monitor
>  
>  Required properties:
>    - compatible: Should be one of:

NAK. You can use existing binding.

									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] 30+ messages in thread

* Re: [PATCH v4 1/7] leds: add TI LMU backlight driver
  2018-10-23 17:06 [PATCH v4 1/7] leds: add TI LMU backlight driver Dan Murphy
                   ` (5 preceding siblings ...)
  2018-10-23 17:06 ` [PATCH v4 7/7] leds: lm3633: Introduce the lm3633 driver Dan Murphy
@ 2018-10-24  9:14 ` Pavel Machek
  2018-10-24 12:27   ` Dan Murphy
  6 siblings, 1 reply; 30+ messages in thread
From: Pavel Machek @ 2018-10-24  9:14 UTC (permalink / raw)
  To: Dan Murphy
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds,
	lee.jones, tony, Milo Kim, Sebastian Reichel

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

On Tue 2018-10-23 12:06:17, Dan Murphy wrote:
> From: Pavel Machek <pavel@ucw.cz>
> 
> This adds backlight support for the following TI LMU
> chips: LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
> 
> It controls LEDs on Droid 4
> smartphone, including keyboard and screen backlights.
> 
> Signed-off-by: Milo Kim <milo.kim@ti.com>
> [add LED subsystem support for keyboard backlight and rework DT
> binding according to Rob Herrings feedback]
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> [remove backlight subsystem support for now]
> Signed-off-by: Pavel Machek <pavel@ucw.cz>

This is not correct way to sign off a patch.

What is worse, this is very different from patch I submitted; this
misses imporant parts while my patch was complete.

I already submitted better patch for both the driver and device tree
bindings. Can we go back and apply that?

Then you can add your changes on top of that, if they actually make
the situation better.

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

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

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

* Re: [PATCH v4 5/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3633
  2018-10-23 17:06 ` [PATCH v4 5/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3633 Dan Murphy
@ 2018-10-24  9:23   ` Pavel Machek
  2018-10-24 14:35     ` Rob Herring
  2018-10-25 18:01     ` Dan Murphy
  0 siblings, 2 replies; 30+ messages in thread
From: Pavel Machek @ 2018-10-24  9:23 UTC (permalink / raw)
  To: Dan Murphy
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds,
	lee.jones, tony

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

On Tue 2018-10-23 12:06:21, Dan Murphy wrote:
> The LM3633 is a single function LED driver. The single function LED
> driver needs to reside in the LED directory as a dedicated LED driver
> and not as a MFD device.  The device does have common brightness and ramp
> features and those can be accomodated by a TI LMU framework.
> 
> The LM3633 dt binding needs to be moved from the ti-lmu.txt and a dedicated
> LED dt binding needs to be added.  The new LM3633 LED dt binding will then
> reside in the Documentation/devicetree/bindings/leds directory and follow the
> current LED and general bindings guidelines.

What?

>  .../devicetree/bindings/leds/leds-lm3633.txt  | 102 ++++++++++++++++++
>  .../devicetree/bindings/mfd/ti-lmu.txt        |  48 ---------
>  2 files changed, 102 insertions(+), 48 deletions(-)
>  create mode 100644
>  Documentation/devicetree/bindings/leds/leds-lm3633.txt

> index 920f910be4e9..573e88578d3d 100644
> --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> @@ -7,7 +7,6 @@ TI LMU driver supports lighting devices below.
>    LM3532       Backlight
>    LM3631       Backlight and regulator
>    LM3632       Backlight and regulator
> -  LM3633       Backlight, LED and fault monitor
>    LM3695       Backlight

Are you seriously proposing to take one binding and split it into 6
copy&pasted ones?

That's not the way we do development. NAK.

We don't want to have copy & pasted code. We also don't want to have
copy & pasted bindings. Nor changelogs, for that matter.

Thank you,
								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] 30+ messages in thread

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-24  9:04   ` Pavel Machek
@ 2018-10-24 12:07     ` Dan Murphy
  2018-10-24 13:43       ` Pavel Machek
  2018-10-24 14:54       ` Rob Herring
  0 siblings, 2 replies; 30+ messages in thread
From: Dan Murphy @ 2018-10-24 12:07 UTC (permalink / raw)
  To: Pavel Machek
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds,
	lee.jones, tony

Pavel

On 10/24/2018 04:04 AM, Pavel Machek wrote:
> Hi!
> 
>> The LM3697 is a single function LED driver. The single function LED
>> driver needs to reside in the LED directory as a dedicated LED driver
>> and not as a MFD device.  The device does have common brightness and ramp
> 
> So it is single function LED driver. That does not mean it can not
> share bindings with the rest. Where the bindings live is not imporant.
> 

It can share bindings that are correctly done, not ones that are incomplete and incorrect.

Where bindings live is important to new Linux kernel developers and product 
developers looking for the proper documentation on the H/W bindings.

>> reside in the Documentation/devicetree/bindings/leds directory and follow the
>> current LED and general bindings guidelines.
> 
> What you forgot to tell us in the changelog:

I can add this to the changelog.

> 
>> +Optional child properties:
>> +	- runtime-ramp-up-msec: Current ramping from one brightness level to
>> +				the a higher brightness level.
>> +				Range from 2048 us - 117.44 s
> 
> The other binding uses "ramp-up-msec". Tell us why you are changing this, or
> better don't change things needlessly.
> 
> We don't want to be using "runtime-ramp-up-msec" for one device and
> "ramp-up-msec" for the other.

This is another example of how the original bindings were incorrect and misleading.

The LM3697 have 2 ramp implementations that can be used.

Startup/Shutdown ramp and Runtime Ramp.  Same Ramp rates different registers and
different end user experience.

So having a single node call ramp-up-msec is misleading and it does not
indicate what the H/W will do.

> 
> I'm not sure what other changes you did, and changelog does not tell
> me.
> 
>> --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
>> +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
>> @@ -9,7 +9,6 @@ TI LMU driver supports lighting devices below.
>>    LM3632       Backlight and regulator
>>    LM3633       Backlight, LED and fault monitor
>>    LM3695       Backlight
>> -  LM3697       Backlight and fault monitor
>>  
>>  Required properties:
>>    - compatible: Should be one of:
> 
> NAK. You can use existing binding.

Thank you for the consistency

Dan

> 
> 									Pavel
> 


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

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

* Re: [PATCH v4 1/7] leds: add TI LMU backlight driver
  2018-10-24  9:14 ` [PATCH v4 1/7] leds: add TI LMU backlight driver Pavel Machek
@ 2018-10-24 12:27   ` Dan Murphy
  2018-10-24 13:17     ` Pavel Machek
  0 siblings, 1 reply; 30+ messages in thread
From: Dan Murphy @ 2018-10-24 12:27 UTC (permalink / raw)
  To: Pavel Machek
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds,
	lee.jones, tony, Sebastian Reichel

Pavel

On 10/24/2018 04:14 AM, Pavel Machek wrote:
> On Tue 2018-10-23 12:06:17, Dan Murphy wrote:
>> From: Pavel Machek <pavel@ucw.cz>
>>
>> This adds backlight support for the following TI LMU
>> chips: LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
>>
>> It controls LEDs on Droid 4
>> smartphone, including keyboard and screen backlights.
>>
>> Signed-off-by: Milo Kim <milo.kim@ti.com>
>> [add LED subsystem support for keyboard backlight and rework DT
>> binding according to Rob Herrings feedback]
>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
>> [remove backlight subsystem support for now]
>> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> This is not correct way to sign off a patch.
> 
> What is worse, this is very different from patch I submitted; this
> misses imporant parts while my patch was complete.
> 
> I already submitted better patch for both the driver and device tree
> bindings. Can we go back and apply that?
> 
> Then you can add your changes on top of that, if they actually make
> the situation better.

Thanks for this.  I wanted to make sure not to loose any authorship or credit
in the commit.  Actually removing Milo might help stop getting the bounce back
from the mail daemon.

I was using the RFC patch as my base https://lore.kernel.org/patchwork/patch/978995/

Which contains this sign off.

Dan

> 
> Thanks,
> 								Pavel
> 


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

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

* Re: [PATCH v4 1/7] leds: add TI LMU backlight driver
  2018-10-24 12:27   ` Dan Murphy
@ 2018-10-24 13:17     ` Pavel Machek
  0 siblings, 0 replies; 30+ messages in thread
From: Pavel Machek @ 2018-10-24 13:17 UTC (permalink / raw)
  To: Dan Murphy
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds,
	lee.jones, tony, Sebastian Reichel

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

On Wed 2018-10-24 07:27:57, Dan Murphy wrote:
> Pavel
> 
> On 10/24/2018 04:14 AM, Pavel Machek wrote:
> > On Tue 2018-10-23 12:06:17, Dan Murphy wrote:
> >> From: Pavel Machek <pavel@ucw.cz>
> >>
> >> This adds backlight support for the following TI LMU
> >> chips: LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697.
> >>
> >> It controls LEDs on Droid 4
> >> smartphone, including keyboard and screen backlights.
> >>
> >> Signed-off-by: Milo Kim <milo.kim@ti.com>
> >> [add LED subsystem support for keyboard backlight and rework DT
> >> binding according to Rob Herrings feedback]
> >> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> >> [remove backlight subsystem support for now]
> >> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> > 
> > This is not correct way to sign off a patch.
> > 
> > What is worse, this is very different from patch I submitted; this
> > misses imporant parts while my patch was complete.
> > 
> > I already submitted better patch for both the driver and device tree
> > bindings. Can we go back and apply that?
> > 
> > Then you can add your changes on top of that, if they actually make
> > the situation better.
> 
> Thanks for this.  I wanted to make sure not to loose any authorship or credit
> in the commit.  Actually removing Milo might help stop getting the bounce back
> from the mail daemon.
> 
> I was using the RFC patch as my base https://lore.kernel.org/patchwork/patch/978995/
> 
> Which contains this sign off.

Yeah, but you need to append your sign-off to the end, preferably
prepending note saying what you modified. [I'm ok with the original
patch, but I'm not ok with your version].

From: Pavel Machek <pavel@ucw.cz>

Is not correct. Original patch is from Milo, if you want to add
explicit From:, I guess it should be his.

									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] 30+ messages in thread

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-24 12:07     ` Dan Murphy
@ 2018-10-24 13:43       ` Pavel Machek
  2018-10-24 14:54       ` Rob Herring
  1 sibling, 0 replies; 30+ messages in thread
From: Pavel Machek @ 2018-10-24 13:43 UTC (permalink / raw)
  To: Dan Murphy
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds,
	lee.jones, tony

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

Hi!

> >> The LM3697 is a single function LED driver. The single function LED
> >> driver needs to reside in the LED directory as a dedicated LED driver
> >> and not as a MFD device.  The device does have common brightness and ramp
> > 
> > So it is single function LED driver. That does not mean it can not
> > share bindings with the rest. Where the bindings live is not imporant.
> 
> It can share bindings that are correctly done, not ones that are incomplete and incorrect.

If you see wrong binding you are welcome to fix it. If it is in wrong
place, you can move it around.

You are not welcome to create 2 "fixed" versions, while leaving
original "buggy" version in tree!

...but that's what your series does. Better changelogs will _not_ help.

> Where bindings live is important to new Linux kernel developers and product 
> developers looking for the proper documentation on the H/W bindings.

We have talked about this before.

> >> @@ -9,7 +9,6 @@ TI LMU driver supports lighting devices below.
> >>    LM3632       Backlight and regulator
> >>    LM3633       Backlight, LED and fault monitor
> >>    LM3695       Backlight
> >> -  LM3697       Backlight and fault monitor
> >>  
> >>  Required properties:
> >>    - compatible: Should be one of:
> > 
> > NAK. You can use existing binding.
> 
> Thank you for the consistency

You are wasting your time. What is worse, you are wasting my time,
too, and time of people on the list.

									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] 30+ messages in thread

* Re: [PATCH v4 5/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3633
  2018-10-24  9:23   ` Pavel Machek
@ 2018-10-24 14:35     ` Rob Herring
  2018-10-24 18:38       ` Pavel Machek
  2018-10-25 18:01     ` Dan Murphy
  1 sibling, 1 reply; 30+ messages in thread
From: Rob Herring @ 2018-10-24 14:35 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Dan Murphy, jacek.anaszewski, devicetree, linux-kernel,
	linux-leds, lee.jones, tony

On Wed, Oct 24, 2018 at 11:23:28AM +0200, Pavel Machek wrote:
> On Tue 2018-10-23 12:06:21, Dan Murphy wrote:
> > The LM3633 is a single function LED driver. The single function LED
> > driver needs to reside in the LED directory as a dedicated LED driver
> > and not as a MFD device.  The device does have common brightness and ramp
> > features and those can be accomodated by a TI LMU framework.
> > 
> > The LM3633 dt binding needs to be moved from the ti-lmu.txt and a dedicated
> > LED dt binding needs to be added.  The new LM3633 LED dt binding will then
> > reside in the Documentation/devicetree/bindings/leds directory and follow the
> > current LED and general bindings guidelines.
> 
> What?
> 
> >  .../devicetree/bindings/leds/leds-lm3633.txt  | 102 ++++++++++++++++++
> >  .../devicetree/bindings/mfd/ti-lmu.txt        |  48 ---------
> >  2 files changed, 102 insertions(+), 48 deletions(-)
> >  create mode 100644
> >  Documentation/devicetree/bindings/leds/leds-lm3633.txt
> 
> > index 920f910be4e9..573e88578d3d 100644
> > --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > @@ -7,7 +7,6 @@ TI LMU driver supports lighting devices below.
> >    LM3532       Backlight
> >    LM3631       Backlight and regulator
> >    LM3632       Backlight and regulator
> > -  LM3633       Backlight, LED and fault monitor
> >    LM3695       Backlight
> 
> Are you seriously proposing to take one binding and split it into 6
> copy&pasted ones?
> 
> That's not the way we do development. NAK.
> 
> We don't want to have copy & pasted code. We also don't want to have
> copy & pasted bindings. Nor changelogs, for that matter.

I looked at the LM3633 and LM3632 datasheets. They look quite different 
to me and should be separate IMO. Just looking at different LED 
functions and GPIO control lines is enough to make that determination. 
The LM3697 looks like a subset of LM3633 at least at a schematic 
diagram level, so maybe those can be shared.

While we could litter the binding with conditions on properties 
depending on specific compatible strings (such as which GPIO properties 
apply to which compatible), that is going to be problematic down the 
line when we convert to json-schema[1].

Rob

[1] https://lkml.org/lkml/2018/10/5/883

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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-23 17:06 ` [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697 Dan Murphy
  2018-10-24  9:04   ` Pavel Machek
@ 2018-10-24 14:49   ` Rob Herring
  2018-10-25 17:56     ` Dan Murphy
  1 sibling, 1 reply; 30+ messages in thread
From: Rob Herring @ 2018-10-24 14:49 UTC (permalink / raw)
  To: Dan Murphy
  Cc: jacek.anaszewski, pavel, devicetree, linux-kernel, linux-leds,
	lee.jones, tony

On Tue, Oct 23, 2018 at 12:06:18PM -0500, Dan Murphy wrote:
> The LM3697 is a single function LED driver. The single function LED
> driver needs to reside in the LED directory as a dedicated LED driver
> and not as a MFD device.  The device does have common brightness and ramp
> features and those can be accomodated by a TI LMU framework.
> 
> The LM3697 dt binding needs to be moved from the ti-lmu.txt and a dedicated
> LED dt binding needs to be added.  The new LM3697 LED dt binding will then
> reside in the Documentation/devicetree/bindings/leds directory and follow the
> current LED and general bindings guidelines.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v4 - Squashed removal and addition of the dt bindings into a single patch - https://lore.kernel.org/patchwork/patch/998703/

Still not really the sequence I'd like to see and what I think would 
help the discussion move along.

Patch 1: move all the devices out of ti-lmu.txt into the grouping which 
makes sense. IOW, no functional changes. Probably only strict 
sub/supersets of each other should be shared.

Patch 2-N: Make binding changes. Then we discuss things like ramp time 
properties separately from binding structure.

> 
>  .../devicetree/bindings/leds/leds-lm3697.txt  | 98 +++++++++++++++++++
>  .../devicetree/bindings/mfd/ti-lmu.txt        | 26 +----
>  2 files changed, 99 insertions(+), 25 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt

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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-24 12:07     ` Dan Murphy
  2018-10-24 13:43       ` Pavel Machek
@ 2018-10-24 14:54       ` Rob Herring
  2018-10-25 18:07         ` Dan Murphy
  1 sibling, 1 reply; 30+ messages in thread
From: Rob Herring @ 2018-10-24 14:54 UTC (permalink / raw)
  To: Dan Murphy
  Cc: Pavel Machek, jacek.anaszewski, devicetree, linux-kernel,
	linux-leds, lee.jones, tony

On Wed, Oct 24, 2018 at 07:07:57AM -0500, Dan Murphy wrote:
> Pavel
> 
> On 10/24/2018 04:04 AM, Pavel Machek wrote:
> > Hi!
> > 
> >> The LM3697 is a single function LED driver. The single function LED
> >> driver needs to reside in the LED directory as a dedicated LED driver
> >> and not as a MFD device.  The device does have common brightness and ramp
> > 
> > So it is single function LED driver. That does not mean it can not
> > share bindings with the rest. Where the bindings live is not imporant.
> > 
> 
> It can share bindings that are correctly done, not ones that are incomplete and incorrect.
> 
> Where bindings live is important to new Linux kernel developers and product 
> developers looking for the proper documentation on the H/W bindings.
> 
> >> reside in the Documentation/devicetree/bindings/leds directory and follow the
> >> current LED and general bindings guidelines.
> > 
> > What you forgot to tell us in the changelog:
> 
> I can add this to the changelog.
> 
> > 
> >> +Optional child properties:
> >> +	- runtime-ramp-up-msec: Current ramping from one brightness level to
> >> +				the a higher brightness level.
> >> +				Range from 2048 us - 117.44 s
> > 
> > The other binding uses "ramp-up-msec". Tell us why you are changing this, or
> > better don't change things needlessly.
> > 
> > We don't want to be using "runtime-ramp-up-msec" for one device and
> > "ramp-up-msec" for the other.
> 
> This is another example of how the original bindings were incorrect and misleading.
> 
> The LM3697 have 2 ramp implementations that can be used.
> 
> Startup/Shutdown ramp and Runtime Ramp.  Same Ramp rates different registers and
> different end user experience.
> 
> So having a single node call ramp-up-msec is misleading and it does not
> indicate what the H/W will do.

The existing ones aren't documented (present in the example is not 
documented). This seems like something that should be common rather than 
TI specific. Though it also seems more like something the user would 
want to control (i.e. sysfs) rather than fixed in DT.

Rob

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

* Re: [PATCH v4 5/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3633
  2018-10-24 14:35     ` Rob Herring
@ 2018-10-24 18:38       ` Pavel Machek
  2018-10-24 21:50         ` Rob Herring
  0 siblings, 1 reply; 30+ messages in thread
From: Pavel Machek @ 2018-10-24 18:38 UTC (permalink / raw)
  To: Rob Herring
  Cc: Dan Murphy, jacek.anaszewski, devicetree, linux-kernel,
	linux-leds, lee.jones, tony

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

On Wed 2018-10-24 09:35:06, Rob Herring wrote:
> On Wed, Oct 24, 2018 at 11:23:28AM +0200, Pavel Machek wrote:
> > On Tue 2018-10-23 12:06:21, Dan Murphy wrote:
> > > The LM3633 is a single function LED driver. The single function LED
> > > driver needs to reside in the LED directory as a dedicated LED driver
> > > and not as a MFD device.  The device does have common brightness and ramp
> > > features and those can be accomodated by a TI LMU framework.
> > > 
> > > The LM3633 dt binding needs to be moved from the ti-lmu.txt and a dedicated
> > > LED dt binding needs to be added.  The new LM3633 LED dt binding will then
> > > reside in the Documentation/devicetree/bindings/leds directory and follow the
> > > current LED and general bindings guidelines.
> > 
> > What?
> > 
> > >  .../devicetree/bindings/leds/leds-lm3633.txt  | 102 ++++++++++++++++++
> > >  .../devicetree/bindings/mfd/ti-lmu.txt        |  48 ---------
> > >  2 files changed, 102 insertions(+), 48 deletions(-)
> > >  create mode 100644
> > >  Documentation/devicetree/bindings/leds/leds-lm3633.txt
> > 
> > > index 920f910be4e9..573e88578d3d 100644
> > > --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > > +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > > @@ -7,7 +7,6 @@ TI LMU driver supports lighting devices below.
> > >    LM3532       Backlight
> > >    LM3631       Backlight and regulator
> > >    LM3632       Backlight and regulator
> > > -  LM3633       Backlight, LED and fault monitor
> > >    LM3695       Backlight
> > 
> > Are you seriously proposing to take one binding and split it into 6
> > copy&pasted ones?
> > 
> > That's not the way we do development. NAK.
> > 
> > We don't want to have copy & pasted code. We also don't want to have
> > copy & pasted bindings. Nor changelogs, for that matter.
> 
> I looked at the LM3633 and LM3632 datasheets. They look quite different 
> to me and should be separate IMO. Just looking at different LED 
> functions and GPIO control lines is enough to make that determination. 
> The LM3697 looks like a subset of LM3633 at least at a schematic 
> diagram level, so maybe those can be shared.

Well, they have blocks in common, and are currently handled by one
driver. Two .c files proposed here shared 80% code when I reviewed
previous version. Original merge documentation is:

https://groups.google.com/forum/#!msg/fa.linux.kernel/hWvxahP7INw/Y2EDZmjoAQAJ

TI LMU(Lighting Management Unit) driver supports lighting devices
below.
 
 	                 Enable pin  Backlights  HWMON  LEDs   Regulators
	                 ----------  ----------  -----  ----  ------------
		  LM3532       o           o         x     x        x
		  LM3631       o           o         x     x    5 regulators
		  LM3632       o           o         x     x    3 regulators
		  LM3633       o           o         o     o        x
		  LM3695       o           o         x     x        x
		  LM3697       o           o         o     x        x


I thought I understood that table, but maybe I'm confused. Anyway,
there seemed to be "enough" to share.

> While we could litter the binding with conditions on properties 
> depending on specific compatible strings (such as which GPIO properties 
> apply to which compatible), that is going to be problematic down the 
> line when we convert to json-schema[1].

Well, situation where different devices share common features /
function blocks is going to be somehow common. Not sure how to solve
it in json, maybe the properties can simply be marked optional?, but I
guess it will need solving somehow.

> [1] https://lkml.org/lkml/2018/10/5/883

								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] 30+ messages in thread

* Re: [PATCH v4 5/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3633
  2018-10-24 18:38       ` Pavel Machek
@ 2018-10-24 21:50         ` Rob Herring
  0 siblings, 0 replies; 30+ messages in thread
From: Rob Herring @ 2018-10-24 21:50 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Dan Murphy, jacek.anaszewski, devicetree, linux-kernel,
	linux-leds, lee.jones, tony

On Wed, Oct 24, 2018 at 08:38:35PM +0200, Pavel Machek wrote:
> On Wed 2018-10-24 09:35:06, Rob Herring wrote:
> > On Wed, Oct 24, 2018 at 11:23:28AM +0200, Pavel Machek wrote:
> > > On Tue 2018-10-23 12:06:21, Dan Murphy wrote:
> > > > The LM3633 is a single function LED driver. The single function LED
> > > > driver needs to reside in the LED directory as a dedicated LED driver
> > > > and not as a MFD device.  The device does have common brightness and ramp
> > > > features and those can be accomodated by a TI LMU framework.
> > > > 
> > > > The LM3633 dt binding needs to be moved from the ti-lmu.txt and a dedicated
> > > > LED dt binding needs to be added.  The new LM3633 LED dt binding will then
> > > > reside in the Documentation/devicetree/bindings/leds directory and follow the
> > > > current LED and general bindings guidelines.
> > > 
> > > What?
> > > 
> > > >  .../devicetree/bindings/leds/leds-lm3633.txt  | 102 ++++++++++++++++++
> > > >  .../devicetree/bindings/mfd/ti-lmu.txt        |  48 ---------
> > > >  2 files changed, 102 insertions(+), 48 deletions(-)
> > > >  create mode 100644
> > > >  Documentation/devicetree/bindings/leds/leds-lm3633.txt
> > > 
> > > > index 920f910be4e9..573e88578d3d 100644
> > > > --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > > > +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
> > > > @@ -7,7 +7,6 @@ TI LMU driver supports lighting devices below.
> > > >    LM3532       Backlight
> > > >    LM3631       Backlight and regulator
> > > >    LM3632       Backlight and regulator
> > > > -  LM3633       Backlight, LED and fault monitor
> > > >    LM3695       Backlight
> > > 
> > > Are you seriously proposing to take one binding and split it into 6
> > > copy&pasted ones?
> > > 
> > > That's not the way we do development. NAK.
> > > 
> > > We don't want to have copy & pasted code. We also don't want to have
> > > copy & pasted bindings. Nor changelogs, for that matter.
> > 
> > I looked at the LM3633 and LM3632 datasheets. They look quite different 
> > to me and should be separate IMO. Just looking at different LED 
> > functions and GPIO control lines is enough to make that determination. 
> > The LM3697 looks like a subset of LM3633 at least at a schematic 
> > diagram level, so maybe those can be shared.
> 
> Well, they have blocks in common, and are currently handled by one
> driver. Two .c files proposed here shared 80% code when I reviewed
> previous version. Original merge documentation is:
> 
> https://groups.google.com/forum/#!msg/fa.linux.kernel/hWvxahP7INw/Y2EDZmjoAQAJ
> 
> TI LMU(Lighting Management Unit) driver supports lighting devices
> below.
>  
>  	                 Enable pin  Backlights  HWMON  LEDs   Regulators
> 	                 ----------  ----------  -----  ----  ------------
> 		  LM3532       o           o         x     x        x
> 		  LM3631       o           o         x     x    5 regulators
> 		  LM3632       o           o         x     x    3 regulators
> 		  LM3633       o           o         o     o        x
> 		  LM3695       o           o         x     x        x
> 		  LM3697       o           o         o     x        x
> 
> 
> I thought I understood that table, but maybe I'm confused. Anyway,
> there seemed to be "enough" to share.

I don't think all the backlight blocks are the same. Some don't have 2 
ramp settings is what I looked at. I only skimmed thru the datasheets 
though. 
 
> > While we could litter the binding with conditions on properties 
> > depending on specific compatible strings (such as which GPIO properties 
> > apply to which compatible), that is going to be problematic down the 
> > line when we convert to json-schema[1].
> 
> Well, situation where different devices share common features /
> function blocks is going to be somehow common. Not sure how to solve
> it in json, maybe the properties can simply be marked optional?, but I
> guess it will need solving somehow.

There are ways to deal with some of that, but at some point when 
every property has different constraints based on compatibles at some 
point it is easier to just split them. We don't try to have all LED 
bindings in one doc even if they only use standard properties.

Rob

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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-24 14:49   ` Rob Herring
@ 2018-10-25 17:56     ` Dan Murphy
  0 siblings, 0 replies; 30+ messages in thread
From: Dan Murphy @ 2018-10-25 17:56 UTC (permalink / raw)
  To: Rob Herring
  Cc: jacek.anaszewski, pavel, devicetree, linux-kernel, linux-leds,
	lee.jones, tony


Rob

Thanks

On 10/24/2018 09:49 AM, Rob Herring wrote:
> On Tue, Oct 23, 2018 at 12:06:18PM -0500, Dan Murphy wrote:
>> The LM3697 is a single function LED driver. The single function LED
>> driver needs to reside in the LED directory as a dedicated LED driver
>> and not as a MFD device.  The device does have common brightness and ramp
>> features and those can be accomodated by a TI LMU framework.
>>
>> The LM3697 dt binding needs to be moved from the ti-lmu.txt and a dedicated
>> LED dt binding needs to be added.  The new LM3697 LED dt binding will then
>> reside in the Documentation/devicetree/bindings/leds directory and follow the
>> current LED and general bindings guidelines.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>> ---
>>
>> v4 - Squashed removal and addition of the dt bindings into a single patch - https://lore.kernel.org/patchwork/patch/998703/
> 
> Still not really the sequence I'd like to see and what I think would 
> help the discussion move along.
> 
> Patch 1: move all the devices out of ti-lmu.txt into the grouping which 
> makes sense. IOW, no functional changes. Probably only strict 
> sub/supersets of each other should be shared.
> 
> Patch 2-N: Make binding changes. Then we discuss things like ramp time 
> properties separately from binding structure.
> 

OK I will re-work the patchset.

Dan

>>
>>  .../devicetree/bindings/leds/leds-lm3697.txt  | 98 +++++++++++++++++++
>>  .../devicetree/bindings/mfd/ti-lmu.txt        | 26 +----
>>  2 files changed, 99 insertions(+), 25 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt


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

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

* Re: [PATCH v4 5/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3633
  2018-10-24  9:23   ` Pavel Machek
  2018-10-24 14:35     ` Rob Herring
@ 2018-10-25 18:01     ` Dan Murphy
  1 sibling, 0 replies; 30+ messages in thread
From: Dan Murphy @ 2018-10-25 18:01 UTC (permalink / raw)
  To: Pavel Machek
  Cc: robh+dt, jacek.anaszewski, devicetree, linux-kernel, linux-leds,
	lee.jones, tony

Pavel

On 10/24/2018 04:23 AM, Pavel Machek wrote:
> On Tue 2018-10-23 12:06:21, Dan Murphy wrote:
>> The LM3633 is a single function LED driver. The single function LED
>> driver needs to reside in the LED directory as a dedicated LED driver
>> and not as a MFD device.  The device does have common brightness and ramp
>> features and those can be accomodated by a TI LMU framework.
>>
>> The LM3633 dt binding needs to be moved from the ti-lmu.txt and a dedicated
>> LED dt binding needs to be added.  The new LM3633 LED dt binding will then
>> reside in the Documentation/devicetree/bindings/leds directory and follow the
>> current LED and general bindings guidelines.
> 
> What?
> 
>>  .../devicetree/bindings/leds/leds-lm3633.txt  | 102 ++++++++++++++++++
>>  .../devicetree/bindings/mfd/ti-lmu.txt        |  48 ---------
>>  2 files changed, 102 insertions(+), 48 deletions(-)
>>  create mode 100644
>>  Documentation/devicetree/bindings/leds/leds-lm3633.txt
> 
>> index 920f910be4e9..573e88578d3d 100644
>> --- a/Documentation/devicetree/bindings/mfd/ti-lmu.txt
>> +++ b/Documentation/devicetree/bindings/mfd/ti-lmu.txt
>> @@ -7,7 +7,6 @@ TI LMU driver supports lighting devices below.
>>    LM3532       Backlight
>>    LM3631       Backlight and regulator
>>    LM3632       Backlight and regulator
>> -  LM3633       Backlight, LED and fault monitor
>>    LM3695       Backlight
> 
> Are you seriously proposing to take one binding and split it into 6
> copy&pasted ones?

No that is not what I am proposing.  And never have.  I support keeping the MFD
devices in the MFD directory and only pulling out the single function devices as we have
debated over and over again.

> 
> That's not the way we do development. NAK.
> 
> We don't want to have copy & pasted code. We also don't want to have
> copy & pasted bindings. Nor changelogs, for that matter.
> 

Change was copy and pasted don't know why I need to rephrase the same exact
change only for a different part but I can modify it.

I do see what I can update here.  As you said I will fix up the ti-lmu binding in
such a way that the dedicated LED driver bindings point to the common binding for the TI-LMU
framework.

Dan

> Thank you,
> 								Pavel
> 


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

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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-24 14:54       ` Rob Herring
@ 2018-10-25 18:07         ` Dan Murphy
  2018-10-25 18:27           ` Jacek Anaszewski
  0 siblings, 1 reply; 30+ messages in thread
From: Dan Murphy @ 2018-10-25 18:07 UTC (permalink / raw)
  To: Rob Herring
  Cc: Pavel Machek, jacek.anaszewski, devicetree, linux-kernel,
	linux-leds, lee.jones, tony

Rob

On 10/24/2018 09:54 AM, Rob Herring wrote:
> On Wed, Oct 24, 2018 at 07:07:57AM -0500, Dan Murphy wrote:
>> Pavel
>>
>> On 10/24/2018 04:04 AM, Pavel Machek wrote:
>>> Hi!
>>>
>>>> The LM3697 is a single function LED driver. The single function LED
>>>> driver needs to reside in the LED directory as a dedicated LED driver
>>>> and not as a MFD device.  The device does have common brightness and ramp
>>>
>>> So it is single function LED driver. That does not mean it can not
>>> share bindings with the rest. Where the bindings live is not imporant.
>>>
>>
>> It can share bindings that are correctly done, not ones that are incomplete and incorrect.
>>
>> Where bindings live is important to new Linux kernel developers and product 
>> developers looking for the proper documentation on the H/W bindings.
>>
>>>> reside in the Documentation/devicetree/bindings/leds directory and follow the
>>>> current LED and general bindings guidelines.
>>>
>>> What you forgot to tell us in the changelog:
>>
>> I can add this to the changelog.
>>
>>>
>>>> +Optional child properties:
>>>> +	- runtime-ramp-up-msec: Current ramping from one brightness level to
>>>> +				the a higher brightness level.
>>>> +				Range from 2048 us - 117.44 s
>>>
>>> The other binding uses "ramp-up-msec". Tell us why you are changing this, or
>>> better don't change things needlessly.
>>>
>>> We don't want to be using "runtime-ramp-up-msec" for one device and
>>> "ramp-up-msec" for the other.
>>
>> This is another example of how the original bindings were incorrect and misleading.
>>
>> The LM3697 have 2 ramp implementations that can be used.
>>
>> Startup/Shutdown ramp and Runtime Ramp.  Same Ramp rates different registers and
>> different end user experience.
>>
>> So having a single node call ramp-up-msec is misleading and it does not
>> indicate what the H/W will do.
> 
> The existing ones aren't documented (present in the example is not 
> documented). This seems like something that should be common rather than 
> TI specific. Though it also seems more like something the user would 
> want to control (i.e. sysfs) rather than fixed in DT.
> 

Changing the runtime ramping or startup/shutdown ramping could also be done via sysfs.
I am not dedicated to having it in the DT file I was following prior art.

Jacek

Do you have an opinion on this?

Dan

> Rob
> 


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

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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-25 18:07         ` Dan Murphy
@ 2018-10-25 18:27           ` Jacek Anaszewski
  2018-10-25 18:32             ` Dan Murphy
                               ` (3 more replies)
  0 siblings, 4 replies; 30+ messages in thread
From: Jacek Anaszewski @ 2018-10-25 18:27 UTC (permalink / raw)
  To: Dan Murphy, Rob Herring
  Cc: Pavel Machek, devicetree, linux-kernel, linux-leds, lee.jones, tony

On 10/25/2018 08:07 PM, Dan Murphy wrote:
> Rob
> 
> On 10/24/2018 09:54 AM, Rob Herring wrote:
>> On Wed, Oct 24, 2018 at 07:07:57AM -0500, Dan Murphy wrote:
>>> Pavel
>>>
>>> On 10/24/2018 04:04 AM, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>> The LM3697 is a single function LED driver. The single function LED
>>>>> driver needs to reside in the LED directory as a dedicated LED driver
>>>>> and not as a MFD device.  The device does have common brightness and ramp
>>>>
>>>> So it is single function LED driver. That does not mean it can not
>>>> share bindings with the rest. Where the bindings live is not imporant.
>>>>
>>>
>>> It can share bindings that are correctly done, not ones that are incomplete and incorrect.
>>>
>>> Where bindings live is important to new Linux kernel developers and product 
>>> developers looking for the proper documentation on the H/W bindings.
>>>
>>>>> reside in the Documentation/devicetree/bindings/leds directory and follow the
>>>>> current LED and general bindings guidelines.
>>>>
>>>> What you forgot to tell us in the changelog:
>>>
>>> I can add this to the changelog.
>>>
>>>>
>>>>> +Optional child properties:
>>>>> +	- runtime-ramp-up-msec: Current ramping from one brightness level to
>>>>> +				the a higher brightness level.
>>>>> +				Range from 2048 us - 117.44 s
>>>>
>>>> The other binding uses "ramp-up-msec". Tell us why you are changing this, or
>>>> better don't change things needlessly.
>>>>
>>>> We don't want to be using "runtime-ramp-up-msec" for one device and
>>>> "ramp-up-msec" for the other.
>>>
>>> This is another example of how the original bindings were incorrect and misleading.
>>>
>>> The LM3697 have 2 ramp implementations that can be used.
>>>
>>> Startup/Shutdown ramp and Runtime Ramp.  Same Ramp rates different registers and
>>> different end user experience.
>>>
>>> So having a single node call ramp-up-msec is misleading and it does not
>>> indicate what the H/W will do.
>>
>> The existing ones aren't documented (present in the example is not 
>> documented). This seems like something that should be common rather than 
>> TI specific. Though it also seems more like something the user would 
>> want to control (i.e. sysfs) rather than fixed in DT.
>>
> 
> Changing the runtime ramping or startup/shutdown ramping could also be done via sysfs.
> I am not dedicated to having it in the DT file I was following prior art.
> 
> Jacek
> 
> Do you have an opinion on this?

This is this problem with the Device Tree's scope of responsibility.
It is defined as a means for "describing the hardware", but often
this rule is abused by the properties that fall into "configuration"
category. E.g. default-state, retain-state-suspended from leds-gpio.txt
or linux-default-trigger from common LED bindings.

In some cases this is justified. The question is whether it is something
that necessarily needs to be configured on driver probing? If not, then
I'd go for sysfs interface.

-- 
Best regards,
Jacek Anaszewski

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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-25 18:27           ` Jacek Anaszewski
@ 2018-10-25 18:32             ` Dan Murphy
  2018-10-25 19:54             ` Rob Herring
                               ` (2 subsequent siblings)
  3 siblings, 0 replies; 30+ messages in thread
From: Dan Murphy @ 2018-10-25 18:32 UTC (permalink / raw)
  To: Jacek Anaszewski, Rob Herring
  Cc: Pavel Machek, devicetree, linux-kernel, linux-leds, lee.jones, tony

Jacek

On 10/25/2018 01:27 PM, Jacek Anaszewski wrote:
> On 10/25/2018 08:07 PM, Dan Murphy wrote:
>> Rob
>>
>> On 10/24/2018 09:54 AM, Rob Herring wrote:
>>> On Wed, Oct 24, 2018 at 07:07:57AM -0500, Dan Murphy wrote:
>>>> Pavel
>>>>
>>>> On 10/24/2018 04:04 AM, Pavel Machek wrote:
>>>>> Hi!
>>>>>
>>>>>> The LM3697 is a single function LED driver. The single function LED
>>>>>> driver needs to reside in the LED directory as a dedicated LED driver
>>>>>> and not as a MFD device.  The device does have common brightness and ramp
>>>>>
>>>>> So it is single function LED driver. That does not mean it can not
>>>>> share bindings with the rest. Where the bindings live is not imporant.
>>>>>
>>>>
>>>> It can share bindings that are correctly done, not ones that are incomplete and incorrect.
>>>>
>>>> Where bindings live is important to new Linux kernel developers and product 
>>>> developers looking for the proper documentation on the H/W bindings.
>>>>
>>>>>> reside in the Documentation/devicetree/bindings/leds directory and follow the
>>>>>> current LED and general bindings guidelines.
>>>>>
>>>>> What you forgot to tell us in the changelog:
>>>>
>>>> I can add this to the changelog.
>>>>
>>>>>
>>>>>> +Optional child properties:
>>>>>> +	- runtime-ramp-up-msec: Current ramping from one brightness level to
>>>>>> +				the a higher brightness level.
>>>>>> +				Range from 2048 us - 117.44 s
>>>>>
>>>>> The other binding uses "ramp-up-msec". Tell us why you are changing this, or
>>>>> better don't change things needlessly.
>>>>>
>>>>> We don't want to be using "runtime-ramp-up-msec" for one device and
>>>>> "ramp-up-msec" for the other.
>>>>
>>>> This is another example of how the original bindings were incorrect and misleading.
>>>>
>>>> The LM3697 have 2 ramp implementations that can be used.
>>>>
>>>> Startup/Shutdown ramp and Runtime Ramp.  Same Ramp rates different registers and
>>>> different end user experience.
>>>>
>>>> So having a single node call ramp-up-msec is misleading and it does not
>>>> indicate what the H/W will do.
>>>
>>> The existing ones aren't documented (present in the example is not 
>>> documented). This seems like something that should be common rather than 
>>> TI specific. Though it also seems more like something the user would 
>>> want to control (i.e. sysfs) rather than fixed in DT.
>>>
>>
>> Changing the runtime ramping or startup/shutdown ramping could also be done via sysfs.
>> I am not dedicated to having it in the DT file I was following prior art.
>>
>> Jacek
>>
>> Do you have an opinion on this?
> 
> This is this problem with the Device Tree's scope of responsibility.
> It is defined as a means for "describing the hardware", but often
> this rule is abused by the properties that fall into "configuration"
> category. E.g. default-state, retain-state-suspended from leds-gpio.txt
> or linux-default-trigger from common LED bindings.
> 
> In some cases this is justified. The question is whether it is something
> that necessarily needs to be configured on driver probing? If not, then
> I'd go for sysfs interface.
> 

Appreciate the feedback.  I think you and Rob are right. This should be a sysfs
entry.  I can think of instances where the ramp times might want to be modified 
or even turned off.

I will change that implementation.

Dan

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

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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-25 18:27           ` Jacek Anaszewski
  2018-10-25 18:32             ` Dan Murphy
@ 2018-10-25 19:54             ` Rob Herring
  2018-10-26  8:30             ` Pavel Machek
  2018-10-26  8:37             ` Pavel Machek
  3 siblings, 0 replies; 30+ messages in thread
From: Rob Herring @ 2018-10-25 19:54 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Dan Murphy, Pavel Machek, devicetree, linux-kernel, linux-leds,
	lee.jones, tony

On Thu, Oct 25, 2018 at 08:27:18PM +0200, Jacek Anaszewski wrote:
> On 10/25/2018 08:07 PM, Dan Murphy wrote:
> > Rob
> > 
> > On 10/24/2018 09:54 AM, Rob Herring wrote:
> >> On Wed, Oct 24, 2018 at 07:07:57AM -0500, Dan Murphy wrote:
> >>> Pavel
> >>>
> >>> On 10/24/2018 04:04 AM, Pavel Machek wrote:
> >>>> Hi!
> >>>>
> >>>>> The LM3697 is a single function LED driver. The single function LED
> >>>>> driver needs to reside in the LED directory as a dedicated LED driver
> >>>>> and not as a MFD device.  The device does have common brightness and ramp
> >>>>
> >>>> So it is single function LED driver. That does not mean it can not
> >>>> share bindings with the rest. Where the bindings live is not imporant.
> >>>>
> >>>
> >>> It can share bindings that are correctly done, not ones that are incomplete and incorrect.
> >>>
> >>> Where bindings live is important to new Linux kernel developers and product 
> >>> developers looking for the proper documentation on the H/W bindings.
> >>>
> >>>>> reside in the Documentation/devicetree/bindings/leds directory and follow the
> >>>>> current LED and general bindings guidelines.
> >>>>
> >>>> What you forgot to tell us in the changelog:
> >>>
> >>> I can add this to the changelog.
> >>>
> >>>>
> >>>>> +Optional child properties:
> >>>>> +	- runtime-ramp-up-msec: Current ramping from one brightness level to
> >>>>> +				the a higher brightness level.
> >>>>> +				Range from 2048 us - 117.44 s
> >>>>
> >>>> The other binding uses "ramp-up-msec". Tell us why you are changing this, or
> >>>> better don't change things needlessly.
> >>>>
> >>>> We don't want to be using "runtime-ramp-up-msec" for one device and
> >>>> "ramp-up-msec" for the other.
> >>>
> >>> This is another example of how the original bindings were incorrect and misleading.
> >>>
> >>> The LM3697 have 2 ramp implementations that can be used.
> >>>
> >>> Startup/Shutdown ramp and Runtime Ramp.  Same Ramp rates different registers and
> >>> different end user experience.
> >>>
> >>> So having a single node call ramp-up-msec is misleading and it does not
> >>> indicate what the H/W will do.
> >>
> >> The existing ones aren't documented (present in the example is not 
> >> documented). This seems like something that should be common rather than 
> >> TI specific. Though it also seems more like something the user would 
> >> want to control (i.e. sysfs) rather than fixed in DT.
> >>
> > 
> > Changing the runtime ramping or startup/shutdown ramping could also be done via sysfs.
> > I am not dedicated to having it in the DT file I was following prior art.
> > 
> > Jacek
> > 
> > Do you have an opinion on this?
> 
> This is this problem with the Device Tree's scope of responsibility.
> It is defined as a means for "describing the hardware", but often
> this rule is abused by the properties that fall into "configuration"
> category. E.g. default-state, retain-state-suspended from leds-gpio.txt
> or linux-default-trigger from common LED bindings.
> 
> In some cases this is justified. The question is whether it is something
> that necessarily needs to be configured on driver probing? If not, then
> I'd go for sysfs interface.

Yes. I'd also add it should be along the lines of for a given 
board it's always configured in that way or is it something you'd want 
in the BIOS of your PC.

Rob

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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-25 18:27           ` Jacek Anaszewski
  2018-10-25 18:32             ` Dan Murphy
  2018-10-25 19:54             ` Rob Herring
@ 2018-10-26  8:30             ` Pavel Machek
  2018-10-26  8:37             ` Pavel Machek
  3 siblings, 0 replies; 30+ messages in thread
From: Pavel Machek @ 2018-10-26  8:30 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Dan Murphy, Rob Herring, devicetree, linux-kernel, linux-leds,
	lee.jones, tony

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

Hi!

> >>>>> +Optional child properties:
> >>>>> +	- runtime-ramp-up-msec: Current ramping from one brightness level to
> >>>>> +				the a higher brightness level.
> >>>>> +				Range from 2048 us - 117.44 s

> This is this problem with the Device Tree's scope of responsibility.
> It is defined as a means for "describing the hardware", but often
> this rule is abused by the properties that fall into "configuration"
> category. E.g. default-state, retain-state-suspended from leds-gpio.txt
> or linux-default-trigger from common LED bindings.

I always assumed that ramp-up time is there because hardware (power
supply?) can not handle "too fast" switching. Missing capacitors or
something. If so, this needs to be in device tree.

If not, it indeed does not belong to device tree.

> In some cases this is justified. The question is whether it is something
> that necessarily needs to be configured on driver probing? If not, then
> I'd go for sysfs interface.

While this would be useful for hardware accelerated patterns, I doubt
we want to make it configurable without that support.

									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] 30+ messages in thread

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-25 18:27           ` Jacek Anaszewski
                               ` (2 preceding siblings ...)
  2018-10-26  8:30             ` Pavel Machek
@ 2018-10-26  8:37             ` Pavel Machek
  2018-10-30 13:40               ` Dan Murphy
  3 siblings, 1 reply; 30+ messages in thread
From: Pavel Machek @ 2018-10-26  8:37 UTC (permalink / raw)
  To: Jacek Anaszewski
  Cc: Dan Murphy, Rob Herring, devicetree, linux-kernel, linux-leds,
	lee.jones, tony

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

Hi!

> > Do you have an opinion on this?
> 
> This is this problem with the Device Tree's scope of responsibility.
> It is defined as a means for "describing the hardware", but often
> this rule is abused by the properties that fall into "configuration"
> category. E.g. default-state, retain-state-suspended from leds-gpio.txt
> or linux-default-trigger from common LED bindings.

"retain-state-suspended" is actually hardware property. On PCs,
(most?) LEDs will go off in suspend. On android phones, LEDs stay on
while suspended.

"linux-default-trigger" is actually kind-of hardware property, too. If
LED has an icon near it (or on it), you want to use that LED in that
meaning.

(Thinkpad X60 has "wifi", "bluetooth", "numlock", "capslock", "hdd",
"power", "battery", "ac" and "sleep" leds. Surely we should use them
in that meaning?)

Now, if someone has leds labeled "user 1..user 4" and uses
"linux-default-trigger" there, that is kind of "more interesting".

"default-state" is similar (subset of "linux-default-trigger"); you
don't want power LED to go off during kernel boot...

								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] 30+ messages in thread

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-26  8:37             ` Pavel Machek
@ 2018-10-30 13:40               ` Dan Murphy
  2019-03-02 23:07                 ` Pavel Machek
  0 siblings, 1 reply; 30+ messages in thread
From: Dan Murphy @ 2018-10-30 13:40 UTC (permalink / raw)
  To: Pavel Machek, Jacek Anaszewski
  Cc: Rob Herring, devicetree, linux-kernel, linux-leds, lee.jones, tony

All

On 10/26/2018 03:37 AM, Pavel Machek wrote:
> Hi!
> 
>>> Do you have an opinion on this?
>>
>> This is this problem with the Device Tree's scope of responsibility.
>> It is defined as a means for "describing the hardware", but often
>> this rule is abused by the properties that fall into "configuration"
>> category. E.g. default-state, retain-state-suspended from leds-gpio.txt
>> or linux-default-trigger from common LED bindings.
> 
> "retain-state-suspended" is actually hardware property. On PCs,
> (most?) LEDs will go off in suspend. On android phones, LEDs stay on
> while suspended.
> 
> "linux-default-trigger" is actually kind-of hardware property, too. If
> LED has an icon near it (or on it), you want to use that LED in that
> meaning.
> 
> (Thinkpad X60 has "wifi", "bluetooth", "numlock", "capslock", "hdd",
> "power", "battery", "ac" and "sleep" leds. Surely we should use them
> in that meaning?)
> 
> Now, if someone has leds labeled "user 1..user 4" and uses
> "linux-default-trigger" there, that is kind of "more interesting".
> 
> "default-state" is similar (subset of "linux-default-trigger"); you
> don't want power LED to go off during kernel boot...


I just want to follow up here and let you know that I have not abandoned this.
I am trying to rework and fix up the ti lmu binding as asked by Pavel.  Its proving
to be a bit of a challenge based on looking forward to what will be the implementation.

I also did go offline and speak with Tony.  I have a Droid 4 so I can test all of this on
production hardware as well.

Updated patchset will take some time to get out.

Dan

> 
> 								Pavel
> 


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

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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2018-10-30 13:40               ` Dan Murphy
@ 2019-03-02 23:07                 ` Pavel Machek
  2019-03-04 19:14                   ` Dan Murphy
  0 siblings, 1 reply; 30+ messages in thread
From: Pavel Machek @ 2019-03-02 23:07 UTC (permalink / raw)
  To: Dan Murphy
  Cc: Jacek Anaszewski, Rob Herring, devicetree, linux-kernel,
	linux-leds, lee.jones, tony

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

Hi!

> I just want to follow up here and let you know that I have not abandoned this.
> I am trying to rework and fix up the ti lmu binding as asked by Pavel.  Its proving
> to be a bit of a challenge based on looking forward to what will be the implementation.
> 
> I also did go offline and speak with Tony.  I have a Droid 4 so I can test all of this on
> production hardware as well.
> 
> Updated patchset will take some time to get out.

Is there any progress here?

This is needed for display on Droid 4, so it is somehow
important... and Milo Kim's patches are pretty reasonable.

I still believe best way forward is to merge them, and let you submit
any improvements on top of them...

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

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

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

* Re: [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697
  2019-03-02 23:07                 ` Pavel Machek
@ 2019-03-04 19:14                   ` Dan Murphy
  0 siblings, 0 replies; 30+ messages in thread
From: Dan Murphy @ 2019-03-04 19:14 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Jacek Anaszewski, Rob Herring, devicetree, linux-kernel,
	linux-leds, lee.jones, tony

Pavel

On 3/2/19 5:07 PM, Pavel Machek wrote:
> Hi!
> 
>> I just want to follow up here and let you know that I have not abandoned this.
>> I am trying to rework and fix up the ti lmu binding as asked by Pavel.  Its proving
>> to be a bit of a challenge based on looking forward to what will be the implementation.
>>
>> I also did go offline and speak with Tony.  I have a Droid 4 so I can test all of this on
>> production hardware as well.
>>
>> Updated patchset will take some time to get out.
> 
> Is there any progress here?
> 
> This is needed for display on Droid 4, so it is somehow
> important... and Milo Kim's patches are pretty reasonable.
> 
> I still believe best way forward is to merge them, and let you submit
> any improvements on top of them...
> 

I had looked at this a while back.  And I will pick this up again as it seems that the
multi color framework will take longer than expected.

For the Droid 4 it uses the lm3552 backlight driver, at least that is what is in the DT.

There is a lm3530 backlight driver that is already available and could be upgraded to DT support and have the lm3552
added to it but the issues are the 3532 has 3 outputs and the 3530 has 1 and the register map is a bit different.

There are register differences between the two devices but the driver already has ramp support, PWM support (exponential/linear),
ALS support and manual support already available along with user space configuration.

I was working on creating a new lm3532 driver based off the lm3530 driver or adapting the 3530 to include the 3532 and
making the driver generic to this family.

Dan

> Best regards,
> 									Pavel
> 


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

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

end of thread, other threads:[~2019-03-04 19:14 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-23 17:06 [PATCH v4 1/7] leds: add TI LMU backlight driver Dan Murphy
2018-10-23 17:06 ` [PATCH v4 2/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3697 Dan Murphy
2018-10-24  9:04   ` Pavel Machek
2018-10-24 12:07     ` Dan Murphy
2018-10-24 13:43       ` Pavel Machek
2018-10-24 14:54       ` Rob Herring
2018-10-25 18:07         ` Dan Murphy
2018-10-25 18:27           ` Jacek Anaszewski
2018-10-25 18:32             ` Dan Murphy
2018-10-25 19:54             ` Rob Herring
2018-10-26  8:30             ` Pavel Machek
2018-10-26  8:37             ` Pavel Machek
2018-10-30 13:40               ` Dan Murphy
2019-03-02 23:07                 ` Pavel Machek
2019-03-04 19:14                   ` Dan Murphy
2018-10-24 14:49   ` Rob Herring
2018-10-25 17:56     ` Dan Murphy
2018-10-23 17:06 ` [PATCH v4 3/7] mfd: ti-lmu: Remove support for LM3697 Dan Murphy
2018-10-23 17:06 ` [PATCH v4 4/7] leds: lm3697: Introduce the lm3697 driver Dan Murphy
2018-10-23 17:06 ` [PATCH v4 5/7] dt-bindings: ti-lmu: Modify dt bindings for the LM3633 Dan Murphy
2018-10-24  9:23   ` Pavel Machek
2018-10-24 14:35     ` Rob Herring
2018-10-24 18:38       ` Pavel Machek
2018-10-24 21:50         ` Rob Herring
2018-10-25 18:01     ` Dan Murphy
2018-10-23 17:06 ` [PATCH v4 6/7] mfd: ti-lmu: Remove support for LM3633 Dan Murphy
2018-10-23 17:06 ` [PATCH v4 7/7] leds: lm3633: Introduce the lm3633 driver Dan Murphy
2018-10-24  9:14 ` [PATCH v4 1/7] leds: add TI LMU backlight driver Pavel Machek
2018-10-24 12:27   ` Dan Murphy
2018-10-24 13:17     ` 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).