Regulator probe on demand (or circular dependencies)

Regulator probe on demand (or circular dependencies)

[Kieran Bingham]

Hi Mark, Liam, (and other Renesas multimedia-devs)...

I have an issue with a circular dependency on regulators and I'm
wondering if you could offer some guidance before I go down a rabbit hole.

Descriptions first, and a couple of questions down at the bottom:

I have a GMSL deserializer (MAX9286) [0] which connects 4 cameras over a
GMSL bus. These are currently expressed as an I2C Mux (each GMSL
connector provides a channel to communicate over I2C).

The MAX9286 also exposes 2 GPIO pins, as such I have configured the
MAX9286 driver [1] to expose a gpio-chip [2].


Before we communicate with the cameras, we must make sure that they are
powered up, which we do by specifying a 'poc' (power-over-coax)
regulator, and we enable it accordingly.

This works fine when the regulator is not directly tied to the max9286.
But alas we've got a board (the eagle-v3m) which uses the gpio-line on
the max9286 to enable power to the cameras.


You'll hopefully be able to see in patch [2], that the gpio-chip is
created before the call to get the regulator:

  ret = max9286_gpio(priv);
	if (ret)
		return ret;

  priv->regulator = regulator_get(&client->dev, "poc");


And thus at that point the GPIO chip 'exists'.

I have updated the DT for our 'eagle' board to create a regulator-fixed
[3], connected to the GPIO line on the max9286, including the required
delays we must wait for the cameras to come up.

Alas, here-in is where we have our circular dependency. The
regulator-fixed device can not be probed until the GPIO controller is
created during the max9286_probe(). And now I can't get the regulator,
because of course it doesn't yet exist.

I can not return -EPROBE_DEFER from max9286, as that would destroy the
gpio_chip already created, and thus the regulator-fixed would still not
be able to probe anyway, (and also there is a further issue in V4L2
which prevents us from probe-deferring this driver).


So - my diving into the code shows that the regulator_dev_lookup() at
drivers/regulator/core.c tries to identify the regulator from the
of_node (r = of_find_regulator_by_node(node);) but this returns empty
and thus returns -EPROBE_DEFER as highlighted by the comment :

"We have a node, but there is no device. assume it has not registered yet."


[0]
https://www.maximintegrated.com/en/products/interface/high-speed-signaling/MAX9286.html

[1] media: i2c: Add MAX9286 driver
 https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git/commit/?id=210913146496a0b7661a7b1af03fa8596ef42303

[2] media: i2c: max9286: Add GPIO chip controller
 https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git/commit/?id=896d77990562068d46749867b5c73b5e62815d47

[3] dts: eagle: Create a regulator-gpio
 https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git/commit/?id=3b49a32cfc83918eba2dd6936bd9bce03e7cb367
  (That commit title should really read "create a regulator-fixed")



So - after that long description, my questions are -

 - is there anything I can do here within regulator_dev_lookup() to
   attempt creating the regulator_dev 'on-demand' when
   of_find_regulator_by_node(node) returns empty? (or is that crazy, and
   just a rabbit-hole?)

   [2] Ensures that the gpio_chip that it is connected to has already
   been created. (and if it had not we certainly couldn't do anything
   else except a -EPROBE_DEFER)

 - My current workaround is to use a gpio-hog, but that doesn't allow me
   to provide a startup-delay-us which we require, thus I've had to hard
   code a *large* delay into the driver for testing.
   Is there anything 'else' I could do to construct this appropriately
   and define the required delay (which is camera specific) in DT?
   I'd rather not code some table of camera specific delays into the
   max9286 driver...


Note that we can't make any assumptions within the max9286 driver to
always use the gpio-line to enable the cameras. It's just a generic
line, which hardware designers can 'choose' to utilise. Indeed on
another board we have, the 'PoC' regulator is connected to an entirely
separate GPIO (not on the max9286), and thus we don't have this issue.


I sort of hope it might be possible to 'probe' the regulator when it is
needed rather than having to defer, but I fear how that would tie into
the driver core, so this e-mail is really to collect my thoughts on the
current issue before I jump into what could be a horrible dead-end, and
see if anyone has any ideas or comments on this topic.

Thanks for reading this far! And I look forward to any comments...
-- 
Regards
--
Kieran

Re: Regulator probe on demand (or circular dependencies)

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

On Fri, Dec 06, 2019 at 04:38:04PM +0000, Kieran Bingham wrote:

> The MAX9286 also exposes 2 GPIO pins, as such I have configured the
> MAX9286 driver [1] to expose a gpio-chip [2].

So this seems like a MFD then?  The nice thing about using the MFD
subsystem is that it means that the drivers for the various subsystems
on the device can instantiate in any order and defer separately without
interfering with each other which seems like it's the issue here.

>  - is there anything I can do here within regulator_dev_lookup() to
>    attempt creating the regulator_dev 'on-demand' when
>    of_find_regulator_by_node(node) returns empty? (or is that crazy, and
>    just a rabbit-hole?)

