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
