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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Aug 07, 2026 at 03:51:55PM +0200, Javier Carrasco wrote:
> The device provides a shared interrupt line for to notify events and
> data ready, which can be used as a trigger. The interrupt line is not a
> requirement for the device to work. Implement variants for the cases
> whether the interrupt line is provided or not.

...

> +#define VEML6031X00_INT_MASK        (VEML6031X00_INT_TH_L | \
> +				     VEML6031X00_INT_TH_H | \
> +				     VEML6031X00_INT_DRDY)

Make it better style with

#define VEML6031X00_INT_MASK						     \
	(VEML6031X00_INT_TH_L | VEML6031X00_INT_TH_H | VEML6031X00_INT_DRDY)


(looking further in the code I kinda have a déjà vu that I said that already in
 the past).

...

> +	/*
> +	 * Serialize access to irq enable/disable by events and trigger
> +	 * (shared line)

Missing period.

> +	 */

...

> +static int veml6031x00_read_period(struct iio_dev *iio, int *val)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret, regval;

Why is regval signed?

> +	ret = regmap_field_read(data->rf.pers, &regval);
> +	if (ret)
> +		return ret;
> +
> +	*val = 1 << regval;

BIT() ?

> +	return IIO_VAL_INT;
> +}

...

> +static int veml6031x00_write_th(struct iio_dev *iio, int val, int val2, int dir)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	__le16 regval = cpu_to_le16(val);

There is no technical need to assign it here, especially if the below
validation won't pass, but it doesn't have any side effects, so I guess
it's fine.

> +	int ret;
> +
> +	if (val < 0 || val > U16_MAX || val2)
> +		return -EINVAL;
> +
> +	if (dir == IIO_EV_DIR_RISING) {
> +		ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WH_L,
> +					&regval, sizeof(regval));
> +		if (ret)
> +			dev_dbg(regmap_get_device(data->regmap),
> +				"Failed to set high threshold %d\n", ret);
> +	} else {
> +		ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WL_L,
> +					&regval, sizeof(regval));
> +		if (ret)
> +			dev_dbg(regmap_get_device(data->regmap),
> +				"Failed to set low threshold %d\n", ret);
> +	}
> +
> +	return ret;
> +}

...

> +static int veml6031x00_set_interrupt(struct veml6031x00_data *data, bool state)
> +	__must_hold(&data->irq_lock)

The sparse annotations is fine, but lockdep one is even better.

> +{
> +	int ret;
> +
> +	if (state) {
> +		data->int_users++;
> +		if (data->int_users > 1)
> +			return 0;
> +	} else {
> +		data->int_users--;
> +		if (data->int_users > 0)
> +			return 0;
> +	}
> +
> +	ret = regmap_field_write(data->rf.int_en, state);
> +	if (ret) {
> +		if (state)
> +			data->int_users--;
> +		else
> +			data->int_users++;
> +	}
> +
> +	return ret;
> +}

...

> +static irqreturn_t veml6031x00_interrupt(int irq, void *private)
> +{
> +	struct iio_dev *iio = private;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	s64 timestamp;
> +	int regval, ret;

Why is regval signed?

> +	bool trigger_poll;
> +
> +	scoped_guard(mutex, &data->irq_lock) {
> +		ret = regmap_read(data->regmap, VEML6031X00_REG_INT, &regval);
> +		if (ret) {
> +			dev_dbg(regmap_get_device(data->regmap),
> +				"Failed to read interrupt register %d\n", ret);
> +			return IRQ_NONE;
> +		}
> +
> +		if (!(regval & VEML6031X00_INT_MASK))
> +			return IRQ_NONE;
> +
> +		if ((regval & (VEML6031X00_INT_TH_H | VEML6031X00_INT_TH_L)) &&
> +		    data->ev_en) {
> +			timestamp = iio_get_time_ns(iio);
> +
> +			if (regval & VEML6031X00_INT_TH_H)
> +				iio_push_event(iio,
> +					       IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +								    IIO_EV_TYPE_THRESH,
> +								    IIO_EV_DIR_RISING),
> +					       timestamp);
> +			if (regval & VEML6031X00_INT_TH_L)
> +				iio_push_event(iio,
> +					       IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +								    IIO_EV_TYPE_THRESH,
> +								    IIO_EV_DIR_FALLING),
> +					       timestamp);
> +		}
> +
> +		trigger_poll = (regval & VEML6031X00_INT_DRDY) && data->trig_en;
> +	}
> +
> +	/*
> +	 * iio_trigger_poll_nested() must be called with irq_lock released:
> +	 * iio_trigger_poll_nested() runs trig_handler() synchronously in this
> +	 * thread, which calls reenable() on completion, and that callback also
> +	 * takes irq_lock.
> +	 */
> +	if (trigger_poll)
> +		iio_trigger_poll_nested(data->trig);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int veml6031x00_setup_irq(struct i2c_client *i2c, struct iio_dev *iio)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret;
> +
> +	data->trig = devm_iio_trigger_alloc(regmap_get_device(data->regmap),
> +					    "%s-drdy%d", iio->name, iio_device_id(iio));
> +	if (!data->trig)
> +		return -ENOMEM;
> +
> +	data->trig->ops = &veml6031x00_trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, iio);
> +
> +	ret = devm_iio_trigger_register(regmap_get_device(data->regmap), data->trig);
> +	if (ret)
> +		return ret;
> +
> +	iio->trig = iio_trigger_get(data->trig);
> +	ret = devm_request_threaded_irq(regmap_get_device(data->regmap),
> +					i2c->irq, NULL,
> +					veml6031x00_interrupt,
> +					IRQF_ONESHOT,
> +					iio->name, iio);
> +	if (ret)

> +		return dev_err_probe(regmap_get_device(data->regmap), ret,
> +				     "Failed to request irq %d\n",
> +				     i2c->irq);

This is a dup message. Remove it.

> +
>  	return 0;

	return devm_request_threaded_irq(...);

>  }

-- 
With Best Regards,
Andy Shevchenko