RE: [PATCH v5 2/2] leds: ltc3208: Add driver for LTC3208 Multidisplay LED Driver

[Roleda, Jan carlo]

Hello Kleine-Konig,

> -----Original Message-----
> From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxx>
> Sent: Friday, June 19, 2026 9:49 PM
> To: Roleda, Jan carlo <Jancarlo.Roleda@xxxxxxxxxx>
> Cc: Lee Jones <lee@xxxxxxxxxx>; Pavel Machek <pavel@xxxxxxxxxx>; Rob
> Herring <robh@xxxxxxxxxx>; Krzysztof Kozlowski <krzk+dt@xxxxxxxxxx>; Conor
> Dooley <conor+dt@xxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx; linux-
> leds@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 2/2] leds: ltc3208: Add driver for LTC3208 Multidisplay
> LED Driver
> 
> [External]
> 
> On Fri, Jun 19, 2026 at 06:45:09AM +0800, Jan Carlo Roleda wrote:
> > [...]
> > +static int ltc3208_probe(struct i2c_client *client) {
> > +	enum ltc3208_aux_channel
> aux_channels[LTC3208_NUM_AUX_LEDS];
> > +	struct ltc3208 *ddata;
> > +	struct regmap *regmap;
> > +	bool disable_rgb_aux4_dropout_signal;
> > +	bool disable_camhl_pin;
> > +	bool set_sub_control_pin;
> > +	int ret;
> > +	u8 reg_val;
> > +
> > +	regmap = devm_regmap_init_i2c(client, &ltc3208_regmap_cfg);
> > +	if (IS_ERR(regmap))
> > +		return dev_err_probe(&client->dev, PTR_ERR(regmap),
> > +				     "Failed to initialize regmap\n");
> > +
> > +	ddata = devm_kzalloc(&client->dev, sizeof(*ddata), GFP_KERNEL);
> > +	if (!ddata)
> > +		return -ENOMEM;
> > +
> > +	ddata->regmap = regmap;
> > +
> > +	disable_camhl_pin = device_property_read_bool(&client->dev,
> > +						      "adi,disable-camhl-pin");
> > +	set_sub_control_pin =
> > +		device_property_read_bool(&client->dev, "adi,cfg-enrgbs-
> pin");
> > +	disable_rgb_aux4_dropout_signal = device_property_read_bool(
> > +		&client->dev, "adi,disable-rgb-aux4-dropout");
> 
> Unusual line break. I'd write that as:
> 
> 	disable_rgb_aux4_dropout_signal =
> 		device_property_read_bool(&client->dev,
> 					  "adi,disable-rgb-aux4-dropout");
> 
> 

This is noted. I will rewrite this part accordingly.

> > +
> > +	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);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "error writing to options register\n");
> > +
> > +	/* Initialize aux channel configurations */
> > +	for (int i = 0; i < LTC3208_NUM_AUX_LEDS; i++) {
> > +		ret = device_property_match_property_string(
> > +			&client->dev, ltc3208_dt_aux_channels[i],
> > +			ltc3208_aux_opt, LTC3208_NUM_AUX_OPT);
> > +		/* Fallback to default value (AUX) if not found */
> > +		if (ret == -EINVAL)
> > +			aux_channels[i] = LTC3208_AUX_CHAN_AUX;
> > +		else if (ret >= 0)
> > +			aux_channels[i] = ret;
> > +	}
> > +
> > +	reg_val = FIELD_PREP(LTC3208_AUX1_MASK, aux_channels[0]) |
> > +		  FIELD_PREP(LTC3208_AUX2_MASK, aux_channels[1]) |
> > +		  FIELD_PREP(LTC3208_AUX3_MASK, aux_channels[2]) |
> > +		  FIELD_PREP(LTC3208_AUX4_MASK, aux_channels[3]);
> > +
> > +	ret = regmap_write(regmap, LTC3208_REG_E_AUX_SELECT, reg_val);
> > +	if (ret)
> > +		return dev_err_probe(&client->dev, ret,
> > +			"error writing to aux channel register.\n");
> > +
> > +	i2c_set_clientdata(client, ddata);
> 
> From a quick glance, this is unused.
> 

You're right, I'll remove this then.

> > +	device_for_each_child_node_scoped(&client->dev, child) {
> > +		struct ltc3208_led *led;
> > +		struct led_init_data init_data = {};
> > +		u32 chan;
> > +
> > +		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, ret,
> > +					     "%d is an invalid LED ID\n", chan);
> > +
> > +		led = &ddata->leds[chan];
> > +
> > +		led->rfield =
> > +			devm_regmap_field_alloc(&client->dev, ddata-
> >regmap,
> > +						ltc3208_led_reg_field[chan]);
> > +		if (IS_ERR(led->rfield))
> > +			return dev_err_probe(&client->dev, PTR_ERR(led-
> >rfield),
> > +					     "cannot allocate regmap field\n");
> > +		led->client = client;
> > +		led->channel = chan;
> > +		led->cdev.brightness_set_blocking =
> ltc3208_led_set_brightness;
> > +		led->cdev.max_brightness =
> LTC3208_MAX_BRIGHTNESS_4BIT;
> > +
> > +		if (chan == LTC3208_CHAN_MAIN || chan ==
> LTC3208_CHAN_SUB)
> > +			led->cdev.max_brightness =
> LTC3208_MAX_BRIGHTNESS_8BIT;
> > +
> > +		init_data.fwnode = child;
> > +
> > +		ret = devm_led_classdev_register_ext(&client->dev, &led-
> >cdev,
> > +						     &init_data);
> > +		if (ret)
> > +			return dev_err_probe(&client->dev, ret,
> > +					     "LED %u Register failed.\n", chan);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ltc3208_match_table[] = {
> > +	{.compatible = "adi,ltc3208"},
> > +	{}
> > +};
> 
> Please make this:
> 
> static const struct of_device_id ltc3208_match_table[] = {
> 	{ .compatible = "adi,ltc3208" },
> 	{ }
> };
> 
> 
> > +MODULE_DEVICE_TABLE(of, ltc3208_match_table);
> > +
> > +static const struct i2c_device_id ltc3208_idtable[] = {
> > +	{ "ltc3208" },
> > +	{}
> > +};
> 
> Please make this:
> 
> static const struct i2c_device_id ltc3208_idtable[] = {
> 	{ .name = "ltc3208" },
> 	{ }
> };
> 

Noted. Thank you for the clarification on the spacing.


> > +MODULE_DEVICE_TABLE(i2c, ltc3208_idtable);
> 
> Best regards
> Uwe

I will submit a new patch with these suggestions and those identified by Sashiko by June 24.

Thank you for the review!

Regards,
Carlo