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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Thierry,

> > 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 <ajayg@xxxxxxxxxx>
> > ---
> > Changes from v1 -> v2: None
> >
> >  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.
> 
> >
> > diff --git a/Documentation/i2c/busses/i2c-gpu
> > b/Documentation/i2c/busses/i2c-gpu
> > new file mode 100644
> > index 0000000..873ba34
> > --- /dev/null
> > +++ b/Documentation/i2c/busses/i2c-gpu
> 
> 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.
ok

> > @@ -0,0 +1,18 @@
> > +Kernel driver i2c-gpu
> > +
> > +Datasheet: not publicly available.
> > +
> > +Authors:
> > +	Ajay Gupta <ajayg@xxxxxxxxxx>
> > +
> > +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.
> > diff --git a/MAINTAINERS b/MAINTAINERS index b2fcd1c..e99f8a2 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6796,6 +6796,13 @@ L:	linux-acpi@xxxxxxxxxxxxxxx
> >  S:	Maintained
> >  F:	drivers/i2c/i2c-core-acpi.c
> >
> > +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> > +M:	Ajay Gupta <ajayg@xxxxxxxxxx>
> > +L:	linux-i2c@xxxxxxxxxxxxxxx
> > +S:	Maintained
> > +F:	Documentation/i2c/busses/i2c-gpu
> > +F:	drivers/i2c/busses/i2c-gpu.c
> > +
> >  I2C MUXES
> >  M:	Peter Rosin <peda@xxxxxxxxxx>
> >  L:	linux-i2c@xxxxxxxxxxxxxxx
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index 451d4ae..ff8b2d4 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_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
> > diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> > index 18b26af..15d2894 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_GPU)          += i2c-gpu.o
> >
> >  ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG diff --git
> > a/drivers/i2c/busses/i2c-gpu.c b/drivers/i2c/busses/i2c-gpu.c new file
> > mode 100644 index 0000000..0fd2944
> > --- /dev/null
> > +++ b/drivers/i2c/busses/i2c-gpu.c
> > @@ -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 <ajayg@xxxxxxxxxx>
> > + *
> > + */
> > +#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>
> > +
> > +/* 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.
Will fix.
 
> > +/* 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.
ok

> > +/* 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;
> > +	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?
Ok, will fix.
> 
> > +
> > +	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)
> > +			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.
Ok, will fix.
> 
> > +	}
> > +
> > +	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 |
> > +		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.
Sure.
> 
> > +
> > +	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 */
> > +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.
Ok, will fix.
> 
> > +	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);
> 		...
> 	}
ok
> 
> > +
> > +				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?
> 
> > +			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;
> > +				}
> > +			}
> > +		}
> > +	}
> > +	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.
ok
> 
> > +
> > +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) {
> > +	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.
ok
> 
> > +
> > +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.
> 
> > +	{ 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_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.
sure
> 
> > +
> > +	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;
> > +	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.
sure
> 
> > +
> > +	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.
ok
> 
> > +	pm_runtime_put_noidle(&dev->dev);
> > +	pm_runtime_allow(&dev->dev);
> > +
> > +	return 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:
> 		...
> 
ok
> > +
> > +static void gpu_i2c_remove(struct pci_dev *dev) {
> > +	struct gpu_i2c_dev *gdev = pci_get_drvdata(dev);
> > +
> > +	i2c_del_adapter(&gdev->adapter);
> > +	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?
Will remove.
> 
> > +
> > +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;
> > +	}
> > +	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?
Ok

Thanks
Ajay
--
nvpublic
--
> 
> > +	.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@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> > +MODULE_LICENSE("GPL v2");



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux