[PATCH 0/2] Support for Texas Instruments TPS6131X flash LED driver

[PATCH 0/2] Support for Texas Instruments TPS6131X flash LED driver

[Matthias Fend]

The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
stage is capable of supplying a maximum total current of roughly 1500mA.
The TPS6131x provides three constant-current sinks, capable of sinking up
to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
each sink (LED1, LED2, LED3) supports currents up to 175m

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
Matthias Fend (2):
      dt-bindings: leds: add Texas Instruments TPS6131x flash LED driver
      leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

 .../devicetree/bindings/leds/ti,tps6131x.yaml      | 123 ++++
 MAINTAINERS                                        |   7 +
 drivers/leds/flash/Kconfig                         |  11 +
 drivers/leds/flash/Makefile                        |   1 +
 drivers/leds/flash/leds-tps6131x.c                 | 798 +++++++++++++++++++++
 5 files changed, 940 insertions(+)
---
base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
change-id: 20250227-leds-tps6131x-a7437883e400

Best regards,
-- 
Matthias Fend <matthias.fend@emfend.at>

[PATCH 1/2] dt-bindings: leds: add Texas Instruments TPS6131x flash LED driver

[Matthias Fend]

Document Texas Instruments TPS61310/TPS61311 flash LED driver devicetree
bindings.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 .../devicetree/bindings/leds/ti,tps6131x.yaml      | 123 +++++++++++++++++++++
 1 file changed, 123 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml b/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..c08b3cef7abcec07237d3271456ff1f888b2b809
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml
@@ -0,0 +1,123 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/leds/ti,tps6131x.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Texas Instruments TPS6131X flash LED driver
+
+maintainers:
+  - Matthias Fend <matthias.fend@emfend.at>
+
+description: |
+  The TPS61310/TPS61311 is a flash LED driver with I2C interface.
+  Its power stage is capable of supplying a maximum total current of roughly 1500mA.
+  The TPS6131x provides three constant-current sinks, capable of sinking
+  up to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode.
+  In torch mode, each sink (LED1, LED2, LED3) supports currents up to 175mA.
+
+  The data sheet can be found at:
+    https://www.ti.com/lit/ds/symlink/tps61310.pdf
+
+properties:
+  compatible:
+    enum:
+      - ti,tps61310
+      - ti,tps61311
+
+  reg:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  reset-gpios:
+    maxItems: 1
+    description: GPIO connected to NRESET pin
+
+  ti,valley-current-limit:
+    type: boolean
+    description:
+      Reduce the valley peak current limit from 1750mA to 1250mA (TPS61310) or
+      from 2480mA to 1800mA (TPS61311).
+
+  led:
+    type: object
+    $ref: common.yaml#
+    unevaluatedProperties: false
+
+    properties:
+      led-sources:
+        allOf:
+          - minItems: 1
+            maxItems: 3
+            items:
+              enum: [1, 2, 3]
+
+      led-max-microamp:
+        anyOf:
+          - minimum: 25000
+            maximum: 350000
+            multipleOf: 50000
+          - minimum: 25000
+            maximum: 525000
+            multipleOf: 25000
+
+      flash-max-microamp:
+        anyOf:
+          - minimum: 25000
+            maximum: 800000
+            multipleOf: 50000
+          - minimum: 25000
+            maximum: 1500000
+            multipleOf: 25000
+
+      flash-max-timeout-us:
+        enum: [ 5300, 10700, 16000, 21300, 26600, 32000, 37300, 68200, 71500,
+                102200, 136300, 170400, 204500, 340800, 579300, 852000 ]
+
+    required:
+      - led-sources
+      - led-max-microamp
+      - flash-max-microamp
+      - flash-max-timeout-us
+
+required:
+  - compatible
+  - reg
+  - '#address-cells'
+  - '#size-cells'
+  - led
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/leds/common.h>
+    #include <dt-bindings/gpio/gpio.h>
+
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      led-controller@33 {
+        compatible = "ti,tps61310";
+        reg = <0x33>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        reset-gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;
+
+        tps61310_flash: led {
+          function = LED_FUNCTION_FLASH;
+          color = <LED_COLOR_ID_WHITE>;
+          led-sources = <1>, <2>, <3>;
+          led-max-microamp = <525000>;
+          flash-max-microamp = <1500000>;
+          flash-max-timeout-us = <852000>;
+        };
+      };
+    };

-- 
2.34.1

[PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Matthias Fend]

The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
stage is capable of supplying a maximum total current of roughly 1500mA.
The TPS6131x provides three constant-current sinks, capable of sinking up
to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
each sink (LED1, LED2, LED3) supports currents up to 175mA.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
---
 MAINTAINERS                        |   7 +
 drivers/leds/flash/Kconfig         |  11 +
 drivers/leds/flash/Makefile        |   1 +
 drivers/leds/flash/leds-tps6131x.c | 798 +++++++++++++++++++++++++++++++++++++
 4 files changed, 817 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 0fa7c5728f1e64d031f4a47b6fce1db484ce0fc2..d2ca840647b566cfee4d8ded1f787e32be4aa163 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -23225,6 +23225,13 @@ F:	Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
 F:	Documentation/hwmon/tps23861.rst
 F:	drivers/hwmon/tps23861.c
 
+TEXAS INSTRUMENTS TPS6131X FLASH LED DRIVER
+M:	Matthias Fend <matthias.fend@emfend.at>
+L:	linux-leds@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/leds/ti,tps6131x.yaml
+F:	drivers/leds/flash/leds-tps6131x.c
+
 TEXAS INSTRUMENTS' DAC7612 DAC DRIVER
 M:	Ricardo Ribalda <ribalda@kernel.org>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
index f39f0bfe6eefcd376405d9d35dc36e323a485002..55ca663ca506ad8be627f58f6d6308368ea2b928 100644
--- a/drivers/leds/flash/Kconfig
+++ b/drivers/leds/flash/Kconfig
@@ -132,4 +132,15 @@ config LEDS_SY7802
 
 	  This driver can be built as a module, it will be called "leds-sy7802".
 
+config LEDS_TPS6131X
+	tristate "LED support for TI TPS6131x flash LED driver"
+	depends on I2C && OF
+	depends on GPIOLIB
+	select REGMAP_I2C
+	help
+	  This option enables support for Texas Instruments TPS61310/TPS61311
+	  flash LED driver.
+
+	  This driver can be built as a module, it will be called "leds-tps6131x".
+
 endif # LEDS_CLASS_FLASH
diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
index 48860eeced79513a0ed303e4af3db9bfe9724b7e..712fb737a428e42747e1aa339058dc4306ade9c8 100644
--- a/drivers/leds/flash/Makefile
+++ b/drivers/leds/flash/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_LEDS_RT4505)	+= leds-rt4505.o
 obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
 obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
 obj-$(CONFIG_LEDS_SY7802)	+= leds-sy7802.o
