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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 24.04.2012 20:08, Wolfgang Grandegger wrote:

> On 04/24/2012 05:41 PM, Oliver Hartkopp wrote:
>> On 24.04.2012 08:46, Wolfgang Grandegger wrote:
>>
>>
>>>> - The blink timer is implemented as:
>>>>   * keep on for delay_time
>>>>   * turn off and retrigger the timer
>>>>   * turn back on after delay
>>>>   so that the hot-path only contains a switch-case, some checks and a mod_timer.
>>>>
>>>> This is tested on an x86 with a custom usb-can interface and on a flexcan-based
>>>> Freescale ARM board. I'll post some patch to implement support in other drivers
>>>> if anyone is interested into testing this one.
>>>>
>>>> Any comments?
>>>
>>> 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.
>>
>>
>> Hello Wolfgang,
>>
>> what exactly do you mean with "timer class" ??
>>
>> To me the current implementation looks like standard jiffie-based timer usage.
>>
>> IMO hrtimers are not needed here - or what are you thinking of?
> 
> I mean that the code doing that kind of blinking should go to the timer
> class as generic function/feature, which other drivers could then use as
> well, similar to led_trigger_event or led_trigger_blink. Should not be a
> big deal, I think. See also my previous mail:
> 
>   http://marc.info/?l=linux-can&m=133425656510148&w=2
> 


Hm - as i see from the two examples there, the LEDs have a different
behaviour. E.g. on which use-case specific events they blink or set the LEDs
to fixed values.

I can't see any real benefit for these LED users to implement a generic LED
blinking framework. IMO using timers and the led_trigger_event stuff is the
appropriate abstraction level.

If you look into other networking drivers (e.g. the wireless) - they do it
with delayed_work:

http://lxr.linux.no/#linux+v3.3.1/drivers/net/wireless/ath/carl9170/led.c#L105

And everyone defines a specific struct like this one:

http://lxr.linux.no/#linux+v3.3.1/drivers/net/wireless/ath/carl9170/carl9170.h#L205

I think it's an enormous effort to unify all the LED users ...

Regards,
Oliver