Skip to contentSkip to conversation

i2c: buses: add i2c bus driver for NVIDIA GPU

PATCH New Thierry Reding 1 participant 1 message Aug 27, 2018 USB

lore
Share & tools
lore

Thierry Reding

author
#0 lore raw

PATCH v2 1/2 i2c: buses: add i2c bus driver for NVIDIA GPU

Quoted 10 lines · Ajay Gupta

Latest NVIDIA GPU card has USB Type-C interface. There is a Type-C controller which can be accessed over I2C.

This driver add I2C bus driver to communicate with Type-C controller. I2C client driver will be part of USB Type-C UCSI driver.

Signed-off-by Ajay Gupta <[email protected]>

---
Changes from v1 -> v2: None

Quoted 8 lines
 Documentation/i2c/busses/i2c-gpu |  18 ++
 MAINTAINERS                      |   7 +
 drivers/i2c/busses/Kconfig       |   9 +
 drivers/i2c/busses/Makefile      |   1 +
 drivers/i2c/busses/i2c-gpu.c     | 493 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 528 insertions(+)
 create mode 100644 Documentation/i2c/busses/i2c-gpu
 create mode 100644 drivers/i2c/busses/i2c-gpu.c

Hi Ajay,

I think this looks pretty good. A couple of minor, mostly nit-picky, comments below.

Documentation/i2c/busses/i2c-gpu

Quoted 6 lines
Documentation/i2c/busses/i2c-gpu new file +0 -0
new file mode 100644
index 0000000..873ba34

I think this is too generic. Maybe use something like i2c-nvidia-gpu here and everywhere else, to make it explicit that this is for NVIDIA GPUs rather than GPUs in general.

Quoted 88 lines
Documentation/i2c/busses/i2c-gpu new file +18 -0
@@ -0,0 +1,18 @@
+Kernel driver i2c-gpu
+
+Datasheet: not publicly available.
+
+Authors:
+	Ajay Gupta <[email protected]>
+
+Description
+-----------
+
+i2c-gpu is a driver for I2C controller included in NVIDIA Turing and later
+GPUs and it is used to communicate with Type-C controller on GPUs.
+
+If your 'lspci -v' listing shows something like the following,
+
+01:00.3 Serial bus controller [0c80]: NVIDIA Corporation Device 1ad9 (rev a1)
+
+then this driver should support the I2C controller of your GPU.
MAINTAINERS +7 -0
index b2fcd1c..e99f8a2 100644
@@ -6796,6 +6796,13 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/i2c/i2c-core-acpi.c
 
+I2C CONTROLLER DRIVER FOR NVIDIA GPU
+M:	Ajay Gupta <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	Documentation/i2c/busses/i2c-gpu
+F:	drivers/i2c/busses/i2c-gpu.c
+
 I2C MUXES
 M:	Peter Rosin <[email protected]>
 L:	[email protected]
drivers/i2c/busses/Kconfig +9 -0
index 451d4ae..ff8b2d4 100644
@@ -224,6 +224,15 @@ config I2C_NFORCE2_S4985
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-nforce2-s4985.
 
+config I2C_GPU
+	tristate "NVIDIA GPU I2C controller"
+	depends on PCI
+	help
+	  If you say yes to this option, support will be included for the
+	  NVIDIA GPU I2C controller which is used to communicate with the GPU's
+	  Type-C controller. This driver can also be built as a module called
+	  i2c-gpu.ko.
+
 config I2C_SIS5595
 	tristate "SiS 5595"
 	depends on PCI
drivers/i2c/busses/Makefile +1 -0
index 18b26af..15d2894 100644
@@ -140,5 +140,6 @@ obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
 obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
 obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
 obj-$(CONFIG_I2C_FSI)		+= i2c-fsi.o
+obj-$(CONFIG_I2C_GPU)          += i2c-gpu.o
 
 ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
