Messages in this thread | | | Date | Tue, 15 May 2007 01:13:36 +0400 | From | Anton Vorontsov <> | Subject | Re: [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/
| |