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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5e131f07-9753-4d2f-a043-35751c278a63@wanadoo.fr>
Date: Thu, 12 Jun 2025 22:27:30 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Lukas Timmermann <linux@...mermann.space>, lee@...nel.org,
 pavel@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Cc: linux-leds@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/2] leds: as3668: Driver for the ams Osram 4-channel
 i2c LED driver

Le 11/06/2025 à 10:31, 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.
> 
> Signed-off-by: Lukas Timmermann <linux@...mermann.space>

Hi,

first, I should that you should wait longer before sending each new 
version, so that you can collect more feedback.

> ---
>   MAINTAINERS                |   1 +
>   drivers/leds/Kconfig       |  13 +++
>   drivers/leds/Makefile      |   1 +
>   drivers/leds/leds-as3668.c | 204 +++++++++++++++++++++++++++++++++++++
>   4 files changed, 219 insertions(+)
>   create mode 100644 drivers/leds/leds-as3668.c

...

> +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;
> +
> +	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\n", err);

as3668_dt_init() is only called from the probe. Sometimes maintainers 
prefer using "return dev_err_probe()" in such a case, to have less 
verbose code.
(I don't know if it is the case for the leds subsystem)

> +			return err;
> +		}
> +
> +		if (reg < 0 || reg > AS3668_MAX_LEDS) {
> +			dev_err(dev, "unsupported led reg %d\n", reg);
> +			return -EOPNOTSUPP;

Same.

> +		}
> +
> +		led = &as3668->leds[reg];
> +		led->fwnode = of_fwnode_handle(child);
> +
> +		led->num = reg;
> +		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", reg);
> +			return err;

Same.

> +		}
> +	}
> +
> +	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 */
> +	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, sizeof(*as3668), GFP_KERNEL);
> +

Unneeded new line.

> +	if (!as3668)
> +		return -ENOMEM;
> +
> +	as3668->client = client;
> +	int err = as3668_dt_init(as3668);

Would be better, IMHO, if err was declared at the top of the function.

> +

Unneeded new line.

> +	if (err) {
> +		dev_err(&client->dev, "failed to initialize device, err %d\n", err);

return dev_err_probe() to be consistent with the code above.

> +		return err;
> +	}
> +
> +	/* 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