Re: [PATCH v5 2/2] leds: as3668: Driver for the ams Osram 4-channel i2c LED driver

[Christophe JAILLET]

Le 09/06/2025 à 01:18, Lukas Timmermann a écrit :
> Since there were no existing drivers for the AS3668 or related devices,
> a new driver was introduced in a separate file. Similar devices were
> reviewed, but none shared enough characteristics to justify code reuse.
> As a result, this driver is written specifically for the AS3668.

Hi

...

> +static int as3668_write_value(struct i2c_client *client, u8 reg, u8 value)
> +{
> +	int err = i2c_smbus_write_byte_data(client, reg, value);
> +
> +	if (err)
> +		dev_err(&client->dev, "error writing to reg 0x%02x, returned %d", reg, err);

Missing \n.

> +
> +	return err;
> +}

...

> +static int as3668_dt_init(struct as3668 *as3668)
> +{
> +	struct device *dev = &as3668->client->dev;
> +	struct as3668_led *led;
> +	struct led_init_data init_data = {};
> +	int err;
> +	u32 reg;
> +	int i = 0;

Unneeded init.

> +
> +	for_each_available_child_of_node_scoped(dev_of_node(dev), child) {
> +		err = of_property_read_u32(child, "reg", &reg);
> +		if (err)
> +			dev_err(dev, "unable to read device tree led reg, err %d", err);

Missing \n.

In this case, we still go on? This looks strange.
What is the value of 'reg' in the later code?

> +
> +		i = reg;
> +
> +		if (i < 0 || i > AS3668_MAX_LEDS) {
> +			dev_err(dev, "unsupported led reg %d\n", i);
> +			return -EOPNOTSUPP;
> +		}
> +
> +		led = &as3668->leds[i];
> +		led->fwnode = of_fwnode_handle(child);
> +
> +		led->num = i;
> +		led->chip = as3668;
> +
> +		led->cdev.max_brightness = U8_MAX;
> +		led->cdev.brightness_get = as3668_brightness_get;
> +		led->cdev.brightness_set = as3668_brightness_set;
> +
> +		init_data.fwnode = led->fwnode;
> +		init_data.default_label = ":";
> +
> +		err = devm_led_classdev_register_ext(dev, &led->cdev, &init_data);
> +		if (err) {
> +			dev_err(dev, "failed to register %d LED\n", i);
> +			return err;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int as3668_probe(struct i2c_client *client)
> +{
> +	u8 chip_id1, chip_id2, chip_serial, chip_rev;
> +	struct as3668 *as3668;
> +
> +	/* Check for sensible i2c address */
> +	if (client->addr != 0x42)
> +		return dev_err_probe(&client->dev, -EFAULT,
> +				     "unexpected address for as3668 device\n");
> +
> +	/* Read identifier from chip */
> +	chip_id1 = as3668_read_value(client, AS3668_CHIP_ID1);
> +
> +	if (chip_id1 != AS3668_CHIP_IDENT)
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				"chip reported wrong id: 0x%02x\n", chip_id1);
> +
> +	/* Check the revision*/

Missing space before */

> +	chip_id2 = as3668_read_value(client, AS3668_CHIP_ID2);
> +	chip_serial = FIELD_GET(AS3668_CHIP_ID2_SERIAL_MASK, chip_id2);
> +	chip_rev = FIELD_GET(AS3668_CHIP_ID2_REV_MASK, chip_id2);
> +
> +	if (chip_rev != AS3668_CHIP_REV1)
> +		dev_warn(&client->dev, "unexpected chip revision\n");
> +
> +	/* Print out information about the chip */
> +	dev_dbg(&client->dev,
> +		"chip_id: 0x%02x | chip_id2: 0x%02x | chip_serial: 0x%02x | chip_rev: 0x%02x\n",
> +		chip_id1, chip_id2, chip_serial, chip_rev);
> +
> +	as3668 = devm_kzalloc(&client->dev, struct_size(as3668, leds, AS3668_MAX_LEDS), GFP_KERNEL);

Why using struct_size()?
as3668 has no flexible array at its end.

Also error handling is missing if the allocation fails.


> +	as3668->client = client;
> +
> +	as3668_dt_init(as3668);

as3668_dt_init() may return an error.
Missing error handling?

> +
> +	/* Initialize the chip */
> +	as3668_write_value(client, AS3668_CURRX_CONTROL, 0x55);
> +	as3668_write_value(client, AS3668_CURR1, 0x00);
> +	as3668_write_value(client, AS3668_CURR2, 0x00);
> +	as3668_write_value(client, AS3668_CURR3, 0x00);
> +	as3668_write_value(client, AS3668_CURR4, 0x00);
> +
> +	return 0;
> +}

...

CJ