dt: bindings: lm3601x: Introduce the lm3601x driver

Introduce the family of LED devices that can
drive a torch, strobe or IR LED.

The LED driver can be configured with a strobe
timer to execute a strobe flash. The IR LED
brightness is controlled via the torch brightness
register.

The data sheet for each the LM36010 and LM36011
LED drivers can be found here:
http://www.ti.com/product/LM36010
http://www.ti.com/product/LM36011

Signed-off-by Dan Murphy <[email protected]>

---

v6 - This driver has been heavily modified from v5. There is no longer reading of individual child nodes. There are too many changes to list here see -
https://patchwork.kernel.org/patch/10392123/

v5 - Fixed magic numbers, change reg cache type, added of_put_node to release
the dt node ref, and I did not change the remove function to leave the LED in its
state on driver removal - https://patchwork.kernel.org/patch/10391741/
v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/
v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed
RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type, fixed unlock and extra semi colon in strobe_set, removed unnecessary out label
in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/
v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/

  drivers/leds/Kconfig        |   9 +
  drivers/leds/Makefile       |   1 +
  drivers/leds/leds-lm3601x.c | 595 ++++++++++++++++++++++++++++++++++++
  3 files changed, 605 insertions(+)

create mode 100644 drivers/leds/leds-lm3601x.c

drivers/leds/Kconfig +9 -0

index 2c896c0e69e1..50ae536f343f 100644
@@ -145,6 +145,15 @@ config LEDS_LM3692X
  	  This option enables support for the TI LM3692x family
  	  of white LED string drivers used for backlighting.
 
+config LEDS_LM3601X
+	tristate "LED support for LM3601x Chips"
+	depends on LEDS_CLASS && I2C && OF
+	depends on LEDS_CLASS_FLASH
+	select REGMAP_I2C
+	help
+	  This option enables support for the TI LM3601x family
+	  of flash, torch and indicator classes.
+
  config LEDS_LOCOMO
  	tristate "LED Support for Locomo device"
  	depends on LEDS_CLASS

drivers/leds/Makefile +1 -0

index 91eca81cae82..b79807fe1b67 100644
@@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG)		+= leds-mlxreg.o
  obj-$(CONFIG_LEDS_NIC78BX)		+= leds-nic78bx.o
  obj-$(CONFIG_LEDS_MT6323)		+= leds-mt6323.o
  obj-$(CONFIG_LEDS_LM3692X)		+= leds-lm3692x.o
+obj-$(CONFIG_LEDS_LM3601X)		+= leds-lm3601x.o
 
  # LED SPI Drivers
  obj-$(CONFIG_LEDS_DAC124S085)		+= leds-dac124s085.o

drivers/leds/leds-lm3601x.c new file +370 -0

