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

[Richard Purdie <rpurdie@rpsys.net>]

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