Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path

From: Benjamin Tissoires <benjamin.tissoires@gmail.com>
To: Henrik Rydberg <rydberg@euromail.se>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Jiri Kosina <jkosina@suse.cz>, Stephane Chatty <chatty@enac.fr>,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 11/11] HID: hid-multitouch: get rid of usbhid depedency for general path
Date: Tue, 30 Oct 2012 12:04:40 +0100	[thread overview]
Message-ID: <CAN+gG=GNucfg4-wCBZQtz0yJDjpTD_56Js3d68nm8VFi1UhZAA@mail.gmail.com> (raw)
In-Reply-To: <20121029225748.GA15632@polaris.bitmath.org>

Hi Henrik,

On Mon, Oct 29, 2012 at 11:57 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
>> This patch factorizes the hid set_feature command by using
>> hid_device->hid_output_raw_report instead of direclty relying on
>> usbhid. This makes the driver usb independant.
>>
>> However I still can't remove the 2 usb related headers because the
>> function mt_resume has a specific patch for usb devices.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/hid-multitouch.c | 63 ++++++++++++++++++++++++++------------------
>>  1 file changed, 37 insertions(+), 26 deletions(-)
>
> In my drawer, I have a patchset that aims to remove all usbhid
> dependence, from all the drivers. Perhaps the attached patch is
> something to consider here?

yep, removing usbhid dependencies is a good thing.
See my review below :)