new file mode 100644
index 000000000000..fa87da5d5159
@@ -0,0 +1,595 @@
+// SPDX-License-Identifier: GPL-2.0
+// Flash and torch driver for Texas Instruments LM3601X LED
+// Flash driver chip family
+// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/led-class-flash.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <uapi/linux/uleds.h>
+
+#define LM3601X_LED_TORCH	0x0
+#define LM3601X_LED_IR		0x1
+
+/* Registers */
+#define LM3601X_ENABLE_REG	0x01
+#define LM3601X_CFG_REG		0x02
+#define LM3601X_LED_FLASH_REG	0x03
+#define LM3601X_LED_TORCH_REG	0x04
+#define LM3601X_FLAGS_REG	0x05
+#define LM3601X_DEV_ID_REG	0x06
+
+#define LM3601X_SW_RESET	BIT(7)
+
+/* Enable Mode bits */
+#define LM3601X_MODE_STANDBY	0x00
+#define LM3601X_MODE_IR_DRV	BIT(0)
+#define LM3601X_MODE_TORCH	BIT(1)
+#define LM3601X_MODE_STROBE	(BIT(0) | BIT(1))
+#define LM3601X_STRB_EN		BIT(2)
+#define LM3601X_STRB_LVL_TRIG	~BIT(3)
+#define LM3601X_STRB_EDGE_TRIG	BIT(3)
+#define LM3601X_IVFM_EN		BIT(4)
+
+#define LM36010_BOOST_LIMIT_19	~BIT(5)
+#define LM36010_BOOST_LIMIT_28	BIT(5)
+#define LM36010_BOOST_FREQ_2MHZ	~BIT(6)
+#define LM36010_BOOST_FREQ_4MHZ	BIT(6)
+#define LM36010_BOOST_MODE_NORM	~BIT(7)
+#define LM36010_BOOST_MODE_PASS	BIT(7)
+
+/* Flag Mask */
+#define LM3601X_FLASH_TIME_OUT	BIT(0)
+#define LM3601X_UVLO_FAULT	BIT(1)
+#define LM3601X_THERM_SHUTDOWN	BIT(2)
+#define LM3601X_THERM_CURR	BIT(3)
+#define LM36010_CURR_LIMIT	BIT(4)
+#define LM3601X_SHORT_FAULT	BIT(5)
+#define LM3601X_IVFM_TRIP	BIT(6)
+#define LM36010_OVP_FAULT	BIT(7)
+
+#define LM3601X_MIN_TORCH_I_UA	2400
+#define LM3601X_MIN_STROBE_I_MA	11
+
+#define LM3601X_TIMEOUT_MASK	0x1e
+#define LM3601X_ENABLE_MASK	0x03
+
+enum lm3601x_type {
+	CHIP_LM36010 = 0,
+	CHIP_LM36011,
+};
+
+/**
+ * struct lm3601x_max_timeouts -
+ * @timeout: timeout value in ms
+ * @regval: the value of the register to write
+ */
+struct lm3601x_max_timeouts {
+	int timeout;
+	int reg_val;
+};
+
+/**
+ * struct lm3601x_led -
+ * @lock: Lock for reading/writing the device
+ * @regmap: Devices register map
+ * @client: Pointer to the I2C client
+ * @led_node: DT device node for the led
+ * @cdev_torch: led class device pointer for the torch
+ * @cdev_ir: led class device pointer for infrared
+ * @fled_cdev: flash led class device pointer
+ * @led_name: LED label for the Torch or IR LED
+ * @strobe: LED label for the strobe
+ * @last_flag: last known flags register value
+ * @strobe_timeout: the timeout for the strobe
+ * @torch_current_max: maximum current for the torch
+ * @strobe_current_max: maximum current for the strobe
+ * @max_strobe_timeout: maximum timeout for the strobe
+ * @led_mode: The mode to enable either IR or Torch
+ */
+struct lm3601x_led {
+	struct mutex lock;
+	struct regmap *regmap;
+	struct i2c_client *client;
+
+	struct device_node *led_node;
+
+	struct led_classdev cdev_torch;
+	struct led_classdev cdev_ir;
+
+	struct led_classdev_flash fled_cdev;
+
+	char led_name[LED_MAX_NAME_SIZE];
+	char strobe[LED_MAX_NAME_SIZE];
+
+	unsigned int last_flag;
+	unsigned int strobe_timeout;
+
+	u32 torch_current_max;
+	u32 strobe_current_max;
+	u32 max_strobe_timeout;
+
+	int led_mode;
+};
+
+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+	{ 40000, 0x00 },
+	{ 80000, 0x01 },
+	{ 120000, 0x02 },
+	{ 160000, 0x03 },
+	{ 200000, 0x04 },
+	{ 240000, 0x05 },
+	{ 280000, 0x06 },
+	{ 320000, 0x07 },
+	{ 360000, 0x08 },
+	{ 400000, 0x09 },
+	{ 600000, 0x0a },
+	{ 800000, 0x0b },
+	{ 1000000, 0x0c },
+	{ 1200000, 0x0d },
+	{ 1400000, 0x0e },
+	{ 1600000, 0x0f },
+};
+
+static const struct reg_default lm3601x_regmap_defs[] = {
+	{ LM3601X_ENABLE_REG, 0x20 },
+	{ LM3601X_CFG_REG, 0x15 },
+	{ LM3601X_LED_FLASH_REG, 0x00 },
+	{ LM3601X_LED_TORCH_REG, 0x00 },
+};
+
+static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case LM3601X_FLAGS_REG:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config lm3601x_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+
+	.max_register = LM3601X_DEV_ID_REG,
+	.reg_defaults = lm3601x_regmap_defs,
+	.num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs),
+	.cache_type = REGCACHE_RBTREE,
+	.volatile_reg = lm3601x_volatile_reg,
+};
+
+static struct lm3601x_led *fled_cdev_to_led(
+				struct led_classdev_flash *fled_cdev)
+{
+	return container_of(fled_cdev, struct lm3601x_led, fled_cdev);
+}
+
+static int lm3601x_read_faults(struct lm3601x_led *led)
+{
+	int flags_val;
+	int ret;
+
+	ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val);
+	if (ret < 0)
+		return -EIO;
+
+	led->last_flag = 0;
+
+	if (flags_val & LM36010_OVP_FAULT)
+		led->last_flag |= LED_FAULT_OVER_VOLTAGE;
+
+	if (flags_val & (LM3601X_THERM_SHUTDOWN | LM3601X_THERM_CURR))
+		led->last_flag |= LED_FAULT_OVER_TEMPERATURE;
+
+	if (flags_val & LM3601X_SHORT_FAULT)
+		led->last_flag |= LED_FAULT_SHORT_CIRCUIT;
+
+	if (flags_val & LM36010_CURR_LIMIT)
+		led->last_flag |= LED_FAULT_OVER_CURRENT;
+
+	if (flags_val & LM3601X_UVLO_FAULT)
+		led->last_flag |= LED_FAULT_UNDER_VOLTAGE;
+
+	if (flags_val & LM3601X_IVFM_TRIP)
+		led->last_flag |= LED_FAULT_INPUT_VOLTAGE;
+
+	if (flags_val & LM3601X_THERM_SHUTDOWN)
+		led->last_flag |= LED_FAULT_LED_OVER_TEMPERATURE;
+
+	return led->last_flag;
+}
+
+static int lm3601x_brightness_set(struct led_classdev *cdev,
+					enum led_brightness brightness)
+{
+	struct lm3601x_led *led =
+	    container_of(cdev, struct lm3601x_led, cdev_torch);
+	u8 brightness_val;
+	int ret, led_mode_val;
+
+	mutex_lock(&led->lock);
+
+	ret = lm3601x_read_faults(led);
+	if (ret < 0)
+		goto out;
+
+	if (led->led_mode == LM3601X_LED_TORCH)
+		led_mode_val = LM3601X_MODE_TORCH;
+	else
+		led_mode_val = LM3601X_MODE_IR_DRV;
+
+	if (brightness == LED_OFF) {
+		ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+					led_mode_val, LED_OFF);
+		goto out;
+	}
+
+	if (brightness == LED_ON)
+		brightness_val = LED_ON;
+	else
+		brightness_val = (brightness/2);
+
+	ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val);
+	if (ret < 0)
+		goto out;
+
+	ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+					led_mode_val,
+					led_mode_val);
+
+out:
+	mutex_unlock(&led->lock);
+	return ret;
+}
+
+static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev,
+				bool state)
+{
+	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+	int ret;
+	int current_timeout;
+	int reg_count;
+	int i;
+	int timeout_reg_val = 0;
+
+	mutex_lock(&led->lock);
+
+	ret = regmap_read(led->regmap, LM3601X_CFG_REG, &current_timeout);
+	if (ret < 0)
+		goto out;
+
+	reg_count = ARRAY_SIZE(strobe_timeouts);
+	for (i = 0; i < reg_count; i++) {
+		if (led->strobe_timeout > strobe_timeouts[i].timeout)
+			continue;
+
+		if (led->strobe_timeout <= strobe_timeouts[i].timeout) {
+			timeout_reg_val = (strobe_timeouts[i].reg_val << 1);
+			break;
+		}
+
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (led->strobe_timeout != current_timeout)
+		ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG,
+					LM3601X_TIMEOUT_MASK, timeout_reg_val);
+
+	if (state)
+		ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+					LM3601X_MODE_STROBE,
+					LM3601X_MODE_STROBE);
+	else
+		ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+					LM3601X_MODE_STROBE, LED_OFF);
+
+	ret = lm3601x_read_faults(led);
+out:
+	mutex_unlock(&led->lock);
+	return ret;
+}
+
+static int lm3601x_strobe_brightness_set(struct led_classdev *cdev,
+					 enum led_brightness brightness)
+{
+	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
+	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+	int ret;
+	u8 brightness_val;
+
+	mutex_lock(&led->lock);
+	ret = lm3601x_read_faults(led);
+	if (ret < 0)
+		goto out;
+
+	if (brightness == LED_OFF) {
+		ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+					LM3601X_MODE_STROBE, LED_OFF);
+		goto out;
+	}
+
+	if (brightness == LED_ON)
+		brightness_val = LED_ON;
+	else
+		brightness_val = (brightness/2);
+
+	ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val);
+
+out:
+	mutex_unlock(&led->lock);
+	return ret;
+}
+
+static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev,
+				u32 timeout)
+{
+	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+	int ret = 0;
+
+	mutex_lock(&led->lock);
+
+	led->strobe_timeout = timeout;
+
+	mutex_unlock(&led->lock);
+
+	return ret;
+}
+
+static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev,
+				bool *state)
+{
+	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+	int ret;
+	int strobe_state;
+
+	mutex_lock(&led->lock);
+
+	ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state);
+	if (ret < 0)
+		goto out;
+
+	*state = strobe_state & LM3601X_MODE_STROBE;
+
+out:
+	mutex_unlock(&led->lock);
+	return ret;
+}
+
+static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev,
+				u32 *fault)
+{
+	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+
+	lm3601x_read_faults(led);
+

drivers/leds/leds-lm3601x.c new file +44 -0

+	return err;
+}
+
+static void lm3601x_init_flash_timeout(struct lm3601x_led *led)
+{
+	struct led_flash_setting *setting;
+
+	setting = &led->fled_cdev.timeout;
+	setting->min = strobe_timeouts[0].timeout;
+	setting->max = led->max_strobe_timeout;
+	setting->step = 40;
+	setting->val = led->max_strobe_timeout;
+}
+
+static int lm3601x_parse_node(struct lm3601x_led *led,
+			      struct device_node *node)
+{
+	struct device_node *child_node;
+	const char *name;
+	char *mode_name;
+	int ret = -ENODEV;
+
+	for_each_available_child_of_node(node, child_node) {
+		led->led_node = of_node_get(child_node);
+		if (!led->led_node) {
+			dev_err(&led->client->dev,
+				"No LED Child node\n");
+
+			goto out_err;
+		}
+
+		ret = of_property_read_u32(led->led_node, "led-sources",
+					   &led->led_mode);
+		if (ret) {
+			dev_err(&led->client->dev,
+				"led-sources DT property missing\n");
+			goto out_err;
+		}
+
+		if (led->led_mode < LM3601X_LED_TORCH ||
+		    led->led_mode > LM3601X_LED_IR) {
+			dev_warn(&led->client->dev,
+				"Invalid led mode requested\n");
+

drivers/leds/leds-lm3601x.c new file +13 -0

+		else
+			snprintf(led->led_name, sizeof(led->led_name),
+				"%s:torch", led->led_node->name);
+
+		ret = of_property_read_u32(led->led_node, "led-max-microamp",
+					&led->torch_current_max);
+		if (ret < 0) {
+			dev_warn(&led->client->dev,
+				"led-max-microamp DT property missing\n");
+
+			goto out_err;
+		}
+

drivers/leds/leds-lm3601x.c new file +55 -0

+
+	ret = of_property_read_u32(led->led_node,
+				"flash-max-microamp",
+				&led->strobe_current_max);
+	if (ret < 0) {
+		dev_warn(&led->client->dev,
+			 "flash-max-microamp DT property missing\n");
+		goto out_err;
+	}
+
+	ret = of_property_read_u32(led->led_node,
+				"flash-max-timeout-us",
+				&led->max_strobe_timeout);
+	if (ret < 0) {
+		dev_warn(&led->client->dev,
+			 "flash-max-timeout-us DT property missing\n");
+
+		goto out_err;
+	}
+
+	lm3601x_init_flash_timeout(led);
+
+out_err:
+	of_node_put(led->led_node);
+	return ret;
+}
+
+static int lm3601x_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct lm3601x_led *led;
+	int err;
+
+	led = devm_kzalloc(&client->dev,
+			    sizeof(struct lm3601x_led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	err = lm3601x_parse_node(led, client->dev.of_node);
+	if (err < 0)
+		return -ENODEV;
+
+	led->client = client;
+	led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap);
+	if (IS_ERR(led->regmap)) {
+		err = PTR_ERR(led->regmap);
+		dev_err(&client->dev,
+			"Failed to allocate register map: %d\n", err);
+		return err;
+	}
+
+	mutex_init(&led->lock);
+	i2c_set_clientdata(client, led);
+	err = lm3601x_register_leds(led);
+

drivers/leds/leds-lm3601x.c new file +32 -0

+
+	return 0;
+}
+
+static const struct i2c_device_id lm3601x_id[] = {
+	{ "LM36010", CHIP_LM36010 },
+	{ "LM36011", CHIP_LM36011 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, lm3601x_id);
+
+static const struct of_device_id of_lm3601x_leds_match[] = {
+	{ .compatible = "ti,lm36010", },
+	{ .compatible = "ti,lm36011", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match);
+
+static struct i2c_driver lm3601x_i2c_driver = {
+	.driver = {
+		.name = "lm3601x",
+		.of_match_table = of_lm3601x_leds_match,
+	},
+	.probe = lm3601x_probe,
+	.remove = lm3601x_remove,
+	.id_table = lm3601x_id,
+};
+module_i2c_driver(lm3601x_i2c_driver);
+
+MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X");
+MODULE_AUTHOR("Dan Murphy <[email protected]>");
+MODULE_LICENSE("GPL v2");

Dan,

Thanks for the update. Please refer to my comments below.

Quoted 446 lines · Dan Murphy

Introduce the family of LED devices that can
drive a torch, strobe or IR LED.

The LED driver can be configured with a strobe
timer to execute a strobe flash.  The IR LED
brightness is controlled via the torch brightness
register.

The data sheet for each the LM36010 and LM36011
LED drivers can be found here:
http://www.ti.com/product/LM36010
http://www.ti.com/product/LM36011

Signed-off-by Dan Murphy <[email protected]>

---

v6 - This driver has been heavily modified from v5.  There is no longer reading of individual child nodes.  There are too many changes to list here see -
https://patchwork.kernel.org/patch/10392123/

v5 - Fixed magic numbers, change reg cache type, added of_put_node to release
the dt node ref, and I did not change the remove function to leave the LED in its
state on driver removal - https://patchwork.kernel.org/patch/10391741/
v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/
v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed
RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type, fixed unlock and extra semi colon in strobe_set, removed unnecessary out label
in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/
v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/

  drivers/leds/Kconfig        |   9 +
  drivers/leds/Makefile       |   1 + drivers/leds/leds-lm3601x.c | 595 ++++++++++++++++++++++++++++++++++++

  3 files changed, 605 insertions(+)

  create mode 100644 drivers/leds/leds-lm3601x.c

drivers/leds/Kconfig +0 -0

index 2c896c0e69e1..50ae536f343f 100644
@@ -145,6 +145,15 @@ config LEDS_LM3692X

        This option enables support for the TI LM3692x family of white LED string drivers used for backlighting. +config LEDS_LM3601X
+    tristate "LED support for LM3601x Chips"
+    depends on LEDS_CLASS && I2C && OF
+    depends on LEDS_CLASS_FLASH
+    select REGMAP_I2C
+    help
+      This option enables support for the TI LM3601x family +      of flash, torch and indicator classes.
+
  config LEDS_LOCOMO
      tristate "LED Support for Locomo device" depends on LEDS_CLASS

drivers/leds/Makefile +0 -0

index 91eca81cae82..b79807fe1b67 100644
@@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG)        += leds-mlxreg.o

  obj-$(CONFIG_LEDS_NIC78BX)        += leds-nic78bx.o obj-$(CONFIG_LEDS_MT6323)        += leds-mt6323.o obj-$(CONFIG_LEDS_LM3692X)        += leds-lm3692x.o +obj-$(CONFIG_LEDS_LM3601X)        += leds-lm3601x.o # LED SPI Drivers obj-$(CONFIG_LEDS_DAC124S085)        += leds-dac124s085.o

drivers/leds/leds-lm3601x.c new file +374 -0

new file mode 100644
index 000000000000..fa87da5d5159
@@ -0,0 +1,595 @@
+// SPDX-License-Identifier: GPL-2.0
+// Flash and torch driver for Texas Instruments LM3601X LED
+// Flash driver chip family
+// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/led-class-flash.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <uapi/linux/uleds.h>
+
+#define LM3601X_LED_TORCH    0x0
+#define LM3601X_LED_IR        0x1
+
+/* Registers */
+#define LM3601X_ENABLE_REG    0x01
+#define LM3601X_CFG_REG        0x02
+#define LM3601X_LED_FLASH_REG    0x03
+#define LM3601X_LED_TORCH_REG    0x04
+#define LM3601X_FLAGS_REG    0x05
+#define LM3601X_DEV_ID_REG    0x06
+
+#define LM3601X_SW_RESET    BIT(7)
+
+/* Enable Mode bits */
+#define LM3601X_MODE_STANDBY    0x00
+#define LM3601X_MODE_IR_DRV    BIT(0)
+#define LM3601X_MODE_TORCH    BIT(1)
+#define LM3601X_MODE_STROBE    (BIT(0) | BIT(1))
+#define LM3601X_STRB_EN        BIT(2)
+#define LM3601X_STRB_LVL_TRIG    ~BIT(3)
+#define LM3601X_STRB_EDGE_TRIG    BIT(3)
+#define LM3601X_IVFM_EN        BIT(4)
+
+#define LM36010_BOOST_LIMIT_19    ~BIT(5)
+#define LM36010_BOOST_LIMIT_28    BIT(5)
+#define LM36010_BOOST_FREQ_2MHZ    ~BIT(6)
+#define LM36010_BOOST_FREQ_4MHZ    BIT(6)
+#define LM36010_BOOST_MODE_NORM    ~BIT(7)
+#define LM36010_BOOST_MODE_PASS    BIT(7)
+
+/* Flag Mask */
+#define LM3601X_FLASH_TIME_OUT    BIT(0)
+#define LM3601X_UVLO_FAULT    BIT(1)
+#define LM3601X_THERM_SHUTDOWN    BIT(2)
+#define LM3601X_THERM_CURR    BIT(3)
+#define LM36010_CURR_LIMIT    BIT(4)
+#define LM3601X_SHORT_FAULT    BIT(5)
+#define LM3601X_IVFM_TRIP    BIT(6)
+#define LM36010_OVP_FAULT    BIT(7)
+
+#define LM3601X_MIN_TORCH_I_UA    2400
+#define LM3601X_MIN_STROBE_I_MA    11
+
+#define LM3601X_TIMEOUT_MASK    0x1e
+#define LM3601X_ENABLE_MASK    0x03
+
+enum lm3601x_type {
+    CHIP_LM36010 = 0,
+    CHIP_LM36011,
+};
+
+/**
+ * struct lm3601x_max_timeouts -
+ * @timeout: timeout value in ms
+ * @regval: the value of the register to write
+ */
+struct lm3601x_max_timeouts {
+    int timeout;
+    int reg_val;
+};
+
+/**
+ * struct lm3601x_led -
+ * @lock: Lock for reading/writing the device
+ * @regmap: Devices register map
+ * @client: Pointer to the I2C client
+ * @led_node: DT device node for the led
+ * @cdev_torch: led class device pointer for the torch
+ * @cdev_ir: led class device pointer for infrared
+ * @fled_cdev: flash led class device pointer
+ * @led_name: LED label for the Torch or IR LED
+ * @strobe: LED label for the strobe
+ * @last_flag: last known flags register value
+ * @strobe_timeout: the timeout for the strobe
+ * @torch_current_max: maximum current for the torch
+ * @strobe_current_max: maximum current for the strobe
+ * @max_strobe_timeout: maximum timeout for the strobe
+ * @led_mode: The mode to enable either IR or Torch
+ */
+struct lm3601x_led {
+    struct mutex lock;
+    struct regmap *regmap;
+    struct i2c_client *client;
+
+    struct device_node *led_node;
+
+    struct led_classdev cdev_torch;
+    struct led_classdev cdev_ir;
+
+    struct led_classdev_flash fled_cdev;
+
+    char led_name[LED_MAX_NAME_SIZE];
+    char strobe[LED_MAX_NAME_SIZE];
+
+    unsigned int last_flag;
+    unsigned int strobe_timeout;
+
+    u32 torch_current_max;
+    u32 strobe_current_max;
+    u32 max_strobe_timeout;
+
+    int led_mode;
+};
+
+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+    { 40000, 0x00 },
+    { 80000, 0x01 },
+    { 120000, 0x02 },
+    { 160000, 0x03 },
+    { 200000, 0x04 },
+    { 240000, 0x05 },
+    { 280000, 0x06 },
+    { 320000, 0x07 },
+    { 360000, 0x08 },
+    { 400000, 0x09 },
+    { 600000, 0x0a },
+    { 800000, 0x0b },
+    { 1000000, 0x0c },
+    { 1200000, 0x0d },
+    { 1400000, 0x0e },
+    { 1600000, 0x0f },
+};
+
+static const struct reg_default lm3601x_regmap_defs[] = {
+    { LM3601X_ENABLE_REG, 0x20 },
+    { LM3601X_CFG_REG, 0x15 },
+    { LM3601X_LED_FLASH_REG, 0x00 },
+    { LM3601X_LED_TORCH_REG, 0x00 },
+};
+
+static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg)
+{
+    switch (reg) {
+    case LM3601X_FLAGS_REG:
+        return true;
+    default:
+        return false;
+    }
+}
+
+static const struct regmap_config lm3601x_regmap = {
+    .reg_bits = 8,
+    .val_bits = 8,
+
+    .max_register = LM3601X_DEV_ID_REG,
+    .reg_defaults = lm3601x_regmap_defs,
+    .num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs),
+    .cache_type = REGCACHE_RBTREE,
+    .volatile_reg = lm3601x_volatile_reg,
+};
+
+static struct lm3601x_led *fled_cdev_to_led(
+                struct led_classdev_flash *fled_cdev)
+{
+    return container_of(fled_cdev, struct lm3601x_led, fled_cdev);
+}
+
+static int lm3601x_read_faults(struct lm3601x_led *led)
+{
+    int flags_val;
+    int ret;
+
+    ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val);
+    if (ret < 0)
+        return -EIO;
+
+    led->last_flag = 0;
+
+    if (flags_val & LM36010_OVP_FAULT)
+        led->last_flag |= LED_FAULT_OVER_VOLTAGE;
+
+    if (flags_val & (LM3601X_THERM_SHUTDOWN | LM3601X_THERM_CURR))
+        led->last_flag |= LED_FAULT_OVER_TEMPERATURE;
+
+    if (flags_val & LM3601X_SHORT_FAULT)
+        led->last_flag |= LED_FAULT_SHORT_CIRCUIT;
+
+    if (flags_val & LM36010_CURR_LIMIT)
+        led->last_flag |= LED_FAULT_OVER_CURRENT;
+
+    if (flags_val & LM3601X_UVLO_FAULT)
+        led->last_flag |= LED_FAULT_UNDER_VOLTAGE;
+
+    if (flags_val & LM3601X_IVFM_TRIP)
+        led->last_flag |= LED_FAULT_INPUT_VOLTAGE;
+
+    if (flags_val & LM3601X_THERM_SHUTDOWN)
+        led->last_flag |= LED_FAULT_LED_OVER_TEMPERATURE;
+
+    return led->last_flag;
+}
+
+static int lm3601x_brightness_set(struct led_classdev *cdev,
+                    enum led_brightness brightness)
+{
+    struct lm3601x_led *led =
+        container_of(cdev, struct lm3601x_led, cdev_torch);
+    u8 brightness_val;
+    int ret, led_mode_val;
+
+    mutex_lock(&led->lock);
+
+    ret = lm3601x_read_faults(led);
+    if (ret < 0)
+        goto out;
+
+    if (led->led_mode == LM3601X_LED_TORCH)
+        led_mode_val = LM3601X_MODE_TORCH;
+    else
+        led_mode_val = LM3601X_MODE_IR_DRV;
+
+    if (brightness == LED_OFF) {
+        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                    led_mode_val, LED_OFF);
+        goto out;
+    }
+
+    if (brightness == LED_ON)
+        brightness_val = LED_ON;
+    else
+        brightness_val = (brightness/2);
+
+    ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val);
+    if (ret < 0)
+        goto out;
+
+    ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                    led_mode_val,
+                    led_mode_val);
+
+out:
+    mutex_unlock(&led->lock);
+    return ret;
+}
+
+static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev,
+                bool state)
+{
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+    int ret;
+    int current_timeout;
+    int reg_count;
+    int i;
+    int timeout_reg_val = 0;
+
+    mutex_lock(&led->lock);
+
+    ret = regmap_read(led->regmap, LM3601X_CFG_REG, &current_timeout);
+    if (ret < 0)
+        goto out;
+
+    reg_count = ARRAY_SIZE(strobe_timeouts);
+    for (i = 0; i < reg_count; i++) {
+        if (led->strobe_timeout > strobe_timeouts[i].timeout)
+            continue;
+
+        if (led->strobe_timeout <= strobe_timeouts[i].timeout) {
+            timeout_reg_val = (strobe_timeouts[i].reg_val << 1);
+            break;
+        }
+
+        ret = -EINVAL;
+        goto out;
+    }
+
+    if (led->strobe_timeout != current_timeout)
+        ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG,
+                    LM3601X_TIMEOUT_MASK, timeout_reg_val);
+
+    if (state)
+        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                    LM3601X_MODE_STROBE,
+                    LM3601X_MODE_STROBE);
+    else
+        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                    LM3601X_MODE_STROBE, LED_OFF);
+
+    ret = lm3601x_read_faults(led);
+out:
+    mutex_unlock(&led->lock);
+    return ret;
+}
+
+static int lm3601x_strobe_brightness_set(struct led_classdev *cdev,
+                     enum led_brightness brightness)
+{
+    struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+    int ret;
+    u8 brightness_val;
+
+    mutex_lock(&led->lock);
+    ret = lm3601x_read_faults(led);
+    if (ret < 0)
+        goto out;
+
+    if (brightness == LED_OFF) {
+        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                    LM3601X_MODE_STROBE, LED_OFF);
+        goto out;
+    }
+
+    if (brightness == LED_ON)
+        brightness_val = LED_ON;
+    else
+        brightness_val = (brightness/2);
+
+    ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val);
+
+out:
+    mutex_unlock(&led->lock);
+    return ret;
+}
+
+static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev,
+                u32 timeout)
+{
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+    int ret = 0;
+
+    mutex_lock(&led->lock);
+
+    led->strobe_timeout = timeout;
+
+    mutex_unlock(&led->lock);
+
+    return ret;
+}
+
+static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev,
+                bool *state)
+{
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+    int ret;
+    int strobe_state;
+
+    mutex_lock(&led->lock);
+
+    ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state);
+    if (ret < 0)
+        goto out;
+
+    *state = strobe_state & LM3601X_MODE_STROBE;
+
+out:
+    mutex_unlock(&led->lock);
+    return ret;
+}
+
+static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev,
+                u32 *fault)
+{
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+
+    lm3601x_read_faults(led);
+
+    *fault = led->last_flag;
+
+    return 0;
+}

