Introduce a led trigger for CPU activity

Attempting to consolidate the ARM LED code, this removes the custom RealView LED trigger code to turn LEDs on and off in response to CPU activity and replace it with a standard trigger.

(bryan.wu at canonical.com:
It moves arch/arm/kernel/leds.c syscore stubs into this trigger. It also provides ledtrig_cpu trigger event stub in <linux/leds.h>. Although it was inspired by ARM work, it can be used in other arch.)

Cc Richard Purdie <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

Signed-off-by Bryan Wu <[email protected]>

---

?drivers/leds/Kconfig ? ? ? | ? 10 +++
?drivers/leds/Makefile ? ? ?| ? ?1 +
?drivers/leds/ledtrig-cpu.c | ?144 ++++++++++++++++++++++++++++++++++++++++++++
?include/linux/leds.h ? ? ? | ? 15 +++++

?4 files changed, 170 insertions(+), 0 deletions(-) ?create mode 100644 drivers/leds/ledtrig-cpu.c

drivers/leds/Kconfig +0 -0

index 713d43b..186000a 100644
@@ -446,6 +446,16 @@ config LEDS_TRIGGER_BACKLIGHT
 

? ? ? ? ?If unsure, say N.

drivers/leds/Kconfig +10 -0

+config LEDS_TRIGGER_CPU
+ ? ? ? bool "LED CPU Trigger"
+ ? ? ? depends on LEDS_TRIGGERS
+ ? ? ? help
+ ? ? ? ? This allows LEDs to be controlled by active CPUs. This shows
+ ? ? ? ? the active CPUs across an array of LEDs so you can see what
+ ? ? ? ? CPUs are active on the system at any given moment.
+
+ ? ? ? ? If unsure, say N.
+

?config LEDS_TRIGGER_GPIO
? ? ? ?tristate "LED GPIO Trigger"
? ? ? ?depends on LEDS_TRIGGERS

drivers/leds/Makefile +0 -0

index bbfd2e3..5e70d2e 100644
@@ -53,4 +53,5 @@ obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK) ? += ledtrig-ide-disk.o

?obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT) ? += ledtrig-heartbeat.o ?obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT) ? += ledtrig-backlight.o ?obj-$(CONFIG_LEDS_TRIGGER_GPIO) ? ? ? ? ? ? ? ?+= ledtrig-gpio.o +obj-$(CONFIG_LEDS_TRIGGER_CPU) ? ? ? ? += ledtrig-cpu.o ?obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) ?+= ledtrig-default-on.o

drivers/leds/ledtrig-cpu.c new file +47 -0

