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]

 



On Fri, Aug 31, 2018 at 12:41 AM Ajay Gupta <ajayg@xxxxxxxxxx> wrote:
>
> 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.

> +#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.

> +#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.

> +                       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.

> +       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; ?

> +       }

> +       return 0;

See above.

> +}

> +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.

> +       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?

> +}

> +/*
> + * 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...

> +#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.

> +       { }
> +};
> +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.

> +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() ?

> +       struct gpu_i2c_dev *i2cd = pci_get_drvdata(to_pci_dev(dev));

Ditto.

-- 
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