Hi K. Y.
On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> 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 <vojt...@suse.cz> for helping me with the
> details of the AT keyboard driver.
>
In addition to what Dan said:
> +
> +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,
> +
> +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.
> +
> +
> +#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.
> + 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?
> +
> + 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?
> +
> +
> + 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.
> + hv_set_drvdata(device->device, NULL);
> + kfree(device);
> +}
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
