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

[Mark Brown]

On Tue, Jun 29, 2021 at 08:34:55PM +0200, H. Nikolaus Schaller wrote:
> > Am 29.06.2021 um 17:59 schrieb Mark Brown <broonie@xxxxxxxxxx>:

> > 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.

So, I did actually look at the commit but I couldn't figure out what the
change was supposed to do about it.

> 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().

Right, this is how a regualtor driver should specify the name of its
supply.

> 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.

No, not at all - it's representing whatever provides input power to the
regulator.  There may be no physical control of it at runtime on your
system but that may not be true on other systems.  It's quite common for
there to be a chain of regulators (eg, DCDCs supplying LDOs) and then
they all need to get get power managed appropriately and you don't end
up thinking a regulator is enabled when the input regulator is disabled.  

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

This means that we won't actually map the supply and any system that
relies on software handling the supply regulator will be broken.

> Well, unless...

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

We resolve supplies during regulator registration, this would
effectively just skip mapping of the supply.

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

What I would expect to happen here would be that once vsys_cobra is
registered the regulators supplied by it can register and then all their
consumers would in turn be able to register.  You should look into why
that supply regulator isn't appearing and resolve that, or if a consumer
isn't handling deferral then that would need to be addressed.

Attachment: signature.asc
Description: PGP signature