Re: [PATCH v4 2/2] iio: light: veml3328: add support for new device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The runtime PM autosuspend timer is not started during probe, preventing the device from entering a low-power state until its first userspace access.
- [Medium] SCALE attributes are incorrectly exported as `shared_by_type` instead of `shared_by_all` for a global hardware setting.
--

commit 3c7cc50c08671c37a310d6d5f49f51c1dd186420
Author: Joshua Crofts <joshua.crofts1@gmail.com>

    iio: light: veml3328: add support for new device
    
    Add support for the Vishay VEML3328 RGB/IR light sensor communicating
    via I2C (SMBus compatible). Also add a new entry for said driver into
    Kconfig and Makefile.

> diff --git a/drivers/iio/light/veml3328.c b/drivers/iio/light/veml3328.c
> new file mode 100644
> index 0000000000000..7ff1753925c47
> --- /dev/null
> +++ b/drivers/iio/light/veml3328.c
[ ... ]
> +#define VEML3328_CHAN_SPEC(_color, _addr) { \
> +	.type = IIO_INTENSITY, \
> +	.modified = 1, \
> +	.channel2 = IIO_MOD_LIGHT_##_color, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +	.info_mask_shared_by_type_available = 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), \
> +	.address = _addr, \
> +}
> +
> +static const struct iio_chan_spec veml3328_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE),

[Severity: Medium]
Will this export two separate sysfs scale attributes that secretly control the
same global hardware register?

Since the sensor gain is controlled globally in VEML3328_REG_CONF, and the
driver registers both IIO_LIGHT and IIO_INTENSITY channels, using
info_mask_shared_by_type will create separate in_light_scale and
in_intensity_scale attributes. Modifying one will alter the hardware gain for
the other, which might break userspace expectations. 

Should this use info_mask_shared_by_all instead, similar to how INT_TIME is
configured?

> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.address = VEML3328_REG_DATA_G,
> +	},
[ ... ]
> +	indio_dev->name = "veml3328";
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &veml3328_info;
> +	indio_dev->channels = veml3328_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(veml3328_channels);
> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 2000);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

[Severity: Medium]
Will the device fail to enter autosuspend after probe?

The driver powers up the sensor earlier in veml3328_probe() and sets the PM
state to RPM_ACTIVE here, but it never increments the PM usage count or queues
an idle check. As a result, the PM core won't start the autosuspend timer
when pm_runtime_enable() is called.

Could this leave the sensor fully powered indefinitely until a userspace read
finally acquires and releases a PM reference? Should there be a call to
pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() at the end of
probe, paired with an earlier pm_runtime_get_noresume()?

> +
> +static int veml3328_runtime_suspend(struct device *dev)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-veml3328-v4-0-f9b65d71d917@gmail.com?part=2