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

On Mon Aug 17, 2026 at 4:36 AM CEST, Jonathan Cameron wrote:
> On Thu, 13 Aug 2026 14:43:26 +0200
> "Javier Carrasco" <[email protected]> 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 <[email protected]> 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 <[email protected]>
>> >> @@ -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 :(

I bet there are way worse sins ;)

>
> Jonathan
>
>>
>> Thanks and best regards,
>> Javier