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

[Anton Vorontsov <cbou@mail.ru>]

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