Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Jonathan Cameron]

On Mon, 01 Jun 2026 22:07:42 +0200
"Javier Carrasco" <javier.carrasco.cruz@xxxxxxxxx> wrote:

> Hi Jonathan, thanks again for your review.
> 
> On Mon Jun 1, 2026 at 12:21 PM CEST, Jonathan Cameron wrote:
> > On Sun, 31 May 2026 21:58:22 +0200
> > Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:
> >  
> >> These sensors provide two light channels (ALS and IR), I2C communication
> >> and a multiplexed interrupt line to signal data ready and configurable
> >> threshold alarms.
> >>
> >> This first implementation provides basic functionality (measurement
> >> configuration, raw reads and ID validation) and defines the different
> >> register regions in preparation for extended features in the subsequent
> >> patches of the series.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>  
> > A few comments from sashiko and a couple from me based on a fresh read.
> >
> >  
> >> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> >> new file mode 100644
> >> index 000000000000..6f9a7bad44d4
> >> --- /dev/null
> >> +++ b/drivers/iio/light/veml6031x00.c  
> >  
> >> +
> >> +static const struct iio_chan_spec veml6031x00_channels[] = {
> >> +	{
> >> +		.type = IIO_LIGHT,
> >> +		.address = VEML6031X00_REG_ALS_L,
> >> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> +				      BIT(IIO_CHAN_INFO_SCALE),
> >> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> >> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> >> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> >> +	},
> >> +	{
> >> +		.type = IIO_INTENSITY,
> >> +		.address = VEML6031X00_REG_IR_L,
> >> +		.modified = 1,
> >> +		.channel2 = IIO_MOD_LIGHT_IR,
> >> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> +				      BIT(IIO_CHAN_INFO_SCALE),  
> >
> > Can we move scale to shared_by_type?
> >
> > Thinking on some of the others, they should probably be shared_by_type as well
> > rather than shared_by_all. If we ever add buffered support and a timestamp
> > the integration_time doesn't apply to that.
> >
> > shared_by_all tends to only include things that are truely universal like
> > sampling_frequency.
> >  
> 
> I am not sure if I get this point. This device has a single IIO_LIGHT
> channel, and the scale only applies to it. Are info_mask_separate and
> info_mask_shared_by_type not the same in that case? I have seen that
> some drivers use both info_mask_separate for INFO_RAW, and
> info_mask_shared_by_type for INFO_INT_TIME and/or INFO_SCALE, but that
> could make more sense if there were multiple channels of the same type.
> What am I missing here?

Ah. I've been reading too many drivers. For some reason I thought this
had more intensity channels. My comment clearly garbage as you point out.
I maybe got thrown by how ALS sensors used to work.  They used IR only
and a clear window that covered both IR and the frequencies we need for
illumiance measurement. The light channel was then some combination of
the two. I guess they've figured out how to filter that IR out of that
these days.  That leaves me a little curious as to what applications actually
use the IR channel.

> 
> On the other hand, the integration time applies to both the IIO_LIGHT
> and IIO_INTENSITY channels, so I guess you are suggesting to add it
> to both channels as info_mask_shared_by_type because the timestamp
> is a channel itself. Moving it form shared_by_all to shared_by_type is
> alright, and I will add it to V5.

Yes, that one was correct.

> 
> >> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> >> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> >> +	},
> >> +};  

> 
> >> +
> >> +	ret = veml6031x00_hw_init(iio);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	pm_runtime_put_autosuspend(dev);  
> >
> > As sashiko shouts, this looks like runtime pm will underflow on remove.
> > Check it by removing your driver.  It doesn't actually result in any
> > problem, as the runtime pm subsystem just saturates at 0 on decrement.
> > Given that's tear down anyway maybe we don't care.  However, it's easy
> > enough to fix by using pm_runtime_get_no_resume() and a couple of
> > explicit calls to put it in error paths.
> >  
> 
> I took some time to audit this in detail, because although my
> expectations are that atomic_add_unless(usage_count, -1, 0) should make
> this shout a false positive. Expectations don't always meet reality. My
> expectations were based on the code, so I have added tracepoints to know
> exactly what's going on.
> 
> Scenario 1: Successful probe -> unbind driver.
> 
> devm_pm_runtime_get_noresume() increases usage_count, and the call to
> pm_runtime_put_autosuspend() decreases it. That is balanced, giving us
> usage_count = 0 and putting the device in power down mode as desired. If
> the device is then unbound, the devres action (a call to
> pm_runtime_put_noidle_action, which only calls pm_runtime_put_noidle)
> triggers a call to atomic_add_unless(&dev->power.usage_count, -1, 0).
> Given that the usage count is 0, nothing happens and there is no
> underflow. In fact, the underflow is not possible this way, and adding a
> remove function to check if usage_count is 0 and calling
> pm_runtime_put() is basically repeating what the devres action already
> offers for free.

Agreed on this analysis. From a readability point of view it is nice
to have balanced calls but as long as we only underflow / get clamped
in a path where we never increment again that's fine.  I'd add a comment
somewhere to say that's intentional.

> 
> Scenario 2: write_event_config -> unbind driver.
> 
> Sashiko says that pm_runtime_resume_and_get() increases usage_count, and
> there is no devres action associated to it. That is only partially
> correct, because the devres action from devm_pm_runtime_get_noresume()
> will be triggered when the driver is unbound. Actually, this is great
> because then pm_runtime_resume_and_get() gets balanced, and there is no
> need for a remove function to check again if usage_count is 0 or not. In
> this case, usage_count = 1 before unbinding the driver, and then the
> devres action is triggered when it gets unbound. Exactly what we want to
> have usage_count = 0.

This one I'm more dubious on.  Basically you are saying on remove we happen
to have an extra decrement for largely unrelated reasons so we are fine.
That to me smells fragile even if it works.  For instance someone tidying
up the imbalance in scenario 1 (which would seem reasonable to do from
a code understandability point of view) would break scenario 2.
I'd rather we had something explicit for this. Probably an devm action
that just checks for events being enabled at exit and disables them as part of
the tear down.

So not bugs as such, but fragile which is not good from maintainability
point of view.

Jonathan


> 
> >> +
> >> +	ret = devm_iio_device_register(dev, iio);
> >> +	if (ret)
> >> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
> >> +
> >> +	return 0;
> >> +}  
> >  
> >>  
> 
> Best regards,
> Javier