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

[Wolfgang Grandegger <wg@grandegger.com>]

On 04/24/2012 08:57 PM, Oliver Hartkopp wrote:
> 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.

It's not the task of the CAN driver to let LEDs blink! Or what should
the LED class then be good for?

> 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 ...

Yes, they do that because there is no generic support. I believe that
the "one-shot-blinking" is useful in general which new drivers may use.
I did not ask for adapting old drivers.

Wolfgang.