Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <5404194d-9ee6-4def-a040-f5858d8c2d66@wanadoo.fr>
Date:   Sun, 3 Dec 2023 16:26:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] hwmon: Add driver for Gigabyte AORUS Waterforce AIO
 coolers
Content-Language: fr
To:     Aleksa Savic <savicaleksa83@gmail.com>, linux-hwmon@vger.kernel.org
Cc:     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>
References: <20231203120651.371429-1-savicaleksa83@gmail.com>
 <2a89f0b4-990a-4d0d-8e54-c4215579c23c@wanadoo.fr>
 <121470f0-6c1f-418a-844c-7ec2e8a54b8e@gmail.com>
From:   Christophe JAILLET <christophe.jaillet@wanadoo.fr>
In-Reply-To: <121470f0-6c1f-418a-844c-7ec2e8a54b8e@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Le 03/12/2023 à 16:00, Aleksa Savic a écrit :
>>> +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.

Done, patch sent.

CH