Re: [RFC PATCH] can: add tx/rx led trigger support

From: Wolfgang Grandegger <wg@grandegger.com>
To: Fabio Baltieri <fabio.baltieri@gmail.com>
Cc: Oliver Hartkopp <socketcan@hartkopp.net>,
	Martin Gysel <m.gysel@gmail.com>,
	linux-can@vger.kernel.org
Subject: Re: [RFC PATCH] can: add tx/rx led trigger support
Date: Thu, 12 Apr 2012 20:47:40 +0200	[thread overview]
Message-ID: <4F87234C.2040707@grandegger.com> (raw)
In-Reply-To: <CABkP77dzvdTFFTxUwAAPtTHFt2SzOGWRxet=bhX+c4LKpS20Gw@mail.gmail.com>

On 04/12/2012 07:28 PM, Fabio Baltieri wrote:
> On Thu, Apr 12, 2012 at 6:13 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> I think the original approach from Fabio was to follow the LED implementation
>>> from mac80211:
>>>
>>> "The implementation is similar to the MAC80211_LEDS one, and takes quite a lot
>>> of inspiration and some code from it."
>>>
>>> And IMHO we should follow this implementation as it is proved to be used in
>>> high traffic netdev environments. I wonder if some 'generic gpio LED'
>>> implementation will perform sufficiently.
>>
>> Yes, I agree. But that implementation does not use timers but toggles
>> the LED on/off for each new packet.
>>
>> http://lxr.linux.no/#linux+v3.3.1/net/mac80211/led.c#L15
> 
> That's correct, but I think it works for 80211 because there you
> always have some traffic. In the canbus case if implemented as
> "toggle" you'll end up with a led full-on when removing the cable with
> 50% probability - which I think is quite misleading.

Well, yes, I see that problem as well.

>> I would vote for that solution as well (because it's very lite and we
>> already have counters for rx and tx packets).
> 
> There are other implementation (which I took as example) in critical
> paths which blink leds on one-time events with timers:
> 
> ledtrig-ide-disk (with fixed 10ms timeout):
> http://lxr.linux.no/#linux+v3.3.1/drivers/leds/ledtrig-ide-disk.c#L28
> 
> netfilter's xt_LED trigger:
> http://lxr.linux.no/#linux+v3.3.1/net/netfilter/xt_LED.c#L69

So we will duplicate again almost the same code for CAN? No, please
don't...

> I think that with the timer called with a reasonably high delay
> (20-30ms) it should be quite lightweight. After all, most of the times
> embedded system's leds are gpio-mapped and will be handled by a bunch
> of function call and single memory write...

... but provide a patch for the LED class first implementing that
functionality, maybe even with a configurable delay. Note that there is
already a led_tigger_blink().

Wolfgang.