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

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

Hi Jonathan, thanks again for your review.

On Mon Jun 1, 2026 at 12:21 PM CEST, Jonathan Cameron wrote:
> 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.
>

I am not sure if I get this point. This device has a single IIO_LIGHT
channel, and the scale only applies to it. Are info_mask_separate and
info_mask_shared_by_type not the same in that case? I have seen that
some drivers use both info_mask_separate for INFO_RAW, and
info_mask_shared_by_type for INFO_INT_TIME and/or INFO_SCALE, but that
could make more sense if there were multiple channels of the same type.
What am I missing here?

On the other hand, the integration time applies to both the IIO_LIGHT
and IIO_INTENSITY channels, so I guess you are suggesting to add it
to both channels as info_mask_shared_by_type because the timestamp
is a channel itself. Moving it form shared_by_all to shared_by_type is
alright, and I will add it to V5.

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

You are right assuming that the datasheet will not give us an answer
about that. I suspect by my measurements that the measurement is carried
out with the first integration time, which would make this issue
harmless. But as I am not 100% sure, I contacted the manufacturer to
have a reliable answer. I will wait for a few days while I fix all the
stuff for V5, and if I don't get an answer, or the new integration time
is used, I will use the mutexes as you suggested. I don't like using
mutexes to protect sections of code "just in case" without a real reason
behind, but if there is no way to know, then it will have to be that
way...

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

Ack.

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

Ack.

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

I took some time to audit this in detail, because although my
expectations are that atomic_add_unless(usage_count, -1, 0) should make
this shout a false positive. Expectations don't always meet reality. My
expectations were based on the code, so I have added tracepoints to know
exactly what's going on.

Scenario 1: Successful probe -> unbind driver.

devm_pm_runtime_get_noresume() increases usage_count, and the call to
pm_runtime_put_autosuspend() decreases it. That is balanced, giving us
usage_count = 0 and putting the device in power down mode as desired. If
the device is then unbound, the devres action (a call to
pm_runtime_put_noidle_action, which only calls pm_runtime_put_noidle)
triggers a call to atomic_add_unless(&dev->power.usage_count, -1, 0).
Given that the usage count is 0, nothing happens and there is no
underflow. In fact, the underflow is not possible this way, and adding a
remove function to check if usage_count is 0 and calling
pm_runtime_put() is basically repeating what the devres action already
offers for free.

Scenario 2: write_event_config -> unbind driver.

Sashiko says that pm_runtime_resume_and_get() increases usage_count, and
there is no devres action associated to it. That is only partially
correct, because the devres action from devm_pm_runtime_get_noresume()
will be triggered when the driver is unbound. Actually, this is great
because then pm_runtime_resume_and_get() gets balanced, and there is no
need for a remove function to check again if usage_count is 0 or not. In
this case, usage_count = 1 before unbinding the driver, and then the
devres action is triggered when it gets unbound. Exactly what we want to
have usage_count = 0.

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

Best regards,
Javier