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

[Ralph Corderoy]

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.