drivers/leds/leds-lm3601x.c line 380

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +6 -0

+
+static const struct led_flash_ops strobe_ops = {
+    .strobe_set        = lm3601x_strobe_set,
+    .strobe_get        = lm3601x_strobe_get,
+    .timeout_set        = lm3601x_strobe_timeout_set,
+    .fault_get        = lm3601x_strobe_fault_get,

You're still missing flash_brightness_set here.

drivers/leds/leds-lm3601x.c line 390

Quoted 11 lines

drivers/leds/leds-lm3601x.c new file +10 -0

+};
+
+static int lm3601x_register_leds(struct lm3601x_led *led)
+{
+    struct led_classdev_flash *fled_cdev;
+    struct led_classdev *led_cdev;
+    int err = -ENODEV;
+
+    led->cdev_torch.name = led->led_name;
+    led->cdev_torch.max_brightness = LED_FULL;

This should be led->torch_current_max converted to brightness levels. Please compare how leds-max77693 and leds-as3645a do that.

drivers/leds/leds-lm3601x.c line 395

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +5 -0

+    led->cdev_torch.brightness_set_blocking = lm3601x_brightness_set;
+    err = devm_led_classdev_register(&led->client->dev,
+            &led->cdev_torch);
+    if (err < 0)
+        return err;

You shouldn't register two LED class devices for a single DT child node, and honestly I don't know why you would have to do it.

drivers/leds/leds-lm3601x.c line 407

Quoted 13 lines

drivers/leds/leds-lm3601x.c new file +12 -0

+
+    fled_cdev = &led->fled_cdev;
+    fled_cdev->ops = &strobe_ops;
+
+    led_cdev = &fled_cdev->led_cdev;
+    led_cdev->name = led->strobe;
+    led_cdev->max_brightness = LED_FUL
+    led_cdev->brightness_set_blocking = lm3601x_strobe_brightness_set;
+    led_cdev->flags |= LED_DEV_CAP_FLASH;
+
+    err = led_classdev_flash_register(&led->client->dev,
+            fled_cdev);

Please look into the implementation of led_classdev_flash_register() and compare other LED flash class drivers. What prevents you from applying the same approach?

To put it concisely: LED flash class wrapper was designed to allow for operating on a single LED in two modes: torch and flash. Torch brightness is governed by brightness file and flash brightness is governed by flash_brightness file. The difference between the two modes is that torch mode is activated by writing value greater than 0 to the brightness file and flash mode is activated by writing 1 to the strobe_set file.

In other words flash_brightness overwrites the (torch) brightness for the time equal to the flash_timeout or until 0 is written to the strobe_set file.

Quoted 49 lines

drivers/leds/leds-lm3601x.c new file +48 -0

+    return err;
+}
+
+static void lm3601x_init_flash_timeout(struct lm3601x_led *led)
+{
+    struct led_flash_setting *setting;
+
+    setting = &led->fled_cdev.timeout;
+    setting->min = strobe_timeouts[0].timeout;
+    setting->max = led->max_strobe_timeout;
+    setting->step = 40;
+    setting->val = led->max_strobe_timeout;
+}
+
+static int lm3601x_parse_node(struct lm3601x_led *led,
+                  struct device_node *node)
+{
+    struct device_node *child_node;
+    const char *name;
+    char *mode_name;
+    int ret = -ENODEV;
+
+    for_each_available_child_of_node(node, child_node) {
+        led->led_node = of_node_get(child_node);
+        if (!led->led_node) {
+            dev_err(&led->client->dev,
+                "No LED Child node\n");
+
+            goto out_err;
+        }
+
+        ret = of_property_read_u32(led->led_node, "led-sources",
+                       &led->led_mode);
+        if (ret) {
+            dev_err(&led->client->dev,
+                "led-sources DT property missing\n");
+            goto out_err;
+        }
+
+        if (led->led_mode < LM3601X_LED_TORCH ||
+            led->led_mode > LM3601X_LED_IR) {
+            dev_warn(&led->client->dev,
+                "Invalid led mode requested\n");
+
+            goto out_err;
+
+        }
+    }

drivers/leds/leds-lm3601x.c line 462

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +6 -0

+
+    if (led->led_mode == LM3601X_LED_TORCH) {
+        ret = of_property_read_string(led->led_node, "label", &name);
+        if (!ret)
+            snprintf(led->led_name, sizeof(led->led_name),
+                "%s:%s", led->led_node->name, name);

First segment of a LED class device name should be "devicename".

Quoted 18 lines

drivers/leds/leds-lm3601x.c new file +17 -0

+        else
+            snprintf(led->led_name, sizeof(led->led_name),
+                "%s:torch", led->led_node->name);
+
+        ret = of_property_read_u32(led->led_node, "led-max-microamp",
+                    &led->torch_current_max);
+        if (ret < 0) {
+            dev_warn(&led->client->dev,
+                "led-max-microamp DT property missing\n");
+
+            goto out_err;
+        }
+
+        mode_name = "torch";
+
+    } else if (led->led_mode == LM3601X_LED_IR) {
+        ret = of_property_read_string(led->led_node, "label", &name);

drivers/leds/leds-lm3601x.c line 485

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +6 -0

+        if (!ret)
+            snprintf(led->led_name, sizeof(led->led_name),
+                "%s:%s", led->led_node->name, name);
+        else
+            snprintf(led->led_name, sizeof(led->led_name),
+                "%s::infrared", led->led_node->name);

Ditto.

Quoted 4 lines

drivers/leds/leds-lm3601x.c new file +3 -0

+
+        mode_name = "ir";
+

drivers/leds/leds-lm3601x.c line 498

Quoted 10 lines

drivers/leds/leds-lm3601x.c new file +10 -0

+    } else {
+        dev_warn(&led->client->dev,
+            "No LED mode is selected exiting probe\n");
+
+        goto out_err;
+    }
+
+    /* Flash mode is available in IR or Torch mode so read the DT */
+    snprintf(led->strobe, sizeof(led->strobe),
+            "%s:%s:strobe", led->led_node->name, mode_name);

strobe is not a proper LED function name (it is not a noun). Please use "flash" instead.

Quoted 60 lines

drivers/leds/leds-lm3601x.c new file +59 -0

+
+    ret = of_property_read_u32(led->led_node,
+                "flash-max-microamp",
+                &led->strobe_current_max);
+    if (ret < 0) {
+        dev_warn(&led->client->dev,
+             "flash-max-microamp DT property missing\n");
+        goto out_err;
+    }
+
+    ret = of_property_read_u32(led->led_node,
+                "flash-max-timeout-us",
+                &led->max_strobe_timeout);
+    if (ret < 0) {
+        dev_warn(&led->client->dev,
+             "flash-max-timeout-us DT property missing\n");
+
+        goto out_err;
+    }
+
+    lm3601x_init_flash_timeout(led);
+
+out_err:
+    of_node_put(led->led_node);
+    return ret;
+}
+
+static int lm3601x_probe(struct i2c_client *client,
+            const struct i2c_device_id *id)
+{
+    struct lm3601x_led *led;
+    int err;
+
+    led = devm_kzalloc(&client->dev,
+                sizeof(struct lm3601x_led), GFP_KERNEL);
+    if (!led)
+        return -ENOMEM;
+
+    err = lm3601x_parse_node(led, client->dev.of_node);
+    if (err < 0)
+        return -ENODEV;
+
+    led->client = client;
+    led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap);
+    if (IS_ERR(led->regmap)) {
+        err = PTR_ERR(led->regmap);
+        dev_err(&client->dev,
+            "Failed to allocate register map: %d\n", err);
+        return err;
+    }
+
+    mutex_init(&led->lock);
+    i2c_set_clientdata(client, led);
+    err = lm3601x_register_leds(led);
+
+    return err;
+}
+
+static int lm3601x_remove(struct i2c_client *client)

drivers/leds/leds-lm3601x.c line 563

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +6 -0

+{
+    struct lm3601x_led *led = i2c_get_clientdata(client);
+
+    regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+               LM3601X_ENABLE_MASK,
+               LM3601X_MODE_STANDBY);

