linux-kernel - Re: [PATCH 6/7] platform/x86: fujitsu-laptop: Define constants for backlight power control

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180218195153.GA3665@kmp-mobile.hq.kempniu.pl>
Date:   Sun, 18 Feb 2018 20:51:53 +0100
From:   Michał Kępień <kernel@...pniu.pl>
To:     Jonathan Woithe <jwoithe@...t42.net>
Cc:     Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/7] platform/x86: fujitsu-laptop: Define constants for
 backlight power control

> > +/* FUNC interface - backlight power control */
> > +#define BACKLIGHT_POWER	0x4
> > +#define BACKLIGHT_OFF	0x3
> > +#define BACKLIGHT_ON	0x0
> 
> A minor detail: BACKLIGHT_OFF and BACKLIGHT_ON are potential parameter
> values while BACKLIGHT_POWER is essentially a parameter selector.  Should
> the name of BACKLIGHT_POWER reflect this difference?  It could be difficult
> to do without making the resulting identifier a little long.  The best I can
> come up with is BACKLIGHT_PARAM_POWER or (if a saving of one character is
> desired) BACKLIGHT_PARM_POWER.

So, I tried to somehow unify constant naming throughout the module a few
times by now, but it seems that whichever naming pattern I chose, there
was always some exception to the rule.  Of course the module code is not
to blame, it is caused by the firmware treating logically related
features (like controlling various LEDs) in diverse ways.

Thus, I am perfectly fine with using BACKLIGHT_PARAM_POWER for now,
because I do not have a better idea yet.  If I ever come up with a
constant naming scheme that would cover all the constants in the module
(or at least all those directly related to call_fext_func()), I will
propose it here.

> > @@ -818,7 +825,8 @@ static int acpi_fujitsu_laptop_add(struct acpi_device *device)
> >  	/* Sync backlight power status */
> >  	if (fujitsu_bl && fujitsu_bl->bl_device &&
> >  	    acpi_video_get_backlight_type() == acpi_backlight_vendor) {
> > -		if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2, 0x4, 0x0) == 3)
> > +		if (call_fext_func(fext, FUNC_BACKLIGHT, 0x2, BACKLIGHT_POWER,
> > +				   0x0) == 3)
> >  			fujitsu_bl->bl_device->props.power = FB_BLANK_POWERDOWN;
> >  		else
> >  			fujitsu_bl->bl_device->props.power = FB_BLANK_UNBLANK;
> 
> I'm curious: this fext function call is, I think, returning the current
> backlight power value.  If that's the case, is the value of 3 used in the
> test of the return function conceptually BACKLIGHT_OFF, and if so, should we
> use BACKLIGHT_OFF instead of the numeric constant?  It seems likely given
> that it results in the backlight power property being set to
> FB_BLANK_POWERDOWN.

Ah, yes, good catch.  Will fix, thanks.

-- 
Best regards,
Michał Kępień