Re: [E3-hacking] [PATCH] LEDS: Add output invertion option to backlight trigger

Hi Janusz,

  int brightness;
  int old_status;
  struct notifier_block notifier;

• unsigned invert;
  

};

I think invert here is only ever 0 or 1 as you convert the sscanf()'d unsigned with !!. Similarly, old_status is only ever UNBLANK or BLANK, 0 or 1. Could they both be smaller than int/unsigned and share a word?

•         if (*blank && n->old_status == UNBLANK) {
  

•         if ((!n->invert && *blank && n->old_status == UNBLANK) ||
  

•                     (n->invert && !*blank && n->old_status == BLANK)) {
                  n->brightness = led->brightness;
                  led_set_brightness(led, LED_OFF);
  

•                 n->old_status = BLANK;
  

•         } else if (!*blank && n->old_status == BLANK) {
  

•                 n->old_status = n->invert ? UNBLANK : BLANK;
  

•         } else if ((!n->invert && !*blank && n->old_status == BLANK) ||
  

•                     (n->invert && *blank && n->old_status == UNBLANK)) {
                  led_set_brightness(led, n->brightness);
  

•                 n->old_status = UNBLANK;
  

•                 n->old_status = n->invert ? BLANK : UNBLANK;
          }
  

I think you're hampered here in readability by the existing code storing BLANK in old_status rather than calling it, say, is_blanked and dropping the #defines of BLANK and UNBLANK. Even so, instead of

if ((!n->invert && *blank && n->old_status == UNBLANK) || (n->invert && !*blank && n->old_status == BLANK)) { n->brightness = led->brightness; led_set_brightness(led, LED_OFF); n->old_status = n->invert ? UNBLANK : BLANK; } else if ((!n->invert && !*blank && n->old_status == BLANK) || (n->invert && *blank && n->old_status == UNBLANK)) { led_set_brightness(led, n->brightness); n->old_status = n->invert ? BLANK : UNBLANK; }

would

if (!*blank == n->old_status) { n->old_status = !n->old_status;

if (n->invert != n->old_status) { n->brightness = led->brightness; led_set_brightness(led, LED_OFF); } else { led_set_brightness(led, n->brightness); } }

be an improvement?

• return sprintf(buf, "%s\n", n->invert ? "yes" : "no");
  

Presumably, if the kernel had

char *noyes[] = { "no", "yes" };

somewhere then everything could refer to that one copy instead of there being multiple string literals about? Same with "false", "true", etc. Just an aside, probably outside the scope of your patch. :-)

Cheers, Ralph.