You need also mutex_destroy(&led->lock) here.

Quoted 34 lines

drivers/leds/leds-lm3601x.c new file +32 -0

+
+    return 0;
+}
+
+static const struct i2c_device_id lm3601x_id[] = {
+    { "LM36010", CHIP_LM36010 },
+    { "LM36011", CHIP_LM36011 },
+    { },
+};
+MODULE_DEVICE_TABLE(i2c, lm3601x_id);
+
+static const struct of_device_id of_lm3601x_leds_match[] = {
+    { .compatible = "ti,lm36010", },
+    { .compatible = "ti,lm36011", },
+    {},
+};
+MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match);
+
+static struct i2c_driver lm3601x_i2c_driver = {
+    .driver = {
+        .name = "lm3601x",
+        .of_match_table = of_lm3601x_leds_match,
+    },
+    .probe = lm3601x_probe,
+    .remove = lm3601x_remove,
+    .id_table = lm3601x_id,
+};
+module_i2c_driver(lm3601x_i2c_driver);
+
+MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X");
+MODULE_AUTHOR("Dan Murphy <[email protected]>");
+MODULE_LICENSE("GPL v2");

Jacek

Quoted 451 lines · Jacek Anaszewski

Dan,

Thanks for the update. Please refer to my comments below.

Quoted 446 lines · Dan Murphy

Introduce the family of LED devices that can
drive a torch, strobe or IR LED.

The LED driver can be configured with a strobe
timer to execute a strobe flash.  The IR LED
brightness is controlled via the torch brightness
register.

The data sheet for each the LM36010 and LM36011
LED drivers can be found here:
http://www.ti.com/product/LM36010
http://www.ti.com/product/LM36011

Signed-off-by Dan Murphy <[email protected]>

---

v6 - This driver has been heavily modified from v5.  There is no longer reading of individual child nodes.  There are too many changes to list here see -
https://patchwork.kernel.org/patch/10392123/

v5 - Fixed magic numbers, change reg cache type, added of_put_node to release
the dt node ref, and I did not change the remove function to leave the LED in its
state on driver removal - https://patchwork.kernel.org/patch/10391741/
v4 - Fixed Cocci issue using ARRAY_SIZE - https://patchwork.kernel.org/patch/10389259/
v3 - removed wildcard dt compatible, fixed copyright, fixed struct doc, removed
RO registers from default, added regmap volatile for FLAGS_REG, updated regmap cache type, fixed unlock and extra semi colon in strobe_set, removed unnecessary out label
in led register and fixed checking of the ret in brightness_set - https://patchwork.kernel.org/patch/10386243/
v2 - Fixed kbuild issue and removed unused cdev_strobe - https://patchwork.kernel.org/patch/10384585/

   drivers/leds/Kconfig        |   9 +
   drivers/leds/Makefile       |   1 +
   drivers/leds/leds-lm3601x.c | 595 ++++++++++++++++++++++++++++++++++++
   3 files changed, 605 insertions(+)

  create mode 100644 drivers/leds/leds-lm3601x.c

drivers/leds/Kconfig +8 -0

index 2c896c0e69e1..50ae536f343f 100644
@@ -145,6 +145,15 @@ config LEDS_LM3692X
         This option enables support for the TI LM3692x family
         of white LED string drivers used for backlighting.
   +config LEDS_LM3601X
+    tristate "LED support for LM3601x Chips"
+    depends on LEDS_CLASS && I2C && OF
+    depends on LEDS_CLASS_FLASH
+    select REGMAP_I2C
+    help
+      This option enables support for the TI LM3601x family
+      of flash, torch and indicator classes.
+
   config LEDS_LOCOMO
       tristate "LED Support for Locomo device"
       depends on LEDS_CLASS

drivers/leds/Makefile +1 -0

index 91eca81cae82..b79807fe1b67 100644
@@ -76,6 +76,7 @@ obj-$(CONFIG_LEDS_MLXREG)        += leds-mlxreg.o
   obj-$(CONFIG_LEDS_NIC78BX)        += leds-nic78bx.o
   obj-$(CONFIG_LEDS_MT6323)        += leds-mt6323.o
   obj-$(CONFIG_LEDS_LM3692X)        += leds-lm3692x.o
+obj-$(CONFIG_LEDS_LM3601X)        += leds-lm3601x.o
     # LED SPI Drivers
   obj-$(CONFIG_LEDS_DAC124S085)        += leds-dac124s085.o

drivers/leds/leds-lm3601x.c new file +374 -0

new file mode 100644
index 000000000000..fa87da5d5159
@@ -0,0 +1,595 @@
+// SPDX-License-Identifier: GPL-2.0
+// Flash and torch driver for Texas Instruments LM3601X LED
+// Flash driver chip family
+// Copyright (C) 2018 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/leds.h>
+#include <linux/led-class-flash.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+#include <uapi/linux/uleds.h>
+
+#define LM3601X_LED_TORCH    0x0
+#define LM3601X_LED_IR        0x1
+
+/* Registers */
+#define LM3601X_ENABLE_REG    0x01
+#define LM3601X_CFG_REG        0x02
+#define LM3601X_LED_FLASH_REG    0x03
+#define LM3601X_LED_TORCH_REG    0x04
+#define LM3601X_FLAGS_REG    0x05
+#define LM3601X_DEV_ID_REG    0x06
+
+#define LM3601X_SW_RESET    BIT(7)
+
+/* Enable Mode bits */
+#define LM3601X_MODE_STANDBY    0x00
+#define LM3601X_MODE_IR_DRV    BIT(0)
+#define LM3601X_MODE_TORCH    BIT(1)
+#define LM3601X_MODE_STROBE    (BIT(0) | BIT(1))
+#define LM3601X_STRB_EN        BIT(2)
+#define LM3601X_STRB_LVL_TRIG    ~BIT(3)
+#define LM3601X_STRB_EDGE_TRIG    BIT(3)
+#define LM3601X_IVFM_EN        BIT(4)
+
+#define LM36010_BOOST_LIMIT_19    ~BIT(5)
+#define LM36010_BOOST_LIMIT_28    BIT(5)
+#define LM36010_BOOST_FREQ_2MHZ    ~BIT(6)
+#define LM36010_BOOST_FREQ_4MHZ    BIT(6)
+#define LM36010_BOOST_MODE_NORM    ~BIT(7)
+#define LM36010_BOOST_MODE_PASS    BIT(7)
+
+/* Flag Mask */
+#define LM3601X_FLASH_TIME_OUT    BIT(0)
+#define LM3601X_UVLO_FAULT    BIT(1)
+#define LM3601X_THERM_SHUTDOWN    BIT(2)
+#define LM3601X_THERM_CURR    BIT(3)
+#define LM36010_CURR_LIMIT    BIT(4)
+#define LM3601X_SHORT_FAULT    BIT(5)
+#define LM3601X_IVFM_TRIP    BIT(6)
+#define LM36010_OVP_FAULT    BIT(7)
+
+#define LM3601X_MIN_TORCH_I_UA    2400
+#define LM3601X_MIN_STROBE_I_MA    11
+
+#define LM3601X_TIMEOUT_MASK    0x1e
+#define LM3601X_ENABLE_MASK    0x03
+
+enum lm3601x_type {
+    CHIP_LM36010 = 0,
+    CHIP_LM36011,
+};
+
+/**
+ * struct lm3601x_max_timeouts -
+ * @timeout: timeout value in ms
+ * @regval: the value of the register to write
+ */
+struct lm3601x_max_timeouts {
+    int timeout;
+    int reg_val;
+};
+
+/**
+ * struct lm3601x_led -
+ * @lock: Lock for reading/writing the device
+ * @regmap: Devices register map
+ * @client: Pointer to the I2C client
+ * @led_node: DT device node for the led
+ * @cdev_torch: led class device pointer for the torch
+ * @cdev_ir: led class device pointer for infrared
+ * @fled_cdev: flash led class device pointer
+ * @led_name: LED label for the Torch or IR LED
+ * @strobe: LED label for the strobe
+ * @last_flag: last known flags register value
+ * @strobe_timeout: the timeout for the strobe
+ * @torch_current_max: maximum current for the torch
+ * @strobe_current_max: maximum current for the strobe
+ * @max_strobe_timeout: maximum timeout for the strobe
+ * @led_mode: The mode to enable either IR or Torch
+ */
+struct lm3601x_led {
+    struct mutex lock;
+    struct regmap *regmap;
+    struct i2c_client *client;
+
+    struct device_node *led_node;
+
+    struct led_classdev cdev_torch;
+    struct led_classdev cdev_ir;
+
+    struct led_classdev_flash fled_cdev;
+
+    char led_name[LED_MAX_NAME_SIZE];
+    char strobe[LED_MAX_NAME_SIZE];
+
+    unsigned int last_flag;
+    unsigned int strobe_timeout;
+
+    u32 torch_current_max;
+    u32 strobe_current_max;
+    u32 max_strobe_timeout;
+
+    int led_mode;
+};
+
+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+    { 40000, 0x00 },
+    { 80000, 0x01 },
+    { 120000, 0x02 },
+    { 160000, 0x03 },
+    { 200000, 0x04 },
+    { 240000, 0x05 },
+    { 280000, 0x06 },
+    { 320000, 0x07 },
+    { 360000, 0x08 },
+    { 400000, 0x09 },
+    { 600000, 0x0a },
+    { 800000, 0x0b },
+    { 1000000, 0x0c },
+    { 1200000, 0x0d },
+    { 1400000, 0x0e },
+    { 1600000, 0x0f },
+};
+
+static const struct reg_default lm3601x_regmap_defs[] = {
+    { LM3601X_ENABLE_REG, 0x20 },
+    { LM3601X_CFG_REG, 0x15 },
+    { LM3601X_LED_FLASH_REG, 0x00 },
+    { LM3601X_LED_TORCH_REG, 0x00 },
+};
+
+static bool lm3601x_volatile_reg(struct device *dev, unsigned int reg)
+{
+    switch (reg) {
+    case LM3601X_FLAGS_REG:
+        return true;
+    default:
+        return false;
+    }
+}
+
+static const struct regmap_config lm3601x_regmap = {
+    .reg_bits = 8,
+    .val_bits = 8,
+
+    .max_register = LM3601X_DEV_ID_REG,
+    .reg_defaults = lm3601x_regmap_defs,
+    .num_reg_defaults = ARRAY_SIZE(lm3601x_regmap_defs),
+    .cache_type = REGCACHE_RBTREE,
+    .volatile_reg = lm3601x_volatile_reg,
+};
+
+static struct lm3601x_led *fled_cdev_to_led(
+                struct led_classdev_flash *fled_cdev)
+{
+    return container_of(fled_cdev, struct lm3601x_led, fled_cdev);
+}
+
+static int lm3601x_read_faults(struct lm3601x_led *led)
+{
+    int flags_val;
+    int ret;
+
+    ret = regmap_read(led->regmap, LM3601X_FLAGS_REG, &flags_val);
+    if (ret < 0)
+        return -EIO;
+
+    led->last_flag = 0;
+
+    if (flags_val & LM36010_OVP_FAULT)
+        led->last_flag |= LED_FAULT_OVER_VOLTAGE;
+
+    if (flags_val & (LM3601X_THERM_SHUTDOWN | LM3601X_THERM_CURR))
+        led->last_flag |= LED_FAULT_OVER_TEMPERATURE;
+
+    if (flags_val & LM3601X_SHORT_FAULT)
+        led->last_flag |= LED_FAULT_SHORT_CIRCUIT;
+
+    if (flags_val & LM36010_CURR_LIMIT)
+        led->last_flag |= LED_FAULT_OVER_CURRENT;
+
+    if (flags_val & LM3601X_UVLO_FAULT)
+        led->last_flag |= LED_FAULT_UNDER_VOLTAGE;
+
+    if (flags_val & LM3601X_IVFM_TRIP)
+        led->last_flag |= LED_FAULT_INPUT_VOLTAGE;
+
+    if (flags_val & LM3601X_THERM_SHUTDOWN)
+        led->last_flag |= LED_FAULT_LED_OVER_TEMPERATURE;
+
+    return led->last_flag;
+}
+
+static int lm3601x_brightness_set(struct led_classdev *cdev,
+                    enum led_brightness brightness)
+{
+    struct lm3601x_led *led =
+        container_of(cdev, struct lm3601x_led, cdev_torch);
+    u8 brightness_val;
+    int ret, led_mode_val;
+
+    mutex_lock(&led->lock);
+
+    ret = lm3601x_read_faults(led);
+    if (ret < 0)
+        goto out;
+
+    if (led->led_mode == LM3601X_LED_TORCH)
+        led_mode_val = LM3601X_MODE_TORCH;
+    else
+        led_mode_val = LM3601X_MODE_IR_DRV;
+
+    if (brightness == LED_OFF) {
+        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                    led_mode_val, LED_OFF);
+        goto out;
+    }
+
+    if (brightness == LED_ON)
+        brightness_val = LED_ON;
+    else
+        brightness_val = (brightness/2);
+
+    ret = regmap_write(led->regmap, LM3601X_LED_TORCH_REG, brightness_val);
+    if (ret < 0)
+        goto out;
+
+    ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                    led_mode_val,
+                    led_mode_val);
+
+out:
+    mutex_unlock(&led->lock);
+    return ret;
+}
+
+static int lm3601x_strobe_set(struct led_classdev_flash *fled_cdev,
+                bool state)
+{
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+    int ret;
+    int current_timeout;
+    int reg_count;
+    int i;
+    int timeout_reg_val = 0;
+
+    mutex_lock(&led->lock);
+
+    ret = regmap_read(led->regmap, LM3601X_CFG_REG, &current_timeout);
+    if (ret < 0)
+        goto out;
+
+    reg_count = ARRAY_SIZE(strobe_timeouts);
+    for (i = 0; i < reg_count; i++) {
+        if (led->strobe_timeout > strobe_timeouts[i].timeout)
+            continue;
+
+        if (led->strobe_timeout <= strobe_timeouts[i].timeout) {
+            timeout_reg_val = (strobe_timeouts[i].reg_val << 1);
+            break;
+        }
+
+        ret = -EINVAL;
+        goto out;
+    }
+
+    if (led->strobe_timeout != current_timeout)
+        ret = regmap_update_bits(led->regmap, LM3601X_CFG_REG,
+                    LM3601X_TIMEOUT_MASK, timeout_reg_val);
+
+    if (state)
+        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                    LM3601X_MODE_STROBE,
+                    LM3601X_MODE_STROBE);
+    else
+        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                    LM3601X_MODE_STROBE, LED_OFF);
+
+    ret = lm3601x_read_faults(led);
+out:
+    mutex_unlock(&led->lock);
+    return ret;
+}
+
+static int lm3601x_strobe_brightness_set(struct led_classdev *cdev,
+                     enum led_brightness brightness)
+{
+    struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+    int ret;
+    u8 brightness_val;
+
+    mutex_lock(&led->lock);
+    ret = lm3601x_read_faults(led);
+    if (ret < 0)
+        goto out;
+
+    if (brightness == LED_OFF) {
+        ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                    LM3601X_MODE_STROBE, LED_OFF);
+        goto out;
+    }
+
+    if (brightness == LED_ON)
+        brightness_val = LED_ON;
+    else
+        brightness_val = (brightness/2);
+
+    ret = regmap_write(led->regmap, LM3601X_LED_FLASH_REG, brightness_val);
+
+out:
+    mutex_unlock(&led->lock);
+    return ret;
+}
+
+static int lm3601x_strobe_timeout_set(struct led_classdev_flash *fled_cdev,
+                u32 timeout)
+{
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+    int ret = 0;
+
+    mutex_lock(&led->lock);
+
+    led->strobe_timeout = timeout;
+
+    mutex_unlock(&led->lock);
+
+    return ret;
+}
+
+static int lm3601x_strobe_get(struct led_classdev_flash *fled_cdev,
+                bool *state)
+{
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+    int ret;
+    int strobe_state;
+
+    mutex_lock(&led->lock);
+
+    ret = regmap_read(led->regmap, LM3601X_ENABLE_REG, &strobe_state);
+    if (ret < 0)
+        goto out;
+
+    *state = strobe_state & LM3601X_MODE_STROBE;
+
+out:
+    mutex_unlock(&led->lock);
+    return ret;
+}
+
+static int lm3601x_strobe_fault_get(struct led_classdev_flash *fled_cdev,
+                u32 *fault)
+{
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+
+    lm3601x_read_faults(led);
+
+    *fault = led->last_flag;
+
+    return 0;
+}

