Re: [PATCH -next 15/19] hwmon: (corsair-psu) Use devm_hid_hw_start_and_open in corsairpsu_probe()

[Wilken Gottwalt <wilken.gottwalt@posteo.net>]

On Wed, 4 Sep 2024 20:36:03 +0800
Li Zetao <lizetao1@huawei.com> 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@huawei.com>
> ---
>  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@posteo.net>

greetings,
Wilken