Re: Iwlwifi and LEDS?

[Richard Purdie <rpurdie@rpsys.net>]

On Fri, 2010-05-28 at 13:22 +0200, Johannes Berg wrote:
> Richard, please see below after the *** -- I have a question for you.
> 
> On Fri, 2010-05-28 at 13:01 +0200, Gregy wrote:
> > > If you want to be able to manipulate the LED in other ways than the
> > > driver supports you could look into adding a debugfs file that
> > > manipulates the driver's led variables (allow_blinking,
> > > last_blink_time, ...). There is already a readable led file in debugfs,
> > > but not writable.
> > >
> > > Reinette
> > 
> > Ok, I have tried that with partial success. It allows me to change led
> > status "mid-flight" but if led is blinking it sometimes works only
> > after second try. Also my understanding of C and kernel programming is
> > very limited so I probably made some mistakes. Could you please look
> > at it?
> 
> I don't think this is the correct approach. I guess since the patch you
> referenced is mine, I should comment.
> 
> Prior to my patch, iwlwifi would register LEDs in the LED subsystem, but
> it didn't really properly do that since a lot of internal code just
> updates the LEDs. Additionally, by default the requirement is that the
> LED blinks according to traffic. Also, the blinking itself is done by
> the device, it is not done by the host CPU.
> 
> At the time of the patch, the LED subsystem didn't support blinking.
> This has changed now, I think, but previously it was not possible to set
> the LED to "blinking" like we need to have it. Additionally the LED
> trigger registration code that I removed was way more code for much less
> functionality than it should have been.
> 
> There's one proper way to fix this, but it is somewhat involved. Yes, I
> could have implemented that, but under the given time constraints, I
> opted for the cleanup that simply removed functionality that wasn't
> fully functional.
> 
> (*** mark for Richard)
> 
> The proper way to do this now would be to rewrite the LED code in
> iwlwifi to:
> 
> 1) register an LED again, which implements the blink_set() callback and
> programs the hw accordingly. This is essentially implementing
> iwl_led_pattern(), but with on_time/off_time parameters.
> 
> 2) Convert the current caller of iwl_led_pattern() to be an LED trigger
> that registers with the LED system. It would call the blink_set() for
> the LED it is connected to. Ideally, there should be some common
> software emulation if the LED has no blink_set(). Richard, is there a
> "make LED blink" callback that would start a timer for that LED? The
> timer trigger uses it but doesn't seem to be usable itself for other
> triggers?
> 
> 3) start the LED on device start, stop it on device stop
> 
> 4) depending on the module's led_mode, connect the LED to the
> appropriate default trigger, e.g. mac80211's assoc trigger or normally
> of course the new iwlwifi-blink trigger.

I have to admit I don't fully understand what the capabilities of your
hardware are.

Certainly registering a trigger with the LED system, maybe only
appearing for this specific hardware sounds like the way to go. Since
this will only appear as a trigger for this hardware, the trigger can
then poke whatever it needs into the hardware to work as a traffic
indication?

Cheers,

Richard