On Wed, Sep 01, 2021 at 03:53:47PM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> > This patch adds support for HP Omen laptops.
> > It adds support for most things that can be controlled via the
> > Windows Omen Command Center application.
> >
> > - Fan speed monitoring through hwmon
> > - Platform Profile support (cool, balanced, performance)
> > - Max fan speed function toggle
> >
> > Also exposes the existing HDD temperature through hwmon since
> > this driver didn't use hwmon before this patch.
> >
> > This patch has been tested on a 2020 HP Omen 15 (AMD) 15-en0023dx.
> >
> > - V1
> > Initial Patch
> > - V2
> > Use standard hwmon ABI attributes
> > Add existing non-standard "hddtemp" to hwmon
> > - V3
> > Fix overflow issue in "hp_wmi_get_fan_speed"
> > Map max fan speed value back to hwmon values on read
> > Code style fixes
> > Fix issue with returning values from "hp_wmi_hwmon_read",
> > the value to return should be written to val and not just
> > returned from the function
> > - V4
> > Use DMI Board names to detect if a device should use the omen
> > specific thermal profile method.
> > Select HWMON instead of depending on it.
> > Code style fixes.
> > Replace some error codes with more specific/meaningful ones.
> > Remove the HDD temperature from HWMON since we don't know what
> > unit it's expressed in.
> > Handle error from hp_wmi_hwmon_init
> >
> > Signed-off-by: Enver Balalic <balalic.enver@xxxxxxxxx>
> > ---
> > drivers/platform/x86/Kconfig | 1 +
> > drivers/platform/x86/hp-wmi.c | 336 ++++++++++++++++++++++++++++++++--
> > 2 files changed, 325 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index d12db6c316ea..2ab0dcf5b598 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -434,6 +434,7 @@ config HP_WMI
> > depends on RFKILL || RFKILL = n
> > select INPUT_SPARSEKMAP
> > select ACPI_PLATFORM_PROFILE
> > + select HWMON
> > help
> > Say Y here if you want to support WMI-based hotkeys on HP laptops and
> > to read data from WMI such as docking or ambient light sensor state.
> > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> > index 027a1467d009..44030f513453 100644
> > --- a/drivers/platform/x86/hp-wmi.c
> > +++ b/drivers/platform/x86/hp-wmi.c
> > @@ -22,9 +22,11 @@
> > #include <linux/input/sparse-keymap.h>
> > #include <linux/platform_device.h>
> > #include <linux/platform_profile.h>
> > +#include <linux/hwmon.h>
> > #include <linux/acpi.h>
> > #include <linux/rfkill.h>
> > #include <linux/string.h>
> > +#include <linux/dmi.h>
> >
> > MODULE_AUTHOR("Matthew Garrett <mjg59@xxxxxxxxxxxxx>");
> > MODULE_DESCRIPTION("HP laptop WMI hotkeys driver");
> > @@ -39,6 +41,25 @@ MODULE_PARM_DESC(enable_tablet_mode_sw, "Enable SW_TABLET_MODE reporting (-1=aut
> >
> > #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
> > #define HPWMI_BIOS_GUID "5FB7F034-2C63-45e9-BE91-3D44E2C707E4"
> > +#define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
> > +
> > +/* DMI board names of devices that should use the omen specific path for
> > + * thermal profiles.
> > + * This was obtained by taking a look in the windows omen command center
> > + * app and parsing a json file that they use to figure out what capabilities
> > + * the device should have.
> > + * A device is considered an omen if the DisplayName in that list contains
> > + * "OMEN", and it can use the thermal profile stuff if the "Feature" array
> > + * contains "PerformanceControl".
> > + */
> > +static const char * const omen_thermal_profile_boards[] = {
> > + "84DA", "84DB", "84DC", "8574", "8575", "860A", "87B5", "8572", "8573",
> > + "8600", "8601", "8602", "8605", "8606", "8607", "8746", "8747", "8749",
> > + "874A", "8603", "8604", "8748", "886B", "886C", "878A", "878B", "878C",
> > + "88C8", "88CB", "8786", "8787", "8788", "88D1", "88D2", "88F4", "88FD",
> > + "88F5", "88F6", "88F7", "88FE", "88FF", "8900", "8901", "8902", "8912",
> > + "8917", "8918", "8949", "894A", "89EB"
> > +};
> >
> > enum hp_wmi_radio {
> > HPWMI_WIFI = 0x0,
> > @@ -89,10 +110,18 @@ enum hp_wmi_commandtype {
> > HPWMI_THERMAL_PROFILE_QUERY = 0x4c,
> > };
> >
> > +enum hp_wmi_gm_commandtype {
> > + HPWMI_FAN_SPEED_GET_QUERY = 0x11,
> > + HPWMI_SET_PERFORMANCE_MODE = 0x1A,
> > + HPWMI_FAN_SPEED_MAX_GET_QUERY = 0x26,
> > + HPWMI_FAN_SPEED_MAX_SET_QUERY = 0x27,
> > +};
> > +
> > enum hp_wmi_command {
> > HPWMI_READ = 0x01,
> > HPWMI_WRITE = 0x02,
> > HPWMI_ODM = 0x03,
> > + HPWMI_GM = 0x20008,
> > };
> >
> > enum hp_wmi_hardware_mask {
> > @@ -120,6 +149,13 @@ enum hp_wireless2_bits {
> > HPWMI_POWER_FW_OR_HW = HPWMI_POWER_BIOS | HPWMI_POWER_HARD,
> > };
> >
> > +
> > +enum hp_thermal_profile_omen {
> > + HP_OMEN_THERMAL_PROFILE_DEFAULT = 0x00,
> > + HP_OMEN_THERMAL_PROFILE_PERFORMANCE = 0x01,
> > + HP_OMEN_THERMAL_PROFILE_COOL = 0x02,
> > +};
> > +
> > enum hp_thermal_profile {
> > HP_THERMAL_PROFILE_PERFORMANCE = 0x00,
> > HP_THERMAL_PROFILE_DEFAULT = 0x01,
> > @@ -169,6 +205,8 @@ static struct platform_device *hp_wmi_platform_dev;
> > static struct platform_profile_handler platform_profile_handler;
> > static bool platform_profile_support;
> >
> > +static bool use_omen_thermal_profile;
>
> I think this variable is not necessary, you set it once,
> and read it once. Maybe you could add a direct function
> call to `detect_is_omen_thermal_profile()` in `thermal_profile_setup()`?
Got it. At first I had that variable just be `is_omen`, but I noticed in the
Windows Command Center code that not all omens support thermal profile
switching this way so i changed it to be more descriptive. Will change.
>
>
> > [...]
> > static int hp_wmi_read_int(int query)
> > {
> > int val = 0, ret;
> > @@ -302,6 +358,75 @@ static int hp_wmi_hw_state(int mask)
> > return !!(state & mask);
> > }
> >
> > +static int omen_thermal_profile_set(int mode)
> > +{
> > + char buffer[2] = {0, mode};
> > + int ret;
> > +
> > + if (mode < 0 || mode > 2)
> > + return -EINVAL;
> > +
> > + ret = hp_wmi_perform_query(HPWMI_SET_PERFORMANCE_MODE, HPWMI_GM,
> > + &buffer, sizeof(buffer), sizeof(buffer));
> > +
> > + if (ret)
> > + return ret < 0 ? ret : -EINVAL;
> > +
> > + return mode;
> > +}
> > +
> > +static bool detect_is_omen_thermal_profile(void)
> > +{
> > + int i;
> > +
> > + const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
>
> It seems that this can be NULL. Most users of this function have
> a NULL check after the call, so please add one here as well.
Got it.
>
>
> > +
> > + for (i = 0; i < ARRAY_SIZE(omen_thermal_profile_boards); i++) {
> > + if (strcmp(board_name, omen_thermal_profile_boards[i]) == 0)
> > + return true;
> > + }
>
> Please see the `match_string()` function.
Was looking for something like that. Thanks.
>
>
> > +
> > + return false;
> > +}
> > [...]
>
>
> Best regards,
> Barnabás Pőcze
Thanks for the suggestions,
Enver.
