Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753435AbbBRUYz (ORCPT ); Wed, 18 Feb 2015 15:24:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:56052 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753175AbbBRUYx (ORCPT ); Wed, 18 Feb 2015 15:24:53 -0500 Date: Wed, 18 Feb 2015 15:24:47 -0500 From: Benjamin Tissoires To: Nikolai Kondrashov Cc: Jiri Kosina , Peter Hutterer , linux-input@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/2] HID: huion: enable button mode reporting Message-ID: <20150218202447.GD5928@mail.corp.redhat.com> References: <1424213653-5970-1-git-send-email-benjamin.tissoires@redhat.com> <1424213653-5970-2-git-send-email-benjamin.tissoires@redhat.com> <54E4656F.4070708@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <54E4656F.4070708@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8815 Lines: 205 On Feb 18 2015 or thereabouts, Nikolai Kondrashov wrote: > On 02/18/2015 12:54 AM, Benjamin Tissoires wrote: > >diff --git a/drivers/hid/hid-huion.c b/drivers/hid/hid-huion.c > >index 61b68ca..50fbda4 100644 > >--- a/drivers/hid/hid-huion.c > >+++ b/drivers/hid/hid-huion.c > >@@ -34,6 +34,9 @@ enum huion_ph_id { > > HUION_PH_ID_NUM > > }; > > > >+/* header of a button report sent through the Pen report */ > >+static const u8 button_report[] = {0x07, 0xa0, 0x01, 0x01}; > > Hmm, I see the second byte being 0xe0 on Huion H610, the rest is the same. > Considering this, the fact that bit 7 is always 1 and bit 6 is pen proximity, > I think we can assume that bit 5 in byte 2 indicates button reports and get > away with just a "data[1] & 0x20" test. that would be a nicer approach. Thanks for the analysis. Actually, I understood the difference. I tested the bits _after_ the driver reverts the in-range bit :) The raw data is {0x07, 0xe0, 0x01, 0x01} on the H610 Pro too. > > > /* Report descriptor template placeholder */ > > #define HUION_PH(_ID) HUION_PH_HEAD, HUION_PH_ID_##_ID > > > >@@ -81,6 +84,31 @@ static const __u8 huion_tablet_rdesc_template[] = { > > HUION_PH(PRESSURE_LM), /* Logical Maximum (PLACEHOLDER), */ > > 0x81, 0x02, /* Input (Variable), */ > > 0xC0, /* End Collection, */ > >+ 0x05, 0x01, /* Usage Page (Desktop) */ > >+ 0x09, 0x07, /* Usage (Keypad) */ > >+ 0xa1, 0x01, /* Collection (Application) */ > >+ 0x85, 0x08, /* Report ID (8) */ > >+ 0x05, 0x0d, /* Usage Page (Digitizers) */ > >+ 0x09, 0x22, /* Usage (Finger) */ > > I'd say "Finger" usage is wrong here. The spec says: > > Finger > > CL – Any human appendage used as a transducer, such as a finger > touching a touch screen to set the location of the screen cursor. A > digitizer typically reports the coordinates of center of the finger. > In the Finger collection a Pointer physical collection will contain > the axes reported by the finger. > > I.e. the buttons are not a pointing device. The specification contains another > collection usage which seems more suitable: > > Tablet Function Keys > > CL – These controls are located on the surface of a digitizing tablet, > and may be implemented as actual switches, or as soft keys actuated by > the digitizing transducer. These are often used to trigger > location-independent macros or other events. Actually, the kernel knows about it: HID_DG_TABLETFUNCTIONKEY. I don't think it should influence to have it set. The hid processing would work on the BTN usages, not on the physical. [5 min later] yep, just works :) > > However the kernel doesn't seem to know anything about it (but we can fix > that). In my version of this I simply used a keyboard with buttons: > > 0x05, 0x01, /* Usage Page (Desktop), */ > 0x09, 0x06, /* Usage (Keyboard), */ > 0xA1, 0x01, /* Collection (Application), */ > 0x85, 0xF7, /* Report ID (247), */ > 0x05, 0x09, /* Usage Page (Button), */ > 0x75, 0x01, /* Report Size (1), */ > 0x95, 0x18, /* Report Count (24), */ > 0x81, 0x03, /* Input (Constant, Variable), */ > 0x19, 0x01, /* Usage Minimum (01h), */ > 0x29, 0x08, /* Usage Maximum (08h), */ > 0x95, 0x08, /* Report Count (8), */ > 0x81, 0x02, /* Input (Variable), */ > 0xC0 /* End Collection */ > > Although it might not be entirely correct either. Even if no-one but hid-core uses the report descriptor, I would rather not declare ourself as a keyboard. It's shooting on our own foot if someone decides to actually merge a keyboard and a tablet. > > >+ 0xa0, /* Collection (Physical) */ > >+ 0x14, /* Logical Minimum (0) */ > >+ 0x25, 0x01, /* Logical Maximum (1) */ > >+ 0x75, 0x08, /* Report Size (8) */ > >+ 0x95, 0x03, /* Report Count (3) */ > >+ 0x81, 0x03, /* Input (Cnst,Var,Abs) */ > >+ 0x05, 0x09, /* Usage Page (Button) */ > >+ 0x19, 0x01, /* Usage Minimum (1) */ > >+ 0x29, 0x08, /* Usage Maximum (8) */ > >+ 0x14, /* Logical Minimum (0) */ > >+ 0x25, 0x01, /* Logical Maximum (1) */ > >+ 0x75, 0x01, /* Report Size (1) */ > >+ 0x95, 0x08, /* Report Count (8) */ > >+ 0x81, 0x02, /* Input (Data,Var,Abs) */ > >+ 0x75, 0x08, /* Report Size (8) */ > >+ 0x95, 0x03, /* Report Count (3) */ > >+ 0x81, 0x03, /* Input (Cnst,Var,Abs) */ > >+ 0xc0, /* End Collection */ > >+ 0xc0, /* End Collection */ > > Which tool did you use to generate this? My own custom-made: https://github.com/bentiss/hid-replay/blob/master/tools/editor.py not 100% implemented, but it works for me :) > > > 0xC0 /* End Collection */ > > }; > > > >@@ -205,6 +233,25 @@ static int huion_tablet_enable(struct hid_device *hdev) > > } > > } > > > >+ /* switch to the button mode reporting */ > >+ rc = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0), > >+ USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, > >+ (USB_DT_STRING << 8) + 0x7b, > >+ 0x0409, buf, len, > >+ USB_CTRL_GET_TIMEOUT); > > I'm a bit uncomfortable about reusing a buffer which was sized specifically > for another task, as it's confusing. But it will work as is, so it's OK. Yes, and no :) Actually, I would prefer that we stick to what the Windows driver do. But it requests 32 bytes in each requests, and we receive 14 and 22 IIRC. The trick I exploited here is that the ctrl message answers back at most len data, so we are find in both cases because 12 is less than 14 and 22. I am not sure we should check at all the length of the returning buffer (though for the first command, we have to be sure that we received enough to get the values in the buffer). Side note: the huion-abstract-keyboard branch uses usb_string() instead of a plain usb_control_msg(). I like this much better and I think we should change the first call with that. This way, it will be clear that the tablet is not fully HID compatible and that we need to keep the usb access. > > >+ if (rc == -EPIPE) { > >+ hid_err(hdev, "button mode switch not found\n"); > >+ rc = -ENODEV; > >+ goto cleanup; > >+ } else if (rc < 0) { > >+ hid_err(hdev, "failed to switch to button mode: %d\n", rc); > >+ rc = -ENODEV; > >+ goto cleanup; > >+ } else if (rc != len) { > >+ hid_err(hdev, "invalid button mode switch\n"); > >+ rc = -ENODEV; > >+ goto cleanup; > >+ } > > rc = 0; > > > > cleanup: > >@@ -262,10 +309,16 @@ static int huion_raw_event(struct hid_device *hdev, struct hid_report *report, > > /* If this is a pen input report */ > > if (intf->cur_altsetting->desc.bInterfaceNumber == 0 && > > report->type == HID_INPUT_REPORT && > >- report->id == 0x07 && size >= 2) > >+ report->id == 0x07 && size >= 2) { > > /* Invert the in-range bit */ > > data[1] ^= 0x40; > > > >+ /* check for buttons events and change the report ID */ > >+ if (size >= sizeof(button_report) && > >+ !memcmp(data, button_report, sizeof(button_report))) > > So, yes, I think it's better to have a "data[1] & 0x20" test here instead. Yep, works just fine. Cheers, Benjamin > > >+ data[0] = 0x08; > >+ } > >+ > > return 0; > > } > > > > > > Nick -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/