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
>
> Avoid words like 'proper' here (what are non-proper runtime PM ops?).
Sure.
>
> > 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. Anyway, ignore this version as it
was already NACKed by Dmitry.
>
> > 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.
>
> > +static int __maybe_unused qcom_edp_runtime_suspend(struct device *dev)
> > +{
> > + struct qcom_edp *edp = dev_get_drvdata(dev);
> > +
> > + dev_err(dev, "Suspending DP phy\n");
>
> You forgot to drop your development printks (same below).
>
> Johan
