Message-ID: <def0351b-c037-47c8-b395-d64cfca7ae25@emfend.at>
Date: Fri, 14 Mar 2025 09:28:09 +0100
From: Matthias Fend <matthias.fend@...end.at>
To: Lee Jones <lee@...nel.org>
Cc: Pavel Machek <pavel@....cz>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-leds@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 bsp-development.geo@...ca-geosystems.com
Subject: Re: [PATCH 2/2] leds: tps6131x: add support for Texas Instruments
 TPS6131X flash LED driver

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@...end.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@...end.at>
>> +L:	linux-leds@...r.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@...nel.org>
>>   L:	linux-iio@...r.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@...end.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@...end.at>");
>> +MODULE_LICENSE("GPL");
>>
>> -- 
>> 2.34.1
>>
> 

Powered by blists - more mailing lists