diff mbox series

[RFCv3,3/7] HID: core: Add support for USI style events

Message ID 20211201164301.44653-4-tero.kristo@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series USI stylus support series | expand

Commit Message

Tero Kristo Dec. 1, 2021, 4:42 p.m. UTC
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(-)

Comments

Benjamin Tissoires Dec. 8, 2021, 3:18 p.m. UTC | #1
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
>
Benjamin Tissoires Dec. 8, 2021, 4:33 p.m. UTC | #2
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
>>
Tero Kristo Dec. 10, 2021, 10:31 a.m. UTC | #3
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 mbox series

Patch

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