Re: [PATCH] regulator: palmas: set supply_name after registering the regulator

[H. Nikolaus Schaller]

Hi Mark,

> Am 29.06.2021 um 17:59 schrieb Mark Brown <broonie@xxxxxxxxxx>:
> 
> On Tue, Jun 29, 2021 at 05:24:03PM +0200, H. Nikolaus Schaller wrote:
>> Commit 98e48cd9283d ("regulator: core: resolve supply for boot-on/always-on regulators")
>> 
>> introduced a new rule which makes Palmas regulator registration fail:
>> 
>> [    5.407712] ldo1: supplied by vsys_cobra
>> [    5.412748] ldo2: supplied by vsys_cobra
>> [    5.417603] palmas-pmic 48070000.i2c:palmas@48:palmas_pmic: failed to register 48070000.i2c:palmas@48:palmas_pmic regulator
>> 
>> This seems to block additions initializations and finally the
>> Pyra-Handheld hangs when trying to access MMC because there is
>> no mmc-supply available.
> 
> What is that rule and how is this patch intended to ensure that Palmas
> meets it?
>  As covered in submitting-patches.rst your changelog should
> explain this so that in review we can verify that this is a good fix.

I am very sorry, but I simply believed that it is not necessary to copy&paste or
describe this because it appears not to be difficult to retrieve.

Commit 98e48cd9283d ("regulator: core: resolve supply for boot-on/always-on regulators")

    regulator: core: resolve supply for boot-on/always-on regulators
    
    For the boot-on/always-on regulators the set_machine_constrainst() is
    called before resolving rdev->supply. Thus the code would try to enable
    rdev before enabling supplying regulator. Enforce resolving supply
    regulator before enabling rdev.

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f192bf19492e..e20e77e4c159 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1425,6 +1425,12 @@ static int set_machine_constraints(struct regulator_dev *rdev)
         * and we have control then make sure it is enabled.
         */
        if (rdev->constraints->always_on || rdev->constraints->boot_on) {
+               /* If we want to enable this regulator, make sure that we know
+                * the supplying regulator.
+                */
+               if (rdev->supply_name && !rdev->supply)
+                       return -EPROBE_DEFER;
+
                if (rdev->supply) {
                        ret = regulator_enable(rdev->supply);
                        if (ret < 0) {

This rule (rdev->supply_name && !rdev->supply) did not exist before 98e48cd9283d
and it seems to return early with EPROBE_DEFER if there is a desc->supply_name defined,
but no supply resolved.

The Palmas driver is setting desc->supply_name to some string constant (i.e. not NULL)
and is then calling devm_regulator_register().

So it was working fine without having the supplying regulator resolved. AFAIK they
just serve as fixed regulators in the device tree and have no physical equivalent.

My proposal just moves setting the supply_name behind devm_regulator_register() and
by that restores the old behaviour.

Well, unless...

... devm_regulator_register() does something differently if desc->supply_name
is not set before and changed afterwards. It may miss that change.

I did not observe any indications, but something could happen in the guts of
devm_regulator_register(). If it is, then my approach is clearly faulty.

There may be a better solution where I hope more reviewers (especially
those more familiar with the Palmas driver than me) can comment on.

> The change itself looks worrying like it just shuts the error up and
> could cause problems for other systems.


Since this change is confined to the Palmas driver and results in the same
behaviour as it had before 98e48cd9283d did break it, it should not harm
other systems.

So I hope for guidance if my approach is good or needs a different solution.

BR and thanks,
Nikolaus Schaller