Skip to contentSkip to conversation

dt: bindings: lm3601x: Introduce the lm3601x driver

PATCH Superseded Dan Murphy 3 participants 8 messages May 21, 2018 – May 22, 2018

Subsystems (2)
Device tree bindings LED

v7 of 9 Superseded Latest is v9 →

lore
Share & tools
lore

Dan Murphy

author
#0 lore raw

PATCH v7 1/2 dt: bindings: lm3601x: Introduce the lm3601x driver

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

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

---

v7 - Removed led-sources in favor of reg, fixed malformatted ranges - https://patchwork.kernel.org/patch/10401435/

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 | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/leds/leds-lm3601x.txt
Documentation/devicetree/bindings/leds/leds-lm3601x.txt new file +45 -0
new file mode 100644
index 000000000000..a88b2c41e75b
@@ -0,0 +1,45 @@
+* 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
+
+Required child properties:
+	- reg : 0 - Indicates a IR mode
+		1 - Indicates a Torch (white LED) mode
+
+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
+
+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 = <1>;
+		label = "white:torch";
+		led-max-microamp = <376000>;
+		flash-max-microamp = <1500000>;
+		flash-max-timeout-us = <1600000>;
+	};
+}
+
+For more product information please see the links below:
+http://www.ti.com/product/LM36010
+http://www.ti.com/product/LM36011
Signature · 1 line
2.17.0.582.gccdcbd54c
author
#1 lore raw

PATCH v7 2/2 leds: lm3601x: Introduce the lm3601x LED 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]>

---

v7 - Numerous fixes to many to list. Highlights are moved from OF APIs to device_property APIs, updated LED registration, timeout to reg algorithim, and fixed label generation -
https://patchwork.kernel.org/patch/10401437/

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 | 501 ++++++++++++++++++++++++++++++++++++
 3 files changed, 511 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 +501 -0

501 lines not on this page · raw message

Signature · 1 line
2.17.0.582.gccdcbd54c
#2 lore raw

Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver

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

Thanks for an update. My comments below.

drivers/leds/Kconfig config LEDS_LM3692X · line 150

Quoted 3 lines
drivers/leds/Kconfig +3 -0
+config LEDS_LM3601X
+       tristate "LED support for LM3601x Chips"
+       depends on LEDS_CLASS && I2C && OF

Now OF here is superfluous.

drivers/leds/Kconfig config LEDS_LM3692X

Quoted 8 lines
drivers/leds/Kconfig +7 -0
+       depends on LEDS_CLASS_FLASH
+       select REGMAP_I2C
+       help
+         This option enables support for the TI LM3601x family
+         of flash, torch and indicator classes.
 
+#define LM3601X_TIMEOUT_MASK   0x1e
+#define LM3601X_ENABLE_MASK    0x03

I dunno if GENMASK() will be better here.

drivers/leds/leds-lm3601x.c line 66

Quoted 6 lines
drivers/leds/leds-lm3601x.c new file +5 -0
+#define LM3601X_LOWER_STEP_US  40000
+#define LM3601X_UPPER_STEP_US  200000
+#define LM3601X_MIN_TIMEOUT_US 40000
+#define LM3601X_MAX_TIMEOUT_US 1600000
 
+#define LM3601X_TIMEOUT_XOVER  400000

Missed unit?

drivers/leds/leds-lm3601x.c line 242

Quoted 7 lines
drivers/leds/leds-lm3601x.c new file +6 -0
+       ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                               LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
+                               led_mode_val);
 
+               ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                                       LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
+                                       LM3601X_MODE_STROBE);

Perhaps
#define ..._MODE_TORCH_WITH_IR (..._TOCRH | ..._IR_DRV)
?

drivers/leds/leds-lm3601x.c line 446

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +8 -0
+static int lm3601x_probe(struct i2c_client *client,
+                       const struct i2c_device_id *id)
+{
+       struct lm3601x_led *led;
+       int err;
 
+       err = lm3601x_parse_node(led, client->dev.of_node);
+       if (err < 0)
 
+               return -ENODEV;

Shouldn't be

return err;

?

drivers/leds/leds-lm3601x.c line 460

Quoted 3 lines
drivers/leds/leds-lm3601x.c new file +3 -0
+       err = lm3601x_register_leds(led);
+
+       return err;

I will leave this to maintainers since you seems have strong objections against more or less standard pattern, i.e.

return lm3601x_register_leds();

Quoted 1 line

+}

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#3 lore raw

Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver

Andy

Thanks for the review. I was pretty sure I missed somethings.

Quoted 14 lines · Andy Shevchenko
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 4 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

Quoted 7 lines

Thanks for an update. My comments below.

drivers/leds/Kconfig config LEDS_LM3692X · line 150

Quoted 3 lines
drivers/leds/Kconfig +3 -0
+config LEDS_LM3601X
+       tristate "LED support for LM3601x Chips"
+       depends on LEDS_CLASS && I2C && OF

Now OF here is superfluous.

This is the first one I missed.

ACK

Quoted 11 lines

drivers/leds/Kconfig config LEDS_LM3692X

Quoted 9 lines
drivers/leds/Kconfig +7 -0
+       depends on LEDS_CLASS_FLASH
+       select REGMAP_I2C
+       help
+         This option enables support for the TI LM3601x family
+         of flash, torch and indicator classes.
 
+#define LM3601X_TIMEOUT_MASK   0x1e
+#define LM3601X_ENABLE_MASK    0x03

I dunno if GENMASK() will be better here.

I could probably make it more readable as well.

I am thinking

#define LM3601X_ENABLE_MASK (LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV)

Quoted 9 lines

drivers/leds/leds-lm3601x.c line 66

Quoted 7 lines
drivers/leds/leds-lm3601x.c new file +5 -0
+#define LM3601X_LOWER_STEP_US  40000
+#define LM3601X_UPPER_STEP_US  200000
+#define LM3601X_MIN_TIMEOUT_US 40000
+#define LM3601X_MAX_TIMEOUT_US 1600000
 
+#define LM3601X_TIMEOUT_XOVER  400000

Missed unit?

Ack

Quoted 5 lines
Quoted 4 lines
drivers/leds/leds-lm3601x.c new file +3 -0
+       ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                               LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
+                               led_mode_val);
Quoted 8 lines

drivers/leds/leds-lm3601x.c line 242

Quoted 3 lines
drivers/leds/leds-lm3601x.c new file +3 -0
+               ret = regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+                                       LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
+                                       LM3601X_MODE_STROBE);

Perhaps
#define ..._MODE_TORCH_WITH_IR (..._TOCRH | ..._IR_DRV)
?

This was defined already I just did not use the #define but I will probably replace this with the LM3601X_ENABLE_MASK #define

Quoted 6 lines
Quoted 5 lines
drivers/leds/leds-lm3601x.c new file +5 -0
+static int lm3601x_probe(struct i2c_client *client,
+                       const struct i2c_device_id *id)
+{
+       struct lm3601x_led *led;
+       int err;
Quoted 8 lines

drivers/leds/leds-lm3601x.c line 446

Quoted 4 lines
drivers/leds/leds-lm3601x.c new file +3 -0
+       err = lm3601x_parse_node(led, client->dev.of_node);
+       if (err < 0)
 
+               return -ENODEV;

Shouldn't be

return err;

Ack. But I might just get rid of err altogether when I make the change below to return the status of the register.

Quoted 11 lines

?

drivers/leds/leds-lm3601x.c line 460

Quoted 3 lines
drivers/leds/leds-lm3601x.c new file +3 -0
+       err = lm3601x_register_leds(led);
+
+       return err;

I will leave this to maintainers since you seems have strong objections against more or less standard pattern, i.e.

return lm3601x_register_leds();

ACk. I was going to change that but I was concerned about loosing err. But I can change it.

Quoted 3 lines
Quoted 2 lines

+}

Signature · 2 lines
------------------
Dan Murphy
#4 lore raw

Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver

In reply to Dan Murphy

Hi Dan,

Thank you for the update.

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

---

v7 - Numerous fixes to many to list. Highlights are moved from OF APIs to device_property APIs, updated LED registration, timeout to reg algorithim, and fixed label generation -
https://patchwork.kernel.org/patch/10401437/

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 | 501 ++++++++++++++++++++++++++++++++++++
  3 files changed, 511 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 +65 -0
new file mode 100644
index 000000000000..7c71cb11d474
@@ -0,0 +1,501 @@
+// 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_IR		0x0
+#define LM3601X_LED_TORCH	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_EDGE_TRIG	BIT(3)
+#define LM3601X_IVFM_EN		BIT(4)
+
+#define LM36010_BOOST_LIMIT_28	BIT(5)
+#define LM36010_BOOST_FREQ_4MHZ	BIT(6)
+#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_MAX_TORCH_I_UA	376000
+#define LM3601X_MIN_TORCH_I_UA	2400
+#define LM3601X_TORCH_REG_DIV	3000
+
+#define LM3601X_MAX_STROBE_I_UA	1500000
+#define LM3601X_MIN_STROBE_I_UA	11000
+#define LM3601X_STROBE_REG_DIV	11800
+
+#define LM3601X_TIMEOUT_MASK	0x1e
+#define LM3601X_ENABLE_MASK	0x03
+
+#define LM3601X_LOWER_STEP_US	40000
+#define LM3601X_UPPER_STEP_US	200000
+#define LM3601X_MIN_TIMEOUT_US	40000
+#define LM3601X_MAX_TIMEOUT_US	1600000

