> -----Original Message----- > From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] > Sent: Monday, September 16, 2013 8:20 AM > To: KY Srinivasan > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx; vojtech@xxxxxxx; > olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx; jasowang@xxxxxxxxxx > 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: > > 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 <vojtech@xxxxxxx> 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, This definition of the data structure is defined by the host. I will see what I can do here. > > > + > > +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? Is there an issue here; I could setup the parent as the vmbus 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. Thank you. I will fix this. > > > + hv_set_drvdata(device->device, NULL); > > + kfree(device); > > +} > > > Thanks. Thank you!. Regards, K. Y -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
