lkml.org 
[lkml]   [2014]   [Feb]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 03/14] HID: core: implement generic .request()
    From
    On Wed, Feb 12, 2014 at 5:25 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
    > Hi
    >
    > On Mon, Feb 10, 2014 at 6:58 PM, Benjamin Tissoires
    > <benjamin.tissoires@redhat.com> wrote:
    >> .request() can be emulated through .raw_request()
    >> we can implement this emulation in hid-core, and make .request
    >> not mandatory for transport layer drivers.
    >>
    >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
    >> ---
    >> drivers/hid/hid-core.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
    >> include/linux/hid.h | 5 ++++-
    >> 2 files changed, 48 insertions(+), 2 deletions(-)
    >>
    >> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
    >> index 18fe49b..f36778a 100644
    >> --- a/drivers/hid/hid-core.c
    >> +++ b/drivers/hid/hid-core.c
    >> @@ -1248,6 +1248,11 @@ void hid_output_report(struct hid_report *report, __u8 *data)
    >> }
    >> EXPORT_SYMBOL_GPL(hid_output_report);
    >>
    >> +static int hid_report_len(struct hid_report *report)
    >> +{
    >> + return ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
    >
    > Just for clarity, this is equivalent to the following, right?
    >
    > return DIV_ROUND_UP(report->size, 8) + !!(report->id > 0) + 7;

    yes, it should (at least that's what I understand too :)

    >
    > I always have to read that shifting code twice to get it.. Maybe add a
    > comment explaining the different entries here.

    good idea.

    >
    >> +}
    >> +
    >> /*
    >> * Allocator for buffer that is going to be passed to hid_output_report()
    >> */
    >> @@ -1258,7 +1263,7 @@ u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags)
    >> * of implement() working on 8 byte chunks
    >> */
    >>
    >> - int len = ((report->size - 1) >> 3) + 1 + (report->id > 0) + 7;
    >> + int len = hid_report_len(report);
    >>
    >> return kmalloc(len, flags);
    >> }
    >> @@ -1314,6 +1319,44 @@ static struct hid_report *hid_get_report(struct hid_report_enum *report_enum,
    >> return report;
    >> }
    >>
    >> +/*
    >> + * Implement a generic .request() callback, using .raw_request()
    >> + * DO NOT USE in hid drivers directly, but through hid_hw_request instead.
    >> + */
    >> +void __hid_request(struct hid_device *hid, struct hid_report *report,
    >> + int reqtype)
    >> +{
    >> + char *buf;
    >> + int ret;
    >> + int len;
    >> +
    >> + if (!hid->ll_driver->raw_request)
    >> + return;
    >> +
    >> + buf = hid_alloc_report_buf(report, GFP_KERNEL);
    >> + if (!buf)
    >> + return;
    >> +
    >> + len = hid_report_len(report);

    actually, after sending the patches, I was wondering if we should use
    the +7 in hid_report_len.
    "len" is used in .raw_request(), and the +7 was only for the implement(), right?

    So maybe a device can reject this because the size of the report is too big...

    Jiri, David, any ideas?

    Cheers,
    Benjamin

    >> +
    >> + if (reqtype == HID_REQ_SET_REPORT)
    >> + hid_output_report(report, buf);
    >> +
    >> + ret = hid->ll_driver->raw_request(hid, report->id, buf, len,
    >> + report->type, reqtype);
    >> + if (ret < 0) {
    >> + dbg_hid("unable to complete request: %d\n", ret);
    >> + goto out;
    >> + }
    >> +
    >> + if (reqtype == HID_REQ_GET_REPORT)
    >> + hid_input_report(hid, report->type, buf, ret, 0);
    >> +
    >> +out:
    >> + kfree(buf);
    >> +}
    >> +EXPORT_SYMBOL_GPL(__hid_request);
    >> +
    >
    > Looks good to me.
    >
    > Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
    >
    > Thanks
    > David
    >
    >> int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
    >> int interrupt)
    >> {
    >> diff --git a/include/linux/hid.h b/include/linux/hid.h
    >> index a837ede..09fbbd7 100644
    >> --- a/include/linux/hid.h
    >> +++ b/include/linux/hid.h
    >> @@ -753,6 +753,7 @@ struct hid_field *hidinput_get_led_field(struct hid_device *hid);
    >> unsigned int hidinput_count_leds(struct hid_device *hid);
    >> __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
    >> void hid_output_report(struct hid_report *report, __u8 *data);
    >> +void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
    >> u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
    >> struct hid_device *hid_allocate_device(void);
    >> struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
    >> @@ -965,7 +966,9 @@ static inline void hid_hw_request(struct hid_device *hdev,
    >> struct hid_report *report, int reqtype)
    >> {
    >> if (hdev->ll_driver->request)
    >> - hdev->ll_driver->request(hdev, report, reqtype);
    >> + return hdev->ll_driver->request(hdev, report, reqtype);
    >> +
    >> + __hid_request(hdev, report, reqtype);
    >> }
    >>
    >> /**
    >> --
    >> 1.8.3.1
    >>


    \
     
     \ /
      Last update: 2014-02-13 17:01    [W:4.148 / U:0.204 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site