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

[Ajay Gupta <ajayg@nvidia.com>]

Hi Peter,
> >> This seems to behave rather strange for len > 4, and I do not see
> >> anything preventing that from happening.
> > Actually the check is in ccg_read() of the client driver at
> > https://marc.info/?l=linux-usb&m=153618608301206&w=2
> >
> >> Am I missing something, or do you need to add a quirk for max_read_len?
> > I will add a check in this function as well.
> 
> No, you should add a pointer to a struct i2c_adapter_quirks, with
> max_read_len set I think. At least that's how e.g. i2c-qup.c does it.
Ok, will fix in next version.
 
> >>> +		break;

> >>> +	mutex_lock(&i2cd->mutex);
> >>> +	for (i = 0; i < num; i++) {
> >>
> >> I don't get how this loop works. You do not seem to always start with a
> start.
> >> E.g., if the first message is I2C_M_RD, i2c_read() is called before
> >> i2c_start(). Is that correct?
> > That’s correct and it is because i2_read() itself does the START and STOP.
> 
> Well, in that case, you don't fully support combined I2C transactions.
> You cannot e.g. generate this from Documentation/i2c/i2c-protocol
> 
> 	S Addr Rd [A] [Data] NA S Addr Wr [A] Data [A] P
> 
> By your description, all reads are terminated by a stop, and that is a quirk. I
> think you need to add some of the I2C_AQ_COMB* flags to the above
> mentioned struct i2c_adapter_quirks.
Ok, will add those quirks flags. Also will modify to have implicit STOP after
last write message.
 
> >> Also, if a message has I2C_M_STOP but not I2C_M_RD, the call to
> >> i2c_stop() happens before the call the i2c_write(). What's up with that?
> > Client driver sends I2C_M_STOP along with every write message.
> 
> Why is it certain that the client driver in 2/2 is the only client of this adapter?
> If that's really fundamentally the case, and it can't change for whatever
> reason, then I think these things should be mentioned somewhere.
There can be other client. I will update the driver with quirks and implicit
STOP after last write message.

Thanks
Ajay

--
nvpublic
--

> >> I would expect an i2c_start() before the loop or first in the loop,
> >> and a
> >> i2c_stop() after the loop.
> > I2c_read() function itself takes care of it.
> >
> >> As is, the stop after the loop is only called on error.
> > To be exact, whenever there is error in i2c_write().
> >
> >> In addition, I would expect messages that have I2C_M_STOP to trigger
> >> an
> >> i2c_stop() call *after* the message, even if the message is not last in the
> loop.
> > Yes, its like that for all write messages.
> >
> >> What am I missing?
> >>
> >>> +		if (msgs[i].flags & I2C_M_RD) {
> >>> +			status = i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> >>> +			if (status < 0) {
> >>> +				dev_err(dev, "i2c_read error %x", status);
> >>> +				goto unlock;
> >>> +			}
> >>> +			i2cd->do_start = true;
> >>> +		} else if (msgs[i].flags & I2C_M_STOP) {
> >>> +			status = i2c_stop(i2cd);
> >>> +			if (status < 0) {
> >>> +				dev_err(dev, "i2c_stop error %x", status);
> >>> +				goto unlock;
> >>> +			}
> >>> +			i2cd->do_start = true;
> >>> +		} else {
> >>> +			if (i2cd->do_start) {
> >>> +				status = i2c_start(i2cd, msgs[i].addr);
> >>> +				if (status < 0) {
> >>> +					dev_err(dev, "i2c_start error %x",
> >>> +						status);
> >>> +					goto unlock;
> >>> +				}
> >>> +				status = i2c_write(i2cd, msgs[i].addr << 1);
> >>> +				if (status < 0) {
> >>> +					dev_err(dev, "i2c_write error %x",
> >>> +						status);
> >>> +					goto stop;
> >>> +				}
> >>> +				i2cd->do_start = false;
> >>> +			}
> >>> +			for (j = 0; j < msgs[i].len; j++) {
> >>> +				status = i2c_write(i2cd, *(msgs[i].buf + j));
> >>> +				if (status < 0) {
> >>> +					dev_err(dev, "i2c_write error %x",
> >>> +						status);
> >>> +					goto stop;
> >>> +				}
> >>> +			}
> >>> +		}
> >>> +	}
> >>> +	status = i;
> >>> +	goto unlock;
> > <here>
> >
> >>> +stop:
> >>> +	status = i2c_stop(i2cd);
> >>> +	if (status < 0)
> >>> +		dev_err(dev, "i2c_stop error %x", status);
> >>
> >> Hmm, every time you call one of i2c_start, i2c_read, i2c_write or
> >> i2c_stop you check for error and issue a generic dev_err message. I
> >> think you should move these error messages into the functions instead
> >> or repeating them for every use.
> > Ok, will fix.
> >
> >>> +unlock:
> >>> +	mutex_unlock(&i2cd->mutex);
> >>> +	return status;
> >>
> >> Here you return status, which seems to be zero when everything is fine.
> >> That is incorrect for sure; you should return num on success.
> > When everything is fine then status is written with number of messages
> > "i". Please see above.
> 
> Right you are, I missed that assignment. Sorry...
> 
> Cheers,
> Peter
> 
> > Thanks
> > Ajay
> >
> > --
> > nvpublic
> > --
> >
> >> Cheers,
> >> Peter
> >>
> >>> +}
> >>> +
> >>> +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) {
> >>> +		dev_err(i2cd->dev, "i2c_new_device failed\n");
> >>> +		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;
> >>> +	}
> >>> +
> >>> +	i2cd->do_start = true;
> >>> +	mutex_init(&i2cd->mutex);
> >>> +	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.dev.parent = &pdev->dev;
> >>> +	status = i2c_add_adapter(&i2cd->adapter);
> >>> +	if (status < 0) {
> >>> +		dev_err(&pdev->dev, "i2c_add_adapter failed %d\n", status);
> >>> +		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);
> >>> +
> >>> +	enable_i2c_bus(i2cd);
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int gpu_i2c_idle(struct device *dev) {
> >>> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> >>> +
> >>> +	if (!mutex_trylock(&i2cd->mutex)) {
> >>> +		dev_info(dev, "-EBUSY\n");
> >>> +		return -EBUSY;
> >>> +	}
> >>> +	mutex_unlock(&i2cd->mutex);
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume,
> >>> +gpu_i2c_idle);
> >>> +
> >>> +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");
> >>>
> >