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

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 31 May 2026 21:58:24 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> 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.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
A few more things inline. Some from sashiko.

> +static int veml6031x00_write_event_config(struct iio_dev *iio,
> +					  const struct iio_chan_spec *chan,
> +					  enum iio_event_type type,
> +					  enum iio_event_direction dir,
> +					  bool state)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret;
> +
> +	guard(mutex)(&data->irq_lock);
> +
> +	/* avoid multiple increments/decrements from one source */
> +	if (state == data->ev_en)
> +		return 0;
> +
> +	if (state) {
> +		ret = pm_runtime_resume_and_get(data->dev);

Sashiko points out that, unlike buffered capture there is no core
based disabling of events in remove path.  Hence if events are on
we may not get the runtime pm put that we need to actually power down
the device and the stuff above it in the system.  You need
to do your own tracking for whether it's needed or not.

For background, we can't do a general disable from the IIO core because
the core doesn't actually know what events are enabled as we don't model
the complex interactions that can occur between different events - i.e.
enabling one can disable another.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = veml6031x00_set_interrupt(data, state);
> +	if (ret) {
> +		if (state)
> +			pm_runtime_put_autosuspend(data->dev);
> +		return ret;
> +	}
> +
> +	data->ev_en = state;
> +
> +	if (!state)
> +		pm_runtime_put_autosuspend(data->dev);
> +
> +	return 0;
> +}

>  
> +/* AF_TRIG is reset by hardware, but the rest of the fields are persistent */

https://sashiko.dev/#/patchset/20260531-veml6031x00-v4-0-e64f7fdce38d%40gmail.com
comment seems valid to me. I think you have to treat this register as volatile
and rewrite the whole thing every time. Maybe a reorder would work. Drop the region
after the update.  However then next access has to do a read so maybe just
do your own sub register caching for this one.


> +static int veml6031x00_set_af_trig(struct veml6031x00_data *data, bool state)
> +{
> +	regcache_drop_region(data->regmap, VEML6031X00_REG_CONF0,
> +			     VEML6031X00_REG_CONF0);
> +
> +	return regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				  VEML6031X00_CONF0_AF_TRIG,
> +				  FIELD_PREP(VEML6031X00_CONF0_AF_TRIG, state));
> +}
> +
> +static irqreturn_t veml6031x00_interrupt(int irq, void *private)
> +{
> +	struct iio_dev *iio = private;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	s64 timestamp;
> +	int ret, reg;

regval. Reg is often used for register address.

> +
> +	ret = regmap_read(data->regmap, VEML6031X00_REG_INT, &reg);
> +	if (ret) {
> +		dev_err(data->dev,
> +			"Failed to read interrupt register %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	if (!(reg & VEML6031X00_INT_MASK))
> +		return IRQ_NONE;
> +
> +	guard(mutex)(&data->irq_lock);

I'm not really sure what this lock is protecting against.  It doesn't cover
the register read above, so the rest may be based on stale value of reg.
Perhaps some comments?

> +
> +	if ((reg & (VEML6031X00_INT_TH_H | VEML6031X00_INT_TH_L)) && data->ev_en) {
> +		timestamp = iio_get_time_ns(iio);
> +
> +		if (reg & 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 (reg & VEML6031X00_INT_TH_L)
> +			iio_push_event(iio, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +								 IIO_EV_TYPE_THRESH,
> +								 IIO_EV_DIR_FALLING),
> +				       timestamp);
> +	}
> +
> +	if ((reg & VEML6031X00_INT_DRDY) && data->trig_en) {
> +		iio_trigger_poll_nested(data->trig);
> +		ret = veml6031x00_set_af_trig(data, true);

This superficially seems like something that maybe belongs in the trigger
reenable callback?  That will get called in much the same place, but has
the advantage that it documents this is about reenabling that trigger.

> +		if (ret)
> +			dev_err(data->dev, "Failed to set trigger %d\n", ret);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int veml6031x00_buffer_preenable(struct iio_dev *iio)
>  {
>  	struct veml6031x00_data *data = iio_priv(iio);
> @@ -534,11 +863,54 @@ static int veml6031x00_buffer_postdisable(struct iio_dev *iio)
>  	return 0;
>  }
>  
> +static int veml6031x00_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *iio = iio_trigger_get_drvdata(trig);
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret;
> +
> +	guard(mutex)(&data->irq_lock);

Correct report form sashiko here. You should not be mixing guard and gotos.
It's fine to just use traditional mutex_lock / unlock in some
functions where guard() is not appropriate.

Note i don't think there is a bug here but this pattern is considered
fragile as future code might see the gotos and add some more and one of
those might cross the declaration hidden in guard()


> +
> +	if (state == data->trig_en)
> +		return 0;
> +
> +	ret = veml6031x00_set_interrupt(data, state);
> +	if (ret)
> +		return ret;
> +
> +	/* The AF bit must be set before setting AF_TRIG */
> +	ret = regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				 VEML6031X00_CONF0_AF,
> +				 FIELD_PREP(VEML6031X00_CONF0_AF, state));
> +	if (ret)
> +		goto err_disable_interrupt;
> +
> +	ret = veml6031x00_set_af_trig(data, state);
> +	if (ret)
> +		goto err_clear_af;
> +
> +	data->trig_en = state;
> +
> +	return 0;
> +
> +err_clear_af:
> +	regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +			   VEML6031X00_CONF0_AF,
> +			   FIELD_PREP(VEML6031X00_CONF0_AF, !state));
> +err_disable_interrupt:
> +	veml6031x00_set_interrupt(data, !state);
> +	return ret;
> +}
> +
>  static const struct iio_buffer_setup_ops veml6031x00_buffer_setup_ops = {
>  	.preenable = veml6031x00_buffer_preenable,
>  	.postdisable = veml6031x00_buffer_postdisable,
>  };
>  
> +static const struct iio_trigger_ops veml6031x00_trigger_ops = {
> +	.set_trigger_state = veml6031x00_set_trigger_state,
Another plausible sashiko comment. Is this trigger usable by another device?
That in theory at least includes when this device is not using it.  Might
be easier to set the validation callback to restrict it to this device.

> +};


>  
> @@ -638,6 +1059,10 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_mutex_init(dev, &data->irq_lock);
> +	if (ret)
> +		return ret;
> +
>  	ret = veml6031x00_regfield_init(data);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to init regfield\n");
> @@ -677,12 +1102,21 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>  	iio->channels = veml6031x00_channels;
>  	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);

Sashiko caught that you need separate channel definitions as well for the
no irq vs irq cases.  Otherwise a bunch of sysfs files that don't do anything
will be created.

>  	iio->modes = INDIO_DIRECT_MODE;
> -	iio->info = &veml6031x00_info;
>  
>  	ret = veml6031x00_hw_init(iio);
>  	if (ret)
>  		return ret;
>  
> +	if (i2c->irq) {
> +		ret = veml6031x00_setup_irq(i2c, iio);
> +		if (ret)
> +			return ret;
> +
> +		iio->info = &veml6031x00_info;
> +	} else {
> +		iio->info = &veml6031x00_info_no_irq;
> +	}
> +
>  	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
>  					      veml6031x00_trig_handler,
>  					      &veml6031x00_buffer_setup_ops);
>