-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Sent: Monday, September 16, 2013 8:20 AM
To: KY Srinivasan
Cc: gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
devel@linuxdriverproject.org; linux-input@vger.kernel.org; vojtech@suse.cz;
olaf@aepfle.de; apw@canonical.com; jasowang@redhat.com
Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V
synthetic keyboard

Hi K. Y.

On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:

quoted

Add a new driver to support synthetic keyboard. On the next generation
Hyper-V guest firmware, many legacy devices will not be emulated and this
driver will be required.

I would like to thank Vojtech Pavlik [off-list ref] for helping me with the
details of the AT keyboard driver.

In addition to what Dan said:

quoted

+
+struct synth_kbd_protocol_response {
+	struct synth_kbd_msg_hdr header;
+	u32 accepted:1;
+	u32 reserved:31;
+};

Use of bitfields for on the wire structures makes me uneasy. I know that
currently you only going to run LE on LE, but still, maybe using
explicit shifts and masks would be better,

This definition of the data structure is defined by the host. I will see what I
can do here.

quoted

+
+struct synth_kbd_keystroke {
+	struct synth_kbd_msg_hdr header;
+	u16 make_code;
+	u16 reserved0;
+	u32 is_unicode:1;
+	u32 is_break:1;
+	u32 is_e0:1;
+	u32 is_e1:1;
+	u32 reserved:28;
+};

Same here.

quoted

+
+
+#define HK_MAXIMUM_MESSAGE_SIZE 256
+
+#define KBD_VSC_SEND_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
+#define KBD_VSC_RECV_RING_BUFFER_SIZE		(10 * PAGE_SIZE)
+
+#define XTKBD_EMUL0     0xe0
+#define XTKBD_EMUL1     0xe1
+#define XTKBD_RELEASE   0x80
+
+
+/*
+ * Represents a keyboard device
+ */
+struct hv_kbd_dev {
+	unsigned char keycode[256];

I do not see anything using keycode table? This is wrong level for it
regardless.

quoted

+	struct hv_device	*device;
+	struct synth_kbd_protocol_request protocol_req;
+	struct synth_kbd_protocol_response protocol_resp;
+	/* Synchronize the request/response if needed */
+	struct completion	wait_event;
+	struct serio		*hv_serio;
+};
+
+
+static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
+{
+	struct hv_kbd_dev *kbd_dev;
+	struct serio	*hv_serio;

You are aligning with tabs half of declarations, leaving the others. Can
we not align at all?

quoted

+
+	kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
+
+	if (!kbd_dev)
+		return NULL;
+
+	hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
+
+	if (hv_serio == NULL) {
+		kfree(kbd_dev);
+		return NULL;
+	}
+
+	hv_serio->id.type	= SERIO_8042_XL;
+	strlcpy(hv_serio->name, dev_name(&device->device),
+		sizeof(hv_serio->name));
+	strlcpy(hv_serio->phys, dev_name(&device->device),
+		sizeof(hv_serio->phys));
+	hv_serio->dev.parent  = &device->device;

Do you also want to set up serio's parent device to point to hv device?

Is there an issue here; I could setup the parent as the vmbus device.

quoted

+
+
+	kbd_dev->device = device;
+	kbd_dev->hv_serio = hv_serio;
+	hv_set_drvdata(device, kbd_dev);
+	init_completion(&kbd_dev->wait_event);
+
+	return kbd_dev;
+}
+
+static void hv_kbd_free_device(struct hv_kbd_dev *device)
+{
+	serio_unregister_port(device->hv_serio);
+	kfree(device->hv_serio);

Serio ports are refcounted, do not free them explicitly after they have
been registered.

Thank you. I will fix this.

quoted

+	hv_set_drvdata(device->device, NULL);
+	kfree(device);
+}

Thanks.

Thank you!.

Regards,

K. Y