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

[Linus Walleij]

On Mon, May 28, 2012 at 2:48 PM, Dmitry Eremin-Solenikov
<dbaryshkov@xxxxxxxxx> 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@xxxxxxxxx>
> ---
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/