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

["Javier Carrasco" <javier.carrasco.cruz@gmail.com>]

Hi Andy, thank you for your review. Even if no more stuff comes, I would
like to update the code in v8 with your suggestions before applying it
if nothing speaks against it.

A couple of comments to the feedback I'd like to discuss.

On Tue Aug 18, 2026 at 4:01 PM CEST, Andy Shevchenko wrote:

...

>> +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.
>

I am not sure if I get this. A reference is unconditionally acquired
when events are enabled as well as in buffer_preenable, and also in the
probe before interrupts are enabled. Runtime PM should be active at this
point. If not, the interrupt should not come from the device, even if it
was on (e.g. before autosuspend kicks in). But maybe I am missing
something?

>
...

>> +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.
>

regval is assigned here to write that value in the first instruction
(regmap_bulk_write()) after the variable declaration.

>> +	int ret, val;
>
> Why is 'val' signed?
>
>> +	ret = regmap_bulk_write(map, VEML6031X00_REG_WL_L, &regval, sizeof(regval));

Here.

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

Best regards,
Javier