Messages in this thread | | | Date | Mon, 11 Apr 2016 13:46:12 +0200 | From | Thierry Reding <> | Subject | Re: [PATCH 1/5] regulator: core: Resolve supply earlier |
| |
On Mon, Apr 11, 2016 at 11:59:02AM +0100, Jon Hunter wrote: > Hi Thierry, > > On 07/04/16 15:22, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > Subsequent patches will need access to the parent supply from within the > > set_machine_constraints() function to properly implement bypass mode. If > > the parent supply hasn't been resolved by that time the voltage can't be > > queried. > > > > Also, by making sure the supply is resolved early most of the changes in > > set_machine_constraints() don't have to be undone if resolution fails. > > > > Suggested-by: Mark Brown <broonie@kernel.org> > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/regulator/core.c | 27 +++++++++++++++++++++------ > > 1 file changed, 21 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c > > index 2786d251b1cc..cc0333a79924 100644 > > --- a/drivers/regulator/core.c > > +++ b/drivers/regulator/core.c > > @@ -3972,18 +3972,27 @@ regulator_register(const struct regulator_desc *regulator_desc, > > > > dev_set_drvdata(&rdev->dev, rdev); > > > > + if (init_data && init_data->supply_regulator) > > + rdev->supply_name = init_data->supply_regulator; > > + else if (regulator_desc->supply_name) > > + rdev->supply_name = regulator_desc->supply_name; > > + > > + /* > > + * set_machine_constraints() needs the supply to be resolved in order > > + * to support querying the current voltage in bypass mode. Resolve it > > + * here to more easily handle deferred probing. > > + */ > > + ret = regulator_resolve_supply(rdev); > > + if (ret < 0) > > + goto scrub; > > + > > Thanks for sending this. However, I think that calling > regulator_resolve_supply() can cause a deadlock, because the > regulator_list_mutex is held at this point and > regulator_resolve_supply() calls regulator_dev_lookup() which may try to > request the mutex again.
True... I never encountered that case in my testing. I'm not sure exactly why, though.
> So may be we need to move this call after the call to > regulator_of_get_init_data() before we acquire the mutex.
I don't think that'll work. regulator_resolve_supply() depends on some operations performed much later (such as rdev->dev.parent being set).
Perhaps moving the locking of the regulator_list_mutex down instead could work. It seems to me like the first place where it would need to be held is set_machine_constraints().
> Also, if we add this call, then I am wondering if we still need ... > > class_for_each_device(®ulator_class, NULL, NULL, > regulator_register_resolve_supply);
Possibly not. That line was introduced to hook up existing orphan regulators with their parents when they were registered, but I guess since we now always defer probe if a parent isn't registered yet the line would become a no-op.
Thierry [unhandled content-type:application/pgp-signature] | |