[v11,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[v11,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-11 00:22, Ajay Gupta wrote:
> Latest NVIDIA GPU card has USB Type-C interface. There is a
> Type-C controller which can be accessed over I2C.
> 
> This driver adds 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 <ajayg@nvidia.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Changes from v1 -> v2
> 	None
> Changes from v2 -> v3
> 	Fixed review comments from Andy and Thierry
> 	Rename i2c-gpu.c -> i2c-nvidia-gpu.c
> Changes from v3 -> v4
> 	Fixed review comments from Andy
> Changes from v4 -> v5
> 	Fixed review comments from Andy
> Changes from v5 -> v6
> 	None 
> Changes from v6 -> v7 -> v8
> 	Fixed review comments from Peter 
> 	- Add implicit STOP for last write message
> 	- Add i2c_adapter_quirks with max_read_len and
> 	  I2C_AQ_COMB flags
> Changes from v8 -> v9
> 	Fixed review comments from Peter
> 	- Drop do_start flag
> 	- Use i2c_8bit_addr_from_msg()
> Changes from v9 -> v10
> 	Fixed review comments from Peter
> 	- Dropped I2C_FUNC_SMBUS_EMUL
> 	- Dropped local mutex
> Changes from v10 -> v11
> 	Fixed review comments from Peter
> 	- Moved stop in master_xfer at end of message
> 	- Change i2c_read without STOP
> 	- Dropped I2C_AC_COMB* flags
> 	
>  Documentation/i2c/busses/i2c-nvidia-gpu |  18 ++
>  MAINTAINERS                             |   7 +
>  drivers/i2c/busses/Kconfig              |   9 +
>  drivers/i2c/busses/Makefile             |   1 +
>  drivers/i2c/busses/i2c-nvidia-gpu.c     | 372 ++++++++++++++++++++++++++++++++
>  5 files changed, 407 insertions(+)
>  create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu
>  create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c
> 
> diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu b/Documentation/i2c/busses/i2c-nvidia-gpu
> new file mode 100644
> index 0000000..31884d2
> --- /dev/null
> +++ b/Documentation/i2c/busses/i2c-nvidia-gpu
> @@ -0,0 +1,18 @@
> +Kernel driver i2c-nvidia-gpu
> +
> +Datasheet: not publicly available.
> +
> +Authors:
> +	Ajay Gupta <ajayg@nvidia.com>
> +
> +Description
> +-----------
> +
> +i2c-nvidia-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.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d870cb5..b71b0b4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6798,6 +6798,13 @@ L:	linux-acpi@vger.kernel.org
>  S:	Maintained
>  F:	drivers/i2c/i2c-core-acpi.c
>  
> +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> +M:	Ajay Gupta <ajayg@nvidia.com>
> +L:	linux-i2c@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/i2c/busses/i2c-nvidia-gpu
> +F:	drivers/i2c/busses/i2c-nvidia-gpu.c
> +
>  I2C MUXES
>  M:	Peter Rosin <peda@axentia.se>
>  L:	linux-i2c@vger.kernel.org
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 451d4ae..eed827b 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -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_NVIDIA_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-nvidia-gpu.
> +
>  config I2C_SIS5595
>  	tristate "SiS 5595"
>  	depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 18b26af..d499813 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -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_NVIDIA_GPU)	+= i2c-nvidia-gpu.o
>  
>  ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
> new file mode 100644
> index 0000000..e88424f
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -0,0 +1,372 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nvidia GPU I2C controller Driver
> + *
> + * Copyright (C) 2018 NVIDIA Corporation. All rights reserved.
> + * Author: Ajay Gupta <ajayg@nvidia.com>
> + */
> +#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>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <asm/unaligned.h>
> +
> +/* I2C definitions */
> +#define I2C_MST_CNTL				0x00
> +#define I2C_MST_CNTL_GEN_START			BIT(0)
> +#define I2C_MST_CNTL_GEN_STOP			BIT(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_GEN_RAB			BIT(4)
> +#define I2C_MST_CNTL_BURST_SIZE_SHIFT		6
> +#define I2C_MST_CNTL_GEN_NACK			BIT(28)
> +#define I2C_MST_CNTL_STATUS			GENMASK(30, 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		BIT(31)
> +
> +#define I2C_MST_ADDR				0x04
> +
> +#define I2C_MST_I2C0_TIMING				0x08
> +#define I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ		0x10e
> +#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		BIT(24)
> +
> +#define I2C_MST_DATA					0x0c
> +
> +#define I2C_MST_HYBRID_PADCTL				0x20
> +#define I2C_MST_HYBRID_PADCTL_MODE_I2C			BIT(0)
> +#define I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV		BIT(14)
> +#define I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV		BIT(15)
> +
> +struct gpu_i2c_dev {
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct i2c_adapter adapter;
> +	struct i2c_client *client;
> +};
> +
> +static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd)
> +{
> +	u32 val;
> +
> +	/* enable I2C */
> +	val = readl(i2cd->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;
> +	writel(val, i2cd->regs + I2C_MST_HYBRID_PADCTL);
> +
> +	/* enable 100KHZ mode */
> +	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;
> +	writel(val, i2cd->regs + I2C_MST_I2C0_TIMING);
> +}
> +
> +static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd)
> +{
> +	unsigned long target = jiffies + msecs_to_jiffies(1000);
> +	u32 val;
> +
> +	do {
> +		val = readl(i2cd->regs + I2C_MST_CNTL);
> +		if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
> +			break;
> +		if ((val & I2C_MST_CNTL_STATUS) !=
> +				I2C_MST_CNTL_STATUS_BUS_BUSY)
> +			break;
> +		usleep_range(1000, 2000);
> +	} while (time_is_after_jiffies(target));
> +
> +	if (time_is_before_jiffies(target)) {
> +		dev_err(i2cd->dev, "i2c timeout error %x\n", val);
> +		return -EIO;
> +	}
> +
> +	val = readl(i2cd->regs + I2C_MST_CNTL);
> +	switch (val & I2C_MST_CNTL_STATUS) {
> +	case I2C_MST_CNTL_STATUS_OKAY:
> +		return 0;
> +	case I2C_MST_CNTL_STATUS_NO_ACK:
> +		return -EIO;
> +	case I2C_MST_CNTL_STATUS_TIMEOUT:
> +		return -ETIME;
> +	case I2C_MST_CNTL_STATUS_BUS_BUSY:
> +		return -EBUSY;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int gpu_i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16 len)
> +{
> +	int status;
> +	u32 val;
> +
> +	val = I2C_MST_CNTL_GEN_START | 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 | I2C_MST_CNTL_GEN_STOP);

Hmm, I didn't notice this before, but this masks out bits that are
already zero and can be dropped AFAICT.

> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	status = gpu_i2c_check_status(i2cd);
> +	if (status < 0)
> +		return status;
> +
> +	val = readl(i2cd->regs + I2C_MST_DATA);
> +	switch (len) {
> +	case 1:
> +		data[0] = val;
> +		break;
> +	case 2:
> +		put_unaligned_be16(val, data);
> +		break;
> +	case 3:
> +		put_unaligned_be16(val >> 8, data);
> +		data[2] = val;
> +		break;
> +	case 4:
> +		put_unaligned_be32(val, data);
> +		break;
> +	default:
> +		break;
> +	}
> +	return status;
> +}
> +
> +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd)
> +{
> +	u32 val;
> +
> +	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);

