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: [email protected]
backup email: [email protected]
irc://irc.freenode.org/bd2
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
