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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 31 May 2026 21:58:22 +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 comments from sashiko and a couple from me based on a fresh read.


> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> new file mode 100644
> index 000000000000..6f9a7bad44d4
> --- /dev/null
> +++ b/drivers/iio/light/veml6031x00.c

> +
> +static const struct iio_chan_spec veml6031x00_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.address = VEML6031X00_REG_ALS_L,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.address = VEML6031X00_REG_IR_L,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_IR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),

Can we move scale to shared_by_type?

Thinking on some of the others, they should probably be shared_by_type as well
rather than shared_by_all. If we ever add buffered support and a timestamp
the integration_time doesn't apply to that. 

shared_by_all tends to only include things that are truely universal like
sampling_frequency.


> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +	},
> +};

> +
> +static int veml6031x00_get_it(struct veml6031x00_data *data, int *val2)
> +{
> +	int ret, it_idx;
> +
> +	scoped_guard(mutex, &data->scale_lock) {

Given below I suggest you need an unlocked variant of this, maybe
think about whether we care if the scope includes the table search.

I'd just take the lock for the whole function.

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

https://sashiko.dev/#/patchset/20260531-veml6031x00-v4-0-e64f7fdce38d%40gmail.com

Hmm. Sashiko shouts race here. It is kind of correct in that we don't know
if we have a matching pair of integration time and reading. Thus we might
have
Thread 1                           Thread 2
Power on.                           
Get small integration time           
                                   Set large integration time
read before new integration done.

Whether this is bug kind of depends on the device when the integration time
is updated.  Does it finish current integration with old one, or does it just
update some register against which a comparator is running. So if we are
already integrating, do we just integrate for longer before stopping?

I haven't checked but this smells like kind of thing a datasheet won't tell
us.  Hence I'd be tempted to throw use of data->mutex to serialize single
reads and integration time updates + anything related to that).  It's
harmless at worst and makes it easier to reason about this code.
You'll need an unlocked  __veml6031x00_get_it() variant to call though
given that takes the same lock. 



 


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

Maybe relax to dev_info() given we expect this to happen if fallback
compatibles get used in future. Warn is a little strong.

> +
> +	return 0;
> +}

> +static int veml6031x00_probe(struct i2c_client *i2c)
> +{

> +	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;
This block doesn't need to happen in the runtime pm region.
> +
> +	ret = veml6031x00_hw_init(iio);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_put_autosuspend(dev);

As sashiko shouts, this looks like runtime pm will underflow on remove.
Check it by removing your driver.  It doesn't actually result in any 
problem, as the runtime pm subsystem just saturates at 0 on decrement.
Given that's tear down anyway maybe we don't care.  However, it's easy
enough to fix by using pm_runtime_get_no_resume() and a couple of
explicit calls to put it in error paths.





> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
> +
> +	return 0;
> +}

>