| Message ID | 20190522183142.11061-3-ajayg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: ccg: add runtime pm support | expand |
> @@ -211,6 +212,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap, > goto exit; > } > > + pm_runtime_mark_last_busy(i2cd->dev); > + pm_runtime_put_autosuspend(i2cd->dev); > return i; > exit: > if (send_stop) { > @@ -218,6 +221,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap, > if (status2 < 0) > dev_err(i2cd->dev, "i2c stop failed %d\n", status2); > } > + pm_runtime_mark_last_busy(i2cd->dev); > + pm_runtime_put_autosuspend(i2cd->dev); Maybe another worthwhile refactorization possible here? The exit code path and 'all good' code path look very similar. This can be done incrementally, though. I think for now it is okay. > +static __maybe_unused int gpu_i2c_suspend(struct device *dev) > +{ > + return 0; > +} Why do we need this?
Hi Wolfram, > -----Original Message----- > From: linux-usb-owner@vger.kernel.org <linux-usb-owner@vger.kernel.org> > On Behalf Of Wolfram Sang > Sent: Saturday, May 25, 2019 12:57 PM > To: Ajay Gupta <ajaykuee@gmail.com> > Cc: heikki.krogerus@linux.intel.com; linux-usb@vger.kernel.org; linux- > i2c@vger.kernel.org; Ajay Gupta <ajayg@nvidia.com> > Subject: Re: [PATCH v3 2/5] i2c: nvidia-gpu: add runtime pm support > > > > @@ -211,6 +212,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter > *adap, > > goto exit; > > } > > > > + pm_runtime_mark_last_busy(i2cd->dev); > > + pm_runtime_put_autosuspend(i2cd->dev); > > return i; > > exit: > > if (send_stop) { > > @@ -218,6 +221,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter > *adap, > > if (status2 < 0) > > dev_err(i2cd->dev, "i2c stop failed %d\n", status2); > > } > > + pm_runtime_mark_last_busy(i2cd->dev); > > + pm_runtime_put_autosuspend(i2cd->dev); > > Maybe another worthwhile refactorization possible here? The exit code path > and 'all good' code path look very similar. > This can be done incrementally, though. I think for now it is okay. Thanks for suggestion. Yes , it is possible to refactor a bit further here. > > +static __maybe_unused int gpu_i2c_suspend(struct device *dev) { > > + return 0; > > +} > > Why do we need this? I2C function of PCI bus remains in D0 (lspci output) without this function. Following document also seems to insist on this. " For this to work, the device's driver has to provide a pm->runtime_suspend() callback " Documentation/power/pci.txt "First, a PCI device is put into a low-power state, or suspended, with the help of pm_schedule_suspend() or pm_runtime_suspend() which for PCI devices call pci_pm_runtime_suspend() to do the actual job. For this to work, the device's driver has to provide a pm->runtime_suspend() callback (see below), which is run by pci_pm_runtime_suspend() as the first action. If the driver's callback returns successfully, the device's standard configuration registers are saved, the device is prepared to generate wakeup signals and, finally, it is put into the target low-power state." Thanks Ajay > nvpublic
Hi Wolfram, > > -----Original Message----- > > From: linux-usb-owner@vger.kernel.org <linux-usb- > owner@vger.kernel.org> > > On Behalf Of Wolfram Sang > > Sent: Saturday, May 25, 2019 12:57 PM > > To: Ajay Gupta <ajaykuee@gmail.com> > > Cc: heikki.krogerus@linux.intel.com; linux-usb@vger.kernel.org; linux- > > i2c@vger.kernel.org; Ajay Gupta <ajayg@nvidia.com> > > Subject: Re: [PATCH v3 2/5] i2c: nvidia-gpu: add runtime pm support > > > > > > > @@ -211,6 +212,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter > > *adap, > > > goto exit; > > > } > > > > > > + pm_runtime_mark_last_busy(i2cd->dev); > > > + pm_runtime_put_autosuspend(i2cd->dev); > > > return i; > > > exit: > > > if (send_stop) { > > > @@ -218,6 +221,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter > > *adap, > > > if (status2 < 0) > > > dev_err(i2cd->dev, "i2c stop failed %d\n", status2); > > > } > > > + pm_runtime_mark_last_busy(i2cd->dev); > > > + pm_runtime_put_autosuspend(i2cd->dev); > > > > Maybe another worthwhile refactorization possible here? The exit code > path > > and 'all good' code path look very similar. > > This can be done incrementally, though. I think for now it is okay. > Thanks for suggestion. Yes , it is possible to refactor a bit further here. > > > > +static __maybe_unused int gpu_i2c_suspend(struct device *dev) { > > > + return 0; > > > +} > > > > Why do we need this? > I2C function of PCI bus remains in D0 (lspci output) without this function. Do you still see any issue with gpu_i2c_suspend()? Thanks Ajay > nvpublic > > Following document also seems to insist on this. > " For this to work, the device's driver has to provide a > pm->runtime_suspend() callback " > > Documentation/power/pci.txt > "First, a PCI device is put into a low-power state, or suspended, with the help > of pm_schedule_suspend() or pm_runtime_suspend() which for PCI devices > call > pci_pm_runtime_suspend() to do the actual job. For this to work, the > device's > driver has to provide a pm->runtime_suspend() callback (see below), which > is > run by pci_pm_runtime_suspend() as the first action. If the driver's callback > returns successfully, the device's standard configuration registers are saved, > the device is prepared to generate wakeup signals and, finally, it is put into > the target low-power state." > > Thanks > Ajay > > nvpublic
diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c index 2d9561ec2320..28fee85135ac 100644 --- a/drivers/i2c/busses/i2c-nvidia-gpu.c +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c @@ -176,6 +176,7 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap, * The controller supports maximum 4 byte read due to known * limitation of sending STOP after every read. */ + pm_runtime_get_sync(i2cd->dev); for (i = 0; i < num; i++) { if (msgs[i].flags & I2C_M_RD) { /* program client address before starting read */ @@ -211,6 +212,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap, goto exit; } + pm_runtime_mark_last_busy(i2cd->dev); + pm_runtime_put_autosuspend(i2cd->dev); return i; exit: if (send_stop) { @@ -218,6 +221,8 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap, if (status2 < 0) dev_err(i2cd->dev, "i2c stop failed %d\n", status2); } + pm_runtime_mark_last_busy(i2cd->dev); + pm_runtime_put_autosuspend(i2cd->dev); return status; } @@ -337,6 +342,11 @@ static int gpu_i2c_probe(struct pci_dev *pdev, const struct pci_device_id *id) goto del_adapter; } + pm_runtime_set_autosuspend_delay(&pdev->dev, 3000); + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_put_autosuspend(&pdev->dev); + pm_runtime_allow(&pdev->dev); + return 0; del_adapter: @@ -350,10 +360,16 @@ static void gpu_i2c_remove(struct pci_dev *pdev) { struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev); + pm_runtime_get_noresume(i2cd->dev); i2c_del_adapter(&i2cd->adapter); pci_free_irq_vectors(pdev); } +static __maybe_unused int gpu_i2c_suspend(struct device *dev) +{ + return 0; +} + static __maybe_unused int gpu_i2c_resume(struct device *dev) { struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev); @@ -362,7 +378,8 @@ static __maybe_unused int gpu_i2c_resume(struct device *dev) return 0; } -static UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume, NULL); +static UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, gpu_i2c_suspend, gpu_i2c_resume, + NULL); static struct pci_driver gpu_i2c_driver = { .name = "nvidia-gpu",