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

[Fabio Baltieri <fabio.baltieri@gmail.com>]

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 :-) )

Regards,
Fabio