| Message ID | 20211201164301.44653-4-tero.kristo@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | USI stylus support series | expand |
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. 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 >
Hi Benjamin, On 08/12/2021 17: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. Afaik, current controllers (panels) only support single pen at a time. You can use multiple pens with them and they effectively re-use the same pen-id, but you can't use them at the same time. > >> + 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. Ok, I can switch to use this one. > >> + >> + 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. > > 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. Ok will update this, thanks. -Tero > > Cheers, > Benjamin > >> +/* TODO: Add USI diagnostic & battery events too */ >> +#define MSC_MAX 0x09 >> +#define MSC_CNT (MSC_MAX + 1) >> >> /* >> * LEDs >> -- >> 2.25.1 >>
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); + 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; + + case 0x70: + case 0x71: + case 0x72: + case 0x73: + case 0x74: + case 0x75: + case 0x76: + case 0x77: + map_msc(MSC_PEN_LINE_STYLE); + 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 #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 +#define MSC_PEN_LINE_STYLE 0x09 +/* TODO: Add USI diagnostic & battery events too */ +#define MSC_MAX 0x09 +#define MSC_CNT (MSC_MAX + 1) /* * LEDs
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(-)