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

On Tue, Aug 18, 2026 at 06:10:51PM +0200, Javier Carrasco wrote:
> On Tue Aug 18, 2026 at 4:01 PM CEST, Andy Shevchenko wrote:

...

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

The device maybe in these states here:
- powered off (by runtime PM) ret == 0
- powered on (by some previous activity) ret > 0
- always on (PM is disabled by user space, for example) ret < 0

Are you telling that the third case is impossible? (Note that autosuspend in
this case is irrelevant.)

...

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

I see, then make it clear by splitting assignment.

> >> +	int ret, val;
> >
> > Why is 'val' signed?

	regval = 0;

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

-- 
With Best Regards,
Andy Shevchenko