Quoted 8 lines

drivers/leds/leds-lm3601x.c line 380

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +6 -0

+
+static const struct led_flash_ops strobe_ops = {
+    .strobe_set        = lm3601x_strobe_set,
+    .strobe_get        = lm3601x_strobe_get,
+    .timeout_set        = lm3601x_strobe_timeout_set,
+    .fault_get        = lm3601x_strobe_fault_get,

You're still missing flash_brightness_set here.

OK not sure how this is working then.

Quoted 15 lines

drivers/leds/leds-lm3601x.c line 390

Quoted 12 lines

drivers/leds/leds-lm3601x.c new file +10 -0

+};
+
+static int lm3601x_register_leds(struct lm3601x_led *led)
+{
+    struct led_classdev_flash *fled_cdev;
+    struct led_classdev *led_cdev;
+    int err = -ENODEV;
+
+    led->cdev_torch.name = led->led_name;
+    led->cdev_torch.max_brightness = LED_FULL;

This should be led->torch_current_max converted to brightness levels. Please compare how leds-max77693 and leds-as3645a do that.

I will look into it.

Quoted 9 lines

drivers/leds/leds-lm3601x.c line 395

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +5 -0

+    led->cdev_torch.brightness_set_blocking = lm3601x_brightness_set;
+    err = devm_led_classdev_register(&led->client->dev,
+            &led->cdev_torch);
+    if (err < 0)
+        return err;

You shouldn't register two LED class devices for a single DT child node, and honestly I don't know why you would have to do it.

See below

Quoted 17 lines

drivers/leds/leds-lm3601x.c line 407

Quoted 13 lines

drivers/leds/leds-lm3601x.c new file +12 -0

+
+    fled_cdev = &led->fled_cdev;
+    fled_cdev->ops = &strobe_ops;
+
+    led_cdev = &fled_cdev->led_cdev;
+    led_cdev->name = led->strobe;
+    led_cdev->max_brightness = LED_FUL
+    led_cdev->brightness_set_blocking = lm3601x_strobe_brightness_set;
+    led_cdev->flags |= LED_DEV_CAP_FLASH;
+
+    err = led_classdev_flash_register(&led->client->dev,
+            fled_cdev);

Please look into the implementation of led_classdev_flash_register() and compare other LED flash class drivers. What prevents you from applying the same approach?

I did actually follow the same approach here as the as3645a driver. I used this driver since I have a data sheet and can read the code.

This is a copy, paste, slight modify from that as3645a driver as3645a_led_class_setup function.

I used the devm register function above as a variation.

I will have to dig further into it.

Quoted 9 lines

To put it concisely: LED flash class wrapper was designed to allow for operating on a single LED in two modes: torch and flash. Torch brightness is governed by brightness file and flash brightness is governed by flash_brightness file. The difference between the two modes is that torch mode is activated by writing value greater than 0 to the brightness file and flash mode is activated by writing 1 to the strobe_set file.

Quoted 3 lines

In other words flash_brightness overwrites the (torch) brightness for the time equal to the flash_timeout or until 0 is written to the strobe_set file.

I will look at it again

Quoted 49 lines

Quoted 49 lines

drivers/leds/leds-lm3601x.c new file +48 -0

+    return err;
+}
+
+static void lm3601x_init_flash_timeout(struct lm3601x_led *led)
+{
+    struct led_flash_setting *setting;
+
+    setting = &led->fled_cdev.timeout;
+    setting->min = strobe_timeouts[0].timeout;
+    setting->max = led->max_strobe_timeout;
+    setting->step = 40;
+    setting->val = led->max_strobe_timeout;
+}
+
+static int lm3601x_parse_node(struct lm3601x_led *led,
+                  struct device_node *node)
+{
+    struct device_node *child_node;
+    const char *name;
+    char *mode_name;
+    int ret = -ENODEV;
+
+    for_each_available_child_of_node(node, child_node) {
+        led->led_node = of_node_get(child_node);
+        if (!led->led_node) {
+            dev_err(&led->client->dev,
+                "No LED Child node\n");
+
+            goto out_err;
+        }
+
+        ret = of_property_read_u32(led->led_node, "led-sources",
+                       &led->led_mode);
+        if (ret) {
+            dev_err(&led->client->dev,
+                "led-sources DT property missing\n");
+            goto out_err;
+        }
+
+        if (led->led_mode < LM3601X_LED_TORCH ||
+            led->led_mode > LM3601X_LED_IR) {
+            dev_warn(&led->client->dev,
+                "Invalid led mode requested\n");
+
+            goto out_err;
+
+        }
+    }

Quoted 8 lines

drivers/leds/leds-lm3601x.c line 462

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +6 -0

+
+    if (led->led_mode == LM3601X_LED_TORCH) {
+        ret = of_property_read_string(led->led_node, "label", &name);
+        if (!ret)
+            snprintf(led->led_name, sizeof(led->led_name),
+                "%s:%s", led->led_node->name, name);

First segment of a LED class device name should be "devicename".

Hmm. I remember this conversation from my other drivers. I just picked the wrong dt node name.

I will fix it.

Quoted 18 lines

Quoted 18 lines

drivers/leds/leds-lm3601x.c new file +17 -0

+        else
+            snprintf(led->led_name, sizeof(led->led_name),
+                "%s:torch", led->led_node->name);
+
+        ret = of_property_read_u32(led->led_node, "led-max-microamp",
+                    &led->torch_current_max);
+        if (ret < 0) {
+            dev_warn(&led->client->dev,
+                "led-max-microamp DT property missing\n");
+
+            goto out_err;
+        }
+
+        mode_name = "torch";
+
+    } else if (led->led_mode == LM3601X_LED_IR) {
+        ret = of_property_read_string(led->led_node, "label", &name);

Quoted 8 lines

drivers/leds/leds-lm3601x.c line 485

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +6 -0

+        if (!ret)
+            snprintf(led->led_name, sizeof(led->led_name),
+                "%s:%s", led->led_node->name, name);
+        else
+            snprintf(led->led_name, sizeof(led->led_name),
+                "%s::infrared", led->led_node->name);

Ditto.

Ack

Quoted 17 lines

Quoted 4 lines

drivers/leds/leds-lm3601x.c new file +3 -0

+
+        mode_name = "ir";
+

drivers/leds/leds-lm3601x.c line 498

Quoted 10 lines

drivers/leds/leds-lm3601x.c new file +10 -0

+    } else {
+        dev_warn(&led->client->dev,
+            "No LED mode is selected exiting probe\n");
+
+        goto out_err;
+    }
+
+    /* Flash mode is available in IR or Torch mode so read the DT */
+    snprintf(led->strobe, sizeof(led->strobe),
+            "%s:%s:strobe", led->led_node->name, mode_name);

strobe is not a proper LED function name (it is not a noun). Please use "flash" instead.

Strobe can be used as a noun to define an electronic camera flash. But this is really a US English definition.

I can change it to flash

Quoted 59 lines

Quoted 59 lines

drivers/leds/leds-lm3601x.c new file +59 -0

+
+    ret = of_property_read_u32(led->led_node,
+                "flash-max-microamp",
+                &led->strobe_current_max);
+    if (ret < 0) {
+        dev_warn(&led->client->dev,
+             "flash-max-microamp DT property missing\n");
+        goto out_err;
+    }
+
+    ret = of_property_read_u32(led->led_node,
+                "flash-max-timeout-us",
+                &led->max_strobe_timeout);
+    if (ret < 0) {
+        dev_warn(&led->client->dev,
+             "flash-max-timeout-us DT property missing\n");
+
+        goto out_err;
+    }
+
+    lm3601x_init_flash_timeout(led);
+
+out_err:
+    of_node_put(led->led_node);
+    return ret;
+}
+
+static int lm3601x_probe(struct i2c_client *client,
+            const struct i2c_device_id *id)
+{
+    struct lm3601x_led *led;
+    int err;
+
+    led = devm_kzalloc(&client->dev,
+                sizeof(struct lm3601x_led), GFP_KERNEL);
+    if (!led)
+        return -ENOMEM;
+
+    err = lm3601x_parse_node(led, client->dev.of_node);
+    if (err < 0)
+        return -ENODEV;
+
+    led->client = client;
+    led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap);
+    if (IS_ERR(led->regmap)) {
+        err = PTR_ERR(led->regmap);
+        dev_err(&client->dev,
+            "Failed to allocate register map: %d\n", err);
+        return err;
+    }
+
+    mutex_init(&led->lock);
+    i2c_set_clientdata(client, led);
+    err = lm3601x_register_leds(led);
+
+    return err;
+}
+
+static int lm3601x_remove(struct i2c_client *client)

Quoted 8 lines

drivers/leds/leds-lm3601x.c line 563

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +6 -0

+{
+    struct lm3601x_led *led = i2c_get_clientdata(client);
+
+    regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+               LM3601X_ENABLE_MASK,
+               LM3601X_MODE_STANDBY);

You need also mutex_destroy(&led->lock) here.

OK.

Quoted 35 lines

Quoted 34 lines

drivers/leds/leds-lm3601x.c new file +32 -0

+
+    return 0;
+}
+
+static const struct i2c_device_id lm3601x_id[] = {
+    { "LM36010", CHIP_LM36010 },
+    { "LM36011", CHIP_LM36011 },
+    { },
+};
+MODULE_DEVICE_TABLE(i2c, lm3601x_id);
+
+static const struct of_device_id of_lm3601x_leds_match[] = {
+    { .compatible = "ti,lm36010", },
+    { .compatible = "ti,lm36011", },
+    {},
+};
+MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match);
+
+static struct i2c_driver lm3601x_i2c_driver = {
+    .driver = {
+        .name = "lm3601x",
+        .of_match_table = of_lm3601x_leds_match,
+    },
+    .probe = lm3601x_probe,
+    .remove = lm3601x_remove,
+    .id_table = lm3601x_id,
+};
+module_i2c_driver(lm3601x_i2c_driver);
+
+MODULE_DESCRIPTION("Texas Instruments Flash Lighting driver for LM3601X");
+MODULE_AUTHOR("Dan Murphy <[email protected]>");
+MODULE_LICENSE("GPL v2");

