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

On Wed, Aug 12, 2026 at 10:27:43PM +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_set_interrupt(struct veml6031x00_data *data, bool state)
> +	__must_hold(&data->irq_lock)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&data->irq_lock);

This needs another include.

> +	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;
> +}

...

> +	ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WL_L, &regval,
> +				sizeof(regval));

I always suggest to go with logical split.

	ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WL_L,
				&regval, sizeof(regval));

With shortened name and

	struct regmap *map = data->regmap;

this can be reduced to

	ret = regmap_bulk_write(map, VEML6031X00_REG_WL_L, &val, sizeof(val));

which is perfectly under 80 limit.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set low threshold\n");
> +
> +	regval = cpu_to_le16(U16_MAX);
> +	ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WH_L, &regval,
> +				sizeof(regval));

Ditto and so on...

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set high threshold\n");

...

> +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);
> +
> +	return devm_request_threaded_irq(dev, i2c->irq,
> +					NULL, veml6031x00_irq,

	return devm_request_threaded_irq(dev, i2c->irq, NULL, veml6031x00_irq,

...is perfectly one line and keeps be split logically.

> +					IRQF_ONESHOT, iio->name, iio);
> +}

...

>  	pm_runtime_get_noresume(dev);
>  
> +	ret = devm_add_action_or_reset(dev, veml6031x00_disable_event_action, data);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to add event cleanup action\n");
> +		goto err_pm_put;

No, this is (I am pretty sure) wrong. It's a red flag if one sees a goto after devm.

> +	}

-- 
With Best Regards,
Andy Shevchenko