Messages in this thread | | | From | Andy Shevchenko <> | Date | Mon, 9 Jan 2017 02:30:28 +0200 | Subject | Re: [PATCH v8 1/3] gpio: exar: add gpio for exar cards |
| |
On Mon, Jan 9, 2017 at 12:32 AM, Sudip Mukherjee <sudipm.mukherjee@gmail.com> 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
| |