drivers/i2c/busses/i2c-gpu.c new file +14 -0
new file mode 100644
index 0000000..0fd2944
@@ -0,0 +1,493 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nvidia GPU I2C controller Driver
+ *
+ * Copyright (C) 2018 NVIDIA Corporation. All rights reserved.
+ * Author: Ajay Gupta <[email protected]>
+ *
+ */
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>

drivers/i2c/busses/i2c-gpu.c line 24

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+
+/* STATUS definitions  */
+#define STATUS_SUCCESS			0
+#define STATUS_UNSUCCESSFUL		0x80000000UL
+#define STATUS_TIMEOUT			0x80000001UL
+#define STATUS_IO_DEVICE_ERROR		0x80000002UL
+#define STATUS_IO_TIMEOUT		0x80000004UL
+#define STATUS_IO_PREEMPTED		0x80000008UL

This seems a little odd. Can these not be converted to standard errno codes? It's more idiomatic to return 0 on success (like you define in the above) and a negative error code on failure. If you use that scheme, there's no need to have a symbolic name for success because it is used pretty much everywhere.

See include/uapi/asm-generic/errno{,-base}.h for a list of standard error codes. I think you should be able to find a match for each of the above.

drivers/i2c/busses/i2c-gpu.c line 26

Quoted 2 lines
drivers/i2c/busses/i2c-gpu.c new file +2 -0
+/* Cypress Type-C controllers (CCGx) device */
+#define CCGX_I2C_DEV_ADDRESS		0x08

I don't think there's a need for the define here. You only use the address once, and that's where the client is defined, so you can just use 0x08 literally in that one location.

