Re: [PATCH v9 4/4] iio: light: veml6031x00: add support for events and trigger

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Device left in Active Force (AF) mode causes stale data in single_read
- [Medium] Spurious I2C read from device configuration register caused by iterating over the timestamp channel
- [Medium] Software state machine desynchronization leaks interrupt user reference on rollback failure
--

commit 20d504b87ca25179c583e1df1a78eee06d4b1837
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

iio: light: veml6031x00: add support for events and trigger

The device provides a shared interrupt line to notify events and
data ready.

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index 3d5a27a0baaed..110f59625a5b1 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c
[ ... ]
> @@ -551,11 +976,83 @@ static int veml6031x00_buffer_postdisable(struct iio_dev *iio)
>  	return 0;
>  }
>  
> +static int __veml6031x00_set_trigger_state(struct veml6031x00_data *data, bool state)
> +	__must_hold(&data->irq_lock)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&data->irq_lock);
> +
> +	ret = veml6031x00_set_interrupt(data, state);
> +	if (ret)
> +		return ret;
> +
> +	/* The AF bit must be updated before updating AF_TRIG */
> +	ret = regmap_assign_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				 VEML6031X00_CONF0_AF, state);
> +	if (ret)
> +		goto clear_interrupt;
> +
> +	ret = regmap_assign_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				 VEML6031X00_CONF0_AF_TRIG, state);
> +	if (ret) {
> +		regmap_assign_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				   VEML6031X00_CONF0_AF, !state);
> +		goto clear_interrupt;
> +	}
> +
> +	data->trig_en = state;
> +
> +	return 0;
> +
> +clear_interrupt:
> +	veml6031x00_set_interrupt(data, !state);

[Severity: Medium]
Does this rollback path cause a state desynchronization if the I2C write in
veml6031x00_set_interrupt() fails?

If the rollback fails, interrupt_users is incremented back to match the
hardware state, but trig_en remains false. This leaves the software counter
orphaned relative to trig_en, which can permanently prevent the driver from
disabling the hardware interrupt in the future.

Can we handle this failure so the interrupt_users count stays synchronized?

> +
> +	return ret;
> +}
[ ... ]
> +static irqreturn_t veml6031x00_trig_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	struct regmap *map = data->regmap;
> +	IIO_DECLARE_BUFFER_WITH_TS(__le16, scan, 2) = { };
> +	unsigned int i;
> +	int ch, ret;
> +
> +	if (test_bit(VEML6031X00_SCAN_ALS, iio->active_scan_mask) &&
> +	    test_bit(VEML6031X00_SCAN_IR, iio->active_scan_mask)) {
> +		ret = regmap_bulk_read(map, VEML6031X00_REG_ALS_L,
> +				       scan, 2 * sizeof(*scan));
> +		if (ret)
> +			goto done;
> +	} else {
> +		i = 0;
> +		iio_for_each_active_channel(iio, ch) {
> +			ret = regmap_bulk_read(map, iio->channels[ch].address,
> +					       &scan[i++], sizeof(*scan));

[Severity: Medium]
Does this loop read from the configuration register by mistake if the
timestamp channel is enabled alongside a single data channel?

The iio_for_each_active_channel() loop processes the timestamp channel, but
its .address is uninitialized (0). This causes a spurious I2C read from
address 0x00 (CONF0), writing the config register contents into the timestamp
padding of the scan buffer.

Could we skip the timestamp channel or check the address before reading?

> +			if (ret)
> +				goto done;
> +		}
> +	}
> +
> +	iio_push_to_buffers_with_ts(iio, scan, sizeof(scan), pf->timestamp);
> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> +static int veml6031x00_hw_init(struct veml6031x00_data *data)
> +{
> +	struct regmap *map = data->regmap;
> +	struct device *dev = regmap_get_device(map);
> +	unsigned int val;
> +	__le16 regval;
> +	int ret;
> +
> +	regval = 0;
> +	ret = regmap_bulk_write(map, VEML6031X00_REG_WL_L, &regval, sizeof(regval));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set low threshold\n");
> +
> +	regval = cpu_to_le16(U16_MAX);
> +	ret = regmap_bulk_write(map, VEML6031X00_REG_WH_L, &regval, sizeof(regval));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set high threshold\n");
> +
> +	ret = regmap_field_write(data->rf.int_en, 0);

[Severity: High]
Does this initialization sequence leave the device stuck in Active Force (AF)
mode?

If the driver is unloaded after using triggered buffers (which sets the AF bit
to 1) and then reloaded, or if a warm reboot occurs with an always-on
regulator, the hardware retains AF=1. Because veml6031x00_hw_init() does not
explicitly reset the VEML6031X00_CONF0_AF bit to 0, the device remains in
Active Force mode.

Subsequent calls to veml6031x00_single_read() do not assert the AF_TRIG bit,
causing the sensor to never execute a new conversion and indefinitely return
stale data.

Should we explicitly reset the AF bit during hardware initialization?

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, VEML6031X00_REG_INT, &val);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to clear interrupts\n");
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-veml6031x00-v9-0-8c93a6c66a4d@gmail.com?part=4