On Mon, Aug 23, 2021 at 04:40:08PM +0000, Barnabás Pőcze wrote:
> 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?
Yes, that is what's happening. I initially had this working as a python script via
the acpi_call module first, and python didn't overflow to -1 here so I didn't think of
that.
>
>
> > + 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.)
I copied this part from the other set/get methods in this driver, they
return -EINVAL on fail here.
>
>
> > +
> > + 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?
Yes, it should.
>
>
> > + 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 */
Noted, I ran this through checkpatch.pl and it didn't lint that,
thanks for the heads up.
>
>
> > + }
> > + default:
> > + return -EOPNOTSUPP;
> > + }
> > +}
> > [...]
>
>
> Best regards,
> Barnabás Pőcze
Thank you very much for the review, I'll fix these and anything else
that comes in and send out a V3.
Regards,
Enver.
