On Wed, 4 Sep 2024 20:36:03 +0800
Li Zetao <lizetao1@xxxxxxxxxx> wrote:
> Currently, the corsair-psu module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> that hid resources are consistent with the device life cycle, and release
> hid resources before device is released. At the same time, it can avoid
> the goto-release encoding, drop the fail_and_close and fail_and_stop
> lables, and directly return the error code when an error occurs.
>
> Signed-off-by: Li Zetao <lizetao1@xxxxxxxxxx>
> ---
> drivers/hwmon/corsair-psu.c | 24 +++++-------------------
> 1 file changed, 5 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
> index f8f22b8a67cd..b574ec9cd00f 100644
> --- a/drivers/hwmon/corsair-psu.c
> +++ b/drivers/hwmon/corsair-psu.c
> @@ -787,14 +787,10 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id if (ret)
> return ret;
>
> - ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> + ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
> if (ret)
> return ret;
>
> - ret = hid_hw_open(hdev);
> - if (ret)
> - goto fail_and_stop;
> -
> priv->hdev = hdev;
> hid_set_drvdata(hdev, priv);
> mutex_init(&priv->lock);
> @@ -805,13 +801,13 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id ret = corsairpsu_init(priv);
> if (ret < 0) {
> dev_err(&hdev->dev, "unable to initialize device (%d)\n", ret);
> - goto fail_and_stop;
> + return ret;
> }
>
> ret = corsairpsu_fwinfo(priv);
> if (ret < 0) {
> dev_err(&hdev->dev, "unable to query firmware (%d)\n", ret);
> - goto fail_and_stop;
> + return ret;
> }
>
> corsairpsu_get_criticals(priv);
> @@ -820,20 +816,12 @@ static int corsairpsu_probe(struct hid_device *hdev, const struct
> hid_device_id priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsairpsu", priv,
> &corsairpsu_chip_info, NULL);
>
> - if (IS_ERR(priv->hwmon_dev)) {
> - ret = PTR_ERR(priv->hwmon_dev);
> - goto fail_and_close;
> - }
> + if (IS_ERR(priv->hwmon_dev))
> + return PTR_ERR(priv->hwmon_dev);
>
> corsairpsu_debugfs_init(priv);
>
> return 0;
> -
> -fail_and_close:
> - hid_hw_close(hdev);
> -fail_and_stop:
> - hid_hw_stop(hdev);
> - return ret;
> }
>
> static void corsairpsu_remove(struct hid_device *hdev)
> @@ -842,8 +830,6 @@ static void corsairpsu_remove(struct hid_device *hdev)
>
> debugfs_remove_recursive(priv->debugfs);
> hwmon_device_unregister(priv->hwmon_dev);
> - hid_hw_close(hdev);
> - hid_hw_stop(hdev);
> }
>
> static int corsairpsu_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *data,
So far looks fine to me.
Reviewed-by: Wilken Gottwalt <wilken.gottwalt@xxxxxxxxxx>
greetings,
Wilken
