| Message ID | 20180211210727.12130-7-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Darren Hart |
| Headers | show |
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@kempniu.pl> > --- [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
> > +/* 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.
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c index 17779b8b7f30..0ee03d1fcbc4 100644 --- a/drivers/platform/x86/fujitsu-laptop.c +++ b/drivers/platform/x86/fujitsu-laptop.c @@ -101,6 +101,11 @@ #define ECO_LED 0x10000 #define ECO_LED_ON 0x80000 +/* FUNC interface - backlight power control */ +#define BACKLIGHT_POWER 0x4 +#define BACKLIGHT_OFF 0x3 +#define BACKLIGHT_ON 0x0 + /* Hotkey details */ #define KEY1_CODE 0x410 /* codes for the keys in the GIRB register */ #define KEY2_CODE 0x411 @@ -257,9 +262,11 @@ static int bl_update_status(struct backlight_device *b) if (fext) { if (b->props.power == FB_BLANK_POWERDOWN) - call_fext_func(fext, FUNC_BACKLIGHT, 0x1, 0x4, 0x3); + call_fext_func(fext, FUNC_BACKLIGHT, 0x1, + BACKLIGHT_POWER, BACKLIGHT_OFF); else - call_fext_func(fext, FUNC_BACKLIGHT, 0x1, 0x4, 0x0); + call_fext_func(fext, FUNC_BACKLIGHT, 0x1, + BACKLIGHT_POWER, BACKLIGHT_ON); } return set_lcd_level(device, b->props.brightness); @@ -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;
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@kempniu.pl> --- drivers/platform/x86/fujitsu-laptop.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)