All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/4] iio: light: add support for veml6031x00 ALS series
@ 2026-05-24 21:53 Javier Carrasco
  2026-05-24 21:53 ` [PATCH v3 1/4] dt-bindings: iio: light: veml6030: add " Javier Carrasco
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Javier Carrasco @ 2026-05-24 21:53 UTC (permalink / raw)
  To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
	Krzysztof Kozlowski, Conor Dooley, Rishi Gupta, David Lechner,
	Nuno Sá, Andy Shevchenko, Matti Vaittinen
  Cc: linux-iio, devicetree, linux-kernel, Javier Carrasco,
	Jonathan Cameron, Krzysztof Kozlowski

These ambient light sensors with I2C interface provide two light
channels (ALS and IR), high/low threshold alarms with configurable
persistence, and a data ready signal.

The devices covered by this driver have the same resolution, and they
share most of their functionality. These are the differences between
them (note that the x belongs to their names, and it is not a wildcard):

 - Device ID: accessible via two 8-byte registers, different values for
   veml6031x00/veml6031x01 and veml60311x00/veml60311x01.
 - I2C address: same grouping, 0x29 and 0x10 I2C addresses.
 - AEC qualification: AEC-Q100 for veml6031x00/veml60311x00 and
   AEC-Q101 for veml6031x01/veml60311x01.

The alarms and the data ready signals share the interrupt pin, and an
interrupt status register must be accessed to identify the source. Such
multiplexing is not new in IIO, and I have followed existing examples
for it. The persistence setting (own attribute) to trigger the alarms
uses the pattern that has already been used for the veml6030.

The device configuration is in general documented in the datasheet and
the application note. There is an exception, though: the activation of
the "active force" mode that is required for the data ready signal must
be carried out in two steps even though the affected bits are located in
the same register: first ALS_AF (active force mode enable) must be set,
and then ALS_TRIG (active force trigger setting) must be enabled. I have
added a brief commentary in the code to explain this behavior, which has
been confirmed by the manufacturer.

The only functionality that has not been implemented yet is the x0.66
gain (and its x0.165 counterpart when PD_DIV=1), which makes the gts
helpers less usable due to the conversions required. It is indeed an
uncommon gain to use (there are x0.5 and x0.125 gains) with no known
use-case at the moment that justifies making adjustments to the gts
helpers or adding artificial conversions to make it work.

This driver has been tested with the four supported devices separately
as well as in pairs where the I2C addresses don't overlap.

To: Jonathan Cameron <jic23@kernel.org>
To: Lars-Peter Clausen <lars@metafoo.de>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Rishi Gupta <gupt21@gmail.com>
To: David Lechner <dlechner@baylibre.com>
To: Nuno Sá <nuno.sa@analog.com>
To: Andy Shevchenko <andy@kernel.org>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Changes in v3:
- Move veml6030 fixes to a separate patch stack.
- Use C99 initializers for i2c_device_id.
- Split driver code into multiple patches to ease its review.
- Rework locking to get rid of atomic increment/decrement ops.
- Fix error paths in pm_runtime operations.
- Use IIO_DEV_ACQUIRE_DIRECT_MODE for single read/write ops.
- Link to v2: https://lore.kernel.org/r/20260513-veml6031x00-v2-0-4703ca661a1d@gmail.com

Changes in v2:
- Add commit to fix bug in veml6030.c (channel type when pushing
  events) and remove dead code.
- Use gts helpers to simplify operations.
- Drop unused gain_idx.
- Build INT_MASK as an OR operation of the involved bits.
- Format arrays to follow the desired standard for IIO.
- Directly return function result as the last operation within another
  function instead of 'ret = x; if (ret) return ret; return 0;'.
- Fix some spacing (double space, tab for alignemnt in info struct).
- Use sizeof() for __le16 reg instead of 2.
- Return an error if the part ID could not be read.
- Spell out sd -> shutdown.
- Use devm_mutex_init() instead of mutex_init().
- Avoid using conditional guard, use claim/release instead.
- Access integration times from the global array to get and set the
  integration time instead of using a switch.
- Simplify read of available periods (persistence).
- Drop IRQF_TRIGGER_FALLING in the threaded irq request.
- Add regmap ranges.
- Link to v1: https://lore.kernel.org/r/20241126-veml6031x00-v1-0-4affa62bfefd@gmail.com

---
Javier Carrasco (4):
      dt-bindings: iio: light: veml6030: add veml6031x00 ALS series
      iio: light: add support for veml6031x00 ALS series
      iio: light: veml6031x00: add support for triggered buffers
      iio: light: veml6031x00: add support for events and trigger

 .../bindings/iio/light/vishay,veml6030.yaml        |   23 +-
 MAINTAINERS                                        |    6 +
 drivers/iio/light/Kconfig                          |   14 +
 drivers/iio/light/Makefile                         |    1 +
 drivers/iio/light/veml6031x00.c                    | 1197 ++++++++++++++++++++
 5 files changed, 1240 insertions(+), 1 deletion(-)
---
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
change-id: 20241109-veml6031x00-aa9463da064a

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread
* Fwd: Re: [PATCH v3 4/4] iio: light: veml6031x00: add support for events and trigger
@ 2026-05-25  9:02 Javier Carrasco
  2026-05-26 17:06 ` Jonathan Cameron
  0 siblings, 1 reply; 18+ messages in thread
