* [PATCH v2] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard()
@ 2026-04-24 22:01 Maxwell Doose
2026-04-27 0:08 ` Maxwell Doose
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Maxwell Doose @ 2026-04-24 22:01 UTC (permalink / raw)
To: rafael, lenb; +Cc: andy, westeri, linux-acpi, linux-kernel
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.
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.
drivers/acpi/pmic/intel_pmic.c | 69 +++++++++++++++-------------------
1 file changed, 30 insertions(+), 39 deletions(-)
diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
index 134e9ca8eaa2..5c37a8f28e71 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(&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,27 +180,23 @@ static acpi_status intel_pmic_thermal_handler(u32 function,
if (result == -ENOENT)
return AE_BAD_PARAMETER;
- mutex_lock(&opregion->lock);
+ scoped_guard(&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,
+ 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);
-
- if (result < 0) {
- if (result == -EINVAL)
- return AE_BAD_PARAMETER;
else
- return AE_ERROR;
+ return AE_BAD_PARAMETER;
+
}
+ if (result < 0)
+ return AE_ERROR;
+
return AE_OK;
}
@@ -345,7 +339,6 @@ int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address,
u32 value, u32 mask)
{
const struct intel_pmic_opregion_data *d;
- int ret;
if (!intel_pmic_opregion) {
pr_warn("%s: No PMIC registered\n", __func__);
@@ -354,30 +347,28 @@ 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(&intel_pmic_opregion->lock);
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);
+ }
+
+ 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);
}
- } 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;
+ }
- 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;
}
EXPORT_SYMBOL_GPL(intel_soc_pmic_exec_mipi_pmic_seq_element);
--
2.53.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard()
2026-04-24 22:01 [PATCH v2] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard() Maxwell Doose
@ 2026-04-27 0:08 ` Maxwell Doose
2026-04-27 7:28 ` Andy Shevchenko
2026-04-27 9:28 ` kernel test robot
2 siblings, 0 replies; 5+ messages in thread
From: Maxwell Doose @ 2026-04-27 0:08 UTC (permalink / raw)
To: rafael, lenb; +Cc: andy, westeri, linux-acpi, linux-kernel
Hi Andy,
On Fri, Apr 24, 2026 at 5:01 PM Maxwell Doose <m32285159@gmail.com> wrote:
>
> diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
> index 134e9ca8eaa2..5c37a8f28e71 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(&opregion->lock);
>
I noticed I made a syntax error here, I believe it should be
guard(mutex)(&opregion->lock); instead.
[snip]
> @@ -354,30 +347,28 @@ 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(&intel_pmic_opregion->lock);
Here as well.
I'm going to get this fixed and I'll send you a v3 shortly. In the
meantime, disregard this version.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard()
2026-04-24 22:01 [PATCH v2] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard() Maxwell Doose
2026-04-27 0:08 ` Maxwell Doose
@ 2026-04-27 7:28 ` Andy Shevchenko
2026-04-27 14:37 ` Maxwell Doose
2026-04-27 9:28 ` kernel test robot
2 siblings, 1 reply; 5+ messages in thread
From: Andy Shevchenko @ 2026-04-27 7:28 UTC (permalink / raw)
To: Maxwell Doose; +Cc: rafael, lenb, andy, westeri, linux-acpi, linux-kernel
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard()
2026-04-24 22:01 [PATCH v2] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard() Maxwell Doose
2026-04-27 0:08 ` Maxwell Doose
2026-04-27 7:28 ` Andy Shevchenko
@ 2026-04-27 9:28 ` kernel test robot
2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2026-04-27 9:28 UTC (permalink / raw)
To: Maxwell Doose, rafael, lenb
Cc: oe-kbuild-all, andy, westeri, linux-acpi, linux-kernel
Hi Maxwell,
kernel test robot noticed the following build errors:
[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on rafael-pm/bleeding-edge linus/master v7.1-rc1 next-20260424]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Maxwell-Doose/ACPI-pmic-Replace-mutex_lock-unlock-with-guard-scoped_guard/20260426-085114
base: https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link: https://lore.kernel.org/r/20260424220110.25929-1-m32285159%40gmail.com
patch subject: [PATCH v2] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard()
config: x86_64-rhel-9.4-ltp (https://download.01.org/0day-ci/archive/20260427/202604271142.bKGFhwIQ-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260427/202604271142.bKGFhwIQ-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202604271142.bKGFhwIQ-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/acpi.h:11,
from drivers/acpi/pmic/intel_pmic.c:9:
drivers/acpi/pmic/intel_pmic.c: In function 'intel_pmic_power_handler':
>> include/linux/cleanup.h:302:9: error: pasting "class_" and "&" does not give a valid preprocessing token
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~~
include/linux/cleanup.h:422:9: note: in expansion of macro 'CLASS'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:70:9: note: in expansion of macro 'guard'
70 | guard(&opregion->lock);
| ^~~~~
>> include/linux/cleanup.h:302:9: error: 'class_' undeclared (first use in this function); did you mean 'class'?
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~~
include/linux/cleanup.h:422:9: note: in expansion of macro 'CLASS'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:70:9: note: in expansion of macro 'guard'
70 | guard(&opregion->lock);
| ^~~~~
include/linux/cleanup.h:302:9: note: each undeclared identifier is reported only once for each function it appears in
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~~
include/linux/cleanup.h:422:9: note: in expansion of macro 'CLASS'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:70:9: note: in expansion of macro 'guard'
70 | guard(&opregion->lock);
| ^~~~~
>> drivers/acpi/pmic/intel_pmic.c:70:26: error: 'struct intel_pmic_opregion' has no member named 'lock_t'; did you mean 'lock'?
70 | guard(&opregion->lock);
| ^~~~
include/linux/cleanup.h:302:17: note: in definition of macro 'CLASS'
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:70:9: note: in expansion of macro 'guard'
70 | guard(&opregion->lock);
| ^~~~~
include/linux/compiler.h:165:17: error: expected ';' before '__UNIQUE_ID_guard_242'
165 | __PASTE(__UNIQUE_ID_, \
| ^~~~~~~~~~~~
include/linux/cleanup.h:302:27: note: in definition of macro 'CLASS'
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~
include/linux/compiler_types.h:16:23: note: in expansion of macro '___PASTE'
16 | #define __PASTE(a, b) ___PASTE(a, b)
| ^~~~~~~~
include/linux/compiler.h:165:9: note: in expansion of macro '__PASTE'
165 | __PASTE(__UNIQUE_ID_, \
| ^~~~~~~
include/linux/cleanup.h:422:22: note: in expansion of macro '__UNIQUE_ID'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:70:9: note: in expansion of macro 'guard'
70 | guard(&opregion->lock);
| ^~~~~
include/linux/cleanup.h:302:41: error: pasting "class_" and "&" does not give a valid preprocessing token
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~~
include/linux/cleanup.h:422:9: note: in expansion of macro 'CLASS'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:70:9: note: in expansion of macro 'guard'
70 | guard(&opregion->lock);
| ^~~~~
include/linux/cleanup.h:303:17: error: pasting "class_" and "&" does not give a valid preprocessing token
303 | class_##_name##_constructor
| ^~~~~~
include/linux/cleanup.h:422:9: note: in expansion of macro 'CLASS'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:70:9: note: in expansion of macro 'guard'
70 | guard(&opregion->lock);
| ^~~~~
drivers/acpi/pmic/intel_pmic.c: In function 'intel_pmic_thermal_handler':
>> include/linux/cleanup.h:302:9: error: pasting "class_" and "&" does not give a valid preprocessing token
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~~
include/linux/cleanup.h:441:14: note: in expansion of macro 'CLASS'
441 | for (CLASS(_name, scope)(args); \
| ^~~~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
>> include/linux/cleanup.h:302:9: error: 'class_' undeclared (first use in this function); did you mean 'class'?
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~~
include/linux/cleanup.h:441:14: note: in expansion of macro 'CLASS'
441 | for (CLASS(_name, scope)(args); \
| ^~~~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:33: error: 'struct intel_pmic_opregion' has no member named 'lock_t'; did you mean 'lock'?
183 | scoped_guard(&opregion->lock) {
| ^~~~
include/linux/cleanup.h:302:17: note: in definition of macro 'CLASS'
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
>> include/linux/cleanup.h:441:27: error: expected ';' before 'scope'
441 | for (CLASS(_name, scope)(args); \
| ^~~~~
include/linux/cleanup.h:302:27: note: in definition of macro 'CLASS'
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
include/linux/cleanup.h:302:41: error: pasting "class_" and "&" does not give a valid preprocessing token
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~~
include/linux/cleanup.h:441:14: note: in expansion of macro 'CLASS'
441 | for (CLASS(_name, scope)(args); \
| ^~~~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
include/linux/cleanup.h:303:17: error: pasting "class_" and "&" does not give a valid preprocessing token
303 | class_##_name##_constructor
| ^~~~~~
include/linux/cleanup.h:441:14: note: in expansion of macro 'CLASS'
441 | for (CLASS(_name, scope)(args); \
| ^~~~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
include/linux/cleanup.h:424:28: error: pasting "class_" and "&" does not give a valid preprocessing token
424 | #define __guard_ptr(_name) class_##_name##_lock_ptr
| ^~~~~~
include/linux/cleanup.h:442:14: note: in expansion of macro '__guard_ptr'
442 | __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name); \
| ^~~~~~~~~~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
>> drivers/acpi/pmic/intel_pmic.c:183:31: error: 'struct intel_pmic_opregion' has no member named 'lock_lock_ptr'
183 | scoped_guard(&opregion->lock) {
| ^~
include/linux/cleanup.h:424:36: note: in definition of macro '__guard_ptr'
424 | #define __guard_ptr(_name) class_##_name##_lock_ptr
| ^~~~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
>> include/linux/cleanup.h:442:34: error: 'scope' undeclared (first use in this function)
442 | __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name); \
| ^~~~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
include/linux/cleanup.h:426:30: error: pasting "class_" and "&" does not give a valid preprocessing token
426 | #define __is_cond_ptr(_name) class_##_name##_is_conditional
| ^~~~~~
include/linux/cleanup.h:442:45: note: in expansion of macro '__is_cond_ptr'
442 | __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name); \
| ^~~~~~~~~~~~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
>> drivers/acpi/pmic/intel_pmic.c:183:31: error: 'struct intel_pmic_opregion' has no member named 'lock_is_conditional'
183 | scoped_guard(&opregion->lock) {
| ^~
include/linux/cleanup.h:426:38: note: in definition of macro '__is_cond_ptr'
426 | #define __is_cond_ptr(_name) class_##_name##_is_conditional
| ^~~~~
include/linux/cleanup.h:450:9: note: in expansion of macro '__scoped_guard'
450 | __scoped_guard(_name, __UNIQUE_ID(label), args)
| ^~~~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:183:9: note: in expansion of macro 'scoped_guard'
183 | scoped_guard(&opregion->lock) {
| ^~~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c: In function 'intel_soc_pmic_exec_mipi_pmic_seq_element':
>> include/linux/cleanup.h:302:9: error: pasting "class_" and "&" does not give a valid preprocessing token
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~~
include/linux/cleanup.h:422:9: note: in expansion of macro 'CLASS'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:350:9: note: in expansion of macro 'guard'
350 | guard(&intel_pmic_opregion->lock);
| ^~~~~
>> include/linux/cleanup.h:302:9: error: 'class_' undeclared (first use in this function); did you mean 'class'?
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~~
include/linux/cleanup.h:422:9: note: in expansion of macro 'CLASS'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:350:9: note: in expansion of macro 'guard'
350 | guard(&intel_pmic_opregion->lock);
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:350:37: error: 'struct intel_pmic_opregion' has no member named 'lock_t'; did you mean 'lock'?
350 | guard(&intel_pmic_opregion->lock);
| ^~~~
include/linux/cleanup.h:302:17: note: in definition of macro 'CLASS'
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:350:9: note: in expansion of macro 'guard'
350 | guard(&intel_pmic_opregion->lock);
| ^~~~~
include/linux/compiler.h:165:17: error: expected ';' before '__UNIQUE_ID_guard_245'
165 | __PASTE(__UNIQUE_ID_, \
| ^~~~~~~~~~~~
include/linux/cleanup.h:302:27: note: in definition of macro 'CLASS'
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~
include/linux/compiler_types.h:16:23: note: in expansion of macro '___PASTE'
16 | #define __PASTE(a, b) ___PASTE(a, b)
| ^~~~~~~~
include/linux/compiler.h:165:9: note: in expansion of macro '__PASTE'
165 | __PASTE(__UNIQUE_ID_, \
| ^~~~~~~
include/linux/cleanup.h:422:22: note: in expansion of macro '__UNIQUE_ID'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~~~~~~~
drivers/acpi/pmic/intel_pmic.c:350:9: note: in expansion of macro 'guard'
350 | guard(&intel_pmic_opregion->lock);
| ^~~~~
include/linux/cleanup.h:302:41: error: pasting "class_" and "&" does not give a valid preprocessing token
302 | class_##_name##_t var __cleanup(class_##_name##_destructor) = \
| ^~~~~~
include/linux/cleanup.h:422:9: note: in expansion of macro 'CLASS'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:350:9: note: in expansion of macro 'guard'
350 | guard(&intel_pmic_opregion->lock);
| ^~~~~
include/linux/cleanup.h:303:17: error: pasting "class_" and "&" does not give a valid preprocessing token
303 | class_##_name##_constructor
| ^~~~~~
include/linux/cleanup.h:422:9: note: in expansion of macro 'CLASS'
422 | CLASS(_name, __UNIQUE_ID(guard))
| ^~~~~
drivers/acpi/pmic/intel_pmic.c:350:9: note: in expansion of macro 'guard'
350 | guard(&intel_pmic_opregion->lock);
| ^~~~~
vim +70 drivers/acpi/pmic/intel_pmic.c
49
50 static acpi_status intel_pmic_power_handler(u32 function,
51 acpi_physical_address address, u32 bits, u64 *value64,
52 void *handler_context, void *region_context)
53 {
54 struct intel_pmic_opregion *opregion = region_context;
55 struct regmap *regmap = opregion->regmap;
56 const struct intel_pmic_opregion_data *d = opregion->data;
57 int reg, bit, result;
58
59 if (bits != 32 || !value64)
60 return AE_BAD_PARAMETER;
61
62 if (function == ACPI_WRITE && !(*value64 == 0 || *value64 == 1))
63 return AE_BAD_PARAMETER;
64
65 result = pmic_get_reg_bit(address, d->power_table,
66 d->power_table_count, ®, &bit);
67 if (result == -ENOENT)
68 return AE_BAD_PARAMETER;
69
> 70 guard(&opregion->lock);
71
72 result = function == ACPI_READ ?
73 d->get_power(regmap, reg, bit, value64) :
74 d->update_power(regmap, reg, bit, *value64 == 1);
75
76 return result ? AE_ERROR : AE_OK;
77 }
78
79 static int pmic_read_temp(struct intel_pmic_opregion *opregion,
80 int reg, u64 *value)
81 {
82 int raw_temp, temp;
83
84 if (!opregion->data->get_raw_temp)
85 return -ENXIO;
86
87 raw_temp = opregion->data->get_raw_temp(opregion->regmap, reg);
88 if (raw_temp < 0)
89 return raw_temp;
90
91 if (!opregion->lpat_table) {
92 *value = raw_temp;
93 return 0;
94 }
95
96 temp = opregion->data->lpat_raw_to_temp(opregion->lpat_table, raw_temp);
97 if (temp < 0)
98 return temp;
99
100 *value = temp;
101 return 0;
102 }
103
104 static int pmic_thermal_temp(struct intel_pmic_opregion *opregion, int reg,
105 u32 function, u64 *value)
106 {
107 return function == ACPI_READ ?
108 pmic_read_temp(opregion, reg, value) : -EINVAL;
109 }
110
111 static int pmic_thermal_aux(struct intel_pmic_opregion *opregion, int reg,
112 u32 function, u64 *value)
113 {
114 int raw_temp;
115
116 if (function == ACPI_READ)
117 return pmic_read_temp(opregion, reg, value);
118
119 if (!opregion->data->update_aux)
120 return -ENXIO;
121
122 if (opregion->lpat_table) {
123 raw_temp = acpi_lpat_temp_to_raw(opregion->lpat_table, *value);
124 if (raw_temp < 0)
125 return raw_temp;
126 } else {
127 raw_temp = *value;
128 }
129
130 return opregion->data->update_aux(opregion->regmap, reg, raw_temp);
131 }
132
133 static int pmic_thermal_pen(struct intel_pmic_opregion *opregion, int reg,
134 int bit, u32 function, u64 *value)
135 {
136 const struct intel_pmic_opregion_data *d = opregion->data;
137 struct regmap *regmap = opregion->regmap;
138
139 if (!d->get_policy || !d->update_policy)
140 return -ENXIO;
141
142 if (function == ACPI_READ)
143 return d->get_policy(regmap, reg, bit, value);
144
145 if (*value != 0 && *value != 1)
146 return -EINVAL;
147
148 return d->update_policy(regmap, reg, bit, *value);
149 }
150
151 static bool pmic_thermal_is_temp(int address)
152 {
153 return (address <= 0x3c) && !(address % 12);
154 }
155
156 static bool pmic_thermal_is_aux(int address)
157 {
158 return (address >= 4 && address <= 0x40 && !((address - 4) % 12)) ||
159 (address >= 8 && address <= 0x44 && !((address - 8) % 12));
160 }
161
162 static bool pmic_thermal_is_pen(int address)
163 {
164 return address >= 0x48 && address <= 0x5c;
165 }
166
167 static acpi_status intel_pmic_thermal_handler(u32 function,
168 acpi_physical_address address, u32 bits, u64 *value64,
169 void *handler_context, void *region_context)
170 {
171 struct intel_pmic_opregion *opregion = region_context;
172 const struct intel_pmic_opregion_data *d = opregion->data;
173 int reg, bit, result;
174
175 if (bits != 32 || !value64)
176 return AE_BAD_PARAMETER;
177
178 result = pmic_get_reg_bit(address, d->thermal_table,
179 d->thermal_table_count, ®, &bit);
180 if (result == -ENOENT)
181 return AE_BAD_PARAMETER;
182
> 183 scoped_guard(&opregion->lock) {
184
185 if (pmic_thermal_is_temp(address))
186 result = pmic_thermal_temp(opregion, reg, function, value64);
187 else if (pmic_thermal_is_aux(address))
188 result = pmic_thermal_aux(opregion, reg, function, value64);
189 else if (pmic_thermal_is_pen(address))
190 result = pmic_thermal_pen(opregion, reg, bit,
191 function, value64);
192 else
193 return AE_BAD_PARAMETER;
194
195 }
196
197 if (result < 0)
198 return AE_ERROR;
199
200 return AE_OK;
201 }
202
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard()
2026-04-27 7:28 ` Andy Shevchenko
@ 2026-04-27 14:37 ` Maxwell Doose
0 siblings, 0 replies; 5+ messages in thread
From: Maxwell Doose @ 2026-04-27 14:37 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: rafael, lenb, andy, westeri, linux-acpi, linux-kernel
On Mon, Apr 27, 2026 at 2:28 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> > - mutex_lock(&opregion->lock);
> > + scoped_guard(&opregion->lock) {
>
> >
>
> Now this blank line become redundant.
LGTM.
>
> > - 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);
Sounds good. I guess it'll be up to the maintainer to decide on this.
>
> > - 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;
>
I can also go back and fix this, seems rather simple.
>
> > + 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;
> > }
Acked, I'll see about getting this fixed.
>
> > 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.
I'm aware that {} isn't needed for single statements, but as you've
noticed, it occupies 3 LoC, and keeping the {} seems like the more
logical solution.
>
> > + 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.
Same here, feels more logical to just keep them.
> > - } 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);
Oops, I must've swapped these when I did my final audit before sending
this patch. I'll go back and fix that for sure.
>
> 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).
I might also do this, keep the ret variable. It's probably a lot safer
to just keep some of this logic the same and send a second patch that
updates this stuff.
> Up to you and maintainers.
Of course, if the maintainers want something else then I can also add
those things to my v3.
best regards,
maxwell
> > + }
> >
> > - 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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-04-27 14:38 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-04-24 22:01 [PATCH v2] ACPI: pmic: Replace mutex_lock/unlock() with guard()/scoped_guard() Maxwell Doose
2026-04-27 0:08 ` Maxwell Doose
2026-04-27 7:28 ` Andy Shevchenko
2026-04-27 14:37 ` Maxwell Doose
2026-04-27 9:28 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox