Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CGME20230323083330eucas1p1f7e3f9beb5ba168c6b13374d74c158f0@eucas1p1.samsung.com>
 <20230323083312.199189-1-m.szyprowski@samsung.com> <20230323114035.GL68926@ediswmail.ad.cirrus.com>
In-Reply-To: <20230323114035.GL68926@ediswmail.ad.cirrus.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 23 Mar 2023 09:53:18 -0700
Message-ID: <CAD=FV=UYO1KaoAZ7o5cA83SC1VHRomvJfaXVWyYPKrEZHyNNjg@mail.gmail.com>
Subject: Re: [PATCH] regulator: wm8994: Use PROBE_FORCE_SYNCHRONOUS
To:     Charles Keepax <ckeepax@opensource.cirrus.com>
Cc:     Marek Szyprowski <m.szyprowski@samsung.com>,
        linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>, patches@opensource.cirrus.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi,

On Thu, Mar 23, 2023 at 4:40=E2=80=AFAM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
>
> On Thu, Mar 23, 2023 at 09:33:12AM +0100, Marek Szyprowski wrote:
> > Restore synchronous probing for wm8994 regulators because otherwise the
> > sound device is never initialized on Exynos5250-based Arndale board.
> >
> > Fixes: 259b93b21a9f ("regulator: Set PROBE_PREFER_ASYNCHRONOUS for driv=
ers that existed in 4.14")
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> > ---
> >  drivers/regulator/wm8994-regulator.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/regulator/wm8994-regulator.c b/drivers/regulator/w=
m8994-regulator.c
> > index 8921051a00e9..2946db448aec 100644
> > --- a/drivers/regulator/wm8994-regulator.c
> > +++ b/drivers/regulator/wm8994-regulator.c
> > @@ -227,7 +227,7 @@ static struct platform_driver wm8994_ldo_driver =3D=
 {
> >       .probe =3D wm8994_ldo_probe,
> >       .driver         =3D {
> >               .name   =3D "wm8994-ldo",
> > -             .probe_type =3D PROBE_PREFER_ASYNCHRONOUS,
> > +             .probe_type =3D PROBE_FORCE_SYNCHRONOUS,
> >       },
> >  };
>
> Acked-by: Charles Keepax <ckeepax@opensource.cirrus.com>
>
> Yes, these seems to be a wider problem with these complex CODECs
> that have an internal LDO. Changing to async probe, means the
> internal LDO driver doesn't probe before the code in the main MFD
> carries on, which means the regulator framework finds no driver
> and swaps in the dummy. Which means the CODEC never powers up.
>
> I think these basically have to be forced sync, I will do a patch
> to update the other devices working like this.

Sorry for the breakage and thank you for the fix.

No question that a quick switch back to PROBE_FORCE_SYNCHRONOUS is the
right first step here, but I'm wondering if there are any further
steps we want to take.

If my analysis is correct, there's still potential to run into similar
problems even with PROBE_FORCE_SYNCHRONOUS. I don't think that
mfd_add_devices() is _guaranteed_ to finish probing all the
sub-devices by the time it returns. Specifically, imagine that
wm8994_ldo_probe() tries to get a GPIO that that system hasn't made
available yet. Potentially the system could return -EPROBE_DEFER there
and that would put the LDO on the deferred probe list. Doing so won't
cause mfd_add_devices() to fail. Now we'll end up with a dummy
regulator again. Admittedly most cases GPIO providers probe really
early and so this argument is a bit of a stretch, but I guess the
point is that there's no official guarantee that mfd_add_devices()
will finish probing all sub-devices so it's not ideal to rely on.
Also, other drivers with a similar pattern might have other reasons to
-EPROBE_DEFER.

These types of issues are the same ones I faced with DP AUX bus and
the solutions were never super amazing...

One solution we ended up with for the DP AUX bus was to add a
"done_probing" callback argument to devm_of_dp_aux_populate_bus().
This allowed the parent to be called back when all the children were
done probing. This implementation is easier for DP AUX bus than it
would be in the generic MFD case, but conceivably it would be possible
there too?

Another possible solution is to somehow break the driver up into more
sub-drivers. Essentially, you have a top-level "wm8994" that's not
much more than a container. Then you create a new-sub-device and
relegate anything that needs the regulators to the new sub-device. The
new sub-device can just -EPROBE_DEFER until it detects that the
regulators have finished probing. I ended up doing stuff like this for
"ti-sn65dsi86.c" using the Linux aux bus (not to be confused with the
DP Aux bus) and it was a bit odd but worked OK.

-Doug