Skip to content
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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

dripsnek
Copy link

@dripsnek dripsnek commented Mar 31, 2025

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.

  • Naming conventions - does anything need to change?
  • 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)
  • Are packet captures need to form part of the pull request?
  • 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:

  • I'd like some packet dumps from models running older firmware to check compatibility and get a definitive format of the firmware version field.
  • There is a PWM scaling hack on the sysfs side to make lm-sensors show a value closer to what is reported over the wire. Not sure of the best way to tidy that up.
  • Test for compatibility with userland RGB control software. I'm looking at adding support for OpenRGB soonish.

Copy link
Member

@aleksamagicka aleksamagicka left a 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.

case FAN_CHANNEL:
switch (val) {
case 1:
ret = hanbo_hid_profile_send(priv, channel, val & 0xFF,
Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored in 1159c87

@aleksamagicka
Copy link
Member

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?

I think it is.

I can acquire the serial number for the AIO in firmware, is this worth exposing somewhere in the driver?

Yes, through debugfs, like fw version.

Is there anything required liquidctl side to use this?

You'll need to implement a 'driver' there as well.

Is it worth while implementing some kind of online reset without reloading the module?

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>
@dripsnek
Copy link
Author

dripsnek commented Apr 1, 2025

Yes, through debugfs, like fw version.

Added in 1159c87

You'll need to implement a 'driver' there as well.

I'll have a poke around then.

Is it worth while implementing some kind of online reset without reloading the module?

Not sure what this means?

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.

Comment on lines 419 to 434
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;
}
Copy link
Member

@aleksamagicka aleksamagicka Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Author

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.

dripsnek added 2 commits April 2, 2025 20:30
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants