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

[Ajay Gupta <ajayg@nvidia.com>]

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 add I2C bus driver to communicate with Type-C controller.
> > I2C client driver will be part of USB Type-C UCSI driver.
> 
> >  drivers/i2c/busses/i2c-gpu.c     | 493
> +++++++++++++++++++++++++++++++++++++++
> 
> Can we got more better name, which includes vendor and/or model of the I2C
> host?
Sure will change to i2c-nvidia-gpu.c

> > +/* 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
> 
> Looks slightly different from my point of view, something like
> 
> /* Bit 31 shows error condition while LSB encodes the error code */
> STATUS_TIMEOUT BIT(0)
> ...
> STATUS_ERROR  BIT(31)
Will fix.

> > +       dev_dbg(dev, "%s: %p (I2C_MST_HYBRID_PADCTL) <- %08x", __func__,
> > +               (gdev->regs + I2C_MST_HYBRID_PADCTL), val);
> 
> Parens are redundant, __func__ is redundant.
Will fix.

> > +       dev_dbg(dev, "%s: %p (I2C_MST_I2C0_TIMING) <- %08x", __func__,
> > +               gdev->regs + I2C_MST_I2C0_TIMING, val);
> 
> Ditto. Check your debug messages, and perheps even drop some.
Will fix.

> > +static u32 i2c_check_status(struct gpu_i2c_dev *gdev)
> > +{
> 
> > +       while (time_is_after_jiffies(target)) {
> > +       }
> 
> For functions like this better to get in a form
> do {
> } while().
Ok, will fix.

> There is no guarantee that it runs even once in your case.
> 
> > +               dev_err(dev, "%si2c timeout", __func__);
> 
> No space?
Ok, will fix.
> 
> > +       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;
> 
> Redundant  & 0xff.
> We have get_unaligned*(), put_unaligned*() and many variations of
> cpu_to_Xe*() and Xe*_to_cpu().
Ok, will fix.
> 
> > +       u32 val = 0;
> 
> Redundant assignment.
Ok, will fix.
> 
> > +       val = addr << I2C_MST_ADDR_DAB;
> 
> > +       val = 0;
> 
> Ditto. What's wrong with assign value below directly?
> 
> > +       val |= I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE |
> > +               I2C_MST_CNTL_GEN_NACK;
> 
> > +       u32 val = 0;
> 
> Check your code for these kind of style mistakes.
> 
> > +/* gdev i2c adapter */
> 
> Pointless.
> 
> > +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;
> 
> > +       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;
> > +}
> 
> Ouch! Besides many small style issues and redundancy (like __LINE__),
> this function needs to be refactored to few smaller and readable ones.
Ok, will fix.
> 
> > +#define PCI_CLASS_SERIAL_UNKNOWN       0x0c80
> 
> > +/* pci driver */
> 
> Pointless.
Ok, will fix.
> 
> > +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},
> 
> Are you sure?!
Yes, we want to identify using vendor ID and class code. 
Currently there is no class code defined for UCSI device over PCI so using UNKNOWN.

> 
> > +       { },
> 
> Terminator line better w/o comma.
Ok, will fix.
> 
> > +};
> > +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);
> 
> Useless. We have PCI core printed this information out several times
> during the boot or, if card is hotpluggable, when it's plugged in.
Ok, will fix.
> 
> > +       gdev = devm_kzalloc(&dev->dev, sizeof(struct gpu_i2c_dev),
> GFP_KERNEL);
> > +       if (!gdev)
> > +               return -ENOMEM;
> 
> > +       status = pci_enable_device(dev);
> 
> Using devm_ without pcim_ sound slightly inconsistent.
Ok, will fix.
> 
> > +       status = pci_enable_msi(dev);
> 
> This done in the other way. Check pci_alloc_irq_vectors(), IIRC.
sure, will fix.
> 
> > +i2c_init_err:
> > +       pci_disable_msi(dev);
> > +enable_msi_err:
> > +       pci_iounmap(dev, gdev->regs);
> > +iomap_err:
> > +       pci_disable_device(dev);
> 
> At least above will gone after switching to pcim_
> 
> > +       pci_disable_msi(dev);
> > +       pci_iounmap(dev, gdev->regs);
> 
> Same.
> 
> > +       dev_dbg(dev, "%s\n", __func__);
> 
> Pointless. We have ftrace, for example to see this.
> 
> > +       dev_dbg(dev, "%s\n", __func__);
> 
> Ditto.
> 
> 
Thanks
Ajay
--
nvpublic
--

> --
> With Best Regards,
> Andy Shevchenko