[PATCH 0/4] usb: typec: ucsi: ccg: add runtime pm support

[PATCH 0/4] usb: typec: ucsi: ccg: add runtime pm support

[Ajay Gupta]

Hi Heikki

These patches add support for runtime power management for UCSI CCGx driver.
I have tested them with NVIDIA GPU card which has i2c interface to talk to
CCG controller. I have added runtime pm support for the i2c bus driver as well.

First two patches add support for runtime pm in i2c bus driver and UCSI CCGx
driver.

Last two patches add workaround for an old version of ccg firmware
which has known issue of not missing interrupt when a device is connected
to runtime resume the ccg controller. The workaround is needed because
if a GPU card doesn't get new firmware but gets new kernel then also it
should continue to work.

Thanks
Ajay

Ajay Gupta (4):
  i2c: nvidia-gpu: add runtime pm support
  usb: typec: ucsi: ccg: enable runtime pm support
  i2c: nvidia-gpu: resume ccgx i2c client
  usb: typec: ucsi: ccg: add runtime pm workaround

 drivers/i2c/busses/i2c-nvidia-gpu.c |  37 ++++++--
 drivers/usb/typec/ucsi/ucsi.c       |   1 +
 drivers/usb/typec/ucsi/ucsi_ccg.c   | 138 +++++++++++++++++++++++++++-
 3 files changed, 165 insertions(+), 11 deletions(-)

-- 
2.17.1

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

[Ajay Gupta]

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(-)

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",
-- 
2.17.1

[PATCH 2/4] usb: typec: ucsi: ccg: enable runtime pm support

[Ajay Gupta]

From: Ajay Gupta <ajayg@nvidia.com>

The change enables runtime pm support to UCSI CCG driver.
ucsi_send_command() is used in resume path and so exported
ucsi_send_command() symbol in ucsi.c for modular build.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/usb/typec/ucsi/ucsi.c     |  1 +
 drivers/usb/typec/ucsi/ucsi_ccg.c | 60 +++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 7850b851cecd..e9454134d399 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -206,6 +206,7 @@ int ucsi_send_command(struct ucsi *ucsi, struct ucsi_control *ctrl,
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ucsi_send_command);
 
 /* -------------------------------------------------------------------------- */
 
diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index 9d46aa9e4e35..cc7094ecda2d 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -14,6 +14,8 @@
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
 
 #include <asm/unaligned.h>
 #include "ucsi.h"
@@ -210,6 +212,7 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	if (quirks && quirks->max_read_len)
 		max_read_len = quirks->max_read_len;
 
+	pm_runtime_get_sync(uc->dev);
 	while (rem_len > 0) {
 		msgs[1].buf = &data[len - rem_len];
 		rlen = min_t(u16, rem_len, max_read_len);
@@ -218,12 +221,14 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 		status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 		if (status < 0) {
 			dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+			pm_runtime_put_sync(uc->dev);
 			return status;
 		}
 		rab += rlen;
 		rem_len -= rlen;
 	}
 
+	pm_runtime_put_sync(uc->dev);
 	return 0;
 }
 
@@ -249,13 +254,16 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	msgs[0].len = len + sizeof(rab);
 	msgs[0].buf = buf;
 
+	pm_runtime_get_sync(uc->dev);
 	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 	if (status < 0) {
 		dev_err(uc->dev, "i2c_transfer failed %d\n", status);
+		pm_runtime_put_sync(uc->dev);
 		kfree(buf);
 		return status;
 	}
 
+	pm_runtime_put_sync(uc->dev);
 	kfree(buf);
 	return 0;
 }
@@ -1134,6 +1142,10 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	if (status)
 		dev_err(uc->dev, "cannot create sysfs group: %d\n", status);
 
+	pm_runtime_set_active(uc->dev);
+	pm_runtime_enable(uc->dev);
+	pm_runtime_idle(uc->dev);
+
 	return 0;
 }
 
@@ -1143,6 +1155,7 @@ static int ucsi_ccg_remove(struct i2c_client *client)
 
 	cancel_work_sync(&uc->work);
 	ucsi_unregister_ppm(uc->ucsi);
+	pm_runtime_disable(uc->dev);
 	free_irq(uc->irq, uc);
 	sysfs_remove_group(&uc->dev->kobj, &ucsi_ccg_attr_group);
 
@@ -1155,9 +1168,56 @@ static const struct i2c_device_id ucsi_ccg_device_id[] = {
 };
 MODULE_DEVICE_TABLE(i2c, ucsi_ccg_device_id);
 
