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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Tue, Aug 18, 2026 at 01:34:29PM +0200, Javier Carrasco wrote:
> The device provides a shared interrupt line to notify events and
> data ready.
> 
> Add support for configurations with and without an interrupt line,
> providing events and trigger support when an interrupt line is available.

...

> +static int veml6031x00_write_period(struct iio_dev *iio, int val)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +
> +	if (val < 0 || val > 8 || hweight8(val) != 1)
> +		return -EINVAL;
> +
> +	return regmap_field_write(data->rf.pers, ffs(val) - 1);

That ffs() was bothering me all these review rounds. Now I got the idea how to
improve

	if (val < 0 || val > 8 || !is_power_of_2(val))
		return -EINVAL;

	return regmap_field_write(data->rf.pers, ilog2(val));

> +}

...

> +static irqreturn_t veml6031x00_irq(int irq, void *private)
> +{
> +	struct iio_dev *iio = private;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	s64 timestamp;
> +	unsigned int regval;
> +	int ret;
> +	bool trigger_poll;
> +
> +	ret = pm_runtime_get_if_active(dev);
> +	if (ret <= 0)

< 0 seems too much to me. If there is disabled runtime PM (and supposedly
device is always on) this prevents from getting events.

> +		return IRQ_NONE;
> +
> +	scoped_guard(mutex, &data->irq_lock) {
> +		ret = regmap_read(data->regmap, VEML6031X00_REG_INT, &regval);
> +		if (ret) {
> +			dev_dbg(dev, "Failed to read interrupt register %d\n", ret);
> +			pm_runtime_put(dev);
> +			return IRQ_NONE;
> +		}
> +
> +		if (!(regval & VEML6031X00_INT_MASK)) {
> +			pm_runtime_put(dev);
> +			return IRQ_NONE;
> +		}
> +
> +		timestamp = iio_get_time_ns(iio);

> +		if ((regval & (VEML6031X00_INT_TH_H | VEML6031X00_INT_TH_L)) &&
> +		    data->ev_en) {

With swapped operands it might be easier to follow.

		if (data->ev_en &&
		    (regval & (VEML6031X00_INT_TH_H | VEML6031X00_INT_TH_L))) {

> +			if (regval & VEML6031X00_INT_TH_H)
> +				iio_push_event(iio,
> +					       IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +								    IIO_EV_TYPE_THRESH,
> +								    IIO_EV_DIR_RISING),
> +					       timestamp);
> +			if (regval & VEML6031X00_INT_TH_L)
> +				iio_push_event(iio,
> +					       IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +								    IIO_EV_TYPE_THRESH,
> +								    IIO_EV_DIR_FALLING),
> +					       timestamp);
> +		}
> +
> +		trigger_poll = (regval & VEML6031X00_INT_DRDY) && data->trig_en;

Ditto.

> +	}
> +
> +	/*
> +	 * iio_trigger_poll_nested() must be called with irq_lock released:
> +	 * it runs trig_handler() synchronously in this thread, which calls
> +	 * reenable() on completion, and that callback also takes irq_lock.
> +	 * iio_pollfunc_store_time() is not called for our own trigger, so the
> +	 * timestamp is stored here.
> +	 */
> +	if (trigger_poll) {
> +		iio->pollfunc->timestamp = timestamp;
> +		iio_trigger_poll_nested(data->trig);
> +	}

> +	pm_runtime_put(dev);

With the above this needs to be conditional like

	if (pm_status > 0)
		pm_runtime_put(dev);

> +	return IRQ_HANDLED;
> +}

...

> +static int veml6031x00_hw_init(struct veml6031x00_data *data)
> +{
> +	struct regmap *map = data->regmap;
> +	struct device *dev = regmap_get_device(map);

> +	__le16 regval = 0;

Redundant assignment.

> +	int ret, val;

Why is 'val' signed?

> +	ret = regmap_bulk_write(map, VEML6031X00_REG_WL_L, &regval, sizeof(regval));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set low threshold\n");
> +
> +	regval = cpu_to_le16(U16_MAX);
> +	ret = regmap_bulk_write(map, VEML6031X00_REG_WH_L, &regval, sizeof(regval));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set high threshold\n");
> +
> +	ret = regmap_field_write(data->rf.int_en, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, VEML6031X00_REG_INT, &val);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to clear interrupts\n");
> +
> +	return 0;
> +}

...

> +static int veml6031x00_init_iiodev(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;
> +
> +	iio->name = data->chip->name;
> +	iio->modes = INDIO_DIRECT_MODE;
> +
> +	if (!i2c->irq) {
> +		iio->channels = veml6031x00_channels;
> +		iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
> +		iio->info = &veml6031x00_info_no_irq;
> +
> +		return 0;
> +	}
> +
> +	iio->channels = veml6031x00_channels_irq;
> +	iio->num_channels = ARRAY_SIZE(veml6031x00_channels_irq);
> +	iio->info = &veml6031x00_info;
> +
> +	ret = veml6031x00_setup_irq(i2c, iio);
> +	if (ret)
> +		return ret;

> +	ret = devm_add_action_or_reset(dev, veml6031x00_disable_event_action, data);
> +	if (ret)
> +		return ret;
> +
> +	return 0;

	return devm_add_action_or_reset(...);

> +}

-- 
With Best Regards,
Andy Shevchenko