+obj-$(CONFIG_LEDS_TPS6131X)	+= leds-tps6131x.o
diff --git a/drivers/leds/flash/leds-tps6131x.c b/drivers/leds/flash/leds-tps6131x.c
new file mode 100644
index 0000000000000000000000000000000000000000..5fca542ac88da1db755a75e982dee8e3dde06373
--- /dev/null
+++ b/drivers/leds/flash/leds-tps6131x.c
@@ -0,0 +1,798 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Texas Instruments TPS61310/TPS61311 flash LED driver with I2C interface
+ *
+ * Copyright 2025 Matthias Fend <matthias.fend@emfend.at>
+ */
+
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/led-class-flash.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <media/v4l2-flash-led-class.h>
+
+/* Registers */
+
+#define TPS6131X_REG_0				0x00
+#define TPS6131X_REG_0_RESET			BIT(7)
+#define TPS6131X_REG_0_DCLC13			GENMASK(5, 3)
+#define TPS6131X_REG_0_DCLC13_SHIFT		3
+#define TPS6131X_REG_0_DCLC2			GENMASK(2, 0)
+#define TPS6131X_REG_0_DCLC2_SHIFT		0
+
+#define TPS6131X_REG_1				0x01
+#define TPS6131X_REG_1_MODE			GENMASK(7, 6)
+#define TPS6131X_REG_1_MODE_SHIFT		6
+#define TPS6131X_REG_1_FC2			GENMASK(5, 0)
+#define TPS6131X_REG_1_FC2_SHIFT		0
+
+#define TPS6131X_REG_2				0x02
+#define TPS6131X_REG_2_MODE			GENMASK(7, 6)
+#define TPS6131X_REG_2_MODE_SHIFT		6
+#define TPS6131X_REG_2_ENVM			BIT(5)
+#define TPS6131X_REG_2_FC13			GENMASK(4, 0)
+#define TPS6131X_REG_2_FC13_SHIFT		0
+
+#define TPS6131X_REG_3				0x03
+#define TPS6131X_REG_3_STIM			GENMASK(7, 5)
+#define TPS6131X_REG_3_STIM_SHIFT		5
+#define TPS6131X_REG_3_HPFL			BIT(4)
+#define TPS6131X_REG_3_SELSTIM_TO		BIT(3)
+#define TPS6131X_REG_3_STT			BIT(2)
+#define TPS6131X_REG_3_SFT			BIT(1)
+#define TPS6131X_REG_3_TXMASK			BIT(0)
+
+#define TPS6131X_REG_4				0x04
+#define TPS6131X_REG_4_PG			BIT(7)
+#define TPS6131X_REG_4_HOTDIE_HI		BIT(6)
+#define TPS6131X_REG_4_HOTDIE_LO		BIT(5)
+#define TPS6131X_REG_4_ILIM			BIT(4)
+#define TPS6131X_REG_4_INDC			GENMASK(3, 0)
+#define TPS6131X_REG_4_SHIFT			0
+
+#define TPS6131X_REG_5				0x05
+#define TPS6131X_REG_5_SELFCAL			BIT(7)
+#define TPS6131X_REG_5_ENPSM			BIT(6)
+#define TPS6131X_REG_5_STSTRB1_DIR		BIT(5)
+#define TPS6131X_REG_5_GPIO			BIT(4)
+#define TPS6131X_REG_5_GPIOTYPE			BIT(3)
+#define TPS6131X_REG_5_ENLED3			BIT(2)
+#define TPS6131X_REG_5_ENLED2			BIT(1)
+#define TPS6131X_REG_5_ENLED1			BIT(0)
+
+#define TPS6131X_REG_6				0x06
+#define TPS6131X_REG_6_ENTS			BIT(7)
+#define TPS6131X_REG_6_LEDHOT			BIT(6)
+#define TPS6131X_REG_6_LEDWARN			BIT(5)
+#define TPS6131X_REG_6_LEDHDR			BIT(4)
+#define TPS6131X_REG_6_OV			GENMASK(3, 0)
+#define TPS6131X_REG_6_OV_SHIFT			0
+
+#define TPS6131X_REG_7				0x07
+#define TPS6131X_REG_7_ENBATMON			BIT(7)
+#define TPS6131X_REG_7_BATDROOP			GENMASK(6, 4)
+#define TPS6131X_REG_7_BATDROOP_SHIFT		4
+#define TPS6131X_REG_7_REVID			GENMASK(2, 0)
+#define TPS6131X_REG_7_REVID_SHIFT		0
+
+/* Constants */
+
+#define TPS6131X_MAX_CHANNELS			3
+
+#define TPS6131X_FLASH_MAX_I_CHAN13_MA		400
+#define TPS6131X_FLASH_MAX_I_CHAN2_MA		800
+#define TPS6131X_FLASH_STEP_I_MA		25
+
+#define TPS6131X_TORCH_MAX_I_CHAN13_MA		175
+#define TPS6131X_TORCH_MAX_I_CHAN2_MA		175
+#define TPS6131X_TORCH_STEP_I_MA		25
+
+#define TPS6131X_TORCH_REFRESH_INTERVAL_JIFFIES msecs_to_jiffies(10000)
+
+struct tps6131x {
+	struct i2c_client *client;
+	struct regmap *regmap;
+	struct gpio_desc *reset_gpio;
+	struct mutex lock; /* */
+	struct delayed_work torch_refresh_work;
+	bool valley_current_limit;
+	bool chan1_en;
+	bool chan2_en;
+	bool chan3_en;
+	struct fwnode_handle *led_node;
+	u32 max_flash_current_ma;
+	u32 step_flash_current_ma;
+	u32 max_torch_current_ma;
+	u32 step_torch_current_ma;
+	u32 max_timeout_us;
+	struct led_classdev_flash fled_cdev;
+	struct v4l2_flash *v4l2_flash;
+};
+
+static struct tps6131x *fled_cdev_to_tps6131x(struct led_classdev_flash *fled_cdev)
+{
+	return container_of(fled_cdev, struct tps6131x, fled_cdev);
+}
+
+static const struct reg_default tps6131x_regmap_defaults[] = {
+	{ TPS6131X_REG_0, 0x0A },
+	{ TPS6131X_REG_1, 0x10 },
+	{ TPS6131X_REG_2, 0x08 },
+	{ TPS6131X_REG_3, 0xC1 },
+	{ TPS6131X_REG_4, 0x00 },
+	{ TPS6131X_REG_5, 0x6A },
+	{ TPS6131X_REG_6, 0x00 },
+	{ TPS6131X_REG_7, 0x46 },
+};
+
+/*
+ * These registers contain flags that are reset when read. Ensure that these registers are not read
+ * outside of a call from the driver.
+ */
+static bool tps6131x_regmap_precious(struct device *dev, unsigned int reg)
+{
+	switch (reg) {
+	case TPS6131X_REG_3:
+	case TPS6131X_REG_4:
+	case TPS6131X_REG_6:
+		return true;
+	default:
+		return false;
+	}
+}
+
+static const struct regmap_config tps6131x_regmap = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = TPS6131X_REG_7,
+	.reg_defaults = tps6131x_regmap_defaults,
+	.num_reg_defaults = ARRAY_SIZE(tps6131x_regmap_defaults),
+	.cache_type = REGCACHE_FLAT,
+	.precious_reg = &tps6131x_regmap_precious,
+};
+
+struct tps6131x_timer_config {
+	u8 val;
+	u8 range;
+	u32 time_us;
+};
+
+static const struct tps6131x_timer_config tps6131x_timer_configs[] = {
+	{ .val = 0, .range = 1, .time_us = 5300 },
+	{ .val = 1, .range = 1, .time_us = 10700 },
+	{ .val = 2, .range = 1, .time_us = 16000 },
+	{ .val = 3, .range = 1, .time_us = 21300 },
+	{ .val = 4, .range = 1, .time_us = 26600 },
+	{ .val = 5, .range = 1, .time_us = 32000 },
+	{ .val = 6, .range = 1, .time_us = 37300 },
+	{ .val = 0, .range = 0, .time_us = 68200 },
+	{ .val = 7, .range = 1, .time_us = 71500 },
+	{ .val = 1, .range = 0, .time_us = 102200 },
+	{ .val = 2, .range = 0, .time_us = 136300 },
+	{ .val = 3, .range = 0, .time_us = 170400 },
+	{ .val = 4, .range = 0, .time_us = 204500 },
+	{ .val = 5, .range = 0, .time_us = 340800 },
+	{ .val = 6, .range = 0, .time_us = 579300 },
+	{ .val = 7, .range = 0, .time_us = 852000 },
+};
+
+static const struct tps6131x_timer_config *tps6131x_find_closest_timer_config(u32 timeout_us)
+{
+	const struct tps6131x_timer_config *timer_config = &tps6131x_timer_configs[0];
+	u32 diff, min_diff = U32_MAX;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tps6131x_timer_configs); i++) {
+		diff = abs(tps6131x_timer_configs[i].time_us - timeout_us);
+		if (diff < min_diff) {
+			timer_config = &tps6131x_timer_configs[i];
+			min_diff = diff;
+			if (!min_diff)
+				break;
+		}
+	}
+
+	return timer_config;
+}
+
+static int tps6131x_reset_chip(struct tps6131x *tps6131x)
+{
+	int ret;
+
+	if (IS_ERR_OR_NULL(tps6131x->reset_gpio)) {
+		ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_0, TPS6131X_REG_0_RESET,
+					 TPS6131X_REG_0_RESET);
+		if (ret)
+			return ret;
+
+		ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_0, TPS6131X_REG_0_RESET, 0);
+		if (ret)
+			return ret;
+	} else {
+		gpiod_set_value_cansleep(tps6131x->reset_gpio, 1);
+		fsleep(10);
+		gpiod_set_value_cansleep(tps6131x->reset_gpio, 0);
+	}
+
+	return 0;
+}
+
+static int tps6131x_init_chip(struct tps6131x *tps6131x)
+{
+	u32 reg4, reg5, reg6;
+	int ret;
+
+	reg4 = tps6131x->valley_current_limit ? TPS6131X_REG_4_ILIM : 0;
+	ret = regmap_write(tps6131x->regmap, TPS6131X_REG_4, reg4);
+	if (ret)
+		return ret;
+
+	reg5 = TPS6131X_REG_5_ENPSM | TPS6131X_REG_5_STSTRB1_DIR | TPS6131X_REG_5_GPIOTYPE;
+	if (tps6131x->chan1_en)
+		reg5 |= TPS6131X_REG_5_ENLED1;
+
+	if (tps6131x->chan2_en)
+		reg5 |= TPS6131X_REG_5_ENLED2;
+
+	if (tps6131x->chan3_en)
+		reg5 |= TPS6131X_REG_5_ENLED3;
+	ret = regmap_write(tps6131x->regmap, TPS6131X_REG_5, reg5);
+	if (ret)
+		return ret;
+
+	reg6 = TPS6131X_REG_6_ENTS;
+	ret = regmap_write(tps6131x->regmap, TPS6131X_REG_6, reg6);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+enum tps6131x_mode {
+	TPS6131X_MODE_SHUTDOWN,
+	TPS6131X_MODE_TORCH,
+	TPS6131X_MODE_FLASH,
+};
+
+static int tps6131x_set_mode(struct tps6131x *tps6131x, enum tps6131x_mode mode, bool force)
+{
+	u8 val;
+
+	switch (mode) {
+	case TPS6131X_MODE_FLASH:
+		val = 2 << TPS6131X_REG_1_MODE_SHIFT;
+		break;
+	case TPS6131X_MODE_TORCH:
+		val = 1 << TPS6131X_REG_1_MODE_SHIFT;
+		break;
+	case TPS6131X_MODE_SHUTDOWN:
+	default:
+		val = 0 << TPS6131X_REG_1_MODE_SHIFT;
+		break;
+	}
+
+	return regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_1, TPS6131X_REG_1_MODE, val,
+				       NULL, false, force);
+}
+
+static void tps6131x_torch_refresh_handler(struct work_struct *work)
+{
+	struct tps6131x *tps6131x = container_of(work, struct tps6131x, torch_refresh_work.work);
+
+	guard(mutex)(&tps6131x->lock);
+
+	tps6131x_set_mode(tps6131x, TPS6131X_MODE_TORCH, true);
+
+	schedule_delayed_work(&tps6131x->torch_refresh_work,
+			      TPS6131X_TORCH_REFRESH_INTERVAL_JIFFIES);
+}
+
+static int tps6131x_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
+{
+	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
+	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
+	int ret;
+	u8 reg0;
+	u32 steps_remaining, steps_chan13, steps_chan2;
+	unsigned int num_chans = tps6131x->chan1_en + tps6131x->chan2_en + tps6131x->chan3_en;
+
+	cancel_delayed_work_sync(&tps6131x->torch_refresh_work);
+
+	guard(mutex)(&tps6131x->lock);
+
+	/*
+	 * The brightness parameter uses the number of current steps as the unit (not the current
+	 * value itself). Since the reported step size can vary depending on the configuration,
+	 * this value must be converted into actual register steps.
+	 */
+	steps_remaining = (brightness * tps6131x->step_torch_current_ma) / TPS6131X_TORCH_STEP_I_MA;
+
+	/*
+	 * The currents are distributed as evenly as possible across the activated channels.
+	 * Since channels 1 and 3 share the same register setting, they always use the same current
+	 * value. Channel 2 supports higher currents and thus takes over the remaining additional
+	 * portion that cannot be covered by the other channels.
+	 */
+	steps_chan13 = min_t(u32, steps_remaining / num_chans,
+			     TPS6131X_TORCH_MAX_I_CHAN13_MA / TPS6131X_TORCH_STEP_I_MA);
+	if (tps6131x->chan1_en)
+		steps_remaining -= steps_chan13;
+	if (tps6131x->chan3_en)
+		steps_remaining -= steps_chan13;
+
+	steps_chan2 = min_t(u32, steps_remaining,
+			    TPS6131X_TORCH_MAX_I_CHAN2_MA / TPS6131X_TORCH_STEP_I_MA);
+
+	reg0 = (steps_chan13 << TPS6131X_REG_0_DCLC13_SHIFT) |
+	       (steps_chan2 << TPS6131X_REG_0_DCLC2_SHIFT);
+	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_0,
+				 TPS6131X_REG_0_DCLC13 | TPS6131X_REG_0_DCLC2, reg0);
+	if (ret < 0)
+		return ret;
+
+	ret = tps6131x_set_mode(tps6131x, brightness ? TPS6131X_MODE_TORCH : TPS6131X_MODE_SHUTDOWN,
+				true);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * In order to use both the flash and the video light functions purely via the I2C
+	 * interface, STRB1 must be low. If STRB1 is low, then the video light watchdog timer
+	 * is also active, which puts the device into the shutdown state after around 13 seconds.
+	 * To prevent this, the mode must be refreshed within the watchdog timeout.
+	 */
+	if (brightness)
+		schedule_delayed_work(&tps6131x->torch_refresh_work,
+				      TPS6131X_TORCH_REFRESH_INTERVAL_JIFFIES);
+
+	return 0;
+}
+
+static int tps6131x_strobe_set(struct led_classdev_flash *fled_cdev, bool state)
+{
+	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
+	int ret;
+
+	guard(mutex)(&tps6131x->lock);
+
+	ret = tps6131x_set_mode(tps6131x, state ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
+				true);
+	if (ret < 0)
+		return ret;
+
+	if (state) {
+		ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_3, TPS6131X_REG_3_SFT,
+					      TPS6131X_REG_3_SFT, NULL, false, true);
+		if (ret)
+			return ret;
+	}
+
+	ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_3, TPS6131X_REG_3_SFT, 0, NULL,
+				      false, true);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int tps6131x_flash_brightness_set(struct led_classdev_flash *fled_cdev, u32 brightness)
+{
+	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
+	u32 steps_remaining, steps_chan13, steps_chan2, num_chans;
+	int ret;
+
+	guard(mutex)(&tps6131x->lock);
+
+	steps_remaining = brightness / TPS6131X_FLASH_STEP_I_MA;
+	num_chans = tps6131x->chan1_en + tps6131x->chan2_en + tps6131x->chan3_en;
+
+	steps_chan13 = min_t(u32, steps_remaining / num_chans,
+			     TPS6131X_FLASH_MAX_I_CHAN13_MA / TPS6131X_FLASH_STEP_I_MA);
+	if (tps6131x->chan1_en)
+		steps_remaining -= steps_chan13;
+	if (tps6131x->chan3_en)
+		steps_remaining -= steps_chan13;
+	steps_chan2 = min_t(u32, steps_remaining,
+			    TPS6131X_FLASH_MAX_I_CHAN2_MA / TPS6131X_FLASH_STEP_I_MA);
+
+	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_2, TPS6131X_REG_2_FC13,
+				 steps_chan13 << TPS6131X_REG_2_FC13_SHIFT);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_1, TPS6131X_REG_1_FC2,
+				 steps_chan2 << TPS6131X_REG_1_FC2_SHIFT);
+	if (ret < 0)
+		return ret;
+
+	fled_cdev->brightness.val = brightness;
+
+	return 0;
+}
+
+static int tps6131x_flash_timeout_set(struct led_classdev_flash *fled_cdev, u32 timeout_us)
+{
+	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
+	int ret;
+	u8 reg3;
+	const struct tps6131x_timer_config *timer_config;
+
+	guard(mutex)(&tps6131x->lock);
+
+	timer_config = tps6131x_find_closest_timer_config(timeout_us);
+
+	reg3 = timer_config->val << TPS6131X_REG_3_STIM_SHIFT;
+	if (timer_config->range)
+		reg3 |= TPS6131X_REG_3_SELSTIM_TO;
+
+	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_3,
+				 TPS6131X_REG_3_STIM | TPS6131X_REG_3_SELSTIM_TO, reg3);
+	if (ret < 0)
+		return ret;
+
+	fled_cdev->timeout.val = timer_config->time_us;
+
+	return 0;
+}
+
+static int tps6131x_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
+{
+	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
+	unsigned int reg3;
+	int ret;
+
+	guard(mutex)(&tps6131x->lock);
+
+	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_3, &reg3);
+	if (ret)
+		return ret;
+
+	*state = !!(reg3 & TPS6131X_REG_3_SFT);
+
+	return 0;
+}
+
+static int tps6131x_flash_fault_get(struct led_classdev_flash *fled_cdev, u32 *fault)
+{
+	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
+	unsigned int reg3, reg4, reg6;
+	int ret;
+
+	*fault = 0;
+
+	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_3, &reg3);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_4, &reg4);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_6, &reg6);
+	if (ret < 0)
+		return ret;
+
+	if (reg3 & TPS6131X_REG_3_HPFL)
+		*fault |= LED_FAULT_SHORT_CIRCUIT;
+
+	if (reg3 & TPS6131X_REG_3_SELSTIM_TO)
+		*fault |= LED_FAULT_TIMEOUT;
+
+	if (reg4 & TPS6131X_REG_4_HOTDIE_HI)
+		*fault |= LED_FAULT_OVER_TEMPERATURE;
+
+	if (reg6 & (TPS6131X_REG_6_LEDHOT | TPS6131X_REG_6_LEDWARN))
+		*fault |= LED_FAULT_LED_OVER_TEMPERATURE;
+
+	if (!(reg6 & TPS6131X_REG_6_LEDHDR))
+		*fault |= LED_FAULT_UNDER_VOLTAGE;
+
+	if (reg6 & TPS6131X_REG_6_LEDHOT) {
+		ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_6,
+					      TPS6131X_REG_6_LEDHOT, 0, NULL, false, true);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static const struct led_flash_ops flash_ops = {
+	.flash_brightness_set = tps6131x_flash_brightness_set,
+	.strobe_set = tps6131x_strobe_set,
+	.strobe_get = tps6131x_strobe_get,
+	.timeout_set = tps6131x_flash_timeout_set,
+	.fault_get = tps6131x_flash_fault_get,
+};
+
+static int tps6131x_parse_node(struct tps6131x *tps6131x)
+{
+	struct device *dev = &tps6131x->client->dev;
+	int ret;
+	u32 current_ua, timeout_us;
+	const struct tps6131x_timer_config *timer_config;
+	u32 flash_max_i_ma, torch_max_i_ma;
+	u32 current_step_multiplier;
+	u32 channels[TPS6131X_MAX_CHANNELS];
+	unsigned int num_channels;
+	int i;
+
+	tps6131x->valley_current_limit = device_property_read_bool(dev, "ti,valley-current-limit");
+
+	tps6131x->led_node = fwnode_get_next_available_child_node(dev->fwnode, NULL);
+	if (!tps6131x->led_node) {
+		dev_err(dev, "Missing LED node\n");
+		return -EINVAL;
+	}
+
+	num_channels = fwnode_property_count_u32(tps6131x->led_node, "led-sources");
+	if (num_channels <= 0) {
+		dev_err(dev, "Failed to read led-sources property\n");
+		return -EINVAL;
+	}
+
+	if (num_channels > TPS6131X_MAX_CHANNELS) {
+		dev_err(dev, "led-sources count %u exceeds maximum channel count %u\n",
+			num_channels, TPS6131X_MAX_CHANNELS);
+		return -EINVAL;
+	}
+
+	ret = fwnode_property_read_u32_array(tps6131x->led_node, "led-sources", channels,
+					     num_channels);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read led-sources property\n");
+		return ret;
+	}
+
+	flash_max_i_ma = 0;
+	torch_max_i_ma = 0;
+	for (i = 0; i < num_channels; i++) {
+		switch (channels[i]) {
+		case 1:
+			tps6131x->chan1_en = true;
+			flash_max_i_ma += TPS6131X_FLASH_MAX_I_CHAN13_MA;
+			torch_max_i_ma += TPS6131X_TORCH_MAX_I_CHAN13_MA;
+			break;
+		case 2:
+			tps6131x->chan2_en = true;
+			flash_max_i_ma += TPS6131X_FLASH_MAX_I_CHAN2_MA;
+			torch_max_i_ma += TPS6131X_TORCH_MAX_I_CHAN2_MA;
+			break;
+		case 3:
+			tps6131x->chan3_en = true;
+			flash_max_i_ma += TPS6131X_FLASH_MAX_I_CHAN13_MA;
+			torch_max_i_ma += TPS6131X_TORCH_MAX_I_CHAN13_MA;
+			break;
+		default:
+			dev_err(dev, "led-source out of range [1-3]\n");
+			return -EINVAL;
+		}
+	}
+
+	/*
+	 * If only channels 1 and 3 are used, the step size is doubled because the two channels
+	 * share the same current control register.
+	 */
+	current_step_multiplier =
+		(tps6131x->chan1_en && tps6131x->chan3_en && !tps6131x->chan2_en) ? 2 : 1;
+	tps6131x->step_flash_current_ma = current_step_multiplier * TPS6131X_FLASH_STEP_I_MA;
+	tps6131x->step_torch_current_ma = current_step_multiplier * TPS6131X_TORCH_STEP_I_MA;
+
+	ret = fwnode_property_read_u32(tps6131x->led_node, "led-max-microamp", &current_ua);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read led-max-microamp property\n");
+		return ret;
+	}
+
+	tps6131x->max_torch_current_ma = current_ua / 1000;
+
+	if (!tps6131x->max_torch_current_ma || tps6131x->max_torch_current_ma > torch_max_i_ma ||
+	    (tps6131x->max_torch_current_ma % tps6131x->step_torch_current_ma)) {
+		dev_err(dev, "led-max-microamp out of range or not a multiple of %u\n",
+			tps6131x->step_torch_current_ma);
+		return -EINVAL;
+	}
+
+	ret = fwnode_property_read_u32(tps6131x->led_node, "flash-max-microamp", &current_ua);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read flash-max-microamp property\n");
+		return ret;
+	}
+
+	tps6131x->max_flash_current_ma = current_ua / 1000;
+
+	if (!tps6131x->max_flash_current_ma || tps6131x->max_flash_current_ma > flash_max_i_ma ||
+	    (tps6131x->max_flash_current_ma % tps6131x->step_flash_current_ma)) {
+		dev_err(dev, "flash-max-microamp out of range or not a multiple of %u\n",
+			tps6131x->step_flash_current_ma);
+		return -EINVAL;
+	}
+
+	ret = fwnode_property_read_u32(tps6131x->led_node, "flash-max-timeout-us", &timeout_us);
+	if (ret < 0) {
+		dev_err(dev, "Failed to read flash-max-timeout-us property\n");
+		return ret;
+	}
+
+	timer_config = tps6131x_find_closest_timer_config(timeout_us);
+	tps6131x->max_timeout_us = timer_config->time_us;
+
+	if (tps6131x->max_timeout_us != timeout_us)
+		dev_warn(dev, "flash-max-timeout-us %u not supported (using %u)\n", timeout_us,
+			 tps6131x->max_timeout_us);
+
+	return 0;
+}
+
+static int tps6131x_led_class_setup(struct tps6131x *tps6131x)
+{
+	struct led_classdev *led_cdev;
+	struct led_flash_setting *setting;
+	struct led_init_data init_data = {};
+	static const struct tps6131x_timer_config *timer_config;
+	int ret;
+
+	tps6131x->fled_cdev.ops = &flash_ops;
+
+	setting = &tps6131x->fled_cdev.timeout;
+	timer_config = tps6131x_find_closest_timer_config(0);
+	setting->min = timer_config->time_us;
+	setting->max = tps6131x->max_timeout_us;
+	setting->step = 1; /* Only some specific time periods are supported. No fixed step size. */
+	setting->val = setting->min;
+
+	setting = &tps6131x->fled_cdev.brightness;
+	setting->min = tps6131x->step_flash_current_ma;
+	setting->max = tps6131x->max_flash_current_ma;
+	setting->step = tps6131x->step_flash_current_ma;
+	setting->val = setting->min;
+
+	led_cdev = &tps6131x->fled_cdev.led_cdev;
+	led_cdev->brightness_set_blocking = tps6131x_brightness_set;
+	led_cdev->max_brightness = tps6131x->max_torch_current_ma;
+	led_cdev->flags |= LED_DEV_CAP_FLASH;
+
+	init_data.fwnode = tps6131x->led_node;
+	init_data.devicename = NULL;
+	init_data.default_label = NULL;
+	init_data.devname_mandatory = false;
+
+	ret = devm_led_classdev_flash_register_ext(&tps6131x->client->dev, &tps6131x->fled_cdev,
+						   &init_data);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
+
+static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
+{
+	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
+	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
+
+	guard(mutex)(&tps6131x->lock);
+
+	return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
+				 false);
+}
+
+static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
+	.external_strobe_set = tps6131x_flash_external_strobe_set,
+};
+
+static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
+{
+	struct v4l2_flash_config v4l2_cfg = { 0 };
+	struct led_flash_setting *intensity = &v4l2_cfg.intensity;
+
+	intensity->min = tps6131x->step_torch_current_ma;
+	intensity->max = tps6131x->max_torch_current_ma;
+	intensity->step = tps6131x->step_torch_current_ma;
+	intensity->val = intensity->min;
+
+	strscpy(v4l2_cfg.dev_name, tps6131x->fled_cdev.led_cdev.dev->kobj.name,
+		sizeof(v4l2_cfg.dev_name));
+
+	v4l2_cfg.has_external_strobe = true;
+	v4l2_cfg.flash_faults = LED_FAULT_TIMEOUT | LED_FAULT_OVER_TEMPERATURE |
+				LED_FAULT_SHORT_CIRCUIT | LED_FAULT_UNDER_VOLTAGE |
+				LED_FAULT_LED_OVER_TEMPERATURE;
+
+	tps6131x->v4l2_flash = v4l2_flash_init(&tps6131x->client->dev, tps6131x->led_node,
+					       &tps6131x->fled_cdev, &tps6131x_v4l2_flash_ops,
+					       &v4l2_cfg);
+	if (IS_ERR(tps6131x->v4l2_flash)) {
+		dev_err(&tps6131x->client->dev, "v4l2_flash_init failed\n");
+		return PTR_ERR(tps6131x->v4l2_flash);
+	}
+
+	return 0;
+}
+
+#else
+
+static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
+{
+	return 0;
+}
+
+#endif
+
+static int tps6131x_probe(struct i2c_client *client)
+{
+	struct tps6131x *tps6131x;
+	int ret;
+
+	tps6131x = devm_kzalloc(&client->dev, sizeof(*tps6131x), GFP_KERNEL);
+	if (!tps6131x)
+		return -ENOMEM;
+
+	tps6131x->client = client;
+	i2c_set_clientdata(client, tps6131x);
+	mutex_init(&tps6131x->lock);
+	INIT_DELAYED_WORK(&tps6131x->torch_refresh_work, tps6131x_torch_refresh_handler);
+
+	ret = tps6131x_parse_node(tps6131x);
+	if (ret)
+		return -ENODEV;
+
+	tps6131x->regmap = devm_regmap_init_i2c(client, &tps6131x_regmap);
+	if (IS_ERR(tps6131x->regmap)) {
+		ret = PTR_ERR(tps6131x->regmap);
+		dev_err(&client->dev, "Failed to allocate register map\n");
+		return ret;
+	}
+
+	tps6131x->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
+	ret = tps6131x_reset_chip(tps6131x);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Failed to reset LED controller\n");
+
+	ret = tps6131x_init_chip(tps6131x);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Failed to initialize LED controller\n");
+
+	ret = tps6131x_led_class_setup(tps6131x);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Failed to setup led class\n");
+
+	ret = tps6131x_v4l2_setup(tps6131x);
+	if (ret)
+		return dev_err_probe(&client->dev, ret, "Failed to setup v4l2 flash\n");
+
+	return 0;
+}
+
+static void tps6131x_remove(struct i2c_client *client)
+{
+	struct tps6131x *tps6131x = i2c_get_clientdata(client);
+
+	v4l2_flash_release(tps6131x->v4l2_flash);
+
+	cancel_delayed_work_sync(&tps6131x->torch_refresh_work);
+}
+
+static const struct of_device_id of_tps6131x_leds_match[] = {
+	{ .compatible = "ti,tps61310" },
+	{ .compatible = "ti,tps61311" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, of_tps6131x_leds_match);
+
+static struct i2c_driver tps6131x_i2c_driver = {
+	.driver = {
+		.name = "tps6131x",
+		.of_match_table = of_tps6131x_leds_match,
+	},
+	.probe = tps6131x_probe,
+	.remove = tps6131x_remove,
+};
+module_i2c_driver(tps6131x_i2c_driver);
+
+MODULE_DESCRIPTION("Texas Instruments TPS6131X flash LED driver");
+MODULE_AUTHOR("Matthias Fend <matthias.fend@emfend.at>");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: leds: add Texas Instruments TPS6131x flash LED driver

[Conor Dooley]

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

On Fri, Feb 28, 2025 at 11:31:23AM +0100, Matthias Fend wrote:
> Document Texas Instruments TPS61310/TPS61311 flash LED driver devicetree
> bindings.
> 
> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> ---
>  .../devicetree/bindings/leds/ti,tps6131x.yaml      | 123 +++++++++++++++++++++
>  1 file changed, 123 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml b/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..c08b3cef7abcec07237d3271456ff1f888b2b809
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml

With a filename matching one of the compatibles in the file,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> @@ -0,0 +1,123 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/leds/ti,tps6131x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Texas Instruments TPS6131X flash LED driver
> +
> +maintainers:
> +  - Matthias Fend <matthias.fend@emfend.at>
> +
> +description: |
> +  The TPS61310/TPS61311 is a flash LED driver with I2C interface.
> +  Its power stage is capable of supplying a maximum total current of roughly 1500mA.
> +  The TPS6131x provides three constant-current sinks, capable of sinking
> +  up to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode.
> +  In torch mode, each sink (LED1, LED2, LED3) supports currents up to 175mA.
> +
> +  The data sheet can be found at:
> +    https://www.ti.com/lit/ds/symlink/tps61310.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,tps61310
> +      - ti,tps61311
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO connected to NRESET pin
> +
> +  ti,valley-current-limit:
> +    type: boolean
> +    description:
> +      Reduce the valley peak current limit from 1750mA to 1250mA (TPS61310) or
> +      from 2480mA to 1800mA (TPS61311).
> +
> +  led:
> +    type: object
> +    $ref: common.yaml#
> +    unevaluatedProperties: false
> +
> +    properties:
> +      led-sources:
> +        allOf:
> +          - minItems: 1
> +            maxItems: 3
> +            items:
> +              enum: [1, 2, 3]
> +
> +      led-max-microamp:
> +        anyOf:
> +          - minimum: 25000
> +            maximum: 350000
> +            multipleOf: 50000
> +          - minimum: 25000
> +            maximum: 525000
> +            multipleOf: 25000
> +
> +      flash-max-microamp:
> +        anyOf:
> +          - minimum: 25000
> +            maximum: 800000
> +            multipleOf: 50000
> +          - minimum: 25000
> +            maximum: 1500000
> +            multipleOf: 25000
> +
> +      flash-max-timeout-us:
> +        enum: [ 5300, 10700, 16000, 21300, 26600, 32000, 37300, 68200, 71500,
> +                102200, 136300, 170400, 204500, 340800, 579300, 852000 ]
> +
> +    required:
> +      - led-sources
> +      - led-max-microamp
> +      - flash-max-microamp
> +      - flash-max-timeout-us
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#address-cells'
> +  - '#size-cells'
> +  - led
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      led-controller@33 {
> +        compatible = "ti,tps61310";
> +        reg = <0x33>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        reset-gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;
> +
> +        tps61310_flash: led {
> +          function = LED_FUNCTION_FLASH;
> +          color = <LED_COLOR_ID_WHITE>;
> +          led-sources = <1>, <2>, <3>;
> +          led-max-microamp = <525000>;
> +          flash-max-microamp = <1500000>;
> +          flash-max-timeout-us = <852000>;
> +        };
> +      };
> +    };
> 
> -- 
> 2.34.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2] dt-bindings: leds: add Texas Instruments TPS6131x flash LED driver

[Matthias Fend]

Hi Conor,

Am 28.02.2025 um 19:24 schrieb Conor Dooley:
> On Fri, Feb 28, 2025 at 11:31:23AM +0100, Matthias Fend wrote:
>> Document Texas Instruments TPS61310/TPS61311 flash LED driver devicetree
>> bindings.
>>
>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>> ---
>>   .../devicetree/bindings/leds/ti,tps6131x.yaml      | 123 +++++++++++++++++++++
>>   1 file changed, 123 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml b/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..c08b3cef7abcec07237d3271456ff1f888b2b809
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml
> 
> With a filename matching one of the compatibles in the file,
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

thank you very much for your feedback.
Since I found different variants in the LED bindings, I wasn't quite 
sure here.
So is it okay if I simply rename the file to 'ti,tps61310.yaml', even 
though there are multiple compatible strings and the driver is called 
leds-tps6131x?

Thanks
  ~Matthias

> 
> Cheers,
> Conor.
> 
>> @@ -0,0 +1,123 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/leds/ti,tps6131x.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Texas Instruments TPS6131X flash LED driver
>> +
>> +maintainers:
>> +  - Matthias Fend <matthias.fend@emfend.at>
>> +
>> +description: |
>> +  The TPS61310/TPS61311 is a flash LED driver with I2C interface.
>> +  Its power stage is capable of supplying a maximum total current of roughly 1500mA.
>> +  The TPS6131x provides three constant-current sinks, capable of sinking
>> +  up to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode.
>> +  In torch mode, each sink (LED1, LED2, LED3) supports currents up to 175mA.
>> +
>> +  The data sheet can be found at:
>> +    https://www.ti.com/lit/ds/symlink/tps61310.pdf
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,tps61310
>> +      - ti,tps61311
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +    description: GPIO connected to NRESET pin
>> +
>> +  ti,valley-current-limit:
>> +    type: boolean
>> +    description:
>> +      Reduce the valley peak current limit from 1750mA to 1250mA (TPS61310) or
>> +      from 2480mA to 1800mA (TPS61311).
>> +
>> +  led:
>> +    type: object
>> +    $ref: common.yaml#
>> +    unevaluatedProperties: false
>> +
>> +    properties:
>> +      led-sources:
>> +        allOf:
>> +          - minItems: 1
>> +            maxItems: 3
>> +            items:
>> +              enum: [1, 2, 3]
>> +
>> +      led-max-microamp:
>> +        anyOf:
>> +          - minimum: 25000
>> +            maximum: 350000
>> +            multipleOf: 50000
>> +          - minimum: 25000
>> +            maximum: 525000
>> +            multipleOf: 25000
>> +
>> +      flash-max-microamp:
>> +        anyOf:
>> +          - minimum: 25000
>> +            maximum: 800000
>> +            multipleOf: 50000
>> +          - minimum: 25000
>> +            maximum: 1500000
>> +            multipleOf: 25000
>> +
>> +      flash-max-timeout-us:
>> +        enum: [ 5300, 10700, 16000, 21300, 26600, 32000, 37300, 68200, 71500,
>> +                102200, 136300, 170400, 204500, 340800, 579300, 852000 ]
>> +
>> +    required:
>> +      - led-sources
>> +      - led-max-microamp
>> +      - flash-max-microamp
>> +      - flash-max-timeout-us
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +  - led
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/leds/common.h>
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      led-controller@33 {
>> +        compatible = "ti,tps61310";
>> +        reg = <0x33>;
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        reset-gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;
>> +
>> +        tps61310_flash: led {
>> +          function = LED_FUNCTION_FLASH;
>> +          color = <LED_COLOR_ID_WHITE>;
>> +          led-sources = <1>, <2>, <3>;
>> +          led-max-microamp = <525000>;
>> +          flash-max-microamp = <1500000>;
>> +          flash-max-timeout-us = <852000>;
>> +        };
>> +      };
>> +    };
>>
>> -- 
>> 2.34.1
>>

Re: [PATCH 1/2] dt-bindings: leds: add Texas Instruments TPS6131x flash LED driver

[Krzysztof Kozlowski]

On 10/03/2025 08:20, Matthias Fend wrote:
> Hi Conor,
> 
> Am 28.02.2025 um 19:24 schrieb Conor Dooley:
>> On Fri, Feb 28, 2025 at 11:31:23AM +0100, Matthias Fend wrote:
>>> Document Texas Instruments TPS61310/TPS61311 flash LED driver devicetree
>>> bindings.
>>>
>>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>>> ---
>>>   .../devicetree/bindings/leds/ti,tps6131x.yaml      | 123 +++++++++++++++++++++
>>>   1 file changed, 123 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml b/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml
>>> new file mode 100644
>>> index 0000000000000000000000000000000000000000..c08b3cef7abcec07237d3271456ff1f888b2b809
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/leds/ti,tps6131x.yaml
>>
>> With a filename matching one of the compatibles in the file,
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> 
> thank you very much for your feedback.
> Since I found different variants in the LED bindings, I wasn't quite 
> sure here.
> So is it okay if I simply rename the file to 'ti,tps61310.yaml', even 
> though there are multiple compatible strings and the driver is called 
> leds-tps6131x?
> 
I think that was exactly the review comment you got.

Best regards,
Krzysztof

Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Krzysztof Kozlowski]