+static int ucsi_ccg_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int ucsi_ccg_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ucsi_ccg *uc = i2c_get_clientdata(client);
+	struct ucsi *ucsi = uc->ucsi;
+	struct ucsi_control c;
+	int ret;
+
+	/* restore UCSI notification enable mask */
+	UCSI_CMD_SET_NTFY_ENABLE(c, UCSI_ENABLE_NTFY_ALL);
+	ret = ucsi_send_command(ucsi, &c, NULL, 0);
+	if (ret < 0) {
+		dev_err(uc->dev, "%s: failed to set notification enable - %d\n",
+			__func__, ret);
+	}
+	return 0;
+}
+
+static int ucsi_ccg_runtime_suspend(struct device *dev)
+{
+	return 0;
+}
+
+static int ucsi_ccg_runtime_resume(struct device *dev)
+{
+	return 0;
+}
+
+static int ucsi_ccg_runtime_idle(struct device *dev)
+{
+	return 0;
+}
+
+static const struct dev_pm_ops ucsi_ccg_pm = {
+	.suspend = ucsi_ccg_suspend,
+	.resume = ucsi_ccg_resume,
+	.runtime_suspend = ucsi_ccg_runtime_suspend,
+	.runtime_resume = ucsi_ccg_runtime_resume,
+	.runtime_idle = ucsi_ccg_runtime_idle,
+};
+
 static struct i2c_driver ucsi_ccg_driver = {
 	.driver = {
 		.name = "ucsi_ccg",
+		.pm = &ucsi_ccg_pm,
 	},
 	.probe = ucsi_ccg_probe,
 	.remove = ucsi_ccg_remove,
-- 
2.17.1

[PATCH 3/4] i2c: nvidia-gpu: resume ccgx i2c client

[Ajay Gupta]

From: Ajay Gupta <ajayg@nvidia.com>

Cypress USB Type-C CCGx controller firmware (which is being used
in many NVIDIA GPU cards) has known issue of not triggering
interrupt when a USB device is hot plugged to runtime resume the
controller. Many of these cards may get latest kernel but may not
get latest fixed firmware so a workaround to check for any
connector change event.

The workaround is to request runtime resume of i2c client
which is UCSI Cypress CCGx driver. CCG driver will call the ISR
for any connector change event only if NVIDIA GPU has old
CCG firmware with the known issue.

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

diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
index 9d347583f8dc..2f72135a547b 100644
--- a/drivers/i2c/busses/i2c-nvidia-gpu.c
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -51,6 +51,7 @@ struct gpu_i2c_dev {
 	void __iomem *regs;
 	struct i2c_adapter adapter;
 	struct i2c_board_info *gpu_ccgx_ucsi;
+	struct i2c_client *ccgx_client;
 };
 
 static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd)
@@ -267,8 +268,6 @@ static const struct property_entry ccgx_props[] = {
 
 static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq)
 {
-	struct i2c_client *ccgx_client;
-
 	i2cd->gpu_ccgx_ucsi = devm_kzalloc(i2cd->dev,
 					   sizeof(*i2cd->gpu_ccgx_ucsi),
 					   GFP_KERNEL);
@@ -280,8 +279,8 @@ static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq)
 	i2cd->gpu_ccgx_ucsi->addr = 0x8;
 	i2cd->gpu_ccgx_ucsi->irq = irq;
 	i2cd->gpu_ccgx_ucsi->properties = ccgx_props;
-	ccgx_client = i2c_new_device(&i2cd->adapter, i2cd->gpu_ccgx_ucsi);
-	if (!ccgx_client)
+	i2cd->ccgx_client = i2c_new_device(&i2cd->adapter, i2cd->gpu_ccgx_ucsi);
+	if (!i2cd->ccgx_client)
 		return -ENODEV;
 
 	return 0;
@@ -371,6 +370,12 @@ static __maybe_unused int gpu_i2c_resume(struct device *dev)
 	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
 
 	gpu_enable_i2c_bus(i2cd);
+	/* runtime resume ccgx client so that it can see for any
+	 * connector change event. Old ccg firmware has known
+	 * issue of not triggering interrupt when a device is
+	 * connected to runtime resume the controller.
+	 */
+	pm_request_resume(&i2cd->ccgx_client->dev);
 	return 0;
 }
 
-- 
2.17.1

[PATCH 4/4] usb: typec: ucsi: ccg: add runtime pm workaround

[Ajay Gupta]

From: Ajay Gupta <ajayg@nvidia.com>