>
>>
>> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
>> index 21a120b..33038c5 100644
>> --- a/drivers/hid/hid-multitouch.c
>> +++ b/drivers/hid/hid-multitouch.c
>> @@ -670,47 +670,58 @@ static int mt_event(struct hid_device *hid, struct hid_field *field,
>>       return 1;
>>  }
>>
>> -static void mt_set_input_mode(struct hid_device *hdev)
>> +static void mt_set_feature(struct hid_device *hdev, __s8 feature_id,
>> +     u8 value, size_t index)
>>  {
>> -     struct mt_device *td = hid_get_drvdata(hdev);
>>       struct hid_report *r;
>>       struct hid_report_enum *re;
>> +     u8 *data;
>> +     u8 max_value;
>> +     int len;
>> +
>> +     if (feature_id < 0 || !hdev->hid_output_raw_report)
>> +             return;
>> +
>> +     re = &hdev->report_enum[HID_FEATURE_REPORT];
>> +     r = re->report_id_hash[feature_id];
>> +     if (!r)
>> +             return;
>> +
>> +     len = ((r->size - 1) >> 3) + 1 + (r->id > 0);
>> +     max_value = r->field[0]->logical_maximum;
>> +     value = min(value, max_value);
>>
>> -     if (td->inputmode < 0)
>> +     if (r->field[0]->value[index] == value || len < 2 || index + 1 >= len)
>>               return;
>>
>> -     re = &(hdev->report_enum[HID_FEATURE_REPORT]);
>> -     r = re->report_id_hash[td->inputmode];
>> -     if (r) {
>> -             r->field[0]->value[td->inputmode_index] = 0x02;
>> -             usbhid_submit_report(hdev, r, USB_DIR_OUT);
>> +     data = kzalloc(len, GFP_ATOMIC);
>> +     if (!data) {
>> +             hid_warn(hdev, "output queueing failed\n");
>> +             return;
>>       }
>> +
>> +     data[0] = r->id;
>> +     data[index + 1] = value;
>> +     hdev->hid_output_raw_report(hdev, data, len, HID_FEATURE_REPORT);
>> +     kfree(data);
>>  }
>>
>> -static void mt_set_maxcontacts(struct hid_device *hdev)
>> +static void mt_set_input_mode(struct hid_device *hdev)
>>  {
>>       struct mt_device *td = hid_get_drvdata(hdev);
>> -     struct hid_report *r;
>> -     struct hid_report_enum *re;
>> -     int fieldmax, max;
>>
>> -     if (td->maxcontact_report_id < 0)
>> -             return;
>> +     mt_set_feature(hdev, td->inputmode, 0x02, td->inputmode_index);
>> +}
>>
>> -     if (!td->mtclass.maxcontacts)
>> +static void mt_set_maxcontacts(struct hid_device *hdev)
>> +{
>> +     struct mt_device *td = hid_get_drvdata(hdev);
>> +     int max = td->mtclass.maxcontacts;
>> +
>> +     if (!max)
>>               return;
>>
>> -     re = &hdev->report_enum[HID_FEATURE_REPORT];
>> -     r = re->report_id_hash[td->maxcontact_report_id];
>> -     if (r) {
>> -             max = td->mtclass.maxcontacts;
>> -             fieldmax = r->field[0]->logical_maximum;
>> -             max = min(fieldmax, max);
>> -             if (r->field[0]->value[0] != max) {
>> -                     r->field[0]->value[0] = max;
>> -                     usbhid_submit_report(hdev, r, USB_DIR_OUT);
>> -             }
>> -     }
>> +     mt_set_feature(hdev, td->maxcontact_report_id, max, 0);
>>  }
>>
>>  static void mt_post_parse_default_settings(struct mt_device *td)
>> --
>> 1.7.11.7
>>
>
> Thanks,
> Henrik
>
> ---
>
> From 5d9a791e0a9e41bcea0fcb286e2849b403675f37 Mon Sep 17 00:00:00 2001
> From: Henrik Rydberg <rydberg@euromail.se>
> Date: Mon, 2 Jul 2012 20:38:21 +0200
> Subject: [PATCH 3/7] hid: extend interface with report requests
>
> ---
>  drivers/hid/usbhid/hid-core.c | 14 ++++++++++++++
>  include/linux/hid.h           | 33 +++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index de1f9ac..3c618da 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1251,6 +1251,18 @@ static int usbhid_power(struct hid_device *hid, int lvl)
>         return r;
>  }
>
> +static void usbhid_send(struct hid_device *hid, struct hid_report *rep, int req)
> +{
> +       switch (req) {
> +       case HID_REQ_GET_REPORT:
> +               usbhid_submit_report(hid, rep, USB_DIR_IN);
> +               break;
> +       case HID_REQ_SET_REPORT:
> +               usbhid_submit_report(hid, rep, USB_DIR_OUT);
> +               break;
> +       }
> +}
> +
>  static struct hid_ll_driver usb_hid_driver = {
>         .parse = usbhid_parse,
>         .start = usbhid_start,
> @@ -1259,6 +1271,8 @@ static struct hid_ll_driver usb_hid_driver = {
>         .close = usbhid_close,
>         .power = usbhid_power,
>         .hidinput_input_event = usb_hidinput_input_event,
> +       .send = usbhid_send,

the name here is a little bit misleading. You are using "send" to also
"get" reports...
Maybe "hid_request" is a better name. This will allows us to add the
missing function:
Get_Descriptor, Set_Descriptor, Get_Report Request, Set_Report
Request, Get_Idle, Set_Idle, Get_Protocol, Set_Protocol and maybe
others - even WAIT for instance :)

> +       .wait = usbhid_wait_io,

is it really necessary to wait for IO? (I think it will not be one
line for hid over i2c...).

Cheers,
Benjamin

>  };
>
>  static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *id)
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 241eb40..5e169c1 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -686,6 +686,8 @@ struct hid_driver {
>   * @hidinput_input_event: event input event (e.g. ff or leds)
>   * @parse: this method is called only once to parse the device data,
>   *        shouldn't allocate anything to not leak memory
> + * @send: send report request to device (e.g. feature report)
> + * @wait: wait for buffered io to complete (send/recv reports)
>   */
>  struct hid_ll_driver {
>         int (*start)(struct hid_device *hdev);
> @@ -700,6 +702,11 @@ struct hid_ll_driver {
>                         unsigned int code, int value);
>
>         int (*parse)(struct hid_device *hdev);
> +
> +       void (*send)(struct hid_device *hdev,
> +                    struct hid_report *report, int req);
> +       int (*wait)(struct hid_device *hdev);
> +
>  };
>
>  #define        PM_HINT_FULLON  1<<5
> @@ -892,6 +899,32 @@ static inline int hid_hw_power(struct hid_device *hdev, int level)
>         return hdev->ll_driver->power ? hdev->ll_driver->power(hdev, level) : 0;
>  }
>
> +
> +/**
> + * hid_hw_send - send report request to device
> + *
> + * @hdev: hid device
> + * @report: report to send
> + * @req: hid request type
> + */
> +static inline void hid_hw_send(struct hid_device *hdev,
> +                              struct hid_report *report, int req)
> +{
> +       if (hdev->ll_driver->send)
> +               hdev->ll_driver->send(hdev, report, req);
> +}
> +
> +/**
> + * hid_hw_wait - wait for buffered io to complete
> + *
> + * @hdev: hid device
> + */
> +static inline void hid_hw_wait(struct hid_device *hdev)
> +{
> +       if (hdev->ll_driver->wait)
> +               hdev->ll_driver->wait(hdev);
> +}
> +
>  int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, int size,
>                 int interrupt);
>
> --
> 1.7.11.1
>