[PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[kogiidena]

To:Richard-san
CC:all

LED driver of I-O DATA LANDISK and USL-5P
Please apply following patch

Signed-off-by: kogiidena <kogiidena@eggplant.ddo.jp>

---
diff -urpN linux-2.6.21-rc4.orig/drivers/leds/Kconfig NEW/drivers/leds/Kconfig
--- linux-2.6.21-rc4.orig/drivers/leds/Kconfig        2007-03-16 09:20:01.000000000 +0900
+++ NEW/drivers/leds/Kconfig        2007-03-27 18:30:44.000000000 +0900
@@ -94,6 +94,13 @@ config LEDS_COBALT
         help
           This option enables support for the front LED on Cobalt Server

+config LEDS_LANDISK
+        tristate "LED Support for LANDISK Series"
+        depends on LEDS_CLASS && SH_LANDISK
+        select LEDS_TRIGGERS
+        help
+          This option enables support for the LED on LANDISK Series
+
 comment "LED Triggers"

 config LEDS_TRIGGERS
diff -urpN linux-2.6.21-rc4.orig/drivers/leds/Makefile NEW/drivers/leds/Makefile
--- linux-2.6.21-rc4.orig/drivers/leds/Makefile        2007-03-16 09:20:01.000000000 +0900
+++ NEW/drivers/leds/Makefile        2007-03-27 18:27:18.000000000 +0900
@@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_NET48XX)                += leds-net4
 obj-$(CONFIG_LEDS_WRAP)                        += leds-wrap.o
 obj-$(CONFIG_LEDS_H1940)                += leds-h1940.o
 obj-$(CONFIG_LEDS_COBALT)                += leds-cobalt.o
+obj-$(CONFIG_LEDS_LANDISK)                += leds-landisk.o

 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)        += ledtrig-timer.o
diff -urpN linux-2.6.21-rc4.orig/drivers/leds/leds-landisk.c NEW/drivers/leds/leds-landisk.c
--- linux-2.6.21-rc4.orig/drivers/leds/leds-landisk.c        1970-01-01 09:00:00.000000000 +0900
+++ NEW/drivers/leds/leds-landisk.c        2007-03-27 18:16:48.000000000 +0900
@@ -0,0 +1,183 @@
+/*
+ * LEDs driver for I-O DATA DEVICE, INC. "LANDISK Series" support.
+ *
+ * Copyright (C) 2007 kogiidena
+ *
+ * Based on the drivers/leds/leds-ams-delta.c by:
+ * Copyright (C) 2006 Jonathan McDowell <noodles@earth.li>
+ *
+ * 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/init.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/leds.h>
+#include <asm/io.h>
+#include <asm/landisk/iodata_landisk.h>
+
+spinlock_t landisk_led_lock;
+static int landisk_arch;                /* 0:LANDISK, LANTank 1:USL-5P */
+
+static void landisk_led_set(struct led_classdev *led_cdev,
+                            enum led_brightness value);
+
+static struct led_classdev landisk_leds[] = {
+        [0] = {
+                        .name = "power",
+                        .brightness_set = landisk_led_set,
+                        .default_trigger = "blink",
+               },
+        [1] = {
+                        .name = "status",
+                        .brightness_set = landisk_led_set,
+                        .default_trigger = "blink",
+               },
+        [2] = {
+                        .name = "led1",
+                        .brightness_set = landisk_led_set,
+               },
+        [3] = {
+                        .name = "led2",
+                        .brightness_set = landisk_led_set,
+               },
+        [4] = {
+                        .name = "led3",
+                        .brightness_set = landisk_led_set,
+               },
+        [5] = {
+                        .name = "led4",
+                        .brightness_set = landisk_led_set,
+               },
+        [6] = {
+                        .name = "led5",
+                        .brightness_set = landisk_led_set,
+               },
+        [7] = {
+                        .name = "buzzer",
+                        .default_trigger = "bitshift",
+                        .brightness_set = landisk_led_set,
+               },
+};
+
+static void landisk_led_set(struct led_classdev *led_cdev,
+                            enum led_brightness value)
+{
+        u8 tmp;
+        int bitmask;
+        unsigned long flags;
+
+        bitmask = 0x01 << (led_cdev - &landisk_leds[0]);
+
+        spin_lock_irqsave(&landisk_led_lock, flags);
+        tmp = ctrl_inb(PA_LED);
+        if (value)
+                tmp |=  bitmask;
+        else
+                tmp &= ~bitmask;
+        ctrl_outb(tmp, PA_LED);
+        spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static int landisk_led_probe(struct platform_device *pdev)
+{
+        int i, nr_leds;
+        int ret;
+
+        nr_leds = landisk_arch ? 8 : 2;
+
+        for (i = ret = 0; ret >= 0 && i < nr_leds; i++) {
+                ret = led_classdev_register(&pdev->dev, &landisk_leds[i]);
+        }
+
+        if (ret < 0 && i > 1) {
+                nr_leds = i - 1;
+                for (i = 0; i < nr_leds; i++)
+                        led_classdev_unregister(&landisk_leds[i]);
+        }
+
+        return ret;
+}
+
+static int landisk_led_remove(struct platform_device *pdev)
+{
+        int i, nr_leds;
+
+        nr_leds = landisk_arch ? 8 : 2;
+
+        for (i = 0; i < nr_leds; i++) {
+                   led_classdev_unregister(&landisk_leds[i]);
+        }
+
+        return 0;
+}
+
+static struct platform_driver landisk_led_driver = {
+        .probe         = landisk_led_probe,
+        .remove = landisk_led_remove,
+        .driver = {
+                   .name = "landisk-led",
+                   },
+};
+
+/* HDD-access-LED setting at landisk status LED */
+static void landisk_disk_trig_activate(struct led_classdev *led_cdev)
+{
+        unsigned long flags;
+        spin_lock_irqsave(&landisk_led_lock, flags);
+        ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x04, PA_LED);
+        spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static void landisk_disk_trig_deactivate(struct led_classdev *led_cdev)
+{
+        unsigned long flags;
+        spin_lock_irqsave(&landisk_led_lock, flags);
+        ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x0c, PA_LED);
+        spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static struct led_trigger landisk_disk_led_trigger = {
+        .name = "disk",
+        .activate = landisk_disk_trig_activate,
+        .deactivate = landisk_disk_trig_deactivate,
+};
+
+static int __init landisk_led_init(void)
+{
+        u8 orig, test;
+
+        orig = ctrl_inb(PA_LED);
+        ctrl_outb(0x40, PA_LED);
+
+        test = ctrl_inb(PA_LED);
+        ctrl_outb(orig, PA_LED);
+
+        landisk_arch = (test == 0x40);
+
+        if (landisk_arch == 0) {
+                /* arch == landisk */
+                ctrl_outb(orig | 0x07, PA_LED);
+                landisk_leds[1].default_trigger = "disk";
+                led_trigger_register(&landisk_disk_led_trigger);
+        } else {
+                /* arch == usl-5p */
+                ctrl_outb(orig | 0x03, PA_LED);
+        }
+        return platform_driver_register(&landisk_led_driver);
+}
+
+static void __exit landisk_led_exit(void)
+{
+        if (landisk_arch == 0)
+                led_trigger_unregister(&landisk_disk_led_trigger);
+        return platform_driver_unregister(&landisk_led_driver);
+}
+
+module_init(landisk_led_init);
+module_exit(landisk_led_exit);
+
+MODULE_AUTHOR("kogiidena <kogiidena@eggplant.ddo.jp>");
+MODULE_DESCRIPTION("landisk LED driver");
+MODULE_LICENSE("GPL");

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[Richard Purdie]

> On Tue, 2007-05-08 at 21:26 +0900, kogiidena wrote:
> To:Richard-san
> CC:all
> 
> LED driver of I-O DATA LANDISK and USL-5P
> Please apply following patch
> 
> Signed-off-by: kogiidena <kogiidena@eggplant.ddo.jp>
> 
[...]
> +/* HDD-access-LED setting at landisk status LED */
> +static void landisk_disk_trig_activate(struct led_classdev *led_cdev)
> +{
> +        unsigned long flags;
> +        spin_lock_irqsave(&landisk_led_lock, flags);
> +        ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x04, PA_LED);
> +        spin_unlock_irqrestore(&landisk_led_lock, flags);
> +}
> +
> +static void landisk_disk_trig_deactivate(struct led_classdev *led_cdev)
> +{
> +        unsigned long flags;
> +        spin_lock_irqsave(&landisk_led_lock, flags);
> +        ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x0c, PA_LED);
> +        spin_unlock_irqrestore(&landisk_led_lock, flags);
> +}
> +
> +static struct led_trigger landisk_disk_led_trigger = {
> +        .name = "disk",
> +        .activate = landisk_disk_trig_activate,
> +        .deactivate = landisk_disk_trig_deactivate,
> +};

You can't do this since the trigger will appear for all LEDs and it only
applies to a single LED. There has been previous discussion of LED
specific triggers and we'll need that support before you can make
something like this work. Nobody has sent me a patch for that yet and I
haven't had time to write one...

+static int __init landisk_led_init(void)
> +{
> +        u8 orig, test;
> +
> +        orig = ctrl_inb(PA_LED);
> +        ctrl_outb(0x40, PA_LED);
> +
> +        test = ctrl_inb(PA_LED);
> +        ctrl_outb(orig, PA_LED);
> +
> +        landisk_arch = (test == 0x40);
> +
> +        if (landisk_arch == 0) {
> +                /* arch == landisk */
> +                ctrl_outb(orig | 0x07, PA_LED);
> +                landisk_leds[1].default_trigger = "disk";
> +                led_trigger_register(&landisk_disk_led_trigger);
> +        } else {
> +                /* arch == usl-5p */
> +                ctrl_outb(orig | 0x03, PA_LED);
> +        }
> +        return platform_driver_register(&landisk_led_driver);

Broken error handling - you should check the return code of
led_trigger_register().

> +static void __exit landisk_led_exit(void)
> +{
> +        if (landisk_arch == 0)
> +                led_trigger_unregister(&landisk_disk_led_trigger);
> +        return platform_driver_unregister(&landisk_led_driver);
> +}

void functions don't return.

Other than those issues, it looks ok.

Richard

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[kogiidena]

Hi Richard-san

The following three points were corrected.

1.
> You can't do this since the trigger will appear for all LEDs and it only
> applies to a single LED. There has been previous discussion of LED
> specific triggers and we'll need that support before you can make
> something like this work. Nobody has sent me a patch for that yet and I
> haven't had time to write one...
The trigger name past "disk" was changed to the name "hard".
This is to mean the hardware of LANDISK controls LED.
The problem of "LED specific triggers" is solved.

2.
> Broken error handling - you should check the return code of
> led_trigger_register().

3.
> void functions don't return.
it is fixed.

I think that I want to reflect the correction of BLINK, when the policy is decided.

Signed-off-by: kogiidena <kogiidena@eggplant.ddo.jp>
---
diff -urpN OLD/drivers/leds/Kconfig NEW/drivers/leds/Kconfig
--- OLD/drivers/leds/Kconfig	2007-04-28 06:49:26.000000000 +0900
+++ NEW/drivers/leds/Kconfig	2007-05-09 20:25:06.000000000 +0900
@@ -94,6 +94,13 @@ config LEDS_COBALT
 	help
 	  This option enables support for the front LED on Cobalt Server

+config LEDS_LANDISK
+        tristate "LED Support for LANDISK Series"
+        depends on LEDS_CLASS && SH_LANDISK
+        select LEDS_TRIGGERS
+        help
+          This option enables support for the LED on LANDISK Series
+
 comment "LED Triggers"

 config LEDS_TRIGGERS
diff -urpN OLD/drivers/leds/Makefile NEW/drivers/leds/Makefile
--- OLD/drivers/leds/Makefile	2007-04-28 06:49:26.000000000 +0900
+++ NEW/drivers/leds/Makefile	2007-05-09 20:24:38.000000000 +0900
@@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_NET48XX)		+= leds-net4
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
 obj-$(CONFIG_LEDS_H1940)		+= leds-h1940.o
 obj-$(CONFIG_LEDS_COBALT)		+= leds-cobalt.o
+obj-$(CONFIG_LEDS_LANDISK)		+= leds-landisk.o

 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
diff -urpN OLD/drivers/leds/leds-landisk.c NEW/drivers/leds/leds-landisk.c
--- OLD/drivers/leds/leds-landisk.c	1970-01-01 09:00:00.000000000 +0900
+++ NEW/drivers/leds/leds-landisk.c	2007-05-09 23:08:15.000000000 +0900
@@ -0,0 +1,203 @@
+/*
+ * LEDs driver for I-O DATA DEVICE, INC. "LANDISK Series" support.
+ *
+ * Copyright (C) 2007 kogiidena
+ *
+ * Based on the drivers/leds/leds-ams-delta.c by:
+ * Copyright (C) 2006 Jonathan McDowell <noodles@earth.li>
+ *
+ * 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/init.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/leds.h>
+#include <asm/io.h>
+#include <asm/landisk/iodata_landisk.h>
+
+spinlock_t landisk_led_lock;
+static int landisk_arch;	/* 0:LANDISK, LANTank 1:USL-5P */
+
+static void landisk_led_set(struct led_classdev *led_cdev,
+			    enum led_brightness value);
+
+static struct led_classdev landisk_leds[] = {
+	[0] = {
+	       .name = "power",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "blink",
+	       },
+	[1] = {
+	       .name = "status",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "blink",
+	       },
+	[2] = {
+	       .name = "led1",
+	       .brightness_set = landisk_led_set,
+	       },
+	[3] = {
+	       .name = "led2",
+	       .brightness_set = landisk_led_set,
+	       },
+	[4] = {
+	       .name = "led3",
+	       .brightness_set = landisk_led_set,
+	       },
+	[5] = {
+	       .name = "led4",
+	       .brightness_set = landisk_led_set,
+	       },
+	[6] = {
+	       .name = "led5",
+	       .brightness_set = landisk_led_set,
+	       },
+	[7] = {
+	       .name = "buzzer",
+	       .default_trigger = "bitshift",
+	       .brightness_set = landisk_led_set,
+	       },
+};
+
+static void landisk_led_set(struct led_classdev *led_cdev,
+			    enum led_brightness value)
+{
+	u8 tmp;
+	int bitmask;
+	unsigned long flags;
+
+	bitmask = 0x01 << (led_cdev - &landisk_leds[0]);
+
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	tmp = ctrl_inb(PA_LED);
+	if (value)
+		tmp |= bitmask;
+	else
+		tmp &= ~bitmask;
+	ctrl_outb(tmp, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static int landisk_led_probe(struct platform_device *pdev)
+{
+	int i, nr_leds;
+	int ret;
+
+	nr_leds = landisk_arch ? 8 : 2;
+
+	for (i = ret = 0; ret >= 0 && i < nr_leds; i++) {
+		ret = led_classdev_register(&pdev->dev, &landisk_leds[i]);
+	}
+
+	if (ret < 0 && i > 1) {
+		nr_leds = i - 1;
+		for (i = 0; i < nr_leds; i++)
+			led_classdev_unregister(&landisk_leds[i]);
+	}
+
+	return ret;
+}
+
+static int landisk_led_remove(struct platform_device *pdev)
+{
+	int i, nr_leds;
+
+	nr_leds = landisk_arch ? 8 : 2;
+
+	for (i = 0; i < nr_leds; i++) {
+		led_classdev_unregister(&landisk_leds[i]);
+	}
+
+	return 0;
+}
+
+static struct platform_driver landisk_led_driver = {
+	.probe = landisk_led_probe,
+	.remove = landisk_led_remove,
+	.driver = {
+		   .name = "landisk-led",
+		   },
+};
+
+/*
+ * LANDISK hardware controled LED trigger
+ *
+ *  bit[3:0] = x0xx : power-led  is allways ON.
+ *  bit[3:0] = x1x0 : power-led  is ON.
+ *  bit[3:0] = x1x1 : power-led  is OFF.
+ *  bit[3:0] = 0xxx : status-led is HDD access-LED.
+ *  bit[3:0] = 1x0x : status-led is ON
+ *  bit[3:0] = 1x1x : status-led is OFF
+ */
+static void landisk_hard_trig_activate(struct led_classdev *led_cdev)
+{
+	unsigned long flags;
+	int bitmask;
+
+	bitmask = 0x04 << (led_cdev - &landisk_leds[0]);
+
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	ctrl_outb(ctrl_inb(PA_LED) & ~bitmask, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static void landisk_hard_trig_deactivate(struct led_classdev *led_cdev)
+{
+	unsigned long flags;
+	int bitmask;
+
+	bitmask = 0x04 << (led_cdev - &landisk_leds[0]);
+
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	ctrl_outb(ctrl_inb(PA_LED) | bitmask, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static struct led_trigger landisk_hard_led_trigger = {
+	.name = "hard",
+	.activate = landisk_hard_trig_activate,
+	.deactivate = landisk_hard_trig_deactivate,
+};
+
+static int __init landisk_led_init(void)
+{
+	u8 orig, test;
+	int err = 0;
+
+	orig = ctrl_inb(PA_LED);
+	ctrl_outb(0x40, PA_LED);
+
+	test = ctrl_inb(PA_LED);
+	ctrl_outb(orig, PA_LED);
+
+	landisk_arch = (test == 0x40);
+
+	if (landisk_arch == 0) {
+		/* arch == landisk */
+		ctrl_outb(orig | 0x07, PA_LED);
+		landisk_leds[1].default_trigger = "hard";
+		err = led_trigger_register(&landisk_hard_led_trigger);
+		if (err)
+			return err;
+	} else {
+		/* arch == usl-5p */
+		ctrl_outb(orig | 0x03, PA_LED);
+	}
+	return platform_driver_register(&landisk_led_driver);
+}
+
+static void __exit landisk_led_exit(void)
+{
+	if (landisk_arch == 0)
+		led_trigger_unregister(&landisk_hard_led_trigger);
+	platform_driver_unregister(&landisk_led_driver);
+}
+
+module_init(landisk_led_init);
+module_exit(landisk_led_exit);
+
+MODULE_AUTHOR("kogiidena <kogiidena@eggplant.ddo.jp>");
+MODULE_DESCRIPTION("landisk LED driver");
+MODULE_LICENSE("GPL");

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[Richard Purdie]

On Wed, 2007-05-09 at 23:26 +0900, kogiidena wrote:
> Hi Richard-san
> 
> The following three points were corrected.
> 
> 1.
> > You can't do this since the trigger will appear for all LEDs and it only
> > applies to a single LED. There has been previous discussion of LED
> > specific triggers and we'll need that support before you can make
> > something like this work. Nobody has sent me a patch for that yet and I
> > haven't had time to write one...
> The trigger name past "disk" was changed to the name "hard".
> This is to mean the hardware of LANDISK controls LED.
> The problem of "LED specific triggers" is solved.

No, its not solved.

Imagine I connect some device with its own LEDs and LED drivers. I'll
use "corgi:amber" as an example.

I run:

'cat /sys/class/leds/corgi:amber/triggers'

I see "hard" listed.

'echo "hard" > /sys/class/leds/corgi:amber/triggers'

doesn't work though...

If its not going to work, it shouldn't be listed.

Regards,

Richard

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[Anton Vorontsov]

On Wed, May 09, 2007 at 04:13:14PM +0100, Richard Purdie wrote:
> On Wed, 2007-05-09 at 23:26 +0900, kogiidena wrote:
> > Hi Richard-san
> > 
> > The following three points were corrected.
> > 
> > 1.
> > > You can't do this since the trigger will appear for all LEDs and it only
> > > applies to a single LED. There has been previous discussion of LED
> > > specific triggers and we'll need that support before you can make
> > > something like this work. Nobody has sent me a patch for that yet and I
> > > haven't had time to write one...
> > The trigger name past "disk" was changed to the name "hard".
> > This is to mean the hardware of LANDISK controls LED.
> > The problem of "LED specific triggers" is solved.
> 
> No, its not solved.
> 
> Imagine I connect some device with its own LEDs and LED drivers. I'll
> use "corgi:amber" as an example.
> 
> I run:
> 
> 'cat /sys/class/leds/corgi:amber/triggers'
> 
> I see "hard" listed.
> 
> 'echo "hard" > /sys/class/leds/corgi:amber/triggers'
> 
> doesn't work though...
> 
> If its not going to work, it shouldn't be listed.
> 
> Regards,
> 
> Richard

Following patch sitting for a long time in our handhelds.org tree.

kogiidena, I'm almost sure you'll find it useful, just apply patch,
and implement .is_led_supported function for your trigger, which will
eliminate trigger showing in
/sys/class/leds/LED_WHICH_NOT_SUPPORTS_CUSTOM_TRIGGER/triggers


- - - - -

Custom triggers support, which are might not supported by all LEDs
    
Signed-off-by: Anton Vorontsov <cbou@mail.ru>

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 454fb09..0af0d61 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -53,6 +53,9 @@ ssize_t led_trigger_store(struct class_device *dev, const char *buf,
 	read_lock(&triggers_list_lock);
 	list_for_each_entry(trig, &trigger_list, next_trig) {
 		if (!strcmp(trigger_name, trig->name)) {
+			if (trig->is_led_supported &&
+			    !trig->is_led_supported(led_cdev)) break;
+
 			write_lock(&led_cdev->trigger_lock);
 			led_trigger_set(led_cdev, trig);
 			write_unlock(&led_cdev->trigger_lock);
@@ -85,6 +88,8 @@ ssize_t led_trigger_show(struct class_device *dev, char *buf)
 		if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
 							trig->name))
 			len += sprintf(buf+len, "[%s] ", trig->name);
+		else if (trig->is_led_supported &&
+			 !trig->is_led_supported(led_cdev)) continue;
 		else
 			len += sprintf(buf+len, "%s ", trig->name);
 	}
@@ -127,6 +132,7 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
 			led_cdev->trigger->deactivate(led_cdev);
 		led_set_brightness(led_cdev, LED_OFF);
 	}
+	led_cdev->trigger = trigger;
 	if (trigger) {
 		write_lock_irqsave(&trigger->leddev_list_lock, flags);
 		list_add_tail(&led_cdev->trig_list, &trigger->led_cdevs);
@@ -134,7 +140,6 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
 		if (trigger->activate)
 			trigger->activate(led_cdev);
 	}
-	led_cdev->trigger = trigger;
 }
 
 void led_trigger_set_default(struct led_classdev *led_cdev)
@@ -171,7 +176,9 @@ int led_trigger_register(struct led_trigger *trigger)
 	list_for_each_entry(led_cdev, &leds_list, node) {
 		write_lock(&led_cdev->trigger_lock);
 		if (!led_cdev->trigger && led_cdev->default_trigger &&
-			    !strcmp(led_cdev->default_trigger, trigger->name))
+			   !strcmp(led_cdev->default_trigger, trigger->name) &&
+			   (!trigger->is_led_supported ||
+			    trigger->is_led_supported(led_cdev)))
 			led_trigger_set(led_cdev, trigger);
 		write_unlock(&led_cdev->trigger_lock);
 	}
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 88afcef..71175f6 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -70,6 +70,7 @@ struct led_trigger {
 	const char	 *name;
 	void		(*activate)(struct led_classdev *led_cdev);
 	void		(*deactivate)(struct led_classdev *led_cdev);
+	int		(*is_led_supported)(struct led_classdev *led_cdev);
 
 	/* LEDs under control by this trigger (for simple triggers) */
 	rwlock_t	  leddev_list_lock;

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[kogiidena]

> Following patch sitting for a long time in our handhelds.org tree.
>
> kogiidena, I'm almost sure you'll find it useful, just apply patch,
> and implement .is_led_supported function for your trigger, which will
> eliminate trigger showing in
> /sys/class/leds/LED_WHICH_NOT_SUPPORTS_CUSTOM_TRIGGER/triggers

Anton Vorontsov-san
Thank you very much .
I confirmed the correct operation by the following patch.

Hi Richard-san
Is it OK ?

LED driver of I-O DATA LANDISK and USL-5P

Signed-off-by: kogiidena <kogiidena@eggplant.ddo.jp>

---
diff -urpN OLD/drivers/leds/Kconfig NEW/drivers/leds/Kconfig
--- OLD/drivers/leds/Kconfig	2007-04-28 06:49:26.000000000 +0900
+++ NEW/drivers/leds/Kconfig	2007-05-10 19:54:23.000000000 +0900
@@ -94,6 +94,12 @@ config LEDS_COBALT
 	help
 	  This option enables support for the front LED on Cobalt Server

+config LEDS_LANDISK
+	tristate "LED Support for LANDISK Series"
+	depends on LEDS_CLASS && SH_LANDISK
+	help
+	  This option enables support for the LED on LANDISK Series
+
 comment "LED Triggers"

 config LEDS_TRIGGERS
diff -urpN OLD/drivers/leds/Makefile NEW/drivers/leds/Makefile
--- OLD/drivers/leds/Makefile	2007-04-28 06:49:26.000000000 +0900
+++ NEW/drivers/leds/Makefile	2007-05-10 19:54:44.000000000 +0900
@@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_NET48XX)		+= leds-net4
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
 obj-$(CONFIG_LEDS_H1940)		+= leds-h1940.o
 obj-$(CONFIG_LEDS_COBALT)		+= leds-cobalt.o
+obj-$(CONFIG_LEDS_LANDISK)		+= leds-landisk.o

 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
diff -urpN OLD/drivers/leds/leds-landisk.c NEW/drivers/leds/leds-landisk.c
--- OLD/drivers/leds/leds-landisk.c	1970-01-01 09:00:00.000000000 +0900
+++ NEW/drivers/leds/leds-landisk.c	2007-05-10 20:07:11.000000000 +0900
@@ -0,0 +1,194 @@
+/*
+ * LEDs driver for I-O DATA DEVICE, INC. "LANDISK Series" support.
+ *
+ * Copyright (C) 2007 kogiidena
+ *
+ * Based on the drivers/leds/leds-ams-delta.c by:
+ * Copyright (C) 2006 Jonathan McDowell <noodles@earth.li>
+ *
+ * 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/init.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/leds.h>
+#include <asm/io.h>
+#include <asm/landisk/iodata_landisk.h>
+
+spinlock_t landisk_led_lock;
+static int landisk_arch;	/* 0:LANDISK, LANTank 1:USL-5P */
+
+static void landisk_led_set(struct led_classdev *led_cdev,
+			    enum led_brightness value);
+
+static struct led_classdev landisk_leds[] = {
+	[0] = {
+	       .name = "power",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "blink",
+	       },
+	[1] = {
+	       .name = "status",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "blink",
+	       },
+	[2] = {
+	       .name = "led1",
+	       .brightness_set = landisk_led_set,
+	       },
+	[3] = {
+	       .name = "led2",
+	       .brightness_set = landisk_led_set,
+	       },
+	[4] = {
+	       .name = "led3",
+	       .brightness_set = landisk_led_set,
+	       },
+	[5] = {
+	       .name = "led4",
+	       .brightness_set = landisk_led_set,
+	       },
+	[6] = {
+	       .name = "led5",
+	       .brightness_set = landisk_led_set,
+	       },
+	[7] = {
+	       .name = "buzzer",
+	       .default_trigger = "bitshift",
+	       .brightness_set = landisk_led_set,
+	       },
+};
+
+static void landisk_led_set(struct led_classdev *led_cdev,
+			    enum led_brightness value)
+{
+	u8 tmp;
+	int bitmask;
+	unsigned long flags;
+
+	bitmask = 0x01 << (led_cdev - &landisk_leds[0]);
+
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	tmp = ctrl_inb(PA_LED);
+	if (value)
+		tmp |= bitmask;
+	else
+		tmp &= ~bitmask;
+	ctrl_outb(tmp, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static int landisk_led_probe(struct platform_device *pdev)
+{
+	int i, nr_leds;
+	int ret;
+
+	nr_leds = landisk_arch ? 8 : 2;
+
+	for (i = ret = 0; ret >= 0 && i < nr_leds; i++) {
+		ret = led_classdev_register(&pdev->dev, &landisk_leds[i]);
+	}
+
+	if (ret < 0 && i > 1) {
+		nr_leds = i - 1;
+		for (i = 0; i < nr_leds; i++)
+			led_classdev_unregister(&landisk_leds[i]);
+	}
+	return ret;
+}
+
+static int landisk_led_remove(struct platform_device *pdev)
+{
+	int i, nr_leds;
+
+	nr_leds = landisk_arch ? 8 : 2;
+
+	for (i = 0; i < nr_leds; i++) {
+		led_classdev_unregister(&landisk_leds[i]);
+	}
+	return 0;
+}
+
+static struct platform_driver landisk_led_driver = {
+	.probe = landisk_led_probe,
+	.remove = landisk_led_remove,
+	.driver = {
+		   .name = "landisk-led",
+		   },
+};
+
+/* HDD-access-LED setting at landisk status LED */
+static void landisk_disk_trig_activate(struct led_classdev *led_cdev)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x04, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static void landisk_disk_trig_deactivate(struct led_classdev *led_cdev)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x0c, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static int landisk_disk_trig_is_led_supported(struct led_classdev *led_cdev)
+{
+	int led;
+
+	led = (led_cdev - &landisk_leds[0]);
+	return ((landisk_arch == 0) && (led == 1));
+}
+
+static struct led_trigger landisk_disk_led_trigger = {
+	.name = "disk",
+	.activate = landisk_disk_trig_activate,
+	.deactivate = landisk_disk_trig_deactivate,
+	.is_led_supported = landisk_disk_trig_is_led_supported,
+};
+
+static int __init landisk_led_init(void)
+{
+	u8 orig, test;
+	int err = 0;
+
+	orig = ctrl_inb(PA_LED);
+	ctrl_outb(0x40, PA_LED);
+
+	test = ctrl_inb(PA_LED);
+	ctrl_outb(orig, PA_LED);
+
+	landisk_arch = (test == 0x40);
+
+	if (landisk_arch == 0) {
+		/* arch == landisk */
+		ctrl_outb(orig | 0x07, PA_LED);
+		landisk_leds[1].default_trigger = "disk";
+	} else {
+		/* arch == usl-5p */
+		ctrl_outb(orig | 0x03, PA_LED);
+	}
+
+	err = led_trigger_register(&landisk_disk_led_trigger);
+	if (err)
+		return err;
+
+	return platform_driver_register(&landisk_led_driver);
+}
+
+static void __exit landisk_led_exit(void)
+{
+	led_trigger_unregister(&landisk_disk_led_trigger);
+	platform_driver_unregister(&landisk_led_driver);
+}
+
+module_init(landisk_led_init);
+module_exit(landisk_led_exit);
+
+MODULE_AUTHOR("kogiidena <kogiidena@eggplant.ddo.jp>");
+MODULE_DESCRIPTION("landisk LED driver");
+MODULE_LICENSE("GPL");

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[Paul Mundt]

kogiidena-san,

On Thu, May 10, 2007 at 08:52:13PM +0900, kogiidena wrote:
> diff -urpN OLD/drivers/leds/leds-landisk.c NEW/drivers/leds/leds-landisk.c
> --- OLD/drivers/leds/leds-landisk.c	1970-01-01 09:00:00.000000000 +0900
> +++ NEW/drivers/leds/leds-landisk.c	2007-05-10 20:07:11.000000000 +0900
[snip]
> +spinlock_t landisk_led_lock;

DEFINE_SPINLOCK(), please.

> +static int landisk_arch;	/* 0:LANDISK, LANTank 1:USL-5P */
> +
If you're going to do this, enums are probably the cleaner way to go.
Checking landisk_arch == 0 all over the place is non-obvious without
seeing this comment.

> +static void landisk_led_set(struct led_classdev *led_cdev,
> +			    enum led_brightness value)
> +{
> +	u8 tmp;
> +	int bitmask;
> +	unsigned long flags;
> +
> +	bitmask = 0x01 << (led_cdev - &landisk_leds[0]);
> +
> +	spin_lock_irqsave(&landisk_led_lock, flags);
> +	tmp = ctrl_inb(PA_LED);
> +	if (value)
> +		tmp |= bitmask;
> +	else
> +		tmp &= ~bitmask;
> +	ctrl_outb(tmp, PA_LED);

You may also want some sanity checking here, while PA_LED is only 8-bits,
your bitmask is not.

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[kogiidena]

To: Paul-san
The following three points were corrected.
thank you for you check.

1.
> DEFINE_SPINLOCK(), please.

2.
> If you're going to do this, enums are probably the cleaner way to go.
> Checking landisk_arch == 0 all over the place is non-obvious without
> seeing this comment.

3.
> You may also want some sanity checking here, while PA_LED is only 8-bits,
> your bitmask is not.

To: Richard-san
Please apply.

LED driver of I-O DATA LANDISK and USL-5P

Signed-off-by: kogiidena <kogiidena@eggplant.ddo.jp>
---
diff -urpN OLD/drivers/leds/Kconfig NEW/drivers/leds/Kconfig
--- OLD/drivers/leds/Kconfig	2007-04-28 06:49:26.000000000 +0900
+++ NEW/drivers/leds/Kconfig	2007-05-11 21:15:28.000000000 +0900
@@ -94,6 +94,12 @@ config LEDS_COBALT
 	help
 	  This option enables support for the front LED on Cobalt Server

+config LEDS_LANDISK
+	tristate "LED Support for LANDISK Series"
+	depends on LEDS_CLASS && SH_LANDISK
+	help
+	  This option enables support for the LED on LANDISK Series
+
 comment "LED Triggers"

 config LEDS_TRIGGERS
diff -urpN OLD/drivers/leds/Makefile NEW/drivers/leds/Makefile
--- OLD/drivers/leds/Makefile	2007-04-28 06:49:26.000000000 +0900
+++ NEW/drivers/leds/Makefile	2007-05-11 23:34:07.000000000 +0900
@@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_NET48XX)		+= leds-net4
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
 obj-$(CONFIG_LEDS_H1940)		+= leds-h1940.o
 obj-$(CONFIG_LEDS_COBALT)		+= leds-cobalt.o
+obj-$(CONFIG_LEDS_LANDISK)		+= leds-landisk.o

 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
diff -urpN OLD/drivers/leds/leds-landisk.c NEW/drivers/leds/leds-landisk.c
--- OLD/drivers/leds/leds-landisk.c	1970-01-01 09:00:00.000000000 +0900
+++ NEW/drivers/leds/leds-landisk.c	2007-05-11 23:12:16.000000000 +0900
@@ -0,0 +1,214 @@
+/*
+ * LEDs driver for I-O DATA DEVICE, INC. "LANDISK Series" support.
+ *
+ * Copyright (C) 2007 kogiidena
+ *
+ * Based on the drivers/leds/leds-ams-delta.c by:
+ * Copyright (C) 2006 Jonathan McDowell <noodles@earth.li>
+ *
+ * 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/init.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/leds.h>
+#include <asm/io.h>
+#include <asm/landisk/iodata_landisk.h>
+
+static enum {
+	LANDISK = 0,
+	USL_5P  = 1,
+} landisk_product;
+
+static DEFINE_SPINLOCK(landisk_led_lock);
+
+static void landisk_led_set(struct led_classdev *led_cdev,
+			    enum led_brightness value);
+
+static struct led_classdev landisk_leds[] = {
+	[0] = {
+	       .name = "power",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[1] = {
+	       .name = "status",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[2] = {
+	       .name = "led1",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[3] = {
+	       .name = "led2",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[4] = {
+	       .name = "led3",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[5] = {
+	       .name = "led4",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[6] = {
+	       .name = "led5",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[7] = {
+	       .name = "buzzer",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+};
+
+void ledtrig_bitpat_default(struct led_classdev *led_cdev,
+			    unsigned long *interval, char *bitdata)
+{
+	int led;
+
+	*interval  = 250;
+	bitdata[0] = '\0';
+
+	led = (led_cdev - &landisk_leds[0]);
+	if ((led == 0) || (led == 1)) {
+		strcpy(bitdata, "blink");
+	}
+}
+
+static void landisk_led_set(struct led_classdev *led_cdev,
+			    enum led_brightness value)
+{
+	u8 tmp, bitmask;
+	unsigned long flags;
+
+	bitmask = 0x01 << (led_cdev - &landisk_leds[0]);
+
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	tmp = ctrl_inb(PA_LED);
+	if (value)
+		tmp |= bitmask;
+	else
+		tmp &= ~bitmask;
+	ctrl_outb(tmp, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static int landisk_led_probe(struct platform_device *pdev)
+{
+	int i, nr_leds;
+	int ret;
+
+	nr_leds = (landisk_product == LANDISK) ? 2 : 8;
+
+	for (i = ret = 0; ret >= 0 && i < nr_leds; i++) {
+		ret = led_classdev_register(&pdev->dev, &landisk_leds[i]);
+	}
+
+	if (ret < 0 && i > 1) {
+		nr_leds = i - 1;
+		for (i = 0; i < nr_leds; i++)
+			led_classdev_unregister(&landisk_leds[i]);
+	}
+	return ret;
+}
+
+static int landisk_led_remove(struct platform_device *pdev)
+{
+	int i, nr_leds;
+
+	nr_leds = (landisk_product == LANDISK) ? 2 : 8;
+
+	for (i = 0; i < nr_leds; i++) {
+		led_classdev_unregister(&landisk_leds[i]);
+	}
+	return 0;
+}
+
+static struct platform_driver landisk_led_driver = {
+	.probe = landisk_led_probe,
+	.remove = landisk_led_remove,
+	.driver = {
+		   .name = "landisk-led",
+		   },
+};
+
+/* HDD-access-LED setting at landisk status LED */
+static void landisk_disk_trig_activate(struct led_classdev *led_cdev)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x04, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static void landisk_disk_trig_deactivate(struct led_classdev *led_cdev)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x0c, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static int landisk_disk_trig_is_led_supported(struct led_classdev *led_cdev)
+{
+	int led;
+
+	led = (led_cdev - &landisk_leds[0]);
+	return ((landisk_product == LANDISK) && (led == 1));
+}
+
+static struct led_trigger landisk_disk_led_trigger = {
+	.name = "disk",
+	.activate = landisk_disk_trig_activate,
+	.deactivate = landisk_disk_trig_deactivate,
+	.is_led_supported = landisk_disk_trig_is_led_supported,
+};
+
+static int __init landisk_led_init(void)
+{
+	u8 orig, test;
+	int err = 0;
+
+	orig = ctrl_inb(PA_LED);
+	ctrl_outb(0x40, PA_LED);
+
+	test = ctrl_inb(PA_LED);
+	ctrl_outb(orig, PA_LED);
+
+	if (test != 0x40) {
+		landisk_product = LANDISK;
+		ctrl_outb(orig | 0x07, PA_LED);
+		landisk_leds[1].default_trigger = "disk";
+	} else {
+		landisk_product = USL_5P;
+		ctrl_outb(orig | 0x03, PA_LED);
+	}
+
+	err = led_trigger_register(&landisk_disk_led_trigger);
+	if (err)
+		return err;
+
+	return platform_driver_register(&landisk_led_driver);
+}
+
+static void __exit landisk_led_exit(void)
+{
+	led_trigger_unregister(&landisk_disk_led_trigger);
+	platform_driver_unregister(&landisk_led_driver);
+}
+
+module_init(landisk_led_init);
+module_exit(landisk_led_exit);
+
+MODULE_AUTHOR("kogiidena <kogiidena@eggplant.ddo.jp>");
+MODULE_DESCRIPTION("landisk LED driver");
+MODULE_LICENSE("GPL");

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[kogiidena]

To: Richard-san

I'm sorry. The patch sent yesterday is corrected.
Only the ledtrig_bitpat_default function was changed.
The patch of "Custom triggers support, which are might not supported by all LEDs"
is necessary.

LED driver of I-O DATA LANDISK and USL-5P

Signed-off-by: kogiidena <kogiidena@eggplant.ddo.jp>
---
diff -urpN OLD/drivers/leds/Kconfig NEW/drivers/leds/Kconfig
--- OLD/drivers/leds/Kconfig	2007-04-28 06:49:26.000000000 +0900
+++ NEW/drivers/leds/Kconfig	2007-05-11 21:15:28.000000000 +0900
@@ -94,6 +94,12 @@ config LEDS_COBALT
 	help
 	  This option enables support for the front LED on Cobalt Server

+config LEDS_LANDISK
+	tristate "LED Support for LANDISK Series"
+	depends on LEDS_CLASS && SH_LANDISK
+	help
+	  This option enables support for the LED on LANDISK Series
+
 comment "LED Triggers"

 config LEDS_TRIGGERS
diff -urpN OLD/drivers/leds/Makefile NEW/drivers/leds/Makefile
--- OLD/drivers/leds/Makefile	2007-04-28 06:49:26.000000000 +0900
+++ NEW/drivers/leds/Makefile	2007-05-11 23:34:07.000000000 +0900
@@ -16,6 +16,7 @@ obj-$(CONFIG_LEDS_NET48XX)		+= leds-net4
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
 obj-$(CONFIG_LEDS_H1940)		+= leds-h1940.o
 obj-$(CONFIG_LEDS_COBALT)		+= leds-cobalt.o
+obj-$(CONFIG_LEDS_LANDISK)		+= leds-landisk.o

 # LED Triggers
 obj-$(CONFIG_LEDS_TRIGGER_TIMER)	+= ledtrig-timer.o
diff -urpN OLD/drivers/leds/leds-landisk.c NEW/drivers/leds/leds-landisk.c
--- OLD/drivers/leds/leds-landisk.c	1970-01-01 09:00:00.000000000 +0900
+++ NEW/drivers/leds/leds-landisk.c	2007-05-12 11:31:47.000000000 +0900
@@ -0,0 +1,215 @@
+/*
+ * LEDs driver for I-O DATA DEVICE, INC. "LANDISK Series" support.
+ *
+ * Copyright (C) 2007 kogiidena
+ *
+ * Based on the drivers/leds/leds-ams-delta.c by:
+ * Copyright (C) 2006 Jonathan McDowell <noodles@earth.li>
+ *
+ * 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/init.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/leds.h>
+#include <asm/io.h>
+#include <asm/landisk/iodata_landisk.h>
+
+static enum {
+	LANDISK = 0,
+	USL_5P  = 1,
+} landisk_product;
+
+static DEFINE_SPINLOCK(landisk_led_lock);
+
+static void landisk_led_set(struct led_classdev *led_cdev,
+			    enum led_brightness value);
+
+static struct led_classdev landisk_leds[] = {
+	[0] = {
+	       .name = "power",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[1] = {
+	       .name = "status",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[2] = {
+	       .name = "led1",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[3] = {
+	       .name = "led2",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[4] = {
+	       .name = "led3",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[5] = {
+	       .name = "led4",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[6] = {
+	       .name = "led5",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+	[7] = {
+	       .name = "buzzer",
+	       .brightness_set = landisk_led_set,
+	       .default_trigger = "bitpat",
+	       },
+};
+
+void ledtrig_bitpat_default(struct led_classdev *led_cdev,
+			    unsigned long *delay, char *bitdata)
+{
+	int led;
+
+	led = (led_cdev - &landisk_leds[0]);
+	if ((led == 0) || (led == 1)) {
+		strcpy(bitdata, "blink");
+	}
+	if (led == 7) {
+		*delay = 250;
+	}
+
+}
+
+static void landisk_led_set(struct led_classdev *led_cdev,
+			    enum led_brightness value)
+{
+	u8 tmp, bitmask;
+	unsigned long flags;
+
+	bitmask = 0x01 << (led_cdev - &landisk_leds[0]);
+
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	tmp = ctrl_inb(PA_LED);
+	if (value)
+		tmp |= bitmask;
+	else
+		tmp &= ~bitmask;
+	ctrl_outb(tmp, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static int landisk_led_probe(struct platform_device *pdev)
+{
+	int i, nr_leds;
+	int ret;
+
+	nr_leds = (landisk_product == LANDISK) ? 2 : 8;
+
+	for (i = ret = 0; ret >= 0 && i < nr_leds; i++) {
+		ret = led_classdev_register(&pdev->dev, &landisk_leds[i]);
+	}
+
+	if (ret < 0 && i > 1) {
+		nr_leds = i - 1;
+		for (i = 0; i < nr_leds; i++)
+			led_classdev_unregister(&landisk_leds[i]);
+	}
+	return ret;
+}
+
+static int landisk_led_remove(struct platform_device *pdev)
+{
+	int i, nr_leds;
+
+	nr_leds = (landisk_product == LANDISK) ? 2 : 8;
+
+	for (i = 0; i < nr_leds; i++) {
+		led_classdev_unregister(&landisk_leds[i]);
+	}
+	return 0;
+}
+
+static struct platform_driver landisk_led_driver = {
+	.probe = landisk_led_probe,
+	.remove = landisk_led_remove,
+	.driver = {
+		   .name = "landisk-led",
+		   },
+};
+
+/* HDD-access-LED setting at landisk status LED */
+static void landisk_disk_trig_activate(struct led_classdev *led_cdev)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x04, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static void landisk_disk_trig_deactivate(struct led_classdev *led_cdev)
+{
+	unsigned long flags;
+	spin_lock_irqsave(&landisk_led_lock, flags);
+	ctrl_outb((ctrl_inb(PA_LED) & ~0x0c) | 0x0c, PA_LED);
+	spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static int landisk_disk_trig_is_led_supported(struct led_classdev *led_cdev)
+{
+	int led;
+
+	led = (led_cdev - &landisk_leds[0]);
+	return ((landisk_product == LANDISK) && (led == 1));
+}
+
+static struct led_trigger landisk_disk_led_trigger = {
+	.name = "disk",
+	.activate = landisk_disk_trig_activate,
+	.deactivate = landisk_disk_trig_deactivate,
+	.is_led_supported = landisk_disk_trig_is_led_supported,
+};
+
+static int __init landisk_led_init(void)
+{
+	u8 orig, test;
+	int err = 0;
+
+	orig = ctrl_inb(PA_LED);
+	ctrl_outb(0x40, PA_LED);
+
+	test = ctrl_inb(PA_LED);
+	ctrl_outb(orig, PA_LED);
+
+	if (test != 0x40) {
+		landisk_product = LANDISK;
+		ctrl_outb(orig | 0x07, PA_LED);
+		landisk_leds[1].default_trigger = "disk";
+	} else {
+		landisk_product = USL_5P;
+		ctrl_outb(orig | 0x03, PA_LED);
+	}
+
+	err = led_trigger_register(&landisk_disk_led_trigger);
+	if (err)
+		return err;
+
+	return platform_driver_register(&landisk_led_driver);
+}
+
+static void __exit landisk_led_exit(void)
+{
+	led_trigger_unregister(&landisk_disk_led_trigger);
+	platform_driver_unregister(&landisk_led_driver);
+}
+
+module_init(landisk_led_init);
+module_exit(landisk_led_exit);
+
+MODULE_AUTHOR("kogiidena <kogiidena@eggplant.ddo.jp>");
+MODULE_DESCRIPTION("landisk LED driver");
+MODULE_LICENSE("GPL");

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[Richard Purdie]

On Wed, 2007-05-09 at 20:03 +0400, Anton Vorontsov wrote:
> Following patch sitting for a long time in our handhelds.org tree.
> 
> kogiidena, I'm almost sure you'll find it useful, just apply patch,
> and implement .is_led_supported function for your trigger, which will
> eliminate trigger showing in
> /sys/class/leds/LED_WHICH_NOT_SUPPORTS_CUSTOM_TRIGGER/triggers

I like the approach and will apply something like this. Comments
follow...

> Custom triggers support, which are might not supported by all LEDs
>     
> Signed-off-by: Anton Vorontsov <cbou@mail.ru>
> 
> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> index 454fb09..0af0d61 100644
> --- a/drivers/leds/led-triggers.c
> +++ b/drivers/leds/led-triggers.c
> @@ -53,6 +53,9 @@ ssize_t led_trigger_store(struct class_device *dev, const char *buf,
>  	read_lock(&triggers_list_lock);
>  	list_for_each_entry(trig, &trigger_list, next_trig) {
>  		if (!strcmp(trigger_name, trig->name)) {
> +			if (trig->is_led_supported &&
> +			    !trig->is_led_supported(led_cdev)) break;

Missing newline.

> @@ -85,6 +88,8 @@ ssize_t led_trigger_show(struct class_device *dev, char *buf)
>  		if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
>  							trig->name))
>  			len += sprintf(buf+len, "[%s] ", trig->name);
> +		else if (trig->is_led_supported &&
> +			 !trig->is_led_supported(led_cdev)) continue;

ditto.

> @@ -127,6 +132,7 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
>  			led_cdev->trigger->deactivate(led_cdev);
>  		led_set_brightness(led_cdev, LED_OFF);
>  	}
> +	led_cdev->trigger = trigger;
>  	if (trigger) {
>  		write_lock_irqsave(&trigger->leddev_list_lock, flags);
>  		list_add_tail(&led_cdev->trig_list, &trigger->led_cdevs);
> @@ -134,7 +140,6 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
>  		if (trigger->activate)
>  			trigger->activate(led_cdev);
>  	}
> -	led_cdev->trigger = trigger;
>  }
>  
>  void led_trigger_set_default(struct led_classdev *led_cdev)

Why was the above was changed?

I think we've discussed this before and it would be better to add a
trigger parameter to activate/deactivate if we really need it. Anyhow,
its not part of this patch and if you want that it should be in a
separate one with accompanying rational.

Can you resend with the above fixed and I'll apply.

Thanks,

Richard

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[Anton Vorontsov]

On Mon, May 14, 2007 at 12:16:22AM +0100, Richard Purdie wrote:
> On Wed, 2007-05-09 at 20:03 +0400, Anton Vorontsov wrote:
> > Following patch sitting for a long time in our handhelds.org tree.
> > 
> > kogiidena, I'm almost sure you'll find it useful, just apply patch,
> > and implement .is_led_supported function for your trigger, which will
> > eliminate trigger showing in
> > /sys/class/leds/LED_WHICH_NOT_SUPPORTS_CUSTOM_TRIGGER/triggers
> 
> I like the approach and will apply something like this.

Splendid!

> Comments
> follow..
...
> Missing newline.
... 
> ditto.

Thanks, fixed.

> Thanks,
> 
> Richard


From: Anton Vorontsov <cbou@mail.ru>
Date: Mon, 14 May 2007 23:39:30 +0400
Subject: [PATCH] Custom triggers support, which are might not supported by all LEDs

Signed-off-by: Anton Vorontsov <cbou@mail.ru>
---
 drivers/leds/led-triggers.c |   11 ++++++++++-
 include/linux/leds.h        |    1 +
 2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 454fb09..7fde7d0 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -53,6 +53,10 @@ ssize_t led_trigger_store(struct class_device *dev, const char *buf,
 	read_lock(&triggers_list_lock);
 	list_for_each_entry(trig, &trigger_list, next_trig) {
 		if (!strcmp(trigger_name, trig->name)) {
+			if (trig->is_led_supported &&
+			    !trig->is_led_supported(led_cdev))
+				break;
+
 			write_lock(&led_cdev->trigger_lock);
 			led_trigger_set(led_cdev, trig);
 			write_unlock(&led_cdev->trigger_lock);
@@ -85,6 +89,9 @@ ssize_t led_trigger_show(struct class_device *dev, char *buf)
 		if (led_cdev->trigger && !strcmp(led_cdev->trigger->name,
 							trig->name))
 			len += sprintf(buf+len, "[%s] ", trig->name);
+		else if (trig->is_led_supported &&
+			 !trig->is_led_supported(led_cdev))
+			continue;
 		else
 			len += sprintf(buf+len, "%s ", trig->name);
 	}
@@ -171,7 +178,9 @@ int led_trigger_register(struct led_trigger *trigger)
 	list_for_each_entry(led_cdev, &leds_list, node) {
 		write_lock(&led_cdev->trigger_lock);
 		if (!led_cdev->trigger && led_cdev->default_trigger &&
-			    !strcmp(led_cdev->default_trigger, trigger->name))
+			   !strcmp(led_cdev->default_trigger, trigger->name) &&
+			   (!trigger->is_led_supported ||
+			    trigger->is_led_supported(led_cdev)))
 			led_trigger_set(led_cdev, trigger);
 		write_unlock(&led_cdev->trigger_lock);
 	}
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 88afcef..71175f6 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -70,6 +70,7 @@ struct led_trigger {
 	const char	 *name;
 	void		(*activate)(struct led_classdev *led_cdev);
 	void		(*deactivate)(struct led_classdev *led_cdev);
+	int		(*is_led_supported)(struct led_classdev *led_cdev);
 
 	/* LEDs under control by this trigger (for simple triggers) */
 	rwlock_t	  leddev_list_lock;
-- 
1.5.1.1-dirty

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[Anton Vorontsov]

On Mon, May 14, 2007 at 12:16:22AM +0100, Richard Purdie wrote:
> On Wed, 2007-05-09 at 20:03 +0400, Anton Vorontsov wrote:
> > +	led_cdev->trigger = trigger;
> >  	if (trigger) {
> >  		write_lock_irqsave(&trigger->leddev_list_lock, flags);
> >  		list_add_tail(&led_cdev->trig_list, &trigger->led_cdevs);
> > @@ -134,7 +140,6 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
> >  		if (trigger->activate)
> >  			trigger->activate(led_cdev);
> >  	}
> > -	led_cdev->trigger = trigger;
> >  }
> >  
> >  void led_trigger_set_default(struct led_classdev *led_cdev)
> 
> Why was the above was changed?
> 
> I think we've discussed this before

Yup.

> and it would be better to add a
> trigger parameter to activate/deactivate if we really need it.

Well.. Passing trigger parameter to activate/deactivate function is not
only purpose of that patch...


From: Anton Vorontsov <cbou@mail.ru>
Date: Mon, 14 May 2007 23:49:09 +0400
Subject: [PATCH] Attach trigger to LED prior calling activate function.

This change needed for two purposes:

1. When somebody sets trigger, and that trigger would setup
   brightness in its activate() function, and led driver would check
   if that trigger supported (used by hwtimer trigger and drivers
   supporting hw blinking LEDs, not in mainline yet).

2. If trigger would access itself through led_cdev in its activate()
   function.

Pros of that patch:

1. Just sane to do;
2. Trivial;
3. Can't break anything;
4. No new code, just one line movement.

Cons of applying that patch:

1. No mainline kernel user, but offshores.

Signed-off-by: Anton Vorontsov <cbou@mail.ru>
---
 drivers/leds/led-triggers.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 7fde7d0..d3ab5d0 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -134,6 +134,7 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
 			led_cdev->trigger->deactivate(led_cdev);
 		led_set_brightness(led_cdev, LED_OFF);
 	}
+	led_cdev->trigger = trigger;
 	if (trigger) {
 		write_lock_irqsave(&trigger->leddev_list_lock, flags);
 		list_add_tail(&led_cdev->trig_list, &trigger->led_cdevs);
@@ -141,7 +142,6 @@ void led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trigger)
 		if (trigger->activate)
 			trigger->activate(led_cdev);
 	}
-	led_cdev->trigger = trigger;
 }
 
 void led_trigger_set_default(struct led_classdev *led_cdev)
-- 
1.5.1.1-dirty

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[Richard Purdie]

On Tue, 2007-05-15 at 00:12 +0400, Anton Vorontsov wrote:
> This change needed for two purposes:
> 
> 1. When somebody sets trigger, and that trigger would setup
>    brightness in its activate() function, and led driver would check
>    if that trigger supported (used by hwtimer trigger and drivers
>    supporting hw blinking LEDs, not in mainline yet).

Why does led_cdev->trigger need to be set to do this? Any well written
LED drivers shouldn't need to look at it as the LED drivers shouldn't
have or need any knowledge about specific triggers. If they need this,
you hw blinking abstraction isn't generic.

I had reservations about the hw blinking support last time we discussed
it but I can't remember the specifics of that discussion now without
looking it up. I'm going to hold this patch until we're about the merge
something that needs it, if it really needs it.

> 2. If trigger would access itself through led_cdev in its activate()
>    function.

I can't see why you'd need to do this...

> Pros of that patch:
> 
> 1. Just sane to do;
> 2. Trivial;
> 3. Can't break anything;
> 4. No new code, just one line movement.
> 
> Cons of applying that patch:
> 
> 1. No mainline kernel user, but offshores.

2. Encourages bad code ;-). 

-- 
Richard

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[Anton Vorontsov]

On Mon, May 14, 2007 at 09:33:36PM +0100, Richard Purdie wrote:
> On Tue, 2007-05-15 at 00:12 +0400, Anton Vorontsov wrote:
> > This change needed for two purposes:
> > 
> > 1. When somebody sets trigger, and that trigger would setup
> >    brightness in its activate() function, and led driver would check
> >    if that trigger supported (used by hwtimer trigger and drivers
> >    supporting hw blinking LEDs, not in mainline yet).
> 
> Why does led_cdev->trigger need to be set to do this? Any well written
> LED drivers shouldn't need to look at it as the LED drivers shouldn't
> have or need any knowledge about specific triggers. If they need this,
> you hw blinking abstraction isn't generic.

Well, yes. Hw blinking abstraction not generic, in sense that driver
*must* know that it may be asked for "blinking trigger", i.e. hwtimer or
its derivate. There just isn't other way to do it, because
brightness_set function accepts only "enum led_brightness" argument,
because your initial intention: avoid other properties but brightness.
It's okay, but..

Because of all above, the only way I see hwtimer could be done (and
it done that way) on driver side is:

static void samcop_leds_set(struct led_classdev *led_cdev,
                            enum led_brightness b)
{
[...]
        if (b == LED_OFF)
                samcop_set_led(samcop_dev, PWM_RATE, led->hw_num, 0, 16);
        else {
                #ifdef CONFIG_LEDS_TRIGGER_HWTIMER
                if (led_cdev->trigger && led_cdev->trigger->is_led_supported &&
                               (led_cdev->trigger->is_led_supported(led_cdev) &
                                LED_SUPPORTS_HWTIMER)) {

/* ^^^ we're checking if trigger require us hwblinking, so it's hwtimer
 * or "derivate", i.e. any trigger passing hwtimer_data. LEDs API don't
 * have any other way to pass "blinking parameters" to the driver, i.e.
 * on/off delays. */

                        struct hwtimer_data *td = led_cdev->trigger_data;

                        if (!td) return;

                        samcop_set_led(samcop_dev, PWM_RATE, led->hw_num,
                          (PWM_RATE * td->delay_on)/1000,
                          (PWM_RATE * (td->delay_on + td->delay_off))/1000);
                }
                else
                #endif
                samcop_set_led(samcop_dev, PWM_RATE, led->hw_num, 16, 16);
        }

        return;
}

> I'm going to hold this patch until we're about the merge
> something that needs it, if it really needs it.

Fair enough.

Side note: I'm still dreaming about hwtimer -> timer merge, and make
it smart enough to use software blinking if hwtimer on/off delays limits
exceeded. Though, I'm still unsure when I'll start that work.

> > 2. If trigger would access itself through led_cdev in its activate()
> >    function.
> 
> I can't see why you'd need to do this...

Yes, there is no any user of that feature. Even not in handhelds.org tree
anymore. Though, hwtimer still using "1." reason.

> > 1. No mainline kernel user, but offshores.
> 
> 2. Encourages bad code ;-). 

:-) I'll agree here, but only after I or anyone else will make hwtimer other
way, generic one.

