Re: [PATCH v4 3/4] iio: light: veml6031x00: add support for triggered buffers

[Andy Shevchenko]

On Sun, May 31, 2026 at 09:58:23PM +0200, Javier Carrasco wrote:
> Add triggered buffer functionality for the two channels the device
> provides (ALS and IR).

...

> +static int veml6031x00_buffer_preenable(struct iio_dev *iio)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret, it_usec;

Can it_usec be negative?

> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret)
> +		return ret;

Wouldn't be better to use respective ACQUIRE() macros from pm_runtime.h?

> +	ret = veml6031x00_get_it(data, &it_usec);
> +	if (ret < 0) {
> +		pm_runtime_put_autosuspend(data->dev);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Wait one integration period + 10% margin so the first triggered
> +	 * read does not race with the sensor completing its first conversion
> +	 * after power-on.
> +	 */
> +	fsleep(it_usec + (it_usec / 10));

fsleep() adds up to 25% of margin, wouldn't be that enough?

> +	return 0;
> +}

...

> +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);
> +	int ch, ret, i = 0;

Make 'i' unsigned and split the assignment so it goes closer to the user, makes
maintenance easier.

> +	struct {
> +		__le16 chans[2];
> +		aligned_s64 timestamp;
> +	} scan = { };
> +
> +	if (test_bit(VEML6031X00_SCAN_ALS, iio->active_scan_mask) &&
> +	    test_bit(VEML6031X00_SCAN_IR, iio->active_scan_mask)) {

I hope everyone understands that this testing may be immediately false here
(without any additional protection added). Even more, the _ALS bit may be
cleared just before checking the _IR one.

> +		ret = regmap_bulk_read(data->regmap,
> +				       VEML6031X00_REG_ALS_L,
> +				       &scan.chans, sizeof(scan.chans));
> +		if (ret)
> +			goto done;
> +	} else {
> +		iio_for_each_active_channel(iio, ch) {
> +			ret = regmap_bulk_read(data->regmap,
> +					       iio->channels[ch].address,
> +					       &scan.chans[i++],
> +					       sizeof(*scan.chans));
> +			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;
> +}

-- 
With Best Regards,
Andy Shevchenko