On Mon, Jan 14, 2019 at 12:13 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > > Hi, > > On 14-01-19 11:52, Rafael J. Wysocki wrote: > > On Mon, Jan 7, 2019 at 5:08 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote: > >> > >> This commit refactors the chassis-type detection introduced by > >> commit 53fa1f6e8a59 ("ACPI / video: Only default only_lcd to true on > >> Win8-ready _desktops_") (where desktop means anything without a builtin > >> screen). > >> > >> The DMI chassis_type is an unsigned integer, so rather then doing a > >> whole bunch of string-compares on it, convert it to an int and feed > >> the result to a switch case. > >> > >> Note the switch case uses hex values, this is done because the spec > >> uses hex values too. This changes the check for "Main Server Chassis" > >> from checking for 11 decimal to 11 hexadecimal, this is a bug fix, > >> the original check for 11 decimal was wrong. > >> > >> Fixes: 53fa1f6e8a59 ("ACPI / video: Only default only_lcd to true ...") > >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > >> --- > >> drivers/acpi/acpi_video.c | 20 +++++++++++++------- > >> 1 file changed, 13 insertions(+), 7 deletions(-) > >> > >> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c > >> index f0b52266b3ac..ba5c2f70babe 100644 > >> --- a/drivers/acpi/acpi_video.c > >> +++ b/drivers/acpi/acpi_video.c > >> @@ -2124,21 +2124,27 @@ static int __init intel_opregion_present(void) > >> return opregion; > >> } > >> > >> +/* Check if the chassis-type indicates there is no builtin LCD panel */ > >> static bool dmi_is_desktop(void) > >> { > >> const char *chassis_type; > >> + unsigned long type; > >> > >> chassis_type = dmi_get_system_info(DMI_CHASSIS_TYPE); > >> if (!chassis_type) > >> return false; > >> > >> - if (!strcmp(chassis_type, "3") || /* 3: Desktop */ > >> - !strcmp(chassis_type, "4") || /* 4: Low Profile Desktop */ > >> - !strcmp(chassis_type, "5") || /* 5: Pizza Box */ > >> - !strcmp(chassis_type, "6") || /* 6: Mini Tower */ > >> - !strcmp(chassis_type, "7") || /* 7: Tower */ > >> - !strcmp(chassis_type, "11")) /* 11: Main Server Chassis */ > >> - return true; > >> + if (kstrtoul(chassis_type, 10, &type) != 0) > >> + return false; > >> + > > > > Why don't you write the code below as a single line: > > > > return (type >= 0x03 && type <= 0x07) || type == 0x11; > > If you look at the second patch in the set it adds a check for type == 0x10, so > then this would become: > > return (type >= 0x03 && type <= 0x07) || type == 0x10 || type == 0x11; That you could write as return (type >= 0x03 && type <= 0x07) || (type & 0x10); too. ;-) > I'm afraid that in the future if we want to add a check for one or > two more values this will become messy. Also the switch-case + > per line comment makes it much clearer which value is what then the > comment you suggest IMHO. Well, fair enough I guess, but can you make the switch a fall-through one? The multiple 'return true;' statements really look odd to me.