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

[Richard Purdie <rpurdie@rpsys.net>]

> 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