linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Tero Kristo <tero.kristo@linux.intel.com>
Cc: "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
	Jiri Kosina <jikos@kernel.org>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	lkml <linux-kernel@vger.kernel.org>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	Peter Hutterer <peter.hutterer@who-t.net>
Subject: Re: [RFCv3 3/7] HID: core: Add support for USI style events
Date: Wed, 8 Dec 2021 17:33:47 +0100	[thread overview]
Message-ID: <b2c95cd3-27b3-9248-e689-b8a47a831225@redhat.com> (raw)
In-Reply-To: <CAO-hwJ+hzHNZVUrL9vC02t4GMBnaLym_vz7JT6+_cKEK8wEAMA@mail.gmail.com>

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


  reply	other threads:[~2021-12-08 16:34 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-01 16:42 [RFCv3 0/7] USI stylus support series Tero Kristo
2021-12-01 16:42 ` [RFCv3 1/7] HID: Add map_msc() to avoid boilerplate code Tero Kristo
2021-12-08 15:03   ` Benjamin Tissoires
2021-12-01 16:42 ` [RFCv3 2/7] HID: hid-input: Add suffix also for HID_DG_PEN Tero Kristo
2021-12-08 15:06   ` Benjamin Tissoires
2021-12-01 16:42 ` [RFCv3 3/7] HID: core: Add support for USI style events Tero Kristo
2021-12-08 15:18   ` Benjamin Tissoires
2021-12-08 16:33     ` Benjamin Tissoires [this message]
2021-12-10 10:31     ` Tero Kristo
2021-12-01 16:42 ` [RFCv3 4/7] HID: input: Make hidinput_find_field() static Tero Kristo
2021-12-08 15:19   ` Benjamin Tissoires
2021-12-01 16:42 ` [RFCv3 5/7] HID: debug: Add USI usages Tero Kristo
2021-12-01 16:43 ` [RFCv3 6/7] samples: hid: add new hid-usi sample Tero Kristo
2021-12-01 16:43 ` [RFCv3 7/7] samples: hid: convert USI sample to use unix socket for comms Tero Kristo
2021-12-08 15:30 ` [RFCv3 0/7] USI stylus support series Benjamin Tissoires
2021-12-09  7:51   ` Tero Kristo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b2c95cd3-27b3-9248-e689-b8a47a831225@redhat.com \
    --to=benjamin.tissoires@redhat.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=jikos@kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mika.westerberg@linux.intel.com \
    --cc=peter.hutterer@who-t.net \
    --cc=tero.kristo@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).