linux-kernel - Re: [PATCH v8 1/3] gpio: exar: add gpio for exar cards

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 9 Jan 2017 02:30:28 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Sudip Mukherjee <sudipm.mukherjee@...il.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        Alexandre Courbot <gnurou@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jslaby@...e.com>,
        One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
        Sudip Mukherjee <sudip.mukherjee@...ethink.co.uk>
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@...il.com> wrote:
> From: Sudip Mukherjee <sudip.mukherjee@...ethink.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