Hi
2021. augusztus 23., hétfő 16:15 keltezéssel, Enver Balalic írta:
> 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
>
> Signed-off-by: Enver Balalic <balalic.enver@xxxxxxxxx>
> ---
> drivers/platform/x86/Kconfig | 1 +
> drivers/platform/x86/hp-wmi.c | 302 ++++++++++++++++++++++++++++++++--
> 2 files changed, 292 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index d12db6c316ea..f0b3d94e182b 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -431,6 +431,7 @@ config HP_WMI
> tristate "HP WMI extras"
> depends on ACPI_WMI
> depends on INPUT
> + depends on HWMON
> depends on RFKILL || RFKILL = n
> select INPUT_SPARSEKMAP
> select ACPI_PLATFORM_PROFILE
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index 027a1467d009..20cf8a32e76e 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -22,6 +22,7 @@
> [...]
> enum hp_wmi_command {
> HPWMI_READ = 0x01,
> HPWMI_WRITE = 0x02,
> HPWMI_ODM = 0x03,
> + HPWMI_GM = 0x20008,
^^^^
The other command values are aligned using a single tab.
> };
> [...]
> +static int hp_wmi_get_fan_speed(int fan)
> +{
> + int fsh, fsl, fan_speed;
> + char fan_data[4] = { fan, 0, 0, 0 };
> +
> + int ret = hp_wmi_perform_query(HPWMI_FAN_SPEED_GET_QUERY, HPWMI_GM,
> + &fan_data, sizeof(fan_data),
> + sizeof(fan_data));
> +
> + if (ret != 0)
> + return -EINVAL;
> +
> + fsh = fan_data[2];
> + fsl = fan_data[3];
> +
> + // sometimes one of these can be negative
If the speed is e.g. 1279 RPM, that's 0x04FF, which will be 4 and -1 if you interpret
the bytes as a signed values on x86. Isn't this what's happening here? Or is the
reason known?
> + fan_speed = ((fsh > 0 ? fsh : 0) << 8) | (fsl > 0 ? fsl : 0);
> +
> + return fan_speed;
> +}
> [...]
> +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;
I think something like EIO would be more appropriate than EINVAL.
(In other functions, too.)
> +
> + return mode;
> +}
> [...]
> +static int omen_thermal_profile_get(void)
I would put this function next to `omen_thermal_profile_set`.
> +{
> + u8 data;
> +
> + int ret = ec_read(HP_OMEN_EC_THERMAL_PROFILE_OFFSET, &data);
> +
> + if (ret)
> + return ret;
> +
> + return data;
> +}
> [...]
> +static int hp_wmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + switch (type) {
> + case hwmon_temp:
> + return hp_wmi_read_int(HPWMI_HDDTEMP_QUERY);
> + case hwmon_fan:
> + int ret = hp_wmi_get_fan_speed(channel);
> +
> + if (ret < 0)
> + return -EINVAL;
> + *val = ret;
> + return 0;
> + case hwmon_pwm:
> + return hp_wmi_fan_speed_max_get();
Shouldn't this "mapped back"? 1 -> 0, 0 -> 2?
> + default:
> + return -EINVAL;
> + };
^
This semicolon is not needed.
> +}
> +
> +static int hp_wmi_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + switch (type) {
> + case hwmon_pwm:
> + switch (val) {
> + case 0:
> + // 1 is no fan speed control(automatic), which is 1 for us
^^
Small thing, but I'd put a space there.
> + return hp_wmi_fan_speed_max_set(1);
> + case 2:
> + // 2 is automatic speed control, which is 0 for us
> + return hp_wmi_fan_speed_max_set(0);
> + default:
> + // we don't support manual fan speed control
> + return -EOPNOTSUPP;
/* this type of comments is preferred */
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> [...]
Best regards,
Barnabás Pőcze
