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

[Javier Carrasco]

On Thu Aug 13, 2026 at 3:04 AM CEST, Jonathan Cameron wrote:
...
>
>> +static int veml6031x00_probe(struct i2c_client *i2c)
>> +{
>> +	struct device *dev = &i2c->dev;
>> +	struct veml6031x00_data *data;
>> +	struct iio_dev *iio;
>> +	int ret;
>> +
>> +	iio = devm_iio_device_alloc(dev, sizeof(*data));
>> +	if (!iio)
>> +		return -ENOMEM;
>> +
>> +	data = iio_priv(iio);
>> +	i2c_set_clientdata(i2c, iio);
>> +
>> +	data->chip = i2c_get_match_data(i2c);
>> +	if (!data->chip)
>> +		return dev_err_probe(dev, -EINVAL, "Failed to get chip data\n");
>> +
>> +	data->regmap = devm_regmap_init_i2c(i2c, &veml6031x00_regmap_config);
>> +	if (IS_ERR(data->regmap))
>> +		return dev_err_probe(dev, PTR_ERR(data->regmap),
>> +				     "Failed to set regmap\n");
>> +
>> +	iio->name = data->chip->name;
>> +	iio->channels = veml6031x00_channels;
>> +	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
>> +	iio->modes = INDIO_DIRECT_MODE;
>> +	iio->info = &veml6031x00_info;
>> +
>> +	ret = devm_mutex_init(dev, &data->scale_lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = veml6031x00_regfield_init(data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to init regfield\n");
>> +
>> +	ret = devm_regulator_get_enable(dev, "vdd");
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
>> +
>> +	/* The device starts in power down mode by default */
>> +	ret = veml6031x00_set_power(data, true);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to power on the device\n");
>> +
>> +	ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to add shutdown action\n");
>> +
>> +	pm_runtime_set_autosuspend_delay(dev, 2000);
>> +	pm_runtime_use_autosuspend(dev);
>> +	ret = devm_pm_runtime_set_active_enabled(dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
>> +
>> +	pm_runtime_get_noresume(dev);
>> +
>> +	ret = veml6031x00_validate_part_id(data);
>> +	if (ret)
>> +		goto err_pm_put;
>> +
>> +	ret = veml6031x00_hw_init(iio);
>> +	if (ret)
>> +		goto err_pm_put;
>> +
>> +	pm_runtime_put_autosuspend(dev);
>
> This and the get_noresume() above smell like they are probably here to ensure
> a 1->0 transition for the runtime pm reference count and hence kick off the timer
> for autosuspend.   That shouldn't be necessary as there is code in the driver
> core to try a suspend. Look for pm_request_idle() calls in drivers/base/dd.c
>
> Note that we've had sashiko give what we have concluded in the past are false
> positives on this dance being required.
> Given you have device to hand, just probe it and check if it suspends.
>
> I haven't quite had the nerve to yet post a series removing this dance from
> drivers I can't test!
>

I tested that mechanism with several devices of this family, and it
worked consistently. I will remove the calls to pm_runtime_get_noresume()
and pm_runtime_put_autosuspend(), and with that all the goto jumps in the
probe function. I still believe that they are absolutely fine when
combined with devm_*() calls, but there is now a better reason to
remove them.

I may have some more IIO devices to validate it with different hardware,
but that's an experiment that will have to wait for a while... Anyway,
right now this dance is suboptimal but harmless, isn't it?

>> +
>> +	ret = devm_iio_device_register(dev, iio);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
>> +
>> +	return 0;
>> +
>> +err_pm_put:
>> +	pm_runtime_put_noidle(dev);
>> +	return ret;
>> +}

Best regards,
Javier