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
