Convert kernel hwmon interface to use PWM (0-255) intsead of RPM #261
Conversation
johnfanv2
changed the title
|
LGTM, I gonna make the python changes and this can be merge. #185 need to be update since it have some fixes to new devices |
* this is the cherry-pick commit of #185 need testing Signed-off-by: Gonçalo Negrier Duarte <[email protected]>
|
The commit I just push need be tested in a newer device but the code did compile. @antheas can you give a look if I miss something. |
|
kernel lgmt I would personally keep using rpm for older models and change the driver name though. Perhaps even split the EC fan stuff to a different module that is out-of-tree, and try to merge only the WMI to the kernel. |
Probably you are right and that would need to be done, but since I don't think the drive would be push to mainline anytime soon, let wait. |
MrDuartePT
force-pushed
the
migrate-to-pwm
branch
4 times, most recently
from
795167b
to
0c998f4
Compare
MrDuartePT
force-pushed
the
migrate-to-pwm
branch
from
0c998f4
to
fb7db66
Compare
| @@ -1712,6 +1712,12 @@ enum OtherMethodFeature { | |||
| OtherMethodFeature_C_U1 = 0x05010000, | |||
| OtherMethodFeature_TEMP_CPU = 0x05040000, | |||
| OtherMethodFeature_TEMP_GPU = 0x05050000, | |||
| OtherMethodFeature_FAN_FULLSPEED = 0x04020000, | |||
There was a problem hiding this comment.
What is the source for that? Is there already a function that uses it that is not in this commit? Maybe you already have info how to use it. It is currently not used in this file.
| u32 FST6; | ||
| u32 FST7; | ||
| u32 FST8; | ||
| u32 FST9; |
There was a problem hiding this comment.
What is FTLE? Length?
FSTL: is this field renamed in Legion Go?
FST* = temp;
FSS* = speed; is it 0-100 (percent), pwn (0-255), or in rpm?
| } __packed; | ||
|
|
||
| static ssize_t wmi_read_fancurve_custom(const struct model_config *model, | ||
| struct fancurve *fancurve) | ||
| { | ||
| u8 buffer[88]; | ||
| struct WMIFanTableRead fantable; |
There was a problem hiding this comment.
Good idea. By adding this new fields we get exactly 88 Bytes. These fields where not defined in the ACPI code of other models.
| fancurve->current_point_i = 0; | ||
| fancurve->size = 10; | ||
| fancurve->size = fantable.FSTL; |
There was a problem hiding this comment.
Hmm. This does not work for other models because FSTL is not set to 10 but just not set.
| true); | ||
| err = wmi_exec_arg(WMI_GUID_LENOVO_FAN_METHOD, 0, | ||
| WMI_METHOD_ID_FAN_SET_TABLE, buffer, sizeof(buffer)); | ||
| WMI_METHOD_ID_FAN_SET_TABLE, &fan, sizeof(fan)); |
There was a problem hiding this comment.
In the old code we write a buffer of size 0x20. Now we write an object of size 0x34. Will this break stuff in old models?
| @@ -5694,6 +5736,34 @@ static struct attribute *fancurve_hwmon_attributes[] = { | |||
| &sensor_dev_attr_pwm1_mode.dev_attr.attr, NULL | |||
| }; | |||
|
|
|||
| static struct attribute *fancurve_hwmon_attributes_legion_go[] = { | |||
There was a problem hiding this comment.
Why create new hwmon entries? Do we need new ones with other properties?
| &sensor_dev_attr_pwm1_auto_point10_pwm.dev_attr.attr, | ||
| // These should be read only if the driver is properly implemented | ||
| // and either fail to set or noop | ||
| // &sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr, |
There was a problem hiding this comment.
I guess, we can make set read/write properties depending on model or just error out/ignore it.
| @@ -5720,6 +5790,11 @@ static const struct attribute_group legion_hwmon_fancurve_group = { | |||
| .is_visible = legion_hwmon_is_visible, | |||
| }; | |||
|
|
|||
| static const struct attribute_group legion_go_hwmon_fancurve_group = { | |||
There was a problem hiding this comment.
See above. I guess, this might be removed.
This will make it compliant with Kernel standard for hwmon and compatible with different fan curves (rpm, percentage, ...) of different models.