| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAO-hwJ+hzHNZVUrL9vC02t4GMBnaLym_vz7JT6+_cKEK8wEAMA@mail.gmail.com>
Date: Wed, 8 Dec 2021 16:18:42 +0100
From: Benjamin Tissoires <benjamin.tissoires@...hat.com>
To: Tero Kristo <tero.kristo@...ux.intel.com>
Cc: "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
Jiri Kosina <jikos@...nel.org>,
Mika Westerberg <mika.westerberg@...ux.intel.com>,
lkml <linux-kernel@...r.kernel.org>,
Dmitry Torokhov <dmitry.torokhov@...il.com>,
Peter Hutterer <peter.hutterer@...-t.net>
Subject: Re: [RFCv3 3/7] HID: core: Add support for USI style events
On Wed, Dec 1, 2021 at 5:43 PM Tero Kristo <tero.kristo@...ux.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@...ux.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
>
Powered by blists - more mailing lists