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

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

[-- Attachment #1: Type: text/plain, Size: 4568 bytes --]

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");


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

> +	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" },
	{ }
};

> +MODULE_DEVICE_TABLE(i2c, ltc3208_idtable);

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]