Dito. I also suspect that ..._GEN_NACK is insignificant here? I have no
way of knowing that, but if so, and given that ..._CMD_NONE is zero, this
can become much more readable with a simple

	writel(I2C_MST_CNTL_GEN_START, i2cd->regs + I2C_MST_CNTL);

> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd)
> +{
> +	u32 val;
> +
> +	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);

Dito, but

	writel(I2C_MST_CNTL_GEN_STOP, i2cd->regs + I2C_MST_CNTL);

> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_write(struct gpu_i2c_dev *i2cd, u8 data)
> +{
> +	u32 val;
> +
> +	writel(data, i2cd->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);

Again, zeroing out empty bits.

> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> +			       struct i2c_msg *msgs, int num)
> +{
> +	struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> +	int status, status2;
> +	int i, j;
> +
> +	for (i = 0; i < num; i++) {
> +		if (msgs[i].flags & I2C_M_RD) {
> +			/* program client address before starting read */
> +			writel(msgs[i].addr, i2cd->regs + I2C_MST_ADDR);
> +			/* gpu_i2c_read has implicit start */
> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> +			if (status < 0)
> +				return status;

				goto stop;

Hmm, that goto stop is however not perfect. Ideally, you shouldn't issue
stop if i == 0 and gpu_i2c_read failed due to failure to output start. Not
sure if you can know that? Maybe you can make the start of reads be explicit,
just like you could with the stop? In that case, maybe you can move the
below gpu_i2c_start call (with fixed error handling) to before the above
if (... & I2C_M_RD)?

> +		} else {
> +			u8 addr = i2c_8bit_addr_from_msg(msgs + i);
> +
> +			status = gpu_i2c_start(i2cd);
> +			if (status < 0)
> +				return status;

You fail to issue a terminating stop if a restart fails.

			if (status < 0) {
				if (i == 0)
					return status;
				goto stop;
			}

> +
> +			status = gpu_i2c_write(i2cd, addr);
> +			if (status < 0)
> +				goto stop;
> +
> +			for (j = 0; j < msgs[i].len; j++) {
> +				status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
> +				if (status < 0)
> +					goto stop;
> +			}
> +		}
> +	}
> +	status = gpu_i2c_stop(i2cd);
> +	if (status < 0)
> +		return status;
> +	return i;
> +stop:
> +	status2 = gpu_i2c_stop(i2cd);
> +	if (status2 < 0)
> +		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
> +	return status;
> +}
> +
> +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> +	.max_read_len = 4,
> +};
> +
> +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,
> +};
> +
> +/*
> + * This driver is for Nvidia GPU cards with USB Type-C interface.
> + * We want to identify the cards using vendor ID and class code only
> + * to avoid dependency of adding product id for any new card which
> + * requires this driver.
> + * Currently there is no class code defined for UCSI device over PCI
> + * so using UNKNOWN class for now and it will be updated when UCSI
> + * over PCI gets a class code.
> + * There is no other NVIDIA cards with UNKNOWN class code. Even if the
> + * driver gets loaded for an undesired card then eventually i2c_read()
> + * (initiated from UCSI i2c_client) will timeout or UCSI commands will
> + * timeout.
> + */
> +#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
> +static const struct pci_device_id gpu_i2c_ids[] = {
> +	{ 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);
> +
> +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq)
> +{
> +	static struct i2c_board_info gpu_ccgx_ucsi = {
> +		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
> +	};

Shouldn't this struct be dynamically allocated and attached to
struct gpu_i2c_dev so that you (maybe) could have more than one instance?

Cheers,
Peter

> +
> +	gpu_ccgx_ucsi.irq = irq;
> +	i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi);
> +	if (!i2cd->client)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int gpu_i2c_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct gpu_i2c_dev *i2cd;
> +	int status;
> +
> +	i2cd = devm_kzalloc(&pdev->dev, sizeof(struct gpu_i2c_dev), GFP_KERNEL);
> +	if (!i2cd)
> +		return -ENOMEM;
> +
> +	i2cd->dev = &pdev->dev;
> +	dev_set_drvdata(&pdev->dev, i2cd);
> +
> +	status = pcim_enable_device(pdev);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "pcim_enable_device failed %d\n", status);
> +		return status;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	i2cd->regs = pcim_iomap(pdev, 0, 0);
> +	if (!i2cd->regs) {
> +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n", status);
> +		return status;
> +	}
> +
> +	gpu_enable_i2c_bus(i2cd);
> +
> +	i2c_set_adapdata(&i2cd->adapter, i2cd);
> +	i2cd->adapter.owner = THIS_MODULE;
> +	strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
> +		sizeof(i2cd->adapter.name));
> +	i2cd->adapter.algo = &gpu_i2c_algorithm;
> +	i2cd->adapter.quirks = &gpu_i2c_quirks;
> +	i2cd->adapter.dev.parent = &pdev->dev;
> +	status = i2c_add_adapter(&i2cd->adapter);
> +	if (status < 0)
> +		goto free_irq_vectors;
> +
> +	status = gpu_populate_client(i2cd, pdev->irq);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "gpu_populate_client failed %d\n", status);
> +		goto del_adapter;
> +	}
> +
> +	return 0;
> +
> +del_adapter:
> +	i2c_del_adapter(&i2cd->adapter);
> +free_irq_vectors:
> +	pci_free_irq_vectors(pdev);
> +	return status;
> +}
> +
> +static void gpu_i2c_remove(struct pci_dev *pdev)
> +{
> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
> +
> +	i2c_del_adapter(&i2cd->adapter);
> +	pci_free_irq_vectors(pdev);
> +}
> +
> +static int gpu_i2c_resume(struct device *dev)
> +{
> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> +
> +	gpu_enable_i2c_bus(i2cd);
> +	return 0;
> +}
> +
> +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume, NULL);
> +
> +static struct pci_driver gpu_i2c_driver = {
> +	.name		= "nvidia-gpu",
> +	.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 <ajayg@nvidia.com>");
> +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> +MODULE_LICENSE("GPL v2");
>

[v11,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-11 18:53, Ajay Gupta wrote:
> Hi Peter,
> 
>> -----Original Message-----
>> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org>
>> On Behalf Of Peter Rosin
>> Sent: Tuesday, September 11, 2018 1:55 AM
>> To: Ajay Gupta <ajayg@nvidia.com>; wsa@the-dreams.de;
>> heikki.krogerus@linux.intel.com
>> Cc: linux-usb@vger.kernel.org; linux-i2c@vger.kernel.org
>> Subject: Re: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
>>
>> [I seem to have lost my local copy of the mail I'm responding to, so I copied
>> bits of it from an archive and broke threading in the process, sorry about that]
> That’s why was wondering how you got "goto stop" in "if (msgs[i].flags & I2C_M_RD) {"
> Block at [1]. There is actually no stop when gpu_i2c_read() fails in master_xfer()
> 
> [1] https://marc.info/?l=linux-usb&m=153662481422174&w=2 

No, the only message I lost was the one I quoted from. The "goto stop"
line was me saying that "goto stop" would fix a bug caused by the
unconditional "return status". I then continued saying that "goto stop"
was not perfect etc.

Cheers,
Peter

[v11,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Ajay Gupta]

Hi Peter,

> -----Original Message-----
> From: linux-i2c-owner@vger.kernel.org <linux-i2c-owner@vger.kernel.org>
> On Behalf Of Peter Rosin
> Sent: Tuesday, September 11, 2018 1:55 AM
> To: Ajay Gupta <ajayg@nvidia.com>; wsa@the-dreams.de;
> heikki.krogerus@linux.intel.com
> Cc: linux-usb@vger.kernel.org; linux-i2c@vger.kernel.org
> Subject: Re: [PATCH v11 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU
> 
> [I seem to have lost my local copy of the mail I'm responding to, so I copied
> bits of it from an archive and broke threading in the process, sorry about that]
That’s why was wondering how you got "goto stop" in "if (msgs[i].flags & I2C_M_RD) {"
Block at [1]. There is actually no stop when gpu_i2c_read() fails in master_xfer()

[1] https://marc.info/?l=linux-usb&m=153662481422174&w=2 
> 
> On 2018-09-11 00:22, Ajay Gupta wrote:
> >> Hmm, that goto stop is however not perfect. Ideally, you shouldn't
> >> issue stop if i == 0 and gpu_i2c_read failed
See above. There is no "stop"


Thanks
Ajay

--nvpublic
> > How can there be a read without rab write first?
> > AFAIU, gpu_i2c_read() can get called only with i=1 onward.
> 
> Well, you said that there could be other I2C devices on this I2C bus. I thought
> that meant that there could be other I2C client drivers using this I2C adapter.
> If so, then those drivers can issue I2C transfers where the first message is a
> read.
> 
> >> > +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq) {
> >> > +	static struct i2c_board_info gpu_ccgx_ucsi = {
> >> > +		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
> >> > +	};
> >>
> >> Shouldn't this struct be dynamically allocated and attached to struct
> >> gpu_i2c_dev so that you (maybe) could have more than one instance?
> > Currently the structure is local variable. Will that not work in multi
> > instance cases?
> 
> Arrggh. NO!
> 
> A static variable in function scope behaves very much the same as a static
> variable in file scope. The only difference is that the visibility is different. All
> calls to the function gets the same gpu_ccgx_ucsi instance which means that
> instances will clobber each other.
> 
> You need to add a struct i2c_board_info (or a pointer to one) to struct
> gpu_i2c_dev so that it can be dynamically allocated.
> 
> Cheers,
> Peter

[v11,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

[I seem to have lost my local copy of the mail I'm responding to, so I
copied bits of it from an archive and broke threading in the process,
sorry about that]

On 2018-09-11 00:22, Ajay Gupta wrote:
>> Hmm, that goto stop is however not perfect. Ideally, 
>> you shouldn't issue stop if i == 0 and gpu_i2c_read failed 
> How can there be a read without rab write first?
> AFAIU, gpu_i2c_read() can get called only with i=1 onward.

Well, you said that there could be other I2C devices on this
I2C bus. I thought that meant that there could be other I2C
client drivers using this I2C adapter. If so, then those
drivers can issue I2C transfers where the first message is a
read.

>> > +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq) {
>> > +	static struct i2c_board_info gpu_ccgx_ucsi = {
>> > +		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
>> > +	};
>> 
>> Shouldn't this struct be dynamically allocated and attached to struct
>> gpu_i2c_dev so that you (maybe) could have more than one instance?
> Currently the structure is local variable. Will that not work in multi
> instance cases?

Arrggh. NO!

A static variable in function scope behaves very much the same as a
static variable in file scope. The only difference is that the
visibility is different. All calls to the function gets the same
gpu_ccgx_ucsi instance which means that instances will clobber
each other.

You need to add a struct i2c_board_info (or a pointer to one) to
struct gpu_i2c_dev so that it can be dynamically allocated.

Cheers,
Peter

[v11,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Ajay Gupta]

Hi Peter,

> > Latest NVIDIA GPU card has USB Type-C interface. There is a Type-C
> > controller which can be accessed over I2C.
> >
> > This driver adds 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 <ajayg@nvidia.com>
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> > Changes from v1 -> v2
> > 	None
> > Changes from v2 -> v3
> > 	Fixed review comments from Andy and Thierry
> > 	Rename i2c-gpu.c -> i2c-nvidia-gpu.c
> > Changes from v3 -> v4
> > 	Fixed review comments from Andy
> > Changes from v4 -> v5
> > 	Fixed review comments from Andy
> > Changes from v5 -> v6
> > 	None
> > Changes from v6 -> v7 -> v8
> > 	Fixed review comments from Peter
> > 	- Add implicit STOP for last write message
> > 	- Add i2c_adapter_quirks with max_read_len and
> > 	  I2C_AQ_COMB flags
> > Changes from v8 -> v9
> > 	Fixed review comments from Peter
> > 	- Drop do_start flag
> > 	- Use i2c_8bit_addr_from_msg()
> > Changes from v9 -> v10
> > 	Fixed review comments from Peter
> > 	- Dropped I2C_FUNC_SMBUS_EMUL
> > 	- Dropped local mutex
> > Changes from v10 -> v11
> > 	Fixed review comments from Peter
> > 	- Moved stop in master_xfer at end of message
> > 	- Change i2c_read without STOP
> > 	- Dropped I2C_AC_COMB* flags
> >
> >  Documentation/i2c/busses/i2c-nvidia-gpu |  18 ++
> >  MAINTAINERS                             |   7 +
> >  drivers/i2c/busses/Kconfig              |   9 +
> >  drivers/i2c/busses/Makefile             |   1 +
> >  drivers/i2c/busses/i2c-nvidia-gpu.c     | 372
> ++++++++++++++++++++++++++++++++
> >  5 files changed, 407 insertions(+)
> >  create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu
> >  create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c
> >
> > diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu
> > b/Documentation/i2c/busses/i2c-nvidia-gpu
> > new file mode 100644
> > index 0000000..31884d2
> > --- /dev/null
> > +++ b/Documentation/i2c/busses/i2c-nvidia-gpu
> > @@ -0,0 +1,18 @@
> > +Kernel driver i2c-nvidia-gpu
> > +
> > +Datasheet: not publicly available.
> > +
> > +Authors:
> > +	Ajay Gupta <ajayg@nvidia.com>
> > +
> > +Description
> > +-----------
> > +
> > +i2c-nvidia-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.
> > diff --git a/MAINTAINERS b/MAINTAINERS index d870cb5..b71b0b4 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6798,6 +6798,13 @@ L:	linux-acpi@vger.kernel.org
> >  S:	Maintained
> >  F:	drivers/i2c/i2c-core-acpi.c
> >
> > +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> > +M:	Ajay Gupta <ajayg@nvidia.com>
> > +L:	linux-i2c@vger.kernel.org
> > +S:	Maintained
> > +F:	Documentation/i2c/busses/i2c-nvidia-gpu
> > +F:	drivers/i2c/busses/i2c-nvidia-gpu.c
> > +
> >  I2C MUXES
> >  M:	Peter Rosin <peda@axentia.se>
> >  L:	linux-i2c@vger.kernel.org
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 451d4ae..eed827b 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -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_NVIDIA_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-nvidia-gpu.
> > +
> >  config I2C_SIS5595
> >  	tristate "SiS 5595"
> >  	depends on PCI
> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index 18b26af..d499813 100644
> > --- a/drivers/i2c/busses/Makefile
> > +++ b/drivers/i2c/busses/Makefile
> > @@ -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_NVIDIA_GPU)	+= i2c-nvidia-gpu.o
> >
> >  ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git
> > a/drivers/i2c/busses/i2c-nvidia-gpu.c
> > b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > new file mode 100644
> > index 0000000..e88424f
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> > @@ -0,0 +1,372 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Nvidia GPU I2C controller Driver
> > + *
> > + * Copyright (C) 2018 NVIDIA Corporation. All rights reserved.
> > + * Author: Ajay Gupta <ajayg@nvidia.com>  */ #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> #include <linux/pm.h> #include
> > +<linux/pm_runtime.h>
> > +
> > +#include <asm/unaligned.h>
> > +
> > +/* I2C definitions */
> > +#define I2C_MST_CNTL				0x00
> > +#define I2C_MST_CNTL_GEN_START			BIT(0)
> > +#define I2C_MST_CNTL_GEN_STOP			BIT(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_GEN_RAB			BIT(4)
> > +#define I2C_MST_CNTL_BURST_SIZE_SHIFT		6
> > +#define I2C_MST_CNTL_GEN_NACK			BIT(28)
> > +#define I2C_MST_CNTL_STATUS			GENMASK(30, 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		BIT(31)
> > +
> > +#define I2C_MST_ADDR				0x04
> > +
> > +#define I2C_MST_I2C0_TIMING				0x08
> > +#define I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ		0x10e
> > +#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		BIT(24)
> > +
> > +#define I2C_MST_DATA					0x0c
> > +
> > +#define I2C_MST_HYBRID_PADCTL				0x20
> > +#define I2C_MST_HYBRID_PADCTL_MODE_I2C			BIT(0)
> > +#define I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV
> 	BIT(14)
> > +#define I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV
> 	BIT(15)
> > +
> > +struct gpu_i2c_dev {
> > +	struct device *dev;
> > +	void __iomem *regs;
> > +	struct i2c_adapter adapter;
> > +	struct i2c_client *client;
> > +};
> > +
> > +static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd) {
> > +	u32 val;
> > +
> > +	/* enable I2C */
> > +	val = readl(i2cd->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;
> > +	writel(val, i2cd->regs + I2C_MST_HYBRID_PADCTL);
> > +
> > +	/* enable 100KHZ mode */
> > +	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;
> > +	writel(val, i2cd->regs + I2C_MST_I2C0_TIMING); }
> > +
> > +static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd) {
> > +	unsigned long target = jiffies + msecs_to_jiffies(1000);
> > +	u32 val;
> > +
> > +	do {
> > +		val = readl(i2cd->regs + I2C_MST_CNTL);
> > +		if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
> > +			break;
> > +		if ((val & I2C_MST_CNTL_STATUS) !=
> > +				I2C_MST_CNTL_STATUS_BUS_BUSY)
> > +			break;
> > +		usleep_range(1000, 2000);
> > +	} while (time_is_after_jiffies(target));
> > +
> > +	if (time_is_before_jiffies(target)) {
> > +		dev_err(i2cd->dev, "i2c timeout error %x\n", val);
> > +		return -EIO;
> > +	}
> > +
> > +	val = readl(i2cd->regs + I2C_MST_CNTL);
> > +	switch (val & I2C_MST_CNTL_STATUS) {
> > +	case I2C_MST_CNTL_STATUS_OKAY:
> > +		return 0;
> > +	case I2C_MST_CNTL_STATUS_NO_ACK:
> > +		return -EIO;
> > +	case I2C_MST_CNTL_STATUS_TIMEOUT:
> > +		return -ETIME;
> > +	case I2C_MST_CNTL_STATUS_BUS_BUSY:
> > +		return -EBUSY;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static int gpu_i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16 len)
> > +{
> > +	int status;
> > +	u32 val;
> > +
> > +	val = I2C_MST_CNTL_GEN_START | 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 | I2C_MST_CNTL_GEN_STOP);
> 
> Hmm, I didn't notice this before, but this masks out bits that are already zero
> and can be dropped AFAICT.
We can remove but I thought its better for code readability on what is not being
done.

> 
> > +	writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > +	status = gpu_i2c_check_status(i2cd);
> > +	if (status < 0)
> > +		return status;
> > +
> > +	val = readl(i2cd->regs + I2C_MST_DATA);
> > +	switch (len) {
> > +	case 1:
> > +		data[0] = val;
> > +		break;
> > +	case 2:
> > +		put_unaligned_be16(val, data);
> > +		break;
> > +	case 3:
> > +		put_unaligned_be16(val >> 8, data);
> > +		data[2] = val;
> > +		break;
> > +	case 4:
> > +		put_unaligned_be32(val, data);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +	return status;
> > +}
> > +
> > +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd) {
> > +	u32 val;
> > +
> > +	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);
> 
> Dito. I also suspect that ..._GEN_NACK is insignificant here? I have no way of
> knowing that, but if so, and given that ..._CMD_NONE is zero, this can become
> much more readable with a simple
ok
> 
> 	writel(I2C_MST_CNTL_GEN_START, i2cd->regs + I2C_MST_CNTL);
> 
> > +	writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > +	return gpu_i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd) {
> > +	u32 val;
> > +
> > +	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);
> 
> Dito, but
> 
> 	writel(I2C_MST_CNTL_GEN_STOP, i2cd->regs + I2C_MST_CNTL);
> 
ok
> > +	writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > +	return gpu_i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_write(struct gpu_i2c_dev *i2cd, u8 data) {
> > +	u32 val;
> > +
> > +	writel(data, i2cd->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);
> 
> Again, zeroing out empty bits.
> 
> > +	writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > +	return gpu_i2c_check_status(i2cd);
> > +}
> > +
> > +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> > +			       struct i2c_msg *msgs, int num) {
> > +	struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> > +	int status, status2;
> > +	int i, j;
> > +
> > +	for (i = 0; i < num; i++) {
> > +		if (msgs[i].flags & I2C_M_RD) {
> > +			/* program client address before starting read */
> > +			writel(msgs[i].addr, i2cd->regs + I2C_MST_ADDR);
> > +			/* gpu_i2c_read has implicit start */
> > +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> > +			if (status < 0)
> > +				return status;
> 
> 				goto stop;
> 
> Hmm, that goto stop is however not perfect. Ideally, 

> you shouldn't issue stop if i == 0 and gpu_i2c_read failed 
How can there be a read without rab write first?
AFAIU, gpu_i2c_read() can get called only with i=1 onward.

> due to failure to output start.Not sure if you can know that? 
There is no way to tell that.

> Maybe you can make the start of reads be explicit, just like
> you could with the stop? 
I need to see if that works.

> In that case, maybe you can move the below
> gpu_i2c_start call (with fixed error handling) to before the above if (... &
> I2C_M_RD)?
Ok, will check.
> 
> > +		} else {
> > +			u8 addr = i2c_8bit_addr_from_msg(msgs + i);
> > +
> > +			status = gpu_i2c_start(i2cd);
> > +			if (status < 0)
> > +				return status;
> 
> You fail to issue a terminating stop if a restart fails.
ok
> 
> 			if (status < 0) {
> 				if (i == 0)
> 					return status;
> 				goto stop;
> 			}
> 
> > +
> > +			status = gpu_i2c_write(i2cd, addr);
> > +			if (status < 0)
> > +				goto stop;
> > +
> > +			for (j = 0; j < msgs[i].len; j++) {
> > +				status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
> > +				if (status < 0)
> > +					goto stop;
> > +			}
> > +		}
> > +	}
> > +	status = gpu_i2c_stop(i2cd);
> > +	if (status < 0)
> > +		return status;
> > +	return i;
> > +stop:
> > +	status2 = gpu_i2c_stop(i2cd);
> > +	if (status2 < 0)
> > +		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
> > +	return status;
> > +}
> > +
> > +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> > +	.max_read_len = 4,
> > +};
> > +
> > +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,
> > +};
> > +
> > +/*
> > + * This driver is for Nvidia GPU cards with USB Type-C interface.
> > + * We want to identify the cards using vendor ID and class code only
> > + * to avoid dependency of adding product id for any new card which
> > + * requires this driver.
> > + * Currently there is no class code defined for UCSI device over PCI
> > + * so using UNKNOWN class for now and it will be updated when UCSI
> > + * over PCI gets a class code.
> > + * There is no other NVIDIA cards with UNKNOWN class code. Even if
> > +the
> > + * driver gets loaded for an undesired card then eventually
> > +i2c_read()
> > + * (initiated from UCSI i2c_client) will timeout or UCSI commands
> > +will
> > + * timeout.
> > + */
> > +#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
> > +static const struct pci_device_id gpu_i2c_ids[] = {
> > +	{ 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);
> > +
> > +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq) {
> > +	static struct i2c_board_info gpu_ccgx_ucsi = {
> > +		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
> > +	};
> 
> Shouldn't this struct be dynamically allocated and attached to struct
> gpu_i2c_dev so that you (maybe) could have more than one instance?
Currently the structure is local variable. Will that not work in multi
instance cases?

Thanks
Ajay

--
nvpublic
--
> 
> Cheers,
> Peter
> 
> > +
> > +	gpu_ccgx_ucsi.irq = irq;
> > +	i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi);
> > +	if (!i2cd->client)
> > +		return -ENODEV;
> > +
> > +	return 0;
> > +}
> > +
> > +static int gpu_i2c_probe(struct pci_dev *pdev, const struct
> > +pci_device_id *id) {
> > +	struct gpu_i2c_dev *i2cd;
> > +	int status;
> > +
> > +	i2cd = devm_kzalloc(&pdev->dev, sizeof(struct gpu_i2c_dev),
> GFP_KERNEL);
> > +	if (!i2cd)
> > +		return -ENOMEM;
> > +
> > +	i2cd->dev = &pdev->dev;
> > +	dev_set_drvdata(&pdev->dev, i2cd);
> > +
> > +	status = pcim_enable_device(pdev);
> > +	if (status < 0) {
> > +		dev_err(&pdev->dev, "pcim_enable_device failed %d\n",
> status);
> > +		return status;
> > +	}
> > +
> > +	pci_set_master(pdev);
> > +
> > +	i2cd->regs = pcim_iomap(pdev, 0, 0);
> > +	if (!i2cd->regs) {
> > +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> > +	if (status < 0) {
> > +		dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n",
> status);
> > +		return status;
> > +	}
> > +
> > +	gpu_enable_i2c_bus(i2cd);
> > +
> > +	i2c_set_adapdata(&i2cd->adapter, i2cd);
> > +	i2cd->adapter.owner = THIS_MODULE;
> > +	strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
> > +		sizeof(i2cd->adapter.name));
> > +	i2cd->adapter.algo = &gpu_i2c_algorithm;
> > +	i2cd->adapter.quirks = &gpu_i2c_quirks;
> > +	i2cd->adapter.dev.parent = &pdev->dev;
> > +	status = i2c_add_adapter(&i2cd->adapter);
> > +	if (status < 0)
> > +		goto free_irq_vectors;
> > +
> > +	status = gpu_populate_client(i2cd, pdev->irq);
> > +	if (status < 0) {
> > +		dev_err(&pdev->dev, "gpu_populate_client failed %d\n",
> status);
> > +		goto del_adapter;
> > +	}
> > +
> > +	return 0;
> > +
> > +del_adapter:
> > +	i2c_del_adapter(&i2cd->adapter);
> > +free_irq_vectors:
> > +	pci_free_irq_vectors(pdev);
> > +	return status;
> > +}
> > +
> > +static void gpu_i2c_remove(struct pci_dev *pdev) {
> > +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
> > +
> > +	i2c_del_adapter(&i2cd->adapter);
> > +	pci_free_irq_vectors(pdev);
> > +}
> > +
> > +static int gpu_i2c_resume(struct device *dev) {
> > +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> > +
> > +	gpu_enable_i2c_bus(i2cd);
> > +	return 0;
> > +}
> > +
> > +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume,
> NULL);
> > +
> > +static struct pci_driver gpu_i2c_driver = {
> > +	.name		= "nvidia-gpu",
> > +	.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 <ajayg@nvidia.com>");
> > +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> > +MODULE_LICENSE("GPL v2");
> >

[v11,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[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 adds 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 <ajayg@nvidia.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
Changes from v1 -> v2
	None
Changes from v2 -> v3
	Fixed review comments from Andy and Thierry
	Rename i2c-gpu.c -> i2c-nvidia-gpu.c
Changes from v3 -> v4
	Fixed review comments from Andy
Changes from v4 -> v5
	Fixed review comments from Andy
Changes from v5 -> v6
	None 
Changes from v6 -> v7 -> v8
	Fixed review comments from Peter 
	- Add implicit STOP for last write message
	- Add i2c_adapter_quirks with max_read_len and
	  I2C_AQ_COMB flags
Changes from v8 -> v9
	Fixed review comments from Peter
	- Drop do_start flag
	- Use i2c_8bit_addr_from_msg()
Changes from v9 -> v10
	Fixed review comments from Peter
	- Dropped I2C_FUNC_SMBUS_EMUL
	- Dropped local mutex
Changes from v10 -> v11
	Fixed review comments from Peter
	- Moved stop in master_xfer at end of message
	- Change i2c_read without STOP
	- Dropped I2C_AC_COMB* flags
	
 Documentation/i2c/busses/i2c-nvidia-gpu |  18 ++
 MAINTAINERS                             |   7 +
 drivers/i2c/busses/Kconfig              |   9 +
 drivers/i2c/busses/Makefile             |   1 +
 drivers/i2c/busses/i2c-nvidia-gpu.c     | 372 ++++++++++++++++++++++++++++++++
 5 files changed, 407 insertions(+)
 create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu
 create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c

diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu b/Documentation/i2c/busses/i2c-nvidia-gpu
new file mode 100644
index 0000000..31884d2
--- /dev/null
+++ b/Documentation/i2c/busses/i2c-nvidia-gpu
@@ -0,0 +1,18 @@
+Kernel driver i2c-nvidia-gpu
+
+Datasheet: not publicly available.
+
+Authors:
+	Ajay Gupta <ajayg@nvidia.com>
+
+Description
+-----------
+
+i2c-nvidia-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.
diff --git a/MAINTAINERS b/MAINTAINERS
index d870cb5..b71b0b4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6798,6 +6798,13 @@ L:	linux-acpi@vger.kernel.org
 S:	Maintained
 F:	drivers/i2c/i2c-core-acpi.c
 
+I2C CONTROLLER DRIVER FOR NVIDIA GPU
+M:	Ajay Gupta <ajayg@nvidia.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/i2c/busses/i2c-nvidia-gpu
+F:	drivers/i2c/busses/i2c-nvidia-gpu.c
+
 I2C MUXES
 M:	Peter Rosin <peda@axentia.se>
 L:	linux-i2c@vger.kernel.org
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 451d4ae..eed827b 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -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_NVIDIA_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-nvidia-gpu.
+
 config I2C_SIS5595
 	tristate "SiS 5595"
 	depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 18b26af..d499813 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -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_NVIDIA_GPU)	+= i2c-nvidia-gpu.o
 
 ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
new file mode 100644
index 0000000..e88424f
--- /dev/null
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -0,0 +1,372 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nvidia GPU I2C controller Driver
+ *
+ * Copyright (C) 2018 NVIDIA Corporation. All rights reserved.
+ * Author: Ajay Gupta <ajayg@nvidia.com>
+ */
+#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>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+
+#include <asm/unaligned.h>
+
+/* I2C definitions */
+#define I2C_MST_CNTL				0x00
+#define I2C_MST_CNTL_GEN_START			BIT(0)
+#define I2C_MST_CNTL_GEN_STOP			BIT(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_GEN_RAB			BIT(4)
+#define I2C_MST_CNTL_BURST_SIZE_SHIFT		6
+#define I2C_MST_CNTL_GEN_NACK			BIT(28)
+#define I2C_MST_CNTL_STATUS			GENMASK(30, 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		BIT(31)
+
+#define I2C_MST_ADDR				0x04
+
+#define I2C_MST_I2C0_TIMING				0x08
+#define I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ		0x10e
+#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		BIT(24)
+
+#define I2C_MST_DATA					0x0c
+
+#define I2C_MST_HYBRID_PADCTL				0x20
+#define I2C_MST_HYBRID_PADCTL_MODE_I2C			BIT(0)
+#define I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV		BIT(14)
+#define I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV		BIT(15)
+
+struct gpu_i2c_dev {
+	struct device *dev;
+	void __iomem *regs;
+	struct i2c_adapter adapter;
+	struct i2c_client *client;
+};
+
+static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd)
+{
+	u32 val;
+
+	/* enable I2C */
+	val = readl(i2cd->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;
+	writel(val, i2cd->regs + I2C_MST_HYBRID_PADCTL);
+
+	/* enable 100KHZ mode */
+	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;
+	writel(val, i2cd->regs + I2C_MST_I2C0_TIMING);
+}
+
+static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd)
+{
+	unsigned long target = jiffies + msecs_to_jiffies(1000);
+	u32 val;
+
+	do {
+		val = readl(i2cd->regs + I2C_MST_CNTL);
+		if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
+			break;
+		if ((val & I2C_MST_CNTL_STATUS) !=
+				I2C_MST_CNTL_STATUS_BUS_BUSY)
+			break;
+		usleep_range(1000, 2000);
+	} while (time_is_after_jiffies(target));
+
+	if (time_is_before_jiffies(target)) {
+		dev_err(i2cd->dev, "i2c timeout error %x\n", val);
+		return -EIO;
+	}
+
+	val = readl(i2cd->regs + I2C_MST_CNTL);
+	switch (val & I2C_MST_CNTL_STATUS) {
+	case I2C_MST_CNTL_STATUS_OKAY:
+		return 0;
+	case I2C_MST_CNTL_STATUS_NO_ACK:
+		return -EIO;
+	case I2C_MST_CNTL_STATUS_TIMEOUT:
+		return -ETIME;
+	case I2C_MST_CNTL_STATUS_BUS_BUSY:
+		return -EBUSY;
+	default:
+		return 0;
+	}
+}
+
+static int gpu_i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16 len)
+{
+	int status;
+	u32 val;
+
+	val = I2C_MST_CNTL_GEN_START | 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 | I2C_MST_CNTL_GEN_STOP);
+	writel(val, i2cd->regs + I2C_MST_CNTL);
+
+	status = gpu_i2c_check_status(i2cd);
+	if (status < 0)
+		return status;
+
+	val = readl(i2cd->regs + I2C_MST_DATA);
+	switch (len) {
+	case 1:
+		data[0] = val;
+		break;
+	case 2:
+		put_unaligned_be16(val, data);
+		break;
+	case 3:
+		put_unaligned_be16(val >> 8, data);
+		data[2] = val;
+		break;
+	case 4:
+		put_unaligned_be32(val, data);
+		break;
+	default:
+		break;
+	}
+	return status;
+}
+
+static int gpu_i2c_start(struct gpu_i2c_dev *i2cd)
+{
+	u32 val;
+
+	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, i2cd->regs + I2C_MST_CNTL);
+
+	return gpu_i2c_check_status(i2cd);
+}
+
+static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd)
+{
+	u32 val;
+
+	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, i2cd->regs + I2C_MST_CNTL);
+
+	return gpu_i2c_check_status(i2cd);
+}
+
+static int gpu_i2c_write(struct gpu_i2c_dev *i2cd, u8 data)
+{
+	u32 val;
+
+	writel(data, i2cd->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, i2cd->regs + I2C_MST_CNTL);
+
+	return gpu_i2c_check_status(i2cd);
+}
+
+static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
+			       struct i2c_msg *msgs, int num)
+{
+	struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
+	int status, status2;
+	int i, j;
+
+	for (i = 0; i < num; i++) {
+		if (msgs[i].flags & I2C_M_RD) {
+			/* program client address before starting read */
+			writel(msgs[i].addr, i2cd->regs + I2C_MST_ADDR);
+			/* gpu_i2c_read has implicit start */
+			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
+			if (status < 0)
+				return status;
+		} else {
+			u8 addr = i2c_8bit_addr_from_msg(msgs + i);
+
+			status = gpu_i2c_start(i2cd);
+			if (status < 0)
+				return status;
+
+			status = gpu_i2c_write(i2cd, addr);
+			if (status < 0)
+				goto stop;
+
+			for (j = 0; j < msgs[i].len; j++) {
+				status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
+				if (status < 0)
+					goto stop;
+			}
+		}
+	}
+	status = gpu_i2c_stop(i2cd);
+	if (status < 0)
+		return status;
+	return i;
+stop:
+	status2 = gpu_i2c_stop(i2cd);
+	if (status2 < 0)
+		dev_err(i2cd->dev, "i2c stop failed %d\n", status2);
+	return status;
+}
+
+static const struct i2c_adapter_quirks gpu_i2c_quirks = {
+	.max_read_len = 4,
+};
+
+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,
+};
+
+/*
+ * This driver is for Nvidia GPU cards with USB Type-C interface.
+ * We want to identify the cards using vendor ID and class code only
+ * to avoid dependency of adding product id for any new card which
+ * requires this driver.
+ * Currently there is no class code defined for UCSI device over PCI
+ * so using UNKNOWN class for now and it will be updated when UCSI
+ * over PCI gets a class code.
+ * There is no other NVIDIA cards with UNKNOWN class code. Even if the
+ * driver gets loaded for an undesired card then eventually i2c_read()
+ * (initiated from UCSI i2c_client) will timeout or UCSI commands will
+ * timeout.
+ */
+#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
+static const struct pci_device_id gpu_i2c_ids[] = {
+	{ 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);
+
+static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq)
+{
+	static struct i2c_board_info gpu_ccgx_ucsi = {
+		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
+	};
+
+	gpu_ccgx_ucsi.irq = irq;
+	i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi);
+	if (!i2cd->client)
+		return -ENODEV;
+
+	return 0;
+}
+
+static int gpu_i2c_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct gpu_i2c_dev *i2cd;
+	int status;
+
+	i2cd = devm_kzalloc(&pdev->dev, sizeof(struct gpu_i2c_dev), GFP_KERNEL);
+	if (!i2cd)
+		return -ENOMEM;
+
+	i2cd->dev = &pdev->dev;
+	dev_set_drvdata(&pdev->dev, i2cd);
+
+	status = pcim_enable_device(pdev);
+	if (status < 0) {
+		dev_err(&pdev->dev, "pcim_enable_device failed %d\n", status);
+		return status;
+	}
+
+	pci_set_master(pdev);
+
+	i2cd->regs = pcim_iomap(pdev, 0, 0);
+	if (!i2cd->regs) {
+		dev_err(&pdev->dev, "pcim_iomap failed\n");
+		return -ENOMEM;
+	}
+
+	status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
+	if (status < 0) {
+		dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n", status);
+		return status;
+	}
+
+	gpu_enable_i2c_bus(i2cd);
+
+	i2c_set_adapdata(&i2cd->adapter, i2cd);
+	i2cd->adapter.owner = THIS_MODULE;
+	strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
+		sizeof(i2cd->adapter.name));
+	i2cd->adapter.algo = &gpu_i2c_algorithm;
+	i2cd->adapter.quirks = &gpu_i2c_quirks;
+	i2cd->adapter.dev.parent = &pdev->dev;
+	status = i2c_add_adapter(&i2cd->adapter);
+	if (status < 0)
+		goto free_irq_vectors;
+
+	status = gpu_populate_client(i2cd, pdev->irq);
+	if (status < 0) {
+		dev_err(&pdev->dev, "gpu_populate_client failed %d\n", status);
+		goto del_adapter;
+	}
+
+	return 0;
+
+del_adapter:
+	i2c_del_adapter(&i2cd->adapter);
+free_irq_vectors:
+	pci_free_irq_vectors(pdev);
+	return status;
+}
+
+static void gpu_i2c_remove(struct pci_dev *pdev)
+{
+	struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
+
+	i2c_del_adapter(&i2cd->adapter);
+	pci_free_irq_vectors(pdev);
+}
+
+static int gpu_i2c_resume(struct device *dev)
+{
+	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
+
+	gpu_enable_i2c_bus(i2cd);
+	return 0;
+}
+
+UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume, NULL);
+
+static struct pci_driver gpu_i2c_driver = {
+	.name		= "nvidia-gpu",
+	.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 <ajayg@nvidia.com>");
+MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
+MODULE_LICENSE("GPL v2");