drivers/leds/leds-lm3601x.c line 75

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+#define LM3601X_TIMEOUT_XOVER	400000
+
+enum lm3601x_type {
+	CHIP_LM36010 = 0,
+	CHIP_LM36011,
+};
+
+/**
+ * struct lm3601x_led -
+ * @fled_cdev: flash led class device pointer

s/led/LED/

Quoted 115 lines
drivers/leds/leds-lm3601x.c new file +115 -0
+ * @client: Pointer to the I2C client
+ * @regmap: Devices register map
+ * @lock: Lock for reading/writing the device
+ * @led_name: LED label for the Torch or IR LED
+ * @strobe_timeout: the timeout for the strobe
+ * @last_flag: last known flags register value
+ * @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 led_classdev_flash fled_cdev;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct mutex lock;
+
+	char led_name[LED_MAX_NAME_SIZE];
+
+	unsigned int strobe_timeout;
+	unsigned int last_flag;
+
+	u32 torch_current_max;
+	u32 strobe_current_max;
+	u32 max_strobe_timeout;
+
+	u32 led_mode;
+};
+
+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 led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
+	struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+	int ret, led_mode_val;
+	u8 brightness_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

drivers/leds/leds-lm3601x.c line 200

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+		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;

Use of LED_ON enum can be a bit misleading here, It was designed for LEDs that can be only turned on/off.

You can use DIV_ROUND_UP to achieve the same in a more clear way.

Quoted 62 lines
drivers/leds/leds-lm3601x.c new file +62 -0
+	else
+		brightness_val = brightness / LM3601X_TORCH_REG_DIV;
+
+	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,
+				LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
+				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 timeout_reg_val = 0;
+	int current_timeout;
+	int ret;
+
+	mutex_lock(&led->lock);
+
+	ret = regmap_read(led->regmap, LM3601X_CFG_REG, &current_timeout);
+	if (ret < 0)
+		goto out;
+
+	if (led->strobe_timeout >= LM3601X_TIMEOUT_XOVER)
+		timeout_reg_val = led->strobe_timeout / LM3601X_UPPER_STEP_US + 0x07;
+	else
+		timeout_reg_val = led->strobe_timeout / LM3601X_LOWER_STEP_US - 0x01;
+
+	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_TORCH | LM3601X_MODE_IR_DRV,
+					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_flash *fled_cdev,
+					u32 brightness)
+{
+	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)

drivers/leds/leds-lm3601x.c line 272

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -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;

Ditto.

Quoted 48 lines
drivers/leds/leds-lm3601x.c new file +48 -0
+	else
+		brightness_val = brightness / LM3601X_STROBE_REG_DIV;
+
+	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);

drivers/leds/leds-lm3601x.c line 330

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+
+	lm3601x_read_faults(led);
+
+	*fault = led->last_flag;
+
+	return 0;
+}
+
+static const struct led_flash_ops strobe_ops = {
+	.flash_brightness_set	= lm3601x_strobe_brightness_set,

s/strobe/flash/

drivers/leds/leds-lm3601x.c line 333

Quoted 3 lines
drivers/leds/leds-lm3601x.c new file +3 -0
+	.strobe_set		= lm3601x_strobe_set,
+	.strobe_get		= lm3601x_strobe_get,
+	.timeout_set		= lm3601x_strobe_timeout_set,

Ditto.

drivers/leds/leds-lm3601x.c line 334

Quoted 1 line
drivers/leds/leds-lm3601x.c new file +1 -0
+	.fault_get		= lm3601x_strobe_fault_get,

Ditto.

Quoted 15 lines
drivers/leds/leds-lm3601x.c new file +15 -0
+};
+
+static int lm3601x_register_leds(struct lm3601x_led *led)
+{
+	struct led_classdev *led_cdev;
+	struct led_flash_setting *setting;
+
+	led->fled_cdev.ops = &strobe_ops;
+
+	setting = &led->fled_cdev.timeout;
+	setting->min = LM3601X_MIN_TIMEOUT_US;
+	setting->max = led->max_strobe_timeout;
+	setting->step = LM3601X_LOWER_STEP_US;
+	setting->val = led->max_strobe_timeout;
+

drivers/leds/leds-lm3601x.c line 359

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+	setting = &led->fled_cdev.brightness;
+	setting->min = LM3601X_MIN_STROBE_I_UA;
+	setting->max = LM3601X_MAX_STROBE_I_UA;
+	setting->step = LM3601X_TORCH_REG_DIV;
+	setting->val = led->strobe_current_max;
+
+	led_cdev = &led->fled_cdev.led_cdev;
+	led_cdev->name = led->led_name;
+	led_cdev->brightness_set_blocking = lm3601x_brightness_set;
+	led_cdev->max_brightness = LM3601X_MAX_TORCH_I_UA;

LED class brightness is expressed in levels, so:

led_cdev->max_brightness = DIV_ROUND_UP(LM3601X_MAX_TORCH_I_UA, LM3601X_TORCH_REG_DIV)

Quoted 31 lines
drivers/leds/leds-lm3601x.c new file +31 -0
+	led_cdev->flags |= LED_DEV_CAP_FLASH;
+
+	return led_classdev_flash_register(&led->client->dev, &led->fled_cdev);
+}
+
+static int lm3601x_parse_node(struct lm3601x_led *led,
+			      struct device_node *node)
+{
+	struct fwnode_handle *child = NULL;
+	int ret = -ENODEV;
+	const char *name;
+
+	child = device_get_next_child_node(&led->client->dev, child);
+	if (!child) {
+		dev_err(&led->client->dev, "No LED Child node\n");
+		return ret;
+	}
+
+	ret = fwnode_property_read_u32(child, "reg", &led->led_mode);
+	if (ret) {
+		dev_err(&led->client->dev, "reg 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");
+		ret = -EINVAL;
+		goto out_err;
+	}
+

drivers/leds/leds-lm3601x.c line 400

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+	ret = fwnode_property_read_string(child, "label", &name);
+	if (ret) {
+		if (led->led_mode == LM3601X_LED_TORCH)
+			name = "torch";
+		else
+			name = "infrared";
+	}
+
+	snprintf(led->led_name, sizeof(led->led_name),
+		"%s:%s", node->name, name);

Reading once again my recent explanation regarding this I realized that I didn't provide clear conclusion, which is: we no longer use child node name for LED class device name if label is absent. (apart from that - you're using parent DT node now, i.e. led-controller).

Please follow what was done for drivers/leds/leds-cr0014114.c.

Quoted 47 lines
drivers/leds/leds-lm3601x.c new file +47 -0
+	ret = fwnode_property_read_u32(child, "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;
+	}
+
+	ret = fwnode_property_read_u32(child, "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 = fwnode_property_read_u32(child, "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;
+	}
+
+out_err:
+	fwnode_handle_put(child);
+	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(*led), GFP_KERNEL);
+	if (!led)
+		return -ENOMEM;
+
+	led->client = client;
+	i2c_set_clientdata(client, led);
+
+	err = lm3601x_parse_node(led, client->dev.of_node);
+	if (err < 0)
+		return -ENODEV;
+
+	led->regmap = devm_regmap_init_i2c(client, &lm3601x_regmap);

drivers/leds/leds-lm3601x.c line 458

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+	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);
+
+	err = lm3601x_register_leds(led);

return lm3601x_register_leds(led)

drivers/leds/leds-lm3601x.c line 470

Quoted 12 lines
drivers/leds/leds-lm3601x.c new file +12 -0
+
+	return err;
+}
+
+static int lm3601x_remove(struct i2c_client *client)
+{
+	struct lm3601x_led *led = i2c_get_clientdata(client);
+
+	regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+			   LM3601X_ENABLE_MASK,
+			   LM3601X_MODE_STANDBY);
+

mutex_destroy() is missing.

Quoted 32 lines
drivers/leds/leds-lm3601x.c new file +31 -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");
Signature · 2 lines
Best regards,
Jacek Anaszewski
author
#5 lore raw

Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver

Jacek

Thanks for the review

Quoted 149 lines · Jacek Anaszewski

Hi Dan,

Thank you for the update.

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

---

v7 - Numerous fixes to many to list.  Highlights are moved from OF APIs to device_property APIs, updated LED registration, timeout to reg algorithim, and fixed label generation -
https://patchwork.kernel.org/patch/10401437/

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 | 501 ++++++++++++++++++++++++++++++++++++

  3 files changed, 511 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 +69 -0
new file mode 100644
index 000000000000..7c71cb11d474
@@ -0,0 +1,501 @@
+// 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_IR        0x0
+#define LM3601X_LED_TORCH    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_EDGE_TRIG    BIT(3)
+#define LM3601X_IVFM_EN        BIT(4)
+
+#define LM36010_BOOST_LIMIT_28    BIT(5)
+#define LM36010_BOOST_FREQ_4MHZ    BIT(6)
+#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_MAX_TORCH_I_UA    376000
+#define LM3601X_MIN_TORCH_I_UA    2400
+#define LM3601X_TORCH_REG_DIV    3000
+
+#define LM3601X_MAX_STROBE_I_UA    1500000
+#define LM3601X_MIN_STROBE_I_UA    11000
+#define LM3601X_STROBE_REG_DIV    11800
+
+#define LM3601X_TIMEOUT_MASK    0x1e
+#define LM3601X_ENABLE_MASK    0x03
+
+#define LM3601X_LOWER_STEP_US    40000
+#define LM3601X_UPPER_STEP_US    200000
+#define LM3601X_MIN_TIMEOUT_US    40000
+#define LM3601X_MAX_TIMEOUT_US    1600000
+#define LM3601X_TIMEOUT_XOVER    400000
+
+enum lm3601x_type {
+    CHIP_LM36010 = 0,
Quoted 8 lines

drivers/leds/leds-lm3601x.c line 75

Quoted 6 lines
drivers/leds/leds-lm3601x.c new file +6 -0
+    CHIP_LM36011,
+};
+
+/**
+ * struct lm3601x_led -
+ * @fled_cdev: flash led class device pointer

s/led/LED/

Ack

Quoted 130 lines
Quoted 116 lines
drivers/leds/leds-lm3601x.c new file +115 -0
+ * @client: Pointer to the I2C client
+ * @regmap: Devices register map
+ * @lock: Lock for reading/writing the device
+ * @led_name: LED label for the Torch or IR LED
+ * @strobe_timeout: the timeout for the strobe
+ * @last_flag: last known flags register value
+ * @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 led_classdev_flash fled_cdev;
+    struct i2c_client *client;
+    struct regmap *regmap;
+    struct mutex lock;
+
+    char led_name[LED_MAX_NAME_SIZE];
+
+    unsigned int strobe_timeout;
+    unsigned int last_flag;
+
+    u32 torch_current_max;
+    u32 strobe_current_max;
+    u32 max_strobe_timeout;
+
+    u32 led_mode;
+};
+
+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 led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
+    struct lm3601x_led *led = fled_cdev_to_led(fled_cdev);
+    int ret, led_mode_val;
+    u8 brightness_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

drivers/leds/leds-lm3601x.c line 200

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+        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;

Use of LED_ON enum can be a bit misleading here, It was designed for LEDs that can be only turned on/off.

Quoted 1 line

You can use DIV_ROUND_UP to achieve the same in a more clear way.

ACK

Quoted 67 lines
Quoted 67 lines
drivers/leds/leds-lm3601x.c new file +66 -0
+    else
+        brightness_val = brightness / LM3601X_TORCH_REG_DIV;
+
+    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,
+                LM3601X_MODE_TORCH | LM3601X_MODE_IR_DRV,
+                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 timeout_reg_val = 0;
+    int current_timeout;
+    int ret;
+
+    mutex_lock(&led->lock);
+
+    ret = regmap_read(led->regmap, LM3601X_CFG_REG, &current_timeout);
+    if (ret < 0)
+        goto out;
+
+    if (led->strobe_timeout >= LM3601X_TIMEOUT_XOVER)
+        timeout_reg_val = led->strobe_timeout / LM3601X_UPPER_STEP_US + 0x07;
+    else
+        timeout_reg_val = led->strobe_timeout / LM3601X_LOWER_STEP_US - 0x01;
+
+    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_TORCH | LM3601X_MODE_IR_DRV,
+                    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_flash *fled_cdev,
+                    u32 brightness)
+{
+    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,
Quoted 8 lines

drivers/leds/leds-lm3601x.c line 272

Quoted 6 lines
drivers/leds/leds-lm3601x.c new file +6 -0
+                    LM3601X_MODE_STROBE, LED_OFF);
+        goto out;
+    }
+
+    if (brightness == LED_ON)
+        brightness_val = LED_ON;

Ditto.

ACK

Quoted 53 lines
Quoted 53 lines
drivers/leds/leds-lm3601x.c new file +52 -0
+    else
+        brightness_val = brightness / LM3601X_STROBE_REG_DIV;
+
+    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;
Quoted 8 lines

drivers/leds/leds-lm3601x.c line 330

Quoted 6 lines
drivers/leds/leds-lm3601x.c new file +6 -0
+
+    return 0;
+}
+
+static const struct led_flash_ops strobe_ops = {
+    .flash_brightness_set    = lm3601x_strobe_brightness_set,

s/strobe/flash/

ACK

Quoted 6 lines

drivers/leds/leds-lm3601x.c line 333

Quoted 4 lines
drivers/leds/leds-lm3601x.c new file +3 -0
+    .strobe_set        = lm3601x_strobe_set,
+    .strobe_get        = lm3601x_strobe_get,
+    .timeout_set        = lm3601x_strobe_timeout_set,

Ditto.

ACK

Quoted 4 lines

drivers/leds/leds-lm3601x.c line 334

Quoted 2 lines
drivers/leds/leds-lm3601x.c new file +1 -0
+    .fault_get        = lm3601x_strobe_fault_get,

Ditto.

ACK

Quoted 29 lines
Quoted 16 lines
drivers/leds/leds-lm3601x.c new file +15 -0
+};
+
+static int lm3601x_register_leds(struct lm3601x_led *led)
+{
+    struct led_classdev *led_cdev;
+    struct led_flash_setting *setting;
+
+    led->fled_cdev.ops = &strobe_ops;
+
+    setting = &led->fled_cdev.timeout;
+    setting->min = LM3601X_MIN_TIMEOUT_US;
+    setting->max = led->max_strobe_timeout;
+    setting->step = LM3601X_LOWER_STEP_US;
+    setting->val = led->max_strobe_timeout;
+

drivers/leds/leds-lm3601x.c line 359

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+    setting = &led->fled_cdev.brightness;
+    setting->min = LM3601X_MIN_STROBE_I_UA;
+    setting->max = LM3601X_MAX_STROBE_I_UA;
+    setting->step = LM3601X_TORCH_REG_DIV;
+    setting->val = led->strobe_current_max;
+
+    led_cdev = &led->fled_cdev.led_cdev;
+    led_cdev->name = led->led_name;
+    led_cdev->brightness_set_blocking = lm3601x_brightness_set;
+    led_cdev->max_brightness = LM3601X_MAX_TORCH_I_UA;

LED class brightness is expressed in levels, so:

Quoted 1 line

led_cdev->max_brightness = DIV_ROUND_UP(LM3601X_MAX_TORCH_I_UA, LM3601X_TORCH_REG_DIV)

ACK

Quoted 49 lines
Quoted 32 lines
drivers/leds/leds-lm3601x.c new file +31 -0
+    led_cdev->flags |= LED_DEV_CAP_FLASH;
+
+    return led_classdev_flash_register(&led->client->dev, &led->fled_cdev);
+}
+
+static int lm3601x_parse_node(struct lm3601x_led *led,
+                  struct device_node *node)
+{
+    struct fwnode_handle *child = NULL;
+    int ret = -ENODEV;
+    const char *name;
+
+    child = device_get_next_child_node(&led->client->dev, child);
+    if (!child) {
+        dev_err(&led->client->dev, "No LED Child node\n");
+        return ret;
+    }
+
+    ret = fwnode_property_read_u32(child, "reg", &led->led_mode);
+    if (ret) {
+        dev_err(&led->client->dev, "reg 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");
+        ret = -EINVAL;
+        goto out_err;
+    }
+

drivers/leds/leds-lm3601x.c line 400

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+    ret = fwnode_property_read_string(child, "label", &name);
+    if (ret) {
+        if (led->led_mode == LM3601X_LED_TORCH)
+            name = "torch";
+        else
+            name = "infrared";
+    }
+
+    snprintf(led->led_name, sizeof(led->led_name),
+        "%s:%s", node->name, name);

Reading once again my recent explanation regarding this I realized that I didn't provide clear conclusion, which is: we no longer use child node name for LED class device name if label is absent. (apart from that - you're using parent DT node now, i.e. led-controller).

Quoted 1 line

Please follow what was done for drivers/leds/leds-cr0014114.c.

Hmmm. If this is calling dev->of_node->name to store the name will this work in non-DT configurations?

I have not dug to deeply into the fwnode code to find out how the nodes get populated. So my question may not even be valid.

Quoted 52 lines
Quoted 52 lines
drivers/leds/leds-lm3601x.c new file +51 -0
+    ret = fwnode_property_read_u32(child, "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;
+    }
+
+    ret = fwnode_property_read_u32(child, "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 = fwnode_property_read_u32(child, "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;
+    }
+
+out_err:
+    fwnode_handle_put(child);
+    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(*led), GFP_KERNEL);
+    if (!led)
+        return -ENOMEM;
+
+    led->client = client;
+    i2c_set_clientdata(client, led);
+
+    err = lm3601x_parse_node(led, client->dev.of_node);
+    if (err < 0)
+        return -ENODEV;
+
+    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);
Quoted 8 lines

drivers/leds/leds-lm3601x.c line 458

Quoted 6 lines
drivers/leds/leds-lm3601x.c new file +6 -0
+        return err;
+    }
+
+    mutex_init(&led->lock);
+
+    err = lm3601x_register_leds(led);

return lm3601x_register_leds(led)

ACK. I will end up dumping the err variable altogether

Quoted 49 lines

drivers/leds/leds-lm3601x.c line 470

Quoted 13 lines
drivers/leds/leds-lm3601x.c new file +12 -0
+
+    return err;
+}
+
+static int lm3601x_remove(struct i2c_client *client)
+{
+    struct lm3601x_led *led = i2c_get_clientdata(client);
+
+    regmap_update_bits(led->regmap, LM3601X_ENABLE_REG,
+               LM3601X_ENABLE_MASK,
+               LM3601X_MODE_STANDBY);
+

mutex_destroy() is missing.

Quoted 32 lines
drivers/leds/leds-lm3601x.c new file +31 -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");
Signature · 2 lines
------------------
Dan Murphy
#6 lore raw

Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver

Quoted 36 lines · Dan Murphy
Quoted 36 lines
Quoted 19 lines
drivers/leds/leds-lm3601x.c new file +19 -0
+    child = device_get_next_child_node(&led->client->dev, child);
+    if (!child) {
+        dev_err(&led->client->dev, "No LED Child node\n");
+        return ret;
+    }
+
+    ret = fwnode_property_read_u32(child, "reg", &led->led_mode);
+    if (ret) {
+        dev_err(&led->client->dev, "reg 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");
+        ret = -EINVAL;
+        goto out_err;
+    }
+

drivers/leds/leds-lm3601x.c line 400

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+    ret = fwnode_property_read_string(child, "label", &name);
+    if (ret) {
+        if (led->led_mode == LM3601X_LED_TORCH)
+            name = "torch";
+        else
+            name = "infrared";
+    }
+
+    snprintf(led->led_name, sizeof(led->led_name),
+        "%s:%s", node->name, name);

Reading once again my recent explanation regarding this I realized that I didn't provide clear conclusion, which is: we no longer use child node name for LED class device name if label is absent. (apart from that - you're using parent DT node now, i.e. led-controller).

Quoted 4 lines
Quoted 1 line

Please follow what was done for drivers/leds/leds-cr0014114.c.

Hmmm. If this is calling dev->of_node->name to store the name will this work in non-DT configurations?

I didn't found this kind of use in linux-next, perhaps I missed something?

In the driver Jacek referred to I found, though, use of of_node, which at some point should be changed to fwnode.

For now you can fill that if you want to using something like this (IIRC it should work):

if (is_of_node(fwnode))
...->of_node = to_of_node(...);

Quoted 2 lines

I have not dug to deeply into the fwnode code to find out how the nodes get populated. So my question may not even be valid.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#7 lore raw

Re: [PATCH v7 2/2] leds: lm3601x: Introduce the lm3601x LED driver

Andy

Quoted 39 lines · Andy Shevchenko
Quoted 36 lines · Dan Murphy
Quoted 36 lines
Quoted 19 lines
drivers/leds/leds-lm3601x.c new file +19 -0
+    child = device_get_next_child_node(&led->client->dev, child);
+    if (!child) {
+        dev_err(&led->client->dev, "No LED Child node\n");
+        return ret;
+    }
+
+    ret = fwnode_property_read_u32(child, "reg", &led->led_mode);
+    if (ret) {
+        dev_err(&led->client->dev, "reg 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");
+        ret = -EINVAL;
+        goto out_err;
+    }
+

drivers/leds/leds-lm3601x.c line 400

Quoted 10 lines
drivers/leds/leds-lm3601x.c new file +10 -0
+    ret = fwnode_property_read_string(child, "label", &name);
+    if (ret) {
+        if (led->led_mode == LM3601X_LED_TORCH)
+            name = "torch";
+        else
+            name = "infrared";
+    }
+
+    snprintf(led->led_name, sizeof(led->led_name),
+        "%s:%s", node->name, name);

Reading once again my recent explanation regarding this I realized that I didn't provide clear conclusion, which is: we no longer use child node name for LED class device name if label is absent. (apart from that - you're using parent DT node now, i.e. led-controller).

Quoted 7 lines
Quoted 4 lines
Quoted 1 line

Please follow what was done for drivers/leds/leds-cr0014114.c.

Hmmm. If this is calling dev->of_node->name to store the name will this work in non-DT configurations?

I didn't found this kind of use in linux-next, perhaps I missed something?

Linux next has a different version then what I found doing a search for the driver.

I will change the code to match what is in linux-next.

Dan

Quoted 12 lines

In the driver Jacek referred to I found, though, use of of_node, which at some point should be changed to fwnode.

For now you can fill that if you want to using something like this (IIRC it should work):

if (is_of_node(fwnode))
...->of_node = to_of_node(...);

Quoted 2 lines

I have not dug to deeply into the fwnode code to find out how the nodes get populated. So my question may not even be valid.

Signature · 2 lines
------------------
Dan Murphy