Re: [PATCH] phy: qcom: edp: Add runtime PM support

From: Abel Vesa <abel.vesa@linaro.org>
To: Johan Hovold <johan@kernel.org>
Cc: Vinod Koul <vkoul@kernel.org>,
	Kishon Vijay Abraham I <kishon@kernel.org>,
	Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
	Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
	linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] phy: qcom: edp: Add runtime PM support
Date: Fri, 20 Sep 2024 12:02:25 +0300	[thread overview]
Message-ID: <Zu06IXym0it9UPpa@linaro.org> (raw)
In-Reply-To: <Zu007YUYie7bEQvj@hovoldconsulting.com>

On 24-09-20 10:40:13, Johan Hovold wrote:
> On Wed, Sep 18, 2024 at 04:10:05PM +0300, Abel Vesa wrote:
> > On 24-09-18 12:05:59, Johan Hovold wrote:
> > > On Sat, Sep 07, 2024 at 06:25:21PM +0300, Abel Vesa wrote:
> > > > Enable runtime PM support by adding proper ops which will handle the
>  
> > > > clocks and regulators. These resources will now be handled on power_on and
> > > > power_off instead of init and exit PHY ops.
> > > 
> > > No, this is simply a false claim and indicates that you haven't reviewed
> > > how PHY runtime PM works. Core will increment the usage count on init()
> > > and decrement it on exit().
> > 
> > Yeah, I guess the better argument here would be that the PHY needs
> > regulators and clocks enabled
> 
> No, that's already handled today so is clearly not a valid argument.

I think we're saying the same thing here. I was trying to say that,
as it is currently done and it's correct, the init() needs those
resources enabled.

> 
> > Anyway, ignore this version as it was already NACKed by Dmitry.
> 
> No, my feedback is still valid, and you're bound to repeat the same
> mistakes over and over again unless you try to understand what I've been
> saying here.

Duly noted. My reply wasn't trying to dismiss your feedback.

Thanks for your feedback.

> 
> > > > Also enable these resources on
> > > > probe in order to balance out the disabling that is happening right after.
> > > > Prevent runtime PM from being ON by default as well.
> > > 
> > > And here you just regressed all current systems that do not have udev
> > > rules to enable runtime PM, and which will now be stuck with these
> > > resources always-on (e.g. during DPMS off and system suspend).
> > > 
> > > In fact, you are even regressing systems that would enable runtime PM,
> > > as the runtime suspend callback would not currently be called when you
> > > enter system suspend so the regulators and clocks will be left on.
> > > 
> > > This clearly hasn't been tested and analysed properly.
> 
> Johan