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

[Rafael J. Wysocki]

On Thu, Apr 30, 2026 at 8:23 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> 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!