* [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).