Re: [PATCH 1/2] HID: huion: enable button mode reporting

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

  /* 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.

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.

+	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?

  	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.

+	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.

+			data[0] = 0x08;
+	}
+
  	return 0;
  }



Nick
--
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




[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux