Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 11 Nov 2020 16:24:58 +0530
From:   Vaibhav Gupta <vaibhavgupta40@gmail.com>
To:     Jarkko Nikula <jarkko.nikula@linux.intel.com>
Cc:     Ajay Gupta <ajayg@nvidia.com>, Bjorn Helgaas <helgaas@kernel.org>,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        Bjorn Helgaas <bhelgaas@google.com>
Subject: Re: [PATCH v1] i2c: nvidia-gpu: drop empty stub for runtime pm
Message-ID: <X6vDAttVkugjceaX@gmail.com>
References: <20201107082151.58239-1-vaibhavgupta40@gmail.com>
 <20201107090442.GA107675@gmail.com>
 <6989eb51-a36c-6e43-86f3-9b02ab490d95@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <6989eb51-a36c-6e43-86f3-9b02ab490d95@linux.intel.com>
Precedence: bulk

On Tue, Nov 10, 2020 at 02:33:43PM +0200, Jarkko Nikula wrote:
> On 11/7/20 11:04 AM, Vaibhav Gupta wrote:
> > On Sat, Nov 07, 2020 at 01:51:51PM +0530, Vaibhav Gupta wrote:
> > > After the commit c5eb1190074c ("PCI / PM: Allow runtime PM without callback
> > > functions") we no more need empty stubs for runtime-pm to work.
> > > 
> > > The driver has no device specific task(s) for .suspend() . The stub was
> > > placed just for runtime-pm, which can be dropped now.
> > > 
> > > Reported-by: Bjorn Helgaas <bhelgaas@google.com>
> > > Signed-off-by: Vaibhav Gupta <vaibhavgupta40@gmail.com>
> > > ---
> > >   drivers/i2c/busses/i2c-nvidia-gpu.c | 10 +---------
> > >   1 file changed, 1 insertion(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > > index f9a69b109e5c..6b20601ffb13 100644
> > > --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
> > > +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > > @@ -353,15 +353,7 @@ static void gpu_i2c_remove(struct pci_dev *pdev)
> > >   	pci_free_irq_vectors(pdev);
> > >   }
> > > -/*
> > > - * We need gpu_i2c_suspend() even if it is stub, for runtime pm to work
> > > - * correctly. Without it, lspci shows runtime pm status as "D0" for the card.
> > > - * Documentation/power/pci.rst also insists for driver to provide this.
> > > - */
> > > -static __maybe_unused int gpu_i2c_suspend(struct device *dev)
> > > -{
> > > -	return 0;
> > > -}
> > > +#define gpu_i2c_suspend NULL
> Perhaps we can put NULL directly into UNIVERSAL_DEV_PM_OPS() for the suspend
> callback?
> 
Yes. I have noticed that the approach for this is random. Many drivers pass
NULL directly to the dev_pm_ops type variable, and rest of them use a macro.

I used it for symmetry. I mean there is 'gpu_i2c_resume', so although a macro,
I have put a 'gpu_i2c_suspend'.

Although it won't make any significant change, but if required, I can send
another patch where NULL is passed into UNIVERSAL_DEV_PM_OPS() instead.
> > >   static __maybe_unused int gpu_i2c_resume(struct device *dev)
> > >   {
> > > -- 
> > > 2.28.0
> > > 
> > The patch is only compile-tested.
> > 
> It should work also system suspend point of view. This patch affects also it
> since callbacks are set with the UNIVERSAL_DEV_PM_OPS(). Means the same
> callback is used for runtime PM and system suspend.
> 
> I quickly debugged this with an another driver and PCI stack does put the
> device into D3 state in system suspend from pci_prepare_to_sleep() if the
> suspend callback is not set and device was on prior it.
> 
> Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>


Thanks for debugging. Theoretically too, according to generic framework, drivers
are supposed to do only device specific jobs. If there is none, or callback is
set as NULL, remaining work to put the device in lower power state is PCI
stack's task.

--Vaibhav