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

From: Krzysztof Kozlowski
Date: Wed Mar 19 2025 - 05:03:42 EST

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().

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

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

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

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

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

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

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

You should handle IS_ERR


Best regards,
Krzysztof