Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754745Ab1CQPnj (ORCPT ); Thu, 17 Mar 2011 11:43:39 -0400 Received: from LUNGE.MIT.EDU ([18.54.1.69]:42548 "EHLO lunge.queued.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754644Ab1CQPnh (ORCPT ); Thu, 17 Mar 2011 11:43:37 -0400 Date: Thu, 17 Mar 2011 08:43:28 -0700 From: Andres Salomon To: Ed W Cc: rpurdie@rpsys.net, linux-geode@lists.infradead.org, const@mimas.ru, linux-kernel@vger.kernel.org, Grant Likely Subject: Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface Message-ID: <20110317084328.41831b4c@debxo> In-Reply-To: <4D81D7FD.1040602@wildgooses.com> References: <4D81D7FD.1040602@wildgooses.com> X-Mailer: Claws Mail 3.7.6 (GTK+ 2.20.1; i486-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10417 Lines: 365 On Thu, 17 Mar 2011 09:44:29 +0000 Ed W wrote: > Hi, please be gentle, first kernel patch. I have already posted this > to the > kernel mailing list, but I didn't obviously get any response. It's best to Cc maintainers directly, as many of them (such as myself) don't follow lkml all that closely. > Grateful if someone could please take a look and guide me as to what > adjustments might be required and also the correct person to address > this to? Rather hoping that this might make 2.6.39? Comments below. BTW, Grant is the new GPIO maintainer, so I've cc'd him as well. > > The patch basically simplifies the Alix LED driver by making it use > the leds_gpio instead of driving GPIOs directly. This allows us to > use the normal kernel gpio facilities to access the rest of the board > normally (Alix has a number of user controllable GPIOs) > > I have already mailed the original authors of the old alix led code, > but without a response. > > It's hard to know, before pressing send, if my mail client will mangle > my patch? Copy to the kernel mailing list direct from git is here: > http://marc.info/?l=linux-kernel&m=129943074113312&w=2 > > Thanks for any feedback > > Ed W > > -------- Original Message -------- > Subject: [PATCH] leds: New PCEngines Alix LED driver using > gpio interface Date: Sun, 6 Mar 2011 16:51:25 +0000 > From: kernel@wildgooses.com > To: rpurdie@rpsys.net > CC: const@mimas.ru, daniel@caiaq.de, a.zummo@towertech.it, > linux-kernel@vger.kernel.org, Ed Wildgoose > > > > From: Ed Wildgoose > > This new driver replaces the old PCEngines Alix 2/3 LED driver with > a new driver that controls the LEDs through the leds-gpio driver. > The old driver accessed GPIOs directly, which created a conflict > and prevented also loading the cs5535-gio driver to read other > GPIOs on the Alix board. With this new driver, both are loaded > and therefore it's possible to access both the LEDs and onboard GPIOs > > This driver is based on leds-net5501.c > by: Alessandro Zummo > Additionally it relies on parts of the patch: 7f131cf3ed > by: Daniel Mack to perform detection of the Alix > board > > Signed-off-by: Ed Wildgoose > --- > :100644 100644 6f190f4... 25f0285... M drivers/leds/Kconfig > :100644 100644 f59ffad... 62c8fff... M > drivers/leds/leds-alix2.c drivers/leds/Kconfig | 2 +- > drivers/leds/leds-alix2.c | 196 > ++++++++++---------------------------------- 2 files changed, 46 > insertions(+), 152 deletions(-) > > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 6f190f4..25f0285 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -100,7 +100,7 @@ config LEDS_WRAP > config LEDS_ALIX2 > tristate "LED Support for ALIX.2 and ALIX.3 series" > depends on LEDS_CLASS > - depends on X86 && !GPIO_CS5535 && !CS5535_GPIO > + depends on X86 && LEDS_GPIO_PLATFORM && GPIO_CS5535 > help > This option enables support for the PCEngines ALIX.2 and > ALIX.3 LEDs. You have to set leds-alix2.force=1 for boards with Award > BIOS. diff --git a/drivers/leds/leds-alix2.c > b/drivers/leds/leds-alix2.c index f59ffad..62c8fff 100644 > --- a/drivers/leds/leds-alix2.c > +++ b/drivers/leds/leds-alix2.c > @@ -1,119 +1,66 @@ > /* > * LEDs driver for PCEngines ALIX.2 and ALIX.3 > * > - * Copyright (C) 2008 Constantin Baranov This copyright line should not be removed, so long as parts of the original driver (such as alix_present) remain. > + * Copyright (C) 2011 Nippy Networks Ltd > + * Written by Ed Wildgoose > + * Based on leds-net5501.c by Alessandro Zummo > + * > + * This program is free software; you can redistribute it and/or > modify > + * it under the terms of the GNU General Public License version 2 > + * as published by the Free Software Foundation. > */ > > -#include > -#include > #include > +#include > +#include > +#include > #include > -#include > #include > -#include > -#include > +#include > + > +#include > > static int force = 0; > module_param(force, bool, 0444); > MODULE_PARM_DESC(force, "Assume system has ALIX.2/ALIX.3 style > LEDs"); > -#define MSR_LBAR_GPIO 0x5140000C > -#define CS5535_GPIO_SIZE 256 > - > -static u32 gpio_base; > - > -static struct pci_device_id divil_pci[] = { > - { PCI_DEVICE(PCI_VENDOR_ID_NS, > PCI_DEVICE_ID_NS_CS5535_ISA) }, > - { PCI_DEVICE(PCI_VENDOR_ID_AMD, > PCI_DEVICE_ID_AMD_CS5536_ISA) }, > - { } /* NULL entry */ > -}; > -MODULE_DEVICE_TABLE(pci, divil_pci); > - > -struct alix_led { > - struct led_classdev cdev; > - unsigned short port; > - unsigned int on_value; > - unsigned int off_value; > -}; > - > -static void alix_led_set(struct led_classdev *led_cdev, > - enum led_brightness brightness) > -{ > - struct alix_led *led_dev = > - container_of(led_cdev, struct alix_led, cdev); > - > - if (brightness) > - outl(led_dev->on_value, gpio_base + led_dev->port); > - else > - outl(led_dev->off_value, gpio_base + led_dev->port); > -} > - > -static struct alix_led alix_leds[] = { > +static struct gpio_led alix_leds[] = { > { > - .cdev = { > - .name = "alix:1", > - .brightness_set = alix_led_set, > - }, > - .port = 0x00, > - .on_value = 1 << 22, > - .off_value = 1 << 6, > + .name = "alix:1", > + .gpio = 6, > + .default_trigger = "default-on", > + .active_low = 1, > }, > { > - .cdev = { > - .name = "alix:2", > - .brightness_set = alix_led_set, > - }, > - .port = 0x80, > - .on_value = 1 << 25, > - .off_value = 1 << 9, > + .name = "alix:2", > + .gpio = 25, > + .default_trigger = "default-off", > + .active_low = 1, > }, > { > - .cdev = { > - .name = "alix:3", > - .brightness_set = alix_led_set, > - }, > - .port = 0x80, > - .on_value = 1 << 27, > - .off_value = 1 << 11, > + .name = "alix:3", > + .gpio = 27, > + .default_trigger = "default-off", > + .active_low = 1, > }, > }; > > -static int __init alix_led_probe(struct platform_device *pdev) > -{ > - int i; > - int ret; > - > - for (i = 0; i < ARRAY_SIZE(alix_leds); i++) { > - alix_leds[i].cdev.flags |= LED_CORE_SUSPENDRESUME; > - ret = led_classdev_register(&pdev->dev, > &alix_leds[i].cdev); > - if (ret < 0) > - goto fail; > - } > - return 0; > +static struct gpio_led_platform_data alix_leds_data = { > + .num_leds = ARRAY_SIZE(alix_leds), > + .leds = alix_leds, > +}; > > -fail: > - while (--i >= 0) > - led_classdev_unregister(&alix_leds[i].cdev); > - return ret; > -} > +static struct platform_device alix_leds_dev = { > + .name = "leds-gpio", This should probably be more unique; something like "leds-alix2". > + .id = -1, > + .dev.platform_data = &alix_leds_data, > +}; > > -static int alix_led_remove(struct platform_device *pdev) > +static void __init register_alix(void) > { > - int i; > - > - for (i = 0; i < ARRAY_SIZE(alix_leds); i++) > - led_classdev_unregister(&alix_leds[i].cdev); > - return 0; > + platform_device_register(&alix_leds_dev); > } > > -static struct platform_driver alix_led_driver = { > - .remove = alix_led_remove, > - .driver = { > - .name = KBUILD_MODNAME, > - .owner = THIS_MODULE, > - }, > -}; > - > static int __init alix_present(unsigned long bios_phys, > const char *alix_sig, > size_t alix_sig_len) > @@ -164,76 +111,23 @@ static int __init alix_present(unsigned long > bios_phys, return 0; > } > > -static struct platform_device *pdev; > - > -static int __init alix_pci_led_init(void) > -{ > - u32 low, hi; > - > - if (pci_dev_present(divil_pci) == 0) { > - printk(KERN_WARNING KBUILD_MODNAME": DIVIL not > found\n"); > - return -ENODEV; > - } > - > - /* Grab the GPIO I/O range */ > - rdmsr(MSR_LBAR_GPIO, low, hi); > - > - /* Check the mask and whether GPIO is enabled (sanity check) > */ > - if (hi != 0x0000f001) { > - printk(KERN_WARNING KBUILD_MODNAME": GPIO not > enabled\n"); > - return -ENODEV; > - } > - > - /* Mask off the IO base address */ > - gpio_base = low & 0x0000ff00; > - > - if (!request_region(gpio_base, CS5535_GPIO_SIZE, > KBUILD_MODNAME)) { > - printk(KERN_ERR KBUILD_MODNAME": can't allocate I/O > for GPIO\n"); > - return -ENODEV; > - } > - > - /* Set GPIO function to output */ > - outl(1 << 6, gpio_base + 0x04); > - outl(1 << 9, gpio_base + 0x84); > - outl(1 << 11, gpio_base + 0x84); > - > - return 0; > -} > - > -static int __init alix_led_init(void) > +static int __init alix_init(void) > { > - int ret = -ENODEV; > const char tinybios_sig[] = "PC Engines ALIX."; > const char coreboot_sig[] = "PC Engines\0ALIX."; > > + if (!is_geode()) > + return 0; > + Presumably you want to return -ENODEV here, especially since your driver has no method for unloading. > if (alix_present(0xf0000, tinybios_sig, sizeof(tinybios_sig) > - 1) || alix_present(0x500, coreboot_sig, sizeof(coreboot_sig) - 1)) > - ret = alix_pci_led_init(); > + register_alix(); Ditto. > > - if (ret < 0) > - return ret; > - > - pdev = platform_device_register_simple(KBUILD_MODNAME, -1, > NULL, 0); > - if (!IS_ERR(pdev)) { > - ret = platform_driver_probe(&alix_led_driver, > alix_led_probe); > - if (ret) > - platform_device_unregister(pdev); > - } else > - ret = PTR_ERR(pdev); > - > - return ret; > -} > - > -static void __exit alix_led_exit(void) > -{ > - platform_device_unregister(pdev); > - platform_driver_unregister(&alix_led_driver); > - release_region(gpio_base, CS5535_GPIO_SIZE); > + return 0; > } > > -module_init(alix_led_init); > -module_exit(alix_led_exit); > +arch_initcall(alix_init); Why is this arch_initcall rather than module_init? If possible, it would be good to have an unload hook as well. > > -MODULE_AUTHOR("Constantin Baranov "); > +MODULE_AUTHOR("Ed Wildgoose "); > MODULE_DESCRIPTION("PCEngines ALIX.2 and ALIX.3 LED driver"); > MODULE_LICENSE("GPL"); -- 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/