This seems like a terrible idea, you'll have a half baked regulator in
the system which will need special casing all over the place and
doubtless be an ongoing source of bugs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Regulator probe on demand (or circular dependencies)

[Kieran Bingham]

Hi Mark,

Thanks for getting back to me,

On 09/12/2019 16:37, Mark Brown wrote:
> On Fri, Dec 06, 2019 at 04:38:04PM +0000, Kieran Bingham wrote:
> 
>> The MAX9286 also exposes 2 GPIO pins, as such I have configured the
>> MAX9286 driver [1] to expose a gpio-chip [2].
> 
> So this seems like a MFD then?  The nice thing about using the MFD
> subsystem is that it means that the drivers for the various subsystems
> on the device can instantiate in any order and defer separately without
> interfering with each other which seems like it's the issue here.

Well that's part of the problem... the V4L2 async framework can not
currently support the device performing a probe-defer at all, so it
*will* fail later (and crash currently).

I hope we can fix this sometime - but it's a recurring pain point it
seems. Unless it's just our video-capture driver, I'll have to dig
deeper here, and check with Niklas.


>>  - is there anything I can do here within regulator_dev_lookup() to
>>    attempt creating the regulator_dev 'on-demand' when
>>    of_find_regulator_by_node(node) returns empty? (or is that crazy, and
>>    just a rabbit-hole?)
> 
> This seems like a terrible idea, you'll have a half baked regulator in
> the system which will need special casing all over the place and
> doubtless be an ongoing source of bugs.

Thanks - that's essentially what I'm glad to hear /before/ going down
some rabbit hole. I'll re-evaluate with the team, and see what the next
best steps are.

-- 
Regards
--
Kieran

Re: Regulator probe on demand (or circular dependencies)

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]

On Mon, Dec 09, 2019 at 05:03:38PM +0000, Kieran Bingham wrote:
> On 09/12/2019 16:37, Mark Brown wrote:
> > On Fri, Dec 06, 2019 at 04:38:04PM +0000, Kieran Bingham wrote:

> >> The MAX9286 also exposes 2 GPIO pins, as such I have configured the
> >> MAX9286 driver [1] to expose a gpio-chip [2].

> > So this seems like a MFD then?  The nice thing about using the MFD
> > subsystem is that it means that the drivers for the various subsystems
> > on the device can instantiate in any order and defer separately without
> > interfering with each other which seems like it's the issue here.

> Well that's part of the problem... the V4L2 async framework can not
> currently support the device performing a probe-defer at all, so it
> *will* fail later (and crash currently).

> I hope we can fix this sometime - but it's a recurring pain point it
> seems. Unless it's just our video-capture driver, I'll have to dig
> deeper here, and check with Niklas.

Yes, that seems like something that should be fixed anyway - if nothing
else for the most part probe defer error handling is just error
handling.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Regulator probe on demand (or circular dependencies)

[Niklas Söderlund]

Hi Kieran,

On 2019-12-09 17:03:38 +0000, Kieran Bingham wrote:
> Hi Mark,
> 
> Thanks for getting back to me,
> 
> On 09/12/2019 16:37, Mark Brown wrote:
> > On Fri, Dec 06, 2019 at 04:38:04PM +0000, Kieran Bingham wrote:
> > 
> >> The MAX9286 also exposes 2 GPIO pins, as such I have configured the
> >> MAX9286 driver [1] to expose a gpio-chip [2].
> > 
> > So this seems like a MFD then?  The nice thing about using the MFD
> > subsystem is that it means that the drivers for the various subsystems
> > on the device can instantiate in any order and defer separately without
> > interfering with each other which seems like it's the issue here.
> 
> Well that's part of the problem... the V4L2 async framework can not
> currently support the device performing a probe-defer at all, so it
> *will* fail later (and crash currently).
> 
> I hope we can fix this sometime - but it's a recurring pain point it
> seems. Unless it's just our video-capture driver, I'll have to dig
> deeper here, and check with Niklas.

The problem is that we can't register, unregister and re-regsiter a 
video device in a sane way. One easy solution to this is to not register 
the max9286 v4l2 subdevice until we know that the probe do not need to 
be deferred as this would sidestep the whole v4l2 issue described above.

> 
> 
> >>  - is there anything I can do here within regulator_dev_lookup() to
> >>    attempt creating the regulator_dev 'on-demand' when
> >>    of_find_regulator_by_node(node) returns empty? (or is that crazy, and
> >>    just a rabbit-hole?)
> > 
> > This seems like a terrible idea, you'll have a half baked regulator in
> > the system which will need special casing all over the place and
> > doubtless be an ongoing source of bugs.
> 
> Thanks - that's essentially what I'm glad to hear /before/ going down
> some rabbit hole. I'll re-evaluate with the team, and see what the next
> best steps are.
> 
> -- 
> Regards
> --
> Kieran

-- 
Regards,
Niklas Söderlund

Re: Regulator probe on demand (or circular dependencies)

[Kieran Bingham]

Hi Mark,

