[PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72

[PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72

[Jonas Malaco]

These are "all-in-one" CPU liquid coolers that can be monitored and
controlled through a proprietary USB HID protocol.

While the models have differently sized radiators and come with varying
numbers of fans, they are all indistinguishable at the software level.

The driver exposes fan/pump speeds and coolant temperature through the
standard hwmon sysfs interface.

Fan and pump control, while supported by the devices, are not currently
exposed.  The firmware accepts up to 61 trip points per channel
(fan/pump), but the same set of trip temperatures has to be maintained
for both; with pwmX_auto_point_Y_temp attributes, users would need to
maintain this invariant themselves.

Instead, fan and pump control, as well as LED control (which the device
also supports for 9 addressable RGB LEDs on the CPU water block) are
left for existing and already mature user-space tools, which can still
be used alongside the driver, thanks to hidraw.  A link to one, which I
also maintain, is provided in the documentation.

The implementation is based on USB traffic analysis.  It has been
runtime tested on x86_64, both as a built-in driver and as a module.

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

I was not sure whether to exhaustively check type, attr and channel in
_is_visible/_read/_read_string.  Would it be preferred if those
functions assumed that they would never be called for unsupported
combinations, since that would be a programming error?

In practice, should kraken2_is_visible be simplified into

	static umode_t kraken2_is_visible(...)
	{
		return 0444;
	}

and should _read/_read_string go through similar (but not as effective)
simplifications?

On another note, the copyright dates back to 2019 because this driver
was left to mature (and then was mostly forgotten about) in an
out-of-tree repository.[1]

[1] https://github.com/liquidctl/liquidtux

 Documentation/hwmon/index.rst        |   1 +
 Documentation/hwmon/nzxt-kraken2.rst |  42 ++++
 MAINTAINERS                          |   7 +
 drivers/hwmon/Kconfig                |  10 +
 drivers/hwmon/Makefile               |   1 +
 drivers/hwmon/nzxt-kraken2.c         | 279 +++++++++++++++++++++++++++
 6 files changed, 340 insertions(+)
 create mode 100644 Documentation/hwmon/nzxt-kraken2.rst
 create mode 100644 drivers/hwmon/nzxt-kraken2.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index d4b422edbe3a..48bfa7887dd4 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -143,6 +143,7 @@ Hardware Monitoring Kernel Drivers
    npcm750-pwm-fan
    nsa320
    ntc_thermistor
+   nzxt-kraken2
    occ
    pc87360
    pc87427
diff --git a/Documentation/hwmon/nzxt-kraken2.rst b/Documentation/hwmon/nzxt-kraken2.rst
new file mode 100644
index 000000000000..94025de65a81
--- /dev/null
+++ b/Documentation/hwmon/nzxt-kraken2.rst
@@ -0,0 +1,42 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+Kernel driver nzxt-kraken2
+==========================
+
+Supported devices:
+
+* NZXT Kraken X42
+* NZXT Kraken X52
+* NZXT Kraken X62
+* NZXT Kraken X72
+
+Author: Jonas Malaco
+
+Description
+-----------
+
+This driver enables hardware monitoring support for NZXT Kraken X42/X52/X62/X72
+all-in-one CPU liquid coolers.  Three sensors are available: fan speed, pump
+speed and coolant temperature.
+
+Fan and pump control, while supported by the firmware, are not currently
+exposed.  The addressable RGB LEDs, present in the integrated CPU water block
+and pump head, are not supported either.  But both features can be found in
+existing user-space tools (e.g. `liquidctl`_).
+
+.. _liquidctl: https://github.com/liquidctl/liquidctl
+
+Usage Notes
+-----------
+
+As these are USB HIDs, the driver can be loaded automatically by the kernel and
+supports hot swapping.
+
+Sysfs entries
+-------------
+
+=======================	========================================================
+fan1_input		Fan speed (in rpm)
+fan2_input		Pump speed (in rpm)
+temp1_input		Coolant temperature (in millidegrees Celsius)
+=======================	========================================================
diff --git a/MAINTAINERS b/MAINTAINERS
index 0635b30e467c..b8f9fc5eaf08 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12911,6 +12911,13 @@ L:	linux-nfc@lists.01.org (moderated for non-subscribers)
 S:	Supported
 F:	drivers/nfc/nxp-nci
 
+NZXT-KRAKEN2 HARDWARE MONITORING DRIVER
+M:	Jonas Malaco <jonas@protocubo.io>
+L:	linux-hwmon@vger.kernel.org
+S:	Maintained
+F:	Documentation/hwmon/nzxt-kraken2.rst
+F:	drivers/hwmon/nzxt-kraken2.c
+
 OBJAGG
 M:	Jiri Pirko <jiri@nvidia.com>
 L:	netdev@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 54f04e61fb83..0ddc974b102e 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1492,6 +1492,16 @@ config SENSORS_NSA320
 	  This driver can also be built as a module. If so, the module
 	  will be called nsa320-hwmon.
 
+config SENSORS_NZXT_KRAKEN2
+	tristate "NZXT Kraken X42/X51/X62/X72 liquid coolers"
+	depends on USB_HID
+	help
+	  If you say yes here you get support for hardware monitoring for the
+	  NZXT Kraken X42/X52/X62/X72 all-in-one CPU liquid coolers.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nzxt-kraken2.
+
 source "drivers/hwmon/occ/Kconfig"
 
 config SENSORS_PCF8591
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index fe38e8a5c979..59e78bc212cf 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -155,6 +155,7 @@ obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
 obj-$(CONFIG_SENSORS_NPCM7XX)	+= npcm750-pwm-fan.o
 obj-$(CONFIG_SENSORS_NSA320)	+= nsa320-hwmon.o
 obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
+obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
 obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
 obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
 obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
diff --git a/drivers/hwmon/nzxt-kraken2.c b/drivers/hwmon/nzxt-kraken2.c
new file mode 100644
index 000000000000..1426310d6965
--- /dev/null
+++ b/drivers/hwmon/nzxt-kraken2.c
@@ -0,0 +1,279 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * nzxt-kraken2.c - hwmon driver for NZXT Kraken X42/X52/X62/X72 coolers
+ *
+ * Copyright 2019  Jonas Malaco <jonas@protocubo.io>
+ */
+
+#include <asm/unaligned.h>
+#include <linux/hid.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+
+#define STATUS_REPORT_ID	0x04
+#define STATUS_USEFUL_SIZE	8
+
+static const char *const kraken2_temp_label[] = {
+	"Coolant",
+};
+
+static const char *const kraken2_fan_label[] = {
+	"Fan",
+	"Pump",
+};
+
+struct kraken2_priv_data {
+	struct hid_device *hid_dev;
+	struct device *hwmon_dev;
+
+	spinlock_t lock; /* protects the last received status */
+	u8 status[STATUS_USEFUL_SIZE];
+};
+
+static umode_t kraken2_is_visible(const void *data,
+				  enum hwmon_sensor_types type,
+				  u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+		case hwmon_temp_label:
+			if (channel == 0)
+				return 0444;
+			return 0;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+		case hwmon_fan_label:
+			if (channel >= 0 && channel < 2)
+				return 0444;
+			return 0;
+		}
+		break;
+	default:
+		break;
+	}
+	return 0;
+}
+
+static int kraken2_read(struct device *dev, enum hwmon_sensor_types type,
+			u32 attr, int channel, long *val)
+{
+	struct kraken2_priv_data *priv = dev_get_drvdata(dev);
+	unsigned long flags;
+
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_input:
+			if (channel != 0)
+				return -EOPNOTSUPP;
+			/*
+			 * The fractional byte has been observed to be in the
+			 * interval [1,9], but some of these steps are also
+			 * consistently skipped for certain integer parts.
+			 *
+			 * For the lack of a better idea, assume that the
+			 * resolution is 0.1°C, and that the missing steps are
+			 * artifacts of how the firmware processes the raw
+			 * sensor data.
+			 */
+			spin_lock_irqsave(&priv->lock, flags);
+			*val = priv->status[1] * 1000 + priv->status[2] * 100;
+			spin_unlock_irqrestore(&priv->lock, flags);
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_input:
+			if (channel < 0 || channel >= 2)
+				return -EOPNOTSUPP;
+			spin_lock_irqsave(&priv->lock, flags);
+			*val = get_unaligned_be16(priv->status + 3 + channel * 2);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
+static int kraken2_read_string(struct device *dev, enum hwmon_sensor_types type,
+			       u32 attr, int channel, const char **str)
+{
+	switch (type) {
+	case hwmon_temp:
+		switch (attr) {
+		case hwmon_temp_label:
+			if (channel != 0)
+				return -EOPNOTSUPP;
+			*str = kraken2_temp_label[channel];
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	case hwmon_fan:
+		switch (attr) {
+		case hwmon_fan_label:
+			if (channel < 0 || channel >= 2)
+				return -EOPNOTSUPP;
+			*str = kraken2_fan_label[channel];
+			break;
+		default:
+			return -EOPNOTSUPP;
+		}
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+	return 0;
+}
+
+static const struct hwmon_ops kraken2_hwmon_ops = {
+	.is_visible = kraken2_is_visible,
+	.read = kraken2_read,
+	.read_string = kraken2_read_string,
+};
+
+static const struct hwmon_channel_info *kraken2_info[] = {
+	HWMON_CHANNEL_INFO(temp,
+			   HWMON_T_INPUT | HWMON_T_LABEL),
+	HWMON_CHANNEL_INFO(fan,
+			   HWMON_F_INPUT | HWMON_F_LABEL,
+			   HWMON_F_INPUT | HWMON_F_LABEL),
+	NULL
+};
+
+static const struct hwmon_chip_info kraken2_chip_info = {
+	.ops = &kraken2_hwmon_ops,
+	.info = kraken2_info,
+};
+
+static int kraken2_raw_event(struct hid_device *hdev,
+			     struct hid_report *report, u8 *data, int size)
+{
+	struct kraken2_priv_data *priv;
+	unsigned long flags;
+
+	if (size < STATUS_USEFUL_SIZE || report->id != STATUS_REPORT_ID)
+		return 0;
+
+	priv = hid_get_drvdata(hdev);
+
+	spin_lock_irqsave(&priv->lock, flags);
+	memcpy(priv->status, data, STATUS_USEFUL_SIZE);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return 0;
+}
+
+static int kraken2_probe(struct hid_device *hdev,
+			 const struct hid_device_id *id)
+{
+	struct kraken2_priv_data *priv;
+	int ret;
+
+	priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->hid_dev = hdev;
+	spin_lock_init(&priv->lock);
+	hid_set_drvdata(hdev, priv);
+
+	ret = hid_parse(hdev);
+	if (ret) {
+		hid_err(hdev, "hid parse failed with %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * Enable hidraw so existing user-space tools can continue to work.
+	 */
+	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (ret) {
+		hid_err(hdev, "hid hw start failed with %d\n", ret);
+		goto fail_and_stop;
+	}
+
+	ret = hid_hw_open(hdev);
+	if (ret) {
+		hid_err(hdev, "hid hw open failed with %d\n", ret);
+		goto fail_and_close;
+	}
+
+	priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "kraken2",
+							  priv, &kraken2_chip_info,
+							  NULL);
+	if (IS_ERR(priv->hwmon_dev)) {
+		ret = PTR_ERR(priv->hwmon_dev);
+		hid_err(hdev, "hwmon registration failed with %d\n", ret);
+		goto fail_and_close;
+	}
+
+	return 0;
+
+fail_and_close:
+	hid_hw_close(hdev);
+fail_and_stop:
+	hid_hw_stop(hdev);
+	return ret;
+}
+
+static void kraken2_remove(struct hid_device *hdev)
+{
+	struct kraken2_priv_data *priv = hid_get_drvdata(hdev);
+
+	hwmon_device_unregister(priv->hwmon_dev);
+
+	hid_hw_close(hdev);
+	hid_hw_stop(hdev);
+}
+
+static const struct hid_device_id kraken2_table[] = {
+	{ HID_USB_DEVICE(0x1e71, 0x170e) }, /* NZXT Kraken X42/X52/X62/X72 */
+	{ }
+};
+
+MODULE_DEVICE_TABLE(hid, kraken2_table);
+
+static struct hid_driver kraken2_driver = {
+	.name = "nzxt-kraken2",
+	.id_table = kraken2_table,
+	.probe = kraken2_probe,
+	.remove = kraken2_remove,
+	.raw_event = kraken2_raw_event,
+};
+
+static int __init kraken2_init(void)
+{
+	return hid_register_driver(&kraken2_driver);
+}
+
+static void __exit kraken2_exit(void)
+{
+	hid_unregister_driver(&kraken2_driver);
+}
+
+/*
+ * When compiled into the kernel, initialize after the hid bus.
+ */
+late_initcall(kraken2_init);
+module_exit(kraken2_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Jonas Malaco <jonas@protocubo.io>");
+MODULE_DESCRIPTION("Hwmon driver for NZXT Kraken X42/X52/X62/X72 coolers");

base-commit: d8a08585259268e7b5305d04422a59d713719ccb
-- 
2.31.0

Re: [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72

[Guenter Roeck]

On 3/18/21 9:48 AM, Jonas Malaco wrote:
> These are "all-in-one" CPU liquid coolers that can be monitored and
> controlled through a proprietary USB HID protocol.
> 
> While the models have differently sized radiators and come with varying
> numbers of fans, they are all indistinguishable at the software level.
> > The driver exposes fan/pump speeds and coolant temperature through the
> standard hwmon sysfs interface.
> 
> Fan and pump control, while supported by the devices, are not currently
> exposed.  The firmware accepts up to 61 trip points per channel
> (fan/pump), but the same set of trip temperatures has to be maintained
> for both; with pwmX_auto_point_Y_temp attributes, users would need to
> maintain this invariant themselves.
> 
> Instead, fan and pump control, as well as LED control (which the device
> also supports for 9 addressable RGB LEDs on the CPU water block) are
> left for existing and already mature user-space tools, which can still
> be used alongside the driver, thanks to hidraw.  A link to one, which I
> also maintain, is provided in the documentation.
> 
> The implementation is based on USB traffic analysis.  It has been
> runtime tested on x86_64, both as a built-in driver and as a module.
> 
> Signed-off-by: Jonas Malaco <jonas@protocubo.io>
> ---
> 
> I was not sure whether to exhaustively check type, attr and channel in
> _is_visible/_read/_read_string.  Would it be preferred if those
> functions assumed that they would never be called for unsupported
> combinations, since that would be a programming error?
> 
> In practice, should kraken2_is_visible be simplified into
> 
> 	static umode_t kraken2_is_visible(...)
> 	{
> 		return 0444;
> 	}
> 
Yes, if nothing is optional, and all permissions are 0444.

> and should _read/_read_string go through similar (but not as effective)
> simplifications?
> 
Unless I am missing something, all the channel checks are unnecessary
and should be removed.

> On another note, the copyright dates back to 2019 because this driver
> was left to mature (and then was mostly forgotten about) in an
> out-of-tree repository.[1]
> 
> [1] https://github.com/liquidctl/liquidtux
> 
>  Documentation/hwmon/index.rst        |   1 +
>  Documentation/hwmon/nzxt-kraken2.rst |  42 ++++
>  MAINTAINERS                          |   7 +
>  drivers/hwmon/Kconfig                |  10 +
>  drivers/hwmon/Makefile               |   1 +
>  drivers/hwmon/nzxt-kraken2.c         | 279 +++++++++++++++++++++++++++
>  6 files changed, 340 insertions(+)
>  create mode 100644 Documentation/hwmon/nzxt-kraken2.rst
>  create mode 100644 drivers/hwmon/nzxt-kraken2.c
> 
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index d4b422edbe3a..48bfa7887dd4 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -143,6 +143,7 @@ Hardware Monitoring Kernel Drivers
>     npcm750-pwm-fan
>     nsa320
>     ntc_thermistor
> +   nzxt-kraken2
>     occ
>     pc87360
>     pc87427
> diff --git a/Documentation/hwmon/nzxt-kraken2.rst b/Documentation/hwmon/nzxt-kraken2.rst
> new file mode 100644
> index 000000000000..94025de65a81
> --- /dev/null
> +++ b/Documentation/hwmon/nzxt-kraken2.rst
> @@ -0,0 +1,42 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver nzxt-kraken2
> +==========================
> +
> +Supported devices:
> +
> +* NZXT Kraken X42
> +* NZXT Kraken X52
> +* NZXT Kraken X62
> +* NZXT Kraken X72
> +
> +Author: Jonas Malaco
> +
> +Description
> +-----------
> +
> +This driver enables hardware monitoring support for NZXT Kraken X42/X52/X62/X72
> +all-in-one CPU liquid coolers.  Three sensors are available: fan speed, pump
> +speed and coolant temperature.
> +
> +Fan and pump control, while supported by the firmware, are not currently
> +exposed.  The addressable RGB LEDs, present in the integrated CPU water block
> +and pump head, are not supported either.  But both features can be found in
> +existing user-space tools (e.g. `liquidctl`_).
> +
> +.. _liquidctl: https://github.com/liquidctl/liquidctl
> +
> +Usage Notes
> +-----------
> +
> +As these are USB HIDs, the driver can be loaded automatically by the kernel and
> +supports hot swapping.
> +
> +Sysfs entries
> +-------------
> +
> +=======================	========================================================
> +fan1_input		Fan speed (in rpm)
> +fan2_input		Pump speed (in rpm)
> +temp1_input		Coolant temperature (in millidegrees Celsius)
> +=======================	========================================================
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0635b30e467c..b8f9fc5eaf08 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12911,6 +12911,13 @@ L:	linux-nfc@lists.01.org (moderated for non-subscribers)
>  S:	Supported
>  F:	drivers/nfc/nxp-nci
>  
> +NZXT-KRAKEN2 HARDWARE MONITORING DRIVER
> +M:	Jonas Malaco <jonas@protocubo.io>
> +L:	linux-hwmon@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/hwmon/nzxt-kraken2.rst
> +F:	drivers/hwmon/nzxt-kraken2.c
> +
>  OBJAGG
>  M:	Jiri Pirko <jiri@nvidia.com>
>  L:	netdev@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 54f04e61fb83..0ddc974b102e 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1492,6 +1492,16 @@ config SENSORS_NSA320
>  	  This driver can also be built as a module. If so, the module
>  	  will be called nsa320-hwmon.
>  
> +config SENSORS_NZXT_KRAKEN2
> +	tristate "NZXT Kraken X42/X51/X62/X72 liquid coolers"
> +	depends on USB_HID
> +	help
> +	  If you say yes here you get support for hardware monitoring for the
> +	  NZXT Kraken X42/X52/X62/X72 all-in-one CPU liquid coolers.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called nzxt-kraken2.
> +
>  source "drivers/hwmon/occ/Kconfig"
>  
>  config SENSORS_PCF8591
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index fe38e8a5c979..59e78bc212cf 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -155,6 +155,7 @@ obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
>  obj-$(CONFIG_SENSORS_NPCM7XX)	+= npcm750-pwm-fan.o
>  obj-$(CONFIG_SENSORS_NSA320)	+= nsa320-hwmon.o
>  obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
> +obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
>  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
>  obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> diff --git a/drivers/hwmon/nzxt-kraken2.c b/drivers/hwmon/nzxt-kraken2.c
> new file mode 100644
> index 000000000000..1426310d6965
> --- /dev/null
> +++ b/drivers/hwmon/nzxt-kraken2.c
> @@ -0,0 +1,279 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * nzxt-kraken2.c - hwmon driver for NZXT Kraken X42/X52/X62/X72 coolers
> + *
> + * Copyright 2019  Jonas Malaco <jonas@protocubo.io>

Should probably be 2019 - 2021

> + */
> +
> +#include <asm/unaligned.h>
> +#include <linux/hid.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +
> +#define STATUS_REPORT_ID	0x04
> +#define STATUS_USEFUL_SIZE	8
> +
> +static const char *const kraken2_temp_label[] = {
> +	"Coolant",
> +};
> +
> +static const char *const kraken2_fan_label[] = {
> +	"Fan",
> +	"Pump",
> +};
> +
> +struct kraken2_priv_data {
> +	struct hid_device *hid_dev;
> +	struct device *hwmon_dev;
> +
> +	spinlock_t lock; /* protects the last received status */
> +	u8 status[STATUS_USEFUL_SIZE];
> +};
> +
> +static umode_t kraken2_is_visible(const void *data,
> +				  enum hwmon_sensor_types type,
> +				  u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +		case hwmon_temp_label:
> +			if (channel == 0)
> +				return 0444;
> +			return 0;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +		case hwmon_fan_label:
> +			if (channel >= 0 && channel < 2)
> +				return 0444;
> +			return 0;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +	return 0;
> +}
> +
> +static int kraken2_read(struct device *dev, enum hwmon_sensor_types type,
> +			u32 attr, int channel, long *val)
> +{
> +	struct kraken2_priv_data *priv = dev_get_drvdata(dev);
> +	unsigned long flags;
> +
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_input:
> +			if (channel != 0)
> +				return -EOPNOTSUPP;
> +			/*
> +			 * The fractional byte has been observed to be in the
> +			 * interval [1,9], but some of these steps are also
> +			 * consistently skipped for certain integer parts.
> +			 *
> +			 * For the lack of a better idea, assume that the
> +			 * resolution is 0.1°C, and that the missing steps are
> +			 * artifacts of how the firmware processes the raw
> +			 * sensor data.
> +			 */
> +			spin_lock_irqsave(&priv->lock, flags);

Why would this need to disable interrupts ?

> +			*val = priv->status[1] * 1000 + priv->status[2] * 100;
> +			spin_unlock_irqrestore(&priv->lock, flags);
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_input:
> +			if (channel < 0 || channel >= 2)
> +				return -EOPNOTSUPP;
> +			spin_lock_irqsave(&priv->lock, flags);

Why would this need to disable interrupts ?

> +			*val = get_unaligned_be16(priv->status + 3 + channel * 2);
> +			spin_unlock_irqrestore(&priv->lock, flags);
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
> +static int kraken2_read_string(struct device *dev, enum hwmon_sensor_types type,
> +			       u32 attr, int channel, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		switch (attr) {
> +		case hwmon_temp_label:
> +			if (channel != 0)
> +				return -EOPNOTSUPP;
> +			*str = kraken2_temp_label[channel];
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	case hwmon_fan:
> +		switch (attr) {
> +		case hwmon_fan_label:
> +			if (channel < 0 || channel >= 2)
> +				return -EOPNOTSUPP;
> +			*str = kraken2_fan_label[channel];
> +			break;
> +		default:
> +			return -EOPNOTSUPP;
> +		}
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +	return 0;
> +}
> +
> +static const struct hwmon_ops kraken2_hwmon_ops = {
> +	.is_visible = kraken2_is_visible,
> +	.read = kraken2_read,
> +	.read_string = kraken2_read_string,
> +};
> +
> +static const struct hwmon_channel_info *kraken2_info[] = {
> +	HWMON_CHANNEL_INFO(temp,
> +			   HWMON_T_INPUT | HWMON_T_LABEL),
> +	HWMON_CHANNEL_INFO(fan,
> +			   HWMON_F_INPUT | HWMON_F_LABEL,
> +			   HWMON_F_INPUT | HWMON_F_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_chip_info kraken2_chip_info = {
> +	.ops = &kraken2_hwmon_ops,
> +	.info = kraken2_info,
> +};
> +
> +static int kraken2_raw_event(struct hid_device *hdev,
> +			     struct hid_report *report, u8 *data, int size)
> +{
> +	struct kraken2_priv_data *priv;
> +	unsigned long flags;
> +
> +	if (size < STATUS_USEFUL_SIZE || report->id != STATUS_REPORT_ID)
> +		return 0;
> +
> +	priv = hid_get_drvdata(hdev);
> +
> +	spin_lock_irqsave(&priv->lock, flags);

I don't see the point of disabling interrupts here and above.

Either case, the spinlocks are overkill. It would be much easier to
convert raw readings here into temperature and fan speed and store
the resulting values in struct kraken2_priv_data, and then to
just report it in the read functions. That would be much less costly
because the spinlock would not be needed at all, and calculations
would be done only once per event.

> +	memcpy(priv->status, data, STATUS_USEFUL_SIZE);
> +	spin_unlock_irqrestore(&priv->lock, flags);
> +
> +	return 0;
> +}

For my education: What triggers those events ? Are they reported
by the hardware autonomously whenever something changes ?
A comment at the top of the driver explaining how this works
might be useful.

Also, is there a way to initialize values during probe ? Otherwise
the driver would report values of 0 until the hardware reports
something.

> +
> +static int kraken2_probe(struct hid_device *hdev,
> +			 const struct hid_device_id *id)
> +{
> +	struct kraken2_priv_data *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->hid_dev = hdev;
> +	spin_lock_init(&priv->lock);
> +	hid_set_drvdata(hdev, priv);
> +
> +	ret = hid_parse(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hid parse failed with %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Enable hidraw so existing user-space tools can continue to work.
> +	 */
> +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> +	if (ret) {
> +		hid_err(hdev, "hid hw start failed with %d\n", ret);
> +		goto fail_and_stop;
> +	}
> +
> +	ret = hid_hw_open(hdev);
> +	if (ret) {
> +		hid_err(hdev, "hid hw open failed with %d\n", ret);
> +		goto fail_and_close;
> +	}
> +
> +	priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "kraken2",
> +							  priv, &kraken2_chip_info,
> +							  NULL);
> +	if (IS_ERR(priv->hwmon_dev)) {
> +		ret = PTR_ERR(priv->hwmon_dev);
> +		hid_err(hdev, "hwmon registration failed with %d\n", ret);
> +		goto fail_and_close;
> +	}
> +
> +	return 0;
> +
> +fail_and_close:
> +	hid_hw_close(hdev);
> +fail_and_stop:
> +	hid_hw_stop(hdev);
> +	return ret;
> +}
> +
> +static void kraken2_remove(struct hid_device *hdev)
> +{
> +	struct kraken2_priv_data *priv = hid_get_drvdata(hdev);
> +
> +	hwmon_device_unregister(priv->hwmon_dev);
> +
> +	hid_hw_close(hdev);
> +	hid_hw_stop(hdev);
> +}
> +
> +static const struct hid_device_id kraken2_table[] = {
> +	{ HID_USB_DEVICE(0x1e71, 0x170e) }, /* NZXT Kraken X42/X52/X62/X72 */
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(hid, kraken2_table);
> +
> +static struct hid_driver kraken2_driver = {
> +	.name = "nzxt-kraken2",
> +	.id_table = kraken2_table,
> +	.probe = kraken2_probe,
> +	.remove = kraken2_remove,
> +	.raw_event = kraken2_raw_event,
> +};
> +
> +static int __init kraken2_init(void)
> +{
> +	return hid_register_driver(&kraken2_driver);
> +}
> +
> +static void __exit kraken2_exit(void)
> +{
> +	hid_unregister_driver(&kraken2_driver);
> +}
> +
> +/*
> + * When compiled into the kernel, initialize after the hid bus.
> + */
> +late_initcall(kraken2_init);
> +module_exit(kraken2_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Jonas Malaco <jonas@protocubo.io>");
> +MODULE_DESCRIPTION("Hwmon driver for NZXT Kraken X42/X52/X62/X72 coolers");
> 
> base-commit: d8a08585259268e7b5305d04422a59d713719ccb
> 

Re: [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72

[Jonas Malaco]

On Thu, Mar 18, 2021 at 11:55:39AM -0700, Guenter Roeck wrote:
> On 3/18/21 9:48 AM, Jonas Malaco wrote:
> > These are "all-in-one" CPU liquid coolers that can be monitored and
> > controlled through a proprietary USB HID protocol.
> > 
> > While the models have differently sized radiators and come with varying
> > numbers of fans, they are all indistinguishable at the software level.
> > > The driver exposes fan/pump speeds and coolant temperature through the
> > standard hwmon sysfs interface.
> > 
> > Fan and pump control, while supported by the devices, are not currently
> > exposed.  The firmware accepts up to 61 trip points per channel
> > (fan/pump), but the same set of trip temperatures has to be maintained
> > for both; with pwmX_auto_point_Y_temp attributes, users would need to
> > maintain this invariant themselves.
> > 
> > Instead, fan and pump control, as well as LED control (which the device
> > also supports for 9 addressable RGB LEDs on the CPU water block) are
> > left for existing and already mature user-space tools, which can still
> > be used alongside the driver, thanks to hidraw.  A link to one, which I
> > also maintain, is provided in the documentation.
> > 
> > The implementation is based on USB traffic analysis.  It has been
> > runtime tested on x86_64, both as a built-in driver and as a module.
> > 
> > Signed-off-by: Jonas Malaco <jonas@protocubo.io>
> > ---
> > 
> > I was not sure whether to exhaustively check type, attr and channel in
> > _is_visible/_read/_read_string.  Would it be preferred if those
> > functions assumed that they would never be called for unsupported
> > combinations, since that would be a programming error?
> > 
> > In practice, should kraken2_is_visible be simplified into
> > 
> > 	static umode_t kraken2_is_visible(...)
> > 	{
> > 		return 0444;
> > 	}
> > 
> Yes, if nothing is optional, and all permissions are 0444.
> 
> > and should _read/_read_string go through similar (but not as effective)
> > simplifications?
> > 
> Unless I am missing something, all the channel checks are unnecessary
> and should be removed.

I'll remove them and the checks in kraken2_read for v2.

> 
> > On another note, the copyright dates back to 2019 because this driver
> > was left to mature (and then was mostly forgotten about) in an
> > out-of-tree repository.[1]
> > 
> > [1] https://github.com/liquidctl/liquidtux
> > 
> >  Documentation/hwmon/index.rst        |   1 +
> >  Documentation/hwmon/nzxt-kraken2.rst |  42 ++++
> >  MAINTAINERS                          |   7 +
> >  drivers/hwmon/Kconfig                |  10 +
> >  drivers/hwmon/Makefile               |   1 +
> >  drivers/hwmon/nzxt-kraken2.c         | 279 +++++++++++++++++++++++++++
> >  6 files changed, 340 insertions(+)
> >  create mode 100644 Documentation/hwmon/nzxt-kraken2.rst
> >  create mode 100644 drivers/hwmon/nzxt-kraken2.c
> > 
> > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> > index d4b422edbe3a..48bfa7887dd4 100644
> > --- a/Documentation/hwmon/index.rst
> > +++ b/Documentation/hwmon/index.rst
> > @@ -143,6 +143,7 @@ Hardware Monitoring Kernel Drivers
> >     npcm750-pwm-fan
> >     nsa320
> >     ntc_thermistor
> > +   nzxt-kraken2
> >     occ
> >     pc87360
> >     pc87427
> > diff --git a/Documentation/hwmon/nzxt-kraken2.rst b/Documentation/hwmon/nzxt-kraken2.rst
> > new file mode 100644
> > index 000000000000..94025de65a81
> > --- /dev/null
> > +++ b/Documentation/hwmon/nzxt-kraken2.rst
> > @@ -0,0 +1,42 @@
> > +.. SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +Kernel driver nzxt-kraken2
> > +==========================
> > +
> > +Supported devices:
> > +
> > +* NZXT Kraken X42
> > +* NZXT Kraken X52
> > +* NZXT Kraken X62
> > +* NZXT Kraken X72
> > +
> > +Author: Jonas Malaco
> > +
> > +Description
> > +-----------
> > +
> > +This driver enables hardware monitoring support for NZXT Kraken X42/X52/X62/X72
> > +all-in-one CPU liquid coolers.  Three sensors are available: fan speed, pump
> > +speed and coolant temperature.
> > +
> > +Fan and pump control, while supported by the firmware, are not currently
> > +exposed.  The addressable RGB LEDs, present in the integrated CPU water block
> > +and pump head, are not supported either.  But both features can be found in
> > +existing user-space tools (e.g. `liquidctl`_).
> > +
> > +.. _liquidctl: https://github.com/liquidctl/liquidctl
> > +
> > +Usage Notes
> > +-----------
> > +
> > +As these are USB HIDs, the driver can be loaded automatically by the kernel and
> > +supports hot swapping.
> > +
> > +Sysfs entries
> > +-------------
> > +
> > +=======================	========================================================
> > +fan1_input		Fan speed (in rpm)
> > +fan2_input		Pump speed (in rpm)
> > +temp1_input		Coolant temperature (in millidegrees Celsius)
> > +=======================	========================================================
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0635b30e467c..b8f9fc5eaf08 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12911,6 +12911,13 @@ L:	linux-nfc@lists.01.org (moderated for non-subscribers)
> >  S:	Supported
> >  F:	drivers/nfc/nxp-nci
> >  
> > +NZXT-KRAKEN2 HARDWARE MONITORING DRIVER
> > +M:	Jonas Malaco <jonas@protocubo.io>
> > +L:	linux-hwmon@vger.kernel.org
> > +S:	Maintained
> > +F:	Documentation/hwmon/nzxt-kraken2.rst
> > +F:	drivers/hwmon/nzxt-kraken2.c
> > +
> >  OBJAGG
> >  M:	Jiri Pirko <jiri@nvidia.com>
> >  L:	netdev@vger.kernel.org
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 54f04e61fb83..0ddc974b102e 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -1492,6 +1492,16 @@ config SENSORS_NSA320
> >  	  This driver can also be built as a module. If so, the module
> >  	  will be called nsa320-hwmon.
> >  
> > +config SENSORS_NZXT_KRAKEN2
> > +	tristate "NZXT Kraken X42/X51/X62/X72 liquid coolers"
> > +	depends on USB_HID
> > +	help
> > +	  If you say yes here you get support for hardware monitoring for the
> > +	  NZXT Kraken X42/X52/X62/X72 all-in-one CPU liquid coolers.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called nzxt-kraken2.
> > +
> >  source "drivers/hwmon/occ/Kconfig"
> >  
> >  config SENSORS_PCF8591
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index fe38e8a5c979..59e78bc212cf 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -155,6 +155,7 @@ obj-$(CONFIG_SENSORS_NCT7904)	+= nct7904.o
> >  obj-$(CONFIG_SENSORS_NPCM7XX)	+= npcm750-pwm-fan.o
> >  obj-$(CONFIG_SENSORS_NSA320)	+= nsa320-hwmon.o
> >  obj-$(CONFIG_SENSORS_NTC_THERMISTOR)	+= ntc_thermistor.o
> > +obj-$(CONFIG_SENSORS_NZXT_KRAKEN2) += nzxt-kraken2.o
> >  obj-$(CONFIG_SENSORS_PC87360)	+= pc87360.o
> >  obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
> >  obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
> > diff --git a/drivers/hwmon/nzxt-kraken2.c b/drivers/hwmon/nzxt-kraken2.c
> > new file mode 100644
> > index 000000000000..1426310d6965
> > --- /dev/null
> > +++ b/drivers/hwmon/nzxt-kraken2.c
> > @@ -0,0 +1,279 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * nzxt-kraken2.c - hwmon driver for NZXT Kraken X42/X52/X62/X72 coolers
> > + *
> > + * Copyright 2019  Jonas Malaco <jonas@protocubo.io>
> 
> Should probably be 2019 - 2021
> 
> > + */
> > +
> > +#include <asm/unaligned.h>
> > +#include <linux/hid.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/module.h>
> > +#include <linux/spinlock.h>
> > +
> > +#define STATUS_REPORT_ID	0x04
> > +#define STATUS_USEFUL_SIZE	8
> > +
> > +static const char *const kraken2_temp_label[] = {
> > +	"Coolant",
> > +};
> > +
> > +static const char *const kraken2_fan_label[] = {
> > +	"Fan",
> > +	"Pump",
> > +};
> > +
> > +struct kraken2_priv_data {
> > +	struct hid_device *hid_dev;
> > +	struct device *hwmon_dev;
> > +
> > +	spinlock_t lock; /* protects the last received status */
> > +	u8 status[STATUS_USEFUL_SIZE];
> > +};
> > +
> > +static umode_t kraken2_is_visible(const void *data,
> > +				  enum hwmon_sensor_types type,
> > +				  u32 attr, int channel)
> > +{
> > +	switch (type) {
> > +	case hwmon_temp:
> > +		switch (attr) {
> > +		case hwmon_temp_input:
> > +		case hwmon_temp_label:
> > +			if (channel == 0)
> > +				return 0444;
> > +			return 0;
> > +		}
> > +		break;
> > +	case hwmon_fan:
> > +		switch (attr) {
> > +		case hwmon_fan_input:
> > +		case hwmon_fan_label:
> > +			if (channel >= 0 && channel < 2)
> > +				return 0444;
> > +			return 0;
> > +		}
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int kraken2_read(struct device *dev, enum hwmon_sensor_types type,
> > +			u32 attr, int channel, long *val)
> > +{
> > +	struct kraken2_priv_data *priv = dev_get_drvdata(dev);
> > +	unsigned long flags;
> > +
> > +	switch (type) {
> > +	case hwmon_temp:
> > +		switch (attr) {
> > +		case hwmon_temp_input:
> > +			if (channel != 0)
> > +				return -EOPNOTSUPP;
> > +			/*
> > +			 * The fractional byte has been observed to be in the
> > +			 * interval [1,9], but some of these steps are also
> > +			 * consistently skipped for certain integer parts.
> > +			 *
> > +			 * For the lack of a better idea, assume that the
> > +			 * resolution is 0.1°C, and that the missing steps are
> > +			 * artifacts of how the firmware processes the raw
> > +			 * sensor data.
> > +			 */
> > +			spin_lock_irqsave(&priv->lock, flags);
> 
> Why would this need to disable interrupts ?

Here and for the fan speeds they are disabled because kraken2_raw_event
is an interrupt handler (specifically, it runs in a hard IRQ context).

> 
> > +			*val = priv->status[1] * 1000 + priv->status[2] * 100;
> > +			spin_unlock_irqrestore(&priv->lock, flags);
> > +			break;
> > +		default:
> > +			return -EOPNOTSUPP;
> > +		}
> > +		break;
> > +	case hwmon_fan:
> > +		switch (attr) {
> > +		case hwmon_fan_input:
> > +			if (channel < 0 || channel >= 2)
> > +				return -EOPNOTSUPP;
> > +			spin_lock_irqsave(&priv->lock, flags);
> 
> Why would this need to disable interrupts ?
> 
> > +			*val = get_unaligned_be16(priv->status + 3 + channel * 2);
> > +			spin_unlock_irqrestore(&priv->lock, flags);
> > +			break;
> > +		default:
> > +			return -EOPNOTSUPP;
> > +		}
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int kraken2_read_string(struct device *dev, enum hwmon_sensor_types type,
> > +			       u32 attr, int channel, const char **str)
> > +{
> > +	switch (type) {
> > +	case hwmon_temp:
> > +		switch (attr) {
> > +		case hwmon_temp_label:
> > +			if (channel != 0)
> > +				return -EOPNOTSUPP;
> > +			*str = kraken2_temp_label[channel];
> > +			break;
> > +		default:
> > +			return -EOPNOTSUPP;
> > +		}
> > +		break;
> > +	case hwmon_fan:
> > +		switch (attr) {
> > +		case hwmon_fan_label:
> > +			if (channel < 0 || channel >= 2)
> > +				return -EOPNOTSUPP;
> > +			*str = kraken2_fan_label[channel];
> > +			break;
> > +		default:
> > +			return -EOPNOTSUPP;
> > +		}
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +	return 0;
> > +}
> > +
> > +static const struct hwmon_ops kraken2_hwmon_ops = {
> > +	.is_visible = kraken2_is_visible,
> > +	.read = kraken2_read,
> > +	.read_string = kraken2_read_string,
> > +};
> > +
> > +static const struct hwmon_channel_info *kraken2_info[] = {
> > +	HWMON_CHANNEL_INFO(temp,
> > +			   HWMON_T_INPUT | HWMON_T_LABEL),
> > +	HWMON_CHANNEL_INFO(fan,
> > +			   HWMON_F_INPUT | HWMON_F_LABEL,
> > +			   HWMON_F_INPUT | HWMON_F_LABEL),
> > +	NULL
> > +};
> > +
> > +static const struct hwmon_chip_info kraken2_chip_info = {
> > +	.ops = &kraken2_hwmon_ops,
> > +	.info = kraken2_info,
> > +};
> > +
> > +static int kraken2_raw_event(struct hid_device *hdev,
> > +			     struct hid_report *report, u8 *data, int size)
> > +{
> > +	struct kraken2_priv_data *priv;
> > +	unsigned long flags;
> > +
> > +	if (size < STATUS_USEFUL_SIZE || report->id != STATUS_REPORT_ID)
> > +		return 0;
> > +
> > +	priv = hid_get_drvdata(hdev);
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> 
> I don't see the point of disabling interrupts here and above.

Before they were disabled because this runs in a hard IRQ context.  But
that also means that, indeed, it wasn't necessary to disable them here.

> 
> Either case, the spinlocks are overkill. It would be much easier to
> convert raw readings here into temperature and fan speed and store
> the resulting values in struct kraken2_priv_data, and then to
> just report it in the read functions. That would be much less costly
> because the spinlock would not be needed at all, and calculations
> would be done only once per event.

Oddly enough, this is very close to how the code read last week.

But I was storing the values in kraken2_priv_data as longs, and I'm not
sure that storing and loading longs is atomic on all architectures.

Was that safe, or should I use something else instead of longs?

> 
> > +	memcpy(priv->status, data, STATUS_USEFUL_SIZE);
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> > +
> > +	return 0;
> > +}
> 
> For my education: What triggers those events ? Are they reported
> by the hardware autonomously whenever something changes ?
> A comment at the top of the driver explaining how this works
> might be useful.

The device autonomously sends these status reports twice a second.

I'll add the comment for v2.

> 
> Also, is there a way to initialize values during probe ? Otherwise
> the driver would report values of 0 until the hardware reports
> something.

The device doesn't respond to HID Get_Report, so we can't get valid
initial values.

The best we can do is identify the situation and report it to
user-space.  Am I right that ENODATA should be used for this?

Thanks,
Jonas

> 
> > +
> > +static int kraken2_probe(struct hid_device *hdev,
> > +			 const struct hid_device_id *id)
> > +{
> > +	struct kraken2_priv_data *priv;
> > +	int ret;
> > +
> > +	priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->hid_dev = hdev;
> > +	spin_lock_init(&priv->lock);
> > +	hid_set_drvdata(hdev, priv);
> > +
> > +	ret = hid_parse(hdev);
> > +	if (ret) {
> > +		hid_err(hdev, "hid parse failed with %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * Enable hidraw so existing user-space tools can continue to work.
> > +	 */
> > +	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> > +	if (ret) {
> > +		hid_err(hdev, "hid hw start failed with %d\n", ret);
> > +		goto fail_and_stop;
> > +	}
> > +
> > +	ret = hid_hw_open(hdev);
> > +	if (ret) {
> > +		hid_err(hdev, "hid hw open failed with %d\n", ret);
> > +		goto fail_and_close;
> > +	}
> > +
> > +	priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "kraken2",
> > +							  priv, &kraken2_chip_info,
> > +							  NULL);
> > +	if (IS_ERR(priv->hwmon_dev)) {
> > +		ret = PTR_ERR(priv->hwmon_dev);
> > +		hid_err(hdev, "hwmon registration failed with %d\n", ret);
> > +		goto fail_and_close;
> > +	}
> > +
> > +	return 0;
> > +
> > +fail_and_close:
> > +	hid_hw_close(hdev);
> > +fail_and_stop:
> > +	hid_hw_stop(hdev);
> > +	return ret;
> > +}
> > +
> > +static void kraken2_remove(struct hid_device *hdev)
> > +{
> > +	struct kraken2_priv_data *priv = hid_get_drvdata(hdev);
> > +
> > +	hwmon_device_unregister(priv->hwmon_dev);
> > +
> > +	hid_hw_close(hdev);
> > +	hid_hw_stop(hdev);
> > +}
> > +
> > +static const struct hid_device_id kraken2_table[] = {
> > +	{ HID_USB_DEVICE(0x1e71, 0x170e) }, /* NZXT Kraken X42/X52/X62/X72 */
> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(hid, kraken2_table);
> > +
> > +static struct hid_driver kraken2_driver = {
> > +	.name = "nzxt-kraken2",
> > +	.id_table = kraken2_table,
> > +	.probe = kraken2_probe,
> > +	.remove = kraken2_remove,
> > +	.raw_event = kraken2_raw_event,
> > +};
> > +
> > +static int __init kraken2_init(void)
> > +{
> > +	return hid_register_driver(&kraken2_driver);
> > +}
> > +
> > +static void __exit kraken2_exit(void)
> > +{
> > +	hid_unregister_driver(&kraken2_driver);
> > +}
> > +
> > +/*
> > + * When compiled into the kernel, initialize after the hid bus.
> > + */
> > +late_initcall(kraken2_init);
> > +module_exit(kraken2_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Jonas Malaco <jonas@protocubo.io>");
> > +MODULE_DESCRIPTION("Hwmon driver for NZXT Kraken X42/X52/X62/X72 coolers");
> > 
> > base-commit: d8a08585259268e7b5305d04422a59d713719ccb
> > 
> 

Re: [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72

[Guenter Roeck]

On Thu, Mar 18, 2021 at 08:15:06PM -0300, Jonas Malaco wrote:

[ ... ]

> > Either case, the spinlocks are overkill. It would be much easier to
> > convert raw readings here into temperature and fan speed and store
> > the resulting values in struct kraken2_priv_data, and then to
> > just report it in the read functions. That would be much less costly
> > because the spinlock would not be needed at all, and calculations
> > would be done only once per event.
> 
> Oddly enough, this is very close to how the code read last week.
> 
> But I was storing the values in kraken2_priv_data as longs, and I'm not
> sure that storing and loading longs is atomic on all architectures.
> 
> Was that safe, or should I use something else instead of longs?
> 

Hard to say, but do you see any code in the kernel that assumes
that loading or storing a long is not atomic, for any architecture ?

Also, I don't see how u16 * 1000 could ever require a long
for storage. int would be good enough.

> > 
> > > +	memcpy(priv->status, data, STATUS_USEFUL_SIZE);
> > > +	spin_unlock_irqrestore(&priv->lock, flags);
> > > +
> > > +	return 0;
> > > +}
> > 
> > For my education: What triggers those events ? Are they reported
> > by the hardware autonomously whenever something changes ?
> > A comment at the top of the driver explaining how this works
> > might be useful.
> 
> The device autonomously sends these status reports twice a second.
> 
> I'll add the comment for v2.
> 
That would be great, thanks.

> > 
> > Also, is there a way to initialize values during probe ? Otherwise
> > the driver would report values of 0 until the hardware reports
> > something.
> 
> The device doesn't respond to HID Get_Report, so we can't get valid
> initial values.
> 
> The best we can do is identify the situation and report it to
> user-space.  Am I right that ENODATA should be used for this?
> 
Yes, I think that would be a good idea, and ENODATA sounds good.

Thanks,
Guenter

Re: [PATCH] hwmon: add driver for NZXT Kraken X42/X52/X62/X72

[Jonas Malaco]

On Thu, Mar 18, 2021 at 04:36:08PM -0700, Guenter Roeck wrote:
> On Thu, Mar 18, 2021 at 08:15:06PM -0300, Jonas Malaco wrote:
> 
> [ ... ]
> 
> > > Either case, the spinlocks are overkill. It would be much easier to
> > > convert raw readings here into temperature and fan speed and store
> > > the resulting values in struct kraken2_priv_data, and then to
> > > just report it in the read functions. That would be much less costly
> > > because the spinlock would not be needed at all, and calculations
> > > would be done only once per event.
> > 
> > Oddly enough, this is very close to how the code read last week.
> > 
> > But I was storing the values in kraken2_priv_data as longs, and I'm not
> > sure that storing and loading longs is atomic on all architectures.
> > 
> > Was that safe, or should I use something else instead of longs?
> > 
> 
> Hard to say, but do you see any code in the kernel that assumes
> that loading or storing a long is not atomic, for any architecture ?

No, I don't think so.

> 
> Also, I don't see how u16 * 1000 could ever require a long
> for storage. int would be good enough.

Oh, that's true.

I'll submit a v2 shortly.

Thanks again,
Jonas

> 
> > > 
> > > > +	memcpy(priv->status, data, STATUS_USEFUL_SIZE);
> > > > +	spin_unlock_irqrestore(&priv->lock, flags);
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > For my education: What triggers those events ? Are they reported
> > > by the hardware autonomously whenever something changes ?
> > > A comment at the top of the driver explaining how this works
> > > might be useful.
> > 
> > The device autonomously sends these status reports twice a second.
> > 
> > I'll add the comment for v2.
> > 
> That would be great, thanks.
> 
> > > 
> > > Also, is there a way to initialize values during probe ? Otherwise
> > > the driver would report values of 0 until the hardware reports
> > > something.
> > 
> > The device doesn't respond to HID Get_Report, so we can't get valid
> > initial values.
> > 
> > The best we can do is identify the situation and report it to
> > user-space.  Am I right that ENODATA should be used for this?
> > 
> Yes, I think that would be a good idea, and ENODATA sounds good.
> 
> Thanks,
> Guenter