On 2023-12-03 15:14:05 GMT+01:00, Christophe JAILLET wrote: > 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, > ... Hi Christophe! > >> +/* 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? Looks like it is, thanks. Didn't know about that one. > >> + 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? Only hwmon_pwm_input is made visible in waterforce_is_visible(), so I'm not sure if anything else can get passed in? (Like the default case below.) I'll add an error return here, just wondering. > >> + } >> + 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?) Hm, yes. > >> + } >> + >> + 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?) Also yes... I based this part on the nzxt-kraken2 driver in the tree, perhaps that should be investigated as well. The aquacomputer_d5next driver seems to be doing it correctly. > >> + } >> + >> + 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) It should. Sorry, missed it. Thanks a lot, Aleksa > > CJ > >> + >> + hid_hw_close(hdev); >> + hid_hw_stop(hdev); >> +} > > ... >
