Re: [Freedreno] [PATCH] drm/msm/dsi: Don't set a load before disabling a regulator

From: Doug Anderson <dianders@chromium.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Cc: "Sean Paul" <sean@poorly.run>, "Vinod Koul" <vkoul@kernel.org>,
	"Loic Poulain" <loic.poulain@linaro.org>,
	"José Expósito" <jose.exposito89@gmail.com>,
	"Jonathan Marek" <jonathan@marek.ca>,
	"David Airlie" <airlied@linux.ie>,
	"Rajeev Nandan" <quic_rajeevny@quicinc.com>,
	"Konrad Dybcio" <konrad.dybcio@somainline.org>,
	"AngeloGioacchino Del Regno"
	<angelogioacchino.delregno@somainline.org>,
	"Vladimir Lypak" <vladimir.lypak@gmail.com>,
	"Abhinav Kumar" <quic_abhinavk@quicinc.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	"Bjorn Andersson" <bjorn.andersson@linaro.org>,
	"Mark Brown" <broonie@kernel.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	"Marijn Suijten" <marijn.suijten@somainline.org>,
	"Stephen Boyd" <swboyd@chromium.org>,
	freedreno <freedreno@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [Freedreno] [PATCH] drm/msm/dsi: Don't set a load before disabling a regulator
Date: Wed, 27 Jul 2022 07:09:07 -0700	[thread overview]
Message-ID: <CAD=FV=WEK-CrFjSrs7ADfkQmoz=xOmppzyZtsEQGv0ga83i3CQ@mail.gmail.com> (raw)
In-Reply-To: <CAA8EJppHr7GjcvqRqXAgjw9mf+ObzCi58-VYSdO_JMfk1-4ifA@mail.gmail.com>

Hi,

On Wed, Jul 27, 2022 at 6:59 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Wed, 27 Jul 2022 at 16:57, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Jul 26, 2022 at 4:53 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote:
> > >
> > > On 7/25/2022 5:49 PM, Douglas Anderson wrote:
> > > > As of commit 5451781dadf8 ("regulator: core: Only count load for
> > > > enabled consumers"), a load isn't counted for a disabled
> > > > regulator. That means all the code in the DSI driver to specify and
> > > > set loads before disabling a regulator is not actually doing anything
> > > > useful. Let's remove it.
> > > >
> > > > It should be noted that all of the loads set that were being specified
> > > > were pointless noise anyway. The only use for this number is to pick
> > > > between low power and high power modes of regulators. Regulators
> > > > appear to do this changeover at loads on the order of 10000 uA. You
> > > > would a lot of clients of the same rail for that 100 uA number to
> > >
> > > I guess you meant "you would need a lot of clients"
> >
> > Yeah, sorry. :( I'll fix it up if I need a v3.
> >
> >
> > > > @@ -259,15 +259,7 @@ static inline struct msm_dsi_host *to_msm_dsi_host(struct mipi_dsi_host *host)
> > > >   static void dsi_host_regulator_disable(struct msm_dsi_host *msm_host)
> > > >   {
> > > It seems like now we can drop this function dsi_host_regulator_disable()
> > > entirely and just call regulator_bulk_disable() ?
> >
> > Sure, if you want. One could still argue that it provides a tiny bit
> > of abstraction and avoids the caller from having to know where to find
> > the number of regulators and all that, but I can go either way. Is
> > this worth a v3, do you think? If so, I might tack it on at the end of
> > the series.
>
> I'd say, drop it. Having extra single-call wrappers doesn't seem to add a lot.

Sounds good. I'll wait a little bit of time for feedback on the larger
series and then send a v3, probably next week.

-Doug