linux-kernel - Re: [PATCH][V2] gpio: add a driver for GPIO pins found on AMD-8111 south bridge chips

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]

Message-ID: <CACRpkdaRSE9ixE6qdJK+X_WqFQt7KauiUxZDpjNjng0U9SVacg@mail.gmail.com>
Date:	Tue, 29 May 2012 00:19:36 +0800
From:	Linus Walleij <linus.walleij@...aro.org>
To:	Dmitry Eremin-Solenikov <dbaryshkov@...il.com>
Cc:	Linus Walleij <linus.walleij@...ricsson.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH][V2] gpio: add a driver for GPIO pins found on AMD-8111
 south bridge chips

On Mon, May 28, 2012 at 2:48 PM, Dmitry Eremin-Solenikov
<dbaryshkov@...il.com> wrote:

> Add a driver to use GPIO pins available on several AMD south bridges
> (currently only AMD 8111 is supported).
>
> Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@...il.com>
> ---
>
> Changes since V1:
> * Replaced magic numbers in register access with named values

This is looking a lot better, some nitpicking:

> +#define AMD_GPIO_MODE_ALTFN    0x08 /* Or 0x09 */

Hm Hm I wonder what this register does... looks a lot like a mux.
(Just philisophizing...)

> +static int __init mod_init(void)
> +{
> +       int err = -ENODEV;
> +       struct pci_dev *pdev = NULL;
> +       const struct pci_device_id *ent;
> +
> +       for_each_pci_dev(pdev) {
> +               ent = pci_match_id(pci_tbl, pdev);
> +               if (ent)
> +                       goto found;

It's not like I know how PCI abstractions really work, but what happens
here if there would be two instances of the device? It looks like you will
only find the first?

> +       }
> +       /* Device not found. */
> +       goto out;

Can't you just return -ENODEV here? Why jump around...

Now as I said I'm not a PCI expert, but isn't the proper way to do this
to use this stuff:

static struct pci_driver amd_gpio_driver = {
        .name =         "amd-gpio",
        .id_table =     pci_ids,
        .probe =        amd_gpio_probe,
        .remove =       __devexit_p(amd_gpio_remove),
};

static int __init amd_gpio_init(void)
{
        return pci_register_driver(&amd_gpio_driver);
}

static void __exit amd_gpio_exit(void)
{
        pci_unregister_driver(&amd_gpio_driver);
}

module_init(amd_gpio_init);
module_exit(amd_gpio_exit);

Then start working from the probe function insteaf of go and
iterate over the PCI bus yourself? I was thinking there was a
reason for but couldn't find any.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/