Re: PATCH: Create new LED trigger for CPU activity (ledtrig-cpu)

["Randy.Dunlap" <rdunlap@xenotime.net>]

On Wed, 5 Jul 2006 21:24:17 -0400 Thomas Tuttle wrote:

> Here is a patch I wrote that creates a new LED trigger for CPU
> activity.  It creates a new config option, CONFIG_LEDS_TRIGGER_CPU, as
> well as four suboptions to select the combination of user, nice,
> system, and iowait that should turn the LED on.

What "the LED" does it turn on/off?
I'm curious how I can use it.

> It's loosely based on the ledtrig-ide-disk plugin, but only for
> general guidance.  It doesn't modify anything in the CPU scheduling
> code, for better or for worse; it is less efficient, because it checks
> the CPU time every 5ms instead of "knowing" when the CPU is idle or
> active, but it doesn't require changes to any other code, and is much
> less hairy for not digging into the scheduling stuff.  (It's also more
> flexible, because it can deal with user/nice/system/iowait time
> without much effort.)
> 
> The same disclaimers from my last email (the asus_acpi LED support
> one) apply--I haven't written much kernel code; this is diffed against
> 2.6.17.1, not .3, but I don't think anything has changed; I've tested
> it, but no guarantees it's perfect; and I apologize for the MIME type
> of text/x-patch, but it should work.

It may not matter, but generally you should diff against the latest
linus-mainline kernel, e.g., 2.6.7-git25 or Linus's git tree.

> Constructive comments would be greatly appreciated.

The attachment makes it difficult to review/comment.  Apparently
gmail munges inline patches so that's not an option (from what I
have read here on lkml).

Oh, where is the LED IDE patch?

Patch comments:  all seem to be for style etc.

+#define UPDATE_INTERVAL (5) // ms

Don't use // style comments, use /* ... */.

+static cputime64_t cpu_usage(void) {

Function { and } should be on separate lines.

+	for_each_possible_cpu(i) {
+		user = cputime64_add(user, kstat_cpu(i).cpustat.user);
+		nice = cputime64_add(nice, kstat_cpu(i).cpustat.nice);
+		system = cputime64_add(system, kstat_cpu(i).cpustat.system);
+		idle = cputime64_add(idle, kstat_cpu(i).cpustat.idle);
+		iowait = cputime64_add(iowait, kstat_cpu(i).cpustat.iowait);
+	}

We don't like ifdefs in C code very much, but since there are some
below here, why not here also?

+	if (used_cputime > 0) {
+		led_trigger_event(ledtrig_cpu, LED_FULL);
+	} else {
+		led_trigger_event(ledtrig_cpu, LED_OFF);
+	}

No braces for 1-statement "blocks".

---
~Randy