Quoted 48 lines
drivers/i2c/busses/i2c-gpu.c new file +48 -0
+/* I2C definitions */
+#define I2C_MST_CNTL				0x00
+#define I2C_MST_CNTL_GEN_START			(1 << 0)
+#define I2C_MST_CNTL_GEN_STOP			(1 << 1)
+#define I2C_MST_CNTL_CMD_NONE			(0 << 2)
+#define I2C_MST_CNTL_CMD_READ			(1 << 2)
+#define I2C_MST_CNTL_CMD_WRITE			(2 << 2)
+#define I2C_MST_CNTL_CMD_RESET			(3 << 2)
+#define I2C_MST_CNTL_GEN_RAB			(1 << 4)
+#define I2C_MST_CNTL_BURST_SIZE_SHIFT		(6)
+#define I2C_MST_CNTL_GEN_NACK			(1 << 28)
+#define I2C_MST_CNTL_STATUS			(3 << 29)
+#define I2C_MST_CNTL_STATUS_OKAY		(0 << 29)
+#define I2C_MST_CNTL_STATUS_NO_ACK		(1 << 29)
+#define I2C_MST_CNTL_STATUS_TIMEOUT		(2 << 29)
+#define I2C_MST_CNTL_STATUS_BUS_BUSY		(3 << 29)
+#define I2C_MST_CNTL_CYCLE_TRIGGER		(1 << 31)
+
+#define I2C_MST_ADDR				0x04
+#define I2C_MST_ADDR_DAB			0
+
+#define I2C_MST_I2C0_TIMING				0x08
+#define I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ		(0x10e << 0)
+#define I2C_MST_I2C0_TIMING_SCL_PERIOD_200KHZ		(0x087 << 0)
+#define I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT		16
+#define I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX		255
+#define I2C_MST_I2C0_TIMING_TIMEOUT_CHECK		(1 << 24)
+
+#define I2C_MST_DATA					0x0c
+
+#define I2C_MST_HYBRID_PADCTL				0x20
+#define I2C_MST_HYBRID_PADCTL_MODE_I2C			(1 << 0)
+#define I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV		(1 << 14)
+#define I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV		(1 << 15)
+
+/* PCI driver data */
+struct gpu_i2c_dev {
+	struct pci_dev *pci_dev;
+	void __iomem *regs;
+	struct i2c_adapter adapter;
+	struct i2c_client *client;
+	struct mutex mutex;
+	bool do_start;
+};
+
+static void enable_i2c_bus(struct gpu_i2c_dev *gdev)
+{
+	struct device *dev = &gdev->pci_dev->dev;

drivers/i2c/busses/i2c-gpu.c line 84

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+	u32 val;
+
+	/* enable I2C */
+	val = readl(gdev->regs + I2C_MST_HYBRID_PADCTL);
+	val |= I2C_MST_HYBRID_PADCTL_MODE_I2C |
+		I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV |
+		I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV;
+
+	dev_dbg(dev, "%s: %p (I2C_MST_HYBRID_PADCTL) <- %08x", __func__,
+		(gdev->regs + I2C_MST_HYBRID_PADCTL), val);

Do we really need these debug messages? None of the configuration above is in any way parameterized, so the values that are written are pretty deterministic (unless maybe the initial value of the register differs).

If you really want to keep the debug messages, I think you should remove at least the register address (you already print the name, which is much more useful). Alternatively, have you considered using ftrace to allow these traces to be dynamically enabled?

Quoted 26 lines
drivers/i2c/busses/i2c-gpu.c new file +26 -0
+
+	writel(val, gdev->regs + I2C_MST_HYBRID_PADCTL);
+
+	/* enable 100KHZ mode */
+	val = 0;
+	val |= I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ;
+	val |= (I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX
+	    << I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT);
+	val |= I2C_MST_I2C0_TIMING_TIMEOUT_CHECK;
+
+	dev_dbg(dev, "%s: %p (I2C_MST_I2C0_TIMING) <- %08x", __func__,
+		gdev->regs + I2C_MST_I2C0_TIMING, val);
+	writel(val, gdev->regs + I2C_MST_I2C0_TIMING);
+}
+
+static u32 i2c_check_status(struct gpu_i2c_dev *gdev)
+{
+	struct device *dev = &gdev->pci_dev->dev;
+	unsigned long target = jiffies + msecs_to_jiffies(1000);
+	u32 status = STATUS_UNSUCCESSFUL;
+	u32 val;
+
+	while (time_is_after_jiffies(target)) {
+		val = readl(gdev->regs + I2C_MST_CNTL);
+		if ((val & I2C_MST_CNTL_CYCLE_TRIGGER) !=
+				I2C_MST_CNTL_CYCLE_TRIGGER)

drivers/i2c/busses/i2c-gpu.c line 120

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+			break;
+		if ((val & I2C_MST_CNTL_STATUS) !=
+				I2C_MST_CNTL_STATUS_BUS_BUSY)
+			break;
+		usleep_range(1000, 2000);
+	}
+
+	if (time_is_before_jiffies(target)) {
+		dev_err(dev, "%si2c timeout", __func__);
+		return status;

The error message looks suspicious here. There should be some sort of separator between the function name and the rest of the message. Also, do we really need to print the function name? That's not very useful to users (it might actually be confusing because the function name is pretty generic, so could be mistaken for a core function) and this is the only place where you check for timeouts anyway, so no need to resolve any ambiguities by adding the function name.

Quoted 29 lines
drivers/i2c/busses/i2c-gpu.c new file +29 -0
+	}
+
+	val = readl(gdev->regs + I2C_MST_CNTL);
+	switch (val & I2C_MST_CNTL_STATUS) {
+	case I2C_MST_CNTL_STATUS_OKAY:
+		status = STATUS_SUCCESS;
+		break;
+	case I2C_MST_CNTL_STATUS_NO_ACK:
+		status = STATUS_IO_DEVICE_ERROR;
+		break;
+	case I2C_MST_CNTL_STATUS_TIMEOUT:
+		status = STATUS_IO_TIMEOUT;
+		break;
+	case I2C_MST_CNTL_STATUS_BUS_BUSY:
+		status = STATUS_IO_PREEMPTED;
+		break;
+	default:
+		break;
+	}
+	return status;
+}
+
+static u32 i2c_read(struct gpu_i2c_dev *gdev, u8 *data, u16 len)
+{
+	struct device *dev = &gdev->pci_dev->dev;
+	u32 status;
+	u32 val = 0;
+
+	val |= I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |

drivers/i2c/busses/i2c-gpu.c line 159

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+		I2C_MST_CNTL_CMD_READ | (len << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
+		I2C_MST_CNTL_CYCLE_TRIGGER | I2C_MST_CNTL_GEN_NACK;
+	val &= ~I2C_MST_CNTL_GEN_RAB;
+	writel(val, gdev->regs + I2C_MST_CNTL);
+
+	status = i2c_check_status(gdev);
+	if (status == STATUS_UNSUCCESSFUL) {
+		dev_err(dev, "%s failed\n", __func__);
+		return status;
+	}

There's no need for this error message, since the caller already provides one.

Quoted 71 lines
drivers/i2c/busses/i2c-gpu.c new file +71 -0
+
+	val = readl(gdev->regs + I2C_MST_DATA);
+	switch (len) {
+	case 1:
+		data[0] = (val >> 0) & 0xff;
+		break;
+	case 2:
+		data[0] = (val >> 8) & 0xff;
+		data[1] = (val >> 0) & 0xff;
+		break;
+	case 3:
+		data[0] = (val >> 16) & 0xff;
+		data[1] = (val >> 8) & 0xff;
+		data[2] = (val >> 0) & 0xff;
+		break;
+	case 4:
+		data[0] = (val >> 24) & 0xff;
+		data[1] = (val >> 16) & 0xff;
+		data[2] = (val >> 8) & 0xff;
+		data[3] = (val >> 0) & 0xff;
+		break;
+	default:
+		break;
+	}
+	return status;
+}
+
+static u32 i2c_manual_start(struct gpu_i2c_dev *gdev, u16 addr)
+{
+	u32 val = 0;
+
+	val = addr << I2C_MST_ADDR_DAB;
+	writel(val, gdev->regs + I2C_MST_ADDR);
+
+	val = 0;
+	val |= I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE |
+		I2C_MST_CNTL_GEN_NACK;
+	val &= ~(I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_GEN_RAB);
+	writel(val, gdev->regs + I2C_MST_CNTL);
+
+	return i2c_check_status(gdev);
+}
+
+static u32 i2c_manual_stop(struct gpu_i2c_dev *gdev)
+{
+	u32 val = 0;
+
+	val |= I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_CMD_NONE |
+		I2C_MST_CNTL_GEN_NACK;
+	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_RAB);
+	writel(val, gdev->regs + I2C_MST_CNTL);
+
+	return i2c_check_status(gdev);
+}
+
+static u32 i2c_manual_write(struct gpu_i2c_dev *gdev, u8 data)
+{
+	u32 val = 0;
+
+	writel(data, gdev->regs + I2C_MST_DATA);
+
+	val |= I2C_MST_CNTL_CMD_WRITE | (1 << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
+		I2C_MST_CNTL_GEN_NACK;
+	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP
+		| I2C_MST_CNTL_GEN_RAB);
+	writel(val, gdev->regs + I2C_MST_CNTL);
+
+	return i2c_check_status(gdev);
+}
+
+/* gdev i2c adapter */