Cypress USB Type-C CCGx controller firmware verson 3.1.10
(which is being used in many NVIDIA GPU cards) has known issue of
not triggering interrupt when a USB device is hot plugged to runtime
resume the controller. Many of these cards may get latest kernel but
may not get latest fixed firmware so a workaround is required to
check for any connector change event.

The workaround is that i2c bus driver will call pm_request_resume()
to runtime resume ucsi_ccg driver. CCG driver will call the ISR
for any connector change event only if NVIDIA GPU has old
CCG firmware with the known issue.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
---
 drivers/usb/typec/ucsi/ucsi_ccg.c | 78 +++++++++++++++++++++++++++++--
 1 file changed, 74 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c b/drivers/usb/typec/ucsi/ucsi_ccg.c
index cc7094ecda2d..3457e112fdbc 100644
--- a/drivers/usb/typec/ucsi/ucsi_ccg.c
+++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
@@ -109,6 +109,8 @@ struct version_format {
 	__le16 build;
 	u8 patch;
 	u8 ver;
+#define CCG_VERSION_PATCH(x) ((x) << 16)
+#define CCG_VERSION(x)	((x) << 24)
 #define CCG_VERSION_MIN_SHIFT (0)
 #define CCG_VERSION_MIN_MASK (0xf << CCG_VERSION_MIN_SHIFT)
 #define CCG_VERSION_MAJ_SHIFT (4)
@@ -172,6 +174,7 @@ struct ucsi_ccg {
 	struct ccg_dev_info info;
 	/* version info for boot, primary and secondary */
 	struct version_info version[FW2 + 1];
+	u32 fw_version;
 	/* CCG HPI communication flags */
 	unsigned long flags;
 #define RESET_PENDING	0
@@ -185,6 +188,10 @@ struct ucsi_ccg {
 
 	/* fw build with vendor information */
 	u16 fw_build;
+	bool run_isr; /* flag to call ISR routine during resume */
+	struct work_struct pm_work;
+	u16 old_fw_build;
+	u32 old_fw_version;
 };
 
 static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
@@ -212,6 +219,17 @@ static int ccg_read(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	if (quirks && quirks->max_read_len)
 		max_read_len = quirks->max_read_len;
 
+	if (uc->old_fw_build == uc->fw_build &&
+	    uc->fw_version <= uc->old_fw_version) {
+		mutex_lock(&uc->lock);
+		/* do not schedule pm_work to run ISR in
+		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
+		 * since we are already in ISR path.
+		 */
+		uc->run_isr = false;
+		mutex_unlock(&uc->lock);
+	}
+
 	pm_runtime_get_sync(uc->dev);
 	while (rem_len > 0) {
 		msgs[1].buf = &data[len - rem_len];
@@ -254,6 +272,17 @@ static int ccg_write(struct ucsi_ccg *uc, u16 rab, u8 *data, u32 len)
 	msgs[0].len = len + sizeof(rab);
 	msgs[0].buf = buf;
 
+	if (uc->old_fw_build == uc->fw_build &&
+	    uc->fw_version <= uc->old_fw_version) {
+		mutex_lock(&uc->lock);
+		/* do not schedule pm_work to run ISR in
+		 * ucsi_ccg_runtime_resume() after pm_runtime_get_sync()
+		 * since we are already in ISR path.
+		 */
+		uc->run_isr = false;
+		mutex_unlock(&uc->lock);
+	}
+
 	pm_runtime_get_sync(uc->dev);
 	status = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
 	if (status < 0) {
@@ -383,6 +412,13 @@ static irqreturn_t ccg_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static void ccg_pm_workaround_work(struct work_struct *pm_work)
+{
+	struct ucsi_ccg *uc = container_of(pm_work, struct ucsi_ccg, pm_work);
+
+	ucsi_notify(uc->ucsi);
+}
+
 static int get_fw_info(struct ucsi_ccg *uc)
 {
 	int err;
@@ -392,6 +428,9 @@ static int get_fw_info(struct ucsi_ccg *uc)
 	if (err < 0)
 		return err;
 
+	uc->fw_version = CCG_VERSION(uc->version[FW2].app.ver) |
+			CCG_VERSION_PATCH(uc->version[FW2].app.patch);
+
 	err = ccg_read(uc, CCGX_RAB_DEVICE_MODE, (u8 *)(&uc->info),
 		       sizeof(uc->info));
 	if (err < 0)
@@ -740,11 +779,12 @@ static bool ccg_check_fw_version(struct ucsi_ccg *uc, const char *fw_name,
 	}
 
 	/* compare input version with FWCT version */
-	cur_version = le16_to_cpu(app->build) | app->patch << 16 |
-			app->ver << 24;
+	cur_version = le16_to_cpu(app->build) | CCG_VERSION_PATCH(app->patch) |
+			CCG_VERSION(app->ver);
 
-	new_version = le16_to_cpu(fw_cfg.app.build) | fw_cfg.app.patch << 16 |
-			fw_cfg.app.ver << 24;
+	new_version = le16_to_cpu(fw_cfg.app.build) |
+			CCG_VERSION_PATCH(fw_cfg.app.patch) |
+			CCG_VERSION(fw_cfg.app.ver);
 
 	if (!ccg_check_vendor_version(uc, app, &fw_cfg))
 		goto out_release_firmware;
@@ -1084,8 +1124,16 @@ static int ucsi_ccg_probe(struct i2c_client *client,
 	uc->ppm.sync = ucsi_ccg_sync;
 	uc->dev = dev;
 	uc->client = client;
+	uc->run_isr = true;
 	mutex_init(&uc->lock);
 	INIT_WORK(&uc->work, ccg_update_firmware);
+	INIT_WORK(&uc->pm_work, ccg_pm_workaround_work);
+
+	/* Firmware version 3.1.10 or earlier, built for NVIDIA has known issue
+	 * of missing interrupt when a device is connected for runtime resume
+	 */
+	uc->old_fw_build = ('n' << 8) | 'v';
+	uc->old_fw_version = CCG_VERSION(0x31) | CCG_VERSION_PATCH(10);
 
 	/* Only fail FW flashing when FW build information is not provided */
 	status = device_property_read_u16(dev, "ccgx,firmware-build",
@@ -1153,6 +1201,7 @@ static int ucsi_ccg_remove(struct i2c_client *client)
 {
 	struct ucsi_ccg *uc = i2c_get_clientdata(client);
 
+	cancel_work_sync(&uc->pm_work);
 	cancel_work_sync(&uc->work);
 	ucsi_unregister_ppm(uc->ucsi);
 	pm_runtime_disable(uc->dev);
@@ -1198,6 +1247,27 @@ static int ucsi_ccg_runtime_suspend(struct device *dev)
 
 static int ucsi_ccg_runtime_resume(struct device *dev)
 {
+	struct i2c_client *client = to_i2c_client(dev);
+	struct ucsi_ccg *uc = i2c_get_clientdata(client);
+	bool schedule = true;
+
+	/* Firmware version "old_fw_version" or earlier, built for NVIDIA has
+	 * known issue of missing interrupt when a device is connected for
+	 * runtime resume. Schedule a work to call ISR as a workaround.
+	 */
+	if (uc->old_fw_build == uc->fw_build &&
+	    uc->fw_version <= uc->old_fw_version) {
+		mutex_lock(&uc->lock);
+		if (!uc->run_isr) {
+			uc->run_isr = true;
+			schedule = false;
+		}
+		mutex_unlock(&uc->lock);
+
+		if (schedule)
+			schedule_work(&uc->pm_work);
+	}
+
 	return 0;
 }
 
-- 
2.17.1

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1665 bytes --]

> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] i2c: nvidia-gpu: resume ccgx i2c client

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]


> +	/* runtime resume ccgx client so that it can see for any
> +	 * connector change event. Old ccg firmware has known
> +	 * issue of not triggering interrupt when a device is
> +	 * connected to runtime resume the controller.
> +	 */

Check coding style for comments.

BTW how do you suggest this gets upstream? Through the usb-tree? Would
be fine with me. Are there any dependencies? The cover-letter doesn't
mention it.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Ajay Gupta]

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

RE: [PATCH 3/4] i2c: nvidia-gpu: resume ccgx i2c client

[Ajay Gupta]

Hi Wolfram,

> -----Original Message-----
> From: Wolfram Sang <wsa@the-dreams.de>
> Sent: Sunday, May 19, 2019 7:49 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 3/4] i2c: nvidia-gpu: resume ccgx i2c client
> 
> 
> > +	/* runtime resume ccgx client so that it can see for any
> > +	 * connector change event. Old ccg firmware has known
> > +	 * issue of not triggering interrupt when a device is
> > +	 * connected to runtime resume the controller.
> > +	 */
> 
> Check coding style for comments.
Sure, will fix.
> 
> BTW how do you suggest this gets upstream? Through the usb-tree? Would be
> fine with me. Are there any dependencies? The cover-letter doesn't mention it.
I would prefer it to go through usb-tree since usb ucsi_ccg driver is the main driver
getting runtime pm functionality with the series. I will add this information in cover
letter too.

Thanks
Ajay
>nvpublic

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 416 bytes --]


> > 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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]