Le 03/12/2023 à 13:06, Aleksa Savic a écrit :
This driver exposes hardware sensors of the Gigabyte AORUS Waterforce
all-in-one CPU liquid coolers, which communicate through a proprietary
USB HID protocol. Report offsets were initially discovered in [1] and
confirmed by me on a Waterforce X240 by observing the sent reports from
the official software.
Available sensors are pump and fan speed in RPM, as well as coolant
temperature. Also available through debugfs is the firmware version.
Attaching a fan is optional and allows it to be controlled from the
device. If it's not connected, the fan-related sensors will report
zeroes.
The addressable RGB LEDs and LCD screen are not supported in this
driver and should be controlled through userspace tools.
[1]: https://github.com/liquidctl/liquidctl/issues/167
Signed-off-by: Aleksa Savic <savicaleksa83@xxxxxxxxx>
---
Hi,
...
+/* Writes the command to the device with the rest of the report filled with zeroes */
+static int waterforce_write_expanded(struct waterforce_data *priv, const u8 *cmd, int cmd_length)
+{
+ int ret;
+
+ mutex_lock(&priv->buffer_lock);
+
+ memset(priv->buffer, 0x00, MAX_REPORT_LENGTH);
+ memcpy(priv->buffer, cmd, cmd_length);
Is memcpy_and_pad() useful here?
+ ret = hid_hw_output_report(priv->hdev, priv->buffer, MAX_REPORT_LENGTH);
+
+ mutex_unlock(&priv->buffer_lock);
+ return ret;
+}
...
+static int waterforce_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ int ret;
+ struct waterforce_data *priv = dev_get_drvdata(dev);
+
+ if (time_after(jiffies, priv->updated + msecs_to_jiffies(STATUS_VALIDITY))) {
+ /* Request status on demand */
+ ret = waterforce_get_status(priv);
+ if (ret < 0)
+ return -ENODATA;
+ }
+
+ switch (type) {
+ case hwmon_temp:
+ *val = priv->temp_input[channel];
+ break;
+ case hwmon_fan:
+ *val = priv->speed_input[channel];
+ break;
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ *val = DIV_ROUND_CLOSEST(priv->duty_input[channel] * 255, 100);
+ break;
+ default:
+ break;
Should we return an error here?
+ }
+ break;
+ default:
+ return -EOPNOTSUPP; /* unreachable */
+ }
+
+ return 0;
+}
...
+static int waterforce_probe(struct hid_device *hdev, const struct hid_device_id *id)
+{
+ struct waterforce_data *priv;
+ int ret;
+
+ priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->hdev = hdev;
+ hid_set_drvdata(hdev, priv);
+
+ /*
+ * Initialize priv->updated to STATUS_VALIDITY seconds in the past, making
+ * the initial empty data invalid for waterforce_read() without the need for
+ * a special case there.
+ */
+ priv->updated = jiffies - msecs_to_jiffies(STATUS_VALIDITY);
+
+ 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;
Should this be 'return ret;' (the _start has failed, so why stop?)
+ }
+
+ ret = hid_hw_open(hdev);
+ if (ret) {
+ hid_err(hdev, "hid hw open failed with %d\n", ret);
+ goto fail_and_close;
Should this be 'fail_and_stop' (the _open has failed, so why close?)
+ }
+
+ priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
+ if (!priv->buffer) {
+ ret = -ENOMEM;
+ goto fail_and_close;
+ }
+
+ mutex_init(&priv->status_report_request_mutex);
+ mutex_init(&priv->buffer_lock);
+ spin_lock_init(&priv->status_report_request_lock);
+ init_completion(&priv->status_report_received);
+ init_completion(&priv->fw_version_processed);
+
+ priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "waterforce",
+ priv, &waterforce_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;
+ }
+
+ hid_device_io_start(hdev);
+ ret = waterforce_get_fw_ver(hdev);
+ if (ret < 0)
+ hid_warn(hdev, "fw version request failed with %d\n", ret);
+ hid_device_io_stop(hdev);
+
+ waterforce_debugfs_init(priv);
+
+ return 0;
+
+fail_and_close:
+ hid_hw_close(hdev);
+fail_and_stop:
+ hid_hw_stop(hdev);
+ return ret;
+}
+
+static void waterforce_remove(struct hid_device *hdev)
+{
+ struct waterforce_data *priv = hid_get_drvdata(hdev);
+
+ hwmon_device_unregister(priv->hwmon_dev);
Should debugfs_remove_recursive(() be called?
(if CONFIG_DEBUG_FS is defined)
CJ
+
+ hid_hw_close(hdev);
+ hid_hw_stop(hdev);
+}
...