drivers/i2c/busses/i2c-gpu.c line 240

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
+	struct i2c_msg *msgs, int num)
+{
+	struct gpu_i2c_dev *gdev = i2c_get_adapdata(adap);
+	struct device *dev = &gdev->pci_dev->dev;
+	int retry1b = 10;
+	u32 status;
+	int i, j;
+
+	dev_dbg(dev, "%s: adap %p msgs %p num %d\n", __func__, adap, msgs, num);

There's already code in drivers/i2c/i2c-core-base.c that will print mostly the same information when DEBUG is enabled.

drivers/i2c/busses/i2c-gpu.c line 250

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+	mutex_lock(&gdev->mutex);
+
+	for (i = 0; i < num; i++) {
+		if (msgs[i].flags & I2C_M_RD) {
+retry2:
+			status = i2c_read(gdev, msgs[i].buf, msgs[i].len);
+			if (status != STATUS_SUCCESS) {
+				dev_err(dev,
+				"%s:%d i2c_read failed %08lx", __func__,
+				__LINE__, (unsigned long)status);

Again, I'm not sure filename and line number will provide useful information. I'd simply refer to the operation that failed, along the lines of:

	err = i2c_read(...);
	if (err < 0) {
		dev_err(dev, "I2C read failed: %d\n", err);
		...
	}

drivers/i2c/busses/i2c-gpu.c line 257

Quoted 7 lines
drivers/i2c/busses/i2c-gpu.c new file +7 -0
+
+				if (--retry1b > 0) {
+					usleep_range(10000, 11000);
+					goto retry2;
+				}
+				break;
+			}

Also, perhaps you want to only output the error right before the break so that you don't potentially get 10 identical error messages?

Quoted 45 lines
drivers/i2c/busses/i2c-gpu.c new file +45 -0
+			gdev->do_start = true;
+		} else if (msgs[i].flags & I2C_M_STOP) {
+			status = i2c_manual_stop(gdev);
+			if (status != STATUS_SUCCESS) {
+				dev_err(dev,
+				"%s:%d i2c_manual_stop failed %08lx", __func__,
+				__LINE__, (unsigned long)status);
+				goto exit;
+			}
+			gdev->do_start = true;
+		} else {
+			dev_dbg(dev, "!I2C_M_RD start %d len %d\n",
+				gdev->do_start, msgs[i].len);
+			if (gdev->do_start) {
+				status = i2c_manual_start(gdev, msgs[i].addr);
+				if (status != STATUS_SUCCESS) {
+					dev_err(dev,
+					"%s:%d i2c_manual_start failed %08lx",
+						__func__, __LINE__,
+						(unsigned long)status);
+					goto exit;
+				}
+				status = i2c_manual_write(gdev,
+						msgs[i].addr << 1);
+				if (status != STATUS_SUCCESS) {
+					dev_err(dev,
+					"%s:%d i2c_manual_write failed %08lx",
+						__func__, __LINE__,
+						(unsigned long)status);
+					goto exit_stop;
+				}
+				gdev->do_start = false;
+			}
+			for (j = 0; j < msgs[i].len; j++) {
+				status = i2c_manual_write(gdev,
+						*(msgs[i].buf + j));
+				if (status != STATUS_SUCCESS) {
+					dev_err(dev,
+					"%s:%d i2c_manual_write failed %08lx",
+						__func__, __LINE__,
+						(unsigned long)status);
+					goto exit_stop;
+				}
+			}
+		}

drivers/i2c/busses/i2c-gpu.c line 312

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+	}
+	goto exit;
+exit_stop:
+	status = i2c_manual_stop(gdev);
+	if (status != STATUS_SUCCESS)
+		dev_err(dev, "i2c_manual_stop failed %x", status);
+exit:
+	mutex_unlock(&gdev->mutex);
+	return i;
+}

