lkml.org 
[lkml]   [2017]   [Jan]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH 13/17] HID: logitech-hidpp: make .probe usbhid capable
    On Tue, Jan 17, 2017 at 3:35 PM, Benjamin Tissoires
    <benjamin.tissoires@redhat.com> wrote:
    > The current custom solution for the G920 is not the best because
    > hid_hw_start() is not called at the end of the .probe().
    > It means that any configuration retrieved after the initial hid_hw_start
    > would not be exposed to user space without races.
    >
    > We can simply force hid_hw_start to just enable the transport layer by
    > not using a connect_mask. This way, we can have a common path between
    > USB, Unifying and Bluetooth devices.
    >
    > Tested with a G502 (plain USB), a T650 and many other unifying devices,
    > and the T651 over Bluetooth.
    >
    > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
    > ---
    > drivers/hid/hid-logitech-hidpp.c | 88 ++++++++++++++++++++++------------------
    > 1 file changed, 48 insertions(+), 40 deletions(-)
    >
    > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
    > index a5d37a4..f5889ff 100644
    > --- a/drivers/hid/hid-logitech-hidpp.c
    > +++ b/drivers/hid/hid-logitech-hidpp.c
    > @@ -2813,6 +2813,17 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
    > if (!hidpp_validate_device(hdev))
    > return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
    >
    > + /*
    > + * HID++ needs to read incoming report while in .probe().
    > + * However, this doesn't work for plain USB devices until the hdev
    > + * status is set with HID_STAT_ADDED (device fully registered once
    > + * with HID).
    > + * So we ask for it to be reprobed later.
    > + */
    > + if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE &&
    > + !(hdev->status & HID_STAT_ADDED))

    Looks like this test breaks the T651 (bluetooth) after all. I seem to
    have better success with:
    if (id->bus == BUS_USB && !(hdev->status & HID_STAT_ADDED))

    But that also means that the solution will not work if there is only
    one USB interface in the device :/

    Cheers,
    Benjamin

    > + return -EPROBE_DEFER;
    > +
    > hidpp = devm_kzalloc(&hdev->dev, sizeof(struct hidpp_device),
    > GFP_KERNEL);
    > if (!hidpp)
    > @@ -2853,24 +2864,23 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
    > hid_warn(hdev, "Cannot allocate sysfs group for %s\n",
    > hdev->name);
    >
    > - if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
    > - connect_mask &= ~HID_CONNECT_HIDINPUT;
    > -
    > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
    > - ret = hid_hw_start(hdev, connect_mask);
    > - if (ret) {
    > - hid_err(hdev, "hw start failed\n");
    > - goto hid_hw_start_fail;
    > - }
    > - ret = hid_hw_open(hdev);
    > - if (ret < 0) {
    > - dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
    > - __func__, ret);
    > - hid_hw_stop(hdev);
    > - goto hid_hw_start_fail;
    > - }
    > + /*
    > + * Plain USB connections need to actually call start and open
    > + * on the transport driver to allow incoming data.
    > + */
    > + ret = hid_hw_start(hdev, 0);
    > + if (ret) {
    > + hid_err(hdev, "hw start failed\n");
    > + goto hid_hw_start_fail;
    > }
    >
    > + ret = hid_hw_open(hdev);
    > + if (ret < 0) {
    > + dev_err(&hdev->dev, "%s:hid_hw_open returned error:%d\n",
    > + __func__, ret);
    > + hid_hw_stop(hdev);
    > + goto hid_hw_open_fail;
    > + }
    >
    > /* Allow incoming packets */
    > hid_device_io_start(hdev);
    > @@ -2880,7 +2890,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
    > if (!connected) {
    > ret = -ENODEV;
    > hid_err(hdev, "Device not connected");
    > - goto hid_hw_open_failed;
    > + goto hid_hw_init_fail;
    > }
    >
    > hid_info(hdev, "HID++ %u.%u device connected.\n",
    > @@ -2893,37 +2903,36 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
    > if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)) {
    > ret = wtp_get_config(hidpp);
    > if (ret)
    > - goto hid_hw_open_failed;
    > + goto hid_hw_init_fail;
    > } else if (connected && (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
    > ret = g920_get_config(hidpp);
    > if (ret)
    > - goto hid_hw_open_failed;
    > + goto hid_hw_init_fail;
    > }
    >
    > - /* Block incoming packets */
    > - hid_device_io_stop(hdev);
    > + hidpp_connect_event(hidpp);
    >
    > - if (!(hidpp->quirks & HIDPP_QUIRK_CLASS_G920)) {
    > - ret = hid_hw_start(hdev, connect_mask);
    > - if (ret) {
    > - hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
    > - goto hid_hw_start_fail;
    > - }
    > - }
    > + /* Reset the HID node state */
    > + hid_device_io_stop(hdev);
    > + hid_hw_close(hdev);
    > + hid_hw_stop(hdev);
    >
    > - /* Allow incoming packets */
    > - hid_device_io_start(hdev);
    > + if (hidpp->quirks & HIDPP_QUIRK_NO_HIDINPUT)
    > + connect_mask &= ~HID_CONNECT_HIDINPUT;
    >
    > - hidpp_connect_event(hidpp);
    > + /* Now export the actual inputs and hidraw nodes to the world */
    > + ret = hid_hw_start(hdev, connect_mask);
    > + if (ret) {
    > + hid_err(hdev, "%s:hid_hw_start returned error\n", __func__);
    > + goto hid_hw_start_fail;
    > + }
    >
    > return ret;
    >
    > -hid_hw_open_failed:
    > - hid_device_io_stop(hdev);
    > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
    > - hid_hw_close(hdev);
    > - hid_hw_stop(hdev);
    > - }
    > +hid_hw_init_fail:
    > + hid_hw_close(hdev);
    > +hid_hw_open_fail:
    > + hid_hw_stop(hdev);
    > hid_hw_start_fail:
    > sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
    > cancel_work_sync(&hidpp->work);
    > @@ -2942,10 +2951,9 @@ static void hidpp_remove(struct hid_device *hdev)
    >
    > sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
    >
    > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920) {
    > + if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
    > hidpp_ff_deinit(hdev);
    > - hid_hw_close(hdev);
    > - }
    > +
    > hid_hw_stop(hdev);
    > cancel_work_sync(&hidpp->work);
    > mutex_destroy(&hidpp->send_mutex);
    > --
    > 2.9.3
    >

    \
     
     \ /
      Last update: 2017-01-18 10:27    [W:2.156 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site