diff mbox series

[1/4] i2c: nvidia-gpu: add runtime pm support

Message ID 20190517163818.5007-2-ajayg@nvidia.com
State Superseded
Headers show
Series usb: typec: ucsi: ccg: add runtime pm support | expand

Commit Message

Ajay Gupta May 17, 2019, 4:38 p.m. UTC 
From: Ajay Gupta <ajayg@nvidia.com>

Enable runtime pm support with autosuspend delay of three second.
This is to make sure I2C client device Cypress CCGx has completed
all transaction.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/i2c/busses/i2c-nvidia-gpu.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

Comments

Wolfram Sang May 19, 2019, 2:47 p.m. UTC | #1 
> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
> index 1c8f708f212b..9d347583f8dc 100644
> --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -175,6 +175,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 */
> @@ -189,7 +190,7 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
>  			status = gpu_i2c_start(i2cd);
>  			if (status < 0) {
>  				if (i == 0)
> -					return status;
> +					goto exit;
>  				goto stop;

Hmm, goto here, goto there... OK, the code didn't have a good flow even
before this patch.

>  			}
>  
> @@ -206,13 +207,18 @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
>  	}
>  	status = gpu_i2c_stop(i2cd);
>  	if (status < 0)
> -		return status;
> +		goto exit;
>  
> +	pm_runtime_mark_last_busy(i2cd->dev);
> +	pm_runtime_put_autosuspend(i2cd->dev);
>  	return i;
>  stop:
>  	status2 = gpu_i2c_stop(i2cd);
>  	if (status2 < 0)
>  		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
> +exit:
> +	pm_runtime_mark_last_busy(i2cd->dev);
> +	pm_runtime_put_autosuspend(i2cd->dev);
>  	return status;
>  }

I am not nacking this, yet the use of goto here seems too much for my
taste. If you could try refactoring the whole code (dunno, maybe using a
flag when to use stop?), I'd appreciate this.
Ajay Gupta May 20, 2019, 5:02 p.m. UTC | #2 
Hi Wolfram,

> -----Original Message-----
> From: Wolfram Sang <wsa@the-dreams.de>
> Sent: Sunday, May 19, 2019 7:47 AM
> 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 1/4] i2c: nvidia-gpu: add runtime pm support
> 
> > diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c
> > b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > index 1c8f708f212b..9d347583f8dc 100644
> > --- a/drivers/i2c/busses/i2c-nvidia-gpu.c
> > +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > @@ -175,6 +175,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 */ @@ -
> 189,7 +190,7
> > @@ static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> >  			status = gpu_i2c_start(i2cd);
> >  			if (status < 0) {
> >  				if (i == 0)
> > -					return status;
> > +					goto exit;
> >  				goto stop;
> 
> Hmm, goto here, goto there... OK, the code didn't have a good flow even before
> this patch.
[...]
> 
> >  			}
> >
> > @@ -206,13 +207,18 @@ static int gpu_i2c_master_xfer(struct i2c_adapter
> *adap,
> >  	}
> >  	status = gpu_i2c_stop(i2cd);
> >  	if (status < 0)
> > -		return status;
> > +		goto exit;
> >
> > +	pm_runtime_mark_last_busy(i2cd->dev);
> > +	pm_runtime_put_autosuspend(i2cd->dev);
> >  	return i;
> >  stop:
> >  	status2 = gpu_i2c_stop(i2cd);
> >  	if (status2 < 0)
> >  		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
> > +exit:
> > +	pm_runtime_mark_last_busy(i2cd->dev);
> > +	pm_runtime_put_autosuspend(i2cd->dev);
> >  	return status;
> >  }
> 
> I am not nacking this, yet the use of goto here seems too much for my taste. If
> you could try refactoring the whole code (dunno, maybe using a flag when to
> use stop?), I'd appreciate this.
Ok, I will add a local variable to make it more readable.

Thanks
Ajay
> nvpublic
Wolfram Sang May 20, 2019, 5:17 p.m. UTC | #3 
> > I am not nacking this, yet the use of goto here seems too much for my taste. If
> > you could try refactoring the whole code (dunno, maybe using a flag when to
> > use stop?), I'd appreciate this.
> Ok, I will add a local variable to make it more readable.

I was brainstorming here, I am not sure if it will work or not. But you
will see. Maybe you get other ideas on the way. However, thanks for
trying out!
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
index 1c8f708f212b..9d347583f8dc 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -175,6 +175,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 */
@@ -189,7 +190,7 @@  static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
 			status = gpu_i2c_start(i2cd);
 			if (status < 0) {
 				if (i == 0)
-					return status;
+					goto exit;
 				goto stop;
 			}
 
@@ -206,13 +207,18 @@  static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
 	}
 	status = gpu_i2c_stop(i2cd);
 	if (status < 0)
-		return status;
+		goto exit;
 
+	pm_runtime_mark_last_busy(i2cd->dev);
+	pm_runtime_put_autosuspend(i2cd->dev);
 	return i;
 stop:
 	status2 = gpu_i2c_stop(i2cd);
 	if (status2 < 0)
 		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
+exit:
+	pm_runtime_mark_last_busy(i2cd->dev);
+	pm_runtime_put_autosuspend(i2cd->dev);
 	return status;
 }
 
@@ -332,6 +338,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:
@@ -345,10 +356,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 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);
@@ -357,7 +374,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",