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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The device will not enter runtime suspend until it is accessed at least once by userspace, wasting power indefinitely.
- [Medium] Mixing `goto` with `guard()` cleanup in the same function.
- [Medium] Unconditional 400ms blocking sleep on runtime resume.
--

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

    iio: light: veml3328: add support for new device

This commit introduces a new driver for the Vishay VEML3328 RGB/IR light sensor
communicating via I2C. It includes runtime PM support and basic IIO channel
configurations.

> diff --git a/drivers/iio/light/veml3328.c b/drivers/iio/light/veml3328.c
> new file mode 100644
> index 0000000000000..9eb5429c813d9
> --- /dev/null
> +++ b/drivers/iio/light/veml3328.c

[ ... ]

> +static int veml3328_power_up(struct veml3328_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, VEML3328_REG_CONF,
> +				 VEML3328_SHUTDOWN, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	fsleep(veml3328_it_times[3][1]);

Does this unconditional 400ms sleep cause a latency regression when the device
is resuming?

Because this function is called from the runtime resume callback, this adds a
400ms latency to every operation that wakes the device from suspend, including
simple configuration reads or writes via sysfs.

It also unconditionally sleeps for the maximum possible integration time
instead of the currently configured one.

Should the measurement delay be handled in veml3328_read_raw() when actually
acquiring data, rather than in the general resume callback?

> +
> +	return 0;
> +}

[ ... ]

> +static int veml3328_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct veml3328_data *data = iio_priv(indio_dev);
> +	struct regmap *regmap = data->regmap;
> +	unsigned int reg_val;
> +	int ret;
> +	int reg;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(regmap, chan->address, &reg_val);
> +		if (ret < 0)
> +			goto exit;

Is it intended to mix goto-based error handling with guard() in the same
function?

The kernel cleanup guidelines explicitly state that mixing goto-based cleanup
labels with guard() declarations in the same function creates confusing
ownership semantics.

Could the goto statements inside the switch blocks be trivially replaced with
break to eliminate the exit label and comply with the guidelines?

> +
> +		*val = reg_val;
> +		ret = IIO_VAL_INT;
> +		break;

[ ... ]

> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +exit:
> +	pm_runtime_put_autosuspend(data->dev);
> +
> +	return ret;
> +}

[ ... ]

> +static int veml3328_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct veml3328_data *data = iio_priv(indio_dev);
> +	struct regmap *regmap = data->regmap;
> +	int ret;
> +	int i;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (val != 0) {
> +			ret = -EINVAL;
> +			goto exit;

This has the same issue with mixing goto and guard() as veml3328_read_raw().

> +		}
> +
> +		for (i = 0; i < ARRAY_SIZE(veml3328_it_times); i++) {
> +			if (veml3328_it_times[i][1] == val2)
> +				break;
> +		}

[ ... ]

> +static int veml3328_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct veml3328_data *data;

[ ... ]

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

Will this device fail to enter runtime suspend until it is accessed at least
once by userspace?

Because the usage count starts at 0 and no idle request is explicitly queued,
the PM core does not automatically start the autosuspend timer. The device
might remain fully powered on indefinitely until the first userspace access
triggers a get/put cycle.

Should the driver queue an idle check by calling pm_request_idle(dev) or
perform a pm_runtime_get_noresume() and pm_runtime_put_autosuspend() sequence
at the end of probe to initiate autosuspend?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-veml3328-v1-0-1d4b663e2fe3@gmail.com?part=2