Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Message-ID: <121470f0-6c1f-418a-844c-7ec2e8a54b8e@gmail.com>
Date:   Sun, 3 Dec 2023 16:00:16 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc:     savicaleksa83@gmail.com, Jean Delvare <jdelvare@suse.com>,
        Guenter Roeck <linux@roeck-us.net>,
        Jonathan Corbet <corbet@lwn.net>,
        "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2] hwmon: Add driver for Gigabyte AORUS Waterforce AIO
 coolers
Content-Language: en-US
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
        linux-hwmon@vger.kernel.org
References: <20231203120651.371429-1-savicaleksa83@gmail.com>
 <2a89f0b4-990a-4d0d-8e54-c4215579c23c@wanadoo.fr>
From:   Aleksa Savic <savicaleksa83@gmail.com>
In-Reply-To: <2a89f0b4-990a-4d0d-8e54-c4215579c23c@wanadoo.fr>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@gmail.com>
>> ---
> 
> 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);
>> +}
> 
> ...
>