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

From: Matthias Fend
Date: Fri Mar 14 2025 - 07:28:02 EST

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@xxxxxxxxx>
---
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