lkml.org 
[lkml]   [2007]   [May]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/2] leds:arch/sh/boards/landisk LEDs supports
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-05-14 23:19    [W:0.042 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site