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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

On Fri, Apr 24, 2026 at 05:01:10PM -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.
> 
> Refactor control flow in affected functions by using direct returns,
> as we no longer need to manually unlock mutexes at the end of
> functions. This will simplify the return logic of affected functions.
> 
> In intel_soc_pmic_exec_mipi_pmic_seq_element(): While at it, remove
> now redundant "ret" variable.

Some remarks and one important comment (see scoped_guard()() error handling).

...

> -	mutex_lock(&opregion->lock);
> +	scoped_guard(&opregion->lock) {

>  

Now this blank line become redundant.

> -	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,
> +		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);

Despite being long, I would still use a single line for the last one:

			result = pmic_thermal_pen(opregion, reg, bit, function, value64);

> -	else
> -		result = -EINVAL;
> -
> -	mutex_unlock(&opregion->lock);

> -	if (result < 0) {
> -		if (result == -EINVAL)
> -			return AE_BAD_PARAMETER;
>  		else
> -			return AE_ERROR;
> +			return AE_BAD_PARAMETER;
> +
>  	}

TBH, I would leave current logic, as it will keep the scope and the semantics
of the each branch consistent.

		else
			result = -EINVAL;


> +	if (result < 0)
> +		return AE_ERROR;

Also (some) compiler(s) might not see well the result being initialised all the
time when we are here.

>  	return AE_OK;
>  }

...

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

{} are not needed for a single statement cases. But I see it occupies 3 LoC,
perhaps it's fine to have them still. Up to maintainers.

> +	if (d->pmic_i2c_address) {
>  		if (i2c_address == d->pmic_i2c_address) {
> -			ret = regmap_update_bits(intel_pmic_opregion->regmap,
> -						 reg_address, mask, value);
> -		} else {
> -			pr_err("%s: Unexpected i2c-addr: 0x%02x (reg-addr 0x%x value 0x%x mask 0x%x)\n",
> -			       __func__, i2c_address, reg_address, value, mask);
> -			ret = -ENXIO;
> +			return regmap_update_bits(intel_pmic_opregion->regmap,
> +						  reg_address, mask, value);
>  		}

Ditto.

> -	} else {
> -		pr_warn("%s: Not implemented\n", __func__);
> -		pr_warn("%s: i2c-addr: 0x%x reg-addr 0x%x value 0x%x mask 0x%x\n",
> -			__func__, i2c_address, reg_address, value, mask);
> -		ret = -EOPNOTSUPP;
> -	}
>  
> -	mutex_unlock(&intel_pmic_opregion->lock);
> +		pr_err("%s: Unexpected i2c-addr: 0x%02x (reg-addr 0x%x value 0x%x mask 0x%x)\n",
> +		       __func__, i2c_address, reg_address, value, mask);
> +		return -ENXIO;

In this case it's probably better to swap conditional to have error case first.
This is standard pattern elsewhere in the kernel.

		if (i2c_address != d->pmic_i2c_address) {
			pr_err("%s: Unexpected i2c-addr: 0x%02x (reg-addr 0x%x value 0x%x mask 0x%x)\n",
			       __func__, i2c_address, reg_address, value, mask);
			return -ENXIO;
		}

		return regmap_update_bits(intel_pmic_opregion->regmap,
					  reg_address, mask, value);

Or leave the inner part untouched as it's not the main part of the patch
anyway. This makes the patch cleaner (however it remains 'ret' to be defined).

Up to you and maintainers.

> +	}
>  
> -	return ret;
> +	pr_warn("%s: Not implemented\n", __func__);
> +	pr_warn("%s: i2c-addr: 0x%x reg-addr 0x%x value 0x%x mask 0x%x\n",
> +		__func__, i2c_address, reg_address, value, mask);
> +	return -EOPNOTSUPP;
>  }

-- 
With Best Regards,
Andy Shevchenko