Introduce the family of LED devices that can drive a torch, strobe or IR LED.

The LED driver can be configured with a strobe timer to execute a strobe flash. The IR LED brightness is controlled via the torch brightness register.

The data sheet for each the LM36010 and LM36011 LED drivers can be found here:
http://www.ti.com/product/LM36010
http://www.ti.com/product/LM36011

+ depends on LEDS_CLASS && I2C && OF

drivers/leds/leds-lm3601x.c new file +8 -0

+};
 
+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },

+ brightness_val = (brightness/2);

+ if (!ret)

(quoted diff) +2 -0

+};
+MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match);

Quoted 8 lines · Dan Murphy

Introduce the family of LED devices that can drive a torch, strobe or IR LED.

The LED driver can be configured with a strobe timer to execute a strobe flash. The IR LED brightness is controlled via the torch brightness register.

Quoted 6 lines

The data sheet for each the LM36010 and LM36011 LED drivers can be found here:
http://www.ti.com/product/LM36010
http://www.ti.com/product/LM36011

+ depends on LEDS_CLASS && I2C && OF

What is OF specific in this driver?

drivers/leds/leds-lm3601x.c line 42

Quoted 9 lines

drivers/leds/leds-lm3601x.c new file +4 -0

+#define LM3601X_STRB_LVL_TRIG  ~BIT(3)
 
+#define LM36010_BOOST_LIMIT_19 ~BIT(5)
 
+#define LM36010_BOOST_FREQ_2MHZ        ~BIT(6)
 
+#define LM36010_BOOST_MODE_NORM        ~BIT(7)

These looks rather strange. Shouldn't be just 0:s?

drivers/leds/leds-lm3601x.c line 99

Quoted 7 lines

drivers/leds/leds-lm3601x.c new file +5 -0

+struct lm3601x_led {
+       struct mutex lock;
+       struct regmap *regmap;
+       struct i2c_client *client;
 
+       struct device_node *led_node;

Why do you need this?

Quoted 10 lines

drivers/leds/leds-lm3601x.c new file +8 -0

+};
 
+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },

drivers/leds/leds-lm3601x.c line 135

Quoted 10 lines

drivers/leds/leds-lm3601x.c new file +10 -0

+       { 280000, 0x06 },
+       { 320000, 0x07 },
+       { 360000, 0x08 },
+       { 400000, 0x09 },
+       { 600000, 0x0a },
+       { 800000, 0x0b },
+       { 1000000, 0x0c },
+       { 1200000, 0x0d },
+       { 1400000, 0x0e },
+       { 1600000, 0x0f },

Huh?!

strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;

drivers/leds/leds-lm3601x.c line 258

Quoted 9 lines

drivers/leds/leds-lm3601x.c new file +7 -0

+};
 
+       struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+       int ret;
+       int current_timeout;
+       int reg_count;
+       int i;
+       int timeout_reg_val = 0;

Better to put lines here in reversed xmas tree order (longest first).

drivers/leds/leds-lm3601x.c line 269

Quoted 5 lines

drivers/leds/leds-lm3601x.c new file +4 -0

+       reg_count = ARRAY_SIZE(strobe_timeouts);
+       for (i = 0; i < reg_count; i++) {
+               if (led->strobe_timeout > strobe_timeouts[i].timeout)
+                       continue;

binary search?

Check lib/bsearch.c (IIRC).

But! See above the formula.

Quoted 1 line

+ brightness_val = (brightness/2);

Spaces.

drivers/leds/leds-lm3601x.c line 387

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +3 -0

+static int lm3601x_register_leds(struct lm3601x_led *led)
+{
 
 
+       int err = -ENODEV;

Useless assignment.

drivers/leds/leds-lm3601x.c line 409

Quoted 5 lines

drivers/leds/leds-lm3601x.c new file +4 -0

+       err = led_classdev_flash_register(&led->client->dev,
+                       fled_cdev);
+
+       return err;

This is return led_...();

drivers/leds/leds-lm3601x.c line 459

Quoted 4 lines

drivers/leds/leds-lm3601x.c new file +2 -0

+}
 
+               ret = of_property_read_string(led->led_node, "label", &name);

device_property_...();

Quoted 2 lines

+ if (!ret)

if (ret) sounds more natural. And better just to split

drivers/leds/leds-lm3601x.c line 465

Quoted 5 lines

drivers/leds/leds-lm3601x.c new file +5 -0

+                       snprintf(led->led_name, sizeof(led->led_name),
+                               "%s:%s", led->led_node->name, name);
+               else
+                       snprintf(led->led_name, sizeof(led->led_name),
+                               "%s:torch", led->led_node->name);

const char *tmp;

ret = device_property_read_...(&tmp);
if (ret)
tmp = ...
sprintf(...);

drivers/leds/leds-lm3601x.c line 533

Quoted 2 lines

drivers/leds/leds-lm3601x.c new file +2 -0

+       led = devm_kzalloc(&client->dev,
+                           sizeof(struct lm3601x_led), GFP_KERNEL);

sizeof(*led) and one line in the result

drivers/leds/leds-lm3601x.c line 571

Quoted 4 lines

drivers/leds/leds-lm3601x.c new file +4 -0

+static const struct i2c_device_id lm3601x_id[] = {
+       { "LM36010", CHIP_LM36010 },
+       { "LM36011", CHIP_LM36011 },
+       { },

Terminators better w/o comma.

drivers/leds/leds-lm3601x.c line 578

Quoted 8 lines

drivers/leds/leds-lm3601x.c new file +7 -0

+};
+MODULE_DEVICE_TABLE(i2c, lm3601x_id);
+
+static const struct of_device_id of_lm3601x_leds_match[] = {
+       { .compatible = "ti,lm36010", },
+       { .compatible = "ti,lm36011", },
+       {},

Ditto.

Quoted 3 lines

(quoted diff) +2 -0

+};
+MODULE_DEVICE_TABLE(of, of_lm3601x_leds_match);

Quoted 5 lines · Andy Shevchenko

Quoted 1 line · Dan Murphy

+ depends on LEDS_CLASS && I2C && OF

What is OF specific in this driver?

as3645a_led_class_setup has a "of" dependency

Quoted 18 lines

Quoted 17 lines

drivers/leds/leds-lm3601x.c new file +17 -0

+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },
+       { 280000, 0x06 },
+       { 320000, 0x07 },
+       { 360000, 0x08 },
+       { 400000, 0x09 },
+       { 600000, 0x0a },
+       { 800000, 0x0b },
+       { 1000000, 0x0c },
+       { 1200000, 0x0d },
+       { 1400000, 0x0e },
+       { 1600000, 0x0f },

Quoted 1 line

Huh?!

Please give comments that actually mean something other wise I will opt to ignore them.

Quoted 1 line

strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;

Not sure what equation you are trying to point out here. But if you are trying to apply a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout step is not linear.

Quoted 3 lines

Quoted 1 line

+ brightness_val = (brightness/2);

Spaces.

Not sure what this means checkpatch was clean

Quoted 1 line

This is return led_...();

That is a preference. It does not have to be that way.

Quoted 3 lines

Quoted 1 line

+ ret = of_property_read_string(led->led_node, "label", &name);

device_property_...();

It can be if the maintainer is requesting this.

Is the trend to move to these functions?

Most drivers use the "of" calls.

Quoted 10 lines

Quoted 1 line

+ if (!ret)

if (ret) sounds more natural. And better just to split

Quoted 5 lines

drivers/leds/leds-lm3601x.c new file +5 -0

+                       snprintf(led->led_name, sizeof(led->led_name),
+                               "%s:%s", led->led_node->name, name);
+               else
+                       snprintf(led->led_name, sizeof(led->led_name),
+                               "%s:torch", led->led_node->name);

Quoted 6 lines

const char *tmp;

ret = device_property_read_...(&tmp);
if (ret)
tmp = ...
sprintf(...);

Quoted 4 lines

drivers/leds/leds-lm3601x.c line 533

Quoted 2 lines

drivers/leds/leds-lm3601x.c new file +2 -0

+       led = devm_kzalloc(&client->dev,
+                           sizeof(struct lm3601x_led), GFP_KERNEL);

sizeof(*led) and one line in the result

Quoted 3 lines

Quoted 1 line

+ { },

Terminators better w/o comma.

Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver

Quoted 3 lines

Quoted 1 line

+ {},

Ditto.

See above

Quoted 20 lines · Dan Murphy

Quoted 17 lines · Andy Shevchenko

Quoted 17 lines

drivers/leds/leds-lm3601x.c new file +17 -0

+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },
+       { 280000, 0x06 },
+       { 320000, 0x07 },
+       { 360000, 0x08 },
+       { 400000, 0x09 },
+       { 600000, 0x0a },
+       { 800000, 0x0b },
+       { 1000000, 0x0c },
+       { 1200000, 0x0d },
+       { 1400000, 0x0e },
+       { 1600000, 0x0f },

Quoted 5 lines

Quoted 1 line

strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;

Not sure what equation you are trying to point out here. But if you are trying to apply a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout step is not linear.

Yeah, I know people are more than often too lazy to think.

if (x < 9)
strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
else
strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;

Quoted 2 lines · Dan Murphy

Quoted 1 line · Andy Shevchenko

On Tue, May 15, 2018 at 6:43 PM, Dan Murphy <[email protected]> wrote:

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ depends on LEDS_CLASS && I2C && OF

What is OF specific in this driver?

as3645a_led_class_setup has a "of" dependency

So what? Is it called from this driver or?

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

drivers/leds/leds-lm3601x.c new file +17 -0

+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },
+       { 280000, 0x06 },
+       { 320000, 0x07 },
+       { 360000, 0x08 },
+       { 400000, 0x09 },
+       { 600000, 0x0a },
+       { 800000, 0x0b },
+       { 1000000, 0x0c },
+       { 1200000, 0x0d },
+       { 1400000, 0x0e },
+       { 1600000, 0x0f },

Quoted 3 lines

Quoted 1 line

Huh?!

Please give comments that actually mean something other wise I will opt to ignore them.

I did below.

Quoted 6 lines

Quoted 2 lines

strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;

Not sure what equation you are trying to point out here. But if you are trying to apply a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout step is not linear.

Yeah, I know people are more than often too lazy to think.

if (x < 9)
strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
else
strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ brightness_val = (brightness/2);

Spaces.

Not sure what this means checkpatch was clean

Even besides missed whispaces it has redundant parens.

checkpatch is not a silver bullet to get your code clean and nice.

Quoted 3 lines

Quoted 1 line

This is return led_...();

That is a preference. It does not have to be that way.

What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ ret = of_property_read_string(led->led_node, "label", &name);

device_property_...();

It can be if the maintainer is requesting this.

Jacek, if you need rationale behind this comment it's here: the driver has nothing DT specific and getting rid of OF centric programming allows to reuse the driver on non-DT platforms w/o touching a source code.

Quoted 1 line

Is the trend to move to these functions?

See above.

Quoted 2 lines

Most drivers use the "of" calls.

So what?

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

+ if (!ret)

if (ret) sounds more natural. And better just to split

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

drivers/leds/leds-lm3601x.c new file +5 -0

+                       snprintf(led->led_name, sizeof(led->led_name),
+                               "%s:%s", led->led_node->name, name);
+               else
+                       snprintf(led->led_name, sizeof(led->led_name),
+                               "%s:torch", led->led_node->name);

Quoted 6 lines

Quoted 6 lines

const char *tmp;

ret = device_property_read_...(&tmp);
if (ret)
tmp = ...
sprintf(...);

No comments on this?

Quoted 5 lines

Quoted 5 lines

drivers/leds/leds-lm3601x.c line 533

Quoted 3 lines

drivers/leds/leds-lm3601x.c new file +2 -0

+       led = devm_kzalloc(&client->dev,
+                           sizeof(struct lm3601x_led), GFP_KERNEL);

sizeof(*led) and one line in the result

And this?

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

+ { },

Terminators better w/o comma.

Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver

So what?

Terminator at compile time even better.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ {},

Ditto.

See above

See above.

Quoted 22 lines · Andy Shevchenko

Quoted 21 lines · Dan Murphy

Quoted 20 lines · Andy Shevchenko

Quoted 17 lines · Dan Murphy

drivers/leds/leds-lm3601x.c new file +17 -0

+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },
+       { 280000, 0x06 },
+       { 320000, 0x07 },
+       { 360000, 0x08 },
+       { 400000, 0x09 },
+       { 600000, 0x0a },
+       { 800000, 0x0b },
+       { 1000000, 0x0c },
+       { 1200000, 0x0d },
+       { 1400000, 0x0e },
+       { 1600000, 0x0f },

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Huh?!

Please give comments that actually mean something other wise I will opt to ignore them.

I did below.

Sort of. "So what?" and "Huh" are not really a technical comments.

I can tell from your LVEE presentation that
you have a passion for making the kernel better. That is awesome but if we try to make the code perfect everytime no code will ever get merged.

And we are human and we will miss coding issues and make mistakes.

Quoted 7 lines

Quoted 5 lines

Quoted 1 line

strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;

Not sure what equation you are trying to point out here. But if you are trying to apply a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout step is not linear.

