Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754841Ab2E1QTj (ORCPT ); Mon, 28 May 2012 12:19:39 -0400 Received: from mail-qc0-f174.google.com ([209.85.216.174]:43223 "EHLO mail-qc0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753855Ab2E1QTh convert rfc822-to-8bit (ORCPT ); Mon, 28 May 2012 12:19:37 -0400 MIME-Version: 1.0 In-Reply-To: <1338187691-32616-1-git-send-email-dbaryshkov@gmail.com> References: <1338187691-32616-1-git-send-email-dbaryshkov@gmail.com> Date: Tue, 29 May 2012 00:19:36 +0800 Message-ID: Subject: Re: [PATCH][V2] gpio: add a driver for GPIO pins found on AMD-8111 south bridge chips From: Linus Walleij To: Dmitry Eremin-Solenikov Cc: Linus Walleij , Grant Likely , linux-kernel@vger.kernel.org Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2131 Lines: 74 On Mon, May 28, 2012 at 2:48 PM, Dmitry Eremin-Solenikov 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 > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/