Hi
On Tue, Jul 16, 2013 at 10:04 AM, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxx> wrote:
> On Mon, Jul 15, 2013 at 7:10 PM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote:
>> The hidinput_input_event() callback converts input events written from
>> userspace into HID reports and sends them to the device. We currently
>> implement this in every HID transport driver, even though most of them do
>> the same.
>>
>> This provides a generic hidinput_input_event() implementation which is
>> mostly copied from usbhid. It uses a delayed worker to allow multiple LED
>> events to be collected into a single output event.
>> We use the custom ->request() transport driver callback to allow drivers
>> to adjust the outgoing report and handle the request asynchronously. If no
>> custom ->request() callback is available, we fall back to the generic raw
>> output report handler (which is synchronous).
>>
>> Drivers can still provide custom hidinput_input_event() handlers (see
>> logitech-dj) if the generic implementation doesn't fit their needs.
>>
>> Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx>
>> ---
>> drivers/hid/hid-input.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++-
>> include/linux/hid.h | 1 +
>> 2 files changed, 80 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
>> index 7480799..308eee8 100644
>> --- a/drivers/hid/hid-input.c
>> +++ b/drivers/hid/hid-input.c
>> @@ -1137,6 +1137,74 @@ unsigned int hidinput_count_leds(struct hid_device *hid)
>> }
>> EXPORT_SYMBOL_GPL(hidinput_count_leds);
>>
>> +static void hidinput_led_worker(struct work_struct *work)
>> +{
>> + struct hid_device *hid = container_of(work, struct hid_device,
>> + led_work);
>> + struct hid_field *field;
>> + struct hid_report *report;
>> + int len;
>> + __u8 *buf;
>> +
>> + field = hidinput_get_led_field(hid);
>> + if (!field)
>> + return;
>> +
>> + /*
>> + * field->report is accessed unlocked regarding HID core. So there might
>> + * be another incoming SET-LED request from user-space, which changes
>> + * the LED state while we assemble our outgoing buffer. However, this
>> + * doesn't matter as hid_output_report() correctly converts it into a
>> + * boolean value no matter what information is currently set on the LED
>> + * field (even garbage). So the remote device will always get a valid
>> + * request.
>> + * And in case we send a wrong value, a next led worker is spawned
>> + * for every SET-LED request so the following worker will send the
>> + * correct value, guaranteed!
>> + */
>> +
>> + report = field->report;
>> +
>> + /* use custom SET_REPORT request if possible (asynchronous) */
>> + if (hid->ll_driver->request)
>> + return hid->ll_driver->request(hid, report, HID_REQ_SET_REPORT);
>> +
>> + /* fall back to generic raw-output-report */
>> + len = ((report->size - 1) >> 3) + 1 + (report->id > 0);
>> + buf = kmalloc(len, GFP_KERNEL);
>> + if (!buf)
>> + return;
>> +
>> + hid_output_report(report, buf);
>> + /* synchronous output report */
>> + hid->hid_output_raw_report(hid, buf, len, HID_OUTPUT_REPORT);
>> + kfree(buf);
>> +}
>
> Instead of writing a specific fallback in case hid->ll_driver->request
> does not exist, I think it would make sense to implement a generic
> hid_hw_request function in hid-input, so that HIDP and UHID will
> benefit from it. I think it will be better because the implementation
> I made in i2c-hid.c is nearly the exact same calls than the fallback
> here.
Yepp, that sounds about right. However, with the current
hid_output_raw_report() callbacks we cannot implement this as they
work differently on each transport driver. That's why I introduced the
raw_request() and output_report() helpers. These will allow me to do
that.
So Jiri, feel free to drop this patch and I will rebase it on the new
series at the end with the new helpers. Otherwise, I will add a patch
at the end which provides a generic fallback for request().
>> +
>> +static int hidinput_input_event(struct input_dev *dev, unsigned int type,
>> + unsigned int code, int value)
>> +{
>> + struct hid_device *hid = input_get_drvdata(dev);
>> + struct hid_field *field;
>> + int offset;
>> +
>> + if (type == EV_FF)
>> + return input_ff_event(dev, type, code, value);
>> +
>> + if (type != EV_LED)
>> + return -1;
>> +
>> + if ((offset = hidinput_find_field(hid, type, code, &field)) == -1) {
>> + hid_warn(dev, "event field not found\n");
>> + return -1;
>> + }
>> +
>> + hid_set_field(field, offset, value);
>> +
>> + schedule_work(&hid->led_work);
>> + return 0;
>> +}
>> +
>> static int hidinput_open(struct input_dev *dev)
>> {
>> struct hid_device *hid = input_get_drvdata(dev);
>> @@ -1183,7 +1251,10 @@ static struct hid_input *hidinput_allocate(struct hid_device *hid)
>> }
>>
>> input_set_drvdata(input_dev, hid);
>> - input_dev->event = hid->ll_driver->hidinput_input_event;
>> + if (hid->ll_driver->hidinput_input_event)
>> + input_dev->event = hid->ll_driver->hidinput_input_event;
>> + else if (hid->ll_driver->request || hid->hid_output_raw_report)
>> + input_dev->event = hidinput_input_event;
>
> with a generic hid_hw_request in hid-input, the else is simpler here.
>
>> input_dev->open = hidinput_open;
>> input_dev->close = hidinput_close;
>> input_dev->setkeycode = hidinput_setkeycode;
>> @@ -1278,6 +1349,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
>> int i, j, k;
>>
>> INIT_LIST_HEAD(&hid->inputs);
>> + INIT_WORK(&hid->led_work, hidinput_led_worker);
>>
>> if (!force) {
>> for (i = 0; i < hid->maxcollection; i++) {
>> @@ -1379,6 +1451,12 @@ void hidinput_disconnect(struct hid_device *hid)
>> input_unregister_device(hidinput->input);
>> kfree(hidinput);
>> }
>> +
>> + /* led_work is spawned by input_dev callbacks, but doesn't access the
>> + * parent input_dev at all. Once all input devices are removed, we
>> + * know that led_work will never get restarted, so we can cancel it
>> + * synchronously and are safe. */
>> + cancel_work_sync(&hid->led_work);
>
> You missed the multi-lines comment formatting style on this one :)
The ./net/ subsystem uses these comments quite a lot and there was a
discussion between davem and linus with the conclusion that these
comments are ok. But I actually don't care, so I can change to normal
CodingStyle.
Thanks for reviewing!
David
>> }
>> EXPORT_SYMBOL_GPL(hidinput_disconnect);
>>
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index b8058c5..ea4b828 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -456,6 +456,7 @@ struct hid_device { /* device report descriptor */
>> enum hid_type type; /* device type (mouse, kbd, ...) */
>> unsigned country; /* HID country */
>> struct hid_report_enum report_enum[HID_REPORT_TYPES];
>> + struct work_struct led_work; /* delayed LED worker */
>>
>> struct semaphore driver_lock; /* protects the current driver, except during input */
>> struct semaphore driver_input_lock; /* protects the current driver */
>> --
>> 1.8.3.2
>>
>
> Cheers,
> Benjamin
--
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
