* Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface [not found] <4D81D7FD.1040602@wildgooses.com> @ 2011-03-17 15:43 ` Andres Salomon 2011-03-17 16:08 ` Grant Likely 2011-03-17 17:04 ` Ed W 0 siblings, 2 replies; 18+ messages in thread From: Andres Salomon @ 2011-03-17 15:43 UTC (permalink / raw) To: Ed W; +Cc: rpurdie, linux-geode, const, linux-kernel, Grant Likely On Thu, 17 Mar 2011 09:44:29 +0000 Ed W <lists@wildgooses.com> 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 <kernel@wildgooses.com> > > > > From: Ed Wildgoose <kernel@wildgooses.com> > > 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 <a.zummo@towertech.it> > Additionally it relies on parts of the patch: 7f131cf3ed > by: Daniel Mack <daniel@caiaq.de> to perform detection of the Alix > board > > Signed-off-by: Ed Wildgoose <kernel@wildgooses.com> > --- > :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 <const@mimas.ru> 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 <kernel@wildgooses.com> > + * Based on leds-net5501.c by Alessandro Zummo <a.zummo@towertech.it> > + * > + * 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 <linux/err.h> > -#include <linux/io.h> > #include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/string.h> > #include <linux/leds.h> > -#include <linux/module.h> > #include <linux/platform_device.h> > -#include <linux/string.h> > -#include <linux/pci.h> > +#include <linux/gpio.h> > + > +#include <asm/geode.h> > > 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 <const@mimas.ru>"); > +MODULE_AUTHOR("Ed Wildgoose <kernel@wildgooses.com>"); > MODULE_DESCRIPTION("PCEngines ALIX.2 and ALIX.3 LED driver"); > MODULE_LICENSE("GPL"); ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-17 15:43 ` Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface Andres Salomon @ 2011-03-17 16:08 ` Grant Likely 2011-03-17 17:24 ` Ed W 2011-03-17 17:04 ` Ed W 1 sibling, 1 reply; 18+ messages in thread From: Grant Likely @ 2011-03-17 16:08 UTC (permalink / raw) To: Andres Salomon; +Cc: Ed W, rpurdie, linux-geode, const, linux-kernel On Thu, Mar 17, 2011 at 9:43 AM, Andres Salomon <dilinger@queued.net> wrote: > On Thu, 17 Mar 2011 09:44:29 +0000 > Ed W <lists@wildgooses.com> 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. Thanks Andres. Hi Ed. Thanks for digging in to clean up this driver. It's considered very bad form for a driver to both register a device and then register the driver that binds against it, so this cleanup is most welcome. Actually, it looks like with your changes this isn't even a driver anymore. It is merely code to register a device on a specific platform. Is there any other alix-specific initialization code in the kernel? If so, you should consider relocating the device registration with the rest of the alix setup code. More comments below... > >> >> 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 <kernel@wildgooses.com> >> >> >> >> From: Ed Wildgoose <kernel@wildgooses.com> >> >> 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 <a.zummo@towertech.it> >> Additionally it relies on parts of the patch: 7f131cf3ed >> by: Daniel Mack <daniel@caiaq.de> to perform detection of the Alix >> board >> >> Signed-off-by: Ed Wildgoose <kernel@wildgooses.com> >> --- >> :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 <const@mimas.ru> > > 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 <kernel@wildgooses.com> >> + * Based on leds-net5501.c by Alessandro Zummo <a.zummo@towertech.it> >> + * >> + * 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 <linux/err.h> >> -#include <linux/io.h> >> #include <linux/kernel.h> >> +#include <linux/init.h> >> +#include <linux/io.h> >> +#include <linux/string.h> >> #include <linux/leds.h> >> -#include <linux/module.h> >> #include <linux/platform_device.h> >> -#include <linux/string.h> >> -#include <linux/pci.h> >> +#include <linux/gpio.h> >> + >> +#include <asm/geode.h> >> >> 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". He is pretty much required to use "leds-gpio" so it will get bound to the leds-gpio driver. > >> + .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. It looks to me that this isn't actually a driver anymore. It is merely a hunk of code that registers a platform_device. Return 0 should be just fine. > > >> 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. Yes, unless you've got specific ordering constraints this should definitely be module_init(). g. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-17 16:08 ` Grant Likely @ 2011-03-17 17:24 ` Ed W 2011-03-17 17:52 ` Andres Salomon 2011-03-17 18:12 ` Grant Likely 0 siblings, 2 replies; 18+ messages in thread From: Ed W @ 2011-03-17 17:24 UTC (permalink / raw) To: Grant Likely; +Cc: Andres Salomon, rpurdie, linux-geode, const, linux-kernel On 17/03/2011 16:08, Grant Likely wrote: > Actually, it looks like with your changes this isn't even a driver > anymore. It is merely code to register a device on a specific > platform. Is there any other alix-specific initialization code in the > kernel? If so, you should consider relocating the device registration > with the rest of the alix setup code. Agreed. I confess that I don't understand the linux driver structure enough to shift the code further though What I observe is that there is a lot of arch specific setup for ARM, etc, however, this is not currently done at all for x86 (which is Alix), so at the moment this would seem to sit slightly awkwardly with current x86 arch code? Instead I found leds-net5501.c, which is for a very similar platform to the Alix (not quite similar enough that I could combine the files) and I used that as my prototype for this driver. I think given that we already have a similar driver in the leds area which does platform alike setup, this gives some justification for doing the same with the Alix leds? Additionally if we ever find we need Alix specific setup code then the code is ready to be used as is by the platform code? >>> -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. > > Yes, unless you've got specific ordering constraints this should > definitely be module_init(). I'm out of my depth here. I would be very happy to resubmit either way? However, is there not a potential ordering issue if leds-alix2 is loaded *before* leds-gpio? Is this not the reason for making it an arch_initcall? Also the same code is used in leds-5501.c - would you like me to submit a patch to change that also (if you confirm it should become a module_init call?). Thanks for final confirmation on this and I will quickly resubmit the patch? Ed W ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-17 17:24 ` Ed W @ 2011-03-17 17:52 ` Andres Salomon 2011-03-17 17:59 ` Ed W 2011-03-17 18:12 ` Grant Likely 1 sibling, 1 reply; 18+ messages in thread From: Andres Salomon @ 2011-03-17 17:52 UTC (permalink / raw) To: Ed W; +Cc: Grant Likely, rpurdie, linux-geode, const, linux-kernel On Thu, 17 Mar 2011 17:24:55 +0000 Ed W <lists@wildgooses.com> wrote: > On 17/03/2011 16:08, Grant Likely wrote: > > Actually, it looks like with your changes this isn't even a driver > > anymore. It is merely code to register a device on a specific > > platform. Is there any other alix-specific initialization code in > > the kernel? If so, you should consider relocating the device > > registration with the rest of the alix setup code. > > Agreed. I confess that I don't understand the linux driver structure > enough to shift the code further though > > What I observe is that there is a lot of arch specific setup for ARM, > etc, however, this is not currently done at all for x86 (which is > Alix), so at the moment this would seem to sit slightly awkwardly > with current x86 arch code? > > Instead I found leds-net5501.c, which is for a very similar platform > to the Alix (not quite similar enough that I could combine the files) > and I used that as my prototype for this driver. > OLPC stuff lives in arch/x86/platform/olpc; if there was more alix-specific stuff, I'd suggest moving it into something similar. However, I didn't find any. Maybe an arch/x86/platform/geode as a place to collect platform drivers for the various geode-based machines out there (alix, soekris, etc)? Though honestly, I'm not that interested in doing the work to migrate stuff over to there. > I think given that we already have a similar driver in the leds area > which does platform alike setup, this gives some justification for > doing the same with the Alix leds? Additionally if we ever find we > need Alix specific setup code then the code is ready to be used as is > by the platform code? > > > >>> -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. > > > > Yes, unless you've got specific ordering constraints this should > > definitely be module_init(). > > I'm out of my depth here. I would be very happy to resubmit either > way? > > However, is there not a potential ordering issue if leds-alix2 is > loaded *before* leds-gpio? Is this not the reason for making it an > arch_initcall? > > Also the same code is used in leds-5501.c - would you like me to > submit a patch to change that also (if you confirm it should become a > module_init call?). Yes, it should be module_init. There shouldn't be any issues with leds-gpio; the driver will only bind once a device is added (so long as nothing else named leds-gpio comes along before leds-alix2). > > Thanks for final confirmation on this and I will quickly resubmit the > patch? > > Ed W ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-17 17:52 ` Andres Salomon @ 2011-03-17 17:59 ` Ed W 2011-03-17 18:17 ` Grant Likely 2011-03-17 18:22 ` Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface Andres Salomon 0 siblings, 2 replies; 18+ messages in thread From: Ed W @ 2011-03-17 17:59 UTC (permalink / raw) To: Andres Salomon; +Cc: Grant Likely, rpurdie, linux-geode, const, linux-kernel On 17/03/2011 17:52, Andres Salomon wrote: > OLPC stuff lives in arch/x86/platform/olpc; if there was more > alix-specific stuff, I'd suggest moving it into something similar. > However, I didn't find any. Maybe an arch/x86/platform/geode as a > place to collect platform drivers for the various geode-based machines > out there (alix, soekris, etc)? Though honestly, I'm not that > interested in doing the work to migrate stuff over to there. As this is my first stab at this, I think you are agreeing that the current change is ok as a first step at least? (I can have a go at a larger code re-org later...) Can I get your final opinion on what should be in the code Copyright statement before I resubmit with the module_init change? To recap I don't want to alienate anyone, but the code is now based on a) leds-5501 and b) a patch from someone not currently credited in the file copyright statement? I have stated this origin in the commit log and noted the derivation in the code itself. What to do..? Thanks Ed W ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-17 17:59 ` Ed W @ 2011-03-17 18:17 ` Grant Likely 2011-03-18 18:12 ` kernel 2011-03-17 18:22 ` Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface Andres Salomon 1 sibling, 1 reply; 18+ messages in thread From: Grant Likely @ 2011-03-17 18:17 UTC (permalink / raw) To: Ed W; +Cc: Andres Salomon, rpurdie, linux-geode, const, linux-kernel On Thu, Mar 17, 2011 at 05:59:06PM +0000, Ed W wrote: > On 17/03/2011 17:52, Andres Salomon wrote: > > OLPC stuff lives in arch/x86/platform/olpc; if there was more > > alix-specific stuff, I'd suggest moving it into something similar. > > However, I didn't find any. Maybe an arch/x86/platform/geode as a > > place to collect platform drivers for the various geode-based machines > > out there (alix, soekris, etc)? Though honestly, I'm not that > > interested in doing the work to migrate stuff over to there. > > > As this is my first stab at this, I think you are agreeing that the > current change is ok as a first step at least? (I can have a go at a > larger code re-org later...) It is actually really easy. You just need to move the file into arch/x86/platform/geode, add a Makefile, and add a line to arch/x86/platform/Makefile to include it. In this particular case, since so much code is sliced out, the patch will actually be easier to review if the file is moved at the same time. g. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-17 18:17 ` Grant Likely @ 2011-03-18 18:12 ` kernel 2011-03-18 18:32 ` Ed W 0 siblings, 1 reply; 18+ messages in thread From: kernel @ 2011-03-18 18:12 UTC (permalink / raw) To: lists, grant.likely, dilinger; +Cc: linux-geode, linux-kernel From: Ed Wildgoose <git@wildgooses.com> 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-gpio driver to read other GPIOs on the Alix board. With this new driver, we hook into leds-gpio which in turn uses GPIO to control the LEDs and therefore it's possible to control both the LEDs and access onboard GPIOs Driver is moved to platform/geode and any other geode initialisation modules should move here also. This driver is inspired by leds-net5501.c by: Alessandro Zummo <a.zummo@towertech.it> Ideally, leds-net5501.c should also be moved to platform/geode. Additionally the driver relies on parts of the patch: 7f131cf3ed by: Daniel Mack <daniel@caiaq.de> to perform detection of the Alix board Signed-off-by: Ed Wildgoose <git@wildgooses.com> --- :100644 100644 d5ed94d... b16ab56... M arch/x86/Kconfig :100644 100644 021eee9... 8d87439... M arch/x86/platform/Makefile :000000 100644 0000000... d4e599f... A arch/x86/platform/geode/Makefile :000000 100644 0000000... bc1b3b3... A arch/x86/platform/geode/alix_leds.c :100644 100644 6f190f4... e4573d6... M drivers/leds/Kconfig :100644 100644 aae6989... bf5a00c... M drivers/leds/Makefile :100644 000000 f59ffad... 0000000... D drivers/leds/leds-alix2.c arch/x86/Kconfig | 9 ++ arch/x86/platform/Makefile | 1 + arch/x86/platform/geode/Makefile | 1 + arch/x86/platform/geode/alix_leds.c | 138 ++++++++++++++++++++ drivers/leds/Kconfig | 8 -- drivers/leds/Makefile | 1 - drivers/leds/leds-alix2.c | 239 ----------------------------------- 7 files changed, 149 insertions(+), 248 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index d5ed94d..b16ab56 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -2094,6 +2094,15 @@ config OLPC_OPENFIRMWARE_DT default y if OLPC_OPENFIRMWARE && PROC_DEVICETREE select OF_PROMTREE +config ALIX_LEDS + bool "PCEngines ALIX.2/.3 LED Support" + select GPIOLIB + depends on LEDS_CLASS + depends on LEDS_GPIO_PLATFORM && GPIO_CS5535 + ---help--- + This option enables support for the PCEngines ALIX.2 and ALIX.3 LEDs. + You have to set alix-leds.force=1 for boards with Award BIOS. + endif # X86_32 config AMD_NB diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile index 021eee9..8d87439 100644 --- a/arch/x86/platform/Makefile +++ b/arch/x86/platform/Makefile @@ -1,6 +1,7 @@ # Platform specific code goes here obj-y += ce4100/ obj-y += efi/ +obj-y += geode/ obj-y += iris/ obj-y += mrst/ obj-y += olpc/ diff --git a/arch/x86/platform/geode/Makefile b/arch/x86/platform/geode/Makefile new file mode 100644 index 0000000..d4e599f --- /dev/null +++ b/arch/x86/platform/geode/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_ALIX_LEDS) += alix-leds.o diff --git a/arch/x86/platform/geode/alix_leds.c b/arch/x86/platform/geode/alix_leds.c new file mode 100644 index 0000000..bc1b3b3 --- /dev/null +++ b/arch/x86/platform/geode/alix_leds.c @@ -0,0 +1,138 @@ +/* + * Configure GPIO control of LEDs on Alix.2/3 from PCEngines + * + * Copyright (C) 2008 Constantin Baranov <const@mimas.ru> + * Copyright (C) 2011 Nippy Networks Ltd + * + * TODO: There are large similarities with leds-net5501.c + * by Alessandro Zummo <a.zummo@towertech.it> + * In the future leds-net5501.c should be migrated over to platform + * + * 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 <linux/kernel.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/string.h> +#include <linux/leds.h> +#include <linux/platform_device.h> +#include <linux/gpio.h> + +#include <asm/geode.h> + +static int force = 0; +module_param(force, bool, 0444); +/* FIXME: Award bios is not automatically detected as Alix platform */ +MODULE_PARM_DESC(force, "Force detection as ALIX.2/ALIX.3 platform"); + +static struct gpio_led alix_leds[] = { + { + .name = "alix:1", + .gpio = 6, + .default_trigger = "default-on", + .active_low = 1, + }, + { + .name = "alix:2", + .gpio = 25, + .default_trigger = "default-off", + .active_low = 1, + }, + { + .name = "alix:3", + .gpio = 27, + .default_trigger = "default-off", + .active_low = 1, + }, +}; + +static struct gpio_led_platform_data alix_leds_data = { + .num_leds = ARRAY_SIZE(alix_leds), + .leds = alix_leds, +}; + +static struct platform_device alix_leds_dev = { + .name = "leds-gpio", + .id = -1, + .dev.platform_data = &alix_leds_data, +}; + +static void __init register_alix(void) +{ + /* Setup LED control through leds-gpio driver */ + platform_device_register(&alix_leds_dev); +} + +static int __init alix_present(unsigned long bios_phys, + const char *alix_sig, + size_t alix_sig_len) +{ + const size_t bios_len = 0x00010000; + const char *bios_virt; + const char *scan_end; + const char *p; + char name[64]; + + if (force) { + printk(KERN_NOTICE "%s: forced to skip BIOS test, " + "assume system is ALIX.2/ALIX.3\n", + KBUILD_MODNAME); + return 1; + } + + bios_virt = phys_to_virt(bios_phys); + scan_end = bios_virt + bios_len - (alix_sig_len + 2); + for (p = bios_virt; p < scan_end; p++) { + const char *tail; + char *a; + + if (memcmp(p, alix_sig, alix_sig_len) != 0) + continue; + + memcpy(name, p, sizeof(name)); + + /* remove the first \0 character from string */ + a = strchr(name, '\0'); + if (a) + *a = ' '; + + /* cut the string at a newline */ + a = strchr(name, '\r'); + if (a) + *a = '\0'; + + tail = p + alix_sig_len; + if ((tail[0] == '2' || tail[0] == '3')) { + printk(KERN_INFO + "%s: system is recognized as \"%s\"\n", + KBUILD_MODNAME, name); + return 1; + } + } + + return 0; +} + +static int __init alix_init(void) +{ + const char tinybios_sig[] = "PC Engines ALIX."; + const char coreboot_sig[] = "PC Engines\0ALIX."; + + if (!is_geode()) + return 0; + + if (alix_present(0xf0000, tinybios_sig, sizeof(tinybios_sig) - 1) || + alix_present(0x500, coreboot_sig, sizeof(coreboot_sig) - 1)) + register_alix(); + + return 0; +} + +module_init(alix_init); + +MODULE_AUTHOR("Ed Wildgoose <kernel@wildgooses.com>"); +MODULE_DESCRIPTION("PCEngines ALIX.2 and ALIX.3 System Setup"); +MODULE_LICENSE("GPL"); diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 6f190f4..e4573d6 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -97,14 +97,6 @@ config LEDS_WRAP help This option enables support for the PCEngines WRAP programmable LEDs. -config LEDS_ALIX2 - tristate "LED Support for ALIX.2 and ALIX.3 series" - depends on LEDS_CLASS - depends on X86 && !GPIO_CS5535 && !CS5535_GPIO - 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. - config LEDS_H1940 tristate "LED Support for iPAQ H1940 device" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index aae6989..bf5a00c 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -15,7 +15,6 @@ obj-$(CONFIG_LEDS_AMS_DELTA) += leds-ams-delta.o obj-$(CONFIG_LEDS_NET48XX) += leds-net48xx.o obj-$(CONFIG_LEDS_NET5501) += leds-net5501.o obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o -obj-$(CONFIG_LEDS_ALIX2) += leds-alix2.o obj-$(CONFIG_LEDS_H1940) += leds-h1940.o obj-$(CONFIG_LEDS_COBALT_QUBE) += leds-cobalt-qube.o obj-$(CONFIG_LEDS_COBALT_RAQ) += leds-cobalt-raq.o diff --git a/drivers/leds/leds-alix2.c b/drivers/leds/leds-alix2.c deleted file mode 100644 index f59ffad..0000000 --- a/drivers/leds/leds-alix2.c +++ /dev/null @@ -1,239 +0,0 @@ -/* - * LEDs driver for PCEngines ALIX.2 and ALIX.3 - * - * Copyright (C) 2008 Constantin Baranov <const@mimas.ru> - */ - -#include <linux/err.h> -#include <linux/io.h> -#include <linux/kernel.h> -#include <linux/leds.h> -#include <linux/module.h> -#include <linux/platform_device.h> -#include <linux/string.h> -#include <linux/pci.h> - -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[] = { - { - .cdev = { - .name = "alix:1", - .brightness_set = alix_led_set, - }, - .port = 0x00, - .on_value = 1 << 22, - .off_value = 1 << 6, - }, - { - .cdev = { - .name = "alix:2", - .brightness_set = alix_led_set, - }, - .port = 0x80, - .on_value = 1 << 25, - .off_value = 1 << 9, - }, - { - .cdev = { - .name = "alix:3", - .brightness_set = alix_led_set, - }, - .port = 0x80, - .on_value = 1 << 27, - .off_value = 1 << 11, - }, -}; - -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; - -fail: - while (--i >= 0) - led_classdev_unregister(&alix_leds[i].cdev); - return ret; -} - -static int alix_led_remove(struct platform_device *pdev) -{ - int i; - - for (i = 0; i < ARRAY_SIZE(alix_leds); i++) - led_classdev_unregister(&alix_leds[i].cdev); - return 0; -} - -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) -{ - const size_t bios_len = 0x00010000; - const char *bios_virt; - const char *scan_end; - const char *p; - char name[64]; - - if (force) { - printk(KERN_NOTICE "%s: forced to skip BIOS test, " - "assume system has ALIX.2 style LEDs\n", - KBUILD_MODNAME); - return 1; - } - - bios_virt = phys_to_virt(bios_phys); - scan_end = bios_virt + bios_len - (alix_sig_len + 2); - for (p = bios_virt; p < scan_end; p++) { - const char *tail; - char *a; - - if (memcmp(p, alix_sig, alix_sig_len) != 0) - continue; - - memcpy(name, p, sizeof(name)); - - /* remove the first \0 character from string */ - a = strchr(name, '\0'); - if (a) - *a = ' '; - - /* cut the string at a newline */ - a = strchr(name, '\r'); - if (a) - *a = '\0'; - - tail = p + alix_sig_len; - if ((tail[0] == '2' || tail[0] == '3')) { - printk(KERN_INFO - "%s: system is recognized as \"%s\"\n", - KBUILD_MODNAME, name); - return 1; - } - } - - 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) -{ - int ret = -ENODEV; - const char tinybios_sig[] = "PC Engines ALIX."; - const char coreboot_sig[] = "PC Engines\0ALIX."; - - if (alix_present(0xf0000, tinybios_sig, sizeof(tinybios_sig) - 1) || - alix_present(0x500, coreboot_sig, sizeof(coreboot_sig) - 1)) - ret = alix_pci_led_init(); - - 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); -} - -module_init(alix_led_init); -module_exit(alix_led_exit); - -MODULE_AUTHOR("Constantin Baranov <const@mimas.ru>"); -MODULE_DESCRIPTION("PCEngines ALIX.2 and ALIX.3 LED driver"); -MODULE_LICENSE("GPL"); -- 1.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-18 18:12 ` kernel @ 2011-03-18 18:32 ` Ed W 2011-03-18 22:48 ` Grant Likely 0 siblings, 1 reply; 18+ messages in thread From: Ed W @ 2011-03-18 18:32 UTC (permalink / raw) To: grant.likely, dilinger; +Cc: linux-geode, linux-kernel Hi, Thanks for "mentoring" me over this. I appreciate that this consumes your valuable time I have reposted what I hope is close to the correct code for this new driver. I'm somewhat apprehensive, so to draw attention to the things I have most likely "done wrong": > :000000 100644 0000000... bc1b3b3... A arch/x86/platform/geode/alix_leds.c ... > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index d5ed94d..b16ab56 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -2094,6 +2094,15 @@ config OLPC_OPENFIRMWARE_DT > default y if OLPC_OPENFIRMWARE && PROC_DEVICETREE > select OF_PROMTREE > > +config ALIX_LEDS > + bool "PCEngines ALIX.2/.3 LED Support" > + select GPIOLIB > + depends on LEDS_CLASS > + depends on LEDS_GPIO_PLATFORM && GPIO_CS5535 > + ---help--- > + This option enables support for the PCEngines ALIX.2 and ALIX.3 LEDs. > + You have to set alix-leds.force=1 for boards with Award BIOS. > + > endif # X86_32 Driver and KConfig show this being specifically a driver for the Alix LEDs rather than being a general Alix plaform initialisation module? I was unsure if the trend was to have one module which initialised all Alix platform stuff (whatever it needs), or to split by function? Looking at other platform modules they seem to be somewhat fine grained so I went with a specific "ALIX Led Module" approach? Additionally I am unsure how strictly to set dependencies for my module? It clearly requires all the LED_GPIO_PLATFORM, GPIO_CS5535 dependencies to do anything, but equally it doesn't seem to *break* anything if those dependencies aren't compiled in? Listing all the dependencies seems to make it hard for users to find the option to enable it since it's not even listed in menuconfig until your deps are met? Please correct me as to what level of deps should be listed? On the topic of dependencies, Andres has changed cs5535-gpio.c to depend on a new module cs5535-mfd - however, apart from the commit log message this is not obvious to see? OK, I'm an idiot, but it took me most of this afternoon to understand why my GPIOs stopped working after moving to 2.6.38? Q: Should the new -mfd module be listed as a dep of -gpio? Or perhaps -gpio should "select" -mfd? At least it would be helpful to list the dependency in the Kconfig message? Happy to submit a change - please advise on what (if anything) would be the best solution? Many thanks for your help and guidance so far Ed W ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-18 18:32 ` Ed W @ 2011-03-18 22:48 ` Grant Likely 2011-03-19 16:51 ` [PATCH] leds: New PCEngines Alix system driver (enables LEDs via gpio interface) kernel 2011-03-19 17:46 ` [PATCH] gpio: Show explicit dependency between GPIO_CS5535 and MFD_CS5535 kernel 0 siblings, 2 replies; 18+ messages in thread From: Grant Likely @ 2011-03-18 22:48 UTC (permalink / raw) To: Ed W; +Cc: dilinger, linux-geode, linux-kernel On Fri, Mar 18, 2011 at 12:32 PM, Ed W <lists@wildgooses.com> wrote: > Hi, Thanks for "mentoring" me over this. I appreciate that this > consumes your valuable time > > I have reposted what I hope is close to the correct code for this new > driver. I'm somewhat apprehensive, so to draw attention to the things I > have most likely "done wrong": > > >> :000000 100644 0000000... bc1b3b3... A arch/x86/platform/geode/alix_leds.c > > ... > >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig >> index d5ed94d..b16ab56 100644 >> --- a/arch/x86/Kconfig >> +++ b/arch/x86/Kconfig >> @@ -2094,6 +2094,15 @@ config OLPC_OPENFIRMWARE_DT >> default y if OLPC_OPENFIRMWARE && PROC_DEVICETREE >> select OF_PROMTREE >> >> +config ALIX_LEDS >> + bool "PCEngines ALIX.2/.3 LED Support" >> + select GPIOLIB >> + depends on LEDS_CLASS >> + depends on LEDS_GPIO_PLATFORM && GPIO_CS5535 >> + ---help--- >> + This option enables support for the PCEngines ALIX.2 and ALIX.3 LEDs. >> + You have to set alix-leds.force=1 for boards with Award BIOS. >> + >> endif # X86_32 > > > Driver and KConfig show this being specifically a driver for the Alix > LEDs rather than being a general Alix plaform initialisation module? > > I was unsure if the trend was to have one module which initialised all > Alix platform stuff (whatever it needs), or to split by function? > Looking at other platform modules they seem to be somewhat fine grained > so I went with a specific "ALIX Led Module" approach? That's very much up to the board maintainer. Because it is just device registrations, I would lump them all into one file. If they were actual drivers then I tend to split them up. > Additionally I am unsure how strictly to set dependencies for my module? > It clearly requires all the LED_GPIO_PLATFORM, GPIO_CS5535 dependencies > to do anything, but equally it doesn't seem to *break* anything if those > dependencies aren't compiled in? Listing all the dependencies seems to > make it hard for users to find the option to enable it since it's not > even listed in menuconfig until your deps are met? Please correct me as > to what level of deps should be listed? Correct, you don't need to depend on the driver because this is just a device registration. Who cares if it never gets bound to the driver? The device registration will happily sit in the device model unregistered. > On the topic of dependencies, Andres has changed cs5535-gpio.c to depend > on a new module cs5535-mfd - however, apart from the commit log message > this is not obvious to see? OK, I'm an idiot, but it took me most of > this afternoon to understand why my GPIOs stopped working after moving > to 2.6.38? Q: Should the new -mfd module be listed as a dep of -gpio? > Or perhaps -gpio should "select" -mfd? At least it would be helpful to > list the dependency in the Kconfig message? select is a little touchy because selecting another symbol bypasses and dependences the other symbol has. In this case, it could skip CONFIG_MFD_CORE which may be bad. -gpio should be a dependancy of -mfd in this case I think. g. ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] leds: New PCEngines Alix system driver (enables LEDs via gpio interface) 2011-03-18 22:48 ` Grant Likely @ 2011-03-19 16:51 ` kernel 2011-03-19 17:21 ` Ed W 2011-03-24 3:52 ` Grant Likely 2011-03-19 17:46 ` [PATCH] gpio: Show explicit dependency between GPIO_CS5535 and MFD_CS5535 kernel 1 sibling, 2 replies; 18+ messages in thread From: kernel @ 2011-03-19 16:51 UTC (permalink / raw) To: lists, grant.likely, dilinger; +Cc: linux-geode, linux-kernel From: Ed Wildgoose <git@wildgooses.com> 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-gpio driver to read other GPIOs on the Alix board. With this new driver, we hook into leds-gpio which in turn uses GPIO to control the LEDs and therefore it's possible to control both the LEDs and access onboard GPIOs Driver is moved to platform/geode and any other geode initialisation modules should move here also. This driver is inspired by leds-net5501.c by: Alessandro Zummo <a.zummo@towertech.it> Ideally, leds-net5501.c should also be moved to platform/geode. Additionally the driver relies on parts of the patch: 7f131cf3ed by: Daniel Mack <daniel@caiaq.de> to perform detection of the Alix board Signed-off-by: Ed Wildgoose <kernel@wildgooses.com> --- :100644 100644 d5ed94d... a10556d... M arch/x86/Kconfig :100644 100644 021eee9... 8d87439... M arch/x86/platform/Makefile :000000 100644 0000000... 07c9cd0... A arch/x86/platform/geode/Makefile :000000 100644 0000000... f3f01e2... A arch/x86/platform/geode/alix.c :100644 100644 6f190f4... e4573d6... M drivers/leds/Kconfig :100644 100644 aae6989... bf5a00c... M drivers/leds/Makefile :100644 000000 f59ffad... 0000000... D drivers/leds/leds-alix2.c arch/x86/Kconfig | 14 +++ arch/x86/platform/Makefile | 1 + arch/x86/platform/geode/Makefile | 1 + arch/x86/platform/geode/alix.c | 141 ++++++++++++++++++++++ drivers/leds/Kconfig | 8 -- drivers/leds/Makefile | 1 - drivers/leds/leds-alix2.c | 239 -------------------------------------- 7 files changed, 157 insertions(+), 248 deletions(-) diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index d5ed94d..a10556d 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -2094,6 +2094,20 @@ config OLPC_OPENFIRMWARE_DT default y if OLPC_OPENFIRMWARE && PROC_DEVICETREE select OF_PROMTREE +config ALIX + bool "PCEngines ALIX System Support (LED setup)" + select GPIOLIB + ---help--- + This option enables system support for the PCEngines ALIX. + At present this just sets up LEDs for GPIO control on + ALIX2/3/6 boards. However, other system specific setup should + get added here. + + Note: You must still enable the drivers for GPIO and LED support + (GPIO_CS5535 & LEDS_GPIO) to actually use the LEDs + + Note: You have to set alix.force=1 for boards with Award BIOS. + endif # X86_32 config AMD_NB diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile index 021eee9..8d87439 100644 --- a/arch/x86/platform/Makefile +++ b/arch/x86/platform/Makefile @@ -1,6 +1,7 @@ # Platform specific code goes here obj-y += ce4100/ obj-y += efi/ +obj-y += geode/ obj-y += iris/ obj-y += mrst/ obj-y += olpc/ diff --git a/arch/x86/platform/geode/Makefile b/arch/x86/platform/geode/Makefile new file mode 100644 index 0000000..07c9cd0 --- /dev/null +++ b/arch/x86/platform/geode/Makefile @@ -0,0 +1 @@ +obj-$(CONFIG_ALIX) += alix.o diff --git a/arch/x86/platform/geode/alix.c b/arch/x86/platform/geode/alix.c new file mode 100644 index 0000000..f3f01e2 --- /dev/null +++ b/arch/x86/platform/geode/alix.c @@ -0,0 +1,141 @@ +/* + * System Specific setup for PCEngines ALIX. + * At the moment this means setup of GPIO control of LEDs + * on Alix.2/3/6 boards. + * + * + * Copyright (C) 2008 Constantin Baranov <const@mimas.ru> + * Copyright (C) 2011 Ed Wildgoose <kernel@wildgooses.com> + * + * TODO: There are large similarities with leds-net5501.c + * by Alessandro Zummo <a.zummo@towertech.it> + * In the future leds-net5501.c should be migrated over to platform + * + * 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 <linux/kernel.h> +#include <linux/init.h> +#include <linux/io.h> +#include <linux/string.h> +#include <linux/leds.h> +#include <linux/platform_device.h> +#include <linux/gpio.h> + +#include <asm/geode.h> + +static int force = 0; +module_param(force, bool, 0444); +/* FIXME: Award bios is not automatically detected as Alix platform */ +MODULE_PARM_DESC(force, "Force detection as ALIX.2/ALIX.3 platform"); + +static struct gpio_led alix_leds[] = { + { + .name = "alix:1", + .gpio = 6, + .default_trigger = "default-on", + .active_low = 1, + }, + { + .name = "alix:2", + .gpio = 25, + .default_trigger = "default-off", + .active_low = 1, + }, + { + .name = "alix:3", + .gpio = 27, + .default_trigger = "default-off", + .active_low = 1, + }, +}; + +static struct gpio_led_platform_data alix_leds_data = { + .num_leds = ARRAY_SIZE(alix_leds), + .leds = alix_leds, +}; + +static struct platform_device alix_leds_dev = { + .name = "leds-gpio", + .id = -1, + .dev.platform_data = &alix_leds_data, +}; + +static void __init register_alix(void) +{ + /* Setup LED control through leds-gpio driver */ + platform_device_register(&alix_leds_dev); +} + +static int __init alix_present(unsigned long bios_phys, + const char *alix_sig, + size_t alix_sig_len) +{ + const size_t bios_len = 0x00010000; + const char *bios_virt; + const char *scan_end; + const char *p; + char name[64]; + + if (force) { + printk(KERN_NOTICE "%s: forced to skip BIOS test, " + "assume system is ALIX.2/ALIX.3\n", + KBUILD_MODNAME); + return 1; + } + + bios_virt = phys_to_virt(bios_phys); + scan_end = bios_virt + bios_len - (alix_sig_len + 2); + for (p = bios_virt; p < scan_end; p++) { + const char *tail; + char *a; + + if (memcmp(p, alix_sig, alix_sig_len) != 0) + continue; + + memcpy(name, p, sizeof(name)); + + /* remove the first \0 character from string */ + a = strchr(name, '\0'); + if (a) + *a = ' '; + + /* cut the string at a newline */ + a = strchr(name, '\r'); + if (a) + *a = '\0'; + + tail = p + alix_sig_len; + if ((tail[0] == '2' || tail[0] == '3')) { + printk(KERN_INFO + "%s: system is recognized as \"%s\"\n", + KBUILD_MODNAME, name); + return 1; + } + } + + return 0; +} + +static int __init alix_init(void) +{ + const char tinybios_sig[] = "PC Engines ALIX."; + const char coreboot_sig[] = "PC Engines\0ALIX."; + + if (!is_geode()) + return 0; + + if (alix_present(0xf0000, tinybios_sig, sizeof(tinybios_sig) - 1) || + alix_present(0x500, coreboot_sig, sizeof(coreboot_sig) - 1)) + register_alix(); + + return 0; +} + +module_init(alix_init); + +MODULE_AUTHOR("Ed Wildgoose <kernel@wildgooses.com>"); +MODULE_DESCRIPTION("PCEngines ALIX System Setup"); +MODULE_LICENSE("GPL"); diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig index 6f190f4..e4573d6 100644 --- a/drivers/leds/Kconfig +++ b/drivers/leds/Kconfig @@ -97,14 +97,6 @@ config LEDS_WRAP help This option enables support for the PCEngines WRAP programmable LEDs. -config LEDS_ALIX2 - tristate "LED Support for ALIX.2 and ALIX.3 series" - depends on LEDS_CLASS - depends on X86 && !GPIO_CS5535 && !CS5535_GPIO - 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. - config LEDS_H1940 tristate "LED Support for iPAQ H1940 device" depends on LEDS_CLASS diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile index aae6989..bf5a00c 100644 --- a/drivers/leds/Makefile +++ b/drivers/leds/Makefile @@ -15,7 +15,6 @@ obj-$(CONFIG_LEDS_AMS_DELTA) += leds-ams-delta.o obj-$(CONFIG_LEDS_NET48XX) += leds-net48xx.o obj-$(CONFIG_LEDS_NET5501) += leds-net5501.o obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o -obj-$(CONFIG_LEDS_ALIX2) += leds-alix2.o obj-$(CONFIG_LEDS_H1940) += leds-h1940.o obj-$(CONFIG_LEDS_COBALT_QUBE) += leds-cobalt-qube.o obj-$(CONFIG_LEDS_COBALT_RAQ) += leds-cobalt-raq.o diff --git a/drivers/leds/leds-alix2.c b/drivers/leds/leds-alix2.c deleted file mode 100644 index f59ffad..0000000 --- a/drivers/leds/leds-alix2.c +++ /dev/null @@ -1,239 +0,0 @@ -/* - * LEDs driver for PCEngines ALIX.2 and ALIX.3 - * - * Copyright (C) 2008 Constantin Baranov <const@mimas.ru> - */ - -#include <linux/err.h> -#include <linux/io.h> -#include <linux/kernel.h> -#include <linux/leds.h> -#include <linux/module.h> -#include <linux/platform_device.h> -#include <linux/string.h> -#include <linux/pci.h> - -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[] = { - { - .cdev = { - .name = "alix:1", - .brightness_set = alix_led_set, - }, - .port = 0x00, - .on_value = 1 << 22, - .off_value = 1 << 6, - }, - { - .cdev = { - .name = "alix:2", - .brightness_set = alix_led_set, - }, - .port = 0x80, - .on_value = 1 << 25, - .off_value = 1 << 9, - }, - { - .cdev = { - .name = "alix:3", - .brightness_set = alix_led_set, - }, - .port = 0x80, - .on_value = 1 << 27, - .off_value = 1 << 11, - }, -}; - -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; - -fail: - while (--i >= 0) - led_classdev_unregister(&alix_leds[i].cdev); - return ret; -} - -static int alix_led_remove(struct platform_device *pdev) -{ - int i; - - for (i = 0; i < ARRAY_SIZE(alix_leds); i++) - led_classdev_unregister(&alix_leds[i].cdev); - return 0; -} - -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) -{ - const size_t bios_len = 0x00010000; - const char *bios_virt; - const char *scan_end; - const char *p; - char name[64]; - - if (force) { - printk(KERN_NOTICE "%s: forced to skip BIOS test, " - "assume system has ALIX.2 style LEDs\n", - KBUILD_MODNAME); - return 1; - } - - bios_virt = phys_to_virt(bios_phys); - scan_end = bios_virt + bios_len - (alix_sig_len + 2); - for (p = bios_virt; p < scan_end; p++) { - const char *tail; - char *a; - - if (memcmp(p, alix_sig, alix_sig_len) != 0) - continue; - - memcpy(name, p, sizeof(name)); - - /* remove the first \0 character from string */ - a = strchr(name, '\0'); - if (a) - *a = ' '; - - /* cut the string at a newline */ - a = strchr(name, '\r'); - if (a) - *a = '\0'; - - tail = p + alix_sig_len; - if ((tail[0] == '2' || tail[0] == '3')) { - printk(KERN_INFO - "%s: system is recognized as \"%s\"\n", - KBUILD_MODNAME, name); - return 1; - } - } - - 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) -{ - int ret = -ENODEV; - const char tinybios_sig[] = "PC Engines ALIX."; - const char coreboot_sig[] = "PC Engines\0ALIX."; - - if (alix_present(0xf0000, tinybios_sig, sizeof(tinybios_sig) - 1) || - alix_present(0x500, coreboot_sig, sizeof(coreboot_sig) - 1)) - ret = alix_pci_led_init(); - - 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); -} - -module_init(alix_led_init); -module_exit(alix_led_exit); - -MODULE_AUTHOR("Constantin Baranov <const@mimas.ru>"); -MODULE_DESCRIPTION("PCEngines ALIX.2 and ALIX.3 LED driver"); -MODULE_LICENSE("GPL"); -- 1.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] leds: New PCEngines Alix system driver (enables LEDs via gpio interface) 2011-03-19 16:51 ` [PATCH] leds: New PCEngines Alix system driver (enables LEDs via gpio interface) kernel @ 2011-03-19 17:21 ` Ed W 2011-03-24 3:52 ` Grant Likely 1 sibling, 0 replies; 18+ messages in thread From: Ed W @ 2011-03-19 17:21 UTC (permalink / raw) To: kernel; +Cc: grant.likely, dilinger, linux-geode, linux-kernel On 19/03/2011 16:51, kernel@wildgooses.com wrote: > From: Ed Wildgoose <git@wildgooses.com> > > 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-gpio driver to read other > GPIOs on the Alix board. With this new driver, we hook into leds-gpio > which in turn uses GPIO to control the LEDs and therefore it's > possible to control both the LEDs and access onboard GPIOs > > Driver is moved to platform/geode and any other geode > initialisation modules should move here also. > > This driver is inspired by leds-net5501.c > by: Alessandro Zummo <a.zummo@towertech.it> > Ideally, leds-net5501.c should also be moved to platform/geode. > Additionally the driver relies on parts of the patch: 7f131cf3ed > by: Daniel Mack <daniel@caiaq.de> to perform detection of the Alix board > Signed-off-by: Ed Wildgoose <kernel@wildgooses.com> Hi Grant I *think* this patch should now be satisfactory and I believe to be implemented as we discussed: - I have made the driver look like a straightforward platform initialisation routine and so further platform setup could be added here if required. My only thought is that it's quite tucked away and hard to find here, given that at present it only enables LEDs on this platform (and seems reasonably unlikely to grow beyond that). Would it be acceptable/sensible to put the Kconfig entry under LEDs for the time being (but the code under /platform/)? Obviously if it starts to do more initialisation/setup then the KConfig can be moved back to /arch/x86/ ? However, grateful if you could please now review this for inclusion? I will submit a second patch momentarily which adds the extra dependency of -gpio on -mfd as we discussed in the previous email Many thanks Ed W ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] leds: New PCEngines Alix system driver (enables LEDs via gpio interface) 2011-03-19 16:51 ` [PATCH] leds: New PCEngines Alix system driver (enables LEDs via gpio interface) kernel 2011-03-19 17:21 ` Ed W @ 2011-03-24 3:52 ` Grant Likely 1 sibling, 0 replies; 18+ messages in thread From: Grant Likely @ 2011-03-24 3:52 UTC (permalink / raw) To: kernel; +Cc: lists, dilinger, linux-geode, linux-kernel On Sat, Mar 19, 2011 at 04:51:06PM +0000, kernel@wildgooses.com wrote: > From: Ed Wildgoose <git@wildgooses.com> > > 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-gpio driver to read other > GPIOs on the Alix board. With this new driver, we hook into leds-gpio > which in turn uses GPIO to control the LEDs and therefore it's > possible to control both the LEDs and access onboard GPIOs > > Driver is moved to platform/geode and any other geode > initialisation modules should move here also. > > This driver is inspired by leds-net5501.c > by: Alessandro Zummo <a.zummo@towertech.it> > Ideally, leds-net5501.c should also be moved to platform/geode. > Additionally the driver relies on parts of the patch: 7f131cf3ed > by: Daniel Mack <daniel@caiaq.de> to perform detection of the Alix board > Signed-off-by: Ed Wildgoose <kernel@wildgooses.com> > --- [grafting in comments from 2nd email] > > Hi Grant > > I *think* this patch should now be satisfactory and I believe to be > implemented as we discussed: > > - I have made the driver look like a straightforward platform > initialisation routine and so further platform setup could be added here > if required. > > My only thought is that it's quite tucked away and hard to find here, > given that at present it only enables LEDs on this platform (and seems > reasonably unlikely to grow beyond that). Would it be > acceptable/sensible to put the Kconfig entry under LEDs for the time > being (but the code under /platform/)? Obviously if it starts to do more > initialisation/setup then the KConfig can be moved back to /arch/x86/ ? > > However, grateful if you could please now review this for inclusion? > > I will submit a second patch momentarily which adds the extra dependency > of -gpio on -mfd as we discussed in the previous email Hi Ed, Reviewed-by: Grant Likely <grant.likely@secretlab.ca> You'll need to get one of the x86 maintainers to pick this patch up. Also, a few notes on patch submission etiquette... (this is not a chastisement, just some things to remember for the next patch you submit. You do not need to resubmit your patch) When following up with new versions of a patch, it is common to include the version number in the subject line in; something like '[PATCH v2]', and to also include a changelog in the commit text that describes what has changed. It makes it easier to know with patch a reviewer should be looking at when there are multiple in the INBOX at one time. The revision history also helps after a patch is committed to know at a later date exactly which version actually made it into Linux mainline. Also if you want to add personal notes to a patch, you can simply add them below the '---' cut line. Text above that line gets committed, while text below it is culled when committing with git. So, instead of replying to your own patch, you can put the text inline. Cheers, g. --- > :100644 100644 d5ed94d... a10556d... M arch/x86/Kconfig > :100644 100644 021eee9... 8d87439... M arch/x86/platform/Makefile > :000000 100644 0000000... 07c9cd0... A arch/x86/platform/geode/Makefile > :000000 100644 0000000... f3f01e2... A arch/x86/platform/geode/alix.c > :100644 100644 6f190f4... e4573d6... M drivers/leds/Kconfig > :100644 100644 aae6989... bf5a00c... M drivers/leds/Makefile > :100644 000000 f59ffad... 0000000... D drivers/leds/leds-alix2.c > arch/x86/Kconfig | 14 +++ > arch/x86/platform/Makefile | 1 + > arch/x86/platform/geode/Makefile | 1 + > arch/x86/platform/geode/alix.c | 141 ++++++++++++++++++++++ > drivers/leds/Kconfig | 8 -- > drivers/leds/Makefile | 1 - > drivers/leds/leds-alix2.c | 239 -------------------------------------- > 7 files changed, 157 insertions(+), 248 deletions(-) > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index d5ed94d..a10556d 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -2094,6 +2094,20 @@ config OLPC_OPENFIRMWARE_DT > default y if OLPC_OPENFIRMWARE && PROC_DEVICETREE > select OF_PROMTREE > > +config ALIX > + bool "PCEngines ALIX System Support (LED setup)" > + select GPIOLIB > + ---help--- > + This option enables system support for the PCEngines ALIX. > + At present this just sets up LEDs for GPIO control on > + ALIX2/3/6 boards. However, other system specific setup should > + get added here. > + > + Note: You must still enable the drivers for GPIO and LED support > + (GPIO_CS5535 & LEDS_GPIO) to actually use the LEDs > + > + Note: You have to set alix.force=1 for boards with Award BIOS. > + > endif # X86_32 > > config AMD_NB > diff --git a/arch/x86/platform/Makefile b/arch/x86/platform/Makefile > index 021eee9..8d87439 100644 > --- a/arch/x86/platform/Makefile > +++ b/arch/x86/platform/Makefile > @@ -1,6 +1,7 @@ > # Platform specific code goes here > obj-y += ce4100/ > obj-y += efi/ > +obj-y += geode/ > obj-y += iris/ > obj-y += mrst/ > obj-y += olpc/ > diff --git a/arch/x86/platform/geode/Makefile b/arch/x86/platform/geode/Makefile > new file mode 100644 > index 0000000..07c9cd0 > --- /dev/null > +++ b/arch/x86/platform/geode/Makefile > @@ -0,0 +1 @@ > +obj-$(CONFIG_ALIX) += alix.o > diff --git a/arch/x86/platform/geode/alix.c b/arch/x86/platform/geode/alix.c > new file mode 100644 > index 0000000..f3f01e2 > --- /dev/null > +++ b/arch/x86/platform/geode/alix.c > @@ -0,0 +1,141 @@ > +/* > + * System Specific setup for PCEngines ALIX. > + * At the moment this means setup of GPIO control of LEDs > + * on Alix.2/3/6 boards. > + * > + * > + * Copyright (C) 2008 Constantin Baranov <const@mimas.ru> > + * Copyright (C) 2011 Ed Wildgoose <kernel@wildgooses.com> > + * > + * TODO: There are large similarities with leds-net5501.c > + * by Alessandro Zummo <a.zummo@towertech.it> > + * In the future leds-net5501.c should be migrated over to platform > + * > + * 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 <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/io.h> > +#include <linux/string.h> > +#include <linux/leds.h> > +#include <linux/platform_device.h> > +#include <linux/gpio.h> > + > +#include <asm/geode.h> > + > +static int force = 0; > +module_param(force, bool, 0444); > +/* FIXME: Award bios is not automatically detected as Alix platform */ > +MODULE_PARM_DESC(force, "Force detection as ALIX.2/ALIX.3 platform"); > + > +static struct gpio_led alix_leds[] = { > + { > + .name = "alix:1", > + .gpio = 6, > + .default_trigger = "default-on", > + .active_low = 1, > + }, > + { > + .name = "alix:2", > + .gpio = 25, > + .default_trigger = "default-off", > + .active_low = 1, > + }, > + { > + .name = "alix:3", > + .gpio = 27, > + .default_trigger = "default-off", > + .active_low = 1, > + }, > +}; > + > +static struct gpio_led_platform_data alix_leds_data = { > + .num_leds = ARRAY_SIZE(alix_leds), > + .leds = alix_leds, > +}; > + > +static struct platform_device alix_leds_dev = { > + .name = "leds-gpio", > + .id = -1, > + .dev.platform_data = &alix_leds_data, > +}; > + > +static void __init register_alix(void) > +{ > + /* Setup LED control through leds-gpio driver */ > + platform_device_register(&alix_leds_dev); > +} > + > +static int __init alix_present(unsigned long bios_phys, > + const char *alix_sig, > + size_t alix_sig_len) > +{ > + const size_t bios_len = 0x00010000; > + const char *bios_virt; > + const char *scan_end; > + const char *p; > + char name[64]; > + > + if (force) { > + printk(KERN_NOTICE "%s: forced to skip BIOS test, " > + "assume system is ALIX.2/ALIX.3\n", > + KBUILD_MODNAME); > + return 1; > + } > + > + bios_virt = phys_to_virt(bios_phys); > + scan_end = bios_virt + bios_len - (alix_sig_len + 2); > + for (p = bios_virt; p < scan_end; p++) { > + const char *tail; > + char *a; > + > + if (memcmp(p, alix_sig, alix_sig_len) != 0) > + continue; > + > + memcpy(name, p, sizeof(name)); > + > + /* remove the first \0 character from string */ > + a = strchr(name, '\0'); > + if (a) > + *a = ' '; > + > + /* cut the string at a newline */ > + a = strchr(name, '\r'); > + if (a) > + *a = '\0'; > + > + tail = p + alix_sig_len; > + if ((tail[0] == '2' || tail[0] == '3')) { > + printk(KERN_INFO > + "%s: system is recognized as \"%s\"\n", > + KBUILD_MODNAME, name); > + return 1; > + } > + } > + > + return 0; > +} > + > +static int __init alix_init(void) > +{ > + const char tinybios_sig[] = "PC Engines ALIX."; > + const char coreboot_sig[] = "PC Engines\0ALIX."; > + > + if (!is_geode()) > + return 0; > + > + if (alix_present(0xf0000, tinybios_sig, sizeof(tinybios_sig) - 1) || > + alix_present(0x500, coreboot_sig, sizeof(coreboot_sig) - 1)) > + register_alix(); > + > + return 0; > +} > + > +module_init(alix_init); > + > +MODULE_AUTHOR("Ed Wildgoose <kernel@wildgooses.com>"); > +MODULE_DESCRIPTION("PCEngines ALIX System Setup"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig > index 6f190f4..e4573d6 100644 > --- a/drivers/leds/Kconfig > +++ b/drivers/leds/Kconfig > @@ -97,14 +97,6 @@ config LEDS_WRAP > help > This option enables support for the PCEngines WRAP programmable LEDs. > > -config LEDS_ALIX2 > - tristate "LED Support for ALIX.2 and ALIX.3 series" > - depends on LEDS_CLASS > - depends on X86 && !GPIO_CS5535 && !CS5535_GPIO > - 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. > - > config LEDS_H1940 > tristate "LED Support for iPAQ H1940 device" > depends on LEDS_CLASS > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile > index aae6989..bf5a00c 100644 > --- a/drivers/leds/Makefile > +++ b/drivers/leds/Makefile > @@ -15,7 +15,6 @@ obj-$(CONFIG_LEDS_AMS_DELTA) += leds-ams-delta.o > obj-$(CONFIG_LEDS_NET48XX) += leds-net48xx.o > obj-$(CONFIG_LEDS_NET5501) += leds-net5501.o > obj-$(CONFIG_LEDS_WRAP) += leds-wrap.o > -obj-$(CONFIG_LEDS_ALIX2) += leds-alix2.o > obj-$(CONFIG_LEDS_H1940) += leds-h1940.o > obj-$(CONFIG_LEDS_COBALT_QUBE) += leds-cobalt-qube.o > obj-$(CONFIG_LEDS_COBALT_RAQ) += leds-cobalt-raq.o > diff --git a/drivers/leds/leds-alix2.c b/drivers/leds/leds-alix2.c > deleted file mode 100644 > index f59ffad..0000000 > --- a/drivers/leds/leds-alix2.c > +++ /dev/null > @@ -1,239 +0,0 @@ > -/* > - * LEDs driver for PCEngines ALIX.2 and ALIX.3 > - * > - * Copyright (C) 2008 Constantin Baranov <const@mimas.ru> > - */ > - > -#include <linux/err.h> > -#include <linux/io.h> > -#include <linux/kernel.h> > -#include <linux/leds.h> > -#include <linux/module.h> > -#include <linux/platform_device.h> > -#include <linux/string.h> > -#include <linux/pci.h> > - > -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[] = { > - { > - .cdev = { > - .name = "alix:1", > - .brightness_set = alix_led_set, > - }, > - .port = 0x00, > - .on_value = 1 << 22, > - .off_value = 1 << 6, > - }, > - { > - .cdev = { > - .name = "alix:2", > - .brightness_set = alix_led_set, > - }, > - .port = 0x80, > - .on_value = 1 << 25, > - .off_value = 1 << 9, > - }, > - { > - .cdev = { > - .name = "alix:3", > - .brightness_set = alix_led_set, > - }, > - .port = 0x80, > - .on_value = 1 << 27, > - .off_value = 1 << 11, > - }, > -}; > - > -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; > - > -fail: > - while (--i >= 0) > - led_classdev_unregister(&alix_leds[i].cdev); > - return ret; > -} > - > -static int alix_led_remove(struct platform_device *pdev) > -{ > - int i; > - > - for (i = 0; i < ARRAY_SIZE(alix_leds); i++) > - led_classdev_unregister(&alix_leds[i].cdev); > - return 0; > -} > - > -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) > -{ > - const size_t bios_len = 0x00010000; > - const char *bios_virt; > - const char *scan_end; > - const char *p; > - char name[64]; > - > - if (force) { > - printk(KERN_NOTICE "%s: forced to skip BIOS test, " > - "assume system has ALIX.2 style LEDs\n", > - KBUILD_MODNAME); > - return 1; > - } > - > - bios_virt = phys_to_virt(bios_phys); > - scan_end = bios_virt + bios_len - (alix_sig_len + 2); > - for (p = bios_virt; p < scan_end; p++) { > - const char *tail; > - char *a; > - > - if (memcmp(p, alix_sig, alix_sig_len) != 0) > - continue; > - > - memcpy(name, p, sizeof(name)); > - > - /* remove the first \0 character from string */ > - a = strchr(name, '\0'); > - if (a) > - *a = ' '; > - > - /* cut the string at a newline */ > - a = strchr(name, '\r'); > - if (a) > - *a = '\0'; > - > - tail = p + alix_sig_len; > - if ((tail[0] == '2' || tail[0] == '3')) { > - printk(KERN_INFO > - "%s: system is recognized as \"%s\"\n", > - KBUILD_MODNAME, name); > - return 1; > - } > - } > - > - 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) > -{ > - int ret = -ENODEV; > - const char tinybios_sig[] = "PC Engines ALIX."; > - const char coreboot_sig[] = "PC Engines\0ALIX."; > - > - if (alix_present(0xf0000, tinybios_sig, sizeof(tinybios_sig) - 1) || > - alix_present(0x500, coreboot_sig, sizeof(coreboot_sig) - 1)) > - ret = alix_pci_led_init(); > - > - 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); > -} > - > -module_init(alix_led_init); > -module_exit(alix_led_exit); > - > -MODULE_AUTHOR("Constantin Baranov <const@mimas.ru>"); > -MODULE_DESCRIPTION("PCEngines ALIX.2 and ALIX.3 LED driver"); > -MODULE_LICENSE("GPL"); > -- > 1.7.4 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] gpio: Show explicit dependency between GPIO_CS5535 and MFD_CS5535 2011-03-18 22:48 ` Grant Likely 2011-03-19 16:51 ` [PATCH] leds: New PCEngines Alix system driver (enables LEDs via gpio interface) kernel @ 2011-03-19 17:46 ` kernel 2011-03-19 19:59 ` Andres Salomon 1 sibling, 1 reply; 18+ messages in thread From: kernel @ 2011-03-19 17:46 UTC (permalink / raw) To: lists, grant.likely, dilinger; +Cc: linux-geode, linux-kernel From: Ed Wildgoose <kernel@wildgooses.com> cs5535-gpio.c has been split into two, with various setup moved into cs5535-mfd.c. Given that cs5535-gpio will not load without the -mfd part, lets make that dependency explicit in Kconfig Signed-off-by: Ed Wildgoose <kernel@wildgooses.com> --- :100644 100644 664660e... 3246ad4... M drivers/gpio/Kconfig drivers/gpio/Kconfig | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index 664660e..3246ad4 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -295,7 +295,7 @@ comment "PCI GPIO expanders:" config GPIO_CS5535 tristate "AMD CS5535/CS5536 GPIO support" - depends on PCI && X86 && !CS5535_GPIO + depends on PCI && X86 && !CS5535_GPIO && MFD_CS5535 help The AMD CS5535 and CS5536 southbridges support 28 GPIO pins that can be used for quite a number of things. The CS5535/6 is found on -- 1.7.4 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] gpio: Show explicit dependency between GPIO_CS5535 and MFD_CS5535 2011-03-19 17:46 ` [PATCH] gpio: Show explicit dependency between GPIO_CS5535 and MFD_CS5535 kernel @ 2011-03-19 19:59 ` Andres Salomon 0 siblings, 0 replies; 18+ messages in thread From: Andres Salomon @ 2011-03-19 19:59 UTC (permalink / raw) To: kernel; +Cc: lists, grant.likely, linux-geode, linux-kernel On Sat, 19 Mar 2011 17:46:25 +0000 kernel@wildgooses.com wrote: > From: Ed Wildgoose <kernel@wildgooses.com> > > cs5535-gpio.c has been split into two, with various setup moved > into cs5535-mfd.c. Given that cs5535-gpio will not load without > the -mfd part, lets make that dependency explicit in Kconfig Thanks! Acked-by: Andres Salomon <dilinger@queued.net> > > Signed-off-by: Ed Wildgoose <kernel@wildgooses.com> > --- > :100644 100644 664660e... 3246ad4... M drivers/gpio/Kconfig > drivers/gpio/Kconfig | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig > index 664660e..3246ad4 100644 > --- a/drivers/gpio/Kconfig > +++ b/drivers/gpio/Kconfig > @@ -295,7 +295,7 @@ comment "PCI GPIO expanders:" > > config GPIO_CS5535 > tristate "AMD CS5535/CS5536 GPIO support" > - depends on PCI && X86 && !CS5535_GPIO > + depends on PCI && X86 && !CS5535_GPIO && MFD_CS5535 > help > The AMD CS5535 and CS5536 southbridges support 28 GPIO > pins that can be used for quite a number of things. The CS5535/6 is > found on ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-17 17:59 ` Ed W 2011-03-17 18:17 ` Grant Likely @ 2011-03-17 18:22 ` Andres Salomon 1 sibling, 0 replies; 18+ messages in thread From: Andres Salomon @ 2011-03-17 18:22 UTC (permalink / raw) To: Ed W; +Cc: Grant Likely, rpurdie, linux-geode, const, linux-kernel On Thu, 17 Mar 2011 17:59:06 +0000 Ed W <lists@wildgooses.com> wrote: > On 17/03/2011 17:52, Andres Salomon wrote: > > OLPC stuff lives in arch/x86/platform/olpc; if there was more > > alix-specific stuff, I'd suggest moving it into something similar. > > However, I didn't find any. Maybe an arch/x86/platform/geode as a > > place to collect platform drivers for the various geode-based > > machines out there (alix, soekris, etc)? Though honestly, I'm not > > that interested in doing the work to migrate stuff over to there. > > > As this is my first stab at this, I think you are agreeing that the > current change is ok as a first step at least? (I can have a go at a > larger code re-org later...) Yes. > > Can I get your final opinion on what should be in the code Copyright > statement before I resubmit with the module_init change? To recap I > don't want to alienate anyone, but the code is now based on a) > leds-5501 and b) a patch from someone not currently credited in the > file copyright statement? I have stated this origin in the commit > log and noted the derivation in the code itself. What to do..? alix_present came from commit ec9a943ce9f6d6a8ea09587b49d29a020c418c76 , which was from Constantin. As such, it's probably not a good idea to remove the copyright notice. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-17 17:24 ` Ed W 2011-03-17 17:52 ` Andres Salomon @ 2011-03-17 18:12 ` Grant Likely 1 sibling, 0 replies; 18+ messages in thread From: Grant Likely @ 2011-03-17 18:12 UTC (permalink / raw) To: Ed W; +Cc: Andres Salomon, rpurdie, linux-geode, const, linux-kernel On Thu, Mar 17, 2011 at 05:24:55PM +0000, Ed W wrote: > On 17/03/2011 16:08, Grant Likely wrote: > > Actually, it looks like with your changes this isn't even a driver > > anymore. It is merely code to register a device on a specific > > platform. Is there any other alix-specific initialization code in the > > kernel? If so, you should consider relocating the device registration > > with the rest of the alix setup code. > > Agreed. I confess that I don't understand the linux driver structure > enough to shift the code further though > > What I observe is that there is a lot of arch specific setup for ARM, > etc, however, this is not currently done at all for x86 (which is Alix), > so at the moment this would seem to sit slightly awkwardly with current > x86 arch code? > > Instead I found leds-net5501.c, which is for a very similar platform to > the Alix (not quite similar enough that I could combine the files) and I > used that as my prototype for this driver. > > I think given that we already have a similar driver in the leds area > which does platform alike setup, this gives some justification for doing > the same with the Alix leds? No, that's bad practise. You should have one Alix setup file, probably in arch/x86/platform, that takes care of all Alix specific setup registrations. Right now that only consists of gpio leds, but it could potentially grow. Note that this is only for on-time setup and registrations. Actual device drivers still belong under drivers/ > Additionally if we ever find we need Alix > specific setup code then the code is ready to be used as is by the > platform code? > > > >>> -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. > > > > Yes, unless you've got specific ordering constraints this should > > definitely be module_init(). > > I'm out of my depth here. I would be very happy to resubmit either way? > > However, is there not a potential ordering issue if leds-alix2 is loaded > *before* leds-gpio? Is this not the reason for making it an arch_initcall? Nope. It does not matter if the driver gets registered first or the device. > Also the same code is used in leds-5501.c - would you like me to submit > a patch to change that also (if you confirm it should become a > module_init call?). Unless you've got the hardware to test, probably not. > Thanks for final confirmation on this and I will quickly resubmit the patch? Thanks g. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-17 15:43 ` Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface Andres Salomon 2011-03-17 16:08 ` Grant Likely @ 2011-03-17 17:04 ` Ed W 2011-03-17 18:07 ` Grant Likely 1 sibling, 1 reply; 18+ messages in thread From: Ed W @ 2011-03-17 17:04 UTC (permalink / raw) To: Andres Salomon; +Cc: rpurdie, linux-geode, const, linux-kernel, Grant Likely Hi Andres Thanks for your feedback! >> Additionally it relies on parts of the patch: 7f131cf3ed >> by: Daniel Mack <daniel@caiaq.de> to perform detection of the Alix >> board .. >> - * Copyright (C) 2008 Constantin Baranov <const@mimas.ru> > > This copyright line should not be removed, so long as parts of the > original driver (such as alix_present) remain. Thanks for guidance here. Can I ask for further thought on this - there is very close to zero original driver present, and although I had better do some more diffs to be sure, I think you would see the only common code was the #includes, a "force" param and a few other { }s? I did deliberately reuse the "alix_present" function, but this appears to be written by Daniel Mack and contributed in patch: 7f131cf3ed - however, Daniel is not listed in the current copyright statement on the module (I did copy him and Constantin in on this patch so that either might object?). Also I have tried to show this code attribution in the commit statement? I have also noted in the code that this is based on leds-net5501.c - is this a sufficient and normal attribution? Can someone offer a final "ruling" as to how I should state the copyright line given that the code was written by taking the leds-net5501.c skeleton and approximately applying the commit 7f131cf3ed, to give the current code? Many thanks Ed W ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface 2011-03-17 17:04 ` Ed W @ 2011-03-17 18:07 ` Grant Likely 0 siblings, 0 replies; 18+ messages in thread From: Grant Likely @ 2011-03-17 18:07 UTC (permalink / raw) To: Ed W; +Cc: Andres Salomon, rpurdie, linux-geode, const, linux-kernel On Thu, Mar 17, 2011 at 05:04:25PM +0000, Ed W wrote: > Hi Andres > > Thanks for your feedback! > > >> Additionally it relies on parts of the patch: 7f131cf3ed > >> by: Daniel Mack <daniel@caiaq.de> to perform detection of the Alix > >> board > > .. > >> - * Copyright (C) 2008 Constantin Baranov <const@mimas.ru> > > > > This copyright line should not be removed, so long as parts of the > > original driver (such as alix_present) remain. > > Thanks for guidance here. > > Can I ask for further thought on this - there is very close to zero > original driver present, and although I had better do some more diffs to > be sure, I think you would see the only common code was the #includes, a > "force" param and a few other { }s? > > I did deliberately reuse the "alix_present" function, but this appears > to be written by Daniel Mack and contributed in patch: 7f131cf3ed - > however, Daniel is not listed in the current copyright statement on the > module (I did copy him and Constantin in on this patch so that either > might object?). Also I have tried to show this code attribution in the > commit statement? > > I have also noted in the code that this is based on leds-net5501.c - is > this a sufficient and normal attribution? > > Can someone offer a final "ruling" as to how I should state the > copyright line given that the code was written by taking the > leds-net5501.c skeleton and approximately applying the commit > 7f131cf3ed, to give the current code? Use common sense. It is always safe to leave copyright notices in place, but if it really does look like the old stuff is gone then go ahead and drop it. Same goes when you clone a driver. The original copyrights should not be stripped off unless there really is nothing left. g. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2011-03-24 3:52 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <4D81D7FD.1040602@wildgooses.com>
2011-03-17 15:43 ` Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface Andres Salomon
2011-03-17 16:08 ` Grant Likely
2011-03-17 17:24 ` Ed W
2011-03-17 17:52 ` Andres Salomon
2011-03-17 17:59 ` Ed W
2011-03-17 18:17 ` Grant Likely
2011-03-18 18:12 ` kernel
2011-03-18 18:32 ` Ed W
2011-03-18 22:48 ` Grant Likely
2011-03-19 16:51 ` [PATCH] leds: New PCEngines Alix system driver (enables LEDs via gpio interface) kernel
2011-03-19 17:21 ` Ed W
2011-03-24 3:52 ` Grant Likely
2011-03-19 17:46 ` [PATCH] gpio: Show explicit dependency between GPIO_CS5535 and MFD_CS5535 kernel
2011-03-19 19:59 ` Andres Salomon
2011-03-17 18:22 ` Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface Andres Salomon
2011-03-17 18:12 ` Grant Likely
2011-03-17 17:04 ` Ed W
2011-03-17 18:07 ` Grant Likely
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox