diff mbox series

[-next,v2,09/15] hwmon: Use devm_hid_hw_start_and_open in rog_ryujin_probe()

Message ID 20240909012313.500341-10-lizetao1@huawei.com (mailing list archive)
State Superseded
Headers show
Series HID: convert to devm_hid_hw_start_and_open() | expand

Commit Message

Li Zetao Sept. 9, 2024, 1:23 a.m. UTC
Currently, the rog_ryujin module needs to maintain hid resources
by itself. Use 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.

Further optimization, use devm_hwmon_device_register_with_info to replace
hwmon_device_register_with_info, the remote operation can be completely
deleted, and the rog_ryujin_data structure no longer needs to hold
hwmon device.

Signed-off-by: Li Zetao <lizetao1@huawei.com>
---
v1 -> v2:
  1) Further optimize using devm_hwmon_device_register_with_info
and remove the .remove operation
  2) Adjust commit information
v1:
https://lore.kernel.org/all/20240904123607.3407364-14-lizetao1@huawei.com/

 drivers/hwmon/asus_rog_ryujin.c | 47 +++++----------------------------
 1 file changed, 7 insertions(+), 40 deletions(-)

Comments

Guenter Roeck Sept. 9, 2024, 4:59 p.m. UTC | #1
On Mon, Sep 09, 2024 at 09:23:07AM +0800, Li Zetao wrote:
> Currently, the rog_ryujin module needs to maintain hid resources
> by itself. Use 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.
> 
> Further optimization, use devm_hwmon_device_register_with_info to replace
> hwmon_device_register_with_info, the remote operation can be completely
> deleted, and the rog_ryujin_data structure no longer needs to hold
> hwmon device.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>

Subject should include 'asus_rog_ryujin'. Other than that,

Acked-by: Guenter Roeck <linux@roeck-us.net>

