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

[Lukas Timmermann <linux@timmermann.space>]

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@timmermann.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
>