nzxt-kraken3: Add support for NZXT Kraken 2023 (normal and elite) models

[aleksamagicka]

Looking at https://github.com/liquidctl/liquidctl/pull/605, seems like the write commands being different is pretty much all that has to be covered (?)

[darkfrog26]

If this gets liquidctl to recognize my Kraken Elite I just installed I would be very happy!

[aleksamagicka]

No, this will get the kernel driver to recognize it.

Support for that Kraken in liquidctl has been merged in October. Please retry with the Git version for the time being.

---

@jonasmalacofilho, maybe it's time for a liquidctl release?

[alfagulf]

It worked for me!

I am using Kraken ELITE 360 RGB.

Now I can see the temperature in sensors:

    kraken2023elite-hid-3-5
    Adapter: HID adapter
    Coolant temp:  +28.4°C  

Thanks for all who made it possible.

[darkfrog26]

Having very strange issues after installing from Git:

[aleksamagicka]

Let's not go offtopic in this PR for that. Please follow the instructions in liquidctl README for setting up permissions, right after the Git install step.

[aleksamagicka]

Names of macros defining constants and labels in enums are capitalized. —https://www.kernel.org/doc/html/latest/process/coding-style.html#macros-enums-and-rtl

Huh, I'll make it uppercase. Guess Guenter doesn't seem to care much.

Additionally, maybe we should consider that enum kinds purpose is switching between our internally different code paths, in which a single Kraken 2023 variant would suffice.

I think the name mapping should stay in one place and with them the two enum entries. They are different PIDs, sure, but it would be weird to have to override the name later for only of one of them, IMO.

[jonasmalacofilho]

Sorry, on second thought I think there may actually be some functional issues here:

• in kraken3_is_visible(), the new enum variants aren't handled, disabling fan and pwm attributes for the new models;
• in kraken3_curve_props_are_visible(), shouldn't the condition be ... && priv->kind == X53?
• towards the bottom of kraken3_raw_event, the new kinds and, consequently, their additional fan/pwm values, are also not handled.

---

I think the name mapping should stay in one place and with them the two enum entries. They are different PIDs, sure, but it would be weird to have to override the name later for only of one of them, IMO.

I don't quite understand what you mean there. And I think I may have been unclear before. So, to avoid confusion, here's what I think you might want to consider:

enum kinds { X53, Z53, KRAKEN2023 } __packed;

Because, as far as this driver is concerned (=no LCD support), both "standard" and Elite models are identical.

That simplification would in turn require:

static const char *const kraken3_device_names[] = {
    [X53] = "x53",
    [Z53] = "z53",
    [KRAKEN2023] = "kraken2023",
};

(Or, alternatively, changing the way names are handled so that they map pid, rather than kind, into strings).

[aleksamagicka]

If you can hear it, that's the sound of me facepalming... Completely forgot, sorry, these need to be mapped to Z-series code.

---

As for naming, I know what you meant, and I'll implement it like that. Do we consider it important to note if it's an elite or not? That's what I was thinking about in the previous message, but seeing as I forgot to fixup the rest of the code, having just one thing in the enum makes much more sense.

[jonasmalacofilho]

Do we consider it important to note if it's an elite or not? That's what I was thinking about in the previous message, [...]

As far as debugfs is concerned, no. At that point requiring a check for the actual USB PID or device hierarchy is reasonable, if that information isn't already obvious from context.

For hwmon, I suppose the argument for it would be the possibility that the user has more than one Kraken 2023 and that they are of different screen sizes. I think that scenario is uncommon, and even then it's a nice-to-have, but non essential feature. And we could still tack it on later with one more branch in kraken3_probe, if it turns out someone really needs it.

[aleksamagicka]

Should be implemented properly now.

[jonasmalacofilho]

I think we can clean up the device name logic a bit with something like this:

diff --git a/nzxt-kraken3.c b/nzxt-kraken3.c
index aa7076758477..2e6fcd88d231 100644
--- a/nzxt-kraken3.c
+++ b/nzxt-kraken3.c
@@ -29,12 +29,6 @@
 enum kinds { X53, Z53, KRAKEN2023 } __packed;
 enum pwm_enable { off, manual, curve } __packed;

-static const char *const kraken3_device_names[] = {
-   [X53] = "x53",
-   [Z53] = "z53",
-   [KRAKEN2023] = "kraken2023"
-};
-
 #define DRIVER_NAME        "nzxt_kraken3"
 #define STATUS_REPORT_ID   0x75
 #define FIRMWARE_REPORT_ID 0x11
