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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 12 Aug 2026 22:27:43 +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>
Trivial only.
> ---
>  drivers/iio/light/veml6031x00.c | 545 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 539 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index 43a701f62aea..6dc7ea7e8a4f 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c


> +
> +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);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +
> +	guard(mutex)(&data->irq_lock);
> +
> +	/* avoid multiple increments/decrements from one source */
> +	if (state == data->ev_en)
> +		return 0;

There is not a lot of shared code in here between state true/false
I'd split as two helpers.

	if (state)
		ret = veml6031x00_event_enable();
	else
		ret = veml6031x00_even_disable();

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


> @@ -549,11 +948,78 @@ 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);
> +
> +	if (state == data->trig_en)
> +		return 0;
> +
> +	ret = veml6031x00_set_interrupt(data, state);
> +	if (ret)
> +		return ret;
> +
> +	/* The AF bit must be updated before updating AF_TRIG */
> +	ret = regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				 VEML6031X00_CONF0_AF,
> +				 FIELD_PREP(VEML6031X00_CONF0_AF, state));
> +	if (ret) {
> +		veml6031x00_set_interrupt(data, !state);
> +
> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				 VEML6031X00_CONF0_AF_TRIG,
> +				 FIELD_PREP(VEML6031X00_CONF0_AF_TRIG, state));
> +	if (ret) {
> +		regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				   VEML6031X00_CONF0_AF,
> +				   FIELD_PREP(VEML6031X00_CONF0_AF, !state));
> +		veml6031x00_set_interrupt(data, !state);

This dance vs a goto is I guess due to the mutex.  I'd clean it up
by using a helper function for the stuff done under the guard(). The
helper can do goto based cleanup and avoid repetition plus reduce chance
of missing cleaning something up on error.  The outer function can
still use guard().

> +
> +		return ret;
> +	}
> +
> +	data->trig_en = state;
> +
> +	return 0;
> +}

>  
> +static int veml6031x00_setup_irq(struct i2c_client *i2c, struct iio_dev *iio)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +
> +	data->trig = devm_iio_trigger_alloc(dev, "%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(dev, data->trig);
> +	if (ret)
> +		return ret;
> +
> +	iio->trig = iio_trigger_get(data->trig);

Sashiko is correct that we loose a reference here on error and right now
there is no IIO core infrastructure to solve this

Why are we setting a default trigger?   Userspace tools should be
fine looking for a data ready trigger, or choosing a different one if they
would prefer. Added advantage of not setting it here is the reference count
issue goes away :)

> +
> +	return devm_request_threaded_irq(dev, i2c->irq,
> +					NULL, veml6031x00_irq,
> +					IRQF_ONESHOT, iio->name, iio);
> +}