* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
2011-10-26 0:19 ` [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean K. Y. Srinivasan
@ 2011-10-26 0:03 ` Joe Perches
2011-10-26 6:07 ` Greg KH
2011-10-26 0:19 ` [PATCH 2/6] Staging: hv: mousevsc: Inline the code for mousevsc_on_device_add() K. Y. Srinivasan
` (4 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Joe Perches @ 2011-10-26 0:03 UTC (permalink / raw)
To: K. Y. Srinivasan
Cc: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, Haiyang Zhang
On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> Make some state that is boolean in nature, a boolean variable.
[]
> diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
[]
@@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> */
> struct mousevsc_dev {
> struct hv_device *device;
> - unsigned char init_complete;
> + bool init_complete;
> struct mousevsc_prt_msg protocol_req;
> struct mousevsc_prt_msg protocol_resp;
> /* Synchronize the request/response if needed */
> @@ -159,7 +159,7 @@ struct mousevsc_dev {
> unsigned char *report_desc;
> u32 report_desc_size;
> struct hv_input_dev_info hid_dev_info;
> - int connected;
> + bool connected;
> struct hid_device *hid_device;
> };
One of the bools should be moved to be adjacent to the other.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/6] Staging: hv: mousevsc: cleanup the mouse driver
@ 2011-10-26 0:18 K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean K. Y. Srinivasan
0 siblings, 1 reply; 13+ messages in thread
From: K. Y. Srinivasan @ 2011-10-26 0:18 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan
As part of the effort to move the mouse driver out of staging, I had posted
the code for review by the community. This patch-set addresses the comments I
received. I would like to thank Dmitry and Joe for their comments and suggestions.
Regards,
K. Y
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
2011-10-26 0:18 [PATCH 0/6] Staging: hv: mousevsc: cleanup the mouse driver K. Y. Srinivasan
@ 2011-10-26 0:19 ` K. Y. Srinivasan
2011-10-26 0:03 ` Joe Perches
` (5 more replies)
0 siblings, 6 replies; 13+ messages in thread
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: Haiyang Zhang
Make some state that is boolean in nature, a boolean variable.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index ccd39c7..15aa2ce 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
*/
struct mousevsc_dev {
struct hv_device *device;
- unsigned char init_complete;
+ bool init_complete;
struct mousevsc_prt_msg protocol_req;
struct mousevsc_prt_msg protocol_resp;
/* Synchronize the request/response if needed */
@@ -159,7 +159,7 @@ struct mousevsc_dev {
unsigned char *report_desc;
u32 report_desc_size;
struct hv_input_dev_info hid_dev_info;
- int connected;
+ bool connected;
struct hid_device *hid_device;
};
@@ -487,7 +487,7 @@ static void reportdesc_callback(struct hv_device *dev, void *packet, u32 len)
if (!hidinput_connect(hid_dev, 0)) {
hid_dev->claimed |= HID_CLAIMED_INPUT;
- input_device->connected = 1;
+ input_device->connected = true;
}
@@ -558,7 +558,7 @@ static int mousevsc_remove(struct hv_device *dev)
if (input_dev->connected) {
hidinput_disconnect(input_dev->hid_device);
- input_dev->connected = 0;
+ input_dev->connected = false;
hid_destroy_device(input_dev->hid_device);
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/6] Staging: hv: mousevsc: Inline the code for mousevsc_on_device_add()
2011-10-26 0:19 ` [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean K. Y. Srinivasan
2011-10-26 0:03 ` Joe Perches
@ 2011-10-26 0:19 ` K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 3/6] Staging: hv: mousevsc: Inline the code for reportdesc_callback() K. Y. Srinivasan
` (3 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan, Haiyang Zhang
Inline the code for mousevsc_on_device_add() as this only used from
the function mousevsc_probe().
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 10 ++--------
1 files changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index 15aa2ce..0a5676c 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -494,7 +494,8 @@ static void reportdesc_callback(struct hv_device *dev, void *packet, u32 len)
input_device->hid_device = hid_dev;
}
-static int mousevsc_on_device_add(struct hv_device *device)
+static int mousevsc_probe(struct hv_device *device,
+ const struct hv_vmbus_device_id *dev_id)
{
int ret = 0;
struct mousevsc_dev *input_dev;
@@ -542,13 +543,6 @@ static int mousevsc_on_device_add(struct hv_device *device)
return ret;
}
-static int mousevsc_probe(struct hv_device *dev,
- const struct hv_vmbus_device_id *dev_id)
-{
-
- return mousevsc_on_device_add(dev);
-
-}
static int mousevsc_remove(struct hv_device *dev)
{
--
1.7.4.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/6] Staging: hv: mousevsc: Inline the code for reportdesc_callback()
2011-10-26 0:19 ` [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean K. Y. Srinivasan
2011-10-26 0:03 ` Joe Perches
2011-10-26 0:19 ` [PATCH 2/6] Staging: hv: mousevsc: Inline the code for mousevsc_on_device_add() K. Y. Srinivasan
@ 2011-10-26 0:19 ` K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 4/6] Staging: hv: mousevsc: Cleanup mousevsc_on_channel_callback() K. Y. Srinivasan
` (2 subsequent siblings)
5 siblings, 0 replies; 13+ messages in thread
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan, Haiyang Zhang
Inline the code for reportdesc_callback() as this function is called from
mousevsc_probe(). As part of this, cleanup the code in reportdesc_callback().
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 93 +++++++++++++++++++++++-----------------
1 files changed, 53 insertions(+), 40 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index 0a5676c..ed071b8 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -451,54 +451,34 @@ static int mousevsc_hid_open(struct hid_device *hid)
return 0;
}
+static int mousevsc_hid_start(struct hid_device *hid)
+{
+ return 0;
+}
+
static void mousevsc_hid_close(struct hid_device *hid)
{
}
+static void mousevsc_hid_stop(struct hid_device *hid)
+{
+}
+
static struct hid_ll_driver mousevsc_ll_driver = {
.open = mousevsc_hid_open,
.close = mousevsc_hid_close,
+ .start = mousevsc_hid_start,
+ .stop = mousevsc_hid_stop,
};
static struct hid_driver mousevsc_hid_driver;
-static void reportdesc_callback(struct hv_device *dev, void *packet, u32 len)
-{
- struct hid_device *hid_dev;
- struct mousevsc_dev *input_device = hv_get_drvdata(dev);
-
- hid_dev = hid_allocate_device();
- if (IS_ERR(hid_dev))
- return;
-
- hid_dev->ll_driver = &mousevsc_ll_driver;
- hid_dev->driver = &mousevsc_hid_driver;
-
- if (hid_parse_report(hid_dev, packet, len))
- return;
-
- hid_dev->bus = BUS_VIRTUAL;
- hid_dev->vendor = input_device->hid_dev_info.vendor;
- hid_dev->product = input_device->hid_dev_info.product;
- hid_dev->version = input_device->hid_dev_info.version;
-
- sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
-
- if (!hidinput_connect(hid_dev, 0)) {
- hid_dev->claimed |= HID_CLAIMED_INPUT;
-
- input_device->connected = true;
-
- }
-
- input_device->hid_device = hid_dev;
-}
-
static int mousevsc_probe(struct hv_device *device,
const struct hv_vmbus_device_id *dev_id)
{
int ret = 0;
struct mousevsc_dev *input_dev;
+ struct hid_device *hid_dev;
input_dev = alloc_input_device(device);
@@ -524,23 +504,56 @@ static int mousevsc_probe(struct hv_device *device,
ret = mousevsc_connect_to_vsp(device);
- if (ret != 0) {
- vmbus_close(device->channel);
- free_input_device(input_dev);
- return ret;
- }
-
+ if (ret != 0)
+ goto probe_err0;
/* workaround SA-167 */
if (input_dev->report_desc[14] == 0x25)
input_dev->report_desc[14] = 0x29;
- reportdesc_callback(device, input_dev->report_desc,
- input_dev->report_desc_size);
+ hid_dev = hid_allocate_device();
+ if (IS_ERR(hid_dev)) {
+ ret = PTR_ERR(hid_dev);
+ goto probe_err0;
+ }
+
+ hid_dev->ll_driver = &mousevsc_ll_driver;
+ hid_dev->driver = &mousevsc_hid_driver;
+ hid_dev->bus = BUS_VIRTUAL;
+ hid_dev->vendor = input_dev->hid_dev_info.vendor;
+ hid_dev->product = input_dev->hid_dev_info.product;
+ hid_dev->version = input_dev->hid_dev_info.version;
+ input_dev->hid_device = hid_dev;
+
+ sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse");
+
+ ret = hid_parse_report(hid_dev, input_dev->report_desc,
+ input_dev->report_desc_size);
+
+ if (ret) {
+ hid_err(hid_dev, "parse failed\n");
+ goto probe_err1;
+ }
+
+ ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | HID_CONNECT_HIDDEV);
+ if (ret) {
+ hid_err(hid_dev, "hw start failed\n");
+ goto probe_err1;
+ }
+
+ input_dev->connected = true;
input_dev->init_complete = true;
return ret;
+
+probe_err1:
+ hid_destroy_device(hid_dev);
+
+probe_err0:
+ vmbus_close(device->channel);
+ free_input_device(input_dev);
+ return ret;
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/6] Staging: hv: mousevsc: Cleanup mousevsc_on_channel_callback()
2011-10-26 0:19 ` [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean K. Y. Srinivasan
` (2 preceding siblings ...)
2011-10-26 0:19 ` [PATCH 3/6] Staging: hv: mousevsc: Inline the code for reportdesc_callback() K. Y. Srinivasan
@ 2011-10-26 0:19 ` K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 5/6] Staging: hv: mousevsc: Add a new line to a debug string K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 6/6] Staging: hv: mousevsc: Get rid of unnecessary include files K. Y. Srinivasan
5 siblings, 0 replies; 13+ messages in thread
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan, Haiyang Zhang
Cleanup mousevsc_on_channel_callback(). This is based on the code provided
by Joe Perches <joe@perches.com>.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 85 ++++++++++++++++++-----------------------
1 files changed, 37 insertions(+), 48 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index ed071b8..b10466f 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -313,73 +313,62 @@ static void mousevsc_on_receive(struct hv_device *device,
static void mousevsc_on_channel_callback(void *context)
{
- const int packetSize = 0x100;
- int ret = 0;
- struct hv_device *device = (struct hv_device *)context;
-
+ const int packet_size = 0x100;
+ int ret;
+ struct hv_device *device = context;
u32 bytes_recvd;
u64 req_id;
- unsigned char packet[0x100];
struct vmpacket_descriptor *desc;
- unsigned char *buffer = packet;
- int bufferlen = packetSize;
+ unsigned char *buffer;
+ int bufferlen = packet_size;
+ buffer = kmalloc(bufferlen, GFP_ATOMIC);
+ if (!buffer)
+ return;
do {
ret = vmbus_recvpacket_raw(device->channel, buffer,
bufferlen, &bytes_recvd, &req_id);
- if (ret == 0) {
- if (bytes_recvd > 0) {
- desc = (struct vmpacket_descriptor *)buffer;
-
- switch (desc->type) {
- case VM_PKT_COMP:
- break;
-
- case VM_PKT_DATA_INBAND:
- mousevsc_on_receive(
- device, desc);
- break;
-
- default:
- pr_err("unhandled packet type %d, tid %llx len %d\n",
- desc->type,
- req_id,
- bytes_recvd);
- break;
- }
-
- /* reset */
- if (bufferlen > packetSize) {
- kfree(buffer);
-
- buffer = packet;
- bufferlen = packetSize;
- }
- } else {
- if (bufferlen > packetSize) {
- kfree(buffer);
-
- buffer = packet;
- bufferlen = packetSize;
- }
+ switch (ret) {
+ case 0:
+ if (bytes_recvd <= 0) {
+ kfree(buffer);
+ return;
+ }
+ desc = (struct vmpacket_descriptor *)buffer;
+
+ switch (desc->type) {
+ case VM_PKT_COMP:
+ break;
+
+ case VM_PKT_DATA_INBAND:
+ mousevsc_on_receive(device, desc);
+ break;
+
+ default:
+ pr_err("unhandled packet type %d, tid %llx len %d\n",
+ desc->type,
+ req_id,
+ bytes_recvd);
break;
}
- } else if (ret == -ENOBUFS) {
+
+ break;
+
+ case -ENOBUFS:
+ kfree(buffer);
/* Handle large packet */
bufferlen = bytes_recvd;
- buffer = kzalloc(bytes_recvd, GFP_ATOMIC);
+ buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
if (buffer == NULL) {
- buffer = packet;
- bufferlen = packetSize;
- break;
+ return;
}
+ break;
}
} while (1);
- return;
}
static int mousevsc_connect_to_vsp(struct hv_device *device)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 5/6] Staging: hv: mousevsc: Add a new line to a debug string
2011-10-26 0:19 ` [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean K. Y. Srinivasan
` (3 preceding siblings ...)
2011-10-26 0:19 ` [PATCH 4/6] Staging: hv: mousevsc: Cleanup mousevsc_on_channel_callback() K. Y. Srinivasan
@ 2011-10-26 0:19 ` K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 6/6] Staging: hv: mousevsc: Get rid of unnecessary include files K. Y. Srinivasan
5 siblings, 0 replies; 13+ messages in thread
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: Haiyang Zhang
Add a new line to a debug string.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index b10466f..21cd050 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -411,7 +411,7 @@ static int mousevsc_connect_to_vsp(struct hv_device *device)
response = &input_dev->protocol_resp;
if (!response->response.approved) {
- pr_err("synthhid protocol request failed (version %d)",
+ pr_err("synthhid protocol request failed (version %d)\n",
SYNTHHID_INPUT_VERSION);
ret = -ENODEV;
goto cleanup;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 6/6] Staging: hv: mousevsc: Get rid of unnecessary include files
2011-10-26 0:19 ` [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean K. Y. Srinivasan
` (4 preceding siblings ...)
2011-10-26 0:19 ` [PATCH 5/6] Staging: hv: mousevsc: Add a new line to a debug string K. Y. Srinivasan
@ 2011-10-26 0:19 ` K. Y. Srinivasan
5 siblings, 0 replies; 13+ messages in thread
From: K. Y. Srinivasan @ 2011-10-26 0:19 UTC (permalink / raw)
To: gregkh, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, joe
Cc: K. Y. Srinivasan, Haiyang Zhang
Get rid of unnecessary include files.
Signed-off-by: K. Y. Srinivasan <kys@microsoft.com>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/staging/hv/hv_mouse.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
index 21cd050..dd42411 100644
--- a/drivers/staging/hv/hv_mouse.c
+++ b/drivers/staging/hv/hv_mouse.c
@@ -14,11 +14,8 @@
*/
#include <linux/init.h>
#include <linux/module.h>
-#include <linux/delay.h>
#include <linux/device.h>
-#include <linux/workqueue.h>
-#include <linux/sched.h>
-#include <linux/wait.h>
+#include <linux/completion.h>
#include <linux/input.h>
#include <linux/hid.h>
#include <linux/hiddev.h>
--
1.7.4.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
2011-10-26 0:03 ` Joe Perches
@ 2011-10-26 6:07 ` Greg KH
2011-10-26 22:46 ` Joe Perches
0 siblings, 1 reply; 13+ messages in thread
From: Greg KH @ 2011-10-26 6:07 UTC (permalink / raw)
To: Joe Perches
Cc: K. Y. Srinivasan, linux-kernel, devel, virtualization, ohering,
dmitry.torokhov, Haiyang Zhang
On Tue, Oct 25, 2011 at 05:03:47PM -0700, Joe Perches wrote:
> On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> > Make some state that is boolean in nature, a boolean variable.
> []
> > diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> []
> @@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> > */
> > struct mousevsc_dev {
> > struct hv_device *device;
> > - unsigned char init_complete;
> > + bool init_complete;
> > struct mousevsc_prt_msg protocol_req;
> > struct mousevsc_prt_msg protocol_resp;
> > /* Synchronize the request/response if needed */
> > @@ -159,7 +159,7 @@ struct mousevsc_dev {
> > unsigned char *report_desc;
> > u32 report_desc_size;
> > struct hv_input_dev_info hid_dev_info;
> > - int connected;
> > + bool connected;
> > struct hid_device *hid_device;
> > };
>
> One of the bools should be moved to be adjacent to the other.
For a mouse driver, it really doesn't matter.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
2011-10-26 6:07 ` Greg KH
@ 2011-10-26 22:46 ` Joe Perches
2011-10-26 23:50 ` Greg KH
0 siblings, 1 reply; 13+ messages in thread
From: Joe Perches @ 2011-10-26 22:46 UTC (permalink / raw)
To: Greg KH
Cc: Haiyang Zhang, dmitry.torokhov, ohering, linux-kernel,
virtualization, devel
On Wed, 2011-10-26 at 08:07 +0200, Greg KH wrote:
> On Tue, Oct 25, 2011 at 05:03:47PM -0700, Joe Perches wrote:
> > On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> > > Make some state that is boolean in nature, a boolean variable.
> > []
> > > diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> > []
> > @@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> > > */
> > > struct mousevsc_dev {
> > > struct hv_device *device;
> > > - unsigned char init_complete;
> > > + bool init_complete;
> > > struct mousevsc_prt_msg protocol_req;
> > > struct mousevsc_prt_msg protocol_resp;
> > > /* Synchronize the request/response if needed */
> > > @@ -159,7 +159,7 @@ struct mousevsc_dev {
> > > unsigned char *report_desc;
> > > u32 report_desc_size;
> > > struct hv_input_dev_info hid_dev_info;
> > > - int connected;
> > > + bool connected;
> > > struct hid_device *hid_device;
> > > };
> >
> > One of the bools should be moved to be adjacent to the other.
> For a mouse driver, it really doesn't matter.
'course it doesn't matter, just like whitespace
doesn't matter. It's just done for form's sake.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
2011-10-26 22:46 ` Joe Perches
@ 2011-10-26 23:50 ` Greg KH
2011-10-26 23:59 ` Joe Perches
2011-10-27 0:03 ` Dmitry Torokhov
0 siblings, 2 replies; 13+ messages in thread
From: Greg KH @ 2011-10-26 23:50 UTC (permalink / raw)
To: Joe Perches
Cc: Haiyang Zhang, dmitry.torokhov, ohering, linux-kernel,
virtualization, devel
On Wed, Oct 26, 2011 at 03:46:51PM -0700, Joe Perches wrote:
> On Wed, 2011-10-26 at 08:07 +0200, Greg KH wrote:
> > On Tue, Oct 25, 2011 at 05:03:47PM -0700, Joe Perches wrote:
> > > On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> > > > Make some state that is boolean in nature, a boolean variable.
> > > []
> > > > diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> > > []
> > > @@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> > > > */
> > > > struct mousevsc_dev {
> > > > struct hv_device *device;
> > > > - unsigned char init_complete;
> > > > + bool init_complete;
> > > > struct mousevsc_prt_msg protocol_req;
> > > > struct mousevsc_prt_msg protocol_resp;
> > > > /* Synchronize the request/response if needed */
> > > > @@ -159,7 +159,7 @@ struct mousevsc_dev {
> > > > unsigned char *report_desc;
> > > > u32 report_desc_size;
> > > > struct hv_input_dev_info hid_dev_info;
> > > > - int connected;
> > > > + bool connected;
> > > > struct hid_device *hid_device;
> > > > };
> > >
> > > One of the bools should be moved to be adjacent to the other.
> > For a mouse driver, it really doesn't matter.
>
> 'course it doesn't matter, just like whitespace
> doesn't matter. It's just done for form's sake.
Not at all.
Whitespace does matter, there is documented proof of that.
The fact that you want to move a bool to a different part of a structure
really is only your personal choice. It doesn't matter for size or
layout to make this code work better at all to do this.
This is your personal opinion only, please don't construe it as anything
else.
greg k-h
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
2011-10-26 23:50 ` Greg KH
@ 2011-10-26 23:59 ` Joe Perches
2011-10-27 0:03 ` Dmitry Torokhov
1 sibling, 0 replies; 13+ messages in thread
From: Joe Perches @ 2011-10-26 23:59 UTC (permalink / raw)
To: Greg KH
Cc: Haiyang Zhang, dmitry.torokhov, ohering, linux-kernel,
virtualization, devel
On Thu, 2011-10-27 at 01:50 +0200, Greg KH wrote:
> On Wed, Oct 26, 2011 at 03:46:51PM -0700, Joe Perches wrote:
> > On Wed, 2011-10-26 at 08:07 +0200, Greg KH wrote:
> > > On Tue, Oct 25, 2011 at 05:03:47PM -0700, Joe Perches wrote:
> > > > On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> > > > > Make some state that is boolean in nature, a boolean variable.
> > > > []
> > > > > diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> > > > []
> > > > @@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> > > > > */
> > > > > struct mousevsc_dev {
> > > > > struct hv_device *device;
> > > > > - unsigned char init_complete;
> > > > > + bool init_complete;
> > > > > struct mousevsc_prt_msg protocol_req;
> > > > > struct mousevsc_prt_msg protocol_resp;
> > > > > /* Synchronize the request/response if needed */
> > > > > @@ -159,7 +159,7 @@ struct mousevsc_dev {
> > > > > unsigned char *report_desc;
> > > > > u32 report_desc_size;
> > > > > struct hv_input_dev_info hid_dev_info;
> > > > > - int connected;
> > > > > + bool connected;
> > > > > struct hid_device *hid_device;
> > > > > };
> > > > One of the bools should be moved to be adjacent to the other.
> > > For a mouse driver, it really doesn't matter.
> > 'course it doesn't matter, just like whitespace
> > doesn't matter. It's just done for form's sake.
> Not at all.
> Whitespace does matter, there is documented proof of that.
> The fact that you want to move a bool to a different part of a structure
> really is only your personal choice. It doesn't matter for size or
> layout to make this code work better at all to do this.
Just as what you suggest is your opinion.
Whether or not 4 or 8 extra/wasted bytes in
a struct is important or not is arguable.
I think disaggregating bools is akin to
disaggregating bitfields.
Not necessarily wrong, just odd.
cheers, Joe
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean
2011-10-26 23:50 ` Greg KH
2011-10-26 23:59 ` Joe Perches
@ 2011-10-27 0:03 ` Dmitry Torokhov
1 sibling, 0 replies; 13+ messages in thread
From: Dmitry Torokhov @ 2011-10-27 0:03 UTC (permalink / raw)
To: Greg KH
Cc: Joe Perches, Haiyang Zhang, ohering, linux-kernel, virtualization,
devel
On Thu, Oct 27, 2011 at 01:50:54AM +0200, Greg KH wrote:
> On Wed, Oct 26, 2011 at 03:46:51PM -0700, Joe Perches wrote:
> > On Wed, 2011-10-26 at 08:07 +0200, Greg KH wrote:
> > > On Tue, Oct 25, 2011 at 05:03:47PM -0700, Joe Perches wrote:
> > > > On Tue, 2011-10-25 at 17:19 -0700, K. Y. Srinivasan wrote:
> > > > > Make some state that is boolean in nature, a boolean variable.
> > > > []
> > > > > diff --git a/drivers/staging/hv/hv_mouse.c b/drivers/staging/hv/hv_mouse.c
> > > > []
> > > > @@ -148,7 +148,7 @@ struct mousevsc_prt_msg {
> > > > > */
> > > > > struct mousevsc_dev {
> > > > > struct hv_device *device;
> > > > > - unsigned char init_complete;
> > > > > + bool init_complete;
> > > > > struct mousevsc_prt_msg protocol_req;
> > > > > struct mousevsc_prt_msg protocol_resp;
> > > > > /* Synchronize the request/response if needed */
> > > > > @@ -159,7 +159,7 @@ struct mousevsc_dev {
> > > > > unsigned char *report_desc;
> > > > > u32 report_desc_size;
> > > > > struct hv_input_dev_info hid_dev_info;
> > > > > - int connected;
> > > > > + bool connected;
> > > > > struct hid_device *hid_device;
> > > > > };
> > > >
> > > > One of the bools should be moved to be adjacent to the other.
> > > For a mouse driver, it really doesn't matter.
> >
> > 'course it doesn't matter, just like whitespace
> > doesn't matter. It's just done for form's sake.
>
> Not at all.
>
> Whitespace does matter, there is documented proof of that.
>
> The fact that you want to move a bool to a different part of a structure
> really is only your personal choice. It doesn't matter for size or
> layout to make this code work better at all to do this.
It is a good practice to try minimizing footprint though. While it might
not matter much in this particular driver we should still try to produce
best code we can as it is often taken as source for new drivers.
>
> This is your personal opinion only, please don't construe it as anything
> else.
Same could be said about the email I am replying to.
--
Dmitry
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2011-10-27 0:03 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-10-26 0:18 [PATCH 0/6] Staging: hv: mousevsc: cleanup the mouse driver K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 1/6] Staging: hv: mousevsc: Make boolean states boolean K. Y. Srinivasan
2011-10-26 0:03 ` Joe Perches
2011-10-26 6:07 ` Greg KH
2011-10-26 22:46 ` Joe Perches
2011-10-26 23:50 ` Greg KH
2011-10-26 23:59 ` Joe Perches
2011-10-27 0:03 ` Dmitry Torokhov
2011-10-26 0:19 ` [PATCH 2/6] Staging: hv: mousevsc: Inline the code for mousevsc_on_device_add() K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 3/6] Staging: hv: mousevsc: Inline the code for reportdesc_callback() K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 4/6] Staging: hv: mousevsc: Cleanup mousevsc_on_channel_callback() K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 5/6] Staging: hv: mousevsc: Add a new line to a debug string K. Y. Srinivasan
2011-10-26 0:19 ` [PATCH 6/6] Staging: hv: mousevsc: Get rid of unnecessary include files K. Y. Srinivasan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).