Re: [PATCH 2/2] iio: light: veml3328: add support for new device
From: Andy Shevchenko
Date: Sun May 17 2026 - 03:53:29 EST
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
> +#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;
> +};
...
> +/* 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.
> +};
...
> + 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.
...
> +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.
> + return 0;
> +}
...
> + ret = regmap_read(regmap, chan->address, ®_val);
> + if (ret < 0)
Do all these ' < 0' for regmap API make any sense?
> + 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?
> + return dev_err_probe(dev, -ENODEV, "Invalid device ID\n");
We don't fail on that, now we just warn.
...
> +static DEFINE_RUNTIME_DEV_PM_OPS(veml3328_pm_ops, veml3328_runtime_suspend,
> + veml3328_runtime_resume, NULL);
Split logically.
...
> +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.
> + { }
> +};
...
Was this AI-assisted? If so, don't forget to add tags.
--
With Best Regards,
Andy Shevchenko