Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

["Javier Carrasco" <javier.carrasco.cruz@gmail.com>]

Hi Andy, thanks for your review.

On Tue Jun 2, 2026 at 12:01 PM CEST, Andy Shevchenko wrote:
> 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
>

Do you know any tool to automate this beyond asking an AI? Manual
auditing is not very reliablo and it is not that difficult to miss a
header that has been indirectly included. Building with W=1 and similar
stuff did not help.

>> +#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.
>
Ack.
>> +	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.
>

I prefer these definitions, and a similar pattern is used in multiple
drivers in IIO, but I have no strong feelings about it.

>> +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'.
>
Ack.
>> +	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?
>
I'll make it unsigned int.
>> +
>> +	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...
>
Ack.
>> +	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?
>
> ...
>
Ack.
>> +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?
>

The problem is that it adds UP to 25%, but that is not guaranteed. If
almost no extra delay is added, it will be below the margin I added,
which was necessary when I tested this delay with real hardware.

>> +	fsleep(it_usec + (it_usec / 10));
>> +
>> +	ret = regmap_bulk_read(data->regmap, addr, &reg, 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, &reg,
>> +			       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?
>

Jonathan would like this to be a dev_info().

>> +	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
>
Ack.
>> +	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.
>
Ack.
>> +	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.
>
Ack.
>> +	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.
>

Ack.

>> +	/* 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?
>

I could move this a bit further down after devm_iio_device_register(),
but as I replied to a Sashiko complaint, the reference count will never
underflow in this configuration.

>> +	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);
>
> ...
>

Ack.

>> +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.
>
Ack.
>> +	},
>> +	{
>> +		.name = "veml6031x01",
>> +		.driver_data = (kernel_ulong_t)&veml6031x01_chip },
>
> Broken indentation, should be a new line somewhere.
>
Ack.
>> +	{
>> +		.name = "veml60311x00",
>> +		.driver_data = (kernel_ulong_t)&veml60311x00_chip
>> +	},
>> +	{
>> +		.name = "veml60311x01",
>> +		.driver_data = (kernel_ulong_t)&veml60311x01_chip
>> +	},
>
>> +};

Best regards,
Javier