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

[Jonathan Cameron]

On Thu, 13 Aug 2026 14:43:26 +0200
"Javier Carrasco" <javier.carrasco.cruz@xxxxxxxxx> wrote:

> Hi Jonathan, thank you for your review to the series.
> 
> On Thu Aug 13, 2026 at 3:24 AM CEST, Jonathan Cameron wrote:
> > On Wed, 12 Aug 2026 22:27:43 +0200
> > Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> 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@xxxxxxxxx>
> >> @@ -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().
> >  
> 
> Yes, that was the reason why some code was duplicated. I will add a
> helper function with the __must_hold() annotation and
> lockdep_assert_held().
> 
> >> +
> >> +		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 :)
> >  
> 
> I will drop iio->trig = iio_trigger_get(data->trig) for v7.
> 
> I added it because it is (or at least, it was) a common practice in many IIO
> drivers to assign their own trigger, and in the end it is by far the most
> common use case. Of course, we're not going to touch existing drivers to
> remove that, but is it then something to be advised against in the future
> unless there is a good reason for it?

It makes sense if they also 'require' that trigger - but generally if 
a driver has dealt with the potential extra complexity of allowing other
triggers that is because they want to use them. As such a default has
no particular benefit.

I probably let a few defaults in over the years where this wasn't
the case though :(

Jonathan

> 
> Thanks and best regards,
> Javier