On Mon, Jan 9, 2017 at 12:32 AM, Sudip Mukherjee
[off-list ref] wrote:

From: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>

Exar XR17V352/354/358 chips have 16 multi-purpose inputs/outputs which
can be controlled using gpio interface.

Add the gpio specific code.W

Few comments since it will be v9.

+#define DRIVER_NAME "gpio_exar"
+
+static LIST_HEAD(exar_list);
+static DEFINE_MUTEX(exar_list_mtx);

+DEFINE_IDA(ida_index);

Should be static?

+
+struct exar_gpio_chip {
+       struct gpio_chip gpio_chip;
+       struct pci_dev *pcidev;
+       struct mutex lock;
+       struct list_head list;
+       int index;
+       void __iomem *regs;
+       char name[16];

Since you are using %d + 7 characters 16 wouldn't be enough.

+};

+static int exar_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+       int val;
+
+       if (offset < 8)
+               val = exar_get(chip, EXAR_OFFSET_MPIOSEL_LO) >> offset;
+       else
+               val = exar_get(chip, EXAR_OFFSET_MPIOSEL_HI) >>
+                              (offset - 8);

By the way, does it support up to 16 pins only?

You might consider something like this instead

addr = offset / 8 ? HI : LO;

op(addr) >> (offset % 8);

Similar for the rest of functions.

+static int gpio_exar_probe(struct platform_device *pdev)
+{
+       struct pci_dev *dev = platform_get_drvdata(pdev);
+       struct exar_gpio_chip *exar_gpio;
+       void __iomem *p;

+       int index = 1;

Redundant assignment

+       int ret;
+
+       if (dev->vendor != PCI_VENDOR_ID_EXAR)
+               return -ENODEV;
+
+       p = pci_ioremap_bar(dev, 0);

I would make it clear by using pcim_iomap(dev, 0, 0);
And put a comment that explains why we are using managed function here.

-- 
With Best Regards,
Andy Shevchenko