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

[Jonathan Woithe]

On Sun, Feb 11, 2018 at 10:07:26PM +0100, Micha?? K??pie?? wrote:
> Use named constants instead of integers in call_fext_func() invocations
> related to backlight power control in order to more clearly convey the
> intent of each call.
> 
> Signed-off-by: Micha?? K??pie?? <kernel@xxxxxxxxxx>
> ---
[cut]
> +/* 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.

[cut]
> @@ -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.

Regards
  jonathan