Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface

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.

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?

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: [email protected]
To: [email protected]
CC: [email protected], [email protected], [email protected],
[email protected], Ed Wildgoose <[email protected]>

From: Ed Wildgoose <[email protected]>

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 <[email protected]> Additionally it relies on parts of the patch: 7f131cf3ed
by: Daniel Mack <[email protected]> to perform detection of the Alix board

Signed-off-by Ed Wildgoose <[email protected]>

---
: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(-)

drivers/leds/Kconfig +1 -1

index 6f190f4..25f0285 100644
@@ -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

drivers/leds/leds-alix2.c +5 -0

+ * Copyright (C) 2011 Nippy Networks Ltd
+ * Written by Ed Wildgoose <[email protected]>
+ * Based on leds-net5501.c by Alessandro Zummo <[email protected]>
+ *
+ * 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.
*/

drivers/leds/leds-alix2.c +6 -5

-#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");

drivers/leds/leds-alix2.c +0 -7

-#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) },

drivers/leds/leds-alix2.c +0 -1

-	{ PCI_DEVICE(PCI_VENDOR_ID_AMD,

PCI_DEVICE_ID_AMD_CS5536_ISA) },

drivers/leds/leds-alix2.c +13 -53

-	{ } /* 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);

drivers/leds/leds-alix2.c +2 -4

-		if (ret < 0)
-			goto fail;
-	}
-	return 0;
+static struct gpio_led_platform_data alix_leds_data = {
+	.num_leds = ARRAY_SIZE(alix_leds),

drivers/leds/leds-alix2.c +5 -50

+	.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;
-}
-

drivers/leds/leds-alix2.c +0 -15

-	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)

drivers/leds/leds-alix2.c +1 -1

-MODULE_AUTHOR("Constantin Baranov <[email protected]>");
+MODULE_AUTHOR("Ed Wildgoose <[email protected]>");
 MODULE_DESCRIPTION("PCEngines ALIX.2 and ALIX.3 LED driver");
 MODULE_LICENSE("GPL");

Quoted 3 lines · On Thu, 17 Mar 2011 09:44:29 +0000 Ed W <[email protected]> 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.

Quoted 3 lines

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.

Quoted 70 lines

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:         [email protected]
To:   [email protected]
CC:   [email protected], [email protected], [email protected], [email protected], Ed Wildgoose <[email protected]>

From: Ed Wildgoose <[email protected]>

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 <[email protected]> Additionally it relies on parts of the patch: 7f131cf3ed
by: Daniel Mack <[email protected]> to perform detection of the Alix board

Signed-off-by Ed Wildgoose <[email protected]>

---
: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(-)

drivers/leds/Kconfig +0 -0

index 6f190f4..25f0285 100644
@@ -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

drivers/leds/leds-alix2.c line 4

Quoted 4 lines

drivers/leds/leds-alix2.c +0 -1

@@ -1,119 +1,66 @@
  /*
   * LEDs driver for PCEngines ALIX.2 and ALIX.3
   *
- * Copyright (C) 2008 Constantin Baranov <[email protected]>

This copyright line should not be removed, so long as parts of the original driver (such as alix_present) remain.

Quoted 123 lines

drivers/leds/leds-alix2.c +5 -0

+ * Copyright (C) 2011 Nippy Networks Ltd
+ * Written by Ed Wildgoose <[email protected]>
+ * Based on leds-net5501.c by Alessandro Zummo <[email protected]>
+ *
+ * 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.
  */

drivers/leds/leds-alix2.c +0 -2

-#include <linux/err.h>
-#include <linux/io.h>

 #include <linux/kernel.h>

drivers/leds/leds-alix2.c +3 -0

+#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>

drivers/leds/leds-alix2.c +3 -2

-#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");

drivers/leds/leds-alix2.c +0 -7

-#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 */

drivers/leds/leds-alix2.c +1 -23

-};
-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,
      },
 };

drivers/leds/leds-alix2.c +0 -8

-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;

drivers/leds/leds-alix2.c +4 -0

+static struct gpio_led_platform_data alix_leds_data = {
+     .num_leds = ARRAY_SIZE(alix_leds),
+     .leds = alix_leds,
+};

drivers/leds/leds-alix2.c

Quoted 7 lines

drivers/leds/leds-alix2.c +2 -5

-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".

Quoted 79 lines

+     .id = -1,
+     .dev.platform_data = &alix_leds_data,

drivers/leds/leds-alix2.c +2 -1

+};
 
-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);
 }

drivers/leds/leds-alix2.c +0 -8

-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)

drivers/leds/leds-alix2.c +1 -38

@@ -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.";
 

