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

[Javier Carrasco]

On Fri Aug 14, 2026 at 9:49 AM CEST, Andy Shevchenko wrote:
> On Thu, Aug 13, 2026 at 11:46:32AM +0200, Javier Carrasco wrote:
>> 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:
>
> ...
>
>> >> +	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?
>
> That's my question to you. If you think there is none, then we are all good.
> Basically the Q is if that mutex (that is initialised later in the probe) is
> required for the device IO which might be possible immediately when regmap
> is registered.
>
>> 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.
>
> User may read (and depending on the driver case writing) the register map.
> Would it be a problem?
>

That is not a problem, as no assumptions about the original values are
made.

>> >> +	ret = devm_mutex_init(dev, &data->scale_lock);
>> >> +	if (ret)
>> >> +		return ret;
>
> ...
>
>> >> +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.
>
> No, the goto here may (or may not) lead to the wrong ordering on the cleanup
> and/or error paths. It's usually recommended to avoid such a style even if
> it works without races in that case. That's why when switching to managed
> resources it's highly recommended to wrap everything to be managed (in case
> there is no native APIs available) and avoid any flow disruption (goto:s).

In this case I can't see any issue with the goto. It only leads to a
call to pm_runtime_put_noidle(), and after returning 'ret', the
device-managed resources are released. If the goto is removed and a
call to pm_runtime_put_noidle() is carried out in every error path, the
result is exactly the same as there are no operations within the probe
execution path that affect their behavior.

I can imagine that it might be problematic in some special cases, but
the use of devm_*() and goto jumps within a function is allowed and used
in the official documentation [1], and in several upstream drivers in IIO
[2][3] and other subsystems [4]. Of course, all those examples might be bad
examples, but I could not find any reference discouraging the use of
goto with devm_*().

[1] https://docs.kernel.org/driver-api/usb/writing_musb_glue_layer.html
[2] drivers/iio/accel/kxcjk-1013.c
[3] drivers/iio/pressure/hid-sensor-press.c
[4] drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c

Best regards,
Javier