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

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

Hello Andy, thank you again for your thorough review.

On Thu Aug 13, 2026 at 8:59 AM CEST, Andy Shevchenko wrote:
> On Wed, Aug 12, 2026 at 10:27:41PM +0200, Javier Carrasco wrote:
>> These sensors provide two light channels (ALS and IR), I2C communication
>> and a multiplexed interrupt line to signal data ready and configurable
>> threshold alarms.
>
>> This first implementation provides basic functionality (measurement
>> configuration, raw reads and ID validation) and defines the different
>> register regions in preparation for extended features in the subsequent
>> patches of the series.
>
> This paragraph needs to be rephrased. In the current form it suits cover letter
> and not the commit message. Here, just list the features supported.
>
> The "the subsequent patches of the series." in the commit message is very
> ambiguous. What patch series? Which patches? Are they landed in the upstream?
> If yes, which commit IDs? If not, when if ever? Et cetera! Usually it can be
> simply said "The other features may be implemented later on."
>

This commit message will be re-worked. Up to v3 the driver was sent as a
single patch, and as I split it, it stayed a bit too dependent of what I
was sending as separate patches. But there is no real need for it.

...
>
>> +	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");
>
> Is debugfs access already enabled for regmap after this call? Perhaps you want
> mutex to be initialised before that?
>

Could you please explain what you are trying to avoid? Even if the
debugfs is already enabled for regmap at this point, what is the
possible race condition? The IIO device is still not registered at this
point, and the registers are set to their right values in _hw_init()
after the mutexes were initialized. Moving the mutex initialization a
couple of lines towards the top is not an issue, but I would like to
understand the reasoning behind.

>> +	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);
>> +
>> +	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);
>
> Hmm... This is usually a red flag to see a goto after devm_*() calls.
>

Could you please elaborate on this? I am aware of the dangers of a goto
after the cleanup attribute, but devm_*() calls work on a different
scope, not local to the function. What could go wrong? This goto is just
a way not to repeat pm_runtime_put_noidle(); return ret; and I am not
strongly against repeating these two lines wherever there is a goto, but
I am not sure what we are avoiding in this case.

>> +	return ret;
>> +}

Best regards,
Javier