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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 24 May 2026 23:53:56 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> 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.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

A few things inline.  Biggest one is that device driver specific state needs
local well documented locking. Here the whole complex gain handling means there
are a bunch of register field where the accesses to each set of them need
to appear atomic.

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> new file mode 100644
> index 000000000000..50979d239230
> --- /dev/null
> +++ b/drivers/iio/light/veml6031x00.c
> @@ -0,0 +1,657 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * VEML6031X00 Ambient Light Sensor
> + *
> + * Copyright (c) 2026, Javier Carrasco <javier.carrasco.cruz@gmail.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>

mod_devicetable.h is missing. Please check again for others.

> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/units.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
This isn't needed yet - bring it in when it is.

> +#include <linux/iio/iio-gts-helper.h>

> +static int veml6031x00_set_scale(struct iio_dev *iio, int val, int val2)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int gain_sel, it_sel, ret;
> +
> +	ret = iio_gts_find_gain_time_sel_for_scale(&data->gts, val, val2,
> +						   &gain_sel, &it_sel);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_write(data->rf.it, it_sel);
> +	if (ret)
> +		return ret;
> +
> +	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);
> +}
> +
> +static int veml6031x00_get_scale(struct veml6031x00_data *data, int *val,

One of the few thing sashiko raised on this patch that seems reasonable is
this is doing a bunch of accesses to fields that are updated via other
paths. It may make sense to ensure we get a consistent set and don't manage
to hit the middle of a write elsewhere - such as in set_scale()

Given that this is a driver specific thing rather than a state transition
matter, a local mutex should be used to ensure these are serialized.
Using the direct mode claim is relying on an internal implementation detail
(that there is a serializing lock in there).


> +				 int *val2)
> +{
> +	int gain, it, gain_reg, pd_div4, it_reg, ret, sel;
> +
> +	ret = regmap_field_read(data->rf.gain, &gain_reg);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_read(data->rf.pd_div4, &pd_div4);
> +	if (ret)
> +		return ret;
> +
> +	sel = (pd_div4 << 2) | gain_reg;
> +	gain = iio_gts_find_gain_by_sel(&data->gts, sel);
> +	if (gain < 0)
> +		return gain;
> +
> +	ret = regmap_field_read(data->rf.it, &it_reg);
> +	if (ret)
> +		return ret;
> +
> +	it = iio_gts_find_int_time_by_sel(&data->gts, it_reg);
> +	if (it < 0)
> +		return it;
> +
> +	ret = iio_gts_get_scale(&data->gts, gain, it, val, val2);
> +	if (ret)
> +		return ret;
> +
> +	return IIO_VAL_INT_PLUS_NANO;
> +}
> +
> +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;
> +	}
> +
> +	IIO_DEV_ACQUIRE_DIRECT_MODE(iio, claim);
Doing this before you have added any means to get into any other mode
is premature. This claim belongs in the next patch.

> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;
> +
> +	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(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_read_raw(struct iio_dev *iio,
> +				struct iio_chan_spec const *chan, int *val,
> +				int *val2, long mask)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		return veml6031x00_single_read(iio, chan->type, val);
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		return veml6031x00_get_it(data, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		return veml6031x00_get_scale(data, val, val2);
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static int veml6031x00_write_raw(struct iio_dev *iio,
> +				 struct iio_chan_spec const *chan,
> +				 int val, int val2, long mask)
> +{
> +	IIO_DEV_ACQUIRE_DIRECT_MODE(iio, claim);
As above. 
> +	if (IIO_DEV_ACQUIRE_FAILED(claim))
> +		return -EBUSY;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		return veml6031x00_set_it(iio, val, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		return veml6031x00_set_scale(iio, val, val2);
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +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");
> +
> +	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;
> +
> +	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");
> +
> +	ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);

Why is shutdown cleanup registered before the power_on?  If that has to happen
then add a comment on why here. Otherwise move it down a few lines.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add shutdown action\n");
> +
> +	/* 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");
> +
> +	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);
> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
> +
> +	return 0;
> +}

> +
> +static const struct of_device_id veml6031x00_of_match[] = {
> +	{
> +		.compatible = "vishay,veml6031x00",
> +		.data = &veml6031x00_chip,
> +	},
> +	{
> +		.compatible = "vishay,veml6031x01",
> +		.data = &veml6031x01_chip,
> +	},
> +	{
> +		.compatible = "vishay,veml60311x00",
> +		.data = &veml60311x00_chip,
> +	},
> +	{
> +		.compatible = "vishay,veml60311x01",
> +		.data = &veml60311x01_chip,

These would actually end up shorter than the i2c_device_id ones below
if they were on one line.  Pick a style and use it for both tables.
I prefer this one a little.


> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, veml6031x00_of_match);
> +
> +static const struct i2c_device_id veml6031x00_id[] = {
> +	{ .name = "veml6031x00", .driver_data = (kernel_ulong_t)&veml6031x00_chip },
> +	{ .name = "veml6031x01", .driver_data = (kernel_ulong_t)&veml6031x01_chip },
> +	{ .name = "veml60311x00", .driver_data = (kernel_ulong_t)&veml60311x00_chip },
> +	{ .name = "veml60311x01", .driver_data = (kernel_ulong_t)&veml60311x01_chip },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, veml6031x00_id);
> +
> +static struct i2c_driver veml6031x00_driver = {
> +	.driver = {
> +		.name = "veml6031x00",
> +		.of_match_table = veml6031x00_of_match,
> +		.pm = pm_ptr(&veml6031x00_pm_ops),
> +	},
> +	.probe = veml6031x00_probe,
> +	.id_table = veml6031x00_id,
> +};
> +module_i2c_driver(veml6031x00_driver);
> +
> +MODULE_AUTHOR("Javier Carrasco <javier.carrasco.cruz@gmail.com>");
> +MODULE_DESCRIPTION("VEML6031X00 Ambient Light Sensor");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("IIO_GTS_HELPER");
>