You might want to consider using more descriptive names for the labels to make the code more readable. Something like:

		...
		goto stop;
	}

	goto unlock;

	stop:
		...
	unlock:
		...

Is more readable I think. Also, I think the high level of indentation makes this function hard to read. You may want to split it up into smaller chunks.

Quoted 13 lines
drivers/i2c/busses/i2c-gpu.c new file +13 -0
+
+static u32 gpu_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm gpu_i2c_algorithm = {
+	.master_xfer	= gpu_i2c_master_xfer,
+	.functionality	= gpu_i2c_functionality,
+};
+
+static int gpu_i2c_dev_init(struct gpu_i2c_dev *gdev)
+{

drivers/i2c/busses/i2c-gpu.c line 335

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+	gdev->do_start = true;
+
+	/* initialize mutex */
+	mutex_init(&gdev->mutex);
+
+	/* initialize i2c */
+	enable_i2c_bus(gdev);
+
+	return 0;
+}

You might want to consider moving this directly into the ->probe() implementation. You never reuse this elsewhere and it's not big enough to warrant the separate function. Inlining it into ->probe() will make the code easier to follow, in my opinion.

drivers/i2c/busses/i2c-gpu.c line 343

Quoted 8 lines
drivers/i2c/busses/i2c-gpu.c new file +8 -0
+
+struct i2c_board_info gpu_i2c_ucsi_board_info = {
+	I2C_BOARD_INFO("i2c-gpu-ucsi", CCGX_I2C_DEV_ADDRESS),
+};
+
+#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
+/* pci driver */
+static const struct pci_device_id gpu_i2c_ids[] = {

The comment there looks out of place.

Quoted 5 lines
drivers/i2c/busses/i2c-gpu.c new file +5 -0
+	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
+		PCI_CLASS_SERIAL_UNKNOWN << 8, 0xffffff00},
+	{ },
+};
+MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);