> -- 
> Richard

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.org/bd2

Re: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[kogiidena]

> On Wed, 2007-05-09 at 23:26 +0900, kogiidena wrote:
>> Hi Richard-san
>> The following three points were corrected.
>> 1.
>> > You can't do this since the trigger will appear for all LEDs and it only
>> > applies to a single LED. There has been previous discussion of LED
>> > specific triggers and we'll need that support before you can make
>> > something like this work. Nobody has sent me a patch for that yet and I
>> > haven't had time to write one...
>> The trigger name past "disk" was changed to the name "hard".
>> This is to mean the hardware of LANDISK controls LED.
>> The problem of "LED specific triggers" is solved.
>
> No, its not solved.
>
> Imagine I connect some device with its own LEDs and LED drivers. I'll
> use "corgi:amber" as an example.
>
> I run:
>
> 'cat /sys/class/leds/corgi:amber/triggers'
>
> I see "hard" listed.
>
> 'echo "hard" > /sys/class/leds/corgi:amber/triggers'
>
> doesn't work though...
>
> If its not going to work, it shouldn't be listed.
>
> Regards,
>
> Richard
>

I think, "Solved it" because I changed the source code.

LANDISK have two LED.
One is power-led. The other is status-led.
Power-led and status-led can respectively set "hard", and it operates normally.