> ---
> v1 -> v2:
>   1) Further optimize using devm_hwmon_device_register_with_info
> and remove the .remove operation
>   2) Adjust commit information
> v1:
> https://lore.kernel.org/all/20240904123607.3407364-14-lizetao1@huawei.com/
> 
>  drivers/hwmon/asus_rog_ryujin.c | 47 +++++----------------------------
>  1 file changed, 7 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
> index f8b20346a995..ef0d9b72a824 100644
> --- a/drivers/hwmon/asus_rog_ryujin.c
> +++ b/drivers/hwmon/asus_rog_ryujin.c
> @@ -80,7 +80,6 @@ static const char *const rog_ryujin_speed_label[] = {
>  
>  struct rog_ryujin_data {
>  	struct hid_device *hdev;
> -	struct device *hwmon_dev;
>  	/* For locking access to buffer */
>  	struct mutex buffer_lock;
>  	/* For queueing multiple readers */
> @@ -497,6 +496,7 @@ static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
>  static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
>  	struct rog_ryujin_data *priv;
> +	struct device *hwmon_dev;
>  	int ret;
>  
>  	priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
> @@ -520,23 +520,13 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
>  	}
>  
>  	/* 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);
> +	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
> +	if (ret)
>  		return ret;
> -	}
> -
> -	ret = hid_hw_open(hdev);
> -	if (ret) {
> -		hid_err(hdev, "hid hw open failed with %d\n", ret);
> -		goto fail_and_stop;
> -	}
>  
>  	priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
> -	if (!priv->buffer) {
> -		ret = -ENOMEM;
> -		goto fail_and_close;
> -	}
> +	if (!priv->buffer)
> +		return -ENOMEM;
>  
>  	mutex_init(&priv->status_report_request_mutex);
>  	mutex_init(&priv->buffer_lock);
> @@ -548,31 +538,9 @@ static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
>  	init_completion(&priv->cooler_duty_set);
>  	init_completion(&priv->controller_duty_set);
>  
> -	priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "rog_ryujin",
> +	hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "rog_ryujin",
>  							  priv, &rog_ryujin_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;
> -	}
> -
> -	return 0;
> -
> -fail_and_close:
> -	hid_hw_close(hdev);
> -fail_and_stop:
> -	hid_hw_stop(hdev);
> -	return ret;
> -}
> -
> -static void rog_ryujin_remove(struct hid_device *hdev)
> -{
> -	struct rog_ryujin_data *priv = hid_get_drvdata(hdev);
> -
> -	hwmon_device_unregister(priv->hwmon_dev);
> -
> -	hid_hw_close(hdev);
> -	hid_hw_stop(hdev);
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
>  }
>  
>  static const struct hid_device_id rog_ryujin_table[] = {
> @@ -586,7 +554,6 @@ static struct hid_driver rog_ryujin_driver = {
>  	.name = "rog_ryujin",
>  	.id_table = rog_ryujin_table,
>  	.probe = rog_ryujin_probe,
> -	.remove = rog_ryujin_remove,
>  	.raw_event = rog_ryujin_raw_event,
>  };
>  
> -- 
> 2.34.1
> 
>
Aleksa Savic Sept. 9, 2024, 5:32 p.m. UTC | #2
On 2024-09-09 03:23:07 GMT+02:00, Li Zetao wrote:
> Currently, the rog_ryujin module needs to maintain hid resources
> by itself. Use 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.
> 
> Further optimization, use devm_hwmon_device_register_with_info to replace
> hwmon_device_register_with_info, the remote operation can be completely
> deleted, and the rog_ryujin_data structure no longer needs to hold
> hwmon device.
> 
> Signed-off-by: Li Zetao <lizetao1@huawei.com>

Reviewed-by: Aleksa Savic <savicaleksa83@gmail.com>
diff mbox series

Patch

diff --git a/drivers/hwmon/asus_rog_ryujin.c b/drivers/hwmon/asus_rog_ryujin.c
index f8b20346a995..ef0d9b72a824 100644
--- a/drivers/hwmon/asus_rog_ryujin.c
+++ b/drivers/hwmon/asus_rog_ryujin.c
@@ -80,7 +80,6 @@  static const char *const rog_ryujin_speed_label[] = {
 
 struct rog_ryujin_data {
 	struct hid_device *hdev;
-	struct device *hwmon_dev;
 	/* For locking access to buffer */
 	struct mutex buffer_lock;
 	/* For queueing multiple readers */
@@ -497,6 +496,7 @@  static int rog_ryujin_raw_event(struct hid_device *hdev, struct hid_report *repo
 static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id *id)
 {
 	struct rog_ryujin_data *priv;
+	struct device *hwmon_dev;
 	int ret;
 
 	priv = devm_kzalloc(&hdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -520,23 +520,13 @@  static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
 	}
 
 	/* 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);
+	ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
+	if (ret)
 		return ret;
-	}
-
-	ret = hid_hw_open(hdev);
-	if (ret) {
-		hid_err(hdev, "hid hw open failed with %d\n", ret);
-		goto fail_and_stop;
-	}
 
 	priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
-	if (!priv->buffer) {
-		ret = -ENOMEM;
-		goto fail_and_close;
-	}
+	if (!priv->buffer)
+		return -ENOMEM;
 
 	mutex_init(&priv->status_report_request_mutex);
 	mutex_init(&priv->buffer_lock);
@@ -548,31 +538,9 @@  static int rog_ryujin_probe(struct hid_device *hdev, const struct hid_device_id
 	init_completion(&priv->cooler_duty_set);
 	init_completion(&priv->controller_duty_set);
 
-	priv->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "rog_ryujin",
+	hwmon_dev = devm_hwmon_device_register_with_info(&hdev->dev, "rog_ryujin",
 							  priv, &rog_ryujin_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;
-	}
-
-	return 0;
-
-fail_and_close:
-	hid_hw_close(hdev);
-fail_and_stop:
-	hid_hw_stop(hdev);
-	return ret;
-}
-
-static void rog_ryujin_remove(struct hid_device *hdev)
-{
-	struct rog_ryujin_data *priv = hid_get_drvdata(hdev);
-
-	hwmon_device_unregister(priv->hwmon_dev);
-
-	hid_hw_close(hdev);
-	hid_hw_stop(hdev);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
 }
 
 static const struct hid_device_id rog_ryujin_table[] = {
@@ -586,7 +554,6 @@  static struct hid_driver rog_ryujin_driver = {
 	.name = "rog_ryujin",
 	.id_table = rog_ryujin_table,
 	.probe = rog_ryujin_probe,
-	.remove = rog_ryujin_remove,
 	.raw_event = rog_ryujin_raw_event,
 };