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

[Oliver Hartkopp <socketcan@hartkopp.net>]

On 03.05.2012 23:49, Fabio Baltieri wrote:

> Hello everyone,
> 
> I'm back on the activity-trigger subject.
> 
> On Wed, Apr 25, 2012 at 12:04:56PM +0200, Wolfgang Grandegger wrote:
>>>>> 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.
>>>>>>
>>>> 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).
>>
>> The MAINAINERS file does not list a mailing list, therefore the LKML
>> should be used, with a CC to Richard Purdie, I think. While checking if
>> LED support is discussed, I found related mails. One shot timer LED
>> support seems to be a frequently discussed issue:
>>
>>   http://marc.info/?t=133489487700001&r=1&w=2
>>   http://marc.info/?l=linux-kernel&m=133331013422467&w=4
>>
>> As usual, it's better to send a patch than to ask a (general) questions.
>> I think we would get rather quick response.
> 
> I spent last couple of days trying to come up with a generic one-shot
> blink patch clean enough to get some chance of integration, but what I
> ended up with was these considerations in favor of framework-specific
> code:
> 
> The only clean way I see to do that is to implement the function as an
> extension of the actual soft-blink code.
> 
> That functions are used as fallback if no hw-blink is available and are
> just blindly toggling values on-off, so they have to be modified quite
> heavily to add necessary state information to support the behaviour of
> the actual code (like when a trigger is added for an already up
> interface) and the feature required by other usage cases (like the
> trigger currently discussed in the list - which is going in the way of a
> trigger-specific implemetation anyway).  This would involve adding flags
> and cases in both trigger and timer code.
> 
> So, I'm taking a break on this and trying to feed this argument: why the
> socketcan code would benefit from a specific implementation rather than
> a generic one?
> 
> Because overall code is *much* more simple and clean!
> 
> - the timer function body is just 12 lines of code, whithout special
>   cases, that's faster to execute and easy to understand (and debug).
> 
> - the event function for TX and RX paths is just some checks and a
>   mod_timer, without list iterations or locking, and as it's going to be
>   called in hard-irq context, I think this is a *strong* plus for the
>   implementation (the most important thing here is to get out of the way
>   as fast as possible, right? A generic implementation would only make
>   latencies worse here).
> 
> So, my point, do you still think that this feature would benefit of a
> generic triggering implementation?
> 
> (of course, I'm posting a v3 with just Oliver's Kconfig notes if you
> agree with my point :-) )
> 


Hi Fabio,

as the former implementation was fine to me there's no need to come up with
additional remarks for me ... btw. i think your implementation is clean,
simple and efficient. Therefore i would vote for it too.

I don't know if we should guide anyone to implement a generic led blinking
framework covering all the (existing) cases in the kernel. Adding your patches
would probably make the creation of such a framework more challenging to a
volunteer in terms of creating a solution which is that simple and efficient.

just my two cents ...

Regards,
Oliver