RE: [PATCH v4 1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

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

 



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




[Index of Archives]     [Linux GPIO]     [Linux SPI]     [Linux Hardward Monitoring]     [LM Sensors]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux