[PATCH v7 0/6] LM3697 dedicated LED driver

[PATCH v7 0/6] LM3697 dedicated LED driver

[Dan Murphy]

All

The LM3697 LED driver has two opportunities to be included in the Linux
kernel source.

The ti-lmu MFD driver and the leds-lm3697 driver contained in this series.

This LED driver is basic in nature and is not really suitable for the MFD
inclusion.  This device has no other function than to control the LEDs.

So this series will remove the lm3697 from the ti-lmu and add the driver
in the LEDs directory.

Dan Murphy


Dan Murphy (6):
  dt-bindings: ti-lmu: Remove LM3697
  mfd: ti-lmu: Remove support for LM3697
  dt-bindings: leds: Add bindings for lm3697 driver
  leds: lm3697: Introduce the lm3697 driver
  dt-bindings: leds: Add runtime ramp node for LM3697
  leds: lm3697: Add ramp rate feature

 .../devicetree/bindings/leds/leds-lm3697.txt  |  98 ++++
 .../devicetree/bindings/mfd/ti-lmu.txt        |  26 +-
 drivers/leds/Kconfig                          |   9 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/leds-lm3697.c                    | 449 ++++++++++++++++++
 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 -
 9 files changed, 559 insertions(+), 88 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt
 create mode 100644 drivers/leds/leds-lm3697.c

-- 
2.17.0.1855.g63749b2dea

[PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Dan Murphy]

Remove support for the LM3697 LED device
from the ti-lmu.  The LM3697 will be supported
via a stand alone LED driver.

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

v7 - New change for the series based on the comments in https://lore.kernel.org/patchwork/patch/982550/

 .../devicetree/bindings/mfd/ti-lmu.txt        | 26 +------------------
 1 file changed, 1 insertion(+), 25 deletions(-)

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.17.0.1855.g63749b2dea

[PATCH v7 2/6] mfd: ti-lmu: Remove support for LM3697

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

v7 - New change for the series based on the comments in https://lore.kernel.org/patchwork/patch/982550/

 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 -
 4 files changed, 1 insertion(+), 63 deletions(-)

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.17.0.1855.g63749b2dea

[PATCH v7 3/6] dt-bindings: leds: Add bindings for lm3697 driver

[Dan Murphy]

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

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

v7 - Fix compatible from 3967 to 3697, and other comments. - https://lore.kernel.org/patchwork/patch/982550/

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

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

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
new file mode 100644
index 000000000000..85ae075f6677
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
@@ -0,0 +1,86 @@
+* Texas Instruments - LM3697 Highly Efficient White LED Driver
+
+The LM3697 11-bit LED driver provides high-
+performance backlight dimming for 1, 2, or 3 series
+LED strings while delivering up to 90% efficiency.
+
+This device is suitable for display and keypad Lighting
+
+Required properties:
+	- compatible:
+		"ti,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:
+	- 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>;
+		label = "white:first_backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
+
+	led@1 {
+		reg = <1>;
+		led-sources = <0 1 0>;
+		label = "white:second_backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
+}
+
+All HVLED strings controlled by control bank A
+
+led-controller@36 {
+	compatible = "ti,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>;
+		label = "white:backlight_cluster";
+		linux,default-trigger = "backlight";
+	};
+}
+
+For more product information please see the link below:
+http://www.ti.com/lit/ds/symlink/lm3697.pdf
-- 
2.17.0.1855.g63749b2dea

[PATCH v7 4/6] leds: lm3697: Introduce the lm3697 driver

[Dan Murphy]

Introduce the lm3697 LED driver for
backlighting and display.

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

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

v7 - No change - https://lore.kernel.org/patchwork/patch/982551/

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

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

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

[PATCH v7 5/6] dt-bindings: leds: Add runtime ramp node for LM3697

[Dan Murphy]

Add runtime ramp rates for the LM3697 LED
driver.  This LED driver supports separate
runtime ramp rates for going from a higher
or lower level of brightnesses

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

v7 - New change for the series to support feature in ti-lmu

 .../devicetree/bindings/leds/leds-lm3697.txt         | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/leds-lm3697.txt b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
index 85ae075f6677..4bb2ed51025b 100644
--- a/Documentation/devicetree/bindings/leds/leds-lm3697.txt
+++ b/Documentation/devicetree/bindings/leds/leds-lm3697.txt
@@ -30,6 +30,12 @@ Required child properties:
 			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
@@ -51,6 +57,8 @@ led-controller@36 {
 	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";
 	};
@@ -58,6 +66,8 @@ led-controller@36 {
 	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";
 	};
@@ -77,6 +87,8 @@ led-controller@36 {
 	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";
 	};
-- 
2.17.0.1855.g63749b2dea

[PATCH v7 6/6] leds: lm3697: Add ramp rate feature

[Dan Murphy]

Add the runtime ramp up and down of the
LEDs in the specific control bank.

Each control bank can have separate ramp up
and ramp down values for the lighting zones
the control banks manage.

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

v7 - New change for the series to support feature in ti-lmu

 drivers/leds/leds-lm3697.c | 72 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/leds/leds-lm3697.c b/drivers/leds/leds-lm3697.c
