| 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
| ||
|
Message-ID: <10680b25-2953-4dbb-9ff1-362bcf0f84cc@timmermann.space>
Date: Fri, 13 Jun 2025 13:34:19 +0200
From: Lukas Timmermann <linux@...mermann.space>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>, 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
Hi,
okay thanks, I will wait a week or two until sending the next patch.
I've never worked with this email-based workflow before. That's the
reason I've chosen this simple driver as a first patch in the first
place. Just to get a little bit of experience under my belt. I know I'm
making a few very obvious mistakes. I'm getting a bit overwhelmed by all
the things to remember, I guess.
Apologies for the noise.
Best regards,
Lukas Timmermann
Am 12.06.25 um 22:27 schrieb Christophe JAILLET:
> 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", ®);
>> + 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
>
Powered by blists - more mailing lists