| Message ID | 20170405064910.3162-4-kernel@kempniu.pl (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Darren Hart |
| Headers | show |
On Wed, Apr 05, 2017 at 08:49:02AM +0200, Micha?? K??pie?? wrote: > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c > index 59107a599d22..2f563aa00592 100644 > --- a/drivers/platform/x86/fujitsu-laptop.c > +++ b/drivers/platform/x86/fujitsu-laptop.c > @@ -360,41 +360,26 @@ static int set_lcd_level(int level) > { > acpi_status status = AE_OK; > acpi_handle handle = NULL; > - > - vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBLL [%d]\n", > - level); > - > - if (level < 0 || level >= fujitsu_bl->max_brightness) > - return -EINVAL; > - > - status = acpi_get_handle(fujitsu_bl->acpi_handle, "SBLL", &handle); > - if (ACPI_FAILURE(status)) { > - vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBLL not present\n"); > - return -ENODEV; > + char *method; > + > + switch (use_alt_lcd_levels) { > + case 1: > + method = "SBL2"; > + break; > + default: > + method = "SBLL"; > + break; > } This is not necessary something actionable, but I am wondering about the rationale of using a switch statement here given that there really are only two options. In my mind at least a simple "if" clause would be clearer and easier to read (with or without the braces): if (use_alt_lcd_levels) { method = "SBL2"; } else { method = "SBLL"; } Regards jonathan
On Fri, Apr 07, 2017 at 09:23:47PM +0930, I wrote: > On Wed, Apr 05, 2017 at 08:49:02AM +0200, Micha?? K??pie?? wrote: > > diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c > > index 59107a599d22..2f563aa00592 100644 > > --- a/drivers/platform/x86/fujitsu-laptop.c > > +++ b/drivers/platform/x86/fujitsu-laptop.c > > @@ -360,41 +360,26 @@ static int set_lcd_level(int level) > > { > > acpi_status status = AE_OK; > > acpi_handle handle = NULL; > > - > > - vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBLL [%d]\n", > > - level); > > - > > - if (level < 0 || level >= fujitsu_bl->max_brightness) > > - return -EINVAL; > > - > > - status = acpi_get_handle(fujitsu_bl->acpi_handle, "SBLL", &handle); > > - if (ACPI_FAILURE(status)) { > > - vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBLL not present\n"); > > - return -ENODEV; > > + char *method; > > + > > + switch (use_alt_lcd_levels) { > > + case 1: > > + method = "SBL2"; > > + break; > > + default: > > + method = "SBLL"; > > + break; > > } > > This is not necessary something actionable, but I am wondering about the > rationale of using a switch statement here given that there really are only > two options. In my mind at least a simple "if" clause would be clearer and > easier to read (with or without the braces): > > if (use_alt_lcd_levels) { > method = "SBL2"; > } else { > method = "SBLL"; > } Ah, the reason for the use of the switch was to prepare the way for patch 06/11 which adds an autodetection value to the definition of use_alt_lcd_levels. All good. Regards jonathan
diff --git a/drivers/platform/x86/fujitsu-laptop.c b/drivers/platform/x86/fujitsu-laptop.c index 59107a599d22..2f563aa00592 100644 --- a/drivers/platform/x86/fujitsu-laptop.c +++ b/drivers/platform/x86/fujitsu-laptop.c @@ -360,41 +360,26 @@ static int set_lcd_level(int level) { acpi_status status = AE_OK; acpi_handle handle = NULL; - - vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBLL [%d]\n", - level); - - if (level < 0 || level >= fujitsu_bl->max_brightness) - return -EINVAL; - - status = acpi_get_handle(fujitsu_bl->acpi_handle, "SBLL", &handle); - if (ACPI_FAILURE(status)) { - vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBLL not present\n"); - return -ENODEV; + char *method; + + switch (use_alt_lcd_levels) { + case 1: + method = "SBL2"; + break; + default: + method = "SBLL"; + break; } - - status = acpi_execute_simple_method(handle, NULL, level); - if (ACPI_FAILURE(status)) - return -ENODEV; - - return 0; -} - -static int set_lcd_level_alt(int level) -{ - acpi_status status = AE_OK; - acpi_handle handle = NULL; - - vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via SBL2 [%d]\n", - level); + vdbg_printk(FUJLAPTOP_DBG_TRACE, "set lcd level via %s [%d]\n", + method, level); if (level < 0 || level >= fujitsu_bl->max_brightness) return -EINVAL; - status = acpi_get_handle(fujitsu_bl->acpi_handle, "SBL2", &handle); + status = acpi_get_handle(fujitsu_bl->acpi_handle, method, &handle); if (ACPI_FAILURE(status)) { - vdbg_printk(FUJLAPTOP_DBG_ERROR, "SBL2 not present\n"); + vdbg_printk(FUJLAPTOP_DBG_ERROR, "%s not present\n", method); return -ENODEV; } @@ -463,10 +448,7 @@ static int bl_update_status(struct backlight_device *b) "Unable to adjust backlight power, error code %i\n", ret); - if (use_alt_lcd_levels) - ret = set_lcd_level_alt(b->props.brightness); - else - ret = set_lcd_level(b->props.brightness); + ret = set_lcd_level(b->props.brightness); if (ret != 0) vdbg_printk(FUJLAPTOP_DBG_ERROR, "Unable to adjust LCD brightness, error code %i\n", @@ -679,12 +661,8 @@ static void acpi_fujitsu_bl_notify(struct acpi_device *device, u32 event) if (oldb == newb) return; - if (disable_brightness_adjust != 1) { - if (use_alt_lcd_levels) - set_lcd_level_alt(newb); - else - set_lcd_level(newb); - } + if (disable_brightness_adjust != 1) + set_lcd_level(newb); sparse_keymap_report_event(input, oldb < newb, 1, true); }
Depending on the value of the use_alt_lcd_levels module parameter, one of two functions is used for setting LCD brightness level. These functions are almost identical and only differ in the name of the ACPI method they call. Instead of checking the value of use_alt_lcd_levels at each call site, move that check to set_lcd_level() and get rid of set_lcd_level_alt(). Signed-off-by: Michał Kępień <kernel@kempniu.pl> --- drivers/platform/x86/fujitsu-laptop.c | 54 +++++++++++------------------------ 1 file changed, 16 insertions(+), 38 deletions(-)