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

[Maxwell Doose]

On Mon, Apr 27, 2026 at 2:28 AM Andy Shevchenko
<andriy.shevchenko@xxxxxxxxx> 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
>
>