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

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]

On 04/25/2012 09:26 AM, Wolfgang Grandegger wrote:
> 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.

+1
At least start a discussion on the LED list (I don't know which list
that is).

Marc


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]