new file mode 100644
index 0000000..4d63ada
@@ -0,0 +1,144 @@
+/*
+ * ledtrig-cpu.c - LED trigger based on CPU activity
+ *
+ * Copyright 2011 Linus Walleij <[email protected]>
+ * Copyright 2011 Bryan Wu <[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.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/percpu.h>
+#include <linux/syscore_ops.h>
+#include "leds.h"
+
+struct cpu_trig_data {
+ ? ? ? struct led_classdev *led;
+};
+
+static DEFINE_PER_CPU(struct cpu_trig_data *, cpu_triggers);
+
+void ledtrig_cpu(enum cpu_led_event ledevt)
+{
+ ? ? ? struct cpu_trig_data *trigdata = __get_cpu_var(cpu_triggers);
+
+ ? ? ? if (!trigdata)
+ ? ? ? ? ? ? ? return;
+
+ ? ? ? /* Locate the correct CPU LED */
+
+ ? ? ? switch (ledevt) {
+ ? ? ? case CPU_LED_START:
+ ? ? ? case CPU_LED_IDLE_START:
+ ? ? ? ? ? ? ? /* Will turn the LED on, max brightness */
+ ? ? ? ? ? ? ? if (trigdata->led)
+ ? ? ? ? ? ? ? ? ? ? ? led_set_brightness(trigdata->led,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?trigdata->led->max_brightness);
+ ? ? ? ? ? ? ? break;
+
+ ? ? ? case CPU_LED_STOP:
+ ? ? ? case CPU_LED_IDLE_END:
+ ? ? ? case CPU_LED_HALTED:

drivers/leds/ledtrig-cpu.c new file +53 -0

+EXPORT_SYMBOL(ledtrig_cpu);
+
+static void cpu_trig_activate_cpu(void *data)
+{
+ ? ? ? struct cpu_trig_data *cpu_data;
+ ? ? ? struct led_classdev *led = data;
+ ? ? ? int my_cpu = smp_processor_id();
+ ? ? ? unsigned long target_cpu = (unsigned long) led->trigger_data;
+
+ ? ? ? if (target_cpu != my_cpu)
+ ? ? ? ? ? ? ? return;
+
+ ? ? ? cpu_data = kzalloc(sizeof(*cpu_data), GFP_KERNEL);
+ ? ? ? if (!cpu_data)
+ ? ? ? ? ? ? ? return;
+
+ ? ? ? dev_info(led->dev, "led %s indicate activity on CPU %d\n",
+ ? ? ? ? ? ? ? ?led->name, my_cpu);
+
+ ? ? ? cpu_data->led = led;
+ ? ? ? __get_cpu_var(cpu_triggers) = cpu_data;
+}
+
+static void cpu_trig_activate(struct led_classdev *led)
+{
+ ? ? ? on_each_cpu(cpu_trig_activate_cpu, led, 1);
+}
+
+static void cpu_trig_deactivate(struct led_classdev *led)
+{
+ ? ? ? struct cpu_trig_data *cpu_data = led->trigger_data;
+
+ ? ? ? kfree(cpu_data);
+}
+
+static struct led_trigger cpu_led_trigger = {
+ ? ? ? .name ? ? ? ? ? = "cpu",
+ ? ? ? .activate ? ? ? = cpu_trig_activate,
+ ? ? ? .deactivate ? ? = cpu_trig_deactivate,
+};
+
+
+static int cpu_syscore_suspend(void)
+{
+ ? ? ? ledtrig_cpu(CPU_LED_STOP);
+ ? ? ? return 0;
+}
+
+static void cpu_syscore_resume(void)
+{
+ ? ? ? ledtrig_cpu(CPU_LED_START);
+}
+

drivers/leds/ledtrig-cpu.c new file +9 -0

+
+static int __init cpu_trig_init(void)
+{
+ ? ? ? int ret;
+
+ ? ? ? ret = led_trigger_register(&cpu_led_trigger);
+ ? ? ? if (!ret)
+ ? ? ? ? ? ? ? register_syscore_ops(&cpu_syscore_ops);
+

drivers/leds/ledtrig-cpu.c new file +5 -0

+
+MODULE_AUTHOR("Linus Walleij <[email protected]>");
+MODULE_AUTHOR("Bryan Wu <[email protected]>");
+MODULE_DESCRIPTION("CPU LED trigger");
+MODULE_LICENSE("GPL");

include/linux/leds.h +0 -0

index 5884def..94cdb9a 100644
@@ -210,4 +210,19 @@ struct gpio_led_platform_data {

?struct platform_device *gpio_led_register_device(
? ? ? ? ? ? ? ?int id, const struct gpio_led_platform_data *pdata);

include/linux/leds.h +7 -0

+
+/* Use this routine to handle LEDs */
+extern void ledtrig_cpu(enum cpu_led_event evt);
+#else
+#define ledtrig_cpu(evt) do {} while (0)
+#endif
+

?#endif ? ? ? ? /* __LINUX_LEDS_H_INCLUDED */
--
1.7.5

Quoted 17 lines · Bryan Wu

+ ? ? ? switch (ledevt) {
+ ? ? ? case CPU_LED_START:
+ ? ? ? case CPU_LED_IDLE_START:
+ ? ? ? ? ? ? ? /* Will turn the LED on, max brightness */
+ ? ? ? ? ? ? ? if (trigdata->led)
+ ? ? ? ? ? ? ? ? ? ? ? led_set_brightness(trigdata->led,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?trigdata->led->max_brightness);
+ ? ? ? ? ? ? ? break;

(quoted diff) +9 -0

+
+ ? ? ? case CPU_LED_STOP:
+ ? ? ? case CPU_LED_IDLE_END:
+ ? ? ? case CPU_LED_HALTED:
+ ? ? ? ? ? ? ? /* Will turn the LED off */
+ ? ? ? ? ? ? ? if (trigdata->led)
+ ? ? ? ? ? ? ? ? ? ? ? led_set_brightness(trigdata->led, LED_OFF);
+ ? ? ? ? ? ? ? break;
+

Quoted 5 lines

+ ? ? ? default:
+ ? ? ? ? ? ? ? /* Will leave the LED as it is */
+ ? ? ? ? ? ? ? break;
+ ? ? ? }
+}

I'm a bit worrying about the LED brightness hardcoded here, what if a platform requires different behavior, e.g. LED_OFF when IDLE_START.

Quoted 18 lines · Eric Miao

Quoted 17 lines · Bryan Wu

+ ? ? ? switch (ledevt) {
+ ? ? ? case CPU_LED_START:
+ ? ? ? case CPU_LED_IDLE_START:
+ ? ? ? ? ? ? ? /* Will turn the LED on, max brightness */
+ ? ? ? ? ? ? ? if (trigdata->led)
+ ? ? ? ? ? ? ? ? ? ? ? led_set_brightness(trigdata->led,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?trigdata->led->max_brightness);
+ ? ? ? ? ? ? ? break;

(quoted diff) +9 -0

+
+ ? ? ? case CPU_LED_STOP:
+ ? ? ? case CPU_LED_IDLE_END:
+ ? ? ? case CPU_LED_HALTED:
+ ? ? ? ? ? ? ? /* Will turn the LED off */
+ ? ? ? ? ? ? ? if (trigdata->led)
+ ? ? ? ? ? ? ? ? ? ? ? led_set_brightness(trigdata->led, LED_OFF);
+ ? ? ? ? ? ? ? break;
+

Quoted 8 lines

Quoted 5 lines

+ ? ? ? default:
+ ? ? ? ? ? ? ? /* Will leave the LED as it is */
+ ? ? ? ? ? ? ? break;
+ ? ? ? }
+}

I'm a bit worrying about the LED brightness hardcoded here, what if a platform requires different behavior, e.g. LED_OFF when IDLE_START.

Indeed - there are platforms (Netwinder, Assabet etc) which use LED=ON to mean that the system is not idle - and also turn the LED on when halted.

Quoted 104 lines · Bryan Wu

Attempting to consolidate the ARM LED code, this removes the custom RealView LED trigger code to turn LEDs on and off in response to CPU activity and replace it with a standard trigger.

(bryan.wu at canonical.com:
It moves arch/arm/kernel/leds.c syscore stubs into this trigger. It also provides ledtrig_cpu trigger event stub in <linux/leds.h>. Although it was inspired by ARM work, it can be used in other arch.)

Cc Richard Purdie <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

Signed-off-by Bryan Wu <[email protected]>

---

?drivers/leds/Kconfig ? ? ? | ? 10 +++
?drivers/leds/Makefile ? ? ?| ? ?1 +
?drivers/leds/ledtrig-cpu.c | ?144 ++++++++++++++++++++++++++++++++++++++++++++
?include/linux/leds.h ? ? ? | ? 15 +++++

?4 files changed, 170 insertions(+), 0 deletions(-) ?create mode 100644 drivers/leds/ledtrig-cpu.c

drivers/leds/Kconfig +0 -0

index 713d43b..186000a 100644
@@ -446,6 +446,16 @@ config LEDS_TRIGGER_BACKLIGHT
 

? ? ? ? ?If unsure, say N.

drivers/leds/Kconfig +10 -0

+config LEDS_TRIGGER_CPU
+ ? ? ? bool "LED CPU Trigger"
+ ? ? ? depends on LEDS_TRIGGERS
+ ? ? ? help
+ ? ? ? ? This allows LEDs to be controlled by active CPUs. This shows
+ ? ? ? ? the active CPUs across an array of LEDs so you can see what
+ ? ? ? ? CPUs are active on the system at any given moment.
+
+ ? ? ? ? If unsure, say N.
+

?config LEDS_TRIGGER_GPIO
? ? ? ?tristate "LED GPIO Trigger"
? ? ? ?depends on LEDS_TRIGGERS

drivers/leds/Makefile +0 -0

index bbfd2e3..5e70d2e 100644
@@ -53,4 +53,5 @@ obj-$(CONFIG_LEDS_TRIGGER_IDE_DISK) ? += ledtrig-ide-disk.o

?obj-$(CONFIG_LEDS_TRIGGER_HEARTBEAT) ? += ledtrig-heartbeat.o ?obj-$(CONFIG_LEDS_TRIGGER_BACKLIGHT) ? += ledtrig-backlight.o ?obj-$(CONFIG_LEDS_TRIGGER_GPIO) ? ? ? ? ? ? ? ?+= ledtrig-gpio.o +obj-$(CONFIG_LEDS_TRIGGER_CPU) ? ? ? ? += ledtrig-cpu.o ?obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) ?+= ledtrig-default-on.o