index 7416c545ec59..997c270a46b5 100644
--- a/drivers/leds/leds-lm3697.c
+++ b/drivers/leds/leds-lm3697.c
@@ -59,6 +59,8 @@ struct lm3697_led {
 	struct led_classdev led_dev;
 	struct lm3697 *priv;
 	int control_bank;
+	int ramp_up_rate;
+	int ramp_down_rate;
 };
 
 /**
@@ -178,6 +180,62 @@ static int lm3697_set_control_bank(struct lm3697 *priv)
 	return ret;
 }
 
+static int lm3697_find_ramp_reg_val(int rate)
+{
+	const static int lookup[16] = { 2, 262, 524, 1049, 2090, 4194, 8389,
+					16780, 33550, 41940, 50330, 58720,
+					67110, 83880, 100660, 117440};
+	int i;
+
+	for (i = 1; i < ARRAY_SIZE(lookup); i++) {
+		if (rate == lookup[i])
+			return i;
+
+		if (rate > lookup[i - 1] && rate < lookup[i]) {
+			if (rate - lookup[i - 1] < lookup[i] - rate)
+				return i - 1;
+			else
+				return i;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int lm3697_set_ramp_rates(struct lm3697 *priv)
+{
+	u8 ramp, ramp_up, ramp_down;
+	struct lm3697_led *led;
+	u8 ramp_reg;
+	int i, ret = 0;
+
+	for (i = 0; i < 2; i++) {
+		led = &priv->leds[i];
+		if (led->ramp_up_rate == 0 && led->ramp_down_rate == 0)
+			continue;
+
+		if (led->control_bank == LM3697_CONTROL_A)
+			ramp_reg = LM3697_CTRL_A_RAMP;
+		else
+			ramp_reg = LM3697_CTRL_B_RAMP;
+
+		ramp_up = lm3697_find_ramp_reg_val(led->ramp_up_rate);
+		ramp_down = lm3697_find_ramp_reg_val(led->ramp_down_rate);
+
+		if (ramp_up < 0 || ramp_down < 0) {
+			dev_err(&priv->client->dev, "Cannot find ramp rate\n");
+			continue;
+		}
+
+		ramp = (ramp_up << 4) | ramp_down;
+		ret = regmap_write(priv->regmap, ramp_reg, ramp);
+		if (ret)
+			dev_err(&priv->client->dev, "Cannot write ramp config\n");
+	}
+
+	return ret;
+}
+
 static int lm3697_init(struct lm3697 *priv)
 {
 	int ret;
@@ -202,6 +260,9 @@ static int lm3697_init(struct lm3697 *priv)
 	if (ret)
 		dev_err(&priv->client->dev, "Setting the CRTL bank failed\n");
 
+	ret = lm3697_set_ramp_rates(priv);
+	if (ret)
+		dev_err(&priv->client->dev, "Setting the ramp rate failed\n");
 out:
 	return ret;
 }
@@ -254,6 +315,17 @@ static int lm3697_probe_dt(struct lm3697 *priv)
 			goto child_out;
 		}
 
+		ret = fwnode_property_read_u32(child, "runtime-ramp-up-msec",
+					       &led->ramp_up_rate);
+		if (ret)
+			dev_warn(&priv->client->dev, "runtime-ramp-up-msec property missing\n");
+
+		ret = fwnode_property_read_u32(child, "runtime-ramp-down-msec",
+					       &led->ramp_down_rate);
+		if (ret)
+			dev_warn(&priv->client->dev, "runtime-ramp-down-msec property missing\n");
+
+
 		fwnode_property_read_string(child, "linux,default-trigger",
 					    &led->led_dev.default_trigger);
 
-- 
2.17.0.1855.g63749b2dea

Re: [PATCH v7 2/6] mfd: ti-lmu: Remove support for LM3697

[Lee Jones]

On Tue, 11 Sep 2018, Dan Murphy wrote:

> 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>
> ---
> 
> v7 - New change for the series based on the comments in https://lore.kernel.org/patchwork/patch/982550/
> 
>  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 -
>  4 files changed, 1 insertion(+), 63 deletions(-)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Lee Jones]

On Tue, 11 Sep 2018, Dan Murphy wrote:

> Remove support for the LM3697 LED device
> from the ti-lmu.  The LM3697 will be supported
> via a stand alone LED driver.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v7 - New change for the series based on the comments in https://lore.kernel.org/patchwork/patch/982550/
> 
>  .../devicetree/bindings/mfd/ti-lmu.txt        | 26 +------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Pavel Machek]

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

On Tue 2018-09-11 12:08:20, Dan Murphy wrote:
> Remove support for the LM3697 LED device
> from the ti-lmu.  The LM3697 will be supported
> via a stand alone LED driver.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>

I'd really like to see better explanation here.

We have existing binding, for lm3697 and similar devices. With this
series, different binding is introduced, without documented reason.

That's bad.

Now, maybe you are right and the hardware should be handled by
drivers/leds, not drivers/mfd. But we should have solution for all the
similar chips, and that still does not mean we have to modify the
binding. (But maybe we want to move it to different
directory). Bindings are supposed to describe hardware, not mirror
structure of our drivers.

Unless there's something fatally wrong with the binding... but in such
case we'd like to know what is wrong.

[And yes, I recognize current situation is ... not ideal and I'm
willing to help. But I'm not sure this is step in right direction.]

Thanks,
								Pavel


> ---
> 
> v7 - New change for the series based on the comments in https://lore.kernel.org/patchwork/patch/982550/
> 
>  .../devicetree/bindings/mfd/ti-lmu.txt        | 26 +------------------
>  1 file changed, 1 insertion(+), 25 deletions(-)
> 
> 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";
> -	};
> -};

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

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

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Dan Murphy]

Pavel

On 09/11/2018 03:05 PM, Pavel Machek wrote:
> On Tue 2018-09-11 12:08:20, Dan Murphy wrote:
>> Remove support for the LM3697 LED device
>> from the ti-lmu.  The LM3697 will be supported
>> via a stand alone LED driver.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> 
> I'd really like to see better explanation here.
> 

I will update the commit message but....

How do I politely explain that the original implementation was wrong for certain devices?
How do I politely explain that this binding has information that does not exist?
Isn't code and documentation supposed to be pushed in stages together?
Like document the current supported source features and then submit patches to amend new
features or changes to the source.  I did that in this series
where I introduced the driver and when I added a feature I added the binding information.

Unfortunately Milo left TI before he had a chance to finish upstreaming.

Some issues with the current binding:
1) The documentation referenced in the ti-lmu.txt does not exist but the binding was accepted.
[1] ../leds/backlight/ti-lmu-backlight.txt
[2] ../leds/leds-lm3633.txt
[3] ../regulator/lm363x-regulator.txt

2) ramp-up-msec and ramp-down-msec are not explained or defined in this current binding.
Looks to be defined in the staged code though.

3) The ramp-up-msec and ramp-down-msec are not the right node parameters
the code shows ti,ramp-down-ms and ti,ramp-up-ms.  Looking at the clean up of the binding
in the staged commits the binding still does not match the code.

https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/commit/?h=droid4-pending-v4.19&id=d774c7e447ac911e73a1b3c775e6d89f0422218c

4) The ramp rates ranges are incorrect for the LM3697 it should be from 0 -> 117400 ms

5) The children compatibles are not defined in this binding but appear to be removed in the staged code.

6) address-cells and size-cells are missing from the binding

I am attempting to clean this up for all the dedicated LED drivers.
There is a lot of bloat with this driver to support a single LED driver that is a single function device.

This ti-lmu driver may be a great thing for the Droid 4 but from a customers
stand point that just wants to use just 1 of the dedicated LED drivers this driver is
overly complex and possibly hard to add code for differentiation.

> We have existing binding, for lm3697 and similar devices. With this
> series, different binding is introduced, without documented reason.
> 

I can update the commit message to indicate that this device is not a MFD
device and only is a SFD that needs to be supported by a dedicated driver.

> That's bad.

This ti-lmu binding is more misleading in the current state.

> 
> Now, maybe you are right and the hardware should be handled by
> drivers/leds, not drivers/mfd. But we should have solution for all the
> similar chips, and that still does not mean we have to modify the
> binding. (But maybe we want to move it to different
> directory). Bindings are supposed to describe hardware, not mirror
> structure of our drivers.

But this is not clean to have this device in the MFD bindings directory
when the support is in the LEDs directory.

I am working on creating driver for the other single function devices.
It will take some time.  I am waiting on the hardware to come in.

> 
> Unless there's something fatally wrong with the binding... but in such
> case we'd like to know what is wrong.
> 
> [And yes, I recognize current situation is ... not ideal and I'm
> willing to help. But I'm not sure this is step in right direction.]

It will take some time to get this all cleaned up.  But I am willing to
get this done.

Dan

> 
> Thanks,
> 								Pavel
> 
> 
>> ---
>>
>> v7 - New change for the series based on the comments in https://lore.kernel.org/patchwork/patch/982550/
>>
>>  .../devicetree/bindings/mfd/ti-lmu.txt        | 26 +------------------
>>  1 file changed, 1 insertion(+), 25 deletions(-)
>>
>> 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";
>> -	};
>> -};
> 


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

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Jacek Anaszewski]

On 09/11/2018 10:05 PM, Pavel Machek wrote:
> On Tue 2018-09-11 12:08:20, Dan Murphy wrote:
>> Remove support for the LM3697 LED device
>> from the ti-lmu.  The LM3697 will be supported
>> via a stand alone LED driver.
>>
>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> 
> I'd really like to see better explanation here.
> 
> We have existing binding, for lm3697 and similar devices. With this
> series, different binding is introduced, without documented reason.
> 
> That's bad.
> 
> Now, maybe you are right and the hardware should be handled by
> drivers/leds, not drivers/mfd. But we should have solution for all the
> similar chips, and that still does not mean we have to modify the
> binding. (But maybe we want to move it to different
> directory). Bindings are supposed to describe hardware, not mirror
> structure of our drivers.
> 
> Unless there's something fatally wrong with the binding... but in such
> case we'd like to know what is wrong.

Dangling references ?

> [And yes, I recognize current situation is ... not ideal and I'm
> willing to help. But I'm not sure this is step in right direction.]
> 
> Thanks,


-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Pavel Machek]

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

Hi!

> On 09/11/2018 03:05 PM, Pavel Machek wrote:
> > On Tue 2018-09-11 12:08:20, Dan Murphy wrote:
> >> Remove support for the LM3697 LED device
> >> from the ti-lmu.  The LM3697 will be supported
> >> via a stand alone LED driver.
> >>
> >> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> > 
> > I'd really like to see better explanation here.
> > 
> 
> I will update the commit message but....
> 
> How do I politely explain that the original implementation was wrong for certain devices?

Implementation? Device tree is hardware description.

> How do I politely explain that this binding has information that
>  does not exist?

I don't understand this sentence.

> Isn't code and documentation supposed to be pushed in stages
> together?

Device tree is _not_ documentation. And yes, it is normally pushed
together. But that did not happen here, and bindings are already in.

Bindings are an ABI between bootloader and kernel. They should not be
changed lightly. 

Here I believe they should be fixed; not deleted.

> Unfortunately Milo left TI before he had a chance to finish upstreaming.

Well -- I wanted to finish upstreaming. I still have those patches,
and they still look reasonably well. Better than 

> Some issues with the current binding:
> 1) The documentation referenced in the ti-lmu.txt does not exist but the binding was accepted.
> [1] ../leds/backlight/ti-lmu-backlight.txt
> [2] ../leds/leds-lm3633.txt
> [3] ../regulator/lm363x-regulator.txt
> 
> 2) ramp-up-msec and ramp-down-msec are not explained or defined in this current binding.
> Looks to be defined in the staged code though.
> 
> 3) The ramp-up-msec and ramp-down-msec are not the right node parameters
> the code shows ti,ramp-down-ms and ti,ramp-up-ms.  Looking at the clean up of the binding
> in the staged commits the binding still does not match the code.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/commit/?h=droid4-pending-v4.19&id=d774c7e447ac911e73a1b3c775e6d89f0422218c
>

Bindings are authoritative. Of course, they can be fixed if really
neccessary, but normally code should be adapted to the bindings, not
the other way around.

If slow ramping up/down is something that would be expected in other
chips, too, ti, prefix is not good idea.

> 4) The ramp rates ranges are incorrect for the LM3697 it should be from 0 -> 117400 ms
> 
> 5) The children compatibles are not defined in this binding but appear to be removed in the staged code.
> 
> 6) address-cells and size-cells are missing from the binding

Yep, ok, so these should be fixed. Still the bindings should be fixed,
not removed and started over.

> I am attempting to clean this up for all the dedicated LED drivers.
> There is a lot of bloat with this driver to support a single LED driver that is a single function device.
>

What about the multi function devices? They should have same binding.

> This ti-lmu driver may be a great thing for the Droid 4 but from a customers
> stand point that just wants to use just 1 of the dedicated LED drivers this driver is
> overly complex and possibly hard to add code for differentiation.

The ti-lmu driver sounds like a way to go. We really want single
driver, even if more complex, than 6 separate ones.

> > We have existing binding, for lm3697 and similar devices. With this
> > series, different binding is introduced, without documented reason.
> > 
> 
> I can update the commit message to indicate that this device is not a MFD
> device and only is a SFD that needs to be supported by a dedicated
> driver.

Device bindings are operating systems independend. This is not an argument.

> > Now, maybe you are right and the hardware should be handled by
> > drivers/leds, not drivers/mfd. But we should have solution for all the
> > similar chips, and that still does not mean we have to modify the
> > binding. (But maybe we want to move it to different
> > directory). Bindings are supposed to describe hardware, not mirror
> > structure of our drivers.
> 
> But this is not clean to have this device in the MFD bindings directory
> when the support is in the LEDs directory.

Perhaps location of the binding is wrong. Still not reason to drop it
and introduce new one.

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

You see? Even if your "new" binding has no flaws, "flawed" binding
still remains in tree, and we still have problems for lm3632, 3633,
3695. That's why it needs to be fixed, not replaced.

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

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Dan Murphy]

Pavel

On 09/12/2018 04:49 PM, Pavel Machek wrote:
> Hi!
> 
>> On 09/11/2018 03:05 PM, Pavel Machek wrote:
>>> On Tue 2018-09-11 12:08:20, Dan Murphy wrote:
>>>> Remove support for the LM3697 LED device
>>>> from the ti-lmu.  The LM3697 will be supported
>>>> via a stand alone LED driver.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>>
>>> I'd really like to see better explanation here.
>>>
>>
>> I will update the commit message but....
>>
>> How do I politely explain that the original implementation was wrong for certain devices?
> 
> Implementation? Device tree is hardware description.

Yes this hardware description is incorrect.  The hardware description is
describing a MFD but this LED driver (and a couple others) only perform
1 function and that is to drive a LED string.

> 
>> How do I politely explain that this binding has information that
>>  does not exist?
> 
> I don't understand this sentence.

That was explained later on the issues with the binding.

> 
>> Isn't code and documentation supposed to be pushed in stages
>> together?
> 
> Device tree is _not_ documentation. And yes, it is normally pushed
> together. But that did not happen here, and bindings are already in.
> 

Hmm..  Its not documentation but it is in the Documentation folder.
And just because the bindings are in does not mean they cannot be changed.

So someone hurried up and pushed the bindings that were not accurate and
they were taken and now we are stuck with this implementation?

That does not seem like progress.

> Bindings are an ABI between bootloader and kernel. They should not be
> changed lightly. 
> 
> Here I believe they should be fixed; not deleted.
> 

I am fixing them.  Removing devices that should not have been there to begin with.

Also if they are not to be changed lightly then why is there so much churn in the
staged patches.  Looks like the compatible is changing with this patch and appears that it
was not documented appropriately.
According to the above statement this patch should be rejected because it is changing the ABI.

https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/commit/?h=droid4-pending-v4.19&id=d774c7e447ac911e73a1b3c775e6d89f0422218c

>> Unfortunately Milo left TI before he had a chance to finish upstreaming.
> 
> Well -- I wanted to finish upstreaming. I still have those patches,
> and they still look reasonably well. Better than 
> 

than?

>> Some issues with the current binding:
>> 1) The documentation referenced in the ti-lmu.txt does not exist but the binding was accepted.
>> [1] ../leds/backlight/ti-lmu-backlight.txt
>> [2] ../leds/leds-lm3633.txt
>> [3] ../regulator/lm363x-regulator.txt
>>
>> 2) ramp-up-msec and ramp-down-msec are not explained or defined in this current binding.
>> Looks to be defined in the staged code though.
>>
>> 3) The ramp-up-msec and ramp-down-msec are not the right node parameters
>> the code shows ti,ramp-down-ms and ti,ramp-up-ms.  Looking at the clean up of the binding
>> in the staged commits the binding still does not match the code.
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/commit/?h=droid4-pending-v4.19&id=d774c7e447ac911e73a1b3c775e6d89f0422218c
>>
> 
> Bindings are authoritative. Of course, they can be fixed if really
> neccessary, but normally code should be adapted to the bindings, not
> the other way around.

They should only be authoritative when they are correct.

> 
> If slow ramping up/down is something that would be expected in other
> chips, too, ti, prefix is not good idea.
> 

Well if this is something expected for other chips shouldn't the LED framework be
updated as opposed to creating a stub layer to do this?

>> 4) The ramp rates ranges are incorrect for the LM3697 it should be from 0 -> 117400 ms
>>
>> 5) The children compatibles are not defined in this binding but appear to be removed in the staged code.
>>
>> 6) address-cells and size-cells are missing from the binding
> 
> Yep, ok, so these should be fixed. Still the bindings should be fixed,
> not removed and started over.
> 

So once something is in the binding it can never be removed?

>> I am attempting to clean this up for all the dedicated LED drivers.
>> There is a lot of bloat with this driver to support a single LED driver that is a single function device.
>>
> 
> What about the multi function devices? They should have same binding.

The MFD devices defined are not in contention here only the SFD.
The only comment there is data section bloat that occurs in attempting to support
only one device out of the supported devices.  But that can be done via #ifconfig

> 
>> This ti-lmu driver may be a great thing for the Droid 4 but from a customers
>> stand point that just wants to use just 1 of the dedicated LED drivers this driver is
>> overly complex and possibly hard to add code for differentiation.
> 
> The ti-lmu driver sounds like a way to go. We really want single
> driver, even if more complex, than 6 separate ones.

Not really in agreement here.  The ti-lmu driver only makes sense for MFD devices where
the device itself supports more then just one function.  Otherwise we could just lump every
single LED device into this driver.

Again not sure why we gloss over the fact that this driver with all of the device data is
the correct way to go for every single device.  If a target product only has the LM3697 why does the MFD need
to be used?  Why do we need to bring in all the support for all the additional devices?

This may be great for Droid 4 but not for any other device.  Including IoT devices that need smaller kernel
foot prints.

All be it the analog side of these devices may be the same but the digital side for each chip is different.
The register maps are not the same, the number of supported LED strings are not
the same, there are features not supported by this driver and will be a pain to add.
Adding features will be a complete night mare especially for the LM3632 which also support Torch and Flash.


Here are where a single driver will start getting messy and support difficult
LM3533 has ALS and an ADC for an ALS analog sensor
LM3631 has no ALS functionality
LM3632 has strobe/torch functionality and no ramp support
LM3633 has lvled support coupled with the hvled support
LM3695 does not even appear to be available publicly
LM3697 is the only device that that this driver could be used for as is.

Jacek has already agreed that having a dedicated LED driver for SFD devices is preferred method.

> 
>>> We have existing binding, for lm3697 and similar devices. With this
>>> series, different binding is introduced, without documented reason.
>>>
>>
>> I can update the commit message to indicate that this device is not a MFD
>> device and only is a SFD that needs to be supported by a dedicated
>> driver.
> 
> Device bindings are operating systems independend. This is not an argument.
> 

I have no idea what this statement means.

>>> Now, maybe you are right and the hardware should be handled by
>>> drivers/leds, not drivers/mfd. But we should have solution for all the
>>> similar chips, and that still does not mean we have to modify the
>>> binding. (But maybe we want to move it to different
>>> directory). Bindings are supposed to describe hardware, not mirror
>>> structure of our drivers.
>>
>> But this is not clean to have this device in the MFD bindings directory
>> when the support is in the LEDs directory.
> 
> Perhaps location of the binding is wrong. Still not reason to drop it
> and introduce new one.
> 

Location and implementation is wrong for certain devices.

>>>> 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:
> 
> You see? Even if your "new" binding has no flaws, "flawed" binding
> still remains in tree, and we still have problems for lm3632, 3633,
> 3695. That's why it needs to be fixed, not replaced.

I am working on those as well.  LM3632 is a MFD device and can be supported via
the MFD framework.

This work for the other LED drivers needs to be done before we bring in any other
code.

Dan

> 
> Best regards,
> 									Pavel
> 


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

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Pavel Machek]

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

Hi!

> >> How do I politely explain that the original implementation was wrong for certain devices?
> > 
> > Implementation? Device tree is hardware description.
> 
> Yes this hardware description is incorrect.  The hardware description is
> describing a MFD but this LED driver (and a couple others) only perform
> 1 function and that is to drive a LED string.

So what? Does not seem incorrect to me. Maybe the description should
not be in MFD directory, but other than that...

> >> Isn't code and documentation supposed to be pushed in stages
> >> together?
> > 
> > Device tree is _not_ documentation. And yes, it is normally pushed
> > together. But that did not happen here, and bindings are already in.
> 
> Hmm..  Its not documentation but it is in the Documentation folder.
> And just because the bindings are in does not mean they cannot be
> changed.

You may want to learn more about device tree and/or talk to the device
tree maintainers. This is an old article. https://lwn.net/Articles/561462/

NAK on this patch. I see that this binding has problems, but
introducing different binding for subset of devices is _not_ a fix.

> > What about the multi function devices? They should have same binding.
> 
> The MFD devices defined are not in contention here only the SFD.

I'd like to see common solutions for SFD and MFD, as the hardware is
similar, and that includes the code. Having code that is easier to
maintain is important, and having many drivers are harder to maintain
than one driver.

Milo's code looks better than yours in that regard. I disagree about
Milo's code being "nightmare" to modify, and care about "easy to
maintain" more than "binary size".

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

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Pavel Machek]

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

Hi!

> All be it the analog side of these devices may be the same but the digital side for each chip is different.
> The register maps are not the same, the number of supported LED strings are not
> the same, there are features not supported by this driver and will
> be a pain to add.

Digital side looks similar enough that Milo was able to share code
between them.

Yes, obviously register map will be different.

> Adding features will be a complete night mare especially for the LM3632 which also support Torch and Flash.
> 
> 
> Here are where a single driver will start getting messy and support difficult
> LM3533 has ALS and an ADC for an ALS analog sensor
> LM3631 has no ALS functionality
> LM3632 has strobe/torch functionality and no ramp support
> LM3633 has lvled support coupled with the hvled support
> LM3695 does not even appear to be available publicly
> LM3697 is the only device that that this driver could be used for as is.

We already been through this once. More than one of these has ramp
support, and more than one has overvoltage protection. (Plus, all of
them are LED drivers).

Surely non-ugly design can be done where the code is shared?

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

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

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Jacek Anaszewski]

Hi Pavel,

On 09/14/2018 10:18 AM, Pavel Machek wrote:
> Hi!
> 
>>>> How do I politely explain that the original implementation was wrong for certain devices?
>>>
>>> Implementation? Device tree is hardware description.
>>
>> Yes this hardware description is incorrect.  The hardware description is
>> describing a MFD but this LED driver (and a couple others) only perform
>> 1 function and that is to drive a LED string.
> 
> So what? Does not seem incorrect to me. Maybe the description should
> not be in MFD directory, but other than that...
> 
>>>> Isn't code and documentation supposed to be pushed in stages
>>>> together?
>>>
>>> Device tree is _not_ documentation. And yes, it is normally pushed
>>> together. But that did not happen here, and bindings are already in.
>>
>> Hmm..  Its not documentation but it is in the Documentation folder.
>> And just because the bindings are in does not mean they cannot be
>> changed.
> 
> You may want to learn more about device tree and/or talk to the device
> tree maintainers. This is an old article. https://lwn.net/Articles/561462/

The article title is "Device trees as ABI". A device tree is defined
in the "*.dts" file that is then compiled to a dtb blob, which
constitutes the ABI. And this ABI should be kept backwards compatible.

What is discussed here is a documentation of bindings, i.e. according
to ePAPR: "requirements for how specific types and classes of devices
are represented in the device tree".

From the bindings documented in the
Documentation/devicetree/bindings/mfd/ti-lmu.txt only
ti,lm3532-backlight is used in the mainline dts file
(arch/arm/boot/dts/omap4-droid4-xt894.dts).

Having the above it seems that there is no risk of breaking any
users.

> NAK on this patch. I see that this binding has problems, but
> introducing different binding for subset of devices is _not_ a fix.
> 
>>> What about the multi function devices? They should have same binding.
>>
>> The MFD devices defined are not in contention here only the SFD.
> 
> I'd like to see common solutions for SFD and MFD, as the hardware is
> similar, and that includes the code. Having code that is easier to
> maintain is important, and having many drivers are harder to maintain
> than one driver.
> 
> Milo's code looks better than yours in that regard. I disagree about
> Milo's code being "nightmare" to modify, and care about "easy to
> maintain" more than "binary size".

Easy to maintain will be a dedicated LED class driver.

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Pavel Machek]

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

Hi!

> > You may want to learn more about device tree and/or talk to the device
> > tree maintainers. This is an old article. https://lwn.net/Articles/561462/
> 
> The article title is "Device trees as ABI". A device tree is defined
> in the "*.dts" file that is then compiled to a dtb blob, which
> constitutes the ABI. And this ABI should be kept backwards compatible.
> 
> What is discussed here is a documentation of bindings, i.e. according
> to ePAPR: "requirements for how specific types and classes of devices
> are represented in the device tree".
> 
> >From the bindings documented in the
> Documentation/devicetree/bindings/mfd/ti-lmu.txt only
> ti,lm3532-backlight is used in the mainline dts file
> (arch/arm/boot/dts/omap4-droid4-xt894.dts).
> 
> Having the above it seems that there is no risk of breaking any
> users.

DTBs and bindings are supposed to be portable between operating
systems. You are right there are no _mainline_ _Linux_ users.

> > NAK on this patch. I see that this binding has problems, but
> > introducing different binding for subset of devices is _not_ a fix.
> > 
> >>> What about the multi function devices? They should have same binding.
> >>
> >> The MFD devices defined are not in contention here only the SFD.
> > 
> > I'd like to see common solutions for SFD and MFD, as the hardware is
> > similar, and that includes the code. Having code that is easier to
> > maintain is important, and having many drivers are harder to maintain
> > than one driver.
> > 
> > Milo's code looks better than yours in that regard. I disagree about
> > Milo's code being "nightmare" to modify, and care about "easy to
> > maintain" more than "binary size".
> 
> Easy to maintain will be a dedicated LED class driver.

You mean, 3 dedicated LED class drivers and 3 MFD drivers with LED
parts? We'll need complex driver anyway, and I'd really like to have
just one.

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

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

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Jacek Anaszewski]

Hi Pavel.

On 09/14/2018 11:42 PM, Pavel Machek wrote:
> Hi!
> 
>>> You may want to learn more about device tree and/or talk to the device
>>> tree maintainers. This is an old article. https://lwn.net/Articles/561462/
>>
>> The article title is "Device trees as ABI". A device tree is defined
>> in the "*.dts" file that is then compiled to a dtb blob, which
>> constitutes the ABI. And this ABI should be kept backwards compatible.
>>
>> What is discussed here is a documentation of bindings, i.e. according
>> to ePAPR: "requirements for how specific types and classes of devices
>> are represented in the device tree".
>>
>> >From the bindings documented in the
>> Documentation/devicetree/bindings/mfd/ti-lmu.txt only
>> ti,lm3532-backlight is used in the mainline dts file
>> (arch/arm/boot/dts/omap4-droid4-xt894.dts).
>>
>> Having the above it seems that there is no risk of breaking any
>> users.
> 
> DTBs and bindings are supposed to be portable between operating
> systems. You are right there are no _mainline_ _Linux_ users.

No mainline users means no users we should care of.
Other people also don't care - see patch [0].

>>> NAK on this patch. I see that this binding has problems, but
>>> introducing different binding for subset of devices is _not_ a fix.
>>>
>>>>> What about the multi function devices? They should have same binding.
>>>>
>>>> The MFD devices defined are not in contention here only the SFD.
>>>
>>> I'd like to see common solutions for SFD and MFD, as the hardware is
>>> similar, and that includes the code. Having code that is easier to
>>> maintain is important, and having many drivers are harder to maintain
>>> than one driver.
>>>
>>> Milo's code looks better than yours in that regard. I disagree about
>>> Milo's code being "nightmare" to modify, and care about "easy to
>>> maintain" more than "binary size".
>>
>> Easy to maintain will be a dedicated LED class driver.
> 
> You mean, 3 dedicated LED class drivers and 3 MFD drivers with LED
> parts? We'll need complex driver anyway, and I'd really like to have
> just one.

In the LED subsystem we can wrap common functionalities
into a library object. MFD driver will be able to reuse it then.

[0]
https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/commit/?h=droid4-pending-v4.19&id=d774c7e447ac911e73a1b3c775e6d89f0422218c

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Dan Murphy]

Jacek

On 09/15/2018 03:00 PM, Jacek Anaszewski wrote:
> Hi Pavel.
> 
> On 09/14/2018 11:42 PM, Pavel Machek wrote:
>> Hi!
>>
>>>> You may want to learn more about device tree and/or talk to the device
>>>> tree maintainers. This is an old article. https://lwn.net/Articles/561462/
>>>
>>> The article title is "Device trees as ABI". A device tree is defined
>>> in the "*.dts" file that is then compiled to a dtb blob, which
>>> constitutes the ABI. And this ABI should be kept backwards compatible.
>>>
>>> What is discussed here is a documentation of bindings, i.e. according
>>> to ePAPR: "requirements for how specific types and classes of devices
>>> are represented in the device tree".
>>>
>>> >From the bindings documented in the
>>> Documentation/devicetree/bindings/mfd/ti-lmu.txt only
>>> ti,lm3532-backlight is used in the mainline dts file
>>> (arch/arm/boot/dts/omap4-droid4-xt894.dts).
>>>
>>> Having the above it seems that there is no risk of breaking any
>>> users.
>>
>> DTBs and bindings are supposed to be portable between operating
>> systems. You are right there are no _mainline_ _Linux_ users.
> 
> No mainline users means no users we should care of.
> Other people also don't care - see patch [0].
> 
>>>> NAK on this patch. I see that this binding has problems, but
>>>> introducing different binding for subset of devices is _not_ a fix.
>>>>
>>>>>> What about the multi function devices? They should have same binding.
>>>>>
>>>>> The MFD devices defined are not in contention here only the SFD.
>>>>
>>>> I'd like to see common solutions for SFD and MFD, as the hardware is
>>>> similar, and that includes the code. Having code that is easier to
>>>> maintain is important, and having many drivers are harder to maintain
>>>> than one driver.
>>>>
>>>> Milo's code looks better than yours in that regard. I disagree about
>>>> Milo's code being "nightmare" to modify, and care about "easy to
>>>> maintain" more than "binary size".
>>>
>>> Easy to maintain will be a dedicated LED class driver.
>>
>> You mean, 3 dedicated LED class drivers and 3 MFD drivers with LED
>> parts? We'll need complex driver anyway, and I'd really like to have
>> just one.
> 
> In the LED subsystem we can wrap common functionalities
> into a library object. MFD driver will be able to reuse it then.

I am currently working on that code now.  I expect a RFC on this this week.

Dan

> 
> [0]
> https://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git/commit/?h=droid4-pending-v4.19&id=d774c7e447ac911e73a1b3c775e6d89f0422218c
> 


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

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Jacek Anaszewski]

Dan,

On 09/17/2018 05:24 PM, Dan Murphy wrote:
> Jacek
> 
> On 09/15/2018 03:00 PM, Jacek Anaszewski wrote:
>> Hi Pavel.
>>
>> On 09/14/2018 11:42 PM, Pavel Machek wrote:
>>> Hi!
>>>
>>>>> You may want to learn more about device tree and/or talk to the device
>>>>> tree maintainers. This is an old article. https://lwn.net/Articles/561462/
>>>>
>>>> The article title is "Device trees as ABI". A device tree is defined
>>>> in the "*.dts" file that is then compiled to a dtb blob, which
>>>> constitutes the ABI. And this ABI should be kept backwards compatible.
>>>>
>>>> What is discussed here is a documentation of bindings, i.e. according
>>>> to ePAPR: "requirements for how specific types and classes of devices
>>>> are represented in the device tree".
>>>>
>>>> >From the bindings documented in the
>>>> Documentation/devicetree/bindings/mfd/ti-lmu.txt only
>>>> ti,lm3532-backlight is used in the mainline dts file
>>>> (arch/arm/boot/dts/omap4-droid4-xt894.dts).
>>>>
>>>> Having the above it seems that there is no risk of breaking any
>>>> users.
>>>
>>> DTBs and bindings are supposed to be portable between operating
>>> systems. You are right there are no _mainline_ _Linux_ users.
>>
>> No mainline users means no users we should care of.
>> Other people also don't care - see patch [0].
>>
>>>>> NAK on this patch. I see that this binding has problems, but
>>>>> introducing different binding for subset of devices is _not_ a fix.
>>>>>
>>>>>>> What about the multi function devices? They should have same binding.
>>>>>>
>>>>>> The MFD devices defined are not in contention here only the SFD.
>>>>>
>>>>> I'd like to see common solutions for SFD and MFD, as the hardware is
>>>>> similar, and that includes the code. Having code that is easier to
>>>>> maintain is important, and having many drivers are harder to maintain
>>>>> than one driver.
>>>>>
>>>>> Milo's code looks better than yours in that regard. I disagree about
>>>>> Milo's code being "nightmare" to modify, and care about "easy to
>>>>> maintain" more than "binary size".
>>>>
>>>> Easy to maintain will be a dedicated LED class driver.
>>>
>>> You mean, 3 dedicated LED class drivers and 3 MFD drivers with LED
>>> parts? We'll need complex driver anyway, and I'd really like to have
>>> just one.
>>
>> In the LED subsystem we can wrap common functionalities
>> into a library object. MFD driver will be able to reuse it then.
> 
> I am currently working on that code now.  I expect a RFC on this this week.

Will it affect the shape of the driver for lm3697 as of v7 [0]?
Or the patch set [0] state should be deemed "awaiting merge"?

[0] https://lkml.org/lkml/2018/9/11/760

-- 
Best regards,
Jacek Anaszewski

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Dan Murphy]

Jacek

On 09/17/2018 02:22 PM, Jacek Anaszewski wrote:
> Dan,
> 
> On 09/17/2018 05:24 PM, Dan Murphy wrote:
>> Jacek
>>
>> On 09/15/2018 03:00 PM, Jacek Anaszewski wrote:
>>> Hi Pavel.
>>>
>>> On 09/14/2018 11:42 PM, Pavel Machek wrote:
>>>> Hi!
>>>>
>>>>>> You may want to learn more about device tree and/or talk to the device
>>>>>> tree maintainers. This is an old article. https://lwn.net/Articles/561462/
>>>>>
>>>>> The article title is "Device trees as ABI". A device tree is defined
>>>>> in the "*.dts" file that is then compiled to a dtb blob, which
>>>>> constitutes the ABI. And this ABI should be kept backwards compatible.
>>>>>
>>>>> What is discussed here is a documentation of bindings, i.e. according
>>>>> to ePAPR: "requirements for how specific types and classes of devices
>>>>> are represented in the device tree".
>>>>>
>>>>> >From the bindings documented in the
>>>>> Documentation/devicetree/bindings/mfd/ti-lmu.txt only
>>>>> ti,lm3532-backlight is used in the mainline dts file
>>>>> (arch/arm/boot/dts/omap4-droid4-xt894.dts).
>>>>>
>>>>> Having the above it seems that there is no risk of breaking any
>>>>> users.
>>>>
>>>> DTBs and bindings are supposed to be portable between operating
>>>> systems. You are right there are no _mainline_ _Linux_ users.
>>>
>>> No mainline users means no users we should care of.
>>> Other people also don't care - see patch [0].
>>>
>>>>>> NAK on this patch. I see that this binding has problems, but
>>>>>> introducing different binding for subset of devices is _not_ a fix.
>>>>>>
>>>>>>>> What about the multi function devices? They should have same binding.
>>>>>>>
>>>>>>> The MFD devices defined are not in contention here only the SFD.
>>>>>>
>>>>>> I'd like to see common solutions for SFD and MFD, as the hardware is
>>>>>> similar, and that includes the code. Having code that is easier to
>>>>>> maintain is important, and having many drivers are harder to maintain
>>>>>> than one driver.
>>>>>>
>>>>>> Milo's code looks better than yours in that regard. I disagree about
>>>>>> Milo's code being "nightmare" to modify, and care about "easy to
>>>>>> maintain" more than "binary size".
>>>>>
>>>>> Easy to maintain will be a dedicated LED class driver.
>>>>
>>>> You mean, 3 dedicated LED class drivers and 3 MFD drivers with LED
>>>> parts? We'll need complex driver anyway, and I'd really like to have
>>>> just one.
>>>
>>> In the LED subsystem we can wrap common functionalities
>>> into a library object. MFD driver will be able to reuse it then.
>>
>> I am currently working on that code now.  I expect a RFC on this this week.
> 
> Will it affect the shape of the driver for lm3697 as of v7 [0]?
> Or the patch set [0] state should be deemed "awaiting merge"?
> 

It will probably affect the driver to call into the library.  We can pull it in and
modify or we can modify after the common code is there.

It should not affect the bindings though.

Dan

> [0] https://lkml.org/lkml/2018/9/11/760
> 


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

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Pavel Machek]

Hi!

> >>> Easy to maintain will be a dedicated LED class driver.
> >>
> >> You mean, 3 dedicated LED class drivers and 3 MFD drivers with LED
> >> parts? We'll need complex driver anyway, and I'd really like to have
> >> just one.
> > 
> > In the LED subsystem we can wrap common functionalities
> > into a library object. MFD driver will be able to reuse it then.
> 
> I am currently working on that code now.  I expect a RFC on this this week.

Looking forward to that.

But you really need acks for the bindings, and since Rob is usually quite slow
acking them, it is easiest to use the existing binding... if it is wrong it
needs to be fixed, anyway.

THanks,

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

Re: [PATCH v7 1/6] dt-bindings: ti-lmu: Remove LM3697

[Dan Murphy]

Pavel

On 09/20/2018 05:04 PM, Pavel Machek wrote:
> Hi!
> 
>>>>> Easy to maintain will be a dedicated LED class driver.
>>>>
>>>> You mean, 3 dedicated LED class drivers and 3 MFD drivers with LED
>>>> parts? We'll need complex driver anyway, and I'd really like to have
>>>> just one.
>>>
>>> In the LED subsystem we can wrap common functionalities
>>> into a library object. MFD driver will be able to reuse it then.
>>
>> I am currently working on that code now.  I expect a RFC on this this week.
> 
> Looking forward to that.
> 
> But you really need acks for the bindings, and since Rob is usually quite slow
> acking them, it is easiest to use the existing binding... if it is wrong it
> needs to be fixed, anyway.
> 

OK I am in the midst of creating the code now. I have the common code done I just need to
make sure that it scales to other devices in the list.

The bindings will need to be updated to follow the LED bindings binding style.

I am wondering for the non-MFD parts if we should move the bindings to the LED
directory to make them easier to find.

Dan 

> THanks,
> 
> 									Pavel
> 


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

Re: [PATCH v7 3/6] dt-bindings: leds: Add bindings for lm3697 driver

[Rob Herring]

On Tue, Sep 11, 2018 at 12:08:22PM -0500, Dan Murphy wrote:
> Add the device tree bindings for the lm3697
> LED driver for backlighting and display.
> 
> Signed-off-by: Dan Murphy <dmurphy@ti.com>
> ---
> 
> v7 - Fix compatible from 3967 to 3697, and other comments. - https://lore.kernel.org/patchwork/patch/982550/
> 
> v6 - Fix minor issues - https://lore.kernel.org/patchwork/patch/975387/
> v5 - Fix the comment for the example - https://lore.kernel.org/patchwork/patch/975060/
> v4 - Removed HVLED definition in favor of HVLED place definition - https://lore.kernel.org/patchwork/patch/974812/
> v3 - Updated subject with prefered title - https://lore.kernel.org/patchwork/patch/972337/
> v2 - Fixed subject and patch commit message - https://lore.kernel.org/patchwork/patch/971326/
> 
>  .../devicetree/bindings/leds/leds-lm3697.txt  | 86 +++++++++++++++++++
>  1 file changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v7 3/6] dt-bindings: leds: Add bindings for lm3697 driver

[Pavel Machek]

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

Hi!

> > Add the device tree bindings for the lm3697
> > LED driver for backlighting and display.
> > 
> > Signed-off-by: Dan Murphy <dmurphy@ti.com>

> >  .../devicetree/bindings/leds/leds-lm3697.txt  | 86 +++++++++++++++++++
> >  1 file changed, 86 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

We already have binding for lm3697 in
Documentation/devicetree/bindings/mfd/ti-lmu.txt . Is it good idea to
have second one?
								Pavel

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

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

Re: [PATCH v7 3/6] dt-bindings: leds: Add bindings for lm3697 driver

[Rob Herring]

On Mon, Sep 24, 2018 at 1:02 PM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > > Add the device tree bindings for the lm3697
> > > LED driver for backlighting and display.
> > >
> > > Signed-off-by: Dan Murphy <dmurphy@ti.com>
>
> > >  .../devicetree/bindings/leds/leds-lm3697.txt  | 86 +++++++++++++++++++
> > >  1 file changed, 86 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
>
> We already have binding for lm3697 in
> Documentation/devicetree/bindings/mfd/ti-lmu.txt . Is it good idea to
> have second one?

Of course not. Now that you mention it, I do remember seeing some
discussion on this.

Rob

Re: [PATCH v7 3/6] dt-bindings: leds: Add bindings for lm3697 driver

[Dan Murphy]

Rob/Pavel/Jacek

On 09/25/2018 02:39 PM, Rob Herring wrote:
> On Mon, Sep 24, 2018 at 1:02 PM Pavel Machek <pavel@ucw.cz> wrote:
>>
>> Hi!
>>
>>>> Add the device tree bindings for the lm3697
>>>> LED driver for backlighting and display.
>>>>
>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com>
>>
>>>>  .../devicetree/bindings/leds/leds-lm3697.txt  | 86 +++++++++++++++++++
>>>>  1 file changed, 86 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3697.txt
>>>
>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>
>> We already have binding for lm3697 in
>> Documentation/devicetree/bindings/mfd/ti-lmu.txt . Is it good idea to
>> have second one?
> 
> Of course not. Now that you mention it, I do remember seeing some
> discussion on this.
> 
> Rob
> 

I will be posting a RFC tomorrow with all the changes that shows 3 devices
attached to the common code along with extending out the features on the LED
drivers themselves.

Its not complete or tested code but it should give a basis for discussion.  This way I
can do some other code while this is being discussed.

Dan

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