Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series
From: Andy Shevchenko
Date: Tue Jun 02 2026 - 06:10:58 EST
On Sun, May 31, 2026 at 09:58:22PM +0200, Javier Carrasco wrote:
> These sensors provide two light channels (ALS and IR), I2C communication
> and a multiplexed interrupt line to signal data ready and configurable
> threshold alarms.
>
> This first implementation provides basic functionality (measurement
> configuration, raw reads and ID validation) and defines the different
> register regions in preparation for extended features in the subsequent
> patches of the series.
...
+ array_size.h
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
In C locale it seems wrong order.
> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
+ types.h
> +#include <linux/units.h>
+ Blank line.
> +#include <linux/iio/iio.h>
> +#include <linux/iio/iio-gts-helper.h>
...
> +struct veml6031x00_data {
Have you run `pahole`? Does it agree with your layout?
> + struct device *dev;
> + struct iio_gts gts;
> + struct regmap *regmap;
Do you need dev and regmap? One may be derived from the other in case regmap
uses the same dev on initialisation as the one stored here.
> + struct veml6031x00_rf rf;
> + const struct veml6031x00_chip *chip;
> + /*
> + * Serialize access to scale register fields scattered across multiple
> + * registers (rf.gain, rf.pd_div4, rf.it) to read and write them as a
> + * consistent set.
> + */
> + struct mutex scale_lock;
> +};
...
> +/*
> + * The gain selector encodes (PD_D4 << 2) | GAIN to identify each gain setting.
> + * Gains are multiplied by 8 to work with integers. The values in the iio-gts
> + * tables don't need corrections because the maximum value of the scale refers
> + * to GAIN = x1, and the rest of the values are obtained from the resulting
> + * linear function.
> + * TODO: add support for MILLI_GAIN_X165 and MILLI_GAIN_X660
> + */
> +#define VEML6031X00_SEL_MILLI_GAIN_X125 0x07
> +#define VEML6031X00_SEL_MILLI_GAIN_X250 0x04
> +#define VEML6031X00_SEL_MILLI_GAIN_X500 0x03
> +#define VEML6031X00_SEL_MILLI_GAIN_X1000 0x00
> +#define VEML6031X00_SEL_MILLI_GAIN_X2000 0x01
Not sure if these one-time use definitions improve or not the readability
of the code. Up to Jonathan.
> +static const struct iio_gain_sel_pair veml6031x00_gain_sel[] = {
> + GAIN_SCALE_GAIN(1, VEML6031X00_SEL_MILLI_GAIN_X125),
> + GAIN_SCALE_GAIN(2, VEML6031X00_SEL_MILLI_GAIN_X250),
> + GAIN_SCALE_GAIN(4, VEML6031X00_SEL_MILLI_GAIN_X500),
> + GAIN_SCALE_GAIN(8, VEML6031X00_SEL_MILLI_GAIN_X1000),
> + GAIN_SCALE_GAIN(16, VEML6031X00_SEL_MILLI_GAIN_X2000),
> +};
...
> +{
> + struct regmap *regmap = data->regmap;
> + struct device *dev = data->dev;
In case you really need a 'dev' here, pass via function parameter, no need to
keep it in the 'data'.
> + struct regmap_field *rm_field;
> + struct veml6031x00_rf *rf = &data->rf;
> +
> + rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_gain);
> + if (IS_ERR(rm_field))
> + return PTR_ERR(rm_field);
> + rf->gain = rm_field;
> +
> + rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_it);
> + if (IS_ERR(rm_field))
> + return PTR_ERR(rm_field);
> + rf->it = rm_field;
> +
> + rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_pd_div4);
> + if (IS_ERR(rm_field))
> + return PTR_ERR(rm_field);
> + rf->pd_div4 = rm_field;
> +
> + return 0;
> +}
...
> +static int veml6031x00_get_it(struct veml6031x00_data *data, int *val2)
> +{
> + int ret, it_idx;
Why is 'it_idx' signed?
> +
> + scoped_guard(mutex, &data->scale_lock) {
> + ret = regmap_field_read(data->rf.it, &it_idx);
> + if (ret)
> + return ret;
> + }
> +
> + ret = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
> + if (ret < 0)
> + return ret;
> +
> + *val2 = ret;
> +
> + return IIO_VAL_INT_PLUS_MICRO;
> +}
...
> +static int veml6031x00_set_it(struct iio_dev *iio, int val, int val2)
> +{
> + struct veml6031x00_data *data = iio_priv(iio);
> + int ret, gain_sel, gain_reg, pd_div4, it_idx, new_gain, prev_gain, prev_it;
Similar question here and so on...
> + bool in_range;
> +}
...
> + ret = regmap_field_write(data->rf.pd_div4, gain_sel >> 2);
> + if (ret)
> + return ret;
> +
> + return regmap_field_write(data->rf.gain, gain_sel & 0x03);
Looks like repetitive piece of code, shouldn't be a helper?
...
> +static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
> + int *val)
> +{
> + struct veml6031x00_data *data = iio_priv(iio);
> + int addr, it_usec, ret;
> + __le16 reg;
> +
> + switch (type) {
> + case IIO_LIGHT:
> + addr = VEML6031X00_REG_ALS_L;
> + break;
> + case IIO_INTENSITY:
> + addr = VEML6031X00_REG_IR_L;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + PM_RUNTIME_ACQUIRE_AUTOSUSPEND(data->dev, pm);
> + ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> + if (ret)
> + return ret;
> +
> + ret = veml6031x00_get_it(data, &it_usec);
> + if (ret < 0)
> + return ret;
> + /* integration time + 10 % to ensure completion */
fsleep() adds up to 25%, isn't it enough?
> + fsleep(it_usec + (it_usec / 10));
> +
> + ret = regmap_bulk_read(data->regmap, addr, ®, sizeof(reg));
> + if (ret)
> + return ret;
> +
> + *val = le16_to_cpu(reg);
> + return IIO_VAL_INT;
> +}
...
> +static int veml6031x00_validate_part_id(struct veml6031x00_data *data)
> +{
> + int part_id, ret;
> + __le16 reg;
> +
> + ret = regmap_bulk_read(data->regmap, VEML6031X00_REG_ID_L, ®,
> + sizeof(reg));
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Failed to read ID\n");
> +
> + part_id = le16_to_cpu(reg);
> + if (part_id != data->chip->part_id)
> + dev_warn(data->dev, "Unknown ID %04x\n", part_id);
dev_warn_probe() for the sake of consistency?
> + return 0;
> +}
...
> +static int veml6031x00_hw_init(struct iio_dev *iio)
> +{
> + struct veml6031x00_data *data = iio_priv(iio);
> + struct device *dev = data->dev;
> + int ret;
> +
> + /* Max resolution = 6.9632 lx/cnt for gain = 0.125 and IT = 3.125ms */
> + ret = devm_iio_init_iio_gts(dev, 6, 963200000,
> + veml6031x00_gain_sel,
> + ARRAY_SIZE(veml6031x00_gain_sel),
> + veml6031x00_it_sel,
> + ARRAY_SIZE(veml6031x00_it_sel),
> + &data->gts);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to init iio gts\n");
IIO GTS
> + return 0;
> +}
...
> +static int veml6031x00_probe(struct i2c_client *i2c)
> +{
> + struct device *dev = &i2c->dev;
> + struct veml6031x00_data *data;
> + struct iio_dev *iio;
> + struct regmap *regmap;
> + int ret;
> +
> + regmap = devm_regmap_init_i2c(i2c, &veml6031x00_regmap_config);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Failed to set regmap\n");
One line is okay.
> + iio = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!iio)
> + return -ENOMEM;
> +
> + data = iio_priv(iio);
> + i2c_set_clientdata(i2c, iio);
> + data->dev = dev;
> + data->regmap = regmap;
As I said, one of these two is redundant.
> + ret = devm_mutex_init(dev, &data->scale_lock);
> + if (ret)
> + return ret;
> +
> + ret = veml6031x00_regfield_init(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to init regfield\n");
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable regulator\n");
> + data->chip = i2c_get_match_data(i2c);
> + if (!data->chip)
> + return dev_err_probe(dev, -EINVAL, "Failed to get chip data\n");
I would move this closer to the point when we have data allocated. This is
a cheap check and it's better to boil out without need to allocate resources,
touch regulators (that might be undesired from power consumption and physical
processes due to the dragging them on and off), et cetera.
> + /* The device starts in power down mode by default */
> + ret = veml6031x00_als_power_on(data);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to power on the device\n");
> +
> + ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to add shutdown action\n");
> +
> + pm_runtime_set_autosuspend_delay(dev, 2000);
> + pm_runtime_use_autosuspend(dev);
> + ret = devm_pm_runtime_set_active_enabled(dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> + ret = devm_pm_runtime_get_noresume(dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to get runtime PM\n");
> +
> + ret = veml6031x00_validate_part_id(data);
> + if (ret)
> + return ret;
> +
> + iio->name = data->chip->name;
> + iio->channels = veml6031x00_channels;
> + iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
> + iio->modes = INDIO_DIRECT_MODE;
> + iio->info = &veml6031x00_info;
> +
> + ret = veml6031x00_hw_init(iio);
> + if (ret)
> + return ret;
> + pm_runtime_put_autosuspend(dev);
Hmm... But why? Wouldn't this be problematic with reference count on the failed
devm_iio_device_register() below?
> + ret = devm_iio_device_register(dev, iio);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register iio device\n");
> +
> + return 0;
> +}
...
> +static DEFINE_RUNTIME_DEV_PM_OPS(veml6031x00_pm_ops, veml6031x00_runtime_suspend,
> + veml6031x00_runtime_resume, NULL);
Wrap this logically:
static DEFINE_RUNTIME_DEV_PM_OPS(veml6031x00_pm_ops,
veml6031x00_runtime_suspend,
veml6031x00_runtime_resume,
NULL);
OR
static DEFINE_RUNTIME_DEV_PM_OPS(veml6031x00_pm_ops,
veml6031x00_runtime_suspend, veml6031x00_runtime_resume, NULL);
...
> +static const struct i2c_device_id veml6031x00_id[] = {
> + {
> + .name = "veml6031x00",
> + .driver_data = (kernel_ulong_t)&veml6031x00_chip
In the similar (to OF ID table) way, leave trailing commas.
> + },
> + {
> + .name = "veml6031x01",
> + .driver_data = (kernel_ulong_t)&veml6031x01_chip },
Broken indentation, should be a new line somewhere.
> + {
> + .name = "veml60311x00",
> + .driver_data = (kernel_ulong_t)&veml60311x00_chip
> + },
> + {
> + .name = "veml60311x01",
> + .driver_data = (kernel_ulong_t)&veml60311x01_chip
> + },
> +};
--
With Best Regards,
Andy Shevchenko