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

[Lukas Timmermann]

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