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

[Janusz Krzysztofik]

Tuesday 20 July 2010 13:54:47 Ralph Corderoy wrote:
> 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?

Hi Ralph,
Thanks for your time once more. I think you may be perfectly right with all 
your suggestions, however, I'd rather prefere the author or maintainer has a 
chance to suggest simplifying his original code while being at it. Let's wait 
for him to answer first ;).

Cheers,
Janusz

> > -             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.
>
>
> _______________________________________________
> e3-hacking mailing list
> e3-hacking at earth.li
> http://www.earth.li/cgi-bin/mailman/listinfo/e3-hacking