@@ -862,14 +856,14 @@ static int firmware_version_show(struct seq_file *seqf, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(firmware_version);

-static void kraken3_debugfs_init(struct kraken3_data *priv)
+static void kraken3_debugfs_init(struct kraken3_data *priv, const char *device_name)
 {
    char name[64];

    if (!priv->firmware_version[0])
        return;     /* Nothing to display in debugfs */

-   scnprintf(name, sizeof(name), "%s_%s-%s", DRIVER_NAME, kraken3_device_names[priv->kind],
+   scnprintf(name, sizeof(name), "%s_%s-%s", DRIVER_NAME, device_name,
          dev_name(&priv->hdev->dev));

    priv->debugfs = debugfs_create_dir(name, NULL);
@@ -919,23 +913,24 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
    case USB_PRODUCT_ID_X53:
    case USB_PRODUCT_ID_X53_SECOND:
        priv->kind = X53;
+       device_name = "x53";
        break;
    case USB_PRODUCT_ID_Z53:
        priv->kind = Z53;
+       device_name = "z53";
        break;
    case USB_PRODUCT_ID_KRAKEN2023:
+       priv->kind = KRAKEN2023;
+       device_name = "kraken2023";
+       break;
    case USB_PRODUCT_ID_KRAKEN2023_ELITE:
        priv->kind = KRAKEN2023;
+       device_name = "kraken2023elite";
        break;
    default:
        break;
    }

-   if (hdev->product == USB_PRODUCT_ID_KRAKEN2023_ELITE)
-       device_name = "kraken2023elite";
-   else
-       device_name = kraken3_device_names[priv->kind];
-
    priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
    if (!priv->buffer) {
        ret = -ENOMEM;
@@ -968,7 +963,7 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
        goto fail_and_close;
    }

-   kraken3_debugfs_init(priv);
+   kraken3_debugfs_init(priv, device_name);

    return 0;

(Draft; builds without warnings, but otherwise untested).

Note: this might require two patches to hwmon, the first doing this sort of transformation without yet support for the new models, and the second adding the extra bits for them.

[jonasmalacofilho]

Transformation as preparatory patch: https://github.com/jonasmalacofilho/liquidtux/commit/1d6d684266358906c390ce9b7d3a5a75b54e0551.

(I initially listed you as a co-developer, but that's probably best left for the actual submission to upstreaam, at which point you can fix/add such tags).

[aleksamagicka]

Thanks! Rebased my patch on top of yours.

[aleksamagicka]

These patches LGTM for v1?

[jonasmalacofilho]

Did you miss the previous/above comments about the "Description" section of the docs probably also needing to be updated?

[aleksamagicka]

I didn't, as I'll have to update Kconfig in the final patch as well. Looks like the force push from SmartGit failed, cmd seems to work just fine (second time this week on unrelated machines!).

[jonasmalacofilho]

We each missed one place where the new models need to be mentioned (I missed the first, you missed the third).

diff --git a/docs/nzxt-kraken3.rst b/docs/nzxt-kraken3.rst
index 8577d5d40ea5..a35b01c2942e 100644
--- a/docs/nzxt-kraken3.rst
+++ b/docs/nzxt-kraken3.rst
@@ -42,9 +42,9 @@ The devices can report if they are faulty. The driver supports that situation
 and will issue a warning. This can also happen when the USB cable is connected,
 but SATA power is not.

-The addressable RGB LEDs and LCD screen (only on Z-series models) are not
-supported in this driver, but can be controlled through existing userspace tools,
-such as `liquidctl`_.
+The addressable RGB LEDs and LCD screen (only on Z-series and Kraken 2023 models)
+are not supported in this driver, but can be controlled through existing
+userspace tools, such as `liquidctl`_.

 .. _liquidctl: https://github.com/liquidctl/liquidctl

[jonasmalacofilho]

Otherwise:

Reviewed-by: Jonas Malaco <jonas@protocubo.io>

[aleksamagicka]

Thanks, fixed.

[aleksamagicka]

@jonasmalacofilho, here are the patches for hwmon (the last two). Cherry picked them directly from this repo, so the only thing that's changed is the commit description.

[jonasmalacofilho]

@aleksamagicka LGTM!

[aleksamagicka]

This just made it in! 🚀