drivers/leds/ledtrig-cpu.c new file +47 -0

new file mode 100644
index 0000000..4d63ada
@@ -0,0 +1,144 @@
+/*
+ * ledtrig-cpu.c - LED trigger based on CPU activity
+ *
+ * Copyright 2011 Linus Walleij <[email protected]>
+ * Copyright 2011 Bryan Wu <[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.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/percpu.h>
+#include <linux/syscore_ops.h>
+#include "leds.h"
+
+struct cpu_trig_data {
+ ? ? ? struct led_classdev *led;
+};
+
+static DEFINE_PER_CPU(struct cpu_trig_data *, cpu_triggers);
+
+void ledtrig_cpu(enum cpu_led_event ledevt)
+{
+ ? ? ? struct cpu_trig_data *trigdata = __get_cpu_var(cpu_triggers);
+
+ ? ? ? if (!trigdata)
+ ? ? ? ? ? ? ? return;
+
+ ? ? ? /* Locate the correct CPU LED */
+
+ ? ? ? switch (ledevt) {
+ ? ? ? case CPU_LED_START:
+ ? ? ? case CPU_LED_IDLE_START:
+ ? ? ? ? ? ? ? /* Will turn the LED on, max brightness */
+ ? ? ? ? ? ? ? if (trigdata->led)
+ ? ? ? ? ? ? ? ? ? ? ? led_set_brightness(trigdata->led,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?trigdata->led->max_brightness);
+ ? ? ? ? ? ? ? break;
+
+ ? ? ? case CPU_LED_STOP:
+ ? ? ? case CPU_LED_IDLE_END:
+ ? ? ? case CPU_LED_HALTED:

drivers/leds/ledtrig-cpu.c line 57

Quoted 10 lines

drivers/leds/ledtrig-cpu.c new file +10 -0

+ ? ? ? ? ? ? ? /* Will turn the LED off */
+ ? ? ? ? ? ? ? if (trigdata->led)
+ ? ? ? ? ? ? ? ? ? ? ? led_set_brightness(trigdata->led, LED_OFF);
+ ? ? ? ? ? ? ? break;
+
+ ? ? ? default:
+ ? ? ? ? ? ? ? /* Will leave the LED as it is */
+ ? ? ? ? ? ? ? break;
+ ? ? ? }
+}

I'm a bit worrying about the LED brightness hardcoded here, what if a platform requires different behavior, e.g. LED_OFF when IDLE_START.

Quoted 59 lines

drivers/leds/ledtrig-cpu.c new file +59 -0

+EXPORT_SYMBOL(ledtrig_cpu);
+
+static void cpu_trig_activate_cpu(void *data)
+{
+ ? ? ? struct cpu_trig_data *cpu_data;
+ ? ? ? struct led_classdev *led = data;
+ ? ? ? int my_cpu = smp_processor_id();
+ ? ? ? unsigned long target_cpu = (unsigned long) led->trigger_data;
+
+ ? ? ? if (target_cpu != my_cpu)
+ ? ? ? ? ? ? ? return;
+
+ ? ? ? cpu_data = kzalloc(sizeof(*cpu_data), GFP_KERNEL);
+ ? ? ? if (!cpu_data)
+ ? ? ? ? ? ? ? return;
+
+ ? ? ? dev_info(led->dev, "led %s indicate activity on CPU %d\n",
+ ? ? ? ? ? ? ? ?led->name, my_cpu);
+
+ ? ? ? cpu_data->led = led;
+ ? ? ? __get_cpu_var(cpu_triggers) = cpu_data;
+}
+
+static void cpu_trig_activate(struct led_classdev *led)
+{
+ ? ? ? on_each_cpu(cpu_trig_activate_cpu, led, 1);
+}
+
+static void cpu_trig_deactivate(struct led_classdev *led)
+{
+ ? ? ? struct cpu_trig_data *cpu_data = led->trigger_data;
+
+ ? ? ? kfree(cpu_data);
+}
+
+static struct led_trigger cpu_led_trigger = {
+ ? ? ? .name ? ? ? ? ? = "cpu",
+ ? ? ? .activate ? ? ? = cpu_trig_activate,
+ ? ? ? .deactivate ? ? = cpu_trig_deactivate,
+};
+
+
+static int cpu_syscore_suspend(void)
+{
+ ? ? ? ledtrig_cpu(CPU_LED_STOP);
+ ? ? ? return 0;
+}
+
+static void cpu_syscore_resume(void)
+{
+ ? ? ? ledtrig_cpu(CPU_LED_START);
+}
+
+static void cpu_syscore_shutdown(void)
+{
+ ? ? ? ledtrig_cpu(CPU_LED_HALTED);
+}
+
+static struct syscore_ops cpu_syscore_ops = {

drivers/leds/ledtrig-cpu.c line 120

Quoted 4 lines

drivers/leds/ledtrig-cpu.c new file +4 -0

+ ? ? ? .shutdown ? ? ? = cpu_syscore_shutdown,
+ ? ? ? .suspend ? ? ? ?= cpu_syscore_suspend,
+ ? ? ? .resume ? ? ? ? = cpu_syscore_resume,
+};

Even this is static, I think naming of ledtrig_cpu_syscore_* will be better here, causing less confusion. As cpu_syscore_shutdown really gave me a first impression it's about the action when the CPU shuts down.

Quoted 9 lines

drivers/leds/ledtrig-cpu.c new file +9 -0

+
+static int __init cpu_trig_init(void)
+{
+ ? ? ? int ret;
+
+ ? ? ? ret = led_trigger_register(&cpu_led_trigger);
+ ? ? ? if (!ret)
+ ? ? ? ? ? ? ? register_syscore_ops(&cpu_syscore_ops);
+

drivers/leds/ledtrig-cpu.c line 139

Quoted 10 lines

drivers/leds/ledtrig-cpu.c new file +10 -0

+ ? ? ? return ret;
+}
+module_init(cpu_trig_init);
+
+static void __exit cpu_trig_exit(void)
+{
+ ? ? ? unregister_syscore_ops(&cpu_syscore_ops);
+ ? ? ? led_trigger_unregister(&cpu_led_trigger);
+}
+module_exit(cpu_trig_exit);

