LinuxLists.cc - [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports

[permalink] [raw]

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

> 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 <[email protected]>
>
[...]
> +/* 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

2007-05-09 14:26:30

[permalink] [raw]

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

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 <[email protected]>
---
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 <[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/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 <[email protected]>");
+MODULE_DESCRIPTION("landisk LED driver");
+MODULE_LICENSE("GPL");

2007-05-09 15:13:37

[permalink] [raw]

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

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

2007-05-09 16:04:54

[permalink] [raw]

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

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

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;

2007-05-09 21:48:34

[permalink] [raw]

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

> 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

2007-05-10 11:52:24

[permalink] [raw]

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

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

---
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 <[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/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 <[email protected]>");
+MODULE_DESCRIPTION("landisk LED driver");
+MODULE_LICENSE("GPL");

2007-05-10 14:05:08

by Paul Mundt

[permalink] [raw]

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

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.

2007-05-11 15:03:27

[permalink] [raw]

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

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 <[email protected]>
---
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 <[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/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 <[email protected]>");
+MODULE_DESCRIPTION("landisk LED driver");
+MODULE_LICENSE("GPL");

2007-05-12 04:01:43

[permalink] [raw]

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

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 <[email protected]>
---
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 <[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/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 <[email protected]>");
+MODULE_DESCRIPTION("landisk LED driver");
+MODULE_LICENSE("GPL");

2007-05-13 23:16:43

[permalink] [raw]

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

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

2007-05-14 19:59:08

[permalink] [raw]

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

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

2007-05-14 20:15:14

[permalink] [raw]

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

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

2007-05-14 20:34:16

[permalink] [raw]

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

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

2007-05-14 21:15:57