Message-ID: <9a470dfd-8d7b-4529-b54b-289754b9eed6@emfend.at>
Date: Wed, 19 Mar 2025 17:25:18 +0100
From: Matthias Fend <matthias.fend@...end.at>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Pavel Machek <pavel@....cz>, Lee Jones <lee@...nel.org>,
 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 v2 2/2] leds: tps6131x: add support for Texas Instruments
 TPS6131X flash LED driver

Hi Krzysztof,

thank you very much for your review!

Am 19.03.2025 um 10:03 schrieb Krzysztof Kozlowski:
> On Tue, Mar 18, 2025 at 08:28:58AM +0100, Matthias Fend wrote:
>> +
>> +static int tps6131x_reset_chip(struct tps6131x *tps6131x)
>> +{
>> +	int ret;
>> +
>> +	if (IS_ERR_OR_NULL(tps6131x->reset_gpio)) {
> 
> No, only check for non-null, see comment in probe().

ACK

> 
>> +		ret = regmap_update_bits(tps6131x->regmap, TPS6131X_REG_0, TPS6131X_REG_0_RESET,
>> +					 TPS6131X_REG_0_RESET);
>> +		if (ret)
>> +			return ret;
>> +
>> +		fsleep(100);
>> +
>> +		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);
>> +		fsleep(100);
>> +	}
>> +
>> +	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;
>> +}
>> +
>> +static int tps6131x_set_mode(struct tps6131x *tps6131x, enum tps6131x_mode mode, bool force)
>> +{
>> +	u8 val;
>> +
>> +	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);
>> +	u32 num_chans;
>> +	u32 steps_chan13, steps_chan2;
>> +	u32 steps_remaining;
>> +	u8 reg0;
>> +	int ret;
>> +
>> +	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;
>> +
>> +	num_chans = tps6131x->chan1_en + tps6131x->chan2_en + tps6131x->chan3_en;
>> +
>> +	/*
>> +	 * 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);
> 
> Looks like guard should start here... or you are not synchronizing
> hardware access but more.

I just found it clearer to have the lock at the beginning.
But of course, I can easily move the lock to this position.

> 
>> +	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 num_chans;
>> +	u32 steps_chan13, steps_chan2;
>> +	u32 steps_remaining;
>> +	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);
>> +
> 
> Same here

ACK

> 
>> +	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)
>> +{
>> +	const struct tps6131x_timer_config *timer_config;
>> +	struct tps6131x *tps6131x = fled_cdev_to_tps6131x(fled_cdev);
>> +	u8 reg3;
>> +	int ret;
>> +
>> +	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;
>> +
> 
> Why no lock here?

The individual regmap operations are already protected by the internal 
lock. If reading from a register has no other problematic effects on the 
overall function, then no additional lock is needed.

I noticed that the lock in tps6131x_strobe_get() is no longer needed. 
I'll removed that as well.

> 
>> +	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);
> 
> And this is not locked?

The read modify write operation is protected by regmap. Since this 
operation does not interact with any other functions, no lock is needed 
here.

> 
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
> 
> ...
> 
>> +
>> +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;
>> +
>> +	if (!IS_BUILTIN(CONFIG_V4L2_FLASH_LED_CLASS))
> 
> Why builtin? That's a tristate, so I don't get why driver and v4l flash
> cannot be modules. You wanted REACHABLE probably... but then it is
> anyway discouraged practice leading to runtime debugging. So actually
> you want CONFIG_V4L2_FLASH_LED_CLASS || !CONFIG_V4L2_FLASH_LED_CLASS
> dependency.

Okay, I'll add 'depends on V4L2_FLASH_LED_CLASS || 
!V4L2_FLASH_LED_CLASS' to the Kconfig entry and do the check in the 
driver like this:
   if (!IS_ENABLED(CONFIG_V4L2_FLASH_LED_CLASS))
     return 0;

Is this solution okay for you?

> 
> See Kconfig description.
> 
>> +		return 0;
>> +
>> +	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->dev, tps6131x->led_node,
>> +					       &tps6131x->fled_cdev, &tps6131x_v4l2_flash_ops,
>> +					       &v4l2_cfg);
>> +	if (IS_ERR(tps6131x->v4l2_flash)) {
>> +		dev_err(tps6131x->dev, "Failed to initialize v4l2 flash LED\n");
>> +		return PTR_ERR(tps6131x->v4l2_flash);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +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->dev = &client->dev;
>> +	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;
> 
> Syntax is:
> 
> return dev_err_probe

Of course. ACK

> 
>> +	}
>> +
>> +	tps6131x->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> 
> You should handle IS_ERR

ACK

Thanks!
  ~Matthias

> 
> 
> Best regards,
> Krzysztof
> 

Powered by blists - more mailing lists