ledtrig_cpu_exit?

Quoted 13 lines

drivers/leds/ledtrig-cpu.c new file +5 -0

+
+MODULE_AUTHOR("Linus Walleij <[email protected]>");
+MODULE_AUTHOR("Bryan Wu <[email protected]>");
+MODULE_DESCRIPTION("CPU LED trigger");
+MODULE_LICENSE("GPL");

include/linux/leds.h +0 -0

index 5884def..94cdb9a 100644
@@ -210,4 +210,19 @@ struct gpio_led_platform_data {

?struct platform_device *gpio_led_register_device(
? ? ? ? ? ? ? ?int id, const struct gpio_led_platform_data *pdata);

include/linux/leds.h

Quoted 8 lines

include/linux/leds.h +8 -0

+#ifdef CONFIG_LEDS_TRIGGER_CPU
+enum cpu_led_event {
+ ? ? ? CPU_LED_IDLE_START,
+ ? ? ? CPU_LED_IDLE_END,
+ ? ? ? CPU_LED_START,
+ ? ? ? CPU_LED_STOP,
+ ? ? ? CPU_LED_HALTED
+};

A brief block of comments explaining these events would be very helpful for passing correct parameters to ledtrig_cpu()

Quoted 7 lines

include/linux/leds.h +7 -0

+
+/* Use this routine to handle LEDs */
+extern void ledtrig_cpu(enum cpu_led_event evt);
+#else
+#define ledtrig_cpu(evt) do {} while (0)
+#endif
+

Quoted 5 lines

?#endif ? ? ? ? /* __LINUX_LEDS_H_INCLUDED */
--
1.7.5

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

arch/arm/plat-versatile/Kconfig +0 -0

index 52353be..c600420 100644

Quoted 7 lines · Bryan Wu

arch/arm/plat-versatile/Kconfig +0 -0

index 52353be..c600420 100644
@@ -7,8 +7,12 @@ config PLAT_VERSATILE_FPGA_IRQ

? ? ? bool

Quoted 8 lines

?config PLAT_VERSATILE_LEDS
- ? ? def_bool y if LEDS_CLASS
+ ? ? def_bool y
? ? ? depends on ARCH_REALVIEW || ARCH_VERSATILE
+ ? ? select NEW_LEDS
+ ? ? select LEDS_CLASS
+ ? ? select LEDS_TRIGGERS
+ ? ? select LEDS_TRIGGER_CPU

Can you explain the choice to force LED support to be enabled with no choice for these?

From: Linus Walleij <[email protected]>

The CPU activity LED is now handled by the trigger in the leds subsystem, retire this old CONFIG_LEDS-based code.

Quoted 4 lines · Bryan Wu

From: Linus Walleij <[email protected]>

The CPU activity LED is now handled by the trigger in the leds subsystem, retire this old CONFIG_LEDS-based code.

Shouldn't this be part of the previous patch? ?The previous patch changes both Realview and Versatile by touching plat-versatile.

Quoted 4 lines · Russell King - ARM Linux

Shouldn't this be part of the previous patch? ?The previous patch changes both Realview and Versatile by touching plat-versatile.

I will fold them together, if Linus has no problem about it.

Signed-off-by Bryan Wu <[email protected]>

Quoted 1 line

Signed-off-by Bryan Wu<[email protected]>

As the original author of this code.

Acked-by Andrew Victor <[email protected]>

Hello.

Quoted 1 line · Andrew Victor

Quoted 1 line

Signed-off-by Bryan Wu<[email protected]>

Quoted 3 lines

As the original author of this code.

Acked-by Andrew Victor <[email protected]>

? If you're the original author, shouldn't there be:

From: Andrew Victor <[email protected]>

Quoted 8 lines · Sergei Shtylyov

Hello.

Quoted 3 lines · Andrew Victor

Quoted 1 line

Signed-off-by Bryan Wu<[email protected]>

As the original author of this code.

Quoted 6 lines

Quoted 1 line

Acked-by Andrew Victor <[email protected]>

? If you're the original author, shouldn't there be:

From: Andrew Victor <[email protected]>

No problem, I will change that.

Quoted 16 lines · Bryan Wu

Quoted 14 lines · Sergei Shtylyov

Hello.

Quoted 5 lines · Andrew Victor

Quoted 1 line

Signed-off-by Bryan Wu<[email protected]>

As the original author of this code.

Acked-by Andrew Victor <[email protected]>

? If you're the original author, shouldn't there be:

From: Andrew Victor <[email protected]>

Quoted 1 line

No problem, I will change that.

Beware. ?Is Andrew Victor the original author of the code being removed here and in this case he's indicating his agreement to the removal of his code, or he is the one actually responsible for the removal? ?If the former then it is wrong to put a "From" with his name and the "Acked-by:" is the right thing to do.

Quoted 6 lines

Beware. ?Is Andrew Victor the original author of the code being removed here and in this case he's indicating his agreement to the removal of his code, or he is the one actually responsible for the removal? ?If the former then it is wrong to put a "From" with his name and the "Acked-by:" is the right thing to do.

Right, I was confused by the email. I prepared the removing patch and Andrew said "Acked-by".
Andrew, is that what you want?

arch/arm/Kconfig +0 -0

index 9adc278..df971c7 100644

Quoted 9 lines · Bryan Wu

arch/arm/Kconfig +0 -0

index 9adc278..df971c7 100644
@@ -783,6 +783,12 @@ config ARCH_SHARK

? ? ? select ZONE_DMA
? ? ? select PCI
? ? ? select ARCH_USES_GETTIMEOFFSET
+ ? ? select NEW_LEDS

Quoted 5 lines

+ ? ? select LEDS_CLASS
+ ? ? select LEDS_TRIGGERS
+ ? ? select LEDS_TRIGGER_CPU
+ ? ? select LEDS_TRIGGER_HEARTBEAT
+ ? ? select LEDS_TRIGGER_DEFAULT_ON

So Shark too gets forced to have LED support no matter what. ?Can we please stop this madness.

Signed-off-by Bryan Wu <[email protected]>

---
 arch/arm/configs/orion5x_defconfig           |    1 +
 arch/arm/mach-orion5x/rd88f5181l-fxo-setup.c |    1 -
 arch/arm/mach-orion5x/rd88f5181l-ge-setup.c  |    1 -
 arch/arm/mach-orion5x/rd88f5182-setup.c      |   74 ++++++++------------------
 arch/arm/mach-orion5x/rd88f6183ap-ge-setup.c |    1 -
 5 files changed, 24 insertions(+), 54 deletions(-)

arch/arm/configs/orion5x_defconfig +1 -0

index a288d70..6579baf 100644
@@ -126,6 +126,7 @@ CONFIG_LEDS_CLASS=y
 CONFIG_LEDS_GPIO=y
 CONFIG_LEDS_TRIGGERS=y
 CONFIG_LEDS_TRIGGER_TIMER=y
+CONFIG_LEDS_TRIGGER_CPU=y
 CONFIG_LEDS_TRIGGER_HEARTBEAT=y
 CONFIG_LEDS_TRIGGER_DEFAULT_ON=y
 CONFIG_RTC_CLASS=y

arch/arm/mach-orion5x/rd88f5181l-fxo-setup.c +0 -1

index 9eec7c2..ddb6be1 100644
@@ -19,7 +19,6 @@
 #include <net/dsa.h>
 #include <asm/mach-types.h>
 #include <asm/gpio.h>
-#include <asm/leds.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/pci.h>
 #include <mach/orion5x.h>

arch/arm/mach-orion5x/rd88f5181l-ge-setup.c +0 -1

index 0cc90bb..454f666 100644
@@ -20,7 +20,6 @@
 #include <net/dsa.h>
 #include <asm/mach-types.h>
 #include <asm/gpio.h>
-#include <asm/leds.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/pci.h>
 #include <mach/orion5x.h>

arch/arm/mach-orion5x/rd88f5182-setup.c +23 -51

index 48da39b..200e02c 100644
@@ -19,9 +19,9 @@
 #include <linux/mv643xx_eth.h>
 #include <linux/ata_platform.h>
 #include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/leds.h>
 #include <asm/mach-types.h>
-#include <asm/gpio.h>
-#include <asm/leds.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/pci.h>
 #include <mach/orion5x.h>

@@ -54,12 +54,6 @@
 #define RD88F5182_PCI_SLOT0_IRQ_A_PIN	7
 #define RD88F5182_PCI_SLOT0_IRQ_B_PIN	6
 
-/*
- * GPIO Debug LED
- */
-
-#define RD88F5182_GPIO_DBG_LED		0
-
 /*****************************************************************************
  * 16M NOR Flash on Device bus CS1
  ****************************************************************************/

@@ -84,55 +78,32 @@ static struct platform_device rd88f5182_nor_flash = {
 	.resource		= &rd88f5182_nor_flash_resource,
 };
 
-#ifdef CONFIG_LEDS
-
 /*****************************************************************************
- * Use GPIO debug led as CPU active indication
+ * Use GPIO LED as CPU active indication
  ****************************************************************************/
 
-static void rd88f5182_dbgled_event(led_event_t evt)
-{
-	int val;
-
-	if (evt == led_idle_end)
-		val = 1;
-	else if (evt == led_idle_start)
-		val = 0;
-	else
-		return;
-
-	gpio_set_value(RD88F5182_GPIO_DBG_LED, val);
-}
-
-static int __init rd88f5182_dbgled_init(void)
-{
-	int pin;
-
-	if (machine_is_rd88f5182()) {
-		pin = RD88F5182_GPIO_DBG_LED;
+#define RD88F5182_GPIO_LED		0
 
-		if (gpio_request(pin, "DBGLED") == 0) {
-			if (gpio_direction_output(pin, 0) != 0) {
-				printk(KERN_ERR "rd88f5182_dbgled_init failed "
-						"to set output pin %d\n", pin);
-				gpio_free(pin);
-				return 0;
-			}
-		} else {
-			printk(KERN_ERR "rd88f5182_dbgled_init failed "
-					"to request gpio %d\n", pin);
-			return 0;
-		}
-
-		leds_event = rd88f5182_dbgled_event;
-	}
-
-	return 0;
-}
+static struct gpio_led rd88f5182_gpio_led_pins[] = {
+	{
+		.name		= "rd88f5182:cpu",
+		.default_trigger = "cpu",
+		.gpio		= RD88F5182_GPIO_LED,
+	},
+};
 
-__initcall(rd88f5182_dbgled_init);
+static struct gpio_led_platform_data rd88f5182_gpio_led_data = {
+	.leds		= rd88f5182_gpio_led_pins,
+	.num_leds	= ARRAY_SIZE(rd88f5182_gpio_led_pins),
+};
 
-#endif
+static struct platform_device rd88f5182_gpio_leds = {
+	.name	= "leds-gpio",
+	.id	= -1,
+	.dev	= {
+		.platform_data = &rd88f5182_gpio_led_data,
+	},
+};
 
 /*****************************************************************************
  * PCI

@@ -299,6 +270,7 @@ static void __init rd88f5182_init(void)
 
 	orion5x_setup_dev1_win(RD88F5182_NOR_BASE, RD88F5182_NOR_SIZE);
 	platform_device_register(&rd88f5182_nor_flash);
+	platform_device_register(&rd88f5182_gpio_leds);
 
 	i2c_register_board_info(0, &rd88f5182_i2c_rtc, 1);
 }

arch/arm/mach-orion5x/rd88f6183ap-ge-setup.c +0 -1

index ad2eba9..0c5d1d2 100644
@@ -22,7 +22,6 @@
 #include <net/dsa.h>
 #include <asm/mach-types.h>
 #include <asm/gpio.h>
-#include <asm/leds.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/pci.h>
 #include <mach/orion5x.h>

Signature · 1 line

1.7.5

Use 4 LEDs in baseboard instead of the LED in core module.

The CPU activity LED is now handled by the trigger in the leds subsystem, retire this old CONFIG_LEDS-based code.

Quoted 4 lines · Bryan Wu

Use 4 LEDs in baseboard instead of the LED in core module.

The CPU activity LED is now handled by the trigger in the leds subsystem, retire this old CONFIG_LEDS-based code.

This is a functional change. ?Integrator CP has four baseboard LEDs. I don't remember Integrator AP having those,

but it does have the LED on the core module, which is used for CPU activity on all Integrator platforms.

Signed-off-by Bryan Wu <[email protected]>

Quoted 1 line · Bryan Wu

Signed-off-by Bryan Wu <[email protected]>

Same comments again.

Signed-off-by Bryan Wu <[email protected]>

Quoted 1 line · Bryan Wu

Signed-off-by Bryan Wu <[email protected]>

Same comments.

Signed-off-by Bryan Wu <[email protected]>

arch/arm/mach-sa1100/simpad.c +9 -0

index cfb7607..6f87258 100644
@@ -13,6 +13,8 @@
  #include<linux/mtd/mtd.h>
  #include<linux/mtd/partitions.h>
  #include<linux/io.h>
+#include<linux/leds.h>
+#include<linux/slab.h>
 
  #include<asm/irq.h>
  #include<mach/hardware.h>

@@ -205,7 +207,53 @@ static struct platform_device *devices[] __initdata = {
  	&simpad_mq200fb
  };
 
+/* LEDs */
+#define LED_GREEN	1
 
+static void simpad_led_set(struct led_classdev *cdev,
+			      enum led_brightness b)
+{
+	if (b != LED_OFF)
+		set_cs3_bit(LED_GREEN);

Quoted 33 lines · Bryan Wu

arch/arm/mach-sa1100/simpad.c +0 -0

index cfb7607..6f87258 100644
@@ -13,6 +13,8 @@

?#include<linux/mtd/mtd.h>
?#include<linux/mtd/partitions.h>
?#include<linux/io.h>

arch/arm/mach-sa1100/simpad.c +2 -0

+#include<linux/leds.h>
+#include<linux/slab.h>

?#include<asm/irq.h>
?#include<mach/hardware.h>

arch/arm/mach-sa1100/simpad.c +0 -0

@@ -205,7 +207,53 @@ static struct platform_device *devices[] __initdata =
 {

? ? ? ?&simpad_mq200fb
?};

arch/arm/mach-sa1100/simpad.c +14 -0

+/* LEDs */
+#define LED_GREEN ? ? ?1
 
+static void simpad_led_set(struct led_classdev *cdev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? enum led_brightness b)
+{
+ ? ? ? if (b != LED_OFF)
+ ? ? ? ? ? ? ? set_cs3_bit(LED_GREEN);
+ ? ? ? else
+ ? ? ? ? ? ? ? clear_cs3_bit(LED_GREEN);
+}
+
+static enum led_brightness simpad_led_get(struct led_classdev *cdev)
+{
+ ? ? ? u32 reg = *(CS3BUSTYPE *)(CS3_BASE);

arch/arm/mach-sa1100/simpad.c

Quoted 3 lines

arch/arm/mach-sa1100/simpad.c +3 -0

+
+ ? ? ? return (reg& ?LED_GREEN) ? LED_FULL : LED_OFF;
+}

NACK, bit 1 on the CS3 register is PCMCIA power control (write) and PCMCIA BVD1 (read), but not LED_GREEN. LED2_ON would be correct for write, but you would need to read cs3_shadow instead of the register itself for the LED status.

BTW: Some time ago I wrote a similar patch for ARM that converts CS3 functions to GPIOs, wich simplifies the LED stuff even further ;-)

http://www.spinics.net/lists/arm-kernel/msg122479.html

This looks good and it's the right way to do that. We need gpiolib supporting firstly. How's status of your patchset, I guess you're trying to update it.

Hi Bryan,

Quoted 3 lines

This looks good and it's the right way to do that. We need gpiolib supporting firstly. How's status of your patchset, I guess you're trying to update it.

I've resubmitted it yesterday for another review. In parallel, I'll retest this on top of 3.0-rc5 on real hardware and ask Russell if it's OK to submit to his patch tracker.

Quoted 3 lines

I've resubmitted it yesterday for another review. In parallel, I'll retest this on top of 3.0-rc5 on real hardware and ask Russell if it's OK to submit to his patch tracker.

Great, I will remove my modification of simpad code in my patchset.

@Russell: is it OK to submit this to the patch tracking system?

arch/arm/mach-sa1100/assabet.c +65 -0

index 5778274..13e4e5d 100644
@@ -19,6 +19,8 @@
 #include <linux/mtd/partitions.h>
 #include <linux/delay.h>
 #include <linux/mm.h>
+#include <linux/leds.h>
+#include <linux/slab.h>
 
 #include <mach/hardware.h>
 #include <asm/mach-types.h>

@@ -445,6 +447,79 @@ static void __init assabet_map_io(void)
 	sa1100_register_uart(2, 3);
 }
 