drivers/i2c/busses/i2c-gpu.c line 358

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+
+static int gpu_i2c_probe(struct pci_dev *dev, const struct pci_device_id *id)
+{
+	struct gpu_i2c_dev *gdev;
+	int status;
+
+	dev_info(&dev->dev,
+		"dev %p id %08x %08x sub %08x %08x class %08x %08x\n",
+		dev, id->vendor, id->device, id->subvendor, id->subdevice,
+		id->class, id->class_mask);

If at all, this should be dev_dbg(). Generally I don't think there's any reason to mention this at all. ->probe() should assume that it will succeed, which is the expected case, and that should not produce any messages. Instead it's better to let the user know if something failed using an error message.

Quoted 40 lines
drivers/i2c/busses/i2c-gpu.c new file +40 -0
+
+	gdev = devm_kzalloc(&dev->dev, sizeof(struct gpu_i2c_dev), GFP_KERNEL);
+	if (!gdev)
+		return -ENOMEM;
+
+	gdev->pci_dev = dev;
+	pci_set_drvdata(dev, gdev);
+
+	status = pci_enable_device(dev);
+	if (status < 0) {
+		dev_err(&dev->dev, "pci_enable_device failed - %d\n", status);
+		return status;
+	}
+
+	pci_set_master(dev);
+
+	gdev->regs = pci_iomap(dev, 0, 0);
+	if (!gdev->regs) {
+		dev_err(&dev->dev, "pci_iomap failed\n");
+		status = -ENOMEM;
+		goto iomap_err;
+	}
+
+	status = pci_enable_msi(dev);
+	if (status < 0) {
+		dev_err(&dev->dev, "pci_enable_msi failed - %d\n", status);
+		goto enable_msi_err;
+	}
+
+	status = gpu_i2c_dev_init(gdev);
+	if (status < 0) {
+		dev_err(&dev->dev, "gpu_i2c_dev_init failed - %d\n", status);
+		goto i2c_init_err;
+	}
+
+	i2c_set_adapdata(&gdev->adapter, gdev);
+	gdev->adapter.owner = THIS_MODULE;
+	strlcpy(gdev->adapter.name, "NVIDIA GPU I2C adapter",
+		sizeof(gdev->adapter.name));
+	gdev->adapter.algo = &gpu_i2c_algorithm;

drivers/i2c/busses/i2c-gpu.c line 408

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+	gdev->adapter.dev.parent = &dev->dev;
+	status = i2c_add_adapter(&gdev->adapter);
+	if (status < 0) {
+		dev_err(&dev->dev, "i2c_add_adapter failed - %d\n", status);
+		goto add_adapter_err;
+	}
+
+	gpu_i2c_ucsi_board_info.irq = dev->irq;
+	gdev->client = i2c_new_device(&gdev->adapter,
+			&gpu_i2c_ucsi_board_info);

I think it's technically possible that the above would race between multiple instances of this I2C controller. The safer way would be to make gpu_i2c_ucsi_board_info variable local to this function.

drivers/i2c/busses/i2c-gpu.c line 416

Quoted 8 lines
drivers/i2c/busses/i2c-gpu.c new file +8 -0
+
+	if (!gdev->client) {
+		dev_err(&dev->dev, "i2c_new_device failed - %d\n", status);
+		status = -ENODEV;
+		goto add_adapter_err;
+	}
+
+	dev_set_drvdata(&dev->dev, gdev);

I don't think you need this here since you've already called pci_set_drvdata() above.

Quoted 5 lines
drivers/i2c/busses/i2c-gpu.c new file +5 -0
+	pm_runtime_put_noidle(&dev->dev);
+	pm_runtime_allow(&dev->dev);
+
+	return 0;
+

drivers/i2c/busses/i2c-gpu.c line 431

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+add_adapter_err:
+	i2c_del_adapter(&gdev->adapter);
+i2c_init_err:
+	pci_disable_msi(dev);
+enable_msi_err:
+	pci_iounmap(dev, gdev->regs);
+iomap_err:
+	pci_disable_device(dev);
+	return status;
+}

