> -----Original Message-----
> From: Dmitry Torokhov [mailto:dmitry.torok...@gmail.com]
> Sent: Monday, September 16, 2013 8:20 AM
> To: KY Srinivasan
> Cc: gre...@linuxfoundation.org; linux-kernel@vger.kernel.org;
> de...@linuxdriverproject.org; linux-in...@vger.kernel.org; vojt...@suse.cz;
> o...@aepfle.de; a...@canonical.com; jasow...@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:
> > 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,
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-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/