On 09/12/2019 16:37, Mark Brown wrote:
> On Fri, Dec 06, 2019 at 04:38:04PM +0000, Kieran Bingham wrote:
> 
>> The MAX9286 also exposes 2 GPIO pins, as such I have configured the
>> MAX9286 driver [1] to expose a gpio-chip [2].
> 
> So this seems like a MFD then?  The nice thing about using the MFD
> subsystem is that it means that the drivers for the various subsystems
> on the device can instantiate in any order and defer separately without
> interfering with each other which seems like it's the issue here.

As long as we can defer and not break the other layers this could
potentially work.

Breaking the GMSL driver out to have it's own bus (generically for
serdes buses) and allowing the MAX9286 to probe fully before probing any
subdevices is another alternative suggestion I've had (but much more
work I think).

>>  - is there anything I can do here within regulator_dev_lookup() to
>>    attempt creating the regulator_dev 'on-demand' when
>>    of_find_regulator_by_node(node) returns empty? (or is that crazy, and
>>    just a rabbit-hole?)
> 
> This seems like a terrible idea, you'll have a half baked regulator in

Ohh eep, I just re-read my description, and I don't think I described my
intention very well at all. (or at all!)

I wouldn't want to have just a half baked struct regulator_dev on it's
own ... I was more wondering if we can kick the core driver framework to
fully probe this regulator (which would thus create the required
regulator_dev structures). It was more a question of can we guide the
core driver framework that it really needs to probe this device
immediately ...

I was sort of wondering if something like this could optimise away some
of the -EPROBE_DEFER iterations at a more global level, but I don't know
how or if that would work anyway.

> the system which will need special casing all over the place and
> doubtless be an ongoing source of bugs.

Indeed, I wasn't expecting to have some different case non-probed
regulator ...

Anyway, it's possibly a moot point I think - Niklas has suggested that
he can indeed resolve the -EPROBE_DEFER restrictions.

I'm not yet sure if we want to go full MFD yet ... so I've left this
feature out of my latest posting for the driver - and we'll discuss the
direction for solving this in our team.

Thanks again,
-- 
Regards
--
Kieran

Re: Regulator probe on demand (or circular dependencies)

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On Wed, Dec 11, 2019 at 10:42:43PM +0000, Kieran Bingham wrote:
> On 09/12/2019 16:37, Mark Brown wrote:

> >>  - is there anything I can do here within regulator_dev_lookup() to
> >>    attempt creating the regulator_dev 'on-demand' when
> >>    of_find_regulator_by_node(node) returns empty? (or is that crazy, and
> >>    just a rabbit-hole?)

> > This seems like a terrible idea, you'll have a half baked regulator in

> Ohh eep, I just re-read my description, and I don't think I described my
> intention very well at all. (or at all!)

> I wouldn't want to have just a half baked struct regulator_dev on it's
> own ... I was more wondering if we can kick the core driver framework to
> fully probe this regulator (which would thus create the required
> regulator_dev structures). It was more a question of can we guide the
> core driver framework that it really needs to probe this device
> immediately ...

Oh, I see.  Last time I looked at this in any detail the driver core did
probe things pretty much immediately when it got a new driver to match a
device (or vice versa), some existing MFDs rely on that.

> I was sort of wondering if something like this could optimise away some
> of the -EPROBE_DEFER iterations at a more global level, but I don't know
> how or if that would work anyway.

In theory someone could try to do some sort of sorting with the DT
graph, people keep talking about it but nobody's done anything that I'm
aware of.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Regulator probe on demand (or circular dependencies)

[Geert Uytterhoeven]

Hi Mark,

On Thu, Dec 12, 2019 at 4:57 PM Mark Brown <broonie@kernel.org> wrote:
> On Wed, Dec 11, 2019 at 10:42:43PM +0000, Kieran Bingham wrote:
 sort of wondering if something like this could optimise away some
> > of the -EPROBE_DEFER iterations at a more global level, but I don't know
> > how or if that would work anyway.
>
> In theory someone could try to do some sort of sorting with the DT
> graph, people keep talking about it but nobody's done anything that I'm
> aware of.

"of_devlink" has landed in v5.5-rc1, cfr. commit a3e1d1a7f5fcccaf ("of:
property: Add functional dependency link from DT bindings").

I gave it a try on some boards lately.  It improved the deferral situation on
Koelsch, but made it worse on Salvator-XS.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Regulator probe on demand (or circular dependencies)

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On Thu, Dec 12, 2019 at 05:18:43PM +0100, Geert Uytterhoeven wrote:
> On Thu, Dec 12, 2019 at 4:57 PM Mark Brown <broonie@kernel.org> wrote:

> > In theory someone could try to do some sort of sorting with the DT
> > graph, people keep talking about it but nobody's done anything that I'm
> > aware of.

> "of_devlink" has landed in v5.5-rc1, cfr. commit a3e1d1a7f5fcccaf ("of:
> property: Add functional dependency link from DT bindings").

> I gave it a try on some boards lately.  It improved the deferral situation on
> Koelsch, but made it worse on Salvator-XS.

Ah, nice.  It's at least a start on that, that's good.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]