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

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

[Maxwell Doose]

Replace mutex_lock() and unlock() macros with the newer guard() and
scoped_guard() macros. This will help modernize and clean the code.

Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
 v2:
 - Refactored control flow in certain functions as suggested by Andy.
 - Indent code in scoped_guard block as suggested by Andy.
 - Fix parameter list alignment issues that I found while making this
   v2.

 v3:
 - Fixed guard(mutex)() and scoped_guard() syntax.
 - Added some logical and style changes per Andy's request. The
   requested changes and my personal thoughts can be found at
   https://lore.kernel.org/linux-acpi/ae8QBnSs9fYvkv_i@ashevche-desk.local/

 v4:
 - Added else keyword to if statement in
   intel_soc_pmic_exec_mipi_pmic_seq_element() per Andy's request.

 v5:
 - Misunderstanding at last version, v4 has been reverted.
 - Made diff less invasive per Andy's request by leaving most code
   below d->exec_mipi_pmic_seq_element check in
   intel_soc_pmic_exec_mipi_pmic_seq_element() intact.
 - Reinstated ret variable in
   intel_soc_pmic_exec_mipi_pmic_seq_element() per Andy's request.

 drivers/acpi/pmic/intel_pmic.c | 37 +++++++++++++++-------------------
 1 file changed, 16 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
index 134e9ca8eaa2..9bd46defc5a2 100644
--- a/drivers/acpi/pmic/intel_pmic.c
+++ b/drivers/acpi/pmic/intel_pmic.c
@@ -67,14 +67,12 @@ static acpi_status intel_pmic_power_handler(u32 function,
 	if (result == -ENOENT)
 		return AE_BAD_PARAMETER;
 
-	mutex_lock(&opregion->lock);
+	guard(mutex)(&opregion->lock);
 
 	result = function == ACPI_READ ?
 		d->get_power(regmap, reg, bit, value64) :
 		d->update_power(regmap, reg, bit, *value64 == 1);
 
-	mutex_unlock(&opregion->lock);
-
 	return result ? AE_ERROR : AE_OK;
 }
 
@@ -182,19 +180,16 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
 	if (result == -ENOENT)
 		return AE_BAD_PARAMETER;
 
-	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;
+	}
 
 	if (result < 0) {
 		if (result == -EINVAL)
@@ -354,13 +349,15 @@ int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address,
 
 	d = intel_pmic_opregion->data;
 
-	mutex_lock(&intel_pmic_opregion->lock);
+	guard(mutex)(&intel_pmic_opregion->lock);
 
 	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) {
 			ret = regmap_update_bits(intel_pmic_opregion->regmap,
 						 reg_address, mask, value);
@@ -376,8 +373,6 @@ int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address,
 		ret = -EOPNOTSUPP;
 	}
 
-	mutex_unlock(&intel_pmic_opregion->lock);
-
 	return ret;
 }
 EXPORT_SYMBOL_GPL(intel_soc_pmic_exec_mipi_pmic_seq_element);
-- 
2.53.0

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