[PATCH v2] HID: hiddev: change hiddev_connect() to return bool

[PATCH v2] HID: hiddev: change hiddev_connect() to return bool

[Luis de Bethencourt]

Since hid_connect() only cares about hiddev_connect() succeeding or
failing, there is no need for this function to return an int and it can
return a bool instead.

Suggested-by: Jiri Kosina <jikos@kernel.org>
Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
---

Hi,

No idea why my local build did not complain about the obvious mistake
on the previous version of the patch.

Sorry about that,
Luis

 drivers/hid/hid-core.c      |  2 +-
 drivers/hid/usbhid/hiddev.c | 10 +++++-----
 include/linux/hid.h         |  2 +-
 include/linux/hiddev.h      |  2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index efed99f..a8f3624 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1631,7 +1631,7 @@ int hid_connect(struct hid_device *hdev, unsigned int connect_mask)
 		hdev->claimed |= HID_CLAIMED_INPUT;
 
 	if ((connect_mask & HID_CONNECT_HIDDEV) && hdev->hiddev_connect &&
-			!hdev->hiddev_connect(hdev,
+			hdev->hiddev_connect(hdev,
 				connect_mask & HID_CONNECT_HIDDEV_FORCE))
 		hdev->claimed |= HID_CLAIMED_HIDDEV;
 	if ((connect_mask & HID_CONNECT_HIDRAW) && !hidraw_connect(hdev))
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 2f1ddca..d715632 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -875,7 +875,7 @@ static struct usb_class_driver hiddev_class = {
 /*
  * This is where hid.c calls us to connect a hid device to the hiddev driver
  */
-int hiddev_connect(struct hid_device *hid, unsigned int force)
+bool hiddev_connect(struct hid_device *hid, unsigned int force)
 {
 	struct hiddev *hiddev;
 	struct usbhid_device *usbhid = hid->driver_data;
@@ -890,11 +890,11 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 				break;
 
 		if (i == hid->maxcollection)
-			return -1;
+			return false;
 	}
 
 	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
-		return -1;
+		return false;
 
 	init_waitqueue_head(&hiddev->wait);
 	INIT_LIST_HEAD(&hiddev->list);
@@ -908,9 +908,9 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 		hid_err(hid, "Not able to get a minor for this device\n");
 		hid->hiddev = NULL;
 		kfree(hiddev);
-		return -1;
+		return false;
 	}
-	return 0;
+	return true;
 }
 
 /*
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 251a1d3..4b5477e 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -547,7 +547,7 @@ struct hid_device {							/* device report descriptor */
 	int (*ff_init)(struct hid_device *);
 
 	/* hiddev event handler */
-	int (*hiddev_connect)(struct hid_device *, unsigned int);
+	bool (*hiddev_connect)(struct hid_device *, unsigned int);
 	void (*hiddev_disconnect)(struct hid_device *);
 	void (*hiddev_hid_event) (struct hid_device *, struct hid_field *field,
 				  struct hid_usage *, __s32);
diff --git a/include/linux/hiddev.h b/include/linux/hiddev.h
index a5dd814..c0f5c25 100644
--- a/include/linux/hiddev.h
+++ b/include/linux/hiddev.h
@@ -38,7 +38,7 @@ struct hid_field;
 struct hid_report;
 
 #ifdef CONFIG_USB_HIDDEV
-int hiddev_connect(struct hid_device *hid, unsigned int force);
+bool hiddev_connect(struct hid_device *hid, unsigned int force);
 void hiddev_disconnect(struct hid_device *);
 void hiddev_hid_event(struct hid_device *hid, struct hid_field *field,
 		      struct hid_usage *usage, __s32 value);
-- 
2.5.1

Re: [PATCH v2] HID: hiddev: change hiddev_connect() to return bool

[Krzysztof Kozlowski]

2015-10-09 22:00 GMT+09:00 Luis de Bethencourt <luisbg@osg.samsung.com>:
> Since hid_connect() only cares about hiddev_connect() succeeding or
> failing, there is no need for this function to return an int and it can
> return a bool instead.

It can return bool but it would not be in line with kernel coding
style. The hiddev_connect() I believe is an action, so "the function
should return an error-code integer.".

Best regards,
Krzysztof

>
> Suggested-by: Jiri Kosina <jikos@kernel.org>
> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
> ---
>
> Hi,
>
> No idea why my local build did not complain about the obvious mistake
> on the previous version of the patch.
>
> Sorry about that,
> Luis
>
>  drivers/hid/hid-core.c      |  2 +-
>  drivers/hid/usbhid/hiddev.c | 10 +++++-----
>  include/linux/hid.h         |  2 +-
>  include/linux/hiddev.h      |  2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)

Re: [PATCH v2] HID: hiddev: change hiddev_connect() to return bool

[Luis de Bethencourt]

On 13/10/15 02:49, Krzysztof Kozlowski wrote:
> 2015-10-09 22:00 GMT+09:00 Luis de Bethencourt <luisbg@osg.samsung.com>:
>> Since hid_connect() only cares about hiddev_connect() succeeding or
>> failing, there is no need for this function to return an int and it can
>> return a bool instead.
> 
> It can return bool but it would not be in line with kernel coding
> style. The hiddev_connect() I believe is an action, so "the function
> should return an error-code integer.".
> 
> Best regards,
> Krzysztof
>


Hi Krysztof,

The idea to switch the function to return bool was offered by Jiri Kosina,
as a result of my initial patch changing the return errno code to ENOMEM.

Considering the return isn't propagated by the only consumer of the function,
and your point about returning an integer being the kernel coding style. It
doesn't make sense to change this function.

Thanks for your review!
Luis
 
>>
>> Suggested-by: Jiri Kosina <jikos@kernel.org>
>> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
>> ---
>>
>> Hi,
>>
>> No idea why my local build did not complain about the obvious mistake
>> on the previous version of the patch.
>>
>> Sorry about that,
>> Luis
>>
>>  drivers/hid/hid-core.c      |  2 +-
>>  drivers/hid/usbhid/hiddev.c | 10 +++++-----
>>  include/linux/hid.h         |  2 +-
>>  include/linux/hiddev.h      |  2 +-
>>  4 files changed, 8 insertions(+), 8 deletions(-)

Re: [PATCH v2] HID: hiddev: change hiddev_connect() to return bool

[Krzysztof Kozlowski]

On 20.10.2015 00:11, Luis de Bethencourt wrote:
> On 13/10/15 02:49, Krzysztof Kozlowski wrote:
>> 2015-10-09 22:00 GMT+09:00 Luis de Bethencourt <luisbg@osg.samsung.com>:
>>> Since hid_connect() only cares about hiddev_connect() succeeding or
>>> failing, there is no need for this function to return an int and it can
>>> return a bool instead.
>>
>> It can return bool but it would not be in line with kernel coding
>> style. The hiddev_connect() I believe is an action, so "the function
>> should return an error-code integer.".
>>
>> Best regards,
>> Krzysztof
>>
> 
> 
> Hi Krysztof,
> 
> The idea to switch the function to return bool was offered by Jiri Kosina,
> as a result of my initial patch changing the return errno code to ENOMEM.

I did not see the original comment from Jiri. Actually changing it to
ENOMEM makes more sense to me...

> 
> Considering the return isn't propagated by the only consumer of the function,
> and your point about returning an integer being the kernel coding style. It
> doesn't make sense to change this function.

Jiri is the maintainer here but for me sticking to coding convention
(return errno) makes it easier to read.

Best regards,
Krzysztof