Yeah, I know people are more than often too lazy to think.

Well this is why I put the table in. It is not laziness on my part as I try to add clarity for our newbies or productization engineers, translation/lookup tables are not as bad as you think.

I am fully aware that this bloats the data section of the image and has negative impacts on IoT small memory foot print devices.

Quoted 6 lines

if (x < 9)
strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
else
strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;

Not sure if this will produce the register value I am looking for. I have to run through the algorithim. The idea is to take in the timeout and get the reg value to program.

If it yields the expected output, or with a modified equation, then I can pull it in.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

+ brightness_val = (brightness/2);

I am still not see the extra spaces here.

Quoted 1 line

What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.

Again for non-IoT cases this would be fine but if we are striving to meet IoT size requirements I do understand your concern.

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

+ ret = of_property_read_string(led->led_node, "label", &name);

device_property_...();

Jacek, if you need rationale behind this comment it's here: the driver has nothing DT specific and getting rid of OF centric programming allows to reuse the driver on non-DT platforms w/o touching a source code.

I am deferring this to Jacek. Because this should be an overall coding change for all LED drivers that are new.
Like I pointed out I don't mind adding/changing the code as long as
all LED drivers will have that same mandate moving forward.

If the maintainers agree we can get this driver to be the example for the use of device_property calls.

Quoted 13 lines

Quoted 12 lines

Quoted 12 lines

drivers/leds/leds-lm3601x.c line 465

Quoted 5 lines

drivers/leds/leds-lm3601x.c new file +5 -0

+                       snprintf(led->led_name, sizeof(led->led_name),
+                               "%s:%s", led->led_node->name, name);
+               else
+                       snprintf(led->led_name, sizeof(led->led_name),
+                               "%s:torch", led->led_node->name);

const char *tmp;

ret = device_property_read_...(&tmp);
if (ret)
tmp = ...
sprintf(...);

Quoted 1 line

No comments on this?

No. This would depend on the disposition of the device_property calls. If we go that way then yes this will probably change. But we need to have an if..else as the label property is option in the DT documentation and we have to make a label if one does not exist.

Quoted 28 lines · Dan Murphy

Quoted 21 lines · Andy Shevchenko

Quoted 11 lines · Dan Murphy

Quoted 9 lines · Andy Shevchenko

Quoted 9 lines

drivers/leds/leds-lm3601x.c new file +9 -0

+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },
+       { 280000, 0x06 },
+       { 320000, 0x07 },

Quoted 8 lines

Quoted 8 lines

drivers/leds/leds-lm3601x.c line 135

Quoted 8 lines

drivers/leds/leds-lm3601x.c new file +8 -0

+       { 360000, 0x08 },
+       { 400000, 0x09 },
+       { 600000, 0x0a },
+       { 800000, 0x0b },
+       { 1000000, 0x0c },
+       { 1200000, 0x0d },
+       { 1400000, 0x0e },
+       { 1600000, 0x0f },

Quoted 4 lines

if (x < 9)
strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
else
strobe_timeout = (x - 7) * 200 * MSECS_IN_SEC;

Quoted 4 lines

Not sure if this will produce the register value I am looking for. I have to run through the algorithim. The idea is to take in the timeout and get the reg value to program.

If it yields the expected output, or with a modified equation, then I can pull it in.

So, you need a reversed calculus in this case.

u8 regval;

/* To make it in range... */
timeout = clamp_val(timeout, 40000, 1600000);
/* ...other possibility to issue an error */

if (timeout >= 400000)
regval = timeout / 200000 + 0x07;
else
regval = timeout / 40000 - 0x01;

In any case, please double check it gives a correct values.

Quoted 8 lines · Dan Murphy

Quoted 5 lines · Andy Shevchenko

Quoted 1 line · Dan Murphy

+ depends on LEDS_CLASS && I2C && OF

What is OF specific in this driver?

as3645a_led_class_setup has a "of" dependency

So what? Is it called from this driver or?

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

drivers/leds/leds-lm3601x.c new file +17 -0

+static const struct lm3601x_max_timeouts strobe_timeouts[] = {
+       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },
+       { 280000, 0x06 },
+       { 320000, 0x07 },
+       { 360000, 0x08 },
+       { 400000, 0x09 },
+       { 600000, 0x0a },
+       { 800000, 0x0b },
+       { 1000000, 0x0c },
+       { 1200000, 0x0d },
+       { 1400000, 0x0e },
+       { 1600000, 0x0f },

Quoted 3 lines

Quoted 1 line

Huh?!

Please give comments that actually mean something other wise I will opt to ignore them.

I did below.

Quoted 5 lines

Quoted 1 line

strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;

Not sure what equation you are trying to point out here. But if you are trying to apply a timeout step you cannot do this with this part. As pointed out in the DT doc the timeout step is not linear.

Yeah, I know people are more than often too lazy to think.

if (x < 9)
strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
else
strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ brightness_val = (brightness/2);

Spaces.

Not sure what this means checkpatch was clean

Even besides missed whispaces it has redundant parens.

checkpatch is not a silver bullet to get your code clean and nice.

Quoted 3 lines

Quoted 1 line

This is return led_...();

That is a preference. It does not have to be that way.

What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

+ ret = of_property_read_string(led->led_node, "label", &name);

Quoted 3 lines

Quoted 1 line

device_property_...();

It can be if the maintainer is requesting this.

Jacek, if you need rationale behind this comment it's here: the driver has nothing DT specific and getting rid of OF centric programming allows to reuse the driver on non-DT platforms w/o touching a source code.

Quoted 1 line

Is the trend to move to these functions?

See above.

Quoted 1 line

Most drivers use the "of" calls.

So what?

Quoted 16 lines

Quoted 16 lines

Quoted 1 line

+ if (!ret)

if (ret) sounds more natural. And better just to split

drivers/leds/leds-lm3601x.c line 465

Quoted 5 lines

drivers/leds/leds-lm3601x.c new file +5 -0

+                       snprintf(led->led_name, sizeof(led->led_name),
+                               "%s:%s", led->led_node->name, name);
+               else
+                       snprintf(led->led_name, sizeof(led->led_name),
+                               "%s:torch", led->led_node->name);

const char *tmp;

ret = device_property_read_...(&tmp);
if (ret)
tmp = ...
sprintf(...);

No comments on this?

Quoted 4 lines

Quoted 4 lines

drivers/leds/leds-lm3601x.c line 533

Quoted 2 lines

drivers/leds/leds-lm3601x.c new file +2 -0

+       led = devm_kzalloc(&client->dev,
+                           sizeof(struct lm3601x_led), GFP_KERNEL);

sizeof(*led) and one line in the result

And this?

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

+ { },

Terminators better w/o comma.

Looking at other drivers adding comma's on the sentinel is accepted. See the as3645a driver

So what?

Terminator at compile time even better.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+ {},

Ditto.

See above

See above.

Hi Andy and Dan,

Quoted 43 lines · Dan Murphy

Quoted 8 lines · Andy Shevchenko

Quoted 5 lines · Dan Murphy

Quoted 2 lines

+       depends on LEDS_CLASS && I2C && OF

What is OF specific in this driver?

as3645a_led_class_setup has a "of" dependency

So what? Is it called from this driver or?

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

+static const struct lm3601x_max_timeouts strobe_timeouts[] = { +       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },
+       { 280000, 0x06 },
+       { 320000, 0x07 },
+       { 360000, 0x08 },
+       { 400000, 0x09 },
+       { 600000, 0x0a },
+       { 800000, 0x0b },
+       { 1000000, 0x0c },
+       { 1200000, 0x0d },
+       { 1400000, 0x0e },
+       { 1600000, 0x0f },

Quoted 3 lines

Quoted 1 line

Huh?!

Please give comments that actually mean something other wise I will opt to ignore them.

I did below.

Quoted 5 lines

Quoted 1 line

strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;

Not sure what equation you are trying to point out here.  But if you are trying to apply a timeout step you cannot do this with this part.  As pointed out in the DT doc the timeout step is not linear.

Quoted 7 lines

Yeah, I know people are more than often too lazy to think.

if (x < 9)
  strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; else
  strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;

I like the idea.

Quoted 13 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+               brightness_val = (brightness/2);

Spaces.

Not sure what this means checkpatch was clean

Even besides missed whispaces it has redundant parens.

checkpatch is not a silver bullet to get your code clean and nice.

Quoted 3 lines

Quoted 1 line

This is return led_...();

That is a preference.  It does not have to be that way.

I missed that. Dan, please follow Andy's advise.

Quoted 5 lines

What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

+               ret = of_property_read_string(led->led_node, "label", &name);

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

device_property_...();

It can be if the maintainer is requesting this.

Jacek, if you need rationale behind this comment it's here: the driver has nothing DT specific and getting rid of OF centric programming allows to reuse the driver on non-DT platforms w/o touching a source code.

It has an added value, so yes, let's use it as a standard approach from now on.

Quoted 25 lines

Quoted 1 line

Is the trend to move to these functions?

See above.

Quoted 1 line

Most drivers use the "of" calls.

So what?

Quoted 16 lines

Quoted 16 lines

Quoted 1 line

+               if (!ret)

if (ret) sounds more natural. And better just to split

Quoted 5 lines

+                       snprintf(led->led_name, sizeof(led->led_name), +                               "%s:%s", led->led_node->name, name); +               else
+                       snprintf(led->led_name, sizeof(led->led_name), +                               "%s:torch", led->led_node->name);

const char *tmp;

ret = device_property_read_...(&tmp);
if (ret)
  tmp = ...
sprintf(...);

We're no longer taking devicename section of a LED class device name from DT, so it will look differently anyway.

Quoted 8 lines

No comments on this?

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

+       led = devm_kzalloc(&client->dev,
+                           sizeof(struct lm3601x_led), GFP_KERNEL);

sizeof(*led) and one line in the result

And this?

Ack.

Quoted 19 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

+       { },

Terminators better w/o comma.

Looking at other drivers adding comma's on the sentinel is accepted.  See the as3645a driver

So what?

Terminator at compile time even better.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+       {},

Ditto.

See above

See above.

Jacek and Andy

Quoted 2 lines · Jacek Anaszewski

Hi Andy and Dan,

I will make all the changes then. I don't want to go through and ack each one.

Thanks for the guidance and the reviews.

It will take a couple days to find all the comments and get this all fixed up.

Dan

Quoted 147 lines · Andy Shevchenko

Quoted 43 lines · Dan Murphy

Quoted 8 lines · Andy Shevchenko

Quoted 5 lines · Dan Murphy

Quoted 2 lines

+       depends on LEDS_CLASS && I2C && OF

What is OF specific in this driver?

as3645a_led_class_setup has a "of" dependency

So what? Is it called from this driver or?

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

+static const struct lm3601x_max_timeouts strobe_timeouts[] = { +       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },
+       { 280000, 0x06 },
+       { 320000, 0x07 },
+       { 360000, 0x08 },
+       { 400000, 0x09 },
+       { 600000, 0x0a },
+       { 800000, 0x0b },
+       { 1000000, 0x0c },
+       { 1200000, 0x0d },
+       { 1400000, 0x0e },
+       { 1600000, 0x0f },

Quoted 3 lines

Quoted 1 line

Huh?!

Please give comments that actually mean something other wise I will opt to ignore them.

I did below.

Quoted 5 lines

Quoted 1 line

strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;

Not sure what equation you are trying to point out here.  But if you are trying to apply a timeout step you cannot do this with this part.  As pointed out in the DT doc the timeout step is not linear.

Quoted 7 lines

Yeah, I know people are more than often too lazy to think.

if (x < 9)
  strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC; else
  strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;

I like the idea.

Quoted 13 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+               brightness_val = (brightness/2);

Spaces.

Not sure what this means checkpatch was clean

Even besides missed whispaces it has redundant parens.

checkpatch is not a silver bullet to get your code clean and nice.

Quoted 3 lines

Quoted 1 line

This is return led_...();

That is a preference.  It does not have to be that way.

I missed that. Dan, please follow Andy's advise.

Quoted 5 lines

What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

+               ret = of_property_read_string(led->led_node, "label", &name);

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

device_property_...();

It can be if the maintainer is requesting this.

Jacek, if you need rationale behind this comment it's here: the driver has nothing DT specific and getting rid of OF centric programming allows to reuse the driver on non-DT platforms w/o touching a source code.

It has an added value, so yes, let's use it as a standard approach from now on.

Quoted 25 lines

Quoted 1 line

Is the trend to move to these functions?

See above.

Quoted 1 line

Most drivers use the "of" calls.

So what?

Quoted 16 lines

Quoted 16 lines

Quoted 1 line

+               if (!ret)

if (ret) sounds more natural. And better just to split

Quoted 5 lines

+                       snprintf(led->led_name, sizeof(led->led_name), +                               "%s:%s", led->led_node->name, name); +               else
+                       snprintf(led->led_name, sizeof(led->led_name), +                               "%s:torch", led->led_node->name);

const char *tmp;

ret = device_property_read_...(&tmp);
if (ret)
  tmp = ...
sprintf(...);

We're no longer taking devicename section of a LED class device name from DT, so it will look differently anyway.

Quoted 8 lines

No comments on this?

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

+       led = devm_kzalloc(&client->dev,
+                           sizeof(struct lm3601x_led), GFP_KERNEL);

