drivers/acpi/pmic/intel_pmic.c | 37 +++++++++++++++------------------- 1 file changed, 16 insertions(+), 21 deletions(-)
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.0On 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
On Thu, Apr 30, 2026 at 8:23 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> 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.
It's fine by me and sashiko.de hasn't found any issues in it, so
applied as 7.2 material.
Thanks!
© 2016 - 2026 Red Hat, Inc.