'echo "hard" >  /sys/class/leds/power/triggers'
It works. Power-led behaves as always lights.
Power-led is controlled with hardware based on specific logic (CPLD) in LANDISK.

'echo "hard" >  /sys/class/leds/status/triggers'
It works too. Status-LED behaves as HDD-access-LED.
Status-led is controlled with hardware based on specific logic (CPLD) in LANDISK too.

+/*
+ * LANDISK hardware controled LED trigger
+ *
+ *  bit[3:0] = x0xx : power-led  is allways ON.
+ *  bit[3:0] = x1x0 : power-led  is ON.
+ *  bit[3:0] = x1x1 : power-led  is OFF.
+ *  bit[3:0] = 0xxx : status-led is HDD access-LED.
+ *  bit[3:0] = 1x0x : status-led is ON
+ *  bit[3:0] = 1x1x : status-led is OFF
+ */
+static void landisk_hard_trig_activate(struct led_classdev *led_cdev)
+{
+        unsigned long flags;
+        int bitmask;
+
+        bitmask = 0x04 << (led_cdev - &landisk_leds[0]);
+
+        spin_lock_irqsave(&landisk_led_lock, flags);
+        ctrl_outb(ctrl_inb(PA_LED) & ~bitmask, PA_LED);
+        spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+
+static void landisk_hard_trig_deactivate(struct led_classdev *led_cdev)
+{
+        unsigned long flags;
+        int bitmask;
+
+        bitmask = 0x04 << (led_cdev - &landisk_leds[0]);
+
+        spin_lock_irqsave(&landisk_led_lock, flags);
+        ctrl_outb(ctrl_inb(PA_LED) | bitmask, PA_LED);
+        spin_unlock_irqrestore(&landisk_led_lock, flags);
+}
+

kogiidena