Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | Date | Wed, 8 Dec 2021 17:33:47 +0100 | Subject | Re: [RFCv3 3/7] HID: core: Add support for USI style events | From | Benjamin Tissoires <> |
| |
On 12/8/21 16:18, Benjamin Tissoires wrote: > On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@linux.intel.com> wrote: >> >> Add support for Universal Stylus Interface (USI) style events to the HID >> core and input layers. >> >> Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com> >> --- >> drivers/hid/hid-input.c | 18 ++++++++++++++++++ >> include/linux/mod_devicetable.h | 2 +- >> include/uapi/linux/hid.h | 10 ++++++++++ >> include/uapi/linux/input-event-codes.h | 22 ++++++++++++++-------- >> 4 files changed, 43 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c >> index 73c2edda742e..b428ee9b4d9b 100644 >> --- a/drivers/hid/hid-input.c >> +++ b/drivers/hid/hid-input.c >> @@ -829,6 +829,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel >> } >> break; >> >> + case 0x38: /* Transducer Index */ >> + map_msc(MSC_PEN_ID); > > This is the new slot version for pens, I am not sure we really want to > blindly introduce that without testing. > > Do you have a panel that supports multiple values (at once) for this > usage (2 pens???). If not, I would simply skip that usage until we get > an actual hardware that makes use of it. > >> + break; >> + >> case 0x3b: /* Battery Strength */ >> hidinput_setup_battery(device, HID_INPUT_REPORT, field, false); >> usage->type = EV_PWR; >> @@ -876,6 +880,20 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel >> map_msc(MSC_SERIAL); >> break; >> >> + case 0x5c: map_msc(MSC_PEN_COLOR); break; >> + case 0x5e: map_msc(MSC_PEN_LINE_WIDTH); break; > > We already have ABS_TOOL_WIDTH which seems to relate to that very closely. > >> + >> + case 0x70: >> + case 0x71: >> + case 0x72: >> + case 0x73: >> + case 0x74: >> + case 0x75: >> + case 0x76: >> + case 0x77: >> + map_msc(MSC_PEN_LINE_STYLE); > > Nope, this is wrong. It took me a long time to understand the report > descriptor (see my last reply to your v2). > Basically, we need one input event for each value between 0x72 and > 0x77. HID core should translate the array of 1 element into a set of > {depressed usage, pressed usage} and from the user-space, we will get > "MSC_PEN_STYLE_CHISEL_MARKER 1" for instance.
This seems to be working (on top of this RFC): --- diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c index cec0470e5485..18035c63c358 100644 --- a/drivers/hid/hid-debug.c +++ b/drivers/hid/hid-debug.c @@ -1027,8 +1027,12 @@ static const char *misc[MSC_MAX + 1] = { [MSC_SERIAL] = "Serial", [MSC_PULSELED] = "Pulseled", [MSC_GESTURE] = "Gesture", [MSC_RAW] = "RawData", [MSC_PEN_ID] = "PenID", [MSC_PEN_COLOR] "PenColor", - [MSC_PEN_LINE_WIDTH] = "PenLineWidth", - [MSC_PEN_LINE_STYLE] = "PenLineStyle", + [MSC_PEN_LINE_STYLE_INK] = "PenLineStyleInk", + [MSC_PEN_LINE_STYLE_PENCIL] = "PenLineStylePencil", + [MSC_PEN_LINE_STYLE_HIGHLIGHTER] = "PenLineStyleHighlighter", + [MSC_PEN_LINE_STYLE_CHISEL_MARKER] = "PenLineStyleChiselMarker", + [MSC_PEN_LINE_STYLE_BRUSH] = "PenLineStyleBrush", + [MSC_PEN_LINE_STYLE_NO_PREFERENCE] = "PenLineStyleNoPreference", }; static const char *leds[LED_MAX + 1] = { diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 079e67c5168f..837585f4e673 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -832,8 +832,7 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel break; case 0x38: /* Transducer Index */ - map_msc(MSC_PEN_ID); - break; + goto ignore; case 0x3b: /* Battery Strength */ hidinput_setup_battery(device, HID_INPUT_REPORT, field, false); @@ -882,18 +881,36 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel map_msc(MSC_SERIAL); break; - case 0x5c: map_msc(MSC_PEN_COLOR); break; - case 0x5e: map_msc(MSC_PEN_LINE_WIDTH); break; - - case 0x70: - case 0x71: - case 0x72: - case 0x73: - case 0x74: - case 0x75: - case 0x76: - case 0x77: - map_msc(MSC_PEN_LINE_STYLE); + case 0x5c: /* Digitizer Preferred Color */ + map_msc(MSC_PEN_COLOR); + break; + + case 0x5e: /* Digitizer Preferred Line Width */ + map_abs_clear(ABS_TOOL_WIDTH); + break; + + + case 0x70: /* Preferred Line Style -> not an input usage*/ + case 0x71: /* Preferred Line Style is Locked */ + goto ignore; + + case 0x72: /* Ink */ + map_msc(MSC_PEN_LINE_STYLE_INK); + break; + case 0x73: /* Pencil */ + map_msc(MSC_PEN_LINE_STYLE_PENCIL); + break; + case 0x74: /* Highlighter */ + map_msc(MSC_PEN_LINE_STYLE_HIGHLIGHTER); + break; + case 0x75: /* Chisel Marker */ + map_msc(MSC_PEN_LINE_STYLE_CHISEL_MARKER); + break; + case 0x76: /* Brush */ + map_msc(MSC_PEN_LINE_STYLE_BRUSH); + break; + case 0x77: /* No Preference */ + map_msc(MSC_PEN_LINE_STYLE_NO_PREFERENCE); break; default: goto unknown; diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h index a76d37082ab5..bf2b76a6e7e8 100644 --- a/include/linux/mod_devicetable.h +++ b/include/linux/mod_devicetable.h @@ -322,7 +322,7 @@ struct pcmcia_device_id { #define INPUT_DEVICE_ID_KEY_MAX 0x2ff #define INPUT_DEVICE_ID_REL_MAX 0x0f #define INPUT_DEVICE_ID_ABS_MAX 0x3f -#define INPUT_DEVICE_ID_MSC_MAX 0x09 +#define INPUT_DEVICE_ID_MSC_MAX 0x0d #define INPUT_DEVICE_ID_LED_MAX 0x0f #define INPUT_DEVICE_ID_SND_MAX 0x07 #define INPUT_DEVICE_ID_FF_MAX 0x7f diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h index 98295f71941a..ff705245b7ae 100644 --- a/include/uapi/linux/input-event-codes.h +++ b/include/uapi/linux/input-event-codes.h @@ -908,12 +908,16 @@ #define MSC_SCAN 0x04 #define MSC_TIMESTAMP 0x05 /* USI Pen events */ -#define MSC_PEN_ID 0x06 -#define MSC_PEN_COLOR 0x07 -#define MSC_PEN_LINE_WIDTH 0x08 -#define MSC_PEN_LINE_STYLE 0x09 +#define MSC_PEN_ID 0x06 +#define MSC_PEN_COLOR 0x07 +#define MSC_PEN_LINE_STYLE_INK 0x08 +#define MSC_PEN_LINE_STYLE_PENCIL 0x09 +#define MSC_PEN_LINE_STYLE_HIGHLIGHTER 0x0a +#define MSC_PEN_LINE_STYLE_CHISEL_MARKER 0x0b +#define MSC_PEN_LINE_STYLE_BRUSH 0x0c +#define MSC_PEN_LINE_STYLE_NO_PREFERENCE 0x0d /* TODO: Add USI diagnostic & battery events too */ -#define MSC_MAX 0x09 +#define MSC_MAX 0x0d #define MSC_CNT (MSC_MAX + 1) /* --- Cheers, Benjamin
> > And *maybe* we could even use KEY events there, so we could remap them > (but that's a very distant maybe). > >> + break; >> + >> default: goto unknown; >> } >> break; >> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h >> index ae2e75d15b21..4ff40be7676b 100644 >> --- a/include/linux/mod_devicetable.h >> +++ b/include/linux/mod_devicetable.h >> @@ -322,7 +322,7 @@ struct pcmcia_device_id { >> #define INPUT_DEVICE_ID_KEY_MAX 0x2ff >> #define INPUT_DEVICE_ID_REL_MAX 0x0f >> #define INPUT_DEVICE_ID_ABS_MAX 0x3f >> -#define INPUT_DEVICE_ID_MSC_MAX 0x07 >> +#define INPUT_DEVICE_ID_MSC_MAX 0x09 >> #define INPUT_DEVICE_ID_LED_MAX 0x0f >> #define INPUT_DEVICE_ID_SND_MAX 0x07 >> #define INPUT_DEVICE_ID_FF_MAX 0x7f >> diff --git a/include/uapi/linux/hid.h b/include/uapi/linux/hid.h >> index 861bfbbfc565..60ef9b615a1a 100644 >> --- a/include/uapi/linux/hid.h >> +++ b/include/uapi/linux/hid.h >> @@ -255,6 +255,7 @@ >> #define HID_DG_TOUCH 0x000d0033 >> #define HID_DG_UNTOUCH 0x000d0034 >> #define HID_DG_TAP 0x000d0035 >> +#define HID_DG_TRANSDUCER_INDEX 0x000d0038 >> #define HID_DG_TABLETFUNCTIONKEY 0x000d0039 >> #define HID_DG_PROGRAMCHANGEKEY 0x000d003a >> #define HID_DG_BATTERYSTRENGTH 0x000d003b >> @@ -267,6 +268,15 @@ >> #define HID_DG_BARRELSWITCH 0x000d0044 >> #define HID_DG_ERASER 0x000d0045 >> #define HID_DG_TABLETPICK 0x000d0046 >> +#define HID_DG_PEN_COLOR 0x000d005c >> +#define HID_DG_PEN_LINE_WIDTH 0x000d005e >> +#define HID_DG_PEN_LINE_STYLE 0x000d0070 >> +#define HID_DG_PEN_LINE_STYLE_INK 0x000d0072 >> +#define HID_DG_PEN_LINE_STYLE_PENCIL 0x000d0073 >> +#define HID_DG_PEN_LINE_STYLE_HIGHLIGHTER 0x000d0074 >> +#define HID_DG_PEN_LINE_STYLE_CHISEL_MARKER 0x000d0075 >> +#define HID_DG_PEN_LINE_STYLE_BRUSH 0x000d0076 >> +#define HID_DG_PEN_LINE_STYLE_NO_PREFERENCE 0x000d0077 > > Could you integrate that patch before my patch "HID: export the > various HID defines from the spec in the uapi"? And also have it as a > separate patch from the mapping? > This way I can schedule that part earlier before we settle on the > actual user space usages. > >> >> #define HID_CP_CONSUMERCONTROL 0x000c0001 >> #define HID_CP_NUMERICKEYPAD 0x000c0002 >> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h >> index 225ec87d4f22..98295f71941a 100644 >> --- a/include/uapi/linux/input-event-codes.h >> +++ b/include/uapi/linux/input-event-codes.h >> @@ -901,14 +901,20 @@ >> * Misc events >> */ >> >> -#define MSC_SERIAL 0x00 >> -#define MSC_PULSELED 0x01 >> -#define MSC_GESTURE 0x02 >> -#define MSC_RAW 0x03 >> -#define MSC_SCAN 0x04 >> -#define MSC_TIMESTAMP 0x05 >> -#define MSC_MAX 0x07 >> -#define MSC_CNT (MSC_MAX+1) >> +#define MSC_SERIAL 0x00 >> +#define MSC_PULSELED 0x01 >> +#define MSC_GESTURE 0x02 >> +#define MSC_RAW 0x03 >> +#define MSC_SCAN 0x04 >> +#define MSC_TIMESTAMP 0x05 >> +/* USI Pen events */ >> +#define MSC_PEN_ID 0x06 >> +#define MSC_PEN_COLOR 0x07 >> +#define MSC_PEN_LINE_WIDTH 0x08 > > PEN_LINE_WIDTH should be dropped. > >> +#define MSC_PEN_LINE_STYLE 0x09 > > Again, PEN_LINE_STYLE is HID only and we should only map the values, > not the title of the array. > > Cheers, > Benjamin > >> +/* TODO: Add USI diagnostic & battery events too */ >> +#define MSC_MAX 0x09 >> +#define MSC_CNT (MSC_MAX + 1) >> >> /* >> * LEDs >> -- >> 2.25.1 >>
| ![\](/images/icornerr.gif) |