On 28/02/2025 11:31, Matthias Fend wrote:
> +	tps6131x->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	ret = tps6131x_reset_chip(tps6131x);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to reset LED controller\n");
> +
> +	ret = tps6131x_init_chip(tps6131x);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to initialize LED controller\n");
> +
> +	ret = tps6131x_led_class_setup(tps6131x);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to setup led class\n");
> +
> +	ret = tps6131x_v4l2_setup(tps6131x);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to setup v4l2 flash\n");
> +
> +	return 0;
> +}
> +
> +static void tps6131x_remove(struct i2c_client *client)
> +{
> +	struct tps6131x *tps6131x = i2c_get_clientdata(client);
> +
> +	v4l2_flash_release(tps6131x->v4l2_flash);
> +
> +	cancel_delayed_work_sync(&tps6131x->torch_refresh_work);
> +}
> +
> +static const struct of_device_id of_tps6131x_leds_match[] = {
> +	{ .compatible = "ti,tps61310" },
> +	{ .compatible = "ti,tps61311" },


No differences? So devices are fully compatible? Then it should be
expressed in the binding with fallback. Or the binding description or
commit msg should explain why they are not compatible.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: add Texas Instruments TPS6131x flash LED driver

[Krzysztof Kozlowski]

On 28/02/2025 11:31, Matthias Fend wrote:
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ti,tps61310
> +      - ti,tps61311
> +
> +  reg:
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0

Why do you need these two?

> +
> +  reset-gpios:
> +    maxItems: 1
> +    description: GPIO connected to NRESET pin
> +
> +  ti,valley-current-limit:
> +    type: boolean
> +    description:
> +      Reduce the valley peak current limit from 1750mA to 1250mA (TPS61310) or
> +      from 2480mA to 1800mA (TPS61311).
> +
> +  led:

Why do you have only one led node? Description says three: LED1-3,
unless these are just sinks which always have to be connected to the
same LED?

> +    type: object
> +    $ref: common.yaml#
> +    unevaluatedProperties: false
> +
> +    properties:
> +      led-sources:
> +        allOf:

Drop allOf

> +          - minItems: 1
> +            maxItems: 3
> +            items:
> +              enum: [1, 2, 3]
> +
> +      led-max-microamp:
> +        anyOf:

oneOf

> +          - minimum: 25000
> +            maximum: 350000
> +            multipleOf: 50000
> +          - minimum: 25000
> +            maximum: 525000

Why two different values?

> +            multipleOf: 25000
> +
> +      flash-max-microamp:
> +        anyOf:

oneOf

> +          - minimum: 25000
> +            maximum: 800000
> +            multipleOf: 50000
> +          - minimum: 25000

Same question

> +            maximum: 1500000
> +            multipleOf: 25000
> +
> +      flash-max-timeout-us:
> +        enum: [ 5300, 10700, 16000, 21300, 26600, 32000, 37300, 68200, 71500,
> +                102200, 136300, 170400, 204500, 340800, 579300, 852000 ]
> +
> +    required:
> +      - led-sources
> +      - led-max-microamp
> +      - flash-max-microamp
> +      - flash-max-timeout-us
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#address-cells'
> +  - '#size-cells'

Why?

> +  - led
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/leds/common.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      led-controller@33 {
> +        compatible = "ti,tps61310";
> +        reg = <0x33>;
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        reset-gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;
> +
> +        tps61310_flash: led {

Drop unused label

Best regards,
Krzysztof

Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Matthias Fend]

Hi Krzysztof,

Am 10.03.2025 um 08:46 schrieb Krzysztof Kozlowski:
> On 28/02/2025 11:31, Matthias Fend wrote:
>> +	tps6131x->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
>> +	ret = tps6131x_reset_chip(tps6131x);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret, "Failed to reset LED controller\n");
>> +
>> +	ret = tps6131x_init_chip(tps6131x);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret, "Failed to initialize LED controller\n");
>> +
>> +	ret = tps6131x_led_class_setup(tps6131x);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret, "Failed to setup led class\n");
>> +
>> +	ret = tps6131x_v4l2_setup(tps6131x);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret, "Failed to setup v4l2 flash\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static void tps6131x_remove(struct i2c_client *client)
>> +{
>> +	struct tps6131x *tps6131x = i2c_get_clientdata(client);
>> +
>> +	v4l2_flash_release(tps6131x->v4l2_flash);
>> +
>> +	cancel_delayed_work_sync(&tps6131x->torch_refresh_work);
>> +}
>> +
>> +static const struct of_device_id of_tps6131x_leds_match[] = {
>> +	{ .compatible = "ti,tps61310" },
>> +	{ .compatible = "ti,tps61311" },
> 
> 
> No differences? So devices are fully compatible? Then it should be
> expressed in the binding with fallback. Or the binding description or
> commit msg should explain why they are not compatible.

Yes, from a software perspective both are identical.
The only difference I found between the two variants are different 
valley current limits. These are described in the bindings.

Best regards
  ~Matthias

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: add Texas Instruments TPS6131x flash LED driver

[Matthias Fend]

Hi Krzysztof,

thanks for your review.

Am 10.03.2025 um 08:49 schrieb Krzysztof Kozlowski:
> On 28/02/2025 11:31, Matthias Fend wrote:
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - ti,tps61310
>> +      - ti,tps61311
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
> 
> Why do you need these two?

As a template, I also used recently added bindings 
(silergy,sy7802.yaml). These entries come from there, but as I 
understand it, they are supposed to be removed, right?

> 
>> +
>> +  reset-gpios:
>> +    maxItems: 1
>> +    description: GPIO connected to NRESET pin
>> +
>> +  ti,valley-current-limit:
>> +    type: boolean
>> +    description:
>> +      Reduce the valley peak current limit from 1750mA to 1250mA (TPS61310) or
>> +      from 2480mA to 1800mA (TPS61311).
>> +
>> +  led:
> 
> Why do you have only one led node? Description says three: LED1-3,
> unless these are just sinks which always have to be connected to the
> same LED?

That is basically correct. If you just want to switch the 3 LEDs on or 
off, you could map that accordingly.
In detail, however, the 3 channels are not really independent of each 
other. All channels share, for example, the flash controller, the safety 
timers and the operating mode. In addition, two channels share the 
configuration registers. It is therefore not possible to use one channel 
as a flash and another as a normal LED.
For use as an LED flash controller (what the chip actually is), it 
therefore only makes sense if one or more channels are combined.

> 
>> +    type: object
>> +    $ref: common.yaml#
>> +    unevaluatedProperties: false
>> +
>> +    properties:
>> +      led-sources:
>> +        allOf:
> 
> Drop allOf
> 
>> +          - minItems: 1
>> +            maxItems: 3
>> +            items:
>> +              enum: [1, 2, 3]
>> +
>> +      led-max-microamp:
>> +        anyOf:
> 
> oneOf
> 
>> +          - minimum: 25000
>> +            maximum: 350000
>> +            multipleOf: 50000
>> +          - minimum: 25000
>> +            maximum: 525000
> 
> Why two different values?

The channels can in principle be configured in 25mA steps.
If only the two channels that share the configuration register are used, 
the step size doubles to 50mA. The maximum current values ​​for such a 
configuration are also different.
There are of course several other combinations of channels that would 
result in different maximum values, but since the step size is an 
important value for the API, I wanted to describe these two cases 
explicitly.

Best regards
  ~Matthias

> 
>> +            multipleOf: 25000
>> +
>> +      flash-max-microamp:
>> +        anyOf:
> 
> oneOf
> 
>> +          - minimum: 25000
>> +            maximum: 800000
>> +            multipleOf: 50000
>> +          - minimum: 25000
> 
> Same question
> 
>> +            maximum: 1500000
>> +            multipleOf: 25000
>> +
>> +      flash-max-timeout-us:
>> +        enum: [ 5300, 10700, 16000, 21300, 26600, 32000, 37300, 68200, 71500,
>> +                102200, 136300, 170400, 204500, 340800, 579300, 852000 ]
>> +
>> +    required:
>> +      - led-sources
>> +      - led-max-microamp
>> +      - flash-max-microamp
>> +      - flash-max-timeout-us
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#address-cells'
>> +  - '#size-cells'
> 
> Why?
> 
>> +  - led
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/leds/common.h>
>> +    #include <dt-bindings/gpio/gpio.h>
>> +
>> +    i2c {
>> +      #address-cells = <1>;
>> +      #size-cells = <0>;
>> +
>> +      led-controller@33 {
>> +        compatible = "ti,tps61310";
>> +        reg = <0x33>;
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        reset-gpios = <&gpio1 0 GPIO_ACTIVE_LOW>;
>> +
>> +        tps61310_flash: led {
> 
> Drop unused label
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: leds: add Texas Instruments TPS6131x flash LED driver

[Krzysztof Kozlowski]

On 10/03/2025 09:40, Matthias Fend wrote:
> Hi Krzysztof,
> 
> thanks for your review.
> 
> Am 10.03.2025 um 08:49 schrieb Krzysztof Kozlowski:
>> On 28/02/2025 11:31, Matthias Fend wrote:
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - ti,tps61310
>>> +      - ti,tps61311
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  '#address-cells':
>>> +    const: 1
>>> +
>>> +  '#size-cells':
>>> +    const: 0
>>
>> Why do you need these two?
> 
> As a template, I also used recently added bindings 
> (silergy,sy7802.yaml). These entries come from there, but as I 
> understand it, they are supposed to be removed, right?

They make no sense based on the bindings.

> 
>>
>>> +
>>> +  reset-gpios:
>>> +    maxItems: 1
>>> +    description: GPIO connected to NRESET pin
>>> +
>>> +  ti,valley-current-limit:
>>> +    type: boolean
>>> +    description:
>>> +      Reduce the valley peak current limit from 1750mA to 1250mA (TPS61310) or
>>> +      from 2480mA to 1800mA (TPS61311).
>>> +
>>> +  led:
>>
>> Why do you have only one led node? Description says three: LED1-3,
>> unless these are just sinks which always have to be connected to the
>> same LED?
> 
> That is basically correct. If you just want to switch the 3 LEDs on or 
> off, you could map that accordingly.
> In detail, however, the 3 channels are not really independent of each 
> other. All channels share, for example, the flash controller, the safety 
> timers and the operating mode. In addition, two channels share the 
> configuration registers. It is therefore not possible to use one channel 
> as a flash and another as a normal LED.
> For use as an LED flash controller (what the chip actually is), it 
> therefore only makes sense if one or more channels are combined.

You define the binding which will be set in stone. This binding says it
is not possible to use two or three LEDs. I am fine with this, but be
aware of it and explain this in binding description.

> 
>>
>>> +    type: object
>>> +    $ref: common.yaml#
>>> +    unevaluatedProperties: false
>>> +
>>> +    properties:
>>> +      led-sources:
>>> +        allOf:
>>
>> Drop allOf
>>
>>> +          - minItems: 1
>>> +            maxItems: 3
>>> +            items:
>>> +              enum: [1, 2, 3]
>>> +
>>> +      led-max-microamp:
>>> +        anyOf:
>>
>> oneOf
>>
>>> +          - minimum: 25000
>>> +            maximum: 350000
>>> +            multipleOf: 50000
>>> +          - minimum: 25000
>>> +            maximum: 525000
>>
>> Why two different values?
> 
> The channels can in principle be configured in 25mA steps.
> If only the two channels that share the configuration register are used, 
> the step size doubles to 50mA. The maximum current values ​​for such a 
> configuration are also different.
> There are of course several other combinations of channels that would 
> result in different maximum values, but since the step size is an 
> important value for the API, I wanted to describe these two cases 
> explicitly.
> 
ok

Best regards,
Krzysztof

Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Krzysztof Kozlowski]

On 10/03/2025 09:04, Matthias Fend wrote:
>>> +
>>> +static void tps6131x_remove(struct i2c_client *client)
>>> +{
>>> +	struct tps6131x *tps6131x = i2c_get_clientdata(client);
>>> +
>>> +	v4l2_flash_release(tps6131x->v4l2_flash);
>>> +
>>> +	cancel_delayed_work_sync(&tps6131x->torch_refresh_work);
>>> +}
>>> +
>>> +static const struct of_device_id of_tps6131x_leds_match[] = {
>>> +	{ .compatible = "ti,tps61310" },
>>> +	{ .compatible = "ti,tps61311" },
>>
>>
>> No differences? So devices are fully compatible? Then it should be
>> expressed in the binding with fallback. Or the binding description or
>> commit msg should explain why they are not compatible.
> 
> Yes, from a software perspective both are identical.
> The only difference I found between the two variants are different 
> valley current limits. These are described in the bindings.

So use compatibility and fallbacks (see example-schema or hundreds of
other bindings).

Best regards,
Krzysztof

Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Lee Jones]

On Fri, 28 Feb 2025, Matthias Fend wrote:

> The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
> stage is capable of supplying a maximum total current of roughly 1500mA.
> The TPS6131x provides three constant-current sinks, capable of sinking up
> to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
> each sink (LED1, LED2, LED3) supports currents up to 175mA.
> 
> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> ---
>  MAINTAINERS                        |   7 +
>  drivers/leds/flash/Kconfig         |  11 +
>  drivers/leds/flash/Makefile        |   1 +
>  drivers/leds/flash/leds-tps6131x.c | 798 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 817 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0fa7c5728f1e64d031f4a47b6fce1db484ce0fc2..d2ca840647b566cfee4d8ded1f787e32be4aa163 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23225,6 +23225,13 @@ F:	Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
>  F:	Documentation/hwmon/tps23861.rst
>  F:	drivers/hwmon/tps23861.c
>  
> +TEXAS INSTRUMENTS TPS6131X FLASH LED DRIVER
> +M:	Matthias Fend <matthias.fend@emfend.at>
> +L:	linux-leds@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/leds/ti,tps6131x.yaml
> +F:	drivers/leds/flash/leds-tps6131x.c
> +
>  TEXAS INSTRUMENTS' DAC7612 DAC DRIVER
>  M:	Ricardo Ribalda <ribalda@kernel.org>
>  L:	linux-iio@vger.kernel.org
> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
> index f39f0bfe6eefcd376405d9d35dc36e323a485002..55ca663ca506ad8be627f58f6d6308368ea2b928 100644
> --- a/drivers/leds/flash/Kconfig
> +++ b/drivers/leds/flash/Kconfig
> @@ -132,4 +132,15 @@ config LEDS_SY7802
>  
>  	  This driver can be built as a module, it will be called "leds-sy7802".
>  
> +config LEDS_TPS6131X
> +	tristate "LED support for TI TPS6131x flash LED driver"
> +	depends on I2C && OF
> +	depends on GPIOLIB
> +	select REGMAP_I2C
> +	help
> +	  This option enables support for Texas Instruments TPS61310/TPS61311
> +	  flash LED driver.
> +
> +	  This driver can be built as a module, it will be called "leds-tps6131x".
> +
>  endif # LEDS_CLASS_FLASH
> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
> index 48860eeced79513a0ed303e4af3db9bfe9724b7e..712fb737a428e42747e1aa339058dc4306ade9c8 100644
> --- a/drivers/leds/flash/Makefile
> +++ b/drivers/leds/flash/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_LEDS_RT4505)	+= leds-rt4505.o
>  obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
>  obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
>  obj-$(CONFIG_LEDS_SY7802)	+= leds-sy7802.o
> +obj-$(CONFIG_LEDS_TPS6131X)	+= leds-tps6131x.o
> diff --git a/drivers/leds/flash/leds-tps6131x.c b/drivers/leds/flash/leds-tps6131x.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..5fca542ac88da1db755a75e982dee8e3dde06373
> --- /dev/null
> +++ b/drivers/leds/flash/leds-tps6131x.c
> @@ -0,0 +1,798 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Texas Instruments TPS61310/TPS61311 flash LED driver with I2C interface
> + *
> + * Copyright 2025 Matthias Fend <matthias.fend@emfend.at>
> + */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/led-class-flash.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <media/v4l2-flash-led-class.h>
> +
> +/* Registers */
> +
> +#define TPS6131X_REG_0				0x00
> +#define TPS6131X_REG_0_RESET			BIT(7)
> +#define TPS6131X_REG_0_DCLC13			GENMASK(5, 3)
> +#define TPS6131X_REG_0_DCLC13_SHIFT		3
> +#define TPS6131X_REG_0_DCLC2			GENMASK(2, 0)
> +#define TPS6131X_REG_0_DCLC2_SHIFT		0
> +
> +#define TPS6131X_REG_1				0x01
> +#define TPS6131X_REG_1_MODE			GENMASK(7, 6)
> +#define TPS6131X_REG_1_MODE_SHIFT		6
> +#define TPS6131X_REG_1_FC2			GENMASK(5, 0)
> +#define TPS6131X_REG_1_FC2_SHIFT		0
> +
> +#define TPS6131X_REG_2				0x02
> +#define TPS6131X_REG_2_MODE			GENMASK(7, 6)
> +#define TPS6131X_REG_2_MODE_SHIFT		6
> +#define TPS6131X_REG_2_ENVM			BIT(5)
> +#define TPS6131X_REG_2_FC13			GENMASK(4, 0)
> +#define TPS6131X_REG_2_FC13_SHIFT		0
> +
> +#define TPS6131X_REG_3				0x03
> +#define TPS6131X_REG_3_STIM			GENMASK(7, 5)
> +#define TPS6131X_REG_3_STIM_SHIFT		5
> +#define TPS6131X_REG_3_HPFL			BIT(4)
> +#define TPS6131X_REG_3_SELSTIM_TO		BIT(3)
> +#define TPS6131X_REG_3_STT			BIT(2)
> +#define TPS6131X_REG_3_SFT			BIT(1)
> +#define TPS6131X_REG_3_TXMASK			BIT(0)
> +
> +#define TPS6131X_REG_4				0x04
> +#define TPS6131X_REG_4_PG			BIT(7)
> +#define TPS6131X_REG_4_HOTDIE_HI		BIT(6)
> +#define TPS6131X_REG_4_HOTDIE_LO		BIT(5)
> +#define TPS6131X_REG_4_ILIM			BIT(4)
> +#define TPS6131X_REG_4_INDC			GENMASK(3, 0)
> +#define TPS6131X_REG_4_SHIFT			0
> +
> +#define TPS6131X_REG_5				0x05
> +#define TPS6131X_REG_5_SELFCAL			BIT(7)
> +#define TPS6131X_REG_5_ENPSM			BIT(6)
> +#define TPS6131X_REG_5_STSTRB1_DIR		BIT(5)
> +#define TPS6131X_REG_5_GPIO			BIT(4)
> +#define TPS6131X_REG_5_GPIOTYPE			BIT(3)
> +#define TPS6131X_REG_5_ENLED3			BIT(2)
> +#define TPS6131X_REG_5_ENLED2			BIT(1)
> +#define TPS6131X_REG_5_ENLED1			BIT(0)
> +
> +#define TPS6131X_REG_6				0x06
> +#define TPS6131X_REG_6_ENTS			BIT(7)
> +#define TPS6131X_REG_6_LEDHOT			BIT(6)
> +#define TPS6131X_REG_6_LEDWARN			BIT(5)
> +#define TPS6131X_REG_6_LEDHDR			BIT(4)
> +#define TPS6131X_REG_6_OV			GENMASK(3, 0)
> +#define TPS6131X_REG_6_OV_SHIFT			0
> +
> +#define TPS6131X_REG_7				0x07
> +#define TPS6131X_REG_7_ENBATMON			BIT(7)
> +#define TPS6131X_REG_7_BATDROOP			GENMASK(6, 4)
> +#define TPS6131X_REG_7_BATDROOP_SHIFT		4
> +#define TPS6131X_REG_7_REVID			GENMASK(2, 0)
> +#define TPS6131X_REG_7_REVID_SHIFT		0
> +
> +/* Constants */
> +
> +#define TPS6131X_MAX_CHANNELS			3
> +
> +#define TPS6131X_FLASH_MAX_I_CHAN13_MA		400
> +#define TPS6131X_FLASH_MAX_I_CHAN2_MA		800
> +#define TPS6131X_FLASH_STEP_I_MA		25
> +
> +#define TPS6131X_TORCH_MAX_I_CHAN13_MA		175
> +#define TPS6131X_TORCH_MAX_I_CHAN2_MA		175
> +#define TPS6131X_TORCH_STEP_I_MA		25
> +
> +#define TPS6131X_TORCH_REFRESH_INTERVAL_JIFFIES msecs_to_jiffies(10000)
> +
> +struct tps6131x {
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +	struct gpio_desc *reset_gpio;
> +	struct mutex lock; /* */

?

> +	struct delayed_work torch_refresh_work;
> +	bool valley_current_limit;
> +	bool chan1_en;
> +	bool chan2_en;
> +	bool chan3_en;
> +	struct fwnode_handle *led_node;
> +	u32 max_flash_current_ma;
> +	u32 step_flash_current_ma;
> +	u32 max_torch_current_ma;
> +	u32 step_torch_current_ma;
> +	u32 max_timeout_us;
> +	struct led_classdev_flash fled_cdev;
> +	struct v4l2_flash *v4l2_flash;
> +};
> +
> +static struct tps6131x *fled_cdev_to_tps6131x(struct led_classdev_flash *fled_cdev)
> +{
> +	return container_of(fled_cdev, struct tps6131x, fled_cdev);
> +}
> +
> +static const struct reg_default tps6131x_regmap_defaults[] = {
> +	{ TPS6131X_REG_0, 0x0A },
> +	{ TPS6131X_REG_1, 0x10 },
> +	{ TPS6131X_REG_2, 0x08 },
> +	{ TPS6131X_REG_3, 0xC1 },
> +	{ TPS6131X_REG_4, 0x00 },
> +	{ TPS6131X_REG_5, 0x6A },
> +	{ TPS6131X_REG_6, 0x00 },
> +	{ TPS6131X_REG_7, 0x46 },

I'm not a big fan of blind register patching.

These would be better either defined (preferred) or commented.

> +};
> +
> +/*
> + * These registers contain flags that are reset when read. Ensure that these registers are not read
> + * outside of a call from the driver.
> + */
> +static bool tps6131x_regmap_precious(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case TPS6131X_REG_3:
> +	case TPS6131X_REG_4:
> +	case TPS6131X_REG_6:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config tps6131x_regmap = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = TPS6131X_REG_7,
> +	.reg_defaults = tps6131x_regmap_defaults,
> +	.num_reg_defaults = ARRAY_SIZE(tps6131x_regmap_defaults),
> +	.cache_type = REGCACHE_FLAT,
> +	.precious_reg = &tps6131x_regmap_precious,
> +};
> +
> +struct tps6131x_timer_config {
> +	u8 val;
> +	u8 range;
> +	u32 time_us;
> +};
> +
> +static const struct tps6131x_timer_config tps6131x_timer_configs[] = {
> +	{ .val = 0, .range = 1, .time_us = 5300 },
> +	{ .val = 1, .range = 1, .time_us = 10700 },
> +	{ .val = 2, .range = 1, .time_us = 16000 },
> +	{ .val = 3, .range = 1, .time_us = 21300 },
> +	{ .val = 4, .range = 1, .time_us = 26600 },
> +	{ .val = 5, .range = 1, .time_us = 32000 },
> +	{ .val = 6, .range = 1, .time_us = 37300 },
> +	{ .val = 0, .range = 0, .time_us = 68200 },
> +	{ .val = 7, .range = 1, .time_us = 71500 },
> +	{ .val = 1, .range = 0, .time_us = 102200 },
> +	{ .val = 2, .range = 0, .time_us = 136300 },
> +	{ .val = 3, .range = 0, .time_us = 170400 },
> +	{ .val = 4, .range = 0, .time_us = 204500 },
> +	{ .val = 5, .range = 0, .time_us = 340800 },
> +	{ .val = 6, .range = 0, .time_us = 579300 },
> +	{ .val = 7, .range = 0, .time_us = 852000 },
> +};
> +
> +static const struct tps6131x_timer_config *tps6131x_find_closest_timer_config(u32 timeout_us)
> +{
> +	const struct tps6131x_timer_config *timer_config = &tps6131x_timer_configs[0];
> +	u32 diff, min_diff = U32_MAX;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tps6131x_timer_configs); i++) {
> +		diff = abs(tps6131x_timer_configs[i].time_us - timeout_us);
> +		if (diff < min_diff) {
> +			timer_config = &tps6131x_timer_configs[i];
> +			min_diff = diff;
> +			if (!min_diff)
> +				break;
> +		}
> +	}
> +
> +	return timer_config;
> +}
> +
> +static int tps6131x_reset_chip(struct tps6131x *tps6131x)
> +{
> +	int ret;
> +
> +	if (IS_ERR_OR_NULL(tps6131x->reset_gpio)) {
> +		ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_0, TPS6131X_REG_0_RESET,
> +					 TPS6131X_REG_0_RESET);
> +		if (ret)
> +			return ret;

No delay required for this path?

> +		ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_0, TPS6131X_REG_0_RESET, 0);
> +		if (ret)
> +			return ret;
> +	} else {
> +		gpiod_set_value_cansleep(tps6131x->reset_gpio, 1);
> +		fsleep(10);
> +		gpiod_set_value_cansleep(tps6131x->reset_gpio, 0);
> +	}
> +
> +	return 0;
> +}
> +
> +static int tps6131x_init_chip(struct tps6131x *tps6131x)
> +{
> +	u32 reg4, reg5, reg6;
> +	int ret;
> +
> +	reg4 = tps6131x->valley_current_limit ? TPS6131X_REG_4_ILIM : 0;
> +	ret = regmap_write(tps6131x->regmap, TPS6131X_REG_4, reg4);
> +	if (ret)
> +		return ret;
> +
> +	reg5 = TPS6131X_REG_5_ENPSM | TPS6131X_REG_5_STSTRB1_DIR | TPS6131X_REG_5_GPIOTYPE;
> +	if (tps6131x->chan1_en)
> +		reg5 |= TPS6131X_REG_5_ENLED1;
> +
> +	if (tps6131x->chan2_en)
> +		reg5 |= TPS6131X_REG_5_ENLED2;
> +
> +	if (tps6131x->chan3_en)
> +		reg5 |= TPS6131X_REG_5_ENLED3;
> +	ret = regmap_write(tps6131x->regmap, TPS6131X_REG_5, reg5);
> +	if (ret)
> +		return ret;
> +
> +	reg6 = TPS6131X_REG_6_ENTS;
> +	ret = regmap_write(tps6131x->regmap, TPS6131X_REG_6, reg6);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +enum tps6131x_mode {
> +	TPS6131X_MODE_SHUTDOWN,
> +	TPS6131X_MODE_TORCH,
> +	TPS6131X_MODE_FLASH,
> +};
> +
> +static int tps6131x_set_mode(struct tps6131x *tps6131x, enum tps6131x_mode mode, bool force)
> +{
> +	u8 val;
> +
> +	switch (mode) {
> +	case TPS6131X_MODE_FLASH:
> +		val = 2 << TPS6131X_REG_1_MODE_SHIFT;
> +		break;
> +	case TPS6131X_MODE_TORCH:
> +		val = 1 << TPS6131X_REG_1_MODE_SHIFT;
> +		break;
> +	case TPS6131X_MODE_SHUTDOWN:
> +	default:
> +		val = 0 << TPS6131X_REG_1_MODE_SHIFT;
> +		break;
> +	}

What about:

	val = mode << TPS6131X_REG_1_MODE_SHIFT;

> +
> +	return regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_1, TPS6131X_REG_1_MODE, val,
> +				       NULL, false, force);
> +}
> +
> +static void tps6131x_torch_refresh_handler(struct work_struct *work)
> +{
> +	struct tps6131x *tps6131x = container_of(work, struct tps6131x, torch_refresh_work.work);
> +
> +	guard(mutex)(&tps6131x->lock);
> +
> +	tps6131x_set_mode(tps6131x, TPS6131X_MODE_TORCH, true);
> +
> +	schedule_delayed_work(&tps6131x->torch_refresh_work,
> +			      TPS6131X_TORCH_REFRESH_INTERVAL_JIFFIES);
> +}
> +
> +static int tps6131x_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
> +{
> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> +	int ret;
> +	u8 reg0;
> +	u32 steps_remaining, steps_chan13, steps_chan2;
> +	unsigned int num_chans = tps6131x->chan1_en + tps6131x->chan2_en + tps6131x->chan3_en;

It would quell my OCD if you reversed these 4 lines.

> +	cancel_delayed_work_sync(&tps6131x->torch_refresh_work);
> +
> +	guard(mutex)(&tps6131x->lock);
> +
> +	/*
> +	 * The brightness parameter uses the number of current steps as the unit (not the current
> +	 * value itself). Since the reported step size can vary depending on the configuration,
> +	 * this value must be converted into actual register steps.
> +	 */
> +	steps_remaining = (brightness * tps6131x->step_torch_current_ma) / TPS6131X_TORCH_STEP_I_MA;
> +
> +	/*
> +	 * The currents are distributed as evenly as possible across the activated channels.
> +	 * Since channels 1 and 3 share the same register setting, they always use the same current
> +	 * value. Channel 2 supports higher currents and thus takes over the remaining additional
> +	 * portion that cannot be covered by the other channels.
> +	 */
> +	steps_chan13 = min_t(u32, steps_remaining / num_chans,
> +			     TPS6131X_TORCH_MAX_I_CHAN13_MA / TPS6131X_TORCH_STEP_I_MA);
> +	if (tps6131x->chan1_en)
> +		steps_remaining -= steps_chan13;
> +	if (tps6131x->chan3_en)
> +		steps_remaining -= steps_chan13;
> +
> +	steps_chan2 = min_t(u32, steps_remaining,
> +			    TPS6131X_TORCH_MAX_I_CHAN2_MA / TPS6131X_TORCH_STEP_I_MA);
> +
> +	reg0 = (steps_chan13 << TPS6131X_REG_0_DCLC13_SHIFT) |
> +	       (steps_chan2 << TPS6131X_REG_0_DCLC2_SHIFT);
> +	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_0,
> +				 TPS6131X_REG_0_DCLC13 | TPS6131X_REG_0_DCLC2, reg0);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = tps6131x_set_mode(tps6131x, brightness ? TPS6131X_MODE_TORCH : TPS6131X_MODE_SHUTDOWN,
> +				true);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * In order to use both the flash and the video light functions purely via the I2C
> +	 * interface, STRB1 must be low. If STRB1 is low, then the video light watchdog timer
> +	 * is also active, which puts the device into the shutdown state after around 13 seconds.
> +	 * To prevent this, the mode must be refreshed within the watchdog timeout.
> +	 */
> +	if (brightness)
> +		schedule_delayed_work(&tps6131x->torch_refresh_work,
> +				      TPS6131X_TORCH_REFRESH_INTERVAL_JIFFIES);
> +
> +	return 0;
> +}
> +
> +static int tps6131x_strobe_set(struct led_classdev_flash *fled_cdev, bool state)
> +{
> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> +	int ret;
> +
> +	guard(mutex)(&tps6131x->lock);
> +
> +	ret = tps6131x_set_mode(tps6131x, state ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
> +				true);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (state) {
> +		ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_3, TPS6131X_REG_3_SFT,
> +					      TPS6131X_REG_3_SFT, NULL, false, true);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_3, TPS6131X_REG_3_SFT, 0, NULL,
> +				      false, true);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int tps6131x_flash_brightness_set(struct led_classdev_flash *fled_cdev, u32 brightness)
> +{
> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> +	u32 steps_remaining, steps_chan13, steps_chan2, num_chans;
> +	int ret;
> +
> +	guard(mutex)(&tps6131x->lock);
> +
> +	steps_remaining = brightness / TPS6131X_FLASH_STEP_I_MA;
> +	num_chans = tps6131x->chan1_en + tps6131x->chan2_en + tps6131x->chan3_en;
> +
> +	steps_chan13 = min_t(u32, steps_remaining / num_chans,
> +			     TPS6131X_FLASH_MAX_I_CHAN13_MA / TPS6131X_FLASH_STEP_I_MA);
> +	if (tps6131x->chan1_en)
> +		steps_remaining -= steps_chan13;
> +	if (tps6131x->chan3_en)
> +		steps_remaining -= steps_chan13;
> +	steps_chan2 = min_t(u32, steps_remaining,
> +			    TPS6131X_FLASH_MAX_I_CHAN2_MA / TPS6131X_FLASH_STEP_I_MA);
> +
> +	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_2, TPS6131X_REG_2_FC13,
> +				 steps_chan13 << TPS6131X_REG_2_FC13_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_1, TPS6131X_REG_1_FC2,
> +				 steps_chan2 << TPS6131X_REG_1_FC2_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	fled_cdev->brightness.val = brightness;
> +
> +	return 0;
> +}
> +
> +static int tps6131x_flash_timeout_set(struct led_classdev_flash *fled_cdev, u32 timeout_us)
> +{
> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> +	int ret;
> +	u8 reg3;
> +	const struct tps6131x_timer_config *timer_config;
> +
> +	guard(mutex)(&tps6131x->lock);
> +
> +	timer_config = tps6131x_find_closest_timer_config(timeout_us);
> +
> +	reg3 = timer_config->val << TPS6131X_REG_3_STIM_SHIFT;
> +	if (timer_config->range)
> +		reg3 |= TPS6131X_REG_3_SELSTIM_TO;
> +
> +	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_3,
> +				 TPS6131X_REG_3_STIM | TPS6131X_REG_3_SELSTIM_TO, reg3);
> +	if (ret < 0)
> +		return ret;
> +
> +	fled_cdev->timeout.val = timer_config->time_us;
> +
> +	return 0;
> +}
> +
> +static int tps6131x_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
> +{
> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> +	unsigned int reg3;
> +	int ret;
> +
> +	guard(mutex)(&tps6131x->lock);
> +
> +	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_3, &reg3);
> +	if (ret)
> +		return ret;
> +
> +	*state = !!(reg3 & TPS6131X_REG_3_SFT);
> +
> +	return 0;
> +}
> +
> +static int tps6131x_flash_fault_get(struct led_classdev_flash *fled_cdev, u32 *fault)
> +{
> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> +	unsigned int reg3, reg4, reg6;
> +	int ret;
> +
> +	*fault = 0;
> +
> +	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_3, &reg3);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_4, &reg4);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_6, &reg6);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (reg3 & TPS6131X_REG_3_HPFL)
> +		*fault |= LED_FAULT_SHORT_CIRCUIT;
> +
> +	if (reg3 & TPS6131X_REG_3_SELSTIM_TO)
> +		*fault |= LED_FAULT_TIMEOUT;
> +
> +	if (reg4 & TPS6131X_REG_4_HOTDIE_HI)
> +		*fault |= LED_FAULT_OVER_TEMPERATURE;
> +
> +	if (reg6 & (TPS6131X_REG_6_LEDHOT | TPS6131X_REG_6_LEDWARN))
> +		*fault |= LED_FAULT_LED_OVER_TEMPERATURE;
> +
> +	if (!(reg6 & TPS6131X_REG_6_LEDHDR))
> +		*fault |= LED_FAULT_UNDER_VOLTAGE;
> +
> +	if (reg6 & TPS6131X_REG_6_LEDHOT) {
> +		ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_6,
> +					      TPS6131X_REG_6_LEDHOT, 0, NULL, false, true);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct led_flash_ops flash_ops = {
> +	.flash_brightness_set = tps6131x_flash_brightness_set,
> +	.strobe_set = tps6131x_strobe_set,
> +	.strobe_get = tps6131x_strobe_get,
> +	.timeout_set = tps6131x_flash_timeout_set,
> +	.fault_get = tps6131x_flash_fault_get,
> +};
> +
> +static int tps6131x_parse_node(struct tps6131x *tps6131x)
> +{
> +	struct device *dev = &tps6131x->client->dev;
> +	int ret;
> +	u32 current_ua, timeout_us;
> +	const struct tps6131x_timer_config *timer_config;
> +	u32 flash_max_i_ma, torch_max_i_ma;
> +	u32 current_step_multiplier;
> +	u32 channels[TPS6131X_MAX_CHANNELS];
> +	unsigned int num_channels;
> +	int i;

There doesn't appear to be any ordering of these variables at all

*twitch*

Please employ some kind of structure - I don't mind which.

> +
> +	tps6131x->valley_current_limit = device_property_read_bool(dev, "ti,valley-current-limit");
> +
> +	tps6131x->led_node = fwnode_get_next_available_child_node(dev->fwnode, NULL);
> +	if (!tps6131x->led_node) {
> +		dev_err(dev, "Missing LED node\n");
> +		return -EINVAL;
> +	}
> +
> +	num_channels = fwnode_property_count_u32(tps6131x->led_node, "led-sources");
> +	if (num_channels <= 0) {
> +		dev_err(dev, "Failed to read led-sources property\n");
> +		return -EINVAL;
> +	}
> +
> +	if (num_channels > TPS6131X_MAX_CHANNELS) {
> +		dev_err(dev, "led-sources count %u exceeds maximum channel count %u\n",
> +			num_channels, TPS6131X_MAX_CHANNELS);
> +		return -EINVAL;
> +	}
> +
> +	ret = fwnode_property_read_u32_array(tps6131x->led_node, "led-sources", channels,
> +					     num_channels);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read led-sources property\n");
> +		return ret;
> +	}
> +
> +	flash_max_i_ma = 0;
> +	torch_max_i_ma = 0;
> +	for (i = 0; i < num_channels; i++) {
> +		switch (channels[i]) {
> +		case 1:
> +			tps6131x->chan1_en = true;
> +			flash_max_i_ma += TPS6131X_FLASH_MAX_I_CHAN13_MA;
> +			torch_max_i_ma += TPS6131X_TORCH_MAX_I_CHAN13_MA;
> +			break;
> +		case 2:
> +			tps6131x->chan2_en = true;
> +			flash_max_i_ma += TPS6131X_FLASH_MAX_I_CHAN2_MA;
> +			torch_max_i_ma += TPS6131X_TORCH_MAX_I_CHAN2_MA;
> +			break;
> +		case 3:
> +			tps6131x->chan3_en = true;
> +			flash_max_i_ma += TPS6131X_FLASH_MAX_I_CHAN13_MA;
> +			torch_max_i_ma += TPS6131X_TORCH_MAX_I_CHAN13_MA;
> +			break;
> +		default:
> +			dev_err(dev, "led-source out of range [1-3]\n");
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/*
> +	 * If only channels 1 and 3 are used, the step size is doubled because the two channels
> +	 * share the same current control register.
> +	 */
> +	current_step_multiplier =
> +		(tps6131x->chan1_en && tps6131x->chan3_en && !tps6131x->chan2_en) ? 2 : 1;
> +	tps6131x->step_flash_current_ma = current_step_multiplier * TPS6131X_FLASH_STEP_I_MA;
> +	tps6131x->step_torch_current_ma = current_step_multiplier * TPS6131X_TORCH_STEP_I_MA;
> +
> +	ret = fwnode_property_read_u32(tps6131x->led_node, "led-max-microamp", &current_ua);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read led-max-microamp property\n");
> +		return ret;
> +	}
> +
> +	tps6131x->max_torch_current_ma = current_ua / 1000;

There should be a MACRO for these types of conversions.

> +
> +	if (!tps6131x->max_torch_current_ma || tps6131x->max_torch_current_ma > torch_max_i_ma ||
> +	    (tps6131x->max_torch_current_ma % tps6131x->step_torch_current_ma)) {
> +		dev_err(dev, "led-max-microamp out of range or not a multiple of %u\n",
> +			tps6131x->step_torch_current_ma);
> +		return -EINVAL;
> +	}
> +
> +	ret = fwnode_property_read_u32(tps6131x->led_node, "flash-max-microamp", &current_ua);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read flash-max-microamp property\n");
> +		return ret;
> +	}
> +
> +	tps6131x->max_flash_current_ma = current_ua / 1000;
> +
> +	if (!tps6131x->max_flash_current_ma || tps6131x->max_flash_current_ma > flash_max_i_ma ||
> +	    (tps6131x->max_flash_current_ma % tps6131x->step_flash_current_ma)) {
> +		dev_err(dev, "flash-max-microamp out of range or not a multiple of %u\n",
> +			tps6131x->step_flash_current_ma);
> +		return -EINVAL;
> +	}
> +
> +	ret = fwnode_property_read_u32(tps6131x->led_node, "flash-max-timeout-us", &timeout_us);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to read flash-max-timeout-us property\n");
> +		return ret;
> +	}
> +
> +	timer_config = tps6131x_find_closest_timer_config(timeout_us);
> +	tps6131x->max_timeout_us = timer_config->time_us;
> +
> +	if (tps6131x->max_timeout_us != timeout_us)
> +		dev_warn(dev, "flash-max-timeout-us %u not supported (using %u)\n", timeout_us,
> +			 tps6131x->max_timeout_us);
> +
> +	return 0;
> +}
> +
> +static int tps6131x_led_class_setup(struct tps6131x *tps6131x)
> +{
> +	struct led_classdev *led_cdev;
> +	struct led_flash_setting *setting;
> +	struct led_init_data init_data = {};
> +	static const struct tps6131x_timer_config *timer_config;
> +	int ret;
> +
> +	tps6131x->fled_cdev.ops = &flash_ops;
> +
> +	setting = &tps6131x->fled_cdev.timeout;
> +	timer_config = tps6131x_find_closest_timer_config(0);
> +	setting->min = timer_config->time_us;
> +	setting->max = tps6131x->max_timeout_us;
> +	setting->step = 1; /* Only some specific time periods are supported. No fixed step size. */
> +	setting->val = setting->min;
> +
> +	setting = &tps6131x->fled_cdev.brightness;
> +	setting->min = tps6131x->step_flash_current_ma;
> +	setting->max = tps6131x->max_flash_current_ma;
> +	setting->step = tps6131x->step_flash_current_ma;
> +	setting->val = setting->min;
> +
> +	led_cdev = &tps6131x->fled_cdev.led_cdev;
> +	led_cdev->brightness_set_blocking = tps6131x_brightness_set;
> +	led_cdev->max_brightness = tps6131x->max_torch_current_ma;
> +	led_cdev->flags |= LED_DEV_CAP_FLASH;
> +
> +	init_data.fwnode = tps6131x->led_node;
> +	init_data.devicename = NULL;
> +	init_data.default_label = NULL;
> +	init_data.devname_mandatory = false;
> +
> +	ret = devm_led_classdev_flash_register_ext(&tps6131x->client->dev, &tps6131x->fled_cdev,
> +						   &init_data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)

Not keen on #ifery in C files.

Can you use is_defined() and return early instead?

I see that there is a precedent for this already. :(

> +static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
> +{
> +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> +
> +	guard(mutex)(&tps6131x->lock);
> +
/> +	return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
> +				 false);
> +}
> +
> +static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
> +	.external_strobe_set = tps6131x_flash_external_strobe_set,
> +};
> +
> +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
> +{
> +	struct v4l2_flash_config v4l2_cfg = { 0 };
> +	struct led_flash_setting *intensity = &v4l2_cfg.intensity;
> +
> +	intensity->min = tps6131x->step_torch_current_ma;
> +	intensity->max = tps6131x->max_torch_current_ma;
> +	intensity->step = tps6131x->step_torch_current_ma;
> +	intensity->val = intensity->min;
> +
> +	strscpy(v4l2_cfg.dev_name, tps6131x->fled_cdev.led_cdev.dev->kobj.name,

tps6131x->client->dev?

> +		sizeof(v4l2_cfg.dev_name));
> +
> +	v4l2_cfg.has_external_strobe = true;
> +	v4l2_cfg.flash_faults = LED_FAULT_TIMEOUT | LED_FAULT_OVER_TEMPERATURE |
> +				LED_FAULT_SHORT_CIRCUIT | LED_FAULT_UNDER_VOLTAGE |
> +				LED_FAULT_LED_OVER_TEMPERATURE;
> +
> +	tps6131x->v4l2_flash = v4l2_flash_init(&tps6131x->client->dev, tps6131x->led_node,
> +					       &tps6131x->fled_cdev, &tps6131x_v4l2_flash_ops,
> +					       &v4l2_cfg);
> +	if (IS_ERR(tps6131x->v4l2_flash)) {
> +		dev_err(&tps6131x->client->dev, "v4l2_flash_init failed\n");

"Failed to initialise V4L2 flash LED" ?

> +		return PTR_ERR(tps6131x->v4l2_flash);
> +	}
> +
> +	return 0;
> +}
> +
> +#else
> +
> +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
> +{
> +	return 0;
> +}
> +
> +#endif
> +
> +static int tps6131x_probe(struct i2c_client *client)
> +{
> +	struct tps6131x *tps6131x;
> +	int ret;
> +
> +	tps6131x = devm_kzalloc(&client->dev, sizeof(*tps6131x), GFP_KERNEL);
> +	if (!tps6131x)
> +		return -ENOMEM;
> +
> +	tps6131x->client = client;

What are you planning on using client for?

> +	i2c_set_clientdata(client, tps6131x);

How are you going to _get_ this without client?

Why not save dev and reduce the amount of dereferencing levels required.

> +	mutex_init(&tps6131x->lock);
> +	INIT_DELAYED_WORK(&tps6131x->torch_refresh_work, tps6131x_torch_refresh_handler);
> +
> +	ret = tps6131x_parse_node(tps6131x);
> +	if (ret)
> +		return -ENODEV;
> +
> +	tps6131x->regmap = devm_regmap_init_i2c(client, &tps6131x_regmap);
> +	if (IS_ERR(tps6131x->regmap)) {
> +		ret = PTR_ERR(tps6131x->regmap);
> +		dev_err(&client->dev, "Failed to allocate register map\n");
> +		return ret;
> +	}
> +
> +	tps6131x->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> +	ret = tps6131x_reset_chip(tps6131x);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to reset LED controller\n");

How do you manage the optional part?

> +	ret = tps6131x_init_chip(tps6131x);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to initialize LED controller\n");
> +
> +	ret = tps6131x_led_class_setup(tps6131x);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to setup led class\n");

Always "LED"

> +	ret = tps6131x_v4l2_setup(tps6131x);
> +	if (ret)
> +		return dev_err_probe(&client->dev, ret, "Failed to setup v4l2 flash\n");
> +
> +	return 0;
> +}
> +
> +static void tps6131x_remove(struct i2c_client *client)
> +{
> +	struct tps6131x *tps6131x = i2c_get_clientdata(client);
> +
> +	v4l2_flash_release(tps6131x->v4l2_flash);
> +
> +	cancel_delayed_work_sync(&tps6131x->torch_refresh_work);
> +}
> +
> +static const struct of_device_id of_tps6131x_leds_match[] = {
> +	{ .compatible = "ti,tps61310" },
> +	{ .compatible = "ti,tps61311" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, of_tps6131x_leds_match);
> +
> +static struct i2c_driver tps6131x_i2c_driver = {
> +	.driver = {
> +		.name = "tps6131x",
> +		.of_match_table = of_tps6131x_leds_match,
> +	},
> +	.probe = tps6131x_probe,
> +	.remove = tps6131x_remove,
> +};
> +module_i2c_driver(tps6131x_i2c_driver);
> +
> +MODULE_DESCRIPTION("Texas Instruments TPS6131X flash LED driver");
> +MODULE_AUTHOR("Matthias Fend <matthias.fend@emfend.at>");
> +MODULE_LICENSE("GPL");
> 
> -- 
> 2.34.1
> 

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Matthias Fend]

