Re: [PATCH v6 2/2] leds: ltc3208: Add driver for LTC3208 Multidisplay LED Driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The regmap configuration lacks `.reg_defaults`, which will cause `regmap_field_write()` to attempt a hardware read for shared registers. This can fail entirely on write-only I2C devices.
- [Medium] The driver unconditionally overwrites the CPO (Charge Pump Oscillator) mode bits in the options register, destroying bootloader state.
- [Low] Minor code quality defects: format string mismatches, dead code, and typos.
--

commit 347e80a37d34f3436d81f8c496280febd37eb8a5
Author: Jan Carlo Roleda <jancarlo.roleda@analog.com>

leds: ltc3208: Add driver for LTC3208 Multidisplay LED Driver

Kernel driver implementation for the LTC3208 Multidisplay LED Driver.
The LTC3208 controls up to 7 distinct LED channels and is configured via I2C.

>     The CAM LED channel is configured as 2 separate CAMHI and CAMLO register
>     sub-channels, which currnet is selected via the CAMHL pin, or set to
>     CAMHI register only via setting the S_CAMHILO bit high in register G (0x7).

[Severity: Low]
This isn't a bug, but there's a typo in the commit message: 'currnet' instead
of 'current'.

> diff --git a/drivers/leds/leds-ltc3208.c b/drivers/leds/leds-ltc3208.c
> new file mode 100644
> index 0000000000000..3d6bec99e9ea9
> --- /dev/null
> +++ b/drivers/leds/leds-ltc3208.c
> @@ -0,0 +1,244 @@
[ ... ]
> +#define LTC3208_REG_F_CAM 0x6 /* CAM (High and Low) current DAC*/
> +#define LTC3208_REG_G_OPT 0x7 /* Device Options */
> +#define  LTC3208_OPT_CPO_MASK GENMASK(7, 6)

[Severity: Low]
Is this macro needed? It appears to be defined but never used in the driver.
The same applies to LTC3208_MAX_CPO_OPT defined later.

[ ... ]
> +struct ltc3208_led {
> +	struct led_classdev cdev;
> +	struct i2c_client *client;
> +	struct regmap_field *rfield;
> +	enum ltc3208_channel channel;
> +};
> +
> +struct ltc3208 {
> +	struct ltc3208_led leds[LTC3208_NUM_LED_GRPS];
> +	struct regmap *regmap;
> +};

[Severity: Low]
Is the channel member in struct ltc3208_led used anywhere? It gets assigned
during probe but doesn't seem to be read later.

Similarly, the regmap pointer in struct ltc3208 appears to only be used
locally in ltc3208_probe() and isn't needed in the struct.

[ ... ]
> +static const struct regmap_config ltc3208_regmap_cfg = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = LTC3208_REG_G_OPT,
> +	.cache_type = REGCACHE_FLAT_S,
> +};

[Severity: High]
Does this regmap configuration need a .reg_defaults array to seed the cache?

Since .reg_defaults is missing, the cache won't be seeded. Later, when
ltc3208_led_set_brightness() calls regmap_field_write() for shared registers
(like LTC3208_REG_A_GRNRED), it will trigger a read-modify-write operation
that issues an I2C read.

If the hardware is write-only, the read will fail and break LED functionality.
If it supports reads, this still introduces unnecessary I2C read latency on a
fast path.

[ ... ]
> +static int ltc3208_probe(struct i2c_client *client)
> +{
[ ... ]
> +	reg_val = FIELD_PREP(LTC3208_OPT_EN_RGBS, set_sub_control_pin) |
> +		  FIELD_PREP(LTC3208_OPT_DIS_CAMHILO, disable_camhl_pin) |
> +		  FIELD_PREP(LTC3208_OPT_DIS_RGBDROP,
> +			     disable_rgb_aux4_dropout_signal);
> +
> +	ret = regmap_write(regmap, LTC3208_REG_G_OPT, reg_val);

[Severity: Medium]
Does this unconditional write wipe out the existing CPO bits (bits 7:6)?

By using regmap_write() here, any initial hardware configuration done by
the bootloader or hardware defaults (like Charge Pump mode) might be permanently
erased. Would regmap_update_bits() be safer here to apply only the
explicitly configured fields?

[ ... ]
> +		ret = fwnode_property_read_u32(child, "reg", &chan);
> +		if (ret)
> +			return dev_err_probe(&client->dev, ret,
> +					    "Failed to get reg value of LED\n");
> +		else if (chan >= LTC3208_NUM_LED_GRPS)
> +			return dev_err_probe(&client->dev, -EINVAL,
> +					     "%d is an invalid LED ID\n", chan);

[Severity: Low]
Could this dev_err_probe() use %u instead of %d?

The chan variable is an unsigned u32, so %d produces a format string mismatch.

> +		else if (ddata->leds[chan].client)
> +			return dev_err_probe(&client->dev, -EINVAL,
> +					"%d is already registered\n", chan);
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623-upstream-ltc3208-v6-0-fc6bd9749166@analog.com?part=2