From: Javier Carrasco @ 2026-05-25  9:02 UTC (permalink / raw)
  To: linux-iio

Forwarded message from Javier Carrasco on Mon May 25, 2026 at 10:59 AM:
On Mon May 25, 2026 at 2:13 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
>
> New issues:
> - [High] Cache pollution of AF_TRIG causes unintended conversions and unnecessary I2C reads.
> - [High] The trigger reference acquired via iio_trigger_get() is leaked on probe error paths and driver unbind.
> - [High] PM runtime usage counter leaks if the driver unbinds while an event is enabled.
> - [High] Sysfs read/write functions and trigger operations access hardware without acquiring a PM runtime reference.
> - [High] Out-of-bounds array write in veml6031x00_trig_handler when the timestamp channel is enabled.
> - [Medium] Dead event sysfs attributes are exposed when no IRQ is present.
>
> Pre-existing issues:
> - [High] Unbalanced double-put of the runtime PM usage counter on driver unbind.
> - [High] The shutdown action accesses I2C registers after PM runtime has been disabled.
> --
>
> commit cbdd69d1454af4788d5d18a23d3fb6854c9981e9
> Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>
> iio: light: veml6031x00: add support for events and trigger
>
> The device provides a shared interrupt line for to notify events and
> data ready, which can be used as a trigger. The interrupt line is not a
> requirement for the device to work. Implement variants for the cases
> whether the interrupt line is provided or not.
>
>> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
>> index 9968d4414dc96..c57aaa29abf30 100644
>> --- a/drivers/iio/light/veml6031x00.c
>> +++ b/drivers/iio/light/veml6031x00.c
>
> [ ... ]
>
>> @@ -136,6 +189,8 @@ static const struct iio_chan_spec veml6031x00_channels[] = {
>>  		.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),
>> +		.event_spec = veml6031x00_event_spec,
>> +		.num_event_specs = ARRAY_SIZE(veml6031x00_event_spec),
>
> [Severity: Medium]
> Will this expose dead event sysfs attributes when no IRQ is present?
>
> When i2c->irq is 0, the driver registers veml6031x00_info_no_irq which lacks
> event callbacks. However, this channel array unconditionally defines
> event_spec and num_event_specs. The IIO core will parse these and expose
> event sysfs files. Reading or writing these files will fail with -EINVAL
> because the callbacks are NULL, which violates the sysfs API contract.
>
> [ ... ]
>

@Jonathan: is this how it really works? How can I then dynamically
define even_spec to account for it?

