Re: [PATCH 1/5] hwmon: (corsair-cpro) Use devres function

[Guenter Roeck <linux@xxxxxxxxxxxx>]

On 12/21/21 6:01 PM, Jackie Liu wrote:
> From: Jackie Liu <liuyun01@xxxxxxxxxx>
>
> Use devm_hwmon_device_register_with_info() and remove hwmon_dev
> from ccp_device struct as it is not needed anymore.
>
> Signed-off-by: Jackie Liu <liuyun01@xxxxxxxxxx>
> ---
>   drivers/hwmon/corsair-cpro.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index fa6aa4fc8b52..f476367ba6cf 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -76,7 +76,6 @@
>   
>   struct ccp_device {
>   	struct hid_device *hdev;
> -	struct device *hwmon_dev;
>   	struct completion wait_input_report;
>   	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
>   	u8 *buffer;
> @@ -486,6 +485,7 @@ static int get_temp_cnct(struct ccp_device *ccp)
>   static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   {
>   	struct ccp_device *ccp;
> +	struct device *hwmon_dev;
>   	int ret;
>   
>   	ccp = devm_kzalloc(&hdev->dev, sizeof(*ccp), GFP_KERNEL);
> @@ -523,12 +523,12 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   	ret = get_fan_cnct(ccp);
>   	if (ret)
>   		goto out_hw_close;
> -	ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, "corsaircpro",
> -							 ccp, &ccp_chip_info, 0);
> -	if (IS_ERR(ccp->hwmon_dev)) {
> -		ret = PTR_ERR(ccp->hwmon_dev);
> +	hwmon_dev =
> +		devm_hwmon_device_register_with_info(&hdev->dev, "corsaircpro",
> +						     ccp, &ccp_chip_info, 0);
> +	ret = PTR_ERR_OR_ZERO(hwmon_dev);
> +	if (ret)
>   		goto out_hw_close;
> -	}
>   
>   	return 0;
>   
> @@ -541,9 +541,6 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   
>   static void ccp_remove(struct hid_device *hdev)
>   {
> -	struct ccp_device *ccp = hid_get_drvdata(hdev);
> -
> -	hwmon_device_unregister(ccp->hwmon_dev);
>   	hid_hw_close(hdev);
>   	hid_hw_stop(hdev);

The point is that the above two functions need to be called _after_ the hwmon device
was removed. This patch changes the order and removes the hwmon device after the hid
functions have been removed.

If you think this is valid you'll need to explain in detail why removal order
does not matter. Otherwise this patch deserves a NACK.

Guenter