Re: [PATCH v5] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard()

[Andy Shevchenko]

On Wed, Apr 29, 2026 at 08:35:06PM -0500, Maxwell Doose wrote:
> Replace mutex_lock() and unlock() macros with the newer guard() and
> scoped_guard() macros. This will help modernize and clean the code.

...

> -	mutex_lock(&opregion->lock);
> -
> -	if (pmic_thermal_is_temp(address))
> -		result = pmic_thermal_temp(opregion, reg, function, value64);
> -	else if (pmic_thermal_is_aux(address))
> -		result = pmic_thermal_aux(opregion, reg, function, value64);
> -	else if (pmic_thermal_is_pen(address))
> -		result = pmic_thermal_pen(opregion, reg, bit,
> -						function, value64);
> -	else
> -		result = -EINVAL;
> -
> -	mutex_unlock(&opregion->lock);
> +	scoped_guard(mutex, &opregion->lock) {
> +		if (pmic_thermal_is_temp(address))
> +			result = pmic_thermal_temp(opregion, reg, function, value64);
> +		else if (pmic_thermal_is_aux(address))
> +			result = pmic_thermal_aux(opregion, reg, function, value64);
> +		else if (pmic_thermal_is_pen(address))
> +			result = pmic_thermal_pen(opregion, reg, bit, function, value64);
> +		else
> +			result = -EINVAL;
> +	}

>  

This blank line is not needed as previous is coupled with this check.
This is a minor thing, but what takes my attention is the result assignment for
last else. While this is correct code it might provoke false positive warnings
from the compiler(s) that can not properly parse scoped_guard() and proof that
result is always assigned (the warning is something like "uninitialised variable
in use"). Let's wait for CIs to be sure we do not enter a such. Otherwise, this
will need a refactoring like

	result = -EINVAL;
	scoped_guard(...) {
		...
	}

>  	if (result < 0) {
>  		if (result == -EINVAL)

...

>  	if (d->exec_mipi_pmic_seq_element) {
> -		ret = d->exec_mipi_pmic_seq_element(intel_pmic_opregion->regmap,
> +		return d->exec_mipi_pmic_seq_element(intel_pmic_opregion->regmap,
>  						    i2c_address, reg_address,
>  						    value, mask);
> -	} else if (d->pmic_i2c_address) {
> +	}
> +
> +	if (d->pmic_i2c_address) {
>  		if (i2c_address == d->pmic_i2c_address) {

Maybe we should not touch this either and actually have clean guard()() patch +
refactoring returns, dunno... My whole concern here is that this code is not as
simple as IIO switch-cases when we replace break:s with return:s mostly
automatically without breaking readability of anything. Here is different case
and mixing guard()() with return:s refactoring makes it harder to review and
understand. Also if any regression happens, will be harder to maintain.

...

I don't want you to waste more time on experimenting, I think we better
wait for CIs and Rafael, if they all are okay with the change, that will
work for me.

-- 
With Best Regards,
Andy Shevchenko