>> +static int veml6031x00_read_event_val(struct iio_dev *iio,
>> +				      const struct iio_chan_spec *chan,
>> +				      enum iio_event_type type,
>> +				      enum iio_event_direction dir,
>> +				      enum iio_event_info info,
>> +				      int *val, int *val2)
>> +{
>
> [Severity: High]
> Are PM runtime references needed here before accessing hardware?
>
> This function, as well as veml6031x00_write_event_val() and
> veml6031x00_set_trigger_state(), performs I2C accesses via regmap without
> first calling pm_runtime_resume_and_get(). If the device is
> runtime-suspended and its PM domain has powered it off, will these I2C
> accesses fail or time out?
>
> [ ... ]
>

As I said for [2/4], reading registers in shutdown mode is absolutely
fine.

>> +static int veml6031x00_write_event_config(struct iio_dev *iio,
>> +					  const struct iio_chan_spec *chan,
>> +					  enum iio_event_type type,
>> +					  enum iio_event_direction dir,
>> +					  bool state)
>> +{
>> +	struct veml6031x00_data *data = iio_priv(iio);
>> +	int ret;
>> +
>> +	guard(mutex)(&data->lock);
>> +
>> +	/* avoid multiple increments/decrements from one source */
>> +	if (state == data->ev_en)
>> +		return 0;
>> +
>> +	if (state) {
>> +		ret = pm_runtime_resume_and_get(data->dev);
>
> [Severity: High]
> Could this leak the PM runtime usage counter if the driver unbinds?
>
> Enabling an event increments the PM runtime usage counter here. If the
> driver is unbound via sysfs while an event is enabled, the IIO core does
> not automatically disable the event. Since there is no unbind cleanup
> mechanism to release the pending PM reference when data->ev_en is true,
> does this keep the device powered on indefinitely?
>
> [ ... ]
>

@Jonathan: Does the IIO core let the event enabled? What is the
approach to clear it then?

>> +/* AF_TRIG is reset by hardware, but the rest of the fields are persistent */
>> +static int veml6031x00_set_af_trig(struct veml6031x00_data *data, bool state)
>> +{
>> +	regcache_drop_region(data->regmap, VEML6031X00_REG_CONF0,
>> +			     VEML6031X00_REG_CONF0);
>> +
>> +	return regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
>> +				  VEML6031X00_CONF0_AF_TRIG,
>> +				  FIELD_PREP(VEML6031X00_CONF0_AF_TRIG, state));
>> +}
>
> [Severity: High]
> Can this cause unintentional active force conversions due to cache pollution?
>
> This sets the self-clearing AF_TRIG bit using regmap_update_bits().
> Because CONF0 is not marked as a volatile register, regmap_update_bits()
> writes AF_TRIG=1 and caches it. Any subsequent read-modify-write on CONF0
> (e.g., toggling the interrupt or changing integration time) will read the
> cached AF_TRIG=1 and write it back to hardware, unintentionally triggering
> an active force conversion and causing hardware state corruption.
>
> [ ... ]
>

I will define the CONF0 register as volatile to account for this.

>>  static irqreturn_t veml6031x00_trig_handler(int irq, void *p)
>>  {
>>  	struct iio_poll_func *pf = p;
>>  	struct iio_dev *iio = pf->indio_dev;
>>  	struct veml6031x00_data *data = iio_priv(iio);
>>  	int ch, ret, i = 0;
>>  	struct {
>>  		__le16 chans[2];
>>  		aligned_s64 timestamp;
>>  	} scan = { };
>>
>>  	if (*iio->active_scan_mask == (BIT(VEML6031X00_SCAN_ALS) |
>>  				       BIT(VEML6031X00_SCAN_IR))) {
>>  		ret = regmap_bulk_read(data->regmap,
>>  				       VEML6031X00_REG_ALS_L,
>>  				       &scan.chans, sizeof(scan.chans));
>>  		if (ret)
>>  			goto done;
>>  	} else {
>>  		iio_for_each_active_channel(iio, ch) {
>>  			ret = regmap_bulk_read(data->regmap,
>>  					       iio->channels[ch].address,
>>  					       &scan.chans[i++],
>
> [Severity: High]
> Does this loop cause an out-of-bounds array write if the timestamp
> channel is enabled?
>
> The iio_for_each_active_channel() loop does not exclude the soft timestamp
> channel. If the user enables ALS, IR, and TIMESTAMP channels, the loop
> iterates 3 times. However, the scan.chans array is declared as
> __le16 chans[2]. Processing the timestamp channel increments the index i
> to 2 and writes to scan.chans[2], causing an out-of-bounds memory write.
>
> Furthermore, because the timestamp channel's address is 0, this incorrectly
> attempts a bogus I2C read from VEML6031X00_REG_CONF0.
>
> [ ... ]
>

Not sure if this is a real issue, and the address for channel 0 is not
CONF0 because it is defined in iio_chan_spec
veml6031x00_channels.address.

>> +static int veml6031x00_setup_irq(struct i2c_client *i2c, struct iio_dev *iio)
>> +{
>> +	struct veml6031x00_data *data = iio_priv(iio);
>> +	struct device *dev = data->dev;
>> +	int ret;
>> +
>> +	data->trig = devm_iio_trigger_alloc(dev, "%s-drdy%d", iio->name,
>> +					    iio_device_id(iio));
>> +	if (!data->trig)
>> +		return -ENOMEM;
>> +
>> +	data->trig->ops = &veml6031x00_trigger_ops;
>> +	iio_trigger_set_drvdata(data->trig, iio);
>> +
>> +	ret = devm_iio_trigger_register(dev, data->trig);
>> +	if (ret)
>> +		return ret;
>> +
>> +	iio->trig = iio_trigger_get(data->trig);
>
> [Severity: High]
> Is the trigger reference acquired here leaked on driver unbind?
>
> Calling iio_trigger_get() increments the trigger device's refcount and the
> driver module's refcount. If the driver fails probe afterwards or is unbound
> later, iio_trigger_put() is never called because there is no devres action
> registered for it and no custom remove callback. Does this permanently leak
> the trigger device reference and prevent the module from being unloaded?
>
> [ ... ]
>

Same here. I believe that only industrialio-trigger.c makes use of
iio_trigger_put().

>> @@ -618,6 +1039,10 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>
> [ ... ]
>
>>  	ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
>
> [Severity: High]
> This is a pre-existing issue, but does this shutdown action access I2C
> registers after PM runtime has been disabled?
>
> This action is registered via devm_add_action_or_reset() before PM runtime
> is enabled. On teardown, devm actions run in reverse, meaning PM runtime is
> disabled before the shutdown action runs. The shutdown action then attempts
> I2C writes while PM runtime is disabled. If the device was suspended, it
> cannot be resumed, potentially causing I2C transfer failures.
>
> [ ... ]
>
>>  	pm_runtime_put_autosuspend(dev);
>
> [Severity: High]
> This is a pre-existing issue, but is there an unbalanced double-put of the
> runtime PM usage counter on driver unbind?
>
> The probe function registers a cleanup action to drop the PM reference via
> devm_pm_runtime_get_noresume(dev). However, this explicit put at the end
> of probe means the devm action will drop the reference a second time on
> unbind, causing a PM usage counter underflow that breaks the PM runtime
> state machine.

As I said for [2/4], there won't be any counter underflow that way.

Best,
Javier



^ permalink raw reply	[flat|nested] 18+ messages in thread
end of thread, other threads:[~2026-06-05 18:08 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-24 21:53 [PATCH v3 0/4] iio: light: add support for veml6031x00 ALS series Javier Carrasco
2026-05-24 21:53 ` [PATCH v3 1/4] dt-bindings: iio: light: veml6030: add " Javier Carrasco
2026-05-24 22:11   ` sashiko-bot
2026-05-24 21:53 ` [PATCH v3 2/4] iio: light: add support for " Javier Carrasco
2026-05-24 22:53   ` sashiko-bot
2026-05-25  0:29     ` Javier Carrasco
2026-05-26 17:59   ` Jonathan Cameron
2026-05-26 18:05     ` Jonathan Cameron
2026-05-26 20:46       ` Javier Carrasco
2026-06-05 18:02     ` Andy Shevchenko
2026-06-05 18:08       ` Andy Shevchenko
2026-05-24 21:53 ` [PATCH v3 3/4] iio: light: veml6031x00: add support for triggered buffers Javier Carrasco
2026-05-24 23:29   ` sashiko-bot
2026-05-26 18:03   ` Jonathan Cameron
2026-05-24 21:53 ` [PATCH v3 4/4] iio: light: veml6031x00: add support for events and trigger Javier Carrasco
2026-05-25  0:13   ` sashiko-bot
2026-05-25  8:59     ` Javier Carrasco
  -- strict thread matches above, loose matches on Subject: below --
2026-05-25  9:02 Fwd: " Javier Carrasco
2026-05-26 17:06 ` Jonathan Cameron

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.