[E3-hacking] [PATCH] LEDS: Add output invertion option to backlight trigger
Janusz Krzysztofik
jkrzyszt at tis.icnet.pl
Tue Jul 20 18:52:26 BST 2010
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
More information about the e3-hacking
mailing list