drivers/leds/leds-alix2.c static int __init alix_present(unsigned long · line 126

Quoted 3 lines

drivers/leds/leds-alix2.c +3 -0

+     if (!is_geode())
+             return 0;
+

Presumably you want to return -ENODEV here, especially since your driver has no method for unloading.

drivers/leds/leds-alix2.c static int __init alix_present(unsigned long · line 128

Quoted 6 lines

drivers/leds/leds-alix2.c +1 -2

       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.

Quoted 25 lines

drivers/leds/leds-alix2.c +1 -19

-     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;
  }
 

drivers/leds/leds-alix2.c

Quoted 3 lines

drivers/leds/leds-alix2.c +1 -2

-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.

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.

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +1 -2

-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().

Quoted 5 lines · Grant Likely

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?

Quoted 9 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +1 -2

-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

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.

Quoted 6 lines · Andres Salomon

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...)

:000000 100644 0000000... bc1b3b3... A arch/x86/platform/geode/alix_leds.c

arch/x86/Kconfig +0 -0

index d5ed94d..b16ab56 100644
@@ -2094,6 +2094,15 @@ config OLPC_OPENFIRMWARE_DT
 	default y if OLPC_OPENFIRMWARE && PROC_DEVICETREE
 	select OF_PROMTREE
 

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":

Quoted 1 line

:000000 100644 0000000... bc1b3b3... A        arch/x86/platform/geode/alix_leds.c

...

Quoted 17 lines

arch/x86/Kconfig +0 -0

index d5ed94d..b16ab56 100644
@@ -2094,6 +2094,15 @@ config OLPC_OPENFIRMWARE_DT

      default y if OLPC_OPENFIRMWARE && PROC_DEVICETREE select OF_PROMTREE

arch/x86/Kconfig +9 -0

+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.
+

Quoted 1 line

 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?

From: Ed Wildgoose <[email protected]>

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 <[email protected]>
Ideally, leds-net5501.c should also be moved to platform/geode. Additionally the driver relies on parts of the patch: 7f131cf3ed
by: Daniel Mack <[email protected]> to perform detection of the Alix board

Signed-off-by Ed Wildgoose <[email protected]>

From: Ed Wildgoose <[email protected]>

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 <[email protected]>
Ideally, leds-net5501.c should also be moved to platform/geode. Additionally the driver relies on parts of the patch: 7f131cf3ed
by: Daniel Mack <[email protected]> to perform detection of the Alix board

Signed-off-by Ed Wildgoose <[email protected]>

---

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

: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(-)

arch/x86/Kconfig +14 -0

index d5ed94d..a10556d 100644
@@ -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

arch/x86/platform/Makefile +1 -0

index 021eee9..8d87439 100644
@@ -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/

arch/x86/platform/geode/Makefile new file +1 -0

new file mode 100644
index 0000000..07c9cd0
@@ -0,0 +1 @@
+obj-$(CONFIG_ALIX)		+= alix.o

arch/x86/platform/geode/alix.c new file +141 -0

new file mode 100644
index 0000000..f3f01e2
@@ -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 <[email protected]>
+ * Copyright (C) 2011 Ed Wildgoose <[email protected]>
+ *
+ * TODO: There are large similarities with leds-net5501.c
+ * by Alessandro Zummo <[email protected]>
+ * 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 <[email protected]>");
+MODULE_DESCRIPTION("PCEngines ALIX System Setup");
+MODULE_LICENSE("GPL");

drivers/leds/Kconfig +0 -8

index 6f190f4..e4573d6 100644
@@ -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

drivers/leds/Makefile +0 -1

index aae6989..bf5a00c 100644
@@ -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

drivers/leds/leds-alix2.c deleted +0 -239

deleted file mode 100644
index f59ffad..0000000
@@ -1,239 +0,0 @@
-/*
- * LEDs driver for PCEngines ALIX.2 and ALIX.3
- *
- * Copyright (C) 2008 Constantin Baranov <[email protected]>
- */
-
-#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 <[email protected]>");
-MODULE_DESCRIPTION("PCEngines ALIX.2 and ALIX.3 LED driver");
-MODULE_LICENSE("GPL");

Signature · 1 line

1.7.4

From: Ed Wildgoose <[email protected]>

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 <[email protected]>

---
:100644 100644 664660e... 3246ad4... M drivers/gpio/Kconfig

 drivers/gpio/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/gpio/Kconfig +1 -1

index 664660e..3246ad4 100644
@@ -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

Quoted 6 lines · Andres Salomon

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..?

Quoted 5 lines · Grant Likely

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?

Quoted 7 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +1 -2

-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.

Quoted 2 lines

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?

Quoted 3 lines

Additionally it relies on parts of the patch: 7f131cf3ed
by: Daniel Mack <[email protected]> to perform detection of the Alix board

Quoted 1 line

- * Copyright (C) 2008 Constantin Baranov <[email protected]>

This copyright line should not be removed, so long as parts of the original driver (such as alix_present) remain.

Hi Andres

Thanks for your feedback!

Quoted 3 lines

Quoted 3 lines

Additionally it relies on parts of the patch: 7f131cf3ed
by: Daniel Mack <[email protected]> to perform detection of the Alix board

..

Quoted 4 lines

Quoted 1 line

- * Copyright (C) 2008 Constantin Baranov <[email protected]>

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?