Re: [RFC] HID: uhid: Handle LED input events

On Mon, 5 Nov 2012, David Herrmann wrote:

quoted

When uhid receives an input LED event, we set the proper field,
create the output report and send it to userspace as UHID_OUTPUT
event. Others input events are sent to userspace as UHID_OUTPUT_EV
events.

Signed-off-by: Andre Guedes <redacted>
---
 drivers/hid/uhid.c | 33 +++++++++++++++++++++++++++++----
 1 file changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 714cd8c..e7b7cdc 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c

@@ -122,15 +122,40 @@ static int uhid_hid_input(struct input_dev *input, unsigned int type,
        struct uhid_device *uhid = hid->driver_data;
        unsigned long flags;
        struct uhid_event *ev;
+       struct hid_field *field;
+       struct hid_report *report;
+       int offset;

        ev = kzalloc(sizeof(*ev), GFP_ATOMIC);
        if (!ev)
                return -ENOMEM;

-       ev->type = UHID_OUTPUT_EV;
-       ev->u.output_ev.type = type;
-       ev->u.output_ev.code = code;
-       ev->u.output_ev.value = value;
+       switch (type) {
+       case EV_LED:
+               offset = hidinput_find_field(hid, type, code, &field);
+               if (offset == -1) {
+                       hid_warn(input, "event field not found\n");
+                       kfree(ev);
+                       return -1;
+               }
+
+               hid_set_field(field, offset, value);
+
+               report = field->report;
+
+               ev->type = UHID_OUTPUT;
+               ev->u.output.rtype = UHID_OUTPUT_REPORT;
+               hid_output_report(report, ev->u.output.data);
+               ev->u.output.size = ((report->size - 1) >> 3) + 1 +
+                                                       (report->id > 0);
+               break;
+
+       default:
+               ev->type = UHID_OUTPUT_EV;
+               ev->u.output_ev.type = type;
+               ev->u.output_ev.code = code;
+               ev->u.output_ev.value = value;

I think kernel-coding-style uses "break" in default rules, too.

quoted

+       }

        spin_lock_irqsave(&uhid->qlock, flags);
        uhid_queue(uhid, ev);

It looks like this was copied almost verbatim from usbhid/hid-core.c
usb_hidinput_input_event() and __usbhid_submit_report() so I wonder
why we do not provide a generic helper in hid-input.c.

The code itself looks good, so how about pushing this into a helper in
hid-input.c and using hid_output_raw_report(). We use this helper if
hidinput_input_event is NULL and hid_output_raw_report() is non-NULL.
Maybe Jiri has some comments on that, too. Adding him to CC.

Fully agreed, thanks.

Andre, I will happily merge the patch if you resend it with the suggested 
changes applied.

Thanks,

-- 
Jiri Kosina
SUSE Labs