+/* LEDs */
+struct assabet_led {
+	struct led_classdev cdev;
+	u32 mask;
+};
+
+/*
+ * The triggers lines up below will only be used if the
+ * LED triggers are compiled in.
+ */
+static const struct {
+	const char *name;
+	const char *trigger;
+} assabet_leds[] = {
+	{ "assabet:red", "cpu",},
+	{ "assabet:green", "heartbeat", },
+};
+
+static void assabet_led_set(struct led_classdev *cdev,
+		enum led_brightness b)
+{
+	struct assabet_led *led = container_of(cdev,
+			struct assabet_led, cdev);
+
+	if (b != LED_OFF)
+		ASSABET_BCR_clear(led->mask);
+	else
+		ASSABET_BCR_set(led->mask);
+}
+
+static enum led_brightness assabet_led_get(struct led_classdev *cdev)
+{
+	struct assabet_led *led = container_of(cdev,
+			struct assabet_led, cdev);
+
+	return (ASSABET_BCR & led->mask) ? LED_OFF : LED_FULL;
+}
+
+static int __init assabet_leds_init(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(assabet_leds); i++) {
+		struct assabet_led *led;
+
+		led = kzalloc(sizeof(*led), GFP_KERNEL);
+		if (!led)
+			break;
+
+		led->cdev.name = assabet_leds[i].name;
+		led->cdev.brightness_set = assabet_led_set;
+		led->cdev.brightness_get = assabet_led_get;
+		led->cdev.default_trigger = assabet_leds[i].trigger;
+
+		if (!i)
+			led->mask = ASSABET_BCR_LED_RED;
+		else
+			led->mask = ASSABET_BCR_LED_GREEN;
+
+		if (led_classdev_register(NULL, &led->cdev) < 0) {
+			kfree(led);
+			break;
+		}

arch/arm/mach-sa1100/simpad.c +38 -0

index cfb7607..6f87258 100644
@@ -13,6 +13,8 @@
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 #include <linux/io.h>
+#include <linux/leds.h>
+#include <linux/slab.h>
 
 #include <asm/irq.h>
 #include <mach/hardware.h>

@@ -205,7 +207,53 @@ static struct platform_device *devices[] __initdata = {
 	&simpad_mq200fb
 };
 
+/* LEDs */
+#define LED_GREEN	1
 
+static void simpad_led_set(struct led_classdev *cdev,
+			      enum led_brightness b)
+{
+	if (b != LED_OFF)
+		set_cs3_bit(LED_GREEN);
+	else
+		clear_cs3_bit(LED_GREEN);
+}
+
+static enum led_brightness simpad_led_get(struct led_classdev *cdev)
+{
+	u32 reg = *(CS3BUSTYPE *)(CS3_BASE);
+
+	return (reg & LED_GREEN) ? LED_FULL : LED_OFF;
+}
+
+static int __init simpad_leds_init(void)
+{
+	struct led_classdev *simpad_led;
+	int ret;
+
+	simpad_led = kzalloc(sizeof(*simpad_led), GFP_KERNEL);
+	if (!simpad_led)
+		return -ENOMEM;
+
+	simpad_led->name = "simpad:green";
+	simpad_led->brightness_set = simpad_led_set;
+	simpad_led->brightness_get = simpad_led_get;
+	simpad_led->default_trigger = "heartbeat";
+
+	ret = led_classdev_register(NULL, simpad_led);
+	if (ret < 0) {
+		kfree(simpad_led);
+		return ret;

Quoted 1 line

+fs_initcall(simpad_leds_init);

Looks like simpad is similarly afflicted. I haven't gone through the rest to check for similar issues.

Hi Russell,

Quoted 4 lines

Quoted 1 line

+fs_initcall(simpad_leds_init);

Looks like simpad is similarly afflicted. ?I haven't gone through the rest to check for similar issues.

simpad is buggy in other means, as well (wrong bit, read of write-only register, missing locking). That's why I want to push my CS3 cleanup patch first, converting a custom IO to standard GPIO ;-)

Quoted 87 lines · Bryan Wu

arch/arm/mach-sa1100/assabet.c +0 -0

index 5778274..13e4e5d 100644
@@ -19,6 +19,8 @@

?#include <linux/mtd/partitions.h>
?#include <linux/delay.h>
?#include <linux/mm.h>

arch/arm/mach-sa1100/assabet.c +2 -0

+#include <linux/leds.h>
+#include <linux/slab.h>

?#include <mach/hardware.h>
?#include <asm/mach-types.h>

arch/arm/mach-sa1100/assabet.c +70 -0

@@ -445,6 +447,79 @@ static void __init assabet_map_io(void)
 ? ? ? sa1100_register_uart(2, 3);
 ?}
 
+/* LEDs */
+struct assabet_led {
+ ? ? struct led_classdev cdev;
+ ? ? u32 mask;
+};
+
+/*
+ * The triggers lines up below will only be used if the
+ * LED triggers are compiled in.
+ */
+static const struct {
+ ? ? const char *name;
+ ? ? const char *trigger;
+} assabet_leds[] = {
+ ? ? { "assabet:red", "cpu",},
+ ? ? { "assabet:green", "heartbeat", },
+};
+
+static void assabet_led_set(struct led_classdev *cdev,
+ ? ? ? ? ? ? enum led_brightness b)
+{
+ ? ? struct assabet_led *led = container_of(cdev,
+ ? ? ? ? ? ? ? ? ? ? struct assabet_led, cdev);
+
+ ? ? if (b != LED_OFF)
+ ? ? ? ? ? ? ASSABET_BCR_clear(led->mask);
+ ? ? else
+ ? ? ? ? ? ? ASSABET_BCR_set(led->mask);
+}
+
+static enum led_brightness assabet_led_get(struct led_classdev *cdev)
+{
+ ? ? struct assabet_led *led = container_of(cdev,
+ ? ? ? ? ? ? ? ? ? ? struct assabet_led, cdev);
+
+ ? ? return (ASSABET_BCR & led->mask) ? LED_OFF : LED_FULL;
+}
+
+static int __init assabet_leds_init(void)
+{
+ ? ? int i;
+
+ ? ? for (i = 0; i < ARRAY_SIZE(assabet_leds); i++) {
+ ? ? ? ? ? ? struct assabet_led *led;
+
+ ? ? ? ? ? ? led = kzalloc(sizeof(*led), GFP_KERNEL);
+ ? ? ? ? ? ? if (!led)
+ ? ? ? ? ? ? ? ? ? ? break;
+
+ ? ? ? ? ? ? led->cdev.name = assabet_leds[i].name;
+ ? ? ? ? ? ? led->cdev.brightness_set = assabet_led_set;
+ ? ? ? ? ? ? led->cdev.brightness_get = assabet_led_get;
+ ? ? ? ? ? ? led->cdev.default_trigger = assabet_leds[i].trigger;
+
+ ? ? ? ? ? ? if (!i)
+ ? ? ? ? ? ? ? ? ? ? led->mask = ASSABET_BCR_LED_RED;
+ ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? led->mask = ASSABET_BCR_LED_GREEN;
+
+ ? ? ? ? ? ? if (led_classdev_register(NULL, &led->cdev) < 0) {
+ ? ? ? ? ? ? ? ? ? ? kfree(led);
+ ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? }
+ ? ? }
+
+ ? ? return 0;
+}
+
+/*
+ * Since we may have triggers on any subsystem, defer registration

arch/arm/mach-sa1100/assabet.c static void __init assabet_map_io(void) · line 522

Quoted 3 lines

arch/arm/mach-sa1100/assabet.c +3 -0

+ * until after subsystem_init.
+ */
+fs_initcall(assabet_leds_init);

This is buggy. ?init functions are called irrespective of what platform we're running on, so this means if assabet is built in to the kernel, then we'll register its LEDs and try to access the BCR register irrespective of whether we are on assabet or not.

Quoted 54 lines

arch/arm/mach-sa1100/simpad.c +0 -0

index cfb7607..6f87258 100644
@@ -13,6 +13,8 @@

?#include <linux/mtd/mtd.h>
?#include <linux/mtd/partitions.h>
?#include <linux/io.h>

arch/arm/mach-sa1100/simpad.c +2 -0

+#include <linux/leds.h>
+#include <linux/slab.h>

?#include <asm/irq.h>
?#include <mach/hardware.h>

arch/arm/mach-sa1100/simpad.c +0 -0

@@ -205,7 +207,53 @@ static struct platform_device *devices[] __initdata = {

? ? ? &simpad_mq200fb
?};

arch/arm/mach-sa1100/simpad.c +36 -0

+/* LEDs */
+#define LED_GREEN ? ?1
 
+static void simpad_led_set(struct led_classdev *cdev,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? enum led_brightness b)
+{
+ ? ? if (b != LED_OFF)
+ ? ? ? ? ? ? set_cs3_bit(LED_GREEN);
+ ? ? else
+ ? ? ? ? ? ? clear_cs3_bit(LED_GREEN);
+}
+
+static enum led_brightness simpad_led_get(struct led_classdev *cdev)
+{
+ ? ? u32 reg = *(CS3BUSTYPE *)(CS3_BASE);
+
+ ? ? return (reg & LED_GREEN) ? LED_FULL : LED_OFF;
+}
+
+static int __init simpad_leds_init(void)
+{
+ ? ? struct led_classdev *simpad_led;
+ ? ? int ret;
+
+ ? ? simpad_led = kzalloc(sizeof(*simpad_led), GFP_KERNEL);
+ ? ? if (!simpad_led)
+ ? ? ? ? ? ? return -ENOMEM;
+
+ ? ? simpad_led->name = "simpad:green";
+ ? ? simpad_led->brightness_set = simpad_led_set;
+ ? ? simpad_led->brightness_get = simpad_led_get;
+ ? ? simpad_led->default_trigger = "heartbeat";
+
+ ? ? ret = led_classdev_register(NULL, simpad_led);
+ ? ? if (ret < 0) {
+ ? ? ? ? ? ? kfree(simpad_led);
+ ? ? ? ? ? ? return ret;

arch/arm/mach-sa1100/simpad.c static struct platform_device *devices[] __initdata = { · line 256

Quoted 10 lines

arch/arm/mach-sa1100/simpad.c +10 -0

+ ? ? }
+
+ ? ? return 0;
+}
+
+/*
+ * Since we may have triggers on any subsystem, defer registration
+ * until after subsystem_init.
+ */
+fs_initcall(simpad_leds_init);

Looks like simpad is similarly afflicted. ?I haven't gone through the rest to check for similar issues.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Right, I got it. So if we are going to run a single binary for all the sa1100 machines, how about we check the machine in each fs_initcall

static int __init assabet_leds_init(void)
{
? ? int i;

     if (!machine_is_assabet())
          return -ENODEV,

? ? for (i = 0; i < ARRAY_SIZE(assabet_leds); i++) { ? ? ? ? ? ? struct assabet_led *led;

Quoted 7 lines · Bryan Wu

Right, I got it. So if we are going to run a single binary for all the sa1100 machines, how about we check the machine in each fs_initcall

static int __init assabet_leds_init(void)

{
     int i;

Quoted 5 lines

     if (!machine_is_assabet())
          return -ENODEV,

     for (i = 0; i < ARRAY_SIZE(assabet_leds); i++) {
             struct assabet_led *led;

Yes, that'll do, thanks.

Quoted 8 lines · Russell King - ARM Linux

Quoted 7 lines · Bryan Wu

Right, I got it. So if we are going to run a single binary for all the sa1100 machines, how about we check the machine in each fs_initcall

static int __init assabet_leds_init(void)
{
? ? ?int i;

Quoted 8 lines

Quoted 5 lines

? ? ?if (!machine_is_assabet())
? ? ? ? ? return -ENODEV,

? ? ?for (i = 0; i < ARRAY_SIZE(assabet_leds); i++) { ? ? ? ? ? ? ?struct assabet_led *led;

Yes, that'll do, thanks.

Do you think it's worth copying the logic from powerpc for this?

arch/powerpc/include/asm/machdep.h:
#define __define_machine_initcall(mach,level,fn,id) \
? ? ? ?static int __init __machine_initcall_##mach##_##fn(void) { \
? ? ? ? ? ? ? ?if (machine_is(mach)) return fn(); \
? ? ? ? ? ? ? ?return 0; \
? ? ? ?} \
? ? ? ?__define_initcall(level,__machine_initcall_##mach##_##fn,id);
#define machine_core_initcall(mach,fn) ? ? ? ? ?__define_machine_initcall(mach,"1",fn,1) #define machine_core_initcall_sync(mach,fn) ? ? __define_machine_initcall(mach,"1s",fn,1s) #define machine_postcore_initcall(mach,fn) ? ? ?__define_machine_initcall(mach,"2",fn,2) #define machine_postcore_initcall_sync(mach,fn) __define_machine_initcall(mach,"2s",fn,2s) #define machine_arch_initcall(mach,fn) ? ? ? ? ?__define_machine_initcall(mach,"3",fn,3) #define machine_arch_initcall_sync(mach,fn) ? ? __define_machine_initcall(mach,"3s",fn,3s) #define machine_subsys_initcall(mach,fn) ? ? ? ?__define_machine_initcall(mach,"4",fn,4) #define machine_subsys_initcall_sync(mach,fn) ? __define_machine_initcall(mach,"4s",fn,4s) #define machine_fs_initcall(mach,fn) ? ? ? ? ? ?__define_machine_initcall(mach,"5",fn,5) #define machine_fs_initcall_sync(mach,fn) ? ? ? __define_machine_initcall(mach,"5s",fn,5s) #define machine_rootfs_initcall(mach,fn) ? ? ? ?__define_machine_initcall(mach,"rootfs",fn,rootfs) #define machine_device_initcall(mach,fn) ? ? ? ?__define_machine_initcall(mach,"6",fn,6) #define machine_device_initcall_sync(mach,fn) ? __define_machine_initcall(mach,"6s",fn,6s) #define machine_late_initcall(mach,fn) ? ? ? ? ?__define_machine_initcall(mach,"7",fn,7) #define machine_late_initcall_sync(mach,fn) ? ? __define_machine_initcall(mach,"7s",fn,7s)

I don't really like the magic macros, but my feeling is that it's easier to do the conversion without introducing subtle bugs this way.