It's generally preferred to have names that describe the actions of the label, instead of describing where they failed. I think that makes code easier to read, and it also has the benefit of not being confusing when you jump to a label, say add_adapter_err, from a location that doesn't have anything to do with adding an adapter.

Something like this would be better in my opinion:

	del_adapter:
		...
	disable_msi:
		...
	unmap:
		...
	disable:
		...
Quoted 6 lines
drivers/i2c/busses/i2c-gpu.c new file +6 -0
+
+static void gpu_i2c_remove(struct pci_dev *dev)
+{
+	struct gpu_i2c_dev *gdev = pci_get_drvdata(dev);
+
+	i2c_del_adapter(&gdev->adapter);

drivers/i2c/busses/i2c-gpu.c line 447

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+	pci_disable_msi(dev);
+	pci_iounmap(dev, gdev->regs);
+}
+
+static int gpu_i2c_suspend(struct device *dev)
+{
+	dev_dbg(dev, "%s\n", __func__);
+
+	return 0;
+}

Do we need this if it doesn't do anything?

Quoted 20 lines
drivers/i2c/busses/i2c-gpu.c new file +20 -0
+
+static int gpu_i2c_resume(struct device *dev)
+{
+	struct gpu_i2c_dev *gdev = dev_get_drvdata(dev);
+
+	dev_dbg(dev, "%s\n", __func__);
+
+	enable_i2c_bus(gdev);
+
+	return 0;
+}
+
+static int gpu_i2c_idle(struct device *dev)
+{
+	struct gpu_i2c_dev *gdev = dev_get_drvdata(dev);
+
+	if (!mutex_trylock(&gdev->mutex)) {
+		dev_info(dev, "%s: -EBUSY\n", __func__);
+		return -EBUSY;
+	}

drivers/i2c/busses/i2c-gpu.c line 477

Quoted 10 lines
drivers/i2c/busses/i2c-gpu.c new file +10 -0
+	mutex_unlock(&gdev->mutex);
+
+	return 0;
+}
+
+UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, gpu_i2c_suspend, gpu_i2c_resume,
+	gpu_i2c_idle);
+
+static struct pci_driver gpu_i2c_driver = {
+	.name		= "gpu_i2c_driver",

This name looks unusual for a driver. Perhaps "i2c-nvidia-gpu" to match the module name?

Quoted 13 lines
drivers/i2c/busses/i2c-gpu.c new file +13 -0
+	.id_table	= gpu_i2c_ids,
+	.probe		= gpu_i2c_probe,
+	.remove		= gpu_i2c_remove,
+	.driver		= {
+		.pm	= &gpu_i2c_driver_pm,
+	},
+};
+
+module_pci_driver(gpu_i2c_driver);
+
+MODULE_AUTHOR("Ajay Gupta <[email protected]>");
+MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
+MODULE_LICENSE("GPL v2");