Re: [PATCH 2/2] iio: light: veml3328: add support for new device

[Joshua Crofts]

On Sun, 17 May 2026 at 09:53, Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> wrote:
>
> On Sat, May 16, 2026 at 11:50:54PM +0200, Joshua Crofts wrote:
> > Add support for the Vishay VEML3328 RGB/IR light sensor communicating
> > via I2C (SMBus compatible).
> >
> > Also add a new entry for said driver into Kconfig and Makefile.
>
> ...
>
> IWYU.
>
> + array_size.h

Ah, missed that one.

> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
>
> Most likely + types.h.
>
> ...
>
> > +struct veml3328_data {
> > +     struct regmap *regmap;
> > +     struct device *dev;
>
> Dup? We may derive one from the other (in case if regmap is registered with
> the same dev, otherwise needs a good comment explaining why not).
>
> > +     struct mutex lock;
> > +};

Okay, regmap is registered with the same device.

> ...
>
> > +/* integration times in microseconds */
> > +static const int veml3328_it_times[][2] = {
> > +     { 0, 50000 },
> > +     { 0, 100000 },
> > +     { 0, 200000 },
> > +     { 0, 400000 },
>
> USEC_PER_MSEC in all of them.

Good catch.

> > +};
>
> ...
>
> > +     return regmap_update_bits(data->regmap, VEML3328_REG_CONF,
> > +                               VEML3328_SHUTDOWN, VEML3328_SHUTDOWN);
>
> Here and everywhere else, use regmap_set_bits()/regmap_clear_bits() when
> appropriate.

No problem.

> ...
>
> > +static int veml3328_power_up(struct veml3328_data *data)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_update_bits(data->regmap, VEML3328_REG_CONF,
> > +                              VEML3328_SHUTDOWN, 0);
> > +     if (ret < 0)
> > +             return ret;
>
> > +     fsleep(veml3328_it_times[3][1]);
>
> This is not good. Why do we have a table from which we are using only one
> value? Define it properly and use here.

My bad, will use a macro instead.

> > +     return 0;
> > +}
>
> ...
>
> > +             ret = regmap_read(regmap, chan->address, &reg_val);
> > +             if (ret < 0)
>
> Do all these ' < 0' for regmap API make any sense?

I guess not, will change these to use (ret)
>
> > +                     goto exit;
>
> ...
>
> > +     ret = pm_runtime_resume_and_get(data->dev);
> > +     if (ret < 0)
> > +             return ret;
>
> There are respective PM_RUNTIME_*() macros.
>
> ...
>
> > +     if ((reg_val & 0xff) != VEML3328_ID_VAL)
>
> Do you need the & 0xff? Do you have register width > 8 bits?

The datasheet says that the high byte is reserved (silicon revision imo),
so it does contain data, therefore it has to be masked.

>
> > +             return dev_err_probe(dev, -ENODEV, "Invalid device ID\n");
>
> We don't fail on that, now we just warn.

Alright.

> ...
>
> > +static DEFINE_RUNTIME_DEV_PM_OPS(veml3328_pm_ops, veml3328_runtime_suspend,
> > +                              veml3328_runtime_resume, NULL);
>
> Split logically.

Easy enough.

>
> ...
>
> > +static const struct i2c_device_id veml3328_id[] = {
> > +     { "veml3328" },
>
> Use .name.
> This is a new development due to some Uwe's ongoing (re)work on ID tables.

Sure.

> > +     { }
> > +};
>
> ...
>
> Was this AI-assisted? If so, don't forget to add tags.

My bad, forgot to add it. Thanks for the detailed review!


--
Kind regards

CJD