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.
