On 8/22/21 11:53 AM, Barnabás Pőcze wrote:
Hi
2021. augusztus 22., vasárnap 20:10 keltezéssel, Enver Balalic írta:
[...]
+static int hp_wmi_hwmon_init(void)
+{
+ struct device *dev = &hp_wmi_platform_dev->dev;
+ struct device *hwmon;
+
+ hwmon = devm_hwmon_device_register_with_groups(dev, "hp", &hp_wmi_driver,
+ hwmon_attribute_groups);
I think you should use
[devm_]hwmon_device_register_with_info()
as it creates sysfs attributes for you, etc. You wouldn't need to manually
create device attributes, and you wouldn't need fan{1,2}_show() at all.
Correct. Also, the code as written doesn't really make sense as hwmon driver
because all its attributes are non-standard. The "sensors" command will show
nothing.
I failed to take a look at the documentation instead i just looked at a
different laptop's WMI driver as a reference. My bad.
If you don't want to use standard hwmon ABI attributes, please don't register
a hwmon driver. The non-standard attributes can reside in the platform
sysfs directory. There is already the non-standard "hddtemp", so that would
not make much of a difference anyway. On top of that, "max_fan" is added as
non-hwmon attribute, making the hwmon registration even more pointless.
Guenter
I would like to expose these fan speeds properly, and hwmon seemed like
the way to do it. I'll redo this part to match how it should be done in the
documentation.
The questions I have are:
The only value I have is the current fan speed in RPM, I don't have
the rest of the values like min,max,pulses,target. Is it ok to implement this
in hwmon if I don't have those values? Can I use default values in place of
the ones I don't have, or should i omit those fields entirely.
I can assume min speed to be 0 since it does turn off the fans at light load
but in the case of the max speed property, I don't know what it is. Do I omit
that field entirely or ?
I think you can simply omit what you don't have. In your case,
I imagine you'll have something like this:
static const struct hwmon_channel_info *info[] = {
HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT),
HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE),
NULL
};
static const struct hwmon_ops ops = {
.is_visible = ...,
.read = ...,
.write = ...,
};
static const struct hwmon_chip_info chip_info = {
.ops = &ops,
.info = info,
};
The "max_fan" that I added is not a RPM value, it's a simple toggle in WMI
that either sets the fans to blow at their max speed (1), or it sets them
to auto (0), this matches what the windows omen command center does.
I don't know of a way to expose this "properly" so i just added a simple
attribute. Is there a proper way to expose this behaviour ?
I believe the proper way to expose that is via a pwmN_enable attribute on the
hwmon device, this should be created by the hwmon subsystem when it sees the
HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE),
channel in the info struct.
If both fans are controlled by one switch, I think it's fine to just have a
single pwmN_enable attribute. But Guenter can probably tell you more.
For this attribute the value 2 means automatic, 1 means manual, 0 means full speed
as far as I'm aware (at least this is what thinkpad_acpi does). If manual fan
control is not available, returning -EOPNOTSUPP is probably fine.
That, or -EINVAL.
I would also suggest to create temp1_input for the HD temperature. We can not drop
the existing attribute for backwards compatibility reasons, but it would make sense
to expose the HD temperature as standard attribute (possibly with an associated
_name attribute).
Guenter
