Hi Andy,
> > 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.
>
> Thanks for an update, my comments below.
Thanks for reviewing.
> > +#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>
>
> + blank line.
Ok.
I could not find kernel documentation which requires this blank line.
Can you please point me to it?
> > +#include <asm/unaligned.h>
>
> > +struct gpu_i2c_dev {
> > + struct device *dev;
> > + void __iomem *regs;
> > + struct i2c_adapter adapter;
> > + struct i2c_client *client;
> > + struct mutex mutex; /* to sync read/write */
> > + bool do_start;
> > +};
>
> > +static int 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) !=
> > + I2C_MST_CNTL_CYCLE_TRIGGER)
>
> Part after != redundant since it's one bit.
> But I'm fine with current as well.
Ok, will fix.
>
> > + break;
> > + if ((val & I2C_MST_CNTL_STATUS) !=
> > + I2C_MST_CNTL_STATUS_BUS_BUSY)
> > + break;
> > + usleep_range(1000, 2000);
> > + } while (time_is_after_jiffies(target));
>
> > +
>
> Redundant.
Ok, I will remove it but I didn't understand why its redundant?
I thought adding extra line would be more readable.
> > + if (time_is_before_jiffies(target))
> > + 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:
>
> > + break;
>
> return 0; ?
Ok, will fix.
>
> > + }
>
> > + return 0;
>
> See above.
Ok
>
> > +}
>
> > +static int 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);
>
> "|" should be on previous line to follow common style in this module.
The "|" here is to clear the bit together. [val &= ~(X | Y | Z)]
Style in this module is still followed. Please see first line which does "|" to val.
> > + writel(val, i2cd->regs + I2C_MST_CNTL);
> > +
> > + return 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);
> > + struct device *dev = i2cd->dev;
> > + int status;
> > + int i, j;
>
> > +stop:
> > + status = i2c_stop(i2cd);
> > + if (status < 0)
> > + dev_err(dev, "i2c_stop error %x", status);
> > +unlock:
> > + mutex_unlock(&i2cd->mutex);
>
> > + return i;
>
> Shouldn't it return status in case of error?
Thanks, will fix.
> > +}
>
> > +/*
> > + * This driver is for the Nvidia GPU cards with USB Type-C interface.
> > + * We want to identify the cards using vendor ID and class code.
> > + * Currently there is no class code defined for UCSI device over PCI
> > + * so using UNKNOWN.
> > + */
>
> So, I didn't see how it *guarantees* no collision with other devices of the
> same class...
I checked and there is no other NVIDIA cards with UNKNOWN class code.
Moreover, even if the driver gets loaded for a wrong card then eventually
i2c_read() (initiated from UCSI driver) will timeout or UCSI commands will
timeout. I think this is safe enough for now. We will update the class code
when UCSI gets a PCI class code.
We don't want to add dependency of adding product id for any new card
which requires this driver.
> > +#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},
>
> ...for now, I would suggest to be more stricted, i.e.
>
> { PCI_VDEVICE(NVIDIA, 0x1ad9) },
>
> Whenever the class appears it can be added later on.
See above.
>
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
> > +
>
> > +static int gpu_i2c_probe(struct pci_dev *dev, const struct
> > +pci_device_id *id)
>
> Use pdev instead of dev to distinguish struct device from struct pci_dev type
> in variable name.
ok
>
> > +static void gpu_i2c_remove(struct pci_dev *dev)
>
> Ditto.
>
> > + struct gpu_i2c_dev *i2cd = pci_get_drvdata(to_pci_dev(dev));
> Isn't the same as dev_get_drvdata() ?
ok
>
> > + struct gpu_i2c_dev *i2cd = pci_get_drvdata(to_pci_dev(dev));
>
> Ditto.
Ok
Thanks
Ajay
--
nvpublic
--
>
> --
> With Best Regards,
> Andy Shevchenko
