-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
razer_hanbo: Add Razer Hanbo Chroma AIO driver #73
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an initial review, thanks.
drivers/hwmon/razer_hanbo.c
Outdated
case FAN_CHANNEL: | ||
switch (val) { | ||
case 1: | ||
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplify this by storing 0x14
, 0x32
etc. from the switch in a variable and then just call
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF, magic_value);
if (ret < 0)
goto unlock_and_return;
break;
at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored in 1159c87
I think it is.
Yes, through debugfs, like fw version.
You'll need to implement a 'driver' there as well.
Not sure what this means? |
* Refactor hanbo_hid_validate_header to use less arguments. * Refactor hanbo_hid_profile_send to use less arguments. * Include device serial number in DebugFS. * Removed power management placeholder code. * 'ret' used as return from more functions. * Replaced a number of values with appropriate error codes or constants. * mutex_unlocks occur after spin_unlocks. * Removed redundant goto statements in hanbo_hwmon_write. * Minor formatting updates and changes to constants for clarity. Signed-off-by: Joseph East <eastyjr@gmail.com>
Added in 1159c87
I'll have a poke around then.
There was a power management placeholder there for HID reset/resume, I was tossing up whether to use it or not but it has been removed as of 1159c87. |
drivers/hwmon/razer_hanbo.c
Outdated
switch (val) { | ||
case 1: | ||
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF); | ||
break; | ||
case 2: | ||
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF); | ||
break; | ||
case 3: | ||
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF); | ||
break; | ||
case 4: | ||
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF); | ||
break; | ||
default: | ||
ret = -EINVAL; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switch (val) { | |
case 1: | |
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF); | |
break; | |
case 2: | |
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF); | |
break; | |
case 3: | |
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF); | |
break; | |
case 4: | |
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF); | |
break; | |
default: | |
ret = -EINVAL; | |
} | |
if (val < 1 || val > 4) { | |
ret = -EINVAL; | |
goto unlock_and_return; | |
} | |
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF); | |
break; |
or something like that would be better as to not repeat the same code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored in 8a21201. I chose to sanitise the remaining input arguments in hanbo_hid_profile_send allowing for less switch statements.
* Documentation updated (serial number & sysfs table) * hanbo_hwmon_write simplification by arg checking in hanbo_profile_send * All hwmon_ops return 0 on success as per hwmon.h definition * CPU reference temperature only updated if HID transfer succeeds * Driver will emit hid_fail and bail out if init sequence fails. Signed-off-by: Joseph East <eastyjr@gmail.com>
Signed-off-by: Joseph East <eastyjr@gmail.com>
Signed-off-by: Joseph East <eastyjr@gmail.com>
This is a hwmon driver I've been writing for the Razer Hanbo Chroma AIO cooler. Specifically the 360mm version, though if the 240mm uses the same protocol minus one fan then that should work as well.
The nzxt-kraken3 and gigabyte_waterforce drivers served as inspiration for this. I have been using this driver on my own Hanbo 360mm under Linux 6.13.7.
I believe the documentation pretty much sums up the exposed functionality though I have some queries.
In addition to the fan curves I have made a custom attribute for PWM setpoints so that one could compare the target vs reported PWM. Both values come from HID reports and are reflective of the active profile. Is this an appropriate use of a custom attribute?(yes)I can acquire the serial number for the AIO in firmware, is this worth exposing somewhere in the driver?(incorporated)Is there anything required liquidctl side to use this?(to be addressed later)Is it worth while implementing some kind of online reset without reloading the module?(removed)In terms of TODOs: