Re: [RFC PATCH v2 1/2] can: add tx/rx LED trigger support

[Wolfgang Grandegger <wg@grandegger.com>]

Hi Fabio,

On 04/24/2012 09:02 PM, Fabio Baltieri wrote:
> Hi Wolfgang,
> 
> On Tue, Apr 24, 2012 at 08:46:29AM +0200, Wolfgang Grandegger wrote:
>> I still think that the blinking support should go to the timer class to
>> avoid duplicated code. Any good reason against? Apart from that the
>> patches look good.
>>
>> Wolfgang.
> 
> I can see you point and I considered your note about adding the
> one-shot-blink function to the led-class framework (sorry for not
> mentioning it in my first post).  Still, I ended up with this code for
> a couple of reasons:
> 
> - I think that the led_blink_set function is primarily used to configure
>   leds with hardware blinking (like i2c led drivers).  While it would be
>   possible to extend the function to get one-shot behavior and always
>   fallback on software blink, I think that that's out of the purpose of
>   the led-class, which should just translate on-off requests to
>   underlaying hardware.

Why, there is already led_tigger_event() and led_trigger_blink(). Why
should led_trigger_blink_once() then do not make sense?

> - I think that different drivers may want to obtain different on-off
>   behavior depending on the application.  For example in the ide-disk
>   case the user expects to see a steady-on LED on constant activity,
>   and that's how it's implemented, while in this case the on-if-up
>   keep-blinking-on-activity off-if-down makes much more sense. So I think
>   that even if a generic blink function were available, people would
>   still be using custom functions because they want to fine tune the
>   behavior for the application.  Also, maybe a function too generic may
>   impact on performance in critical paths.

The blinking could be specified with led_trigger_blink_once().

> - in this case, it looks to me like the implementation is as optimized
>   as it can be, in the sense that the hot-path does really only some
>   essential check and engage the timer and the timer function itself is
>   really short.  Also the final blinking effect is nice IMO :-)

For me it still does make sense to provide a generic
led_trigger_blink_once support. But well, go ahead if I'm the only one
with that opinion.

Wolfgang.