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

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

On Tue Jun 2, 2026 at 2:33 PM CEST, Jonathan Cameron wrote:
>> >> 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.
>

According to the application note:

It can be helpful for an application to be able to differentiate between light
sources and react accordingly. The additional IR channel offers the
possibilities to do a light source differentiation based on the IR content
measured with the IR channel. A ratio between the IR and ALS channel can
be calculated to determine the amount of IR light within the light source’s
spectrum. This easily allows, for example halogen bulb to be kept apart from
an LED light.

Probably not the most useful IR channel out there, but there's still
something you could do with it :D

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

I will add a devm action for the events to decrement the usage count if
events are enabled i.e. the usage count was incremented.

Best regards,
Javier