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

[Nikolai Kondrashov <spbnick@gmail.com>]

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