Re: Feedback please: [PATCH] leds: New PCEngines Alix LED driverusing gpio interface
From: Andres Salomon
Date: Thu Mar 17 2011 - 11:43:42 EST
On Thu, 17 Mar 2011 09:44:29 +0000
Ed W <lists@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx
> To: rpurdie@xxxxxxxxx
> CC: const@xxxxxxxx, daniel@xxxxxxxx, a.zummo@xxxxxxxxxxxx,
> linux-kernel@xxxxxxxxxxxxxxx, Ed Wildgoose <kernel@xxxxxxxxxxxxxx>
>
>
>
> From: Ed Wildgoose <kernel@xxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
> Additionally it relies on parts of the patch: 7f131cf3ed
> by: Daniel Mack <daniel@xxxxxxxx> to perform detection of the Alix
> board
>
> Signed-off-by: Ed Wildgoose <kernel@xxxxxxxxxxxxxx>
> ---
> :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@xxxxxxxx>
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@xxxxxxxxxxxxxx>
> + * Based on leds-net5501.c by Alessandro Zummo <a.zummo@xxxxxxxxxxxx>
> + *
> + * 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@xxxxxxxx>");
> +MODULE_AUTHOR("Ed Wildgoose <kernel@xxxxxxxxxxxxxx>");
> MODULE_DESCRIPTION("PCEngines ALIX.2 and ALIX.3 LED driver");
> MODULE_LICENSE("GPL");
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/