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

[Javier Carrasco]

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?

Thanks and best regards,
Javier