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

[Andre Guedes <andre.guedes@openbossa.org>]

Hi David,

On Mon, Nov 5, 2012 at 9:58 AM, David Herrmann
<dh.herrmann@googlemail.com> wrote:
> Hi Andre
>
> On Thu, Nov 1, 2012 at 10:55 PM, Andre Guedes
> <andre.guedes@openbossa.org> wrote:
>> This patches adds support for handling LED input events in uhid
>> driver.
>>
>> 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 <andre.guedes@openbossa.org>
>> ---
>>  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.

Ok, I'll check this.

>> +       }
>>
>>         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.

I can work on that and send a new version. Lets see for Jiri have comments so.

Regards,

Andre