sizeof(*led) and one line in the result

And this?

Ack.

Quoted 19 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

+       { },

Terminators better w/o comma.

Looking at other drivers adding comma's on the sentinel is accepted.  See the as3645a driver

So what?

Terminator at compile time even better.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+       {},

Ditto.

See above

See above.

Jacek and Andy

Quoted 3 lines · Dan Murphy

Jacek and Andy

On 05/16/2018 04:02 PM, Jacek Anaszewski wrote:

Quoted 5 lines

Quoted 2 lines

Hi Andy and Dan,

I will make all the changes then. I don't want to go through and ack each one.

Let me clarify. I will make all the change Jacek has guided on. There is still a terminator comma vs no comma comment that needs disposition at the end of this file.

Quoted 155 lines

Thanks for the guidance and the reviews.

It will take a couple days to find all the comments and get this all fixed up.

Dan

Quoted 147 lines · Andy Shevchenko

Quoted 43 lines · Dan Murphy

Quoted 8 lines · Andy Shevchenko

Quoted 5 lines · Dan Murphy

Quoted 2 lines

+       depends on LEDS_CLASS && I2C && OF

What is OF specific in this driver?

as3645a_led_class_setup has a "of" dependency

So what? Is it called from this driver or?

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

+static const struct lm3601x_max_timeouts strobe_timeouts[] = { +       { 40000, 0x00 },
+       { 80000, 0x01 },
+       { 120000, 0x02 },
+       { 160000, 0x03 },
+       { 200000, 0x04 },
+       { 240000, 0x05 },
+       { 280000, 0x06 },
+       { 320000, 0x07 },
+       { 360000, 0x08 },
+       { 400000, 0x09 },
+       { 600000, 0x0a },
+       { 800000, 0x0b },
+       { 1000000, 0x0c },
+       { 1200000, 0x0d },
+       { 1400000, 0x0e },
+       { 1600000, 0x0f },

Quoted 3 lines

Quoted 1 line

Huh?!

Please give comments that actually mean something other wise I will opt to ignore them.

I did below.

Quoted 5 lines

Quoted 1 line

strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;

Not sure what equation you are trying to point out here.  But if you are trying to apply a timeout step you cannot do this with this part.  As pointed out in the DT doc the timeout step is not linear.

Quoted 7 lines

Yeah, I know people are more than often too lazy to think.

if (x < 9)
  strobe_timeout = (x + 1) * 40 * MSECS_IN_SEC;
else
  strobe_timeout = (400 + (x - 9) * 200) * MSECS_IN_SEC;

I like the idea.

Quoted 13 lines

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+               brightness_val = (brightness/2);

Spaces.

Not sure what this means checkpatch was clean

Even besides missed whispaces it has redundant parens.

checkpatch is not a silver bullet to get your code clean and nice.

Quoted 3 lines

Quoted 1 line

This is return led_...();

That is a preference.  It does not have to be that way.

I missed that. Dan, please follow Andy's advise.

Quoted 5 lines

What do you mean? We do not appreciate +LOCs for no (or even nagative!) benefit.

Quoted 2 lines

Quoted 2 lines

Quoted 1 line

+               ret = of_property_read_string(led->led_node, "label", &name);

Quoted 8 lines

Quoted 3 lines

Quoted 1 line

device_property_...();

It can be if the maintainer is requesting this.

Jacek, if you need rationale behind this comment it's here: the driver has nothing DT specific and getting rid of OF centric programming allows to reuse the driver on non-DT platforms w/o touching a source code.

It has an added value, so yes, let's use it as a standard approach from now on.

Quoted 25 lines

Quoted 1 line

Is the trend to move to these functions?

See above.

Quoted 1 line

Most drivers use the "of" calls.

So what?

Quoted 16 lines

Quoted 16 lines

Quoted 1 line

+               if (!ret)

if (ret) sounds more natural. And better just to split

Quoted 5 lines

+                       snprintf(led->led_name, sizeof(led->led_name), +                               "%s:%s", led->led_node->name, name); +               else
+                       snprintf(led->led_name, sizeof(led->led_name), +                               "%s:torch", led->led_node->name);

const char *tmp;

ret = device_property_read_...(&tmp);
if (ret)
  tmp = ...
sprintf(...);

We're no longer taking devicename section of a LED class device name from DT, so it will look differently anyway.

Quoted 8 lines

No comments on this?

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

+       led = devm_kzalloc(&client->dev,
+                           sizeof(struct lm3601x_led), GFP_KERNEL);

sizeof(*led) and one line in the result

And this?

Ack.

Quoted 19 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

+       { },

Terminators better w/o comma.

Looking at other drivers adding comma's on the sentinel is accepted.  See the as3645a driver

So what?

Terminator at compile time even better.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+       {},

Ditto.

See above

See above.

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

Quoted 3 lines

+               if (!ret)

if (ret) sounds more natural. And better just to split

Quoted 5 lines

+                       snprintf(led->led_name, sizeof(led->led_name), +                               "%s:%s", led->led_node->name, name); +               else
+                       snprintf(led->led_name, sizeof(led->led_name), +                               "%s:torch", led->led_node->name);

const char *tmp;

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

ret = device_property_read_...(&tmp);
if (ret)
  tmp = ...
sprintf(...);

We're no longer taking devicename section of a LED class device name from DT, so it will look differently anyway.

Jacek

On 05/16/2018 04:17 PM, Dan Murphy wrote:
<snip>

Quoted 19 lines

Quoted 19 lines

Quoted 19 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Quoted 3 lines

+               if (!ret)

if (ret) sounds more natural. And better just to split

Quoted 5 lines

+                       snprintf(led->led_name, sizeof(led->led_name), +                               "%s:%s", led->led_node->name, name); +               else
+                       snprintf(led->led_name, sizeof(led->led_name), +                               "%s:torch", led->led_node->name);

const char *tmp;

ret = device_property_read_...(&tmp);
if (ret)
  tmp = ...
sprintf(...);

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

We're no longer taking devicename section of a LED class device name from DT, so it will look differently anyway.

So in adding the device_property code I think we again are reaching the LED label issue. In ARM with DT we would take the parent device node name and append it to the label

if the optional label property was not available. In migrating to the device_property APIs we don't or can't depend on that parent node anymore.

So for the case where the label property does not exist should we use a hard coded name or should we try to use the name from a device_id table.

This is how we did this for the leds-lp8860 driver. If the label did not exist we used the i2c_device_id table and pulled the string from there.

Introduce the device tree bindings for the lm3601x family of LED torch, flash and IR drivers.

Signed-off-by Dan Murphy <[email protected]>

---

v6 - Removed multiple led child nodes, fixed example to display micro ranges for corresponding child nodes and added led-sources to define the current driver -
https://patchwork.kernel.org/patch/10392121/

v5 - No changes - https://patchwork.kernel.org/patch/10391743/
v4 - Added " " around "=", changed strobe to flash on label, removed "support and register" comment and change ir lable to ir:torch - See v2 patchworks for comments v3 - Removed wildcard compatible - https://patchwork.kernel.org/patch/10386241/
v2 - No changes - https://patchwork.kernel.org/patch/10384587/

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

create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt

Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file +7 -0

new file mode 100644
index 000000000000..27930a89e9a5
@@ -0,0 +1,47 @@
+* Texas Instruments - lm3601x Single-LED Flash Driver
+
+The LM3601X are ultra-small LED flash drivers that
+provide a high level of adjustability.
+
+Required properties:
+	- compatible : Can be one of the following

Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file +24 -0

+
+Optional child properties:
+	- label : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+led-controller@64 {
+	compatible = "ti,lm36010";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x64>;
+
+	led@0 {
+		reg = <0>;
+		label = "white:torch";
+		led-max-microamp = <376000>;
+		flash-max-microamp = <1500000>;
+		flash-max-timeout-us = <1600000>;
+		led-sources = <1>;
+	};
+}
+
+For more product information please see the links below:
+http://www.ti.com/product/LM36010
+http://www.ti.com/product/LM36011

Hi Dan,

Thanks for the update.

Quoted 39 lines · Dan Murphy

Introduce the device tree bindings for the lm3601x family of LED torch, flash and IR drivers.

Signed-off-by Dan Murphy <[email protected]>

---

v6 - Removed multiple led child nodes, fixed example to display micro ranges for corresponding child nodes and added led-sources to define the current driver -
https://patchwork.kernel.org/patch/10392121/

v5 - No changes - https://patchwork.kernel.org/patch/10391743/
v4 - Added " " around "=", changed strobe to flash on label, removed "support and register" comment and change ir lable to ir:torch - See v2 patchworks for comments v3 - Removed wildcard compatible - https://patchwork.kernel.org/patch/10386241/
v2 - No changes - https://patchwork.kernel.org/patch/10384587/

  .../devicetree/bindings/leds/leds-lm3601x.txt | 47 +++++++++++++++++++

  1 file changed, 47 insertions(+)

  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt

Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file +13 -0

new file mode 100644
index 000000000000..27930a89e9a5
@@ -0,0 +1,47 @@
+* Texas Instruments - lm3601x Single-LED Flash Driver
+
+The LM3601X are ultra-small LED flash drivers that
+provide a high level of adjustability.
+
+Required properties:
+    - compatible : Can be one of the following
+        "ti,lm36010"
+        "ti,lm36011"
+    - reg : I2C slave address
+    - #address-cells : 1
+    - #size-cells : 0
+

Documentation/devicetree/bindings/leds/leds-lm3601x.txt line 17

Quoted 4 lines

Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file +4 -0

+Required child properties:
+    - reg : 0
+    - led-sources:    0 - Indicates a IR mode
+            1 - Indicates a Torch (white LED) mode

You don't need led-sources at all. Please use reg as in the previous version. led-sources is useful for describing more complex arrangements. Here reg will do.

Documentation/devicetree/bindings/leds/leds-lm3601x.txt line 23

Quoted 7 lines

Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file +6 -0

+
+Required properties for flash LED child nodes:
+    See Documentation/devicetree/bindings/leds/common.txt
+    - flash-max-microamp : Range from 11mA -> 1.5A
+    - flash-max-timeout-us : Range from 40ms -> 1600ms
+    - led-max-microamp : Range from 2.4mA -> 376mA

Please give also a step in the format like below
(taken from max77693):

    Valid values: 62500 - 1000000, step by 62500 (rounded down)

Quoted 27 lines

Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file +24 -0

+
+Optional child properties:
+    - label : see Documentation/devicetree/bindings/leds/common.txt
+
+Example:
+led-controller@64 {
+    compatible = "ti,lm36010";
+    #address-cells = <1>;
+    #size-cells = <0>;
+    reg = <0x64>;
+
+    led@0 {
+        reg = <0>;
+        label = "white:torch";
+        led-max-microamp = <376000>;
+        flash-max-microamp = <1500000>;
+        flash-max-timeout-us = <1600000>;
+        led-sources = <1>;
+    };
+}
+
+For more product information please see the links below:
+http://www.ti.com/product/LM36010
+http://www.ti.com/product/LM36011

Jacek

Quoted 44 lines · Jacek Anaszewski

Hi Dan,

Thanks for the update.

Quoted 39 lines · Dan Murphy

Introduce the device tree bindings for the lm3601x family of LED torch, flash and IR drivers.

Signed-off-by Dan Murphy <[email protected]>

---

v6 - Removed multiple led child nodes, fixed example to display micro ranges for corresponding child nodes and added led-sources to define the current driver -
https://patchwork.kernel.org/patch/10392121/

v5 - No changes - https://patchwork.kernel.org/patch/10391743/
v4 - Added " " around "=", changed strobe to flash on label, removed "support and register" comment and change ir lable to ir:torch - See v2 patchworks for comments v3 - Removed wildcard compatible - https://patchwork.kernel.org/patch/10386241/
v2 - No changes - https://patchwork.kernel.org/patch/10384587/

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

  create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt

Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file +13 -0

new file mode 100644
index 000000000000..27930a89e9a5
@@ -0,0 +1,47 @@
+* Texas Instruments - lm3601x Single-LED Flash Driver
+
+The LM3601X are ultra-small LED flash drivers that
+provide a high level of adjustability.
+
+Required properties:
+    - compatible : Can be one of the following
+        "ti,lm36010"
+        "ti,lm36011"
+    - reg : I2C slave address
+    - #address-cells : 1
+    - #size-cells : 0
+

Quoted 8 lines

Documentation/devicetree/bindings/leds/leds-lm3601x.txt line 17

Quoted 4 lines

Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file +4 -0

+Required child properties:
+    - reg : 0
+    - led-sources:    0 - Indicates a IR mode
+            1 - Indicates a Torch (white LED) mode

You don't need led-sources at all. Please use reg as in the previous version. led-sources is useful for describing more complex arrangements. Here reg will do.

OK. Thought we would keep consistent with the max IC.

Quoted 11 lines

Documentation/devicetree/bindings/leds/leds-lm3601x.txt line 23

Quoted 7 lines

Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file +6 -0

+
+Required properties for flash LED child nodes:
+    See Documentation/devicetree/bindings/leds/common.txt
+    - flash-max-microamp : Range from 11mA -> 1.5A
+    - flash-max-timeout-us : Range from 40ms -> 1600ms
+    - led-max-microamp : Range from 2.4mA -> 376mA

Please give also a step in the format like below
(taken from max77693):