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

On Mon Aug 10, 2026 at 5:32 PM CEST, Andy Shevchenko wrote:
> On Fri, Aug 07, 2026 at 03:51:55PM +0200, Javier Carrasco 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.
>
> ...
>
...

>> +static int veml6031x00_write_th(struct iio_dev *iio, int val, int val2, int dir)
>> +{
>> +	struct veml6031x00_data *data = iio_priv(iio);
>> +	__le16 regval = cpu_to_le16(val);
>
> There is no technical need to assign it here, especially if the below
> validation won't pass, but it doesn't have any side effects, so I guess
> it's fine.
>

I will assign it after the val check instead.

>> +	int ret;
>> +
>> +	if (val < 0 || val > U16_MAX || val2)
>> +		return -EINVAL;
>> +
>> +	if (dir == IIO_EV_DIR_RISING) {
>> +		ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WH_L,
>> +					&regval, sizeof(regval));
>> +		if (ret)
>> +			dev_dbg(regmap_get_device(data->regmap),
>> +				"Failed to set high threshold %d\n", ret);
>> +	} else {
>> +		ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WL_L,
>> +					&regval, sizeof(regval));
>> +		if (ret)
>> +			dev_dbg(regmap_get_device(data->regmap),
>> +				"Failed to set low threshold %d\n", ret);
>> +	}
>> +
>> +	return ret;
>> +}
>
> ...
>
>> +static int veml6031x00_set_interrupt(struct veml6031x00_data *data, bool state)
>> +	__must_hold(&data->irq_lock)
>
> The sparse annotations is fine, but lockdep one is even better.
>

I have seen a few instances in a number of subsystems where both are used
at the same time. What is the preferred alternative here?

>> +{
>> +	int ret;
>> +
>> +	if (state) {
>> +		data->int_users++;
>> +		if (data->int_users > 1)
>> +			return 0;
>> +	} else {
>> +		data->int_users--;
>> +		if (data->int_users > 0)
>> +			return 0;
>> +	}
>> +
>> +	ret = regmap_field_write(data->rf.int_en, state);
>> +	if (ret) {
>> +		if (state)
>> +			data->int_users--;
>> +		else
>> +			data->int_users++;
>> +	}
>> +
>> +	return ret;
>> +}
>

Best regards,
Javier