Hi Lee,

thanks a lot for your feedback!

Am 10.03.2025 um 15:49 schrieb Lee Jones:
> On Fri, 28 Feb 2025, Matthias Fend wrote:
> 
>> The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
>> stage is capable of supplying a maximum total current of roughly 1500mA.
>> The TPS6131x provides three constant-current sinks, capable of sinking up
>> to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
>> each sink (LED1, LED2, LED3) supports currents up to 175mA.
>>
>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>> ---
>>   MAINTAINERS                        |   7 +
>>   drivers/leds/flash/Kconfig         |  11 +
>>   drivers/leds/flash/Makefile        |   1 +
>>   drivers/leds/flash/leds-tps6131x.c | 798 +++++++++++++++++++++++++++++++++++++
>>   4 files changed, 817 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0fa7c5728f1e64d031f4a47b6fce1db484ce0fc2..d2ca840647b566cfee4d8ded1f787e32be4aa163 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -23225,6 +23225,13 @@ F:	Documentation/devicetree/bindings/hwmon/ti,tps23861.yaml
>>   F:	Documentation/hwmon/tps23861.rst
>>   F:	drivers/hwmon/tps23861.c
>>   
>> +TEXAS INSTRUMENTS TPS6131X FLASH LED DRIVER
>> +M:	Matthias Fend <matthias.fend@emfend.at>
>> +L:	linux-leds@vger.kernel.org
>> +S:	Maintained
>> +F:	Documentation/devicetree/bindings/leds/ti,tps6131x.yaml
>> +F:	drivers/leds/flash/leds-tps6131x.c
>> +
>>   TEXAS INSTRUMENTS' DAC7612 DAC DRIVER
>>   M:	Ricardo Ribalda <ribalda@kernel.org>
>>   L:	linux-iio@vger.kernel.org
>> diff --git a/drivers/leds/flash/Kconfig b/drivers/leds/flash/Kconfig
>> index f39f0bfe6eefcd376405d9d35dc36e323a485002..55ca663ca506ad8be627f58f6d6308368ea2b928 100644
>> --- a/drivers/leds/flash/Kconfig
>> +++ b/drivers/leds/flash/Kconfig
>> @@ -132,4 +132,15 @@ config LEDS_SY7802
>>   
>>   	  This driver can be built as a module, it will be called "leds-sy7802".
>>   
>> +config LEDS_TPS6131X
>> +	tristate "LED support for TI TPS6131x flash LED driver"
>> +	depends on I2C && OF
>> +	depends on GPIOLIB
>> +	select REGMAP_I2C
>> +	help
>> +	  This option enables support for Texas Instruments TPS61310/TPS61311
>> +	  flash LED driver.
>> +
>> +	  This driver can be built as a module, it will be called "leds-tps6131x".
>> +
>>   endif # LEDS_CLASS_FLASH
>> diff --git a/drivers/leds/flash/Makefile b/drivers/leds/flash/Makefile
>> index 48860eeced79513a0ed303e4af3db9bfe9724b7e..712fb737a428e42747e1aa339058dc4306ade9c8 100644
>> --- a/drivers/leds/flash/Makefile
>> +++ b/drivers/leds/flash/Makefile
>> @@ -12,3 +12,4 @@ obj-$(CONFIG_LEDS_RT4505)	+= leds-rt4505.o
>>   obj-$(CONFIG_LEDS_RT8515)	+= leds-rt8515.o
>>   obj-$(CONFIG_LEDS_SGM3140)	+= leds-sgm3140.o
>>   obj-$(CONFIG_LEDS_SY7802)	+= leds-sy7802.o
>> +obj-$(CONFIG_LEDS_TPS6131X)	+= leds-tps6131x.o
>> diff --git a/drivers/leds/flash/leds-tps6131x.c b/drivers/leds/flash/leds-tps6131x.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..5fca542ac88da1db755a75e982dee8e3dde06373
>> --- /dev/null
>> +++ b/drivers/leds/flash/leds-tps6131x.c
>> @@ -0,0 +1,798 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Texas Instruments TPS61310/TPS61311 flash LED driver with I2C interface
>> + *
>> + * Copyright 2025 Matthias Fend <matthias.fend@emfend.at>
>> + */
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/i2c.h>
>> +#include <linux/led-class-flash.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <media/v4l2-flash-led-class.h>
>> +
>> +/* Registers */
>> +
>> +#define TPS6131X_REG_0				0x00
>> +#define TPS6131X_REG_0_RESET			BIT(7)
>> +#define TPS6131X_REG_0_DCLC13			GENMASK(5, 3)
>> +#define TPS6131X_REG_0_DCLC13_SHIFT		3
>> +#define TPS6131X_REG_0_DCLC2			GENMASK(2, 0)
>> +#define TPS6131X_REG_0_DCLC2_SHIFT		0
>> +
>> +#define TPS6131X_REG_1				0x01
>> +#define TPS6131X_REG_1_MODE			GENMASK(7, 6)
>> +#define TPS6131X_REG_1_MODE_SHIFT		6
>> +#define TPS6131X_REG_1_FC2			GENMASK(5, 0)
>> +#define TPS6131X_REG_1_FC2_SHIFT		0
>> +
>> +#define TPS6131X_REG_2				0x02
>> +#define TPS6131X_REG_2_MODE			GENMASK(7, 6)
>> +#define TPS6131X_REG_2_MODE_SHIFT		6
>> +#define TPS6131X_REG_2_ENVM			BIT(5)
>> +#define TPS6131X_REG_2_FC13			GENMASK(4, 0)
>> +#define TPS6131X_REG_2_FC13_SHIFT		0
>> +
>> +#define TPS6131X_REG_3				0x03
>> +#define TPS6131X_REG_3_STIM			GENMASK(7, 5)
>> +#define TPS6131X_REG_3_STIM_SHIFT		5
>> +#define TPS6131X_REG_3_HPFL			BIT(4)
>> +#define TPS6131X_REG_3_SELSTIM_TO		BIT(3)
>> +#define TPS6131X_REG_3_STT			BIT(2)
>> +#define TPS6131X_REG_3_SFT			BIT(1)
>> +#define TPS6131X_REG_3_TXMASK			BIT(0)
>> +
>> +#define TPS6131X_REG_4				0x04
>> +#define TPS6131X_REG_4_PG			BIT(7)
>> +#define TPS6131X_REG_4_HOTDIE_HI		BIT(6)
>> +#define TPS6131X_REG_4_HOTDIE_LO		BIT(5)
>> +#define TPS6131X_REG_4_ILIM			BIT(4)
>> +#define TPS6131X_REG_4_INDC			GENMASK(3, 0)
>> +#define TPS6131X_REG_4_SHIFT			0
>> +
>> +#define TPS6131X_REG_5				0x05
>> +#define TPS6131X_REG_5_SELFCAL			BIT(7)
>> +#define TPS6131X_REG_5_ENPSM			BIT(6)
>> +#define TPS6131X_REG_5_STSTRB1_DIR		BIT(5)
>> +#define TPS6131X_REG_5_GPIO			BIT(4)
>> +#define TPS6131X_REG_5_GPIOTYPE			BIT(3)
>> +#define TPS6131X_REG_5_ENLED3			BIT(2)
>> +#define TPS6131X_REG_5_ENLED2			BIT(1)
>> +#define TPS6131X_REG_5_ENLED1			BIT(0)
>> +
>> +#define TPS6131X_REG_6				0x06
>> +#define TPS6131X_REG_6_ENTS			BIT(7)
>> +#define TPS6131X_REG_6_LEDHOT			BIT(6)
>> +#define TPS6131X_REG_6_LEDWARN			BIT(5)
>> +#define TPS6131X_REG_6_LEDHDR			BIT(4)
>> +#define TPS6131X_REG_6_OV			GENMASK(3, 0)
>> +#define TPS6131X_REG_6_OV_SHIFT			0
>> +
>> +#define TPS6131X_REG_7				0x07
>> +#define TPS6131X_REG_7_ENBATMON			BIT(7)
>> +#define TPS6131X_REG_7_BATDROOP			GENMASK(6, 4)
>> +#define TPS6131X_REG_7_BATDROOP_SHIFT		4
>> +#define TPS6131X_REG_7_REVID			GENMASK(2, 0)
>> +#define TPS6131X_REG_7_REVID_SHIFT		0
>> +
>> +/* Constants */
>> +
>> +#define TPS6131X_MAX_CHANNELS			3
>> +
>> +#define TPS6131X_FLASH_MAX_I_CHAN13_MA		400
>> +#define TPS6131X_FLASH_MAX_I_CHAN2_MA		800
>> +#define TPS6131X_FLASH_STEP_I_MA		25
>> +
>> +#define TPS6131X_TORCH_MAX_I_CHAN13_MA		175
>> +#define TPS6131X_TORCH_MAX_I_CHAN2_MA		175
>> +#define TPS6131X_TORCH_STEP_I_MA		25
>> +
>> +#define TPS6131X_TORCH_REFRESH_INTERVAL_JIFFIES msecs_to_jiffies(10000)
>> +
>> +struct tps6131x {
>> +	struct i2c_client *client;
>> +	struct regmap *regmap;
>> +	struct gpio_desc *reset_gpio;
>> +	struct mutex lock; /* */
> 
> ?

Argh. The comment is missing - I probably wanted to quickly silence 
checkpatch :/

> 
>> +	struct delayed_work torch_refresh_work;
>> +	bool valley_current_limit;
>> +	bool chan1_en;
>> +	bool chan2_en;
>> +	bool chan3_en;
>> +	struct fwnode_handle *led_node;
>> +	u32 max_flash_current_ma;
>> +	u32 step_flash_current_ma;
>> +	u32 max_torch_current_ma;
>> +	u32 step_torch_current_ma;
>> +	u32 max_timeout_us;
>> +	struct led_classdev_flash fled_cdev;
>> +	struct v4l2_flash *v4l2_flash;
>> +};
>> +
>> +static struct tps6131x *fled_cdev_to_tps6131x(struct led_classdev_flash *fled_cdev)
>> +{
>> +	return container_of(fled_cdev, struct tps6131x, fled_cdev);
>> +}
>> +
>> +static const struct reg_default tps6131x_regmap_defaults[] = {
>> +	{ TPS6131X_REG_0, 0x0A },
>> +	{ TPS6131X_REG_1, 0x10 },
>> +	{ TPS6131X_REG_2, 0x08 },
>> +	{ TPS6131X_REG_3, 0xC1 },
>> +	{ TPS6131X_REG_4, 0x00 },
>> +	{ TPS6131X_REG_5, 0x6A },
>> +	{ TPS6131X_REG_6, 0x00 },
>> +	{ TPS6131X_REG_7, 0x46 },
> 
> I'm not a big fan of blind register patching.
> 
> These would be better either defined (preferred) or commented.

These are the actual register values ​​after a reset. They must be 
copied 1:1 from the datasheet and can never be changed. I was concerned 
that using the defines here would give the impression that a 
configuration could be changed at this point.
But I'll change it and build the values ​​with the existing defines and 
add a comment.

> 
>> +};
>> +
>> +/*
>> + * These registers contain flags that are reset when read. Ensure that these registers are not read
>> + * outside of a call from the driver.
>> + */
>> +static bool tps6131x_regmap_precious(struct device *dev, unsigned int reg)
>> +{
>> +	switch (reg) {
>> +	case TPS6131X_REG_3:
>> +	case TPS6131X_REG_4:
>> +	case TPS6131X_REG_6:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static const struct regmap_config tps6131x_regmap = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.max_register = TPS6131X_REG_7,
>> +	.reg_defaults = tps6131x_regmap_defaults,
>> +	.num_reg_defaults = ARRAY_SIZE(tps6131x_regmap_defaults),
>> +	.cache_type = REGCACHE_FLAT,
>> +	.precious_reg = &tps6131x_regmap_precious,
>> +};
>> +
>> +struct tps6131x_timer_config {
>> +	u8 val;
>> +	u8 range;
>> +	u32 time_us;
>> +};
>> +
>> +static const struct tps6131x_timer_config tps6131x_timer_configs[] = {
>> +	{ .val = 0, .range = 1, .time_us = 5300 },
>> +	{ .val = 1, .range = 1, .time_us = 10700 },
>> +	{ .val = 2, .range = 1, .time_us = 16000 },
>> +	{ .val = 3, .range = 1, .time_us = 21300 },
>> +	{ .val = 4, .range = 1, .time_us = 26600 },
>> +	{ .val = 5, .range = 1, .time_us = 32000 },
>> +	{ .val = 6, .range = 1, .time_us = 37300 },
>> +	{ .val = 0, .range = 0, .time_us = 68200 },
>> +	{ .val = 7, .range = 1, .time_us = 71500 },
>> +	{ .val = 1, .range = 0, .time_us = 102200 },
>> +	{ .val = 2, .range = 0, .time_us = 136300 },
>> +	{ .val = 3, .range = 0, .time_us = 170400 },
>> +	{ .val = 4, .range = 0, .time_us = 204500 },
>> +	{ .val = 5, .range = 0, .time_us = 340800 },
>> +	{ .val = 6, .range = 0, .time_us = 579300 },
>> +	{ .val = 7, .range = 0, .time_us = 852000 },
>> +};
>> +
>> +static const struct tps6131x_timer_config *tps6131x_find_closest_timer_config(u32 timeout_us)
>> +{
>> +	const struct tps6131x_timer_config *timer_config = &tps6131x_timer_configs[0];
>> +	u32 diff, min_diff = U32_MAX;
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(tps6131x_timer_configs); i++) {
>> +		diff = abs(tps6131x_timer_configs[i].time_us - timeout_us);
>> +		if (diff < min_diff) {
>> +			timer_config = &tps6131x_timer_configs[i];
>> +			min_diff = diff;
>> +			if (!min_diff)
>> +				break;
>> +		}
>> +	}
>> +
>> +	return timer_config;
>> +}
>> +
>> +static int tps6131x_reset_chip(struct tps6131x *tps6131x)
>> +{
>> +	int ret;
>> +
>> +	if (IS_ERR_OR_NULL(tps6131x->reset_gpio)) {
>> +		ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_0, TPS6131X_REG_0_RESET,
>> +					 TPS6131X_REG_0_RESET);
>> +		if (ret)
>> +			return ret;
> 
> No delay required for this path?

Yes, I found that a bit strange too. But I didn't find any reference to 
any reset timing in the data sheet (apart from the minimum width of the 
reset pulse). I also couldn't find any practical problems with the hardware.
Nevertheless, I asked TI support and they said that it would be good to 
wait maybe 50-100us to discharge the analog circuit section.
So I'll add a 100us delay for both paths to be on the safe side.

> 
>> +		ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_0, TPS6131X_REG_0_RESET, 0);
>> +		if (ret)
>> +			return ret;
>> +	} else {
>> +		gpiod_set_value_cansleep(tps6131x->reset_gpio, 1);
>> +		fsleep(10);
>> +		gpiod_set_value_cansleep(tps6131x->reset_gpio, 0);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int tps6131x_init_chip(struct tps6131x *tps6131x)
>> +{
>> +	u32 reg4, reg5, reg6;
>> +	int ret;
>> +
>> +	reg4 = tps6131x->valley_current_limit ? TPS6131X_REG_4_ILIM : 0;
>> +	ret = regmap_write(tps6131x->regmap, TPS6131X_REG_4, reg4);
>> +	if (ret)
>> +		return ret;
>> +
>> +	reg5 = TPS6131X_REG_5_ENPSM | TPS6131X_REG_5_STSTRB1_DIR | TPS6131X_REG_5_GPIOTYPE;
>> +	if (tps6131x->chan1_en)
>> +		reg5 |= TPS6131X_REG_5_ENLED1;
>> +
>> +	if (tps6131x->chan2_en)
>> +		reg5 |= TPS6131X_REG_5_ENLED2;
>> +
>> +	if (tps6131x->chan3_en)
>> +		reg5 |= TPS6131X_REG_5_ENLED3;
>> +	ret = regmap_write(tps6131x->regmap, TPS6131X_REG_5, reg5);
>> +	if (ret)
>> +		return ret;
>> +
>> +	reg6 = TPS6131X_REG_6_ENTS;
>> +	ret = regmap_write(tps6131x->regmap, TPS6131X_REG_6, reg6);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +enum tps6131x_mode {
>> +	TPS6131X_MODE_SHUTDOWN,
>> +	TPS6131X_MODE_TORCH,
>> +	TPS6131X_MODE_FLASH,
>> +};
>> +
>> +static int tps6131x_set_mode(struct tps6131x *tps6131x, enum tps6131x_mode mode, bool force)
>> +{
>> +	u8 val;
>> +
>> +	switch (mode) {
>> +	case TPS6131X_MODE_FLASH:
>> +		val = 2 << TPS6131X_REG_1_MODE_SHIFT;
>> +		break;
>> +	case TPS6131X_MODE_TORCH:
>> +		val = 1 << TPS6131X_REG_1_MODE_SHIFT;
>> +		break;
>> +	case TPS6131X_MODE_SHUTDOWN:
>> +	default:
>> +		val = 0 << TPS6131X_REG_1_MODE_SHIFT;
>> +		break;
>> +	}
> 
> What about:
> 
> 	val = mode << TPS6131X_REG_1_MODE_SHIFT;

I wanted to avoid using fixed enumeration values. But yes, I think it's 
fine for this case, so I'll change it.

> 
>> +
>> +	return regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_1, TPS6131X_REG_1_MODE, val,
>> +				       NULL, false, force);
>> +}
>> +
>> +static void tps6131x_torch_refresh_handler(struct work_struct *work)
>> +{
>> +	struct tps6131x *tps6131x = container_of(work, struct tps6131x, torch_refresh_work.work);
>> +
>> +	guard(mutex)(&tps6131x->lock);
>> +
>> +	tps6131x_set_mode(tps6131x, TPS6131X_MODE_TORCH, true);
>> +
>> +	schedule_delayed_work(&tps6131x->torch_refresh_work,
>> +			      TPS6131X_TORCH_REFRESH_INTERVAL_JIFFIES);
>> +}
>> +
>> +static int tps6131x_brightness_set(struct led_classdev *cdev, enum led_brightness brightness)
>> +{
>> +	struct led_classdev_flash *fled_cdev = lcdev_to_flcdev(cdev);
>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>> +	int ret;
>> +	u8 reg0;
>> +	u32 steps_remaining, steps_chan13, steps_chan2;
>> +	unsigned int num_chans = tps6131x->chan1_en + tps6131x->chan2_en + tps6131x->chan3_en;
> 
> It would quell my OCD if you reversed these 4 lines.

ACK

> 
>> +	cancel_delayed_work_sync(&tps6131x->torch_refresh_work);
>> +
>> +	guard(mutex)(&tps6131x->lock);
>> +
>> +	/*
>> +	 * The brightness parameter uses the number of current steps as the unit (not the current
>> +	 * value itself). Since the reported step size can vary depending on the configuration,
>> +	 * this value must be converted into actual register steps.
>> +	 */
>> +	steps_remaining = (brightness * tps6131x->step_torch_current_ma) / TPS6131X_TORCH_STEP_I_MA;
>> +
>> +	/*
>> +	 * The currents are distributed as evenly as possible across the activated channels.
>> +	 * Since channels 1 and 3 share the same register setting, they always use the same current
>> +	 * value. Channel 2 supports higher currents and thus takes over the remaining additional
>> +	 * portion that cannot be covered by the other channels.
>> +	 */
>> +	steps_chan13 = min_t(u32, steps_remaining / num_chans,
>> +			     TPS6131X_TORCH_MAX_I_CHAN13_MA / TPS6131X_TORCH_STEP_I_MA);
>> +	if (tps6131x->chan1_en)
>> +		steps_remaining -= steps_chan13;
>> +	if (tps6131x->chan3_en)
>> +		steps_remaining -= steps_chan13;
>> +
>> +	steps_chan2 = min_t(u32, steps_remaining,
>> +			    TPS6131X_TORCH_MAX_I_CHAN2_MA / TPS6131X_TORCH_STEP_I_MA);
>> +
>> +	reg0 = (steps_chan13 << TPS6131X_REG_0_DCLC13_SHIFT) |
>> +	       (steps_chan2 << TPS6131X_REG_0_DCLC2_SHIFT);
>> +	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_0,
>> +				 TPS6131X_REG_0_DCLC13 | TPS6131X_REG_0_DCLC2, reg0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = tps6131x_set_mode(tps6131x, brightness ? TPS6131X_MODE_TORCH : TPS6131X_MODE_SHUTDOWN,
>> +				true);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/*
>> +	 * In order to use both the flash and the video light functions purely via the I2C
>> +	 * interface, STRB1 must be low. If STRB1 is low, then the video light watchdog timer
>> +	 * is also active, which puts the device into the shutdown state after around 13 seconds.
>> +	 * To prevent this, the mode must be refreshed within the watchdog timeout.
>> +	 */
>> +	if (brightness)
>> +		schedule_delayed_work(&tps6131x->torch_refresh_work,
>> +				      TPS6131X_TORCH_REFRESH_INTERVAL_JIFFIES);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tps6131x_strobe_set(struct led_classdev_flash *fled_cdev, bool state)
>> +{
>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>> +	int ret;
>> +
>> +	guard(mutex)(&tps6131x->lock);
>> +
>> +	ret = tps6131x_set_mode(tps6131x, state ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
>> +				true);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (state) {
>> +		ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_3, TPS6131X_REG_3_SFT,
>> +					      TPS6131X_REG_3_SFT, NULL, false, true);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_3, TPS6131X_REG_3_SFT, 0, NULL,
>> +				      false, true);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +static int tps6131x_flash_brightness_set(struct led_classdev_flash *fled_cdev, u32 brightness)
>> +{
>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>> +	u32 steps_remaining, steps_chan13, steps_chan2, num_chans;
>> +	int ret;
>> +
>> +	guard(mutex)(&tps6131x->lock);
>> +
>> +	steps_remaining = brightness / TPS6131X_FLASH_STEP_I_MA;
>> +	num_chans = tps6131x->chan1_en + tps6131x->chan2_en + tps6131x->chan3_en;
>> +
>> +	steps_chan13 = min_t(u32, steps_remaining / num_chans,
>> +			     TPS6131X_FLASH_MAX_I_CHAN13_MA / TPS6131X_FLASH_STEP_I_MA);
>> +	if (tps6131x->chan1_en)
>> +		steps_remaining -= steps_chan13;
>> +	if (tps6131x->chan3_en)
>> +		steps_remaining -= steps_chan13;
>> +	steps_chan2 = min_t(u32, steps_remaining,
>> +			    TPS6131X_FLASH_MAX_I_CHAN2_MA / TPS6131X_FLASH_STEP_I_MA);
>> +
>> +	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_2, TPS6131X_REG_2_FC13,
>> +				 steps_chan13 << TPS6131X_REG_2_FC13_SHIFT);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_1, TPS6131X_REG_1_FC2,
>> +				 steps_chan2 << TPS6131X_REG_1_FC2_SHIFT);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	fled_cdev->brightness.val = brightness;
>> +
>> +	return 0;
>> +}
>> +
>> +static int tps6131x_flash_timeout_set(struct led_classdev_flash *fled_cdev, u32 timeout_us)
>> +{
>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>> +	int ret;
>> +	u8 reg3;
>> +	const struct tps6131x_timer_config *timer_config;
>> +
>> +	guard(mutex)(&tps6131x->lock);
>> +
>> +	timer_config = tps6131x_find_closest_timer_config(timeout_us);
>> +
>> +	reg3 = timer_config->val << TPS6131X_REG_3_STIM_SHIFT;
>> +	if (timer_config->range)
>> +		reg3 |= TPS6131X_REG_3_SELSTIM_TO;
>> +
>> +	ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_3,
>> +				 TPS6131X_REG_3_STIM | TPS6131X_REG_3_SELSTIM_TO, reg3);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	fled_cdev->timeout.val = timer_config->time_us;
>> +
>> +	return 0;
>> +}
>> +
>> +static int tps6131x_strobe_get(struct led_classdev_flash *fled_cdev, bool *state)
>> +{
>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>> +	unsigned int reg3;
>> +	int ret;
>> +
>> +	guard(mutex)(&tps6131x->lock);
>> +
>> +	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_3, &reg3);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*state = !!(reg3 & TPS6131X_REG_3_SFT);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tps6131x_flash_fault_get(struct led_classdev_flash *fled_cdev, u32 *fault)
>> +{
>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>> +	unsigned int reg3, reg4, reg6;
>> +	int ret;
>> +
>> +	*fault = 0;
>> +
>> +	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_3, &reg3);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_4, &reg4);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = regmap_read_bypassed(tps6131x->regmap, TPS6131X_REG_6, &reg6);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (reg3 & TPS6131X_REG_3_HPFL)
>> +		*fault |= LED_FAULT_SHORT_CIRCUIT;
>> +
>> +	if (reg3 & TPS6131X_REG_3_SELSTIM_TO)
>> +		*fault |= LED_FAULT_TIMEOUT;
>> +
>> +	if (reg4 & TPS6131X_REG_4_HOTDIE_HI)
>> +		*fault |= LED_FAULT_OVER_TEMPERATURE;
>> +
>> +	if (reg6 & (TPS6131X_REG_6_LEDHOT | TPS6131X_REG_6_LEDWARN))
>> +		*fault |= LED_FAULT_LED_OVER_TEMPERATURE;
>> +
>> +	if (!(reg6 & TPS6131X_REG_6_LEDHDR))
>> +		*fault |= LED_FAULT_UNDER_VOLTAGE;
>> +
>> +	if (reg6 & TPS6131X_REG_6_LEDHOT) {
>> +		ret = regmap_update_bits_base(tps6131x->regmap, TPS6131X_REG_6,
>> +					      TPS6131X_REG_6_LEDHOT, 0, NULL, false, true);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct led_flash_ops flash_ops = {
>> +	.flash_brightness_set = tps6131x_flash_brightness_set,
>> +	.strobe_set = tps6131x_strobe_set,
>> +	.strobe_get = tps6131x_strobe_get,
>> +	.timeout_set = tps6131x_flash_timeout_set,
>> +	.fault_get = tps6131x_flash_fault_get,
>> +};
>> +
>> +static int tps6131x_parse_node(struct tps6131x *tps6131x)
>> +{
>> +	struct device *dev = &tps6131x->client->dev;
>> +	int ret;
>> +	u32 current_ua, timeout_us;
>> +	const struct tps6131x_timer_config *timer_config;
>> +	u32 flash_max_i_ma, torch_max_i_ma;
>> +	u32 current_step_multiplier;
>> +	u32 channels[TPS6131X_MAX_CHANNELS];
>> +	unsigned int num_channels;
>> +	int i;
> 
> There doesn't appear to be any ordering of these variables at all
> 
> *twitch*
> 
> Please employ some kind of structure - I don't mind which.

True. I'll try to sort the variables.

> 
>> +
>> +	tps6131x->valley_current_limit = device_property_read_bool(dev, "ti,valley-current-limit");
>> +
>> +	tps6131x->led_node = fwnode_get_next_available_child_node(dev->fwnode, NULL);
>> +	if (!tps6131x->led_node) {
>> +		dev_err(dev, "Missing LED node\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	num_channels = fwnode_property_count_u32(tps6131x->led_node, "led-sources");
>> +	if (num_channels <= 0) {
>> +		dev_err(dev, "Failed to read led-sources property\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (num_channels > TPS6131X_MAX_CHANNELS) {
>> +		dev_err(dev, "led-sources count %u exceeds maximum channel count %u\n",
>> +			num_channels, TPS6131X_MAX_CHANNELS);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = fwnode_property_read_u32_array(tps6131x->led_node, "led-sources", channels,
>> +					     num_channels);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to read led-sources property\n");
>> +		return ret;
>> +	}
>> +
>> +	flash_max_i_ma = 0;
>> +	torch_max_i_ma = 0;
>> +	for (i = 0; i < num_channels; i++) {
>> +		switch (channels[i]) {
>> +		case 1:
>> +			tps6131x->chan1_en = true;
>> +			flash_max_i_ma += TPS6131X_FLASH_MAX_I_CHAN13_MA;
>> +			torch_max_i_ma += TPS6131X_TORCH_MAX_I_CHAN13_MA;
>> +			break;
>> +		case 2:
>> +			tps6131x->chan2_en = true;
>> +			flash_max_i_ma += TPS6131X_FLASH_MAX_I_CHAN2_MA;
>> +			torch_max_i_ma += TPS6131X_TORCH_MAX_I_CHAN2_MA;
>> +			break;
>> +		case 3:
>> +			tps6131x->chan3_en = true;
>> +			flash_max_i_ma += TPS6131X_FLASH_MAX_I_CHAN13_MA;
>> +			torch_max_i_ma += TPS6131X_TORCH_MAX_I_CHAN13_MA;
>> +			break;
>> +		default:
>> +			dev_err(dev, "led-source out of range [1-3]\n");
>> +			return -EINVAL;
>> +		}
>> +	}
>> +
>> +	/*
>> +	 * If only channels 1 and 3 are used, the step size is doubled because the two channels
>> +	 * share the same current control register.
>> +	 */
>> +	current_step_multiplier =
>> +		(tps6131x->chan1_en && tps6131x->chan3_en && !tps6131x->chan2_en) ? 2 : 1;
>> +	tps6131x->step_flash_current_ma = current_step_multiplier * TPS6131X_FLASH_STEP_I_MA;
>> +	tps6131x->step_torch_current_ma = current_step_multiplier * TPS6131X_TORCH_STEP_I_MA;
>> +
>> +	ret = fwnode_property_read_u32(tps6131x->led_node, "led-max-microamp", &current_ua);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to read led-max-microamp property\n");
>> +		return ret;
>> +	}
>> +
>> +	tps6131x->max_torch_current_ma = current_ua / 1000;
> 
> There should be a MACRO for these types of conversions.

ACK

> 
>> +
>> +	if (!tps6131x->max_torch_current_ma || tps6131x->max_torch_current_ma > torch_max_i_ma ||
>> +	    (tps6131x->max_torch_current_ma % tps6131x->step_torch_current_ma)) {
>> +		dev_err(dev, "led-max-microamp out of range or not a multiple of %u\n",
>> +			tps6131x->step_torch_current_ma);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = fwnode_property_read_u32(tps6131x->led_node, "flash-max-microamp", &current_ua);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to read flash-max-microamp property\n");
>> +		return ret;
>> +	}
>> +
>> +	tps6131x->max_flash_current_ma = current_ua / 1000;
>> +
>> +	if (!tps6131x->max_flash_current_ma || tps6131x->max_flash_current_ma > flash_max_i_ma ||
>> +	    (tps6131x->max_flash_current_ma % tps6131x->step_flash_current_ma)) {
>> +		dev_err(dev, "flash-max-microamp out of range or not a multiple of %u\n",
>> +			tps6131x->step_flash_current_ma);
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = fwnode_property_read_u32(tps6131x->led_node, "flash-max-timeout-us", &timeout_us);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Failed to read flash-max-timeout-us property\n");
>> +		return ret;
>> +	}
>> +
>> +	timer_config = tps6131x_find_closest_timer_config(timeout_us);
>> +	tps6131x->max_timeout_us = timer_config->time_us;
>> +
>> +	if (tps6131x->max_timeout_us != timeout_us)
>> +		dev_warn(dev, "flash-max-timeout-us %u not supported (using %u)\n", timeout_us,
>> +			 tps6131x->max_timeout_us);
>> +
>> +	return 0;
>> +}
>> +
>> +static int tps6131x_led_class_setup(struct tps6131x *tps6131x)
>> +{
>> +	struct led_classdev *led_cdev;
>> +	struct led_flash_setting *setting;
>> +	struct led_init_data init_data = {};
>> +	static const struct tps6131x_timer_config *timer_config;
>> +	int ret;
>> +
>> +	tps6131x->fled_cdev.ops = &flash_ops;
>> +
>> +	setting = &tps6131x->fled_cdev.timeout;
>> +	timer_config = tps6131x_find_closest_timer_config(0);
>> +	setting->min = timer_config->time_us;
>> +	setting->max = tps6131x->max_timeout_us;
>> +	setting->step = 1; /* Only some specific time periods are supported. No fixed step size. */
>> +	setting->val = setting->min;
>> +
>> +	setting = &tps6131x->fled_cdev.brightness;
>> +	setting->min = tps6131x->step_flash_current_ma;
>> +	setting->max = tps6131x->max_flash_current_ma;
>> +	setting->step = tps6131x->step_flash_current_ma;
>> +	setting->val = setting->min;
>> +
>> +	led_cdev = &tps6131x->fled_cdev.led_cdev;
>> +	led_cdev->brightness_set_blocking = tps6131x_brightness_set;
>> +	led_cdev->max_brightness = tps6131x->max_torch_current_ma;
>> +	led_cdev->flags |= LED_DEV_CAP_FLASH;
>> +
>> +	init_data.fwnode = tps6131x->led_node;
>> +	init_data.devicename = NULL;
>> +	init_data.default_label = NULL;
>> +	init_data.devname_mandatory = false;
>> +
>> +	ret = devm_led_classdev_flash_register_ext(&tps6131x->client->dev, &tps6131x->fled_cdev,
>> +						   &init_data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +}
>> +
>> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> 
> Not keen on #ifery in C files.
> 
> Can you use is_defined() and return early instead?
> 
> I see that there is a precedent for this already. :(

Me neither, but since it is done this way in about 9 out of 10 flash 
controllers, I wanted to continue doing it consistently.
But since the required v4l2_flash_* functions are also available as 
dummies if this option is not activated, I could do it like this:

if (!IS_BUILTIN(CONFIG_V4L2_FLASH_LED_CLASS))
   return 0;

Would you prefer this solution?

> 
>> +static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
>> +{
>> +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>> +
>> +	guard(mutex)(&tps6131x->lock);
>> +
> /> +	return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
>> +				 false);
>> +}
>> +
>> +static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
>> +	.external_strobe_set = tps6131x_flash_external_strobe_set,
>> +};
>> +
>> +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
>> +{
>> +	struct v4l2_flash_config v4l2_cfg = { 0 };
>> +	struct led_flash_setting *intensity = &v4l2_cfg.intensity;
>> +
>> +	intensity->min = tps6131x->step_torch_current_ma;
>> +	intensity->max = tps6131x->max_torch_current_ma;
>> +	intensity->step = tps6131x->step_torch_current_ma;
>> +	intensity->val = intensity->min;
>> +
>> +	strscpy(v4l2_cfg.dev_name, tps6131x->fled_cdev.led_cdev.dev->kobj.name,
> 
> tps6131x->client->dev?

Do you mean the name should be taken from the I2C device?
The current name, for example, is 'white:flash-0', while the I2C device 
name would be '4-0033'. So I think the current version is appropriate, 
don't you think?

> 
>> +		sizeof(v4l2_cfg.dev_name));
>> +
>> +	v4l2_cfg.has_external_strobe = true;
>> +	v4l2_cfg.flash_faults = LED_FAULT_TIMEOUT | LED_FAULT_OVER_TEMPERATURE |
>> +				LED_FAULT_SHORT_CIRCUIT | LED_FAULT_UNDER_VOLTAGE |
>> +				LED_FAULT_LED_OVER_TEMPERATURE;
>> +
>> +	tps6131x->v4l2_flash = v4l2_flash_init(&tps6131x->client->dev, tps6131x->led_node,
>> +					       &tps6131x->fled_cdev, &tps6131x_v4l2_flash_ops,
>> +					       &v4l2_cfg);
>> +	if (IS_ERR(tps6131x->v4l2_flash)) {
>> +		dev_err(&tps6131x->client->dev, "v4l2_flash_init failed\n");
> 
> "Failed to initialise V4L2 flash LED" ?

ACK

> 
>> +		return PTR_ERR(tps6131x->v4l2_flash);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +#else
>> +
>> +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
>> +{
>> +	return 0;
>> +}
>> +
>> +#endif
>> +
>> +static int tps6131x_probe(struct i2c_client *client)
>> +{
>> +	struct tps6131x *tps6131x;
>> +	int ret;
>> +
>> +	tps6131x = devm_kzalloc(&client->dev, sizeof(*tps6131x), GFP_KERNEL);
>> +	if (!tps6131x)
>> +		return -ENOMEM;
>> +
>> +	tps6131x->client = client;
> 
> What are you planning on using client for?
> 
>> +	i2c_set_clientdata(client, tps6131x);
> 
> How are you going to _get_ this without client?

Maybe I didn't understand the question correctly, but in 
tps6131x_remove() I get the device data via the client.

> 
> Why not save dev and reduce the amount of dereferencing levels required.

Absolutely. Good idea.

> 
>> +	mutex_init(&tps6131x->lock);
>> +	INIT_DELAYED_WORK(&tps6131x->torch_refresh_work, tps6131x_torch_refresh_handler);
>> +
>> +	ret = tps6131x_parse_node(tps6131x);
>> +	if (ret)
>> +		return -ENODEV;
>> +
>> +	tps6131x->regmap = devm_regmap_init_i2c(client, &tps6131x_regmap);
>> +	if (IS_ERR(tps6131x->regmap)) {
>> +		ret = PTR_ERR(tps6131x->regmap);
>> +		dev_err(&client->dev, "Failed to allocate register map\n");
>> +		return ret;
>> +	}
>> +
>> +	tps6131x->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
>> +	ret = tps6131x_reset_chip(tps6131x);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret, "Failed to reset LED controller\n");
> 
> How do you manage the optional part?

If there is a reset line, then tps6131x_reset_chip() uses it to reset 
the chip. If there is none, the software reset (via an I2C register) is 
used. Therefore the reset pin can be optional.

> 
>> +	ret = tps6131x_init_chip(tps6131x);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret, "Failed to initialize LED controller\n");
>> +
>> +	ret = tps6131x_led_class_setup(tps6131x);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret, "Failed to setup led class\n");
> 
> Always "LED"

ACK

Thanks
  ~Matthias

> 
>> +	ret = tps6131x_v4l2_setup(tps6131x);
>> +	if (ret)
>> +		return dev_err_probe(&client->dev, ret, "Failed to setup v4l2 flash\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static void tps6131x_remove(struct i2c_client *client)
>> +{
>> +	struct tps6131x *tps6131x = i2c_get_clientdata(client);
>> +
>> +	v4l2_flash_release(tps6131x->v4l2_flash);
>> +
>> +	cancel_delayed_work_sync(&tps6131x->torch_refresh_work);
>> +}
>> +
>> +static const struct of_device_id of_tps6131x_leds_match[] = {
>> +	{ .compatible = "ti,tps61310" },
>> +	{ .compatible = "ti,tps61311" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, of_tps6131x_leds_match);
>> +
>> +static struct i2c_driver tps6131x_i2c_driver = {
>> +	.driver = {
>> +		.name = "tps6131x",
>> +		.of_match_table = of_tps6131x_leds_match,
>> +	},
>> +	.probe = tps6131x_probe,
>> +	.remove = tps6131x_remove,
>> +};
>> +module_i2c_driver(tps6131x_i2c_driver);
>> +
>> +MODULE_DESCRIPTION("Texas Instruments TPS6131X flash LED driver");
>> +MODULE_AUTHOR("Matthias Fend <matthias.fend@emfend.at>");
>> +MODULE_LICENSE("GPL");
>>
>> -- 
>> 2.34.1
>>
> 

Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Lee Jones]

On Fri, 14 Mar 2025, Matthias Fend wrote:

> Hi Lee,
> 
> thanks a lot for your feedback!
> 
> Am 10.03.2025 um 15:49 schrieb Lee Jones:
> > On Fri, 28 Feb 2025, Matthias Fend wrote:
> > 
> > > The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
> > > stage is capable of supplying a maximum total current of roughly 1500mA.
> > > The TPS6131x provides three constant-current sinks, capable of sinking up
> > > to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
> > > each sink (LED1, LED2, LED3) supports currents up to 175mA.
> > > 
> > > Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> > > ---
> > >   MAINTAINERS                        |   7 +
> > >   drivers/leds/flash/Kconfig         |  11 +
> > >   drivers/leds/flash/Makefile        |   1 +
> > >   drivers/leds/flash/leds-tps6131x.c | 798 +++++++++++++++++++++++++++++++++++++
> > >   4 files changed, 817 insertions(+)

[...]

> > > +static int tps6131x_led_class_setup(struct tps6131x *tps6131x)
> > > +{
> > > +	struct led_classdev *led_cdev;
> > > +	struct led_flash_setting *setting;
> > > +	struct led_init_data init_data = {};
> > > +	static const struct tps6131x_timer_config *timer_config;
> > > +	int ret;
> > > +
> > > +	tps6131x->fled_cdev.ops = &flash_ops;
> > > +
> > > +	setting = &tps6131x->fled_cdev.timeout;
> > > +	timer_config = tps6131x_find_closest_timer_config(0);
> > > +	setting->min = timer_config->time_us;
> > > +	setting->max = tps6131x->max_timeout_us;
> > > +	setting->step = 1; /* Only some specific time periods are supported. No fixed step size. */
> > > +	setting->val = setting->min;
> > > +
> > > +	setting = &tps6131x->fled_cdev.brightness;
> > > +	setting->min = tps6131x->step_flash_current_ma;
> > > +	setting->max = tps6131x->max_flash_current_ma;
> > > +	setting->step = tps6131x->step_flash_current_ma;
> > > +	setting->val = setting->min;
> > > +
> > > +	led_cdev = &tps6131x->fled_cdev.led_cdev;
> > > +	led_cdev->brightness_set_blocking = tps6131x_brightness_set;
> > > +	led_cdev->max_brightness = tps6131x->max_torch_current_ma;
> > > +	led_cdev->flags |= LED_DEV_CAP_FLASH;
> > > +
> > > +	init_data.fwnode = tps6131x->led_node;
> > > +	init_data.devicename = NULL;
> > > +	init_data.default_label = NULL;
> > > +	init_data.devname_mandatory = false;
> > > +
> > > +	ret = devm_led_classdev_flash_register_ext(&tps6131x->client->dev, &tps6131x->fled_cdev,
> > > +						   &init_data);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
> > 
> > Not keen on #ifery in C files.
> > 
> > Can you use is_defined() and return early instead?
> > 
> > I see that there is a precedent for this already. :(
> 
> Me neither, but since it is done this way in about 9 out of 10 flash
> controllers, I wanted to continue doing it consistently.
> But since the required v4l2_flash_* functions are also available as dummies
> if this option is not activated, I could do it like this:
> 
> if (!IS_BUILTIN(CONFIG_V4L2_FLASH_LED_CLASS))
>   return 0;
> 
> Would you prefer this solution?

I would, yes.  Thank you.

> > > +static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
> > > +{
> > > +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > > +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> > > +
> > > +	guard(mutex)(&tps6131x->lock);
> > > +
> > /> +	return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
> > > +				 false);
> > > +}
> > > +
> > > +static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
> > > +	.external_strobe_set = tps6131x_flash_external_strobe_set,
> > > +};
> > > +
> > > +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
> > > +{
> > > +	struct v4l2_flash_config v4l2_cfg = { 0 };
> > > +	struct led_flash_setting *intensity = &v4l2_cfg.intensity;
> > > +
> > > +	intensity->min = tps6131x->step_torch_current_ma;
> > > +	intensity->max = tps6131x->max_torch_current_ma;
> > > +	intensity->step = tps6131x->step_torch_current_ma;
> > > +	intensity->val = intensity->min;
> > > +
> > > +	strscpy(v4l2_cfg.dev_name, tps6131x->fled_cdev.led_cdev.dev->kobj.name,
> > 
> > tps6131x->client->dev?
> 
> Do you mean the name should be taken from the I2C device?
> The current name, for example, is 'white:flash-0', while the I2C device name
> would be '4-0033'. So I think the current version is appropriate, don't you
> think?

No, I'm implying that:

  tps6131x->client->dev == tps6131x->fled_cdev.led_cdev.dev

... and that the former is shorter / neater.

> > > +		sizeof(v4l2_cfg.dev_name));
> > > +
> > > +	v4l2_cfg.has_external_strobe = true;
> > > +	v4l2_cfg.flash_faults = LED_FAULT_TIMEOUT | LED_FAULT_OVER_TEMPERATURE |
> > > +				LED_FAULT_SHORT_CIRCUIT | LED_FAULT_UNDER_VOLTAGE |
> > > +				LED_FAULT_LED_OVER_TEMPERATURE;
> > > +
> > > +	tps6131x->v4l2_flash = v4l2_flash_init(&tps6131x->client->dev, tps6131x->led_node,
> > > +					       &tps6131x->fled_cdev, &tps6131x_v4l2_flash_ops,
> > > +					       &v4l2_cfg);
> > > +	if (IS_ERR(tps6131x->v4l2_flash)) {
> > > +		dev_err(&tps6131x->client->dev, "v4l2_flash_init failed\n");
> > 
> > "Failed to initialise V4L2 flash LED" ?
> 
> ACK
> 
> > 
> > > +		return PTR_ERR(tps6131x->v4l2_flash);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +#else
> > > +
> > > +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
> > > +{
> > > +	return 0;
> > > +}
> > > +
> > > +#endif
> > > +
> > > +static int tps6131x_probe(struct i2c_client *client)
> > > +{
> > > +	struct tps6131x *tps6131x;
> > > +	int ret;
> > > +
> > > +	tps6131x = devm_kzalloc(&client->dev, sizeof(*tps6131x), GFP_KERNEL);
> > > +	if (!tps6131x)
> > > +		return -ENOMEM;
> > > +
> > > +	tps6131x->client = client;
> > 
> > What are you planning on using client for?
> > 
> > > +	i2c_set_clientdata(client, tps6131x);
> > 
> > How are you going to _get_ this without client?
> 
> Maybe I didn't understand the question correctly, but in tps6131x_remove() I
> get the device data via the client.

Right, which uses 'client' to obtain it, so you don't need to save 'client'.

> > Why not save dev and reduce the amount of dereferencing levels required.
> 
> Absolutely. Good idea.
> 
> > 
> > > +	mutex_init(&tps6131x->lock);
> > > +	INIT_DELAYED_WORK(&tps6131x->torch_refresh_work, tps6131x_torch_refresh_handler);
> > > +
> > > +	ret = tps6131x_parse_node(tps6131x);
> > > +	if (ret)
> > > +		return -ENODEV;
> > > +
> > > +	tps6131x->regmap = devm_regmap_init_i2c(client, &tps6131x_regmap);
> > > +	if (IS_ERR(tps6131x->regmap)) {
> > > +		ret = PTR_ERR(tps6131x->regmap);
> > > +		dev_err(&client->dev, "Failed to allocate register map\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	tps6131x->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> > > +	ret = tps6131x_reset_chip(tps6131x);
> > > +	if (ret)
> > > +		return dev_err_probe(&client->dev, ret, "Failed to reset LED controller\n");
> > 
> > How do you manage the optional part?
> 
> If there is a reset line, then tps6131x_reset_chip() uses it to reset the
> chip. If there is none, the software reset (via an I2C register) is used.
> Therefore the reset pin can be optional.

Right, but didn't you just fail if one is not provided, or is that
accounted for in tps6131x_reset_chip()?

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Matthias Fend]

Hi Lee,

Am 14.03.2025 um 11:52 schrieb Lee Jones:
> On Fri, 14 Mar 2025, Matthias Fend wrote:
> 
>> Hi Lee,
>>
>> thanks a lot for your feedback!
>>
>> Am 10.03.2025 um 15:49 schrieb Lee Jones:
>>> On Fri, 28 Feb 2025, Matthias Fend wrote:
>>>
>>>> The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
>>>> stage is capable of supplying a maximum total current of roughly 1500mA.
>>>> The TPS6131x provides three constant-current sinks, capable of sinking up
>>>> to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
>>>> each sink (LED1, LED2, LED3) supports currents up to 175mA.
>>>>
>>>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>>>> ---
>>>>    MAINTAINERS                        |   7 +
>>>>    drivers/leds/flash/Kconfig         |  11 +
>>>>    drivers/leds/flash/Makefile        |   1 +
>>>>    drivers/leds/flash/leds-tps6131x.c | 798 +++++++++++++++++++++++++++++++++++++
>>>>    4 files changed, 817 insertions(+)
> 
> [...]
> 
>>>> +static int tps6131x_led_class_setup(struct tps6131x *tps6131x)
>>>> +{
>>>> +	struct led_classdev *led_cdev;
>>>> +	struct led_flash_setting *setting;
>>>> +	struct led_init_data init_data = {};
>>>> +	static const struct tps6131x_timer_config *timer_config;
>>>> +	int ret;
>>>> +
>>>> +	tps6131x->fled_cdev.ops = &flash_ops;
>>>> +
>>>> +	setting = &tps6131x->fled_cdev.timeout;
>>>> +	timer_config = tps6131x_find_closest_timer_config(0);
>>>> +	setting->min = timer_config->time_us;
>>>> +	setting->max = tps6131x->max_timeout_us;
>>>> +	setting->step = 1; /* Only some specific time periods are supported. No fixed step size. */
>>>> +	setting->val = setting->min;
>>>> +
>>>> +	setting = &tps6131x->fled_cdev.brightness;
>>>> +	setting->min = tps6131x->step_flash_current_ma;
>>>> +	setting->max = tps6131x->max_flash_current_ma;
>>>> +	setting->step = tps6131x->step_flash_current_ma;
>>>> +	setting->val = setting->min;
>>>> +
>>>> +	led_cdev = &tps6131x->fled_cdev.led_cdev;
>>>> +	led_cdev->brightness_set_blocking = tps6131x_brightness_set;
>>>> +	led_cdev->max_brightness = tps6131x->max_torch_current_ma;
>>>> +	led_cdev->flags |= LED_DEV_CAP_FLASH;
>>>> +
>>>> +	init_data.fwnode = tps6131x->led_node;
>>>> +	init_data.devicename = NULL;
>>>> +	init_data.default_label = NULL;
>>>> +	init_data.devname_mandatory = false;
>>>> +
>>>> +	ret = devm_led_classdev_flash_register_ext(&tps6131x->client->dev, &tps6131x->fled_cdev,
>>>> +						   &init_data);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +#if IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS)
>>>
>>> Not keen on #ifery in C files.
>>>
>>> Can you use is_defined() and return early instead?
>>>
>>> I see that there is a precedent for this already. :(
>>
>> Me neither, but since it is done this way in about 9 out of 10 flash
>> controllers, I wanted to continue doing it consistently.
>> But since the required v4l2_flash_* functions are also available as dummies
>> if this option is not activated, I could do it like this:
>>
>> if (!IS_BUILTIN(CONFIG_V4L2_FLASH_LED_CLASS))
>>    return 0;
>>
>> Would you prefer this solution?
> 
> I would, yes.  Thank you.

Sure, then I'll change that.

> 
>>>> +static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
>>>> +{
>>>> +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>>>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>>>> +
>>>> +	guard(mutex)(&tps6131x->lock);
>>>> +
>>> /> +	return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
>>>> +				 false);
>>>> +}
>>>> +
>>>> +static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
>>>> +	.external_strobe_set = tps6131x_flash_external_strobe_set,
>>>> +};
>>>> +
>>>> +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
>>>> +{
>>>> +	struct v4l2_flash_config v4l2_cfg = { 0 };
>>>> +	struct led_flash_setting *intensity = &v4l2_cfg.intensity;
>>>> +
>>>> +	intensity->min = tps6131x->step_torch_current_ma;
>>>> +	intensity->max = tps6131x->max_torch_current_ma;
>>>> +	intensity->step = tps6131x->step_torch_current_ma;
>>>> +	intensity->val = intensity->min;
>>>> +
>>>> +	strscpy(v4l2_cfg.dev_name, tps6131x->fled_cdev.led_cdev.dev->kobj.name,
>>>
>>> tps6131x->client->dev?
>>
>> Do you mean the name should be taken from the I2C device?
>> The current name, for example, is 'white:flash-0', while the I2C device name
>> would be '4-0033'. So I think the current version is appropriate, don't you
>> think?
> 
> No, I'm implying that:
> 
>    tps6131x->client->dev == tps6131x->fled_cdev.led_cdev.dev
> 
> ... and that the former is shorter / neater.

Hmm. That's interesting. I thought these were two different devices, 
which seems to be actually the case for me. Hence the different names in 
the kobj.
Are the devices really supposed to be identical?

> 
>>>> +		sizeof(v4l2_cfg.dev_name));
>>>> +
>>>> +	v4l2_cfg.has_external_strobe = true;
>>>> +	v4l2_cfg.flash_faults = LED_FAULT_TIMEOUT | LED_FAULT_OVER_TEMPERATURE |
>>>> +				LED_FAULT_SHORT_CIRCUIT | LED_FAULT_UNDER_VOLTAGE |
>>>> +				LED_FAULT_LED_OVER_TEMPERATURE;
>>>> +
>>>> +	tps6131x->v4l2_flash = v4l2_flash_init(&tps6131x->client->dev, tps6131x->led_node,
>>>> +					       &tps6131x->fled_cdev, &tps6131x_v4l2_flash_ops,
>>>> +					       &v4l2_cfg);
>>>> +	if (IS_ERR(tps6131x->v4l2_flash)) {
>>>> +		dev_err(&tps6131x->client->dev, "v4l2_flash_init failed\n");
>>>
>>> "Failed to initialise V4L2 flash LED" ?
>>
>> ACK
>>
>>>
>>>> +		return PTR_ERR(tps6131x->v4l2_flash);
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +#else
>>>> +
>>>> +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +#endif
>>>> +
>>>> +static int tps6131x_probe(struct i2c_client *client)
>>>> +{
>>>> +	struct tps6131x *tps6131x;
>>>> +	int ret;
>>>> +
>>>> +	tps6131x = devm_kzalloc(&client->dev, sizeof(*tps6131x), GFP_KERNEL);
>>>> +	if (!tps6131x)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	tps6131x->client = client;
>>>
>>> What are you planning on using client for?
>>>
>>>> +	i2c_set_clientdata(client, tps6131x);
>>>
>>> How are you going to _get_ this without client?
>>
>> Maybe I didn't understand the question correctly, but in tps6131x_remove() I
>> get the device data via the client.
> 
> Right, which uses 'client' to obtain it, so you don't need to save 'client'.

Okay, now I understand. Since I'm now saving 'dev' instead of 'client,' 
as you suggested, that's no longer an issue.

> 
>>> Why not save dev and reduce the amount of dereferencing levels required.
>>
>> Absolutely. Good idea.
>>
>>>
>>>> +	mutex_init(&tps6131x->lock);
>>>> +	INIT_DELAYED_WORK(&tps6131x->torch_refresh_work, tps6131x_torch_refresh_handler);
>>>> +
>>>> +	ret = tps6131x_parse_node(tps6131x);
>>>> +	if (ret)
>>>> +		return -ENODEV;
>>>> +
>>>> +	tps6131x->regmap = devm_regmap_init_i2c(client, &tps6131x_regmap);
>>>> +	if (IS_ERR(tps6131x->regmap)) {
>>>> +		ret = PTR_ERR(tps6131x->regmap);
>>>> +		dev_err(&client->dev, "Failed to allocate register map\n");
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	tps6131x->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
>>>> +	ret = tps6131x_reset_chip(tps6131x);
>>>> +	if (ret)
>>>> +		return dev_err_probe(&client->dev, ret, "Failed to reset LED controller\n");
>>>
>>> How do you manage the optional part?
>>
>> If there is a reset line, then tps6131x_reset_chip() uses it to reset the
>> chip. If there is none, the software reset (via an I2C register) is used.
>> Therefore the reset pin can be optional.
> 
> Right, but didn't you just fail if one is not provided, or is that
> accounted for in tps6131x_reset_chip()?

Yes, tps6131x_reset_chip() selects the appropriate reset method 
depending on the circumstances. Something like this:

if (IS_ERR_OR_NULL(tps6131x->reset_gpio))
   sofware reset via i2c
else
   hardware reset via gpio

So there's no error just because the reset GPIO isn't present.

Thanks
  ~Matthias

> 

Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Lee Jones]

On Fri, 14 Mar 2025, Matthias Fend wrote:

> Hi Lee,
> 
> Am 14.03.2025 um 11:52 schrieb Lee Jones:
> > On Fri, 14 Mar 2025, Matthias Fend wrote:
> > 
> > > Hi Lee,
> > > 
> > > thanks a lot for your feedback!
> > > 
> > > Am 10.03.2025 um 15:49 schrieb Lee Jones:
> > > > On Fri, 28 Feb 2025, Matthias Fend wrote:
> > > > 
> > > > > The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
> > > > > stage is capable of supplying a maximum total current of roughly 1500mA.
> > > > > The TPS6131x provides three constant-current sinks, capable of sinking up
> > > > > to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
> > > > > each sink (LED1, LED2, LED3) supports currents up to 175mA.
> > > > > 
> > > > > Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> > > > > ---
> > > > >    MAINTAINERS                        |   7 +
> > > > >    drivers/leds/flash/Kconfig         |  11 +
> > > > >    drivers/leds/flash/Makefile        |   1 +
> > > > >    drivers/leds/flash/leds-tps6131x.c | 798 +++++++++++++++++++++++++++++++++++++
> > > > >    4 files changed, 817 insertions(+)

[...]

> > > > > +static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
> > > > > +{
> > > > > +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > > > > +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> > > > > +
> > > > > +	guard(mutex)(&tps6131x->lock);
> > > > > +
> > > > /> +	return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
> > > > > +				 false);
> > > > > +}
> > > > > +
> > > > > +static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
> > > > > +	.external_strobe_set = tps6131x_flash_external_strobe_set,
> > > > > +};
> > > > > +
> > > > > +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
> > > > > +{
> > > > > +	struct v4l2_flash_config v4l2_cfg = { 0 };
> > > > > +	struct led_flash_setting *intensity = &v4l2_cfg.intensity;
> > > > > +
> > > > > +	intensity->min = tps6131x->step_torch_current_ma;
> > > > > +	intensity->max = tps6131x->max_torch_current_ma;
> > > > > +	intensity->step = tps6131x->step_torch_current_ma;
> > > > > +	intensity->val = intensity->min;
> > > > > +
> > > > > +	strscpy(v4l2_cfg.dev_name, tps6131x->fled_cdev.led_cdev.dev->kobj.name,
> > > > 
> > > > tps6131x->client->dev?
> > > 
> > > Do you mean the name should be taken from the I2C device?
> > > The current name, for example, is 'white:flash-0', while the I2C device name
> > > would be '4-0033'. So I think the current version is appropriate, don't you
> > > think?
> > 
> > No, I'm implying that:
> > 
> >    tps6131x->client->dev == tps6131x->fled_cdev.led_cdev.dev
> > 
> > ... and that the former is shorter / neater.
> 
> Hmm. That's interesting. I thought these were two different devices, which
> seems to be actually the case for me. Hence the different names in the kobj.
> Are the devices really supposed to be identical?

Interesting.  What are their names?

-- 
Lee Jones [李琼斯]

Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Matthias Fend]

Hi Lee,

Am 14.03.2025 um 12:45 schrieb Lee Jones:
> On Fri, 14 Mar 2025, Matthias Fend wrote:
> 
>> Hi Lee,
>>
>> Am 14.03.2025 um 11:52 schrieb Lee Jones:
>>> On Fri, 14 Mar 2025, Matthias Fend wrote:
>>>
>>>> Hi Lee,
>>>>
>>>> thanks a lot for your feedback!
>>>>
>>>> Am 10.03.2025 um 15:49 schrieb Lee Jones:
>>>>> On Fri, 28 Feb 2025, Matthias Fend wrote:
>>>>>
>>>>>> The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
>>>>>> stage is capable of supplying a maximum total current of roughly 1500mA.
>>>>>> The TPS6131x provides three constant-current sinks, capable of sinking up
>>>>>> to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
>>>>>> each sink (LED1, LED2, LED3) supports currents up to 175mA.
>>>>>>
>>>>>> Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
>>>>>> ---
>>>>>>     MAINTAINERS                        |   7 +
>>>>>>     drivers/leds/flash/Kconfig         |  11 +
>>>>>>     drivers/leds/flash/Makefile        |   1 +
>>>>>>     drivers/leds/flash/leds-tps6131x.c | 798 +++++++++++++++++++++++++++++++++++++
>>>>>>     4 files changed, 817 insertions(+)
> 
> [...]
> 
>>>>>> +static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
>>>>>> +{
>>>>>> +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
>>>>>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>>>>>> +
>>>>>> +	guard(mutex)(&tps6131x->lock);
>>>>>> +
>>>>> /> +	return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
>>>>>> +				 false);
>>>>>> +}
>>>>>> +
>>>>>> +static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
>>>>>> +	.external_strobe_set = tps6131x_flash_external_strobe_set,
>>>>>> +};
>>>>>> +
>>>>>> +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
>>>>>> +{
>>>>>> +	struct v4l2_flash_config v4l2_cfg = { 0 };
>>>>>> +	struct led_flash_setting *intensity = &v4l2_cfg.intensity;
>>>>>> +
>>>>>> +	intensity->min = tps6131x->step_torch_current_ma;
>>>>>> +	intensity->max = tps6131x->max_torch_current_ma;
>>>>>> +	intensity->step = tps6131x->step_torch_current_ma;
>>>>>> +	intensity->val = intensity->min;
>>>>>> +
>>>>>> +	strscpy(v4l2_cfg.dev_name, tps6131x->fled_cdev.led_cdev.dev->kobj.name,
>>>>>
>>>>> tps6131x->client->dev?
>>>>
>>>> Do you mean the name should be taken from the I2C device?
>>>> The current name, for example, is 'white:flash-0', while the I2C device name
>>>> would be '4-0033'. So I think the current version is appropriate, don't you
>>>> think?
>>>
>>> No, I'm implying that:
>>>
>>>     tps6131x->client->dev == tps6131x->fled_cdev.led_cdev.dev
>>>
>>> ... and that the former is shorter / neater.
>>
>> Hmm. That's interesting. I thought these were two different devices, which
>> seems to be actually the case for me. Hence the different names in the kobj.
>> Are the devices really supposed to be identical?
> 
> Interesting.  What are their names?

tps6131x->fled_cdev.led_cdev.dev: 'white:flash-0'
tps6131x->client->dev: '4-0033'

Best regards
  ~Matthias


> 

Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments TPS6131X flash LED driver

[Lee Jones]

On Fri, 14 Mar 2025, Matthias Fend wrote:

> Hi Lee,
> 
> Am 14.03.2025 um 12:45 schrieb Lee Jones:
> > On Fri, 14 Mar 2025, Matthias Fend wrote:
> > 
> > > Hi Lee,
> > > 
> > > Am 14.03.2025 um 11:52 schrieb Lee Jones:
> > > > On Fri, 14 Mar 2025, Matthias Fend wrote:
> > > > 
> > > > > Hi Lee,
> > > > > 
> > > > > thanks a lot for your feedback!
> > > > > 
> > > > > Am 10.03.2025 um 15:49 schrieb Lee Jones:
> > > > > > On Fri, 28 Feb 2025, Matthias Fend wrote:
> > > > > > 
> > > > > > > The TPS61310/TPS61311 is a flash LED driver with I2C interface. Its power
> > > > > > > stage is capable of supplying a maximum total current of roughly 1500mA.
> > > > > > > The TPS6131x provides three constant-current sinks, capable of sinking up
> > > > > > > to 2 × 400mA (LED1 and LED3) and 800mA (LED2) in flash mode. In torch mode
> > > > > > > each sink (LED1, LED2, LED3) supports currents up to 175mA.
> > > > > > > 
> > > > > > > Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
> > > > > > > ---
> > > > > > >     MAINTAINERS                        |   7 +
> > > > > > >     drivers/leds/flash/Kconfig         |  11 +
> > > > > > >     drivers/leds/flash/Makefile        |   1 +
> > > > > > >     drivers/leds/flash/leds-tps6131x.c | 798 +++++++++++++++++++++++++++++++++++++
> > > > > > >     4 files changed, 817 insertions(+)
> > 
> > [...]
> > 
> > > > > > > +static int tps6131x_flash_external_strobe_set(struct v4l2_flash *v4l2_flash, bool enable)
> > > > > > > +{
> > > > > > > +	struct led_classdev_flash *fled_cdev = v4l2_flash->fled_cdev;
> > > > > > > +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
> > > > > > > +
> > > > > > > +	guard(mutex)(&tps6131x->lock);
> > > > > > > +
> > > > > > /> +	return tps6131x_set_mode(tps6131x, enable ? TPS6131X_MODE_FLASH : TPS6131X_MODE_SHUTDOWN,
> > > > > > > +				 false);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static const struct v4l2_flash_ops tps6131x_v4l2_flash_ops = {
> > > > > > > +	.external_strobe_set = tps6131x_flash_external_strobe_set,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static int tps6131x_v4l2_setup(struct tps6131x *tps6131x)
> > > > > > > +{
> > > > > > > +	struct v4l2_flash_config v4l2_cfg = { 0 };
> > > > > > > +	struct led_flash_setting *intensity = &v4l2_cfg.intensity;
> > > > > > > +
> > > > > > > +	intensity->min = tps6131x->step_torch_current_ma;
> > > > > > > +	intensity->max = tps6131x->max_torch_current_ma;
> > > > > > > +	intensity->step = tps6131x->step_torch_current_ma;
> > > > > > > +	intensity->val = intensity->min;
> > > > > > > +
> > > > > > > +	strscpy(v4l2_cfg.dev_name, tps6131x->fled_cdev.led_cdev.dev->kobj.name,
> > > > > > 
> > > > > > tps6131x->client->dev?
> > > > > 
> > > > > Do you mean the name should be taken from the I2C device?
> > > > > The current name, for example, is 'white:flash-0', while the I2C device name
> > > > > would be '4-0033'. So I think the current version is appropriate, don't you
> > > > > think?
> > > > 
> > > > No, I'm implying that:
> > > > 
> > > >     tps6131x->client->dev == tps6131x->fled_cdev.led_cdev.dev
> > > > 
> > > > ... and that the former is shorter / neater.
> > > 
> > > Hmm. That's interesting. I thought these were two different devices, which
> > > seems to be actually the case for me. Hence the different names in the kobj.
> > > Are the devices really supposed to be identical?
> > 
> > Interesting.  What are their names?
> 
> tps6131x->fled_cdev.led_cdev.dev: 'white:flash-0'
> tps6131x->client->dev: '4-0033'

Presumably this is an I2C address?  What a helpful device name!

Okay, seeing as they are both clearly different, I think you are
correct.  Scrap this review comment.

-- 
Lee Jones [李琼斯]