[PATCH 00/18] *** Implement simple haptic HID support ***

[PATCH 00/18] *** Implement simple haptic HID support ***

[Angela Czubak]

This patch series introduces changes necessary to support devices
using simple haptic HID pages.
Implementation attempts to follow the discussion below:
https://www.spinics.net/lists/linux-input/msg61091.html

Introduce new haptic defines as specified in HID Usage Tables.

Add new force feedback effect type in order to facilitate using
simple haptic force feedback.

Add INPUT_PROP_HAPTIC_TOUCHPAD to mark touchpad exposing simple haptic
support.

Add new struct hid_haptic_device so as to gather simple haptic related
configuration and current state of the device.

Function mt_get_feature() gets renamed to hid_get_feature() and is moved
to hid-core.c as it is not specific to hid multitouch driver and may be
reused, for instance by simple haptic specific source.

Add new functions to be triggered during HID input mapping and
configuration in order to detect simple haptic devices.

Modify HID input so that haptic output reports are parsed.

Initialize a haptic device.

Modify FF core so that effect IDs can be shared between multiple open file
handles.

Add shared release and press effects for a simple haptic device.

Calculate pressure resolution if units are grams or newtons.

Add support for kernel-driven mode of simple haptic device.

Toggle ABS_PRESSURE generation by input-mt on request.

Implement functions allowing switching between kernel-managed mode
and autonomous mode.

Add simple haptic support for hid-multitouch driver.

Implement EVIOCFF(TAKE|RELEASE)CONTROL ioctls so that userspace can take
and release control of shared release and press effects.

Fix i2c_hid_set_or_send_report so that report IDs larger than 0xF are
handled correctly.

Angela Czubak (18):
  HID: add haptics page defines
  Input: add FF_HID effect type
  Input: add INPUT_PROP_HAPTIC_TOUCHPAD
  HID: haptic: introduce hid_haptic_device
  HID: introduce hid_get_feature
  HID: haptic: add functions for mapping and configuration
  HID: input: allow mapping of haptic output
  HID: haptic: initialize haptic device
  Input: add shared effects
  HID: haptic: implement release and press effects
  HID: input: calculate resolution for pressure
  HID: haptic: add functions handling events
  Input: MT - toggle ABS_PRESSURE pointer emulation
  HID: haptic: add hid_haptic_switch_mode
  HID: multitouch: add haptic multitouch support
  Input: introduce EVIOCFF(TAKE|RELEASE)CONTROL
  HID: haptic: add hid_haptic_change_control
  HID: i2c-hid: fix i2c_hid_set_or_send_report

 drivers/hid/Kconfig                    |  15 +
 drivers/hid/Makefile                   |   1 +
 drivers/hid/hid-core.c                 |  39 ++
 drivers/hid/hid-haptic.c               | 745 +++++++++++++++++++++++++
 drivers/hid/hid-haptic.h               | 150 +++++
 drivers/hid/hid-input.c                |  18 +-
 drivers/hid/hid-multitouch.c           | 109 ++--
 drivers/hid/i2c-hid/i2c-hid-core.c     |  12 +-
 drivers/input/evdev.c                  |   6 +
 drivers/input/ff-core.c                | 129 ++++-
 drivers/input/input-mt.c               |  18 +-
 include/linux/hid.h                    |  24 +
 include/linux/input.h                  |   5 +
 include/linux/input/mt.h               |   4 +
 include/uapi/linux/input-event-codes.h |   1 +
 include/uapi/linux/input.h             |  26 +-
 16 files changed, 1247 insertions(+), 55 deletions(-)
 create mode 100644 drivers/hid/hid-haptic.c
 create mode 100644 drivers/hid/hid-haptic.h

-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 01/18] HID: add haptics page defines

[Angela Czubak]

Introduce haptic usages as defined in HID Usage Tables specification.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 include/linux/hid.h | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/include/linux/hid.h b/include/linux/hid.h
index f453be385bd4..70679bf820ce 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -153,6 +153,7 @@ struct hid_item {
 #define HID_UP_TELEPHONY	0x000b0000
 #define HID_UP_CONSUMER		0x000c0000
 #define HID_UP_DIGITIZER	0x000d0000
+#define HID_UP_HAPTIC		0x000e0000
 #define HID_UP_PID		0x000f0000
 #define HID_UP_BATTERY		0x00850000
 #define HID_UP_HPVENDOR         0xff7f0000
@@ -301,6 +302,28 @@ struct hid_item {
 #define HID_DG_TOOLSERIALNUMBER	0x000d005b
 #define HID_DG_LATENCYMODE	0x000d0060
 
+#define HID_HP_SIMPLECONTROLLER	0x000e0001
+#define HID_HP_WAVEFORMLIST	0x000e0010
+#define HID_HP_DURATIONLIST	0x000e0011
+#define HID_HP_AUTOTRIGGER	0x000e0020
+#define HID_HP_MANUALTRIGGER	0x000e0021
+#define HID_HP_AUTOTRIGGERASSOCIATEDCONTROL 0x000e0022
+#define HID_HP_INTENSITY	0x000e0023
+#define HID_HP_REPEATCOUNT	0x000e0024
+#define HID_HP_RETRIGGERPERIOD	0x000e0025
+#define HID_HP_WAVEFORMVENDORPAGE	0x000e0026
+#define HID_HP_WAVEFORMVENDORID	0x000e0027
+#define HID_HP_WAVEFORMCUTOFFTIME	0x000e0028
+#define HID_HP_WAVEFORMNONE	0x000e1001
+#define HID_HP_WAVEFORMSTOP	0x000e1002
+#define HID_HP_WAVEFORMCLICK	0x000e1003
+#define HID_HP_WAVEFORMBUZZCONTINUOUS	0x000e1004
+#define HID_HP_WAVEFORMRUMBLECONTINUOUS	0x000e1005
+#define HID_HP_WAVEFORMPRESS	0x000e1006
+#define HID_HP_WAVEFORMRELEASE	0x000e1007
+#define HID_HP_VENDORWAVEFORMMIN	0x000e2001
+#define HID_HP_VENDORWAVEFORMMAX	0x000e2fff
+
 #define HID_BAT_ABSOLUTESTATEOFCHARGE	0x00850065
 
 #define HID_VD_ASUS_CUSTOM_MEDIA_KEYS	0xff310076
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 01/18] HID: add haptics page defines

[Dmitry Torokhov]

Hi Angela,

On Tue, Dec 21, 2021 at 07:17:26PM +0000, Angela Czubak wrote:
> Introduce haptic usages as defined in HID Usage Tables specification.
> 
> Signed-off-by: Angela Czubak <acz@semihalf.com>
> ---

This needs to be sent to Jiri/Benjamin.

FWIW

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

>  include/linux/hid.h | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index f453be385bd4..70679bf820ce 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -153,6 +153,7 @@ struct hid_item {
>  #define HID_UP_TELEPHONY	0x000b0000
>  #define HID_UP_CONSUMER		0x000c0000
>  #define HID_UP_DIGITIZER	0x000d0000
> +#define HID_UP_HAPTIC		0x000e0000
>  #define HID_UP_PID		0x000f0000
>  #define HID_UP_BATTERY		0x00850000
>  #define HID_UP_HPVENDOR         0xff7f0000
> @@ -301,6 +302,28 @@ struct hid_item {
>  #define HID_DG_TOOLSERIALNUMBER	0x000d005b
>  #define HID_DG_LATENCYMODE	0x000d0060
>  
> +#define HID_HP_SIMPLECONTROLLER	0x000e0001
> +#define HID_HP_WAVEFORMLIST	0x000e0010
> +#define HID_HP_DURATIONLIST	0x000e0011
> +#define HID_HP_AUTOTRIGGER	0x000e0020
> +#define HID_HP_MANUALTRIGGER	0x000e0021
> +#define HID_HP_AUTOTRIGGERASSOCIATEDCONTROL 0x000e0022
> +#define HID_HP_INTENSITY	0x000e0023
> +#define HID_HP_REPEATCOUNT	0x000e0024
> +#define HID_HP_RETRIGGERPERIOD	0x000e0025
> +#define HID_HP_WAVEFORMVENDORPAGE	0x000e0026
> +#define HID_HP_WAVEFORMVENDORID	0x000e0027
> +#define HID_HP_WAVEFORMCUTOFFTIME	0x000e0028
> +#define HID_HP_WAVEFORMNONE	0x000e1001
> +#define HID_HP_WAVEFORMSTOP	0x000e1002
> +#define HID_HP_WAVEFORMCLICK	0x000e1003
> +#define HID_HP_WAVEFORMBUZZCONTINUOUS	0x000e1004
> +#define HID_HP_WAVEFORMRUMBLECONTINUOUS	0x000e1005
> +#define HID_HP_WAVEFORMPRESS	0x000e1006
> +#define HID_HP_WAVEFORMRELEASE	0x000e1007
> +#define HID_HP_VENDORWAVEFORMMIN	0x000e2001
> +#define HID_HP_VENDORWAVEFORMMAX	0x000e2fff
> +
>  #define HID_BAT_ABSOLUTESTATEOFCHARGE	0x00850065
>  
>  #define HID_VD_ASUS_CUSTOM_MEDIA_KEYS	0xff310076
> -- 
> 2.34.1.307.g9b7440fafd-goog
> 

-- 
Dmitry

[PATCH 02/18] Input: add FF_HID effect type

[Angela Czubak]

FF_HID effect type can be used to trigger haptic feedback with HID simple
haptic usages.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 include/uapi/linux/input.h | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index ee3127461ee0..0d4d426cf75a 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -424,6 +424,24 @@ struct ff_rumble_effect {
 	__u16 weak_magnitude;
 };
 
+/**
+ * struct ff_hid_effect
+ * @hid_usage: hid_usage according to Haptics page (WAVEFORM_CLICK, etc.)
+ * @vendor_id: the waveform vendor ID if hid_usage is in the vendor-defined range
+ * @vendor_id: the vendor waveform page if hid_usage is in the vendor-defined range
+ * @intensity: strength of the effect as percentage
+ * @repeat_count: number of times to retrigger effect
+ * @retrigger_period: time before effect is retriggered (in ms)
+ */
+struct ff_hid_effect {
+	__u16 hid_usage;
+	__u16 vendor_id;
+	__u8  vendor_waveform_page;
+	__u16 intensity;
+	__u16 repeat_count;
+	__u16 retrigger_period;
+};
+
 /**
  * struct ff_effect - defines force feedback effect
  * @type: type of the effect (FF_CONSTANT, FF_PERIODIC, FF_RAMP, FF_SPRING,
@@ -460,6 +478,7 @@ struct ff_effect {
 		struct ff_periodic_effect periodic;
 		struct ff_condition_effect condition[2]; /* One for each axis */
 		struct ff_rumble_effect rumble;
+		struct ff_hid_effect hid;
 	} u;
 };
 
@@ -467,6 +486,7 @@ struct ff_effect {
  * Force feedback effect types
  */
 
+#define FF_HID		0x4f
 #define FF_RUMBLE	0x50
 #define FF_PERIODIC	0x51
 #define FF_CONSTANT	0x52
@@ -476,7 +496,7 @@ struct ff_effect {
 #define FF_INERTIA	0x56
 #define FF_RAMP		0x57
 
-#define FF_EFFECT_MIN	FF_RUMBLE
+#define FF_EFFECT_MIN	FF_HID
 #define FF_EFFECT_MAX	FF_RAMP
 
 /*
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 02/18] Input: add FF_HID effect type

[Harry Cutts]

Hi Angela,

On Tue, 21 Dec 2021 at 11:17, Angela Czubak <acz@semihalf.com> wrote:
>
> FF_HID effect type can be used to trigger haptic feedback with HID simple
> haptic usages.
>
> Signed-off-by: Angela Czubak <acz@semihalf.com>
> ---
>  include/uapi/linux/input.h | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index ee3127461ee0..0d4d426cf75a 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -424,6 +424,24 @@ struct ff_rumble_effect {
>         __u16 weak_magnitude;
>  };
>
> +/**
> + * struct ff_hid_effect
> + * @hid_usage: hid_usage according to Haptics page (WAVEFORM_CLICK, etc.)
> + * @vendor_id: the waveform vendor ID if hid_usage is in the vendor-defined range
> + * @vendor_id: the vendor waveform page if hid_usage is in the vendor-defined range

Looks like you forgot to change vendor_id to vendor_waveform_page on
the line above.

> + * @intensity: strength of the effect as percentage
> + * @repeat_count: number of times to retrigger effect
> + * @retrigger_period: time before effect is retriggered (in ms)
> + */
> +struct ff_hid_effect {
> +       __u16 hid_usage;
> +       __u16 vendor_id;
> +       __u8  vendor_waveform_page;
> +       __u16 intensity;
> +       __u16 repeat_count;
> +       __u16 retrigger_period;
> +};
> +
>  /**
>   * struct ff_effect - defines force feedback effect
>   * @type: type of the effect (FF_CONSTANT, FF_PERIODIC, FF_RAMP, FF_SPRING,
> @@ -460,6 +478,7 @@ struct ff_effect {
>                 struct ff_periodic_effect periodic;
>                 struct ff_condition_effect condition[2]; /* One for each axis */
>                 struct ff_rumble_effect rumble;
> +               struct ff_hid_effect hid;
>         } u;
>  };
>
> @@ -467,6 +486,7 @@ struct ff_effect {
>   * Force feedback effect types
>   */
>
> +#define FF_HID         0x4f
>  #define FF_RUMBLE      0x50
>  #define FF_PERIODIC    0x51
>  #define FF_CONSTANT    0x52
> @@ -476,7 +496,7 @@ struct ff_effect {
>  #define FF_INERTIA     0x56
>  #define FF_RAMP                0x57
>
> -#define FF_EFFECT_MIN  FF_RUMBLE
> +#define FF_EFFECT_MIN  FF_HID
>  #define FF_EFFECT_MAX  FF_RAMP
>
>  /*
> --
> 2.34.1.307.g9b7440fafd-goog
>

Harry Cutts
Chrome OS Touch/Input team

[PATCH 03/18] Input: add INPUT_PROP_HAPTIC_TOUCHPAD

[Angela Czubak]

INPUT_PROP_HAPTIC_TOUCHPAD property is to be set for a device with simple
haptic capabilities.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 include/uapi/linux/input-event-codes.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 225ec87d4f22..df2ba5da4eaa 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -27,6 +27,7 @@
 #define INPUT_PROP_TOPBUTTONPAD		0x04	/* softbuttons at top of pad */
 #define INPUT_PROP_POINTING_STICK	0x05	/* is a pointing stick */
 #define INPUT_PROP_ACCELEROMETER	0x06	/* has accelerometer */
+#define INPUT_PROP_HAPTIC_TOUCHPAD	0x07	/* is a haptic touchpad */
 
 #define INPUT_PROP_MAX			0x1f
 #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 03/18] Input: add INPUT_PROP_HAPTIC_TOUCHPAD

[Dmitry Torokhov]

Hi Angela,

On Tue, Dec 21, 2021 at 07:17:28PM +0000, Angela Czubak wrote:
> INPUT_PROP_HAPTIC_TOUCHPAD property is to be set for a device with simple
> haptic capabilities.

This needs to have corresponding documentation in
Documentation/input/event-codes.rst explaning the exact meaning of this
property as it was discussed in the forcepad email thread started by
Sean.

Thanks.

> 
> Signed-off-by: Angela Czubak <acz@semihalf.com>
> ---
>  include/uapi/linux/input-event-codes.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
> index 225ec87d4f22..df2ba5da4eaa 100644
> --- a/include/uapi/linux/input-event-codes.h
> +++ b/include/uapi/linux/input-event-codes.h
> @@ -27,6 +27,7 @@
>  #define INPUT_PROP_TOPBUTTONPAD		0x04	/* softbuttons at top of pad */
>  #define INPUT_PROP_POINTING_STICK	0x05	/* is a pointing stick */
>  #define INPUT_PROP_ACCELEROMETER	0x06	/* has accelerometer */
> +#define INPUT_PROP_HAPTIC_TOUCHPAD	0x07	/* is a haptic touchpad */
>  
>  #define INPUT_PROP_MAX			0x1f
>  #define INPUT_PROP_CNT			(INPUT_PROP_MAX + 1)
> -- 
> 2.34.1.307.g9b7440fafd-goog
> 

-- 
Dmitry

[PATCH 04/18] HID: haptic: introduce hid_haptic_device

[Angela Czubak]

Define a new structure that contains simple haptic device configuration
as well as current state.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/Kconfig      |  4 +++
 drivers/hid/Makefile     |  1 +
 drivers/hid/hid-haptic.c | 10 ++++++
 drivers/hid/hid-haptic.h | 68 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+)
 create mode 100644 drivers/hid/hid-haptic.c
 create mode 100644 drivers/hid/hid-haptic.h

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index a7c78ac96270..8d1eb4491a7f 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -89,6 +89,10 @@ config HID_GENERIC
 
 	If unsure, say Y.
 
+config HID_HAPTIC
+	bool
+	default n
+
 menu "Special HID drivers"
 	depends on HID
 
diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
index 55a6fa3eca5a..65d54ccd4574 100644
--- a/drivers/hid/Makefile
+++ b/drivers/hid/Makefile
@@ -4,6 +4,7 @@
 #
 hid-y			:= hid-core.o hid-input.o hid-quirks.o
 hid-$(CONFIG_DEBUG_FS)		+= hid-debug.o
+hid-$(CONFIG_HID_HAPTIC)	+= hid-haptic.o
 
 obj-$(CONFIG_HID)		+= hid.o
 obj-$(CONFIG_UHID)		+= uhid.o
diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
new file mode 100644
index 000000000000..0910d8af9f38
--- /dev/null
+++ b/drivers/hid/hid-haptic.c
@@ -0,0 +1,10 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  HID Haptic support for Linux
+ *
+ *  Copyright (c) 2021 Angela Czubak
+ */
+
+/*
+ */
+#include "hid-haptic.h"
diff --git a/drivers/hid/hid-haptic.h b/drivers/hid/hid-haptic.h
new file mode 100644
index 000000000000..41f19cd22f75
--- /dev/null
+++ b/drivers/hid/hid-haptic.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ *  HID Haptic support for Linux
+ *
+ *  Copyright (c) 2021 Angela Czubak
+ */
+
+/*
+ */
+
+
+#include <linux/hid.h>
+
+#define HID_HAPTIC_ORDINAL_WAVEFORMNONE 1
+#define HID_HAPTIC_ORDINAL_WAVEFORMSTOP 2
+
+#define HID_HAPTIC_PRESS_THRESH 200
+#define HID_HAPTIC_RELEASE_THRESH 180
+
+#define HID_HAPTIC_MODE_DEVICE 0
+#define HID_HAPTIC_MODE_KERNEL 1
+
+struct hid_haptic_effect {
+	__u8 *report_buf;
+	struct input_dev *input_dev;
+	struct work_struct work;
+	struct list_head control;
+	struct mutex control_mutex;
+};
+
+struct hid_haptic_effect_node {
+	struct list_head node;
+	struct file *file;
+};
+
+struct hid_haptic_device {
+	struct input_dev *input_dev;
+	struct hid_device *hdev;
+	struct hid_report *auto_trigger_report;
+	struct mutex auto_trigger_mutex;
+	struct workqueue_struct *wq;
+	struct hid_report *manual_trigger_report;
+	struct mutex manual_trigger_mutex;
+	size_t manual_trigger_report_len;
+	int pressed_state;
+	__s32 pressure_sum;
+	__s32 force_logical_minimum;
+	__s32 force_physical_minimum;
+	__s32 force_resolution;
+	__u32 press_threshold;
+	__u32 release_threshold;
+	__u32 mode;
+	__u32 default_auto_trigger;
+	__u32 vendor_page;
+	__u32 vendor_id;
+	__u32 max_waveform_id;
+	__u32 max_duration_id;
+	__u16 *hid_usage_map;
+	__u32 *duration_map;
+	__u16 press_ordinal_orig;
+	__u16 press_ordinal_cur;
+	__u16 release_ordinal_orig;
+	__u16 release_ordinal_cur;
+#define HID_HAPTIC_RELEASE_EFFECT_ID 0
+#define HID_HAPTIC_PRESS_EFFECT_ID 1
+	struct hid_haptic_effect *effect;
+	struct hid_haptic_effect stop_effect;
+};
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 04/18] HID: haptic: introduce hid_haptic_device

[Dmitry Torokhov]

On Tue, Dec 21, 2021 at 07:17:29PM +0000, Angela Czubak wrote:
> Define a new structure that contains simple haptic device configuration
> as well as current state.
> 
> Signed-off-by: Angela Czubak <acz@semihalf.com>
> ---
>  drivers/hid/Kconfig      |  4 +++
>  drivers/hid/Makefile     |  1 +
>  drivers/hid/hid-haptic.c | 10 ++++++
>  drivers/hid/hid-haptic.h | 68 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 83 insertions(+)
>  create mode 100644 drivers/hid/hid-haptic.c
>  create mode 100644 drivers/hid/hid-haptic.h
> 
> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> index a7c78ac96270..8d1eb4491a7f 100644
> --- a/drivers/hid/Kconfig
> +++ b/drivers/hid/Kconfig
> @@ -89,6 +89,10 @@ config HID_GENERIC
>  
>  	If unsure, say Y.
>  
> +config HID_HAPTIC
> +	bool
> +	default n

'n' is the default, no need to have it explicit.

> +
>  menu "Special HID drivers"
>  	depends on HID
>  
> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> index 55a6fa3eca5a..65d54ccd4574 100644
> --- a/drivers/hid/Makefile
> +++ b/drivers/hid/Makefile
> @@ -4,6 +4,7 @@
>  #
>  hid-y			:= hid-core.o hid-input.o hid-quirks.o
>  hid-$(CONFIG_DEBUG_FS)		+= hid-debug.o
> +hid-$(CONFIG_HID_HAPTIC)	+= hid-haptic.o
>  
>  obj-$(CONFIG_HID)		+= hid.o
>  obj-$(CONFIG_UHID)		+= uhid.o
> diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
> new file mode 100644
> index 000000000000..0910d8af9f38
> --- /dev/null
> +++ b/drivers/hid/hid-haptic.c
> @@ -0,0 +1,10 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + *  HID Haptic support for Linux
> + *
> + *  Copyright (c) 2021 Angela Czubak
> + */
> +
> +/*
> + */

What is this comment block for? Actually I do not see why this needs to
be a separate patch.

> +#include "hid-haptic.h"
> diff --git a/drivers/hid/hid-haptic.h b/drivers/hid/hid-haptic.h
> new file mode 100644
> index 000000000000..41f19cd22f75
> --- /dev/null
> +++ b/drivers/hid/hid-haptic.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + *  HID Haptic support for Linux
> + *
> + *  Copyright (c) 2021 Angela Czubak
> + */
> +
> +/*
> + */
> +
> +
> +#include <linux/hid.h>
> +
> +#define HID_HAPTIC_ORDINAL_WAVEFORMNONE 1
> +#define HID_HAPTIC_ORDINAL_WAVEFORMSTOP 2
> +
> +#define HID_HAPTIC_PRESS_THRESH 200
> +#define HID_HAPTIC_RELEASE_THRESH 180
> +
> +#define HID_HAPTIC_MODE_DEVICE 0
> +#define HID_HAPTIC_MODE_KERNEL 1
> +
> +struct hid_haptic_effect {
> +	__u8 *report_buf;

This is a matter of preference, but in kernel we normally use u8, s16,
etc, and underscored versions are for headers that are part of uapi.

> +	struct input_dev *input_dev;
> +	struct work_struct work;
> +	struct list_head control;
> +	struct mutex control_mutex;
> +};
> +
> +struct hid_haptic_effect_node {
> +	struct list_head node;
> +	struct file *file;
> +};
> +
> +struct hid_haptic_device {
> +	struct input_dev *input_dev;
> +	struct hid_device *hdev;
> +	struct hid_report *auto_trigger_report;
> +	struct mutex auto_trigger_mutex;
> +	struct workqueue_struct *wq;
> +	struct hid_report *manual_trigger_report;
> +	struct mutex manual_trigger_mutex;
> +	size_t manual_trigger_report_len;
> +	int pressed_state;
> +	__s32 pressure_sum;
> +	__s32 force_logical_minimum;
> +	__s32 force_physical_minimum;
> +	__s32 force_resolution;
> +	__u32 press_threshold;
> +	__u32 release_threshold;
> +	__u32 mode;
> +	__u32 default_auto_trigger;
> +	__u32 vendor_page;
> +	__u32 vendor_id;
> +	__u32 max_waveform_id;
> +	__u32 max_duration_id;
> +	__u16 *hid_usage_map;
> +	__u32 *duration_map;
> +	__u16 press_ordinal_orig;
> +	__u16 press_ordinal_cur;
> +	__u16 release_ordinal_orig;
> +	__u16 release_ordinal_cur;
> +#define HID_HAPTIC_RELEASE_EFFECT_ID 0
> +#define HID_HAPTIC_PRESS_EFFECT_ID 1

Why these definitions are here?

> +	struct hid_haptic_effect *effect;
> +	struct hid_haptic_effect stop_effect;
> +};
> -- 
> 2.34.1.307.g9b7440fafd-goog
> 

Thanks.

-- 
Dmitry

Re: [PATCH 04/18] HID: haptic: introduce hid_haptic_device

[Angela Czubak]

On Fri, Jan 7, 2022 at 11:18 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 07:17:29PM +0000, Angela Czubak wrote:
> > Define a new structure that contains simple haptic device configuration
> > as well as current state.
> >
> > Signed-off-by: Angela Czubak <acz@semihalf.com>
> > ---
> >  drivers/hid/Kconfig      |  4 +++
> >  drivers/hid/Makefile     |  1 +
> >  drivers/hid/hid-haptic.c | 10 ++++++
> >  drivers/hid/hid-haptic.h | 68 ++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 83 insertions(+)
> >  create mode 100644 drivers/hid/hid-haptic.c
> >  create mode 100644 drivers/hid/hid-haptic.h
> >
> > diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
> > index a7c78ac96270..8d1eb4491a7f 100644
> > --- a/drivers/hid/Kconfig
> > +++ b/drivers/hid/Kconfig
> > @@ -89,6 +89,10 @@ config HID_GENERIC
> >
> >       If unsure, say Y.
> >
> > +config HID_HAPTIC
> > +     bool
> > +     default n
>
> 'n' is the default, no need to have it explicit.
>
Ack.
> > +
> >  menu "Special HID drivers"
> >       depends on HID
> >
> > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
> > index 55a6fa3eca5a..65d54ccd4574 100644
> > --- a/drivers/hid/Makefile
> > +++ b/drivers/hid/Makefile
> > @@ -4,6 +4,7 @@
> >  #
> >  hid-y                        := hid-core.o hid-input.o hid-quirks.o
> >  hid-$(CONFIG_DEBUG_FS)               += hid-debug.o
> > +hid-$(CONFIG_HID_HAPTIC)     += hid-haptic.o
> >
> >  obj-$(CONFIG_HID)            += hid.o
> >  obj-$(CONFIG_UHID)           += uhid.o
> > diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
> > new file mode 100644
> > index 000000000000..0910d8af9f38
> > --- /dev/null
> > +++ b/drivers/hid/hid-haptic.c
> > @@ -0,0 +1,10 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + *  HID Haptic support for Linux
> > + *
> > + *  Copyright (c) 2021 Angela Czubak
> > + */
> > +
> > +/*
> > + */
>
> What is this comment block for? Actually I do not see why this needs to
> be a separate patch.
>
I have seen this kind of comment block in both hid-multitouch.c and
hid-core.c, though I can remove it.
Just to be sure: is it OK to introduce all fields/structures at once
with path no 8 ("HID: haptic: initialize haptic device")? Or would you
prefer if I extend the structures as necessary?

> > +#include "hid-haptic.h"
> > diff --git a/drivers/hid/hid-haptic.h b/drivers/hid/hid-haptic.h
> > new file mode 100644
> > index 000000000000..41f19cd22f75
> > --- /dev/null
> > +++ b/drivers/hid/hid-haptic.h
> > @@ -0,0 +1,68 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + *  HID Haptic support for Linux
> > + *
> > + *  Copyright (c) 2021 Angela Czubak
> > + */
> > +
> > +/*
> > + */
> > +
> > +
> > +#include <linux/hid.h>
> > +
> > +#define HID_HAPTIC_ORDINAL_WAVEFORMNONE 1
> > +#define HID_HAPTIC_ORDINAL_WAVEFORMSTOP 2
> > +
> > +#define HID_HAPTIC_PRESS_THRESH 200
> > +#define HID_HAPTIC_RELEASE_THRESH 180
> > +
> > +#define HID_HAPTIC_MODE_DEVICE 0
> > +#define HID_HAPTIC_MODE_KERNEL 1
> > +
> > +struct hid_haptic_effect {
> > +     __u8 *report_buf;
>
> This is a matter of preference, but in kernel we normally use u8, s16,
> etc, and underscored versions are for headers that are part of uapi.
>
> > +     struct input_dev *input_dev;
> > +     struct work_struct work;
> > +     struct list_head control;
> > +     struct mutex control_mutex;
> > +};
> > +
> > +struct hid_haptic_effect_node {
> > +     struct list_head node;
> > +     struct file *file;
> > +};
> > +
> > +struct hid_haptic_device {
> > +     struct input_dev *input_dev;
> > +     struct hid_device *hdev;
> > +     struct hid_report *auto_trigger_report;
> > +     struct mutex auto_trigger_mutex;
> > +     struct workqueue_struct *wq;
> > +     struct hid_report *manual_trigger_report;
> > +     struct mutex manual_trigger_mutex;
> > +     size_t manual_trigger_report_len;
> > +     int pressed_state;
> > +     __s32 pressure_sum;
> > +     __s32 force_logical_minimum;
> > +     __s32 force_physical_minimum;
> > +     __s32 force_resolution;
> > +     __u32 press_threshold;
> > +     __u32 release_threshold;
> > +     __u32 mode;
> > +     __u32 default_auto_trigger;
> > +     __u32 vendor_page;
> > +     __u32 vendor_id;
> > +     __u32 max_waveform_id;
> > +     __u32 max_duration_id;
> > +     __u16 *hid_usage_map;
> > +     __u32 *duration_map;
> > +     __u16 press_ordinal_orig;
> > +     __u16 press_ordinal_cur;
> > +     __u16 release_ordinal_orig;
> > +     __u16 release_ordinal_cur;
> > +#define HID_HAPTIC_RELEASE_EFFECT_ID 0
> > +#define HID_HAPTIC_PRESS_EFFECT_ID 1
>
> Why these definitions are here?
>
I use them as indices in the effect array of effects below, though
they are actually mentioned in Sean O'Brien's kernel design proposal.
Please let me know if you would rather move them above. Perhaps it
should be even somehow exported via uapi (so that userspace does not
hardcode it separately).


> > +     struct hid_haptic_effect *effect;
> > +     struct hid_haptic_effect stop_effect;
> > +};
> > --
> > 2.34.1.307.g9b7440fafd-goog
> >
>
> Thanks.
>
> --
> Dmitry

[PATCH 05/18] HID: introduce hid_get_feature

[Angela Czubak]

Move mt_get_feature from hid-multitouch to hid-core as it is a generic
function that can be used by other drivers as well.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/hid-core.c       | 39 ++++++++++++++++++++++++++++++++++++
 drivers/hid/hid-multitouch.c | 38 +++--------------------------------
 include/linux/hid.h          |  1 +
 3 files changed, 43 insertions(+), 35 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index dbed2524fd47..c11cb7324157 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1796,6 +1796,45 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
 }
 EXPORT_SYMBOL_GPL(hid_report_raw_event);
 
+/**
+ * hid_get_feature - retrieve feature report from device
+ *
+ * @hdev: hid device
+ * @report: hid report to retrieve
+ */
+void hid_get_feature(struct hid_device *hdev, struct hid_report *report)
+{
+	int ret;
+	u32 size = hid_report_len(report);
+	u8 *buf;
+
+	/*
+	 * Do not fetch the feature report if the device has been explicitly
+	 * marked as non-capable.
+	 */
+	if (hdev->quirks & HID_QUIRK_NO_INIT_REPORTS)
+		return;
+
+	buf = hid_alloc_report_buf(report, GFP_KERNEL);
+	if (!buf)
+		return;
+
+	ret = hid_hw_raw_request(hdev, report->id, buf, size,
+				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0) {
+		dev_warn(&hdev->dev, "failed to fetch feature %d\n",
+			 report->id);
+	} else {
+		ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf,
+					   size, 0);
+		if (ret)
+			dev_warn(&hdev->dev, "failed to report feature\n");
+	}
+
+	kfree(buf);
+}
+EXPORT_SYMBOL_GPL(hid_get_feature);
+
 /**
  * hid_input_report - report data from lower layer (usb, bt...)
  *
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 082376a6cb3d..7beb3dfc3e67 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -434,38 +434,6 @@ static const struct attribute_group mt_attribute_group = {
 	.attrs = sysfs_attrs
 };
 
-static void mt_get_feature(struct hid_device *hdev, struct hid_report *report)
-{
-	int ret;
-	u32 size = hid_report_len(report);
-	u8 *buf;
-
-	/*
-	 * Do not fetch the feature report if the device has been explicitly
-	 * marked as non-capable.
-	 */
-	if (hdev->quirks & HID_QUIRK_NO_INIT_REPORTS)
-		return;
-
-	buf = hid_alloc_report_buf(report, GFP_KERNEL);
-	if (!buf)
-		return;
-
-	ret = hid_hw_raw_request(hdev, report->id, buf, size,
-				 HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
-	if (ret < 0) {
-		dev_warn(&hdev->dev, "failed to fetch feature %d\n",
-			 report->id);
-	} else {
-		ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf,
-					   size, 0);
-		if (ret)
-			dev_warn(&hdev->dev, "failed to report feature\n");
-	}
-
-	kfree(buf);
-}
-
 static void mt_feature_mapping(struct hid_device *hdev,
 		struct hid_field *field, struct hid_usage *usage)
 {
@@ -473,7 +441,7 @@ static void mt_feature_mapping(struct hid_device *hdev,
 
 	switch (usage->hid) {
 	case HID_DG_CONTACTMAX:
-		mt_get_feature(hdev, field->report);
+		hid_get_feature(hdev, field->report);
 
 		td->maxcontacts = field->value[0];
 		if (!td->maxcontacts &&
@@ -490,7 +458,7 @@ static void mt_feature_mapping(struct hid_device *hdev,
 			break;
 		}
 
-		mt_get_feature(hdev, field->report);
+		hid_get_feature(hdev, field->report);
 		if (field->value[usage->usage_index] == MT_BUTTONTYPE_CLICKPAD)
 			td->is_buttonpad = true;
 
@@ -498,7 +466,7 @@ static void mt_feature_mapping(struct hid_device *hdev,
 	case 0xff0000c5:
 		/* Retrieve the Win8 blob once to enable some devices */
 		if (usage->usage_index == 0)
-			mt_get_feature(hdev, field->report);
+			hid_get_feature(hdev, field->report);
 		break;
 	}
 }
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 70679bf820ce..fce7966234de 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -1219,6 +1219,7 @@ static inline u32 hid_report_len(struct hid_report *report)
 
 int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
 		int interrupt);
+void hid_get_feature(struct hid_device *hdev, struct hid_report *report);
 
 /* HID quirks API */
 unsigned long hid_lookup_quirk(const struct hid_device *hdev);
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 05/18] HID: introduce hid_get_feature

[Dmitry Torokhov]

On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
> Move mt_get_feature from hid-multitouch to hid-core as it is a generic
> function that can be used by other drivers as well.
> 
> Signed-off-by: Angela Czubak <acz@semihalf.com>
> ---
>  drivers/hid/hid-core.c       | 39 ++++++++++++++++++++++++++++++++++++
>  drivers/hid/hid-multitouch.c | 38 +++--------------------------------
>  include/linux/hid.h          |  1 +
>  3 files changed, 43 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index dbed2524fd47..c11cb7324157 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1796,6 +1796,45 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
>  }
>  EXPORT_SYMBOL_GPL(hid_report_raw_event);
>  
> +/**
> + * hid_get_feature - retrieve feature report from device
> + *
> + * @hdev: hid device
> + * @report: hid report to retrieve
> + */
> +void hid_get_feature(struct hid_device *hdev, struct hid_report *report)

If this is a generic API I believe it should return success/error code
so that users can decide what to do.

Thanks.

-- 
Dmitry

Re: [PATCH 05/18] HID: introduce hid_get_feature

[Angela Czubak]

On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
> > Move mt_get_feature from hid-multitouch to hid-core as it is a generic
> > function that can be used by other drivers as well.
> >
> > Signed-off-by: Angela Czubak <acz@semihalf.com>
> > ---
> >  drivers/hid/hid-core.c       | 39 ++++++++++++++++++++++++++++++++++++
> >  drivers/hid/hid-multitouch.c | 38 +++--------------------------------
> >  include/linux/hid.h          |  1 +
> >  3 files changed, 43 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index dbed2524fd47..c11cb7324157 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -1796,6 +1796,45 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
> >  }
> >  EXPORT_SYMBOL_GPL(hid_report_raw_event);
> >
> > +/**
> > + * hid_get_feature - retrieve feature report from device
> > + *
> > + * @hdev: hid device
> > + * @report: hid report to retrieve
> > + */
> > +void hid_get_feature(struct hid_device *hdev, struct hid_report *report)
>
> If this is a generic API I believe it should return success/error code
> so that users can decide what to do.
>
Does it mean I should also modify hid-multitouch.c so that the return
value is actually checked? Currently it seems to ignore any failures.
> Thanks.

>
> --
> Dmitry

Re: [PATCH 05/18] HID: introduce hid_get_feature

[Benjamin Tissoires]

On 1/10/22 20:43, Angela Czubak wrote:
> On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>>
>> On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
>>> Move mt_get_feature from hid-multitouch to hid-core as it is a generic
>>> function that can be used by other drivers as well.
>>>
>>> Signed-off-by: Angela Czubak <acz@semihalf.com>
>>> ---
>>>   drivers/hid/hid-core.c       | 39 ++++++++++++++++++++++++++++++++++++
>>>   drivers/hid/hid-multitouch.c | 38 +++--------------------------------
>>>   include/linux/hid.h          |  1 +
>>>   3 files changed, 43 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index dbed2524fd47..c11cb7324157 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -1796,6 +1796,45 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
>>>   }
>>>   EXPORT_SYMBOL_GPL(hid_report_raw_event);
>>>
>>> +/**
>>> + * hid_get_feature - retrieve feature report from device
>>> + *
>>> + * @hdev: hid device
>>> + * @report: hid report to retrieve
>>> + */
>>> +void hid_get_feature(struct hid_device *hdev, struct hid_report *report)
>>
>> If this is a generic API I believe it should return success/error code
>> so that users can decide what to do.
>>
> Does it mean I should also modify hid-multitouch.c so that the return
> value is actually checked? Currently it seems to ignore any failures.
>> Thanks.

Honestly that function is a hack in hid-multitouch. You can replace it by:

```
hid_device_io_start(hid);
hid_hw_request(hid, report, HID_REQ_GET_REPORT);
hid_hw_wait(hid);
hid_device_io_stop(hid);
```

The hack allows to not have to use hid_device_io_{start|stop}(), which 
is probably not clean.

As for the return value, hid_hw_request() can be used as asynchronous, 
which is why it returns void. However, returning an actual int would 
definitively be better because some cases are failing silently (like if 
the device is not io started).

Cheers,
Benjamin

> 
>>
>> --
>> Dmitry

Re: [PATCH 05/18] HID: introduce hid_get_feature

[Angela Czubak]

On Wed, Jan 12, 2022 at 10:43 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On 1/10/22 20:43, Angela Czubak wrote:
> > On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >>
> >> On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
> >>> Move mt_get_feature from hid-multitouch to hid-core as it is a generic
> >>> function that can be used by other drivers as well.
> >>>
> >>> Signed-off-by: Angela Czubak <acz@semihalf.com>
> >>> ---
> >>>   drivers/hid/hid-core.c       | 39 ++++++++++++++++++++++++++++++++++++
> >>>   drivers/hid/hid-multitouch.c | 38 +++--------------------------------
> >>>   include/linux/hid.h          |  1 +
> >>>   3 files changed, 43 insertions(+), 35 deletions(-)
> >>>
> >>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> >>> index dbed2524fd47..c11cb7324157 100644
> >>> --- a/drivers/hid/hid-core.c
> >>> +++ b/drivers/hid/hid-core.c
> >>> @@ -1796,6 +1796,45 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
> >>>   }
> >>>   EXPORT_SYMBOL_GPL(hid_report_raw_event);
> >>>
> >>> +/**
> >>> + * hid_get_feature - retrieve feature report from device
> >>> + *
> >>> + * @hdev: hid device
> >>> + * @report: hid report to retrieve
> >>> + */
> >>> +void hid_get_feature(struct hid_device *hdev, struct hid_report *report)
> >>
> >> If this is a generic API I believe it should return success/error code
> >> so that users can decide what to do.
> >>
> > Does it mean I should also modify hid-multitouch.c so that the return
> > value is actually checked? Currently it seems to ignore any failures.
> >> Thanks.
>
> Honestly that function is a hack in hid-multitouch. You can replace it by:
>
> ```
> hid_device_io_start(hid);
> hid_hw_request(hid, report, HID_REQ_GET_REPORT);
> hid_hw_wait(hid);
> hid_device_io_stop(hid);
> ```
>
> The hack allows to not have to use hid_device_io_{start|stop}(), which
> is probably not clean.
>
> As for the return value, hid_hw_request() can be used as asynchronous,
> which is why it returns void. However, returning an actual int would
> definitively be better because some cases are failing silently (like if
> the device is not io started).
>
I am slightly confused; it is hid_hw_raw_request() that is used and it does
not seem asynchronous to me; is there no guarantee that the response
has already been received? It seemed to me that the main purpose of
this function is to retrieve information an have it correctly parsed.
I literally issue it once to learn if auto trigger has been set by default and
to know the durations of waveforms, learn ordinals etc.
I could introduce a new function for the purpose of haptic API, it just
seemed redundant as the one in hid-multitouch.c does what I need.

> Cheers,
> Benjamin
>
> >
> >>
> >> --
> >> Dmitry
>

Re: [PATCH 05/18] HID: introduce hid_get_feature

[Benjamin Tissoires]

On Wed, Jan 12, 2022 at 12:26 PM Angela Czubak <acz@semihalf.com> wrote:
>
> On Wed, Jan 12, 2022 at 10:43 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On 1/10/22 20:43, Angela Czubak wrote:
> > > On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > >>
> > >> On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
> > >>> Move mt_get_feature from hid-multitouch to hid-core as it is a generic
> > >>> function that can be used by other drivers as well.
> > >>>
> > >>> Signed-off-by: Angela Czubak <acz@semihalf.com>
> > >>> ---
> > >>>   drivers/hid/hid-core.c       | 39 ++++++++++++++++++++++++++++++++++++
> > >>>   drivers/hid/hid-multitouch.c | 38 +++--------------------------------
> > >>>   include/linux/hid.h          |  1 +
> > >>>   3 files changed, 43 insertions(+), 35 deletions(-)
> > >>>
> > >>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > >>> index dbed2524fd47..c11cb7324157 100644
> > >>> --- a/drivers/hid/hid-core.c
> > >>> +++ b/drivers/hid/hid-core.c
> > >>> @@ -1796,6 +1796,45 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
> > >>>   }
> > >>>   EXPORT_SYMBOL_GPL(hid_report_raw_event);
> > >>>
> > >>> +/**
> > >>> + * hid_get_feature - retrieve feature report from device
> > >>> + *
> > >>> + * @hdev: hid device
> > >>> + * @report: hid report to retrieve
> > >>> + */
> > >>> +void hid_get_feature(struct hid_device *hdev, struct hid_report *report)
> > >>
> > >> If this is a generic API I believe it should return success/error code
> > >> so that users can decide what to do.
> > >>
> > > Does it mean I should also modify hid-multitouch.c so that the return
> > > value is actually checked? Currently it seems to ignore any failures.
> > >> Thanks.
> >
> > Honestly that function is a hack in hid-multitouch. You can replace it by:
> >
> > ```
> > hid_device_io_start(hid);
> > hid_hw_request(hid, report, HID_REQ_GET_REPORT);
> > hid_hw_wait(hid);
> > hid_device_io_stop(hid);
> > ```
> >
> > The hack allows to not have to use hid_device_io_{start|stop}(), which
> > is probably not clean.
> >
> > As for the return value, hid_hw_request() can be used as asynchronous,
> > which is why it returns void. However, returning an actual int would
> > definitively be better because some cases are failing silently (like if
> > the device is not io started).
> >
> I am slightly confused; it is hid_hw_raw_request() that is used and it does
> not seem asynchronous to me; is there no guarantee that the response
> has already been received?

In the case of usbhid, hid_hw_request() calls directly
__usbhid_submit_report() which is asynchronous.
So no, we have no guarantees that the answer is there.

>  It seemed to me that the main purpose of
> this function is to retrieve information an have it correctly parsed.
> I literally issue it once to learn if auto trigger has been set by default and
> to know the durations of waveforms, learn ordinals etc.
> I could introduce a new function for the purpose of haptic API, it just
> seemed redundant as the one in hid-multitouch.c does what I need.

Again, the one in hid-multitouch is a hack against
hid_device_io_{start|stop}(). So if you need to change something, it's
the hid-multitouch code, not reuse that hack :)

Cheers,
Benjamin

Re: [PATCH 05/18] HID: introduce hid_get_feature

[Angela Czubak]

Hi Benjamin,

On Thu, Jan 13, 2022 at 10:54 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Jan 12, 2022 at 12:26 PM Angela Czubak <acz@semihalf.com> wrote:
> >
> > On Wed, Jan 12, 2022 at 10:43 AM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > On 1/10/22 20:43, Angela Czubak wrote:
> > > > On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > >>
> > > >> On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
> > > >>> Move mt_get_feature from hid-multitouch to hid-core as it is a generic
> > > >>> function that can be used by other drivers as well.
> > > >>>
> > > >>> Signed-off-by: Angela Czubak <acz@semihalf.com>
> > > >>> ---
> > > >>>   drivers/hid/hid-core.c       | 39 ++++++++++++++++++++++++++++++++++++
> > > >>>   drivers/hid/hid-multitouch.c | 38 +++--------------------------------
> > > >>>   include/linux/hid.h          |  1 +
> > > >>>   3 files changed, 43 insertions(+), 35 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > >>> index dbed2524fd47..c11cb7324157 100644
> > > >>> --- a/drivers/hid/hid-core.c
> > > >>> +++ b/drivers/hid/hid-core.c
> > > >>> @@ -1796,6 +1796,45 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
> > > >>>   }
> > > >>>   EXPORT_SYMBOL_GPL(hid_report_raw_event);
> > > >>>
> > > >>> +/**
> > > >>> + * hid_get_feature - retrieve feature report from device
> > > >>> + *
> > > >>> + * @hdev: hid device
> > > >>> + * @report: hid report to retrieve
> > > >>> + */
> > > >>> +void hid_get_feature(struct hid_device *hdev, struct hid_report *report)
> > > >>
> > > >> If this is a generic API I believe it should return success/error code
> > > >> so that users can decide what to do.
> > > >>
> > > > Does it mean I should also modify hid-multitouch.c so that the return
> > > > value is actually checked? Currently it seems to ignore any failures.
> > > >> Thanks.
> > >
> > > Honestly that function is a hack in hid-multitouch. You can replace it by:
> > >
> > > ```
> > > hid_device_io_start(hid);
> > > hid_hw_request(hid, report, HID_REQ_GET_REPORT);
> > > hid_hw_wait(hid);
> > > hid_device_io_stop(hid);
> > > ```
> > >
> > > The hack allows to not have to use hid_device_io_{start|stop}(), which
> > > is probably not clean.
> > >
> > > As for the return value, hid_hw_request() can be used as asynchronous,
> > > which is why it returns void. However, returning an actual int would
> > > definitively be better because some cases are failing silently (like if
> > > the device is not io started).
> > >
> > I am slightly confused; it is hid_hw_raw_request() that is used and it does
> > not seem asynchronous to me; is there no guarantee that the response
> > has already been received?
>
> In the case of usbhid, hid_hw_request() calls directly
> __usbhid_submit_report() which is asynchronous.
> So no, we have no guarantees that the answer is there.
>
> >  It seemed to me that the main purpose of
> > this function is to retrieve information an have it correctly parsed.
> > I literally issue it once to learn if auto trigger has been set by default and
> > to know the durations of waveforms, learn ordinals etc.
> > I could introduce a new function for the purpose of haptic API, it just
> > seemed redundant as the one in hid-multitouch.c does what I need.
>
> Again, the one in hid-multitouch is a hack against
> hid_device_io_{start|stop}(). So if you need to change something, it's
> the hid-multitouch code, not reuse that hack :)
>

ACK, I will use hid_hw_request() and hid_hw_wait() instead as suggested.
BTW is hid_device_io_{start|stop} required to do anything meaningful?
It just seems to me that it currently sets some variable which is not really
useful anywhere else.

> Cheers,
> Benjamin
>

Re: [PATCH 05/18] HID: introduce hid_get_feature

[Benjamin Tissoires]

On Fri, Jan 14, 2022 at 7:24 PM Angela Czubak <acz@semihalf.com> wrote:
>
> Hi Benjamin,
>
> On Thu, Jan 13, 2022 at 10:54 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Wed, Jan 12, 2022 at 12:26 PM Angela Czubak <acz@semihalf.com> wrote:
> > >
> > > On Wed, Jan 12, 2022 at 10:43 AM Benjamin Tissoires
> > > <benjamin.tissoires@redhat.com> wrote:
> > > >
> > > > On 1/10/22 20:43, Angela Czubak wrote:
> > > > > On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >>
> > > > >> On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
> > > > >>> Move mt_get_feature from hid-multitouch to hid-core as it is a generic
> > > > >>> function that can be used by other drivers as well.
> > > > >>>
> > > > >>> Signed-off-by: Angela Czubak <acz@semihalf.com>
> > > > >>> ---
> > > > >>>   drivers/hid/hid-core.c       | 39 ++++++++++++++++++++++++++++++++++++
> > > > >>>   drivers/hid/hid-multitouch.c | 38 +++--------------------------------
> > > > >>>   include/linux/hid.h          |  1 +
> > > > >>>   3 files changed, 43 insertions(+), 35 deletions(-)
> > > > >>>
> > > > >>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > >>> index dbed2524fd47..c11cb7324157 100644
> > > > >>> --- a/drivers/hid/hid-core.c
> > > > >>> +++ b/drivers/hid/hid-core.c
> > > > >>> @@ -1796,6 +1796,45 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
> > > > >>>   }
> > > > >>>   EXPORT_SYMBOL_GPL(hid_report_raw_event);
> > > > >>>
> > > > >>> +/**
> > > > >>> + * hid_get_feature - retrieve feature report from device
> > > > >>> + *
> > > > >>> + * @hdev: hid device
> > > > >>> + * @report: hid report to retrieve
> > > > >>> + */
> > > > >>> +void hid_get_feature(struct hid_device *hdev, struct hid_report *report)
> > > > >>
> > > > >> If this is a generic API I believe it should return success/error code
> > > > >> so that users can decide what to do.
> > > > >>
> > > > > Does it mean I should also modify hid-multitouch.c so that the return
> > > > > value is actually checked? Currently it seems to ignore any failures.
> > > > >> Thanks.
> > > >
> > > > Honestly that function is a hack in hid-multitouch. You can replace it by:
> > > >
> > > > ```
> > > > hid_device_io_start(hid);
> > > > hid_hw_request(hid, report, HID_REQ_GET_REPORT);
> > > > hid_hw_wait(hid);
> > > > hid_device_io_stop(hid);
> > > > ```
> > > >
> > > > The hack allows to not have to use hid_device_io_{start|stop}(), which
> > > > is probably not clean.
> > > >
> > > > As for the return value, hid_hw_request() can be used as asynchronous,
> > > > which is why it returns void. However, returning an actual int would
> > > > definitively be better because some cases are failing silently (like if
> > > > the device is not io started).
> > > >
> > > I am slightly confused; it is hid_hw_raw_request() that is used and it does
> > > not seem asynchronous to me; is there no guarantee that the response
> > > has already been received?
> >
> > In the case of usbhid, hid_hw_request() calls directly
> > __usbhid_submit_report() which is asynchronous.
> > So no, we have no guarantees that the answer is there.
> >
> > >  It seemed to me that the main purpose of
> > > this function is to retrieve information an have it correctly parsed.
> > > I literally issue it once to learn if auto trigger has been set by default and
> > > to know the durations of waveforms, learn ordinals etc.
> > > I could introduce a new function for the purpose of haptic API, it just
> > > seemed redundant as the one in hid-multitouch.c does what I need.
> >
> > Again, the one in hid-multitouch is a hack against
> > hid_device_io_{start|stop}(). So if you need to change something, it's
> > the hid-multitouch code, not reuse that hack :)
> >
>
> ACK, I will use hid_hw_request() and hid_hw_wait() instead as suggested.
> BTW is hid_device_io_{start|stop} required to do anything meaningful?
> It just seems to me that it currently sets some variable which is not really
> useful anywhere else.
>

The thing it does is to release the semaphore
hid->driver_input_lock(), which is then in turn used in hid-core.c, in
hid_input_report(). Without this, hid_input_report() aborts earlier
and the field->values[] are not populated because HID considers it
should ignore the incoming reports.

So without this, you can not access the new values apart from doing
what hid-multitouch does: trick the system and force inject an event
in the processing pipeline.

Cheers,
Benjamin

[PATCH 06/18] HID: haptic: add functions for mapping and configuration

[Angela Czubak]

Add functions that recognize auto trigger and manual trigger reports
as well as save their addresses.
Verify that the pressure unit is either grams or newtons.
Mark the input device as a haptic touchpad if the unit is correct and
the reports are found.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/hid-haptic.c | 63 ++++++++++++++++++++++++++++++++++++++++
 drivers/hid/hid-haptic.h | 46 +++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+)

diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
index 0910d8af9f38..f130ec96a240 100644
--- a/drivers/hid/hid-haptic.c
+++ b/drivers/hid/hid-haptic.c
@@ -7,4 +7,67 @@
 
 /*
  */
+
 #include "hid-haptic.h"
+
+void hid_haptic_feature_mapping(struct hid_device *hdev,
+				struct hid_haptic_device *haptic,
+				struct hid_field *field, struct hid_usage *usage)
+{
+	if (usage->hid == HID_HP_AUTOTRIGGER) {
+		if (usage->usage_index >= field->report_count) {
+			dev_err(&hdev->dev,
+				"HID_HP_AUTOTRIGGER out of range\n");
+			return;
+		}
+
+		hid_get_feature(hdev, field->report);
+		haptic->default_auto_trigger =
+			field->value[usage->usage_index];
+		haptic->auto_trigger_report = field->report;
+	}
+}
+EXPORT_SYMBOL_GPL(hid_haptic_feature_mapping);
+
+bool hid_haptic_check_pressure_unit(struct hid_haptic_device *haptic,
+				    struct hid_input *hi, struct hid_field *field)
+{
+	/* Accepted units are either grams or newtons. */
+	if (field->unit == 0x0101 || field->unit == 0xe111)
+		return true;
+	return false;
+}
+EXPORT_SYMBOL_GPL(hid_haptic_check_pressure_unit);
+
+int hid_haptic_input_mapping(struct hid_device *hdev,
+			     struct hid_haptic_device *haptic,
+			     struct hid_input *hi,
+			     struct hid_field *field, struct hid_usage *usage,
+			     unsigned long **bit, int *max)
+{
+	if (usage->hid == HID_HP_MANUALTRIGGER) {
+		haptic->manual_trigger_report = field->report;
+		/* we don't really want to map these fields */
+		return -1;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(hid_haptic_input_mapping);
+
+int hid_haptic_input_configured(struct hid_device *hdev,
+				struct hid_haptic_device *haptic,
+				struct hid_input *hi)
+{
+
+	if (hi->application == HID_DG_TOUCHPAD) {
+		if (haptic->auto_trigger_report &&
+		    haptic->manual_trigger_report) {
+			__set_bit(INPUT_PROP_HAPTIC_TOUCHPAD, hi->input->propbit);
+			return 1;
+		}
+		return 0;
+	}
+	return -1;
+}
+EXPORT_SYMBOL_GPL(hid_haptic_input_configured);
diff --git a/drivers/hid/hid-haptic.h b/drivers/hid/hid-haptic.h
index 41f19cd22f75..4645d6cfd55a 100644
--- a/drivers/hid/hid-haptic.h
+++ b/drivers/hid/hid-haptic.h
@@ -66,3 +66,49 @@ struct hid_haptic_device {
 	struct hid_haptic_effect *effect;
 	struct hid_haptic_effect stop_effect;
 };
+
+#ifdef CONFIG_MULTITOUCH_HAPTIC
+void hid_haptic_feature_mapping(struct hid_device *hdev,
+				struct hid_haptic_device *haptic,
+				struct hid_field *field, struct hid_usage
+				*usage);
+bool hid_haptic_check_pressure_unit(struct hid_haptic_device *haptic,
+				    struct hid_input *hi, struct hid_field *field);
+int hid_haptic_input_mapping(struct hid_device *hdev,
+			     struct hid_haptic_device *haptic,
+			     struct hid_input *hi,
+			     struct hid_field *field, struct hid_usage *usage,
+			     unsigned long **bit, int *max);
+int hid_haptic_input_configured(struct hid_device *hdev,
+				struct hid_haptic_device *haptic,
+				struct hid_input *hi);
+#else
+static inline
+void hid_haptic_feature_mapping(struct hid_device *hdev,
+				struct hid_haptic_device *haptic,
+				struct hid_field *field, struct hid_usage
+				*usage)
+{}
+static inline
+bool hid_haptic_check_pressure_unit(struct hid_haptic_device *haptic,
+				    struct hid_input *hi, struct hid_field *field)
+{
+	return false;
+}
+static inline
+int hid_haptic_input_mapping(struct hid_device *hdev,
+			     struct hid_haptic_device *haptic,
+			     struct hid_input *hi,
+			     struct hid_field *field, struct hid_usage *usage,
+			     unsigned long **bit, int *max)
+{
+	return 0;
+}
+static inline
+int hid_haptic_input_configured(struct hid_device *hdev,
+				struct hid_haptic_device *haptic,
+				struct hid_input *hi)
+{
+	return 0;
+}
+#endif
-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 07/18] HID: input: allow mapping of haptic output

[Angela Czubak]

This change makes it possible to parse output reports by input mapping
functions by HID drivers.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/hid-input.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 03f994541981..81eb277dee91 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -599,9 +599,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 	if (field->report_count < 1)
 		goto ignore;
 
-	/* only LED usages are supported in output fields */
+	/* only LED and HAPTIC usages are supported in output fields */
 	if (field->report_type == HID_OUTPUT_REPORT &&
-			(usage->hid & HID_USAGE_PAGE) != HID_UP_LED) {
+	    (usage->hid & HID_USAGE_PAGE) != HID_UP_LED &&
+	    (usage->hid & HID_USAGE_PAGE) != HID_UP_HAPTIC) {
 		goto ignore;
 	}
 
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 07/18] HID: input: allow mapping of haptic output

[Dmitry Torokhov]

On Tue, Dec 21, 2021 at 07:17:32PM +0000, Angela Czubak wrote:
> This change makes it possible to parse output reports by input mapping
> functions by HID drivers.
> 
> Signed-off-by: Angela Czubak <acz@semihalf.com>

Please send to Jiri/Benjamin.

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> ---
>  drivers/hid/hid-input.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 03f994541981..81eb277dee91 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -599,9 +599,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>  	if (field->report_count < 1)
>  		goto ignore;
>  
> -	/* only LED usages are supported in output fields */
> +	/* only LED and HAPTIC usages are supported in output fields */
>  	if (field->report_type == HID_OUTPUT_REPORT &&
> -			(usage->hid & HID_USAGE_PAGE) != HID_UP_LED) {
> +	    (usage->hid & HID_USAGE_PAGE) != HID_UP_LED &&
> +	    (usage->hid & HID_USAGE_PAGE) != HID_UP_HAPTIC) {
>  		goto ignore;
>  	}
>  
> -- 
> 2.34.1.307.g9b7440fafd-goog
> 

-- 
Dmitry

[PATCH 08/18] HID: haptic: initialize haptic device

[Angela Czubak]

Add hid_haptic_init(). Parse autotrigger report to retrieve ordinals for
press and release waveforms.
Implement force feedback functions.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/hid-haptic.c | 445 +++++++++++++++++++++++++++++++++++++++
 drivers/hid/hid-haptic.h |   6 +
 2 files changed, 451 insertions(+)

diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
index f130ec96a240..b66bde7475e4 100644
--- a/drivers/hid/hid-haptic.c
+++ b/drivers/hid/hid-haptic.c
@@ -8,12 +8,16 @@
 /*
  */
 
+#include <linux/module.h>
+
 #include "hid-haptic.h"
 
 void hid_haptic_feature_mapping(struct hid_device *hdev,
 				struct hid_haptic_device *haptic,
 				struct hid_field *field, struct hid_usage *usage)
 {
+	u16 usage_hid;
+
 	if (usage->hid == HID_HP_AUTOTRIGGER) {
 		if (usage->usage_index >= field->report_count) {
 			dev_err(&hdev->dev,
@@ -25,6 +29,20 @@ void hid_haptic_feature_mapping(struct hid_device *hdev,
 		haptic->default_auto_trigger =
 			field->value[usage->usage_index];
 		haptic->auto_trigger_report = field->report;
+	} else if ((usage->hid & HID_USAGE_PAGE) == HID_UP_ORDINAL) {
+		usage_hid = usage->hid & HID_USAGE;
+		switch (field->logical) {
+		case HID_HP_WAVEFORMLIST:
+			if (usage_hid > haptic->max_waveform_id)
+				haptic->max_waveform_id = usage_hid;
+			break;
+		case HID_HP_DURATIONLIST:
+			if (usage_hid > haptic->max_duration_id)
+				haptic->max_duration_id = usage_hid;
+			break;
+		default:
+			break;
+		}
 	}
 }
 EXPORT_SYMBOL_GPL(hid_haptic_feature_mapping);
@@ -71,3 +89,430 @@ int hid_haptic_input_configured(struct hid_device *hdev,
 	return -1;
 }
 EXPORT_SYMBOL_GPL(hid_haptic_input_configured);
+
+static void parse_auto_trigger_field(struct hid_haptic_device *haptic,
+				     struct hid_field *field)
+{
+	int count = field->report_count;
+	int n;
+	u16 usage_hid;
+
+	for (n = 0; n < count; n++) {
+		switch (field->usage[n].hid & HID_USAGE_PAGE) {
+		case HID_UP_ORDINAL:
+			usage_hid = field->usage[n].hid & HID_USAGE;
+			switch (field->logical) {
+			case HID_HP_WAVEFORMLIST:
+				haptic->hid_usage_map[usage_hid] = field->value[n];
+				if (field->value[n] ==
+				    (HID_HP_WAVEFORMPRESS & HID_USAGE)) {
+					haptic->press_ordinal_orig = usage_hid;
+					haptic->press_ordinal_cur = usage_hid;
+				} else if (field->value[n] ==
+					   (HID_HP_WAVEFORMRELEASE & HID_USAGE)) {
+					haptic->release_ordinal_orig = usage_hid;
+					haptic->release_ordinal_cur = usage_hid;
+				}
+				break;
+			case HID_HP_DURATIONLIST:
+				haptic->duration_map[usage_hid] =
+					field->value[n];
+				break;
+			default:
+				break;
+			}
+			break;
+		case HID_UP_HAPTIC:
+			switch (field->usage[n].hid) {
+			case HID_HP_WAVEFORMVENDORID:
+				haptic->vendor_id = field->value[n];
+				break;
+			case HID_HP_WAVEFORMVENDORPAGE:
+				haptic->vendor_page = field->value[n];
+				break;
+			default:
+				break;
+			}
+			break;
+		default:
+			/* Should not really happen */
+			break;
+		}
+	}
+}
+
+static void fill_effect_buf(struct hid_haptic_device *haptic,
+			    struct ff_hid_effect *effect,
+			    struct hid_haptic_effect *haptic_effect,
+			    int waveform_ordinal)
+{
+	struct hid_report *rep = haptic->manual_trigger_report;
+	struct hid_usage *usage;
+	struct hid_field *field;
+	s32 value;
+	int i, j;
+	u8 *buf = haptic_effect->report_buf;
+
+	mutex_lock(&haptic->manual_trigger_mutex);
+	for (i = 0; i < rep->maxfield; i++) {
+		field = rep->field[i];
+		/* Ignore if report count is out of bounds. */
+		if (field->report_count < 1)
+			continue;
+
+		for (j = 0; j < field->maxusage; j++) {
+			usage = &field->usage[j];
+
+			switch (usage->hid) {
+			case HID_HP_INTENSITY:
+				if (effect->intensity > 100) {
+					value = field->logical_maximum;
+				} else {
+					value = field->logical_minimum +
+						effect->intensity *
+						(field->logical_maximum -
+						 field->logical_minimum) / 100;
+				}
+				break;
+			case HID_HP_REPEATCOUNT:
+				value = effect->repeat_count;
+				break;
+			case HID_HP_RETRIGGERPERIOD:
+				value = effect->retrigger_period;
+				break;
+			case HID_HP_MANUALTRIGGER:
+				value = waveform_ordinal;
+				break;
+			default:
+				break;
+			}
+
+			field->value[j] = value;
+		}
+	}
+
+	hid_output_report(rep, buf);
+	mutex_unlock(&haptic->manual_trigger_mutex);
+}
+
+static int hid_haptic_upload_effect(struct input_dev *dev, struct ff_effect *effect,
+				    struct ff_effect *old)
+{
+	struct ff_device *ff = dev->ff;
+	struct hid_haptic_device *haptic = ff->private;
+	int i, ordinal = 0;
+
+	/* If vendor range, check vendor id and page */
+	if (effect->u.hid.hid_usage >= (HID_HP_VENDORWAVEFORMMIN & HID_USAGE) &&
+	    effect->u.hid.hid_usage <= (HID_HP_VENDORWAVEFORMMAX & HID_USAGE) &&
+	    (effect->u.hid.vendor_id != haptic->vendor_id ||
+	     effect->u.hid.vendor_waveform_page != haptic->vendor_page))
+		return -EINVAL;
+
+	/* Check hid_usage */
+	for (i = 1; i < haptic->max_waveform_id; i++) {
+		if (haptic->hid_usage_map[i] == effect->u.hid.hid_usage) {
+			ordinal = i;
+			break;
+		}
+	}
+	if (ordinal < 1)
+		return -EINVAL;
+
+	/* Fill the buffer for the efect id */
+	fill_effect_buf(haptic, &effect->u.hid, &haptic->effect[effect->id],
+			ordinal);
+
+	return 0;
+}
+
+static int play_effect(struct hid_device *hdev, struct hid_haptic_device *haptic,
+		       struct hid_haptic_effect *effect)
+{
+	int ret;
+
+	ret = hid_hw_output_report(hdev, effect->report_buf,
+				   haptic->manual_trigger_report_len);
+	if (ret < 0) {
+		ret = hid_hw_raw_request(hdev,
+					 haptic->manual_trigger_report->id,
+					 effect->report_buf,
+					 haptic->manual_trigger_report_len,
+					 HID_OUTPUT_REPORT, HID_REQ_SET_REPORT);
+
+	}
+
+	return ret;
+}
+
+static void haptic_work_handler(struct work_struct *work)
+{
+
+	struct hid_haptic_effect *effect = container_of(work,
+							struct hid_haptic_effect,
+							work);
+	struct input_dev *dev = effect->input_dev;
+	struct hid_device *hdev = input_get_drvdata(dev);
+	struct hid_haptic_device *haptic = dev->ff->private;
+
+	mutex_lock(&haptic->manual_trigger_mutex);
+	if (effect != &haptic->stop_effect)
+		play_effect(hdev, haptic, &haptic->stop_effect);
+
+	play_effect(hdev, haptic, effect);
+	mutex_unlock(&haptic->manual_trigger_mutex);
+
+}
+
+static int hid_haptic_playback(struct input_dev *dev, int effect_id, int value)
+{
+	struct hid_haptic_device *haptic = dev->ff->private;
+
+	if (value)
+		queue_work(haptic->wq, &haptic->effect[effect_id].work);
+	else
+		queue_work(haptic->wq, &haptic->stop_effect.work);
+
+	return 0;
+}
+
+static void effect_set_default(struct ff_effect *effect)
+{
+	effect->type = FF_HID;
+	effect->id = -1;
+	effect->u.hid.hid_usage = HID_HP_WAVEFORMNONE & HID_USAGE;
+	effect->u.hid.intensity = 100;
+	effect->u.hid.retrigger_period = 0;
+	effect->u.hid.repeat_count = 0;
+}
+
+static int hid_haptic_erase(struct input_dev *dev, int effect_id)
+{
+	struct hid_haptic_device *haptic = dev->ff->private;
+	struct ff_effect effect;
+	int ordinal;
+
+	effect_set_default(&effect);
+	switch (effect_id) {
+	case HID_HAPTIC_RELEASE_EFFECT_ID:
+		ordinal = haptic->release_ordinal_orig;
+		if (!ordinal)
+			ordinal = HID_HAPTIC_ORDINAL_WAVEFORMNONE;
+		else
+			effect.u.hid.hid_usage = HID_HP_WAVEFORMRELEASE &
+				HID_USAGE;
+		fill_effect_buf(haptic, &effect.u.hid, &haptic->effect[effect_id],
+				ordinal);
+		break;
+	case HID_HAPTIC_PRESS_EFFECT_ID:
+		ordinal = haptic->press_ordinal_orig;
+		if (!ordinal)
+			ordinal = HID_HAPTIC_ORDINAL_WAVEFORMNONE;
+		else
+			effect.u.hid.hid_usage = HID_HP_WAVEFORMPRESS &
+				HID_USAGE;
+		fill_effect_buf(haptic, &effect.u.hid, &haptic->effect[effect_id],
+				ordinal);
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static void hid_haptic_destroy(struct ff_device *ff)
+{
+	struct hid_haptic_device *haptic = ff->private;
+	struct hid_device *hdev = haptic->hdev;
+	int r;
+
+	if (hdev)
+		put_device(&hdev->dev);
+
+	kfree(haptic->stop_effect.report_buf);
+	haptic->stop_effect.report_buf = NULL;
+
+	if (haptic->effect) {
+		for (r = 0; r < ff->max_effects; r++)
+			kfree(haptic->effect[r].report_buf);
+		kfree(haptic->effect);
+	}
+	haptic->effect = NULL;
+
+	destroy_workqueue(haptic->wq);
+	haptic->wq = NULL;
+
+	kfree(haptic->duration_map);
+	haptic->duration_map = NULL;
+
+	kfree(haptic->hid_usage_map);
+	haptic->hid_usage_map = NULL;
+
+	module_put(THIS_MODULE);
+}
+
+int hid_haptic_init(struct hid_device *hdev,
+		    struct hid_haptic_device **haptic_ptr)
+{
+	struct hid_haptic_device *haptic = *haptic_ptr;
+	struct input_dev *dev = NULL;
+	struct hid_input *hidinput;
+	struct ff_device *ff;
+	int ret = 0, r;
+	struct ff_hid_effect stop_effect = {
+		.hid_usage = HID_HP_WAVEFORMSTOP & HID_USAGE,
+	};
+	const char *prefix = "hid-haptic";
+	char *name;
+	int (*flush)(struct input_dev *dev, struct file *file);
+	int (*event)(struct input_dev *dev, unsigned int type, unsigned int code, int value);
+
+	haptic->hdev = hdev;
+	haptic->max_waveform_id = max(2u, haptic->max_waveform_id);
+	haptic->max_duration_id = max(2u, haptic->max_duration_id);
+
+	haptic->hid_usage_map = kcalloc(haptic->max_waveform_id + 1,
+					sizeof(__u16), GFP_KERNEL);
+	if (!haptic->hid_usage_map) {
+		ret = -ENOMEM;
+		goto exit;
+	}
+	haptic->duration_map = kcalloc(haptic->max_duration_id + 1,
+				       sizeof(__u32), GFP_KERNEL);
+	if (!haptic->duration_map) {
+		ret = -ENOMEM;
+		goto usage_map;
+	}
+
+	if (haptic->max_waveform_id != haptic->max_duration_id)
+		dev_warn(&hdev->dev,
+			 "Haptic duration and waveform lists have different max id (%u and %u).\n",
+			 haptic->max_duration_id, haptic->max_waveform_id);
+
+	haptic->hid_usage_map[HID_HAPTIC_ORDINAL_WAVEFORMNONE] =
+		HID_HP_WAVEFORMNONE & HID_USAGE;
+	haptic->hid_usage_map[HID_HAPTIC_ORDINAL_WAVEFORMSTOP] =
+		HID_HP_WAVEFORMSTOP & HID_USAGE;
+
+	for (r = 0; r < haptic->auto_trigger_report->maxfield; r++)
+		parse_auto_trigger_field(haptic, haptic->auto_trigger_report->field[r]);
+
+	list_for_each_entry(hidinput, &hdev->inputs, list) {
+		if (hidinput->application == HID_DG_TOUCHPAD) {
+			dev = hidinput->input;
+			break;
+		}
+	}
+
+	if (!dev) {
+		dev_err(&hdev->dev, "Failed to find the input device\n");
+		ret = -ENODEV;
+		goto duration_map;
+	}
+
+	haptic->input_dev = dev;
+	haptic->manual_trigger_report_len =
+		hid_report_len(haptic->manual_trigger_report);
+	mutex_init(&haptic->manual_trigger_mutex);
+	name = kmalloc(strlen(prefix) + strlen(hdev->name) + 2, GFP_KERNEL);
+	if (name) {
+		sprintf(name, "%s %s", prefix, hdev->name);
+		haptic->wq = create_singlethread_workqueue(name);
+		kfree(name);
+	}
+	if (!haptic->wq) {
+		ret = -ENOMEM;
+		goto duration_map;
+	}
+	haptic->effect = kcalloc(FF_MAX_EFFECTS,
+				 sizeof(struct hid_haptic_effect), GFP_KERNEL);
+	if (!haptic->effect) {
+		ret = -ENOMEM;
+		goto output_queue;
+	}
+	for (r = 0; r < FF_MAX_EFFECTS; r++) {
+		haptic->effect[r].report_buf =
+			hid_alloc_report_buf(haptic->manual_trigger_report,
+					     GFP_KERNEL);
+		if (!haptic->effect[r].report_buf) {
+			dev_err(&hdev->dev,
+				"Failed to allocate a buffer for an effect.\n");
+			ret = -ENOMEM;
+			goto buffer_free;
+		}
+		haptic->effect[r].input_dev = dev;
+		INIT_WORK(&haptic->effect[r].work, haptic_work_handler);
+	}
+	haptic->stop_effect.report_buf =
+		hid_alloc_report_buf(haptic->manual_trigger_report,
+				     GFP_KERNEL);
+	if (!haptic->stop_effect.report_buf) {
+		dev_err(&hdev->dev,
+			"Failed to allocate a buffer for stop effect.\n");
+		ret = -ENOMEM;
+		goto buffer_free;
+	}
+	haptic->stop_effect.input_dev = dev;
+	INIT_WORK(&haptic->stop_effect.work, haptic_work_handler);
+	fill_effect_buf(haptic, &stop_effect, &haptic->stop_effect,
+			HID_HAPTIC_ORDINAL_WAVEFORMSTOP);
+
+	input_set_capability(dev, EV_FF, FF_HID);
+
+	flush = dev->flush;
+	event = dev->event;
+	ret = input_ff_create(dev, FF_MAX_EFFECTS);
+	if (ret) {
+		dev_err(&hdev->dev, "Failed to create ff device.\n");
+		goto stop_buffer_free;
+	}
+
+	ff = dev->ff;
+	ff->private = haptic;
+	ff->upload = hid_haptic_upload_effect;
+	ff->playback = hid_haptic_playback;
+	ff->erase = hid_haptic_erase;
+	ff->destroy = hid_haptic_destroy;
+	if (!try_module_get(THIS_MODULE)) {
+		dev_err(&hdev->dev, "Failed to increase module count.\n");
+		goto input_free;
+	}
+	if (!get_device(&hdev->dev)) {
+		dev_err(&hdev->dev, "Failed to get hdev device.\n");
+		module_put(THIS_MODULE);
+		goto input_free;
+	}
+	return 0;
+
+input_free:
+	input_ff_destroy(dev);
+	/* Do not let double free happen, input_ff_destroy will call
+	 * hid_haptic_destroy.
+	 */
+	*haptic_ptr = NULL;
+	/* Restore dev flush and event */
+	dev->flush = flush;
+	dev->event = event;
+	return ret;
+stop_buffer_free:
+	kfree(haptic->stop_effect.report_buf);
+	haptic->stop_effect.report_buf = NULL;
+buffer_free:
+	while (--r >= 0)
+		kfree(haptic->effect[r].report_buf);
+	kfree(haptic->effect);
+	haptic->effect = NULL;
+output_queue:
+	destroy_workqueue(haptic->wq);
+	haptic->wq = NULL;
+duration_map:
+	kfree(haptic->duration_map);
+	haptic->duration_map = NULL;
+usage_map:
+	kfree(haptic->hid_usage_map);
+	haptic->hid_usage_map = NULL;
+exit:
+	return ret;
+}
+EXPORT_SYMBOL_GPL(hid_haptic_init);
diff --git a/drivers/hid/hid-haptic.h b/drivers/hid/hid-haptic.h
index 4645d6cfd55a..2581833125dd 100644
--- a/drivers/hid/hid-haptic.h
+++ b/drivers/hid/hid-haptic.h
@@ -82,6 +82,7 @@ int hid_haptic_input_mapping(struct hid_device *hdev,
 int hid_haptic_input_configured(struct hid_device *hdev,
 				struct hid_haptic_device *haptic,
 				struct hid_input *hi);
+int hid_haptic_init(struct hid_device *hdev, struct hid_haptic_device **haptic_ptr);
 #else
 static inline
 void hid_haptic_feature_mapping(struct hid_device *hdev,
@@ -111,4 +112,9 @@ int hid_haptic_input_configured(struct hid_device *hdev,
 {
 	return 0;
 }
+static inline
+int hid_haptic_init(struct hid_device *hdev, struct hid_haptic_device **haptic_ptr)
+{
+	return 0;
+}
 #endif
-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 09/18] Input: add shared effects

[Angela Czubak]

If an effect is uploaded with file handle equal UINTPTR_MAX assume this
effect should be shared and so may be modified using different file
handles.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/input/ff-core.c | 40 ++++++++++++++++++++++++++++++++++------
 1 file changed, 34 insertions(+), 6 deletions(-)

diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index 1cf5deda06e1..960ae0e29348 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -34,6 +34,23 @@ static int check_effect_access(struct ff_device *ff, int effect_id,
 	return 0;
 }
 
+/*
+ * Check that the effect_id is a valid effect and whether the effect
+ * is shared
+ */
+static int check_effect_shared(struct ff_device *ff, int effect_id)
+{
+	if (effect_id < 0 || effect_id >= ff->max_effects ||
+	    !ff->effect_owners[effect_id])
+		return -EINVAL;
+
+	/* Shared effect */
+	if (ff->effect_owners[effect_id] == (struct file *)UINTPTR_MAX)
+		return 0;
+
+	return -EACCES;
+}
+
 /*
  * Checks whether 2 effects can be combined together
  */
@@ -139,8 +156,11 @@ int input_ff_upload(struct input_dev *dev, struct ff_effect *effect,
 		id = effect->id;
 
 		ret = check_effect_access(ff, id, file);
-		if (ret)
-			goto out;
+		if (ret) {
+			ret = check_effect_shared(ff, id);
+			if (ret)
+				goto out;
+		}
 
 		old = &ff->effects[id];
 
@@ -174,21 +194,29 @@ static int erase_effect(struct input_dev *dev, int effect_id,
 {
 	struct ff_device *ff = dev->ff;
 	int error;
+	bool shared = false;
 
 	error = check_effect_access(ff, effect_id, file);
-	if (error)
-		return error;
+	if (error) {
+		error = check_effect_shared(ff, effect_id);
+		if (!error)
+			shared = true;
+		else
+			return error;
+	}
 
 	spin_lock_irq(&dev->event_lock);
 	ff->playback(dev, effect_id, 0);
-	ff->effect_owners[effect_id] = NULL;
+	if (!shared)
+		ff->effect_owners[effect_id] = NULL;
 	spin_unlock_irq(&dev->event_lock);
 
 	if (ff->erase) {
 		error = ff->erase(dev, effect_id);
 		if (error) {
 			spin_lock_irq(&dev->event_lock);
-			ff->effect_owners[effect_id] = file;
+			if (!shared)
+				ff->effect_owners[effect_id] = file;
 			spin_unlock_irq(&dev->event_lock);
 
 			return error;
-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 10/18] HID: haptic: implement release and press effects

[Angela Czubak]

Upload shared haptic affects for release and press waveforms if a device
exposes them.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/hid-haptic.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
index b66bde7475e4..ad458bc7d4c5 100644
--- a/drivers/hid/hid-haptic.c
+++ b/drivers/hid/hid-haptic.c
@@ -367,6 +367,7 @@ int hid_haptic_init(struct hid_device *hdev,
 	char *name;
 	int (*flush)(struct input_dev *dev, struct file *file);
 	int (*event)(struct input_dev *dev, unsigned int type, unsigned int code, int value);
+	struct ff_effect release_effect, press_effect;
 
 	haptic->hdev = hdev;
 	haptic->max_waveform_id = max(2u, haptic->max_waveform_id);
@@ -483,8 +484,41 @@ int hid_haptic_init(struct hid_device *hdev,
 		module_put(THIS_MODULE);
 		goto input_free;
 	}
+
+	effect_set_default(&release_effect);
+	if (haptic->release_ordinal_orig)
+		release_effect.u.hid.hid_usage = HID_HP_WAVEFORMRELEASE &
+			HID_USAGE;
+	ret = input_ff_upload(dev, &release_effect, (struct file *)UINTPTR_MAX);
+	if (ret || release_effect.id != HID_HAPTIC_RELEASE_EFFECT_ID) {
+		if (!ret) {
+			ret = -EBUSY;
+			input_ff_erase(dev, release_effect.id,
+				       (struct file *)UINTPTR_MAX);
+		}
+		dev_err(&hdev->dev,
+			"Failed to allocate id 0 for release effect.\n");
+		goto input_free;
+	}
+	effect_set_default(&press_effect);
+	if (haptic->press_ordinal_orig)
+		press_effect.u.hid.hid_usage = HID_HP_WAVEFORMPRESS & HID_USAGE;
+	ret = input_ff_upload(dev, &press_effect, (struct file *)UINTPTR_MAX);
+	if (ret || press_effect.id != HID_HAPTIC_PRESS_EFFECT_ID) {
+		if (!ret) {
+			ret = -EBUSY;
+			input_ff_erase(dev, press_effect.id,
+				       (struct file *)UINTPTR_MAX);
+		}
+		dev_err(&hdev->dev,
+			"Failed to allocate id 1 for press effect.\n");
+		goto release_free;
+	}
+
 	return 0;
 
+release_free:
+	input_ff_erase(dev, release_effect.id, (struct file *)UINTPTR_MAX);
 input_free:
 	input_ff_destroy(dev);
 	/* Do not let double free happen, input_ff_destroy will call
-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 11/18] HID: input: calculate resolution for pressure

[Angela Czubak]

Assume that if the pressure is given in newtons it should be normalized
to grams. If the pressure has no unit do not calculate resolution.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/hid-input.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 81eb277dee91..b680641a30c0 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -257,6 +257,19 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
 		}
 		break;
 
+	case ABS_PRESSURE:
+	case ABS_MT_PRESSURE:
+		if (field->unit == 0xe111) {		/* If newtons */
+			/* Convert to grams */
+			prev = physical_extents;
+			physical_extents *= 10197;
+			if (physical_extents < prev)
+				return 0;
+			unit_exponent -= 2;
+		} else if (field->unit != 0x101) {	/* If not grams */
+			return 0;
+		}
+		break;
 	default:
 		return 0;
 	}
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 11/18] HID: input: calculate resolution for pressure

[Dmitry Torokhov]

On Tue, Dec 21, 2021 at 07:17:36PM +0000, Angela Czubak wrote:
> Assume that if the pressure is given in newtons it should be normalized
> to grams. If the pressure has no unit do not calculate resolution.
> 
> Signed-off-by: Angela Czubak <acz@semihalf.com>
> ---
>  drivers/hid/hid-input.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 81eb277dee91..b680641a30c0 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -257,6 +257,19 @@ __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code)
>  		}
>  		break;
>  
> +	case ABS_PRESSURE:
> +	case ABS_MT_PRESSURE:
> +		if (field->unit == 0xe111) {		/* If newtons */

Using a named constant would be great.

> +			/* Convert to grams */

If you could add to the comment that 1 newton is 101.97 grams that would
be great.

> +			prev = physical_extents;
> +			physical_extents *= 10197;
> +			if (physical_extents < prev)
> +				return 0;
> +			unit_exponent -= 2;
> +		} else if (field->unit != 0x101) {	/* If not grams */
> +			return 0;
> +		}
> +		break;
>  	default:
>  		return 0;
>  	}
> -- 
> 2.34.1.307.g9b7440fafd-goog
> 

Thanks.

-- 
Dmitry

[PATCH 12/18] HID: haptic: add functions handling events

[Angela Czubak]

Implement hid_haptic_handle_press_release() which generates haptic feedback
as well as saves the pressed state of the haptic device.
Function hid_haptic_handle_input() inserts BTN_LEFT and ABS_PRESSURE events
if the device is in kernel mode.
Add functions to increase and reset the state of the pressure detected by
the device.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/hid-haptic.c | 72 +++++++++++++++++++++++++++++++++++++++-
 drivers/hid/hid-haptic.h | 20 +++++++++++
 2 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
index ad458bc7d4c5..85c4711f685e 100644
--- a/drivers/hid/hid-haptic.c
+++ b/drivers/hid/hid-haptic.c
@@ -51,8 +51,13 @@ bool hid_haptic_check_pressure_unit(struct hid_haptic_device *haptic,
 				    struct hid_input *hi, struct hid_field *field)
 {
 	/* Accepted units are either grams or newtons. */
-	if (field->unit == 0x0101 || field->unit == 0xe111)
+	if (field->unit == 0x0101 || field->unit == 0xe111) {
+		haptic->force_logical_minimum = field->logical_minimum;
+		haptic->force_physical_minimum = field->physical_minimum;
+		haptic->force_resolution = input_abs_get_res(hi->input,
+							     ABS_MT_PRESSURE);
 		return true;
+	}
 	return false;
 }
 EXPORT_SYMBOL_GPL(hid_haptic_check_pressure_unit);
@@ -352,6 +357,13 @@ static void hid_haptic_destroy(struct ff_device *ff)
 	module_put(THIS_MODULE);
 }
 
+static __u32 convert_force_to_logical(struct hid_haptic_device *haptic,
+					 __u32 value)
+{
+	return (value - haptic->force_physical_minimum) *
+		haptic->force_resolution + haptic->force_logical_minimum;
+}
+
 int hid_haptic_init(struct hid_device *hdev,
 		    struct hid_haptic_device **haptic_ptr)
 {
@@ -459,6 +471,13 @@ int hid_haptic_init(struct hid_device *hdev,
 	fill_effect_buf(haptic, &stop_effect, &haptic->stop_effect,
 			HID_HAPTIC_ORDINAL_WAVEFORMSTOP);
 
+	haptic->mode = HID_HAPTIC_MODE_DEVICE;
+	haptic->press_threshold = convert_force_to_logical(haptic,
+							   HID_HAPTIC_PRESS_THRESH);
+	haptic->release_threshold = convert_force_to_logical(haptic,
+							     HID_HAPTIC_RELEASE_THRESH);
+
+
 	input_set_capability(dev, EV_FF, FF_HID);
 
 	flush = dev->flush;
@@ -550,3 +569,54 @@ int hid_haptic_init(struct hid_device *hdev,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(hid_haptic_init);
+
+void hid_haptic_handle_press_release(struct hid_haptic_device *haptic)
+{
+	int prev_pressed_state = haptic->pressed_state;
+	struct input_dev *input = haptic->input_dev;
+	unsigned long flags;
+
+	if (haptic->pressure_sum > haptic->press_threshold)
+		haptic->pressed_state = 1;
+	else if (haptic->pressure_sum < haptic->release_threshold)
+		haptic->pressed_state = 0;
+	if (!prev_pressed_state && haptic->pressed_state &&
+	    haptic->mode == HID_HAPTIC_MODE_KERNEL) {
+		spin_lock_irqsave(&input->event_lock, flags);
+		input->ff->playback(input, PRESS_HID_EFFECT_ID, 1);
+		spin_unlock_irqrestore(&input->event_lock, flags);
+	}
+	if (prev_pressed_state && !haptic->pressed_state &&
+	    haptic->mode == HID_HAPTIC_MODE_KERNEL) {
+		spin_lock_irqsave(&input->event_lock, flags);
+		input->ff->playback(input, RELEASE_HID_EFFECT_ID, 1);
+		spin_unlock_irqrestore(&input->event_lock, flags);
+	}
+}
+EXPORT_SYMBOL_GPL(hid_haptic_handle_press_release);
+
+bool hid_haptic_handle_input(struct hid_haptic_device *haptic)
+{
+	if (haptic->mode == HID_HAPTIC_MODE_KERNEL) {
+		input_event(haptic->input_dev, EV_KEY, BTN_LEFT,
+			    haptic->pressed_state);
+		input_event(haptic->input_dev, EV_ABS, ABS_PRESSURE,
+			    haptic->pressure_sum);
+		return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL_GPL(hid_haptic_handle_input);
+
+void hid_haptic_pressure_reset(struct hid_haptic_device *haptic)
+{
+	haptic->pressure_sum = 0;
+}
+EXPORT_SYMBOL_GPL(hid_haptic_pressure_reset);
+
+void hid_haptic_pressure_increase(struct hid_haptic_device *haptic,
+				 __s32 pressure)
+{
+	haptic->pressure_sum += pressure;
+}
+EXPORT_SYMBOL_GPL(hid_haptic_pressure_increase);
diff --git a/drivers/hid/hid-haptic.h b/drivers/hid/hid-haptic.h
index 2581833125dd..27ae1ed576c4 100644
--- a/drivers/hid/hid-haptic.h
+++ b/drivers/hid/hid-haptic.h
@@ -83,6 +83,11 @@ int hid_haptic_input_configured(struct hid_device *hdev,
 				struct hid_haptic_device *haptic,
 				struct hid_input *hi);
 int hid_haptic_init(struct hid_device *hdev, struct hid_haptic_device **haptic_ptr);
+void hid_haptic_handle_press_release(struct hid_haptic_device *haptic);
+bool hid_haptic_handle_input(struct hid_haptic_device *haptic);
+void hid_haptic_pressure_reset(struct hid_haptic_device *haptic);
+void hid_haptic_pressure_increase(struct hid_haptic_device *haptic,
+				  __s32 pressure);
 #else
 static inline
 void hid_haptic_feature_mapping(struct hid_device *hdev,
@@ -117,4 +122,19 @@ int hid_haptic_init(struct hid_device *hdev, struct hid_haptic_device **haptic_p
 {
 	return 0;
 }
+static inline
+void hid_haptic_handle_press_release(struct hid_haptic_device *haptic)
+{}
+static inline
+bool hid_haptic_handle_input(struct hid_haptic_device *haptic)
+{
+	return false;
+}
+static inline
+void hid_haptic_pressure_reset(struct hid_haptic_device *haptic)
+{}
+static inline
+void hid_haptic_pressure_increase(struct hid_haptic_device *haptic,
+				  __s32 pressure)
+{}
 #endif
-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Angela Czubak]

Add a function to switch off ABS_PRESSURE generation if necessary.
This may be helpful in case drivers want to generate ABS_PRESSURE events
themselves from ABS_MT_PRESSURE.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/input/input-mt.c | 18 ++++++++++++++++--
 include/linux/input/mt.h |  4 ++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index 44fe6f2f063c..e0bf5917a8b5 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -52,6 +52,7 @@ int input_mt_init_slots(struct input_dev *dev, unsigned int num_slots,
 
 	mt->num_slots = num_slots;
 	mt->flags = flags;
+	mt->abs_pressure_gen = true;
 	input_set_abs_params(dev, ABS_MT_SLOT, 0, num_slots - 1, 0, 0);
 	input_set_abs_params(dev, ABS_MT_TRACKING_ID, 0, TRKID_MAX, 0, 0);
 
@@ -244,12 +245,14 @@ void input_mt_report_pointer_emulation(struct input_dev *dev, bool use_count)
 		input_event(dev, EV_ABS, ABS_X, x);
 		input_event(dev, EV_ABS, ABS_Y, y);
 
-		if (test_bit(ABS_MT_PRESSURE, dev->absbit)) {
+		if (test_bit(ABS_MT_PRESSURE, dev->absbit) &&
+		    mt->abs_pressure_gen) {
 			int p = input_mt_get_value(oldest, ABS_MT_PRESSURE);
 			input_event(dev, EV_ABS, ABS_PRESSURE, p);
 		}
 	} else {
-		if (test_bit(ABS_MT_PRESSURE, dev->absbit))
+		if (test_bit(ABS_MT_PRESSURE, dev->absbit) &&
+		    mt->abs_pressure_gen)
 			input_event(dev, EV_ABS, ABS_PRESSURE, 0);
 	}
 }
@@ -312,6 +315,17 @@ void input_mt_sync_frame(struct input_dev *dev)
 }
 EXPORT_SYMBOL(input_mt_sync_frame);
 
+void input_mt_pressure_toggle(struct input_dev *dev, bool toggle)
+{
+	struct input_mt *mt = dev->mt;
+
+	if (!mt)
+		return;
+
+	mt->abs_pressure_gen = toggle;
+}
+EXPORT_SYMBOL(input_mt_pressure_toggle);
+
 static int adjust_dual(int *begin, int step, int *end, int eq, int mu)
 {
 	int f, *p, s, c;
diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
index 3b8580bd33c1..c870a513bde1 100644
--- a/include/linux/input/mt.h
+++ b/include/linux/input/mt.h
@@ -38,6 +38,7 @@ struct input_mt_slot {
  * @flags: input_mt operation flags
  * @frame: increases every time input_mt_sync_frame() is called
  * @red: reduced cost matrix for in-kernel tracking
+ * @abs_pressure_gen: emulate pointer pressure
  * @slots: array of slots holding current values of tracked contacts
  */
 struct input_mt {
@@ -47,6 +48,7 @@ struct input_mt {
 	unsigned int flags;
 	unsigned int frame;
 	int *red;
+	bool abs_pressure_gen;
 	struct input_mt_slot slots[];
 };
 
@@ -111,6 +113,8 @@ void input_mt_drop_unused(struct input_dev *dev);
 
 void input_mt_sync_frame(struct input_dev *dev);
 
+void input_mt_pressure_toggle(struct input_dev *dev, bool toggle);
+
 /**
  * struct input_mt_pos - contact position
  * @x: horizontal coordinate
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Dmitry Torokhov]

Hi Angela,

On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> Add a function to switch off ABS_PRESSURE generation if necessary.
> This may be helpful in case drivers want to generate ABS_PRESSURE events
> themselves from ABS_MT_PRESSURE.

This needs better explanation for why it is needed. I assume this is to
use ABS_PRESSURE to report "true force" for devices. If this is correct
then I believe we should define a new flag for input_mt_init_slots()
and check it here and also use it to calculate the force across contacts
in input_mt_sync_frame().

Or did I misunderstand the point?

Thanks.

-- 
Dmitry

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Angela Czubak]

Hi Dmitry,

On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Angela,
>
> On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > Add a function to switch off ABS_PRESSURE generation if necessary.
> > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > themselves from ABS_MT_PRESSURE.
>
> This needs better explanation for why it is needed. I assume this is to
> use ABS_PRESSURE to report "true force" for devices. If this is correct
> then I believe we should define a new flag for input_mt_init_slots()
> and check it here and also use it to calculate the force across contacts
> in input_mt_sync_frame().
>
> Or did I misunderstand the point?
>
I would say you understood it correctly, though to my mind it is not a
static behaviour,
i.e. we may want to switch this kind of calculation on and off.
Are flags intended to be modified at runtime?
For instance, if user decides to remove the release or press effect (previously
uploaded by them) and there is no default one per device, then we should switch
the haptic handling from kernel mode back to device mode. Currently it
also means
that the driver stops generating ABS_PRESSURE events on its own and so
input-mt layer may/should be used again (i.e. mt report pointer emulation).
Anyhow, if it would be actually better to calculate the true force in
input_mt_sync_frame()/input_mt_report_pointer_emulation()
> Thanks.

>
> --
> Dmitry

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Dmitry Torokhov]

On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> Hi Dmitry,
> 
> On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Angela,
> >
> > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > themselves from ABS_MT_PRESSURE.
> >
> > This needs better explanation for why it is needed. I assume this is to
> > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > then I believe we should define a new flag for input_mt_init_slots()
> > and check it here and also use it to calculate the force across contacts
> > in input_mt_sync_frame().
> >
> > Or did I misunderstand the point?
> >
> I would say you understood it correctly, though to my mind it is not a
> static behaviour,

It should be, otherwise how will userspace know the meaning of the
event?

> i.e. we may want to switch this kind of calculation on and off.
> Are flags intended to be modified at runtime?

No.

> For instance, if user decides to remove the release or press effect (previously
> uploaded by them) and there is no default one per device, then we should switch
> the haptic handling from kernel mode back to device mode.

Why? I think if user removes effects then they do not want to have
haptics effects. I am wondering if this whole thing made too complex.

In my mind we have following cases:

- OS does not know about these haptics devices (touchpads). They work in
  device (?) mode and provide haptic feedback on their own.

- OS does know about haptics devices (that includes having both kernel
  *and* userspace support for them. If one is missing then the other
  should not be enabled, it is up to the distro to make sure all pieces
  are there). In this case OS controls haptics effects all the time,
  except:

- OS supports haptics, but switched it to device mode to allow haptics
  effect playback when waking up.

> Currently it
> also means
> that the driver stops generating ABS_PRESSURE events on its own and so
> input-mt layer may/should be used again (i.e. mt report pointer emulation).
> Anyhow, if it would be actually better to calculate the true force in
> input_mt_sync_frame()/input_mt_report_pointer_emulation()

Thanks.

-- 
Dmitry

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Angela Czubak]

On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > Hi Dmitry,
> >
> > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > Hi Angela,
> > >
> > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > themselves from ABS_MT_PRESSURE.
> > >
> > > This needs better explanation for why it is needed. I assume this is to
> > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > then I believe we should define a new flag for input_mt_init_slots()
> > > and check it here and also use it to calculate the force across contacts
> > > in input_mt_sync_frame().
> > >
> > > Or did I misunderstand the point?
> > >
> > I would say you understood it correctly, though to my mind it is not a
> > static behaviour,
>
> It should be, otherwise how will userspace know the meaning of the
> event?
>
Fair point.

> > i.e. we may want to switch this kind of calculation on and off.
> > Are flags intended to be modified at runtime?
>
> No.
>
> > For instance, if user decides to remove the release or press effect (previously
> > uploaded by them) and there is no default one per device, then we should switch
> > the haptic handling from kernel mode back to device mode.
>
> Why? I think if user removes effects then they do not want to have
> haptics effects. I am wondering if this whole thing made too complex.
>
> In my mind we have following cases:
>
> - OS does not know about these haptics devices (touchpads). They work in
>   device (?) mode and provide haptic feedback on their own.
>
> - OS does know about haptics devices (that includes having both kernel
>   *and* userspace support for them. If one is missing then the other
>   should not be enabled, it is up to the distro to make sure all pieces
>   are there). In this case OS controls haptics effects all the time,
>   except:
>
> - OS supports haptics, but switched it to device mode to allow haptics
>   effect playback when waking up.
>
Perhaps switching between modes should be a separate discussion.
Right now it seems to me that your suggestion could be that if
INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
every ABS_PRESSURE event should actually be a sum of pressures/true forces
across all slots. Does it sound right?
If so, I suppose I will implement it. It should be completely independent from
device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
the pressure sum still gets calculated.

Sean, is it OK for the device to keep kernel mode in the event no
default press/release
waveform is defined in the waveform list and the user removes relevant effects
(after having uploaded them)? I think it was desired to remain in the
device mode
if no such waveforms/effects are defined and, thus, I assumed that removing
following effects (in case no press/release waveforms in the waveform
list) should
trigger coming back to device mode.
Right now it seems that switching back to device mode should be
allowed only when
suspending the device.

Now, the question would be where BTN_LEFT events should be generated.
Normally it happens in hid-multitouch and I override it in hid-haptic.c
This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
Does anyone mind such behaviour?

> > Currently it
> > also means
> > that the driver stops generating ABS_PRESSURE events on its own and so
> > input-mt layer may/should be used again (i.e. mt report pointer emulation).
> > Anyhow, if it would be actually better to calculate the true force in
> > input_mt_sync_frame()/input_mt_report_pointer_emulation()
>
(I suppose I wanted to say I would implement it in such case)

> Thanks.
>
> --
> Dmitry

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Sean O'Brien]

On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
>
> On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > Hi Dmitry,
> > >
> > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > Hi Angela,
> > > >
> > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > themselves from ABS_MT_PRESSURE.
> > > >
> > > > This needs better explanation for why it is needed. I assume this is to
> > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > and check it here and also use it to calculate the force across contacts
> > > > in input_mt_sync_frame().
> > > >
> > > > Or did I misunderstand the point?
> > > >
> > > I would say you understood it correctly, though to my mind it is not a
> > > static behaviour,
> >
> > It should be, otherwise how will userspace know the meaning of the
> > event?
> >
> Fair point.
>
> > > i.e. we may want to switch this kind of calculation on and off.
> > > Are flags intended to be modified at runtime?
> >
> > No.
> >
> > > For instance, if user decides to remove the release or press effect (previously
> > > uploaded by them) and there is no default one per device, then we should switch
> > > the haptic handling from kernel mode back to device mode.
> >
> > Why? I think if user removes effects then they do not want to have
> > haptics effects. I am wondering if this whole thing made too complex.
> >
> > In my mind we have following cases:
> >
> > - OS does not know about these haptics devices (touchpads). They work in
> >   device (?) mode and provide haptic feedback on their own.
> >
> > - OS does know about haptics devices (that includes having both kernel
> >   *and* userspace support for them. If one is missing then the other
> >   should not be enabled, it is up to the distro to make sure all pieces
> >   are there). In this case OS controls haptics effects all the time,
> >   except:
> >
> > - OS supports haptics, but switched it to device mode to allow haptics
> >   effect playback when waking up.
> >
> Perhaps switching between modes should be a separate discussion.
> Right now it seems to me that your suggestion could be that if
> INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> every ABS_PRESSURE event should actually be a sum of pressures/true forces
> across all slots. Does it sound right?
> If so, I suppose I will implement it. It should be completely independent from
> device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> the pressure sum still gets calculated.
>
> Sean, is it OK for the device to keep kernel mode in the event no
> default press/release
> waveform is defined in the waveform list and the user removes relevant effects
> (after having uploaded them)? I think it was desired to remain in the
> device mode
> if no such waveforms/effects are defined and, thus, I assumed that removing
> following effects (in case no press/release waveforms in the waveform
> list) should
> trigger coming back to device mode.
> Right now it seems that switching back to device mode should be
> allowed only when
> suspending the device.

I agree that we should switch to device-controlled mode if press/release are
not defined by the device, and userspace has not supplied alternative
waveforms for either. If we kept it in kernel-controlled mode, there would be
no effect for click/release. This can be achieved by userspace by emitting
EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.

This also allows for the case where userspace may want to send haptics for UX
effects, while still relying on the device for traditional press and release
haptics (in the case where the device doesn't define press/release
waveforms).
>
> Now, the question would be where BTN_LEFT events should be generated.
> Normally it happens in hid-multitouch and I override it in hid-haptic.c
> This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
> Does anyone mind such behaviour?
>
> > > Currently it
> > > also means
> > > that the driver stops generating ABS_PRESSURE events on its own and so
> > > input-mt layer may/should be used again (i.e. mt report pointer emulation).
> > > Anyhow, if it would be actually better to calculate the true force in
> > > input_mt_sync_frame()/input_mt_report_pointer_emulation()
> >
> (I suppose I wanted to say I would implement it in such case)
>
> > Thanks.
> >
> > --
> > Dmitry

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Dmitry Torokhov]

On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> >
> > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > >
> > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > Hi Dmitry,
> > > >
> > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >
> > > > > Hi Angela,
> > > > >
> > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > themselves from ABS_MT_PRESSURE.
> > > > >
> > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > and check it here and also use it to calculate the force across contacts
> > > > > in input_mt_sync_frame().
> > > > >
> > > > > Or did I misunderstand the point?
> > > > >
> > > > I would say you understood it correctly, though to my mind it is not a
> > > > static behaviour,
> > >
> > > It should be, otherwise how will userspace know the meaning of the
> > > event?
> > >
> > Fair point.
> >
> > > > i.e. we may want to switch this kind of calculation on and off.
> > > > Are flags intended to be modified at runtime?
> > >
> > > No.
> > >
> > > > For instance, if user decides to remove the release or press effect (previously
> > > > uploaded by them) and there is no default one per device, then we should switch
> > > > the haptic handling from kernel mode back to device mode.
> > >
> > > Why? I think if user removes effects then they do not want to have
> > > haptics effects. I am wondering if this whole thing made too complex.
> > >
> > > In my mind we have following cases:
> > >
> > > - OS does not know about these haptics devices (touchpads). They work in
> > >   device (?) mode and provide haptic feedback on their own.
> > >
> > > - OS does know about haptics devices (that includes having both kernel
> > >   *and* userspace support for them. If one is missing then the other
> > >   should not be enabled, it is up to the distro to make sure all pieces
> > >   are there). In this case OS controls haptics effects all the time,
> > >   except:
> > >
> > > - OS supports haptics, but switched it to device mode to allow haptics
> > >   effect playback when waking up.
> > >
> > Perhaps switching between modes should be a separate discussion.
> > Right now it seems to me that your suggestion could be that if
> > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > across all slots. Does it sound right?
> > If so, I suppose I will implement it. It should be completely independent from
> > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > the pressure sum still gets calculated.

I'd say that if hid_haptic_init() fails we should not say that the
device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
the device instantiation, which we probably should not).

> >
> > Sean, is it OK for the device to keep kernel mode in the event no
> > default press/release
> > waveform is defined in the waveform list and the user removes relevant effects
> > (after having uploaded them)? I think it was desired to remain in the
> > device mode
> > if no such waveforms/effects are defined and, thus, I assumed that removing
> > following effects (in case no press/release waveforms in the waveform
> > list) should
> > trigger coming back to device mode.
> > Right now it seems that switching back to device mode should be
> > allowed only when
> > suspending the device.
> 
> I agree that we should switch to device-controlled mode if press/release are
> not defined by the device, and userspace has not supplied alternative
> waveforms for either. If we kept it in kernel-controlled mode, there would be
> no effect for click/release. This can be achieved by userspace by emitting
> EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.

What is wrong for not having effect for press/release if userspace did
not bother to set it up? I think this is reasonably to expect that if
user enabled support for haptic touchpad in kernel they should also have
userspace that knows how to handle it. If we go with this requirement I
think we will reduce a lot of complexity.

Benjamin, Jiri, Peter, I'd like you to chime in please.

> 
> This also allows for the case where userspace may want to send haptics for UX
> effects, while still relying on the device for traditional press and release
> haptics (in the case where the device doesn't define press/release
> waveforms).

Again, what is the difference between press/release and other UX
effects? They seem to be the same to me...

> >
> > Now, the question would be where BTN_LEFT events should be generated.
> > Normally it happens in hid-multitouch and I override it in hid-haptic.c
> > This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
> > Does anyone mind such behaviour?
> >
> > > > Currently it
> > > > also means
> > > > that the driver stops generating ABS_PRESSURE events on its own and so
> > > > input-mt layer may/should be used again (i.e. mt report pointer emulation).
> > > > Anyhow, if it would be actually better to calculate the true force in
> > > > input_mt_sync_frame()/input_mt_report_pointer_emulation()
> > >
> > (I suppose I wanted to say I would implement it in such case)

Thanks.

-- 
Dmitry

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Angela Czubak]

On Wed, Jan 12, 2022 at 3:52 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > >
> > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > Hi Dmitry,
> > > > >
> > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > >
> > > > > > Hi Angela,
> > > > > >
> > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > >
> > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > in input_mt_sync_frame().
> > > > > >
> > > > > > Or did I misunderstand the point?
> > > > > >
> > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > static behaviour,
> > > >
> > > > It should be, otherwise how will userspace know the meaning of the
> > > > event?
> > > >
> > > Fair point.
> > >
> > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > Are flags intended to be modified at runtime?
> > > >
> > > > No.
> > > >
> > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > the haptic handling from kernel mode back to device mode.
> > > >
> > > > Why? I think if user removes effects then they do not want to have
> > > > haptics effects. I am wondering if this whole thing made too complex.
> > > >
> > > > In my mind we have following cases:
> > > >
> > > > - OS does not know about these haptics devices (touchpads). They work in
> > > >   device (?) mode and provide haptic feedback on their own.
> > > >
> > > > - OS does know about haptics devices (that includes having both kernel
> > > >   *and* userspace support for them. If one is missing then the other
> > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > >   are there). In this case OS controls haptics effects all the time,
> > > >   except:
> > > >
> > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > >   effect playback when waking up.
> > > >
> > > Perhaps switching between modes should be a separate discussion.
> > > Right now it seems to me that your suggestion could be that if
> > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > across all slots. Does it sound right?
> > > If so, I suppose I will implement it. It should be completely independent from
> > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > the pressure sum still gets calculated.
>
> I'd say that if hid_haptic_init() fails we should not say that the
> device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> the device instantiation, which we probably should not).
>
It would still be a haptic touchpad, I suppose; the fact that
initialization failed isn't going
to do anything with the device, really, rather some failure in memory
allocation etc.
If you are worried about how the userspace can be sure that
initialization succeeded,
if hid_haptic_init() succeeds then the device should report EV_FF
events, otherwise not.
From my point of view it would be also somewhat unnecessary for
hid_haptic_init()
failure to break device instantiation as it would mean that the touchpad gets
nonfunctional.
Please let me know if there are any known obstacles that should discourage us
from using the device after failed hid_haptic_init() or you have some
more thoughts on
INPUT_PROP_HAPTIC_TOUCHPAD.
> > >
> > > Sean, is it OK for the device to keep kernel mode in the event no
> > > default press/release
> > > waveform is defined in the waveform list and the user removes relevant effects
> > > (after having uploaded them)? I think it was desired to remain in the
> > > device mode
> > > if no such waveforms/effects are defined and, thus, I assumed that removing
> > > following effects (in case no press/release waveforms in the waveform
> > > list) should
> > > trigger coming back to device mode.
> > > Right now it seems that switching back to device mode should be
> > > allowed only when
> > > suspending the device.
> >
> > I agree that we should switch to device-controlled mode if press/release are
> > not defined by the device, and userspace has not supplied alternative
> > waveforms for either. If we kept it in kernel-controlled mode, there would be
> > no effect for click/release. This can be achieved by userspace by emitting
> > EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
>
> What is wrong for not having effect for press/release if userspace did
> not bother to set it up? I think this is reasonably to expect that if
> user enabled support for haptic touchpad in kernel they should also have
> userspace that knows how to handle it. If we go with this requirement I
> think we will reduce a lot of complexity.
>
> Benjamin, Jiri, Peter, I'd like you to chime in please.
>
Dmitry, just to be sure, are you suggesting that kernel should not
trigger haptic feedback
at all as well?
I suppose most of the cases a haptic device will supply waveforms for
press and release in
its waveformlist (exposed in autotrigger report) and so the kernel
will use them.
I am just mentioning a case where switching back to device/autonomous
mode would be
somehow justifiable.
> >
> > This also allows for the case where userspace may want to send haptics for UX
> > effects, while still relying on the device for traditional press and release
> > haptics (in the case where the device doesn't define press/release
> > waveforms).
>
> Again, what is the difference between press/release and other UX
> effects? They seem to be the same to me...
>
I suppose the userspace may want the user to feel a different
sensation at times.
For instance, let's say that user tries to click on a button/switch
that is blocked for them
(perhaps this option is not allowed by their admin?). The userspace
may want to trigger
a different haptic effect to let them know in yet another way that
their actions haven't
succeeded, something is prohibited etc.
Sean, you may have some better real-life applications, perhaps it
should be you to
explain this point more clearly.
> > >
> > > Now, the question would be where BTN_LEFT events should be generated.
> > > Normally it happens in hid-multitouch and I override it in hid-haptic.c
> > > This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
> > > Does anyone mind such behaviour?
> > >
> > > > > Currently it
> > > > > also means
> > > > > that the driver stops generating ABS_PRESSURE events on its own and so
> > > > > input-mt layer may/should be used again (i.e. mt report pointer emulation).
> > > > > Anyhow, if it would be actually better to calculate the true force in
> > > > > input_mt_sync_frame()/input_mt_report_pointer_emulation()
> > > >
> > > (I suppose I wanted to say I would implement it in such case)
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Benjamin Tissoires]

On Wed, Jan 12, 2022 at 3:52 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > >
> > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > Hi Dmitry,
> > > > >
> > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > >
> > > > > > Hi Angela,
> > > > > >
> > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > >
> > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > in input_mt_sync_frame().
> > > > > >
> > > > > > Or did I misunderstand the point?
> > > > > >
> > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > static behaviour,
> > > >
> > > > It should be, otherwise how will userspace know the meaning of the
> > > > event?
> > > >
> > > Fair point.
> > >
> > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > Are flags intended to be modified at runtime?
> > > >
> > > > No.
> > > >
> > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > the haptic handling from kernel mode back to device mode.
> > > >
> > > > Why? I think if user removes effects then they do not want to have
> > > > haptics effects. I am wondering if this whole thing made too complex.
> > > >
> > > > In my mind we have following cases:
> > > >
> > > > - OS does not know about these haptics devices (touchpads). They work in
> > > >   device (?) mode and provide haptic feedback on their own.
> > > >
> > > > - OS does know about haptics devices (that includes having both kernel
> > > >   *and* userspace support for them. If one is missing then the other
> > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > >   are there). In this case OS controls haptics effects all the time,
> > > >   except:
> > > >
> > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > >   effect playback when waking up.
> > > >
> > > Perhaps switching between modes should be a separate discussion.
> > > Right now it seems to me that your suggestion could be that if
> > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > across all slots. Does it sound right?
> > > If so, I suppose I will implement it. It should be completely independent from
> > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > the pressure sum still gets calculated.
>
> I'd say that if hid_haptic_init() fails we should not say that the
> device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> the device instantiation, which we probably should not).

Agree. Userspace should know that the device is a pressure pad based
on the unit provided in ABS_MT_PRESSURE IIRC.
So setting the resolution is enough for userspace to emulate the
button clicks based on the pressure. libinput already has code for
that.

So basically, INPUT_PROP_HAPTIC_TOUCHPAD is only an indication that
the haptic is configurable. And if haptic_init() fails, it should not
expose that property.

And BTW, why "TOUCHPAD" in INPUT_PROP_HAPTIC_TOUCHPAD? The Surface
Dial could benefit from that implementation and it is not a
touchpad...

>
> > >
> > > Sean, is it OK for the device to keep kernel mode in the event no
> > > default press/release
> > > waveform is defined in the waveform list and the user removes relevant effects
> > > (after having uploaded them)? I think it was desired to remain in the
> > > device mode
> > > if no such waveforms/effects are defined and, thus, I assumed that removing
> > > following effects (in case no press/release waveforms in the waveform
> > > list) should
> > > trigger coming back to device mode.
> > > Right now it seems that switching back to device mode should be
> > > allowed only when
> > > suspending the device.
> >
> > I agree that we should switch to device-controlled mode if press/release are
> > not defined by the device, and userspace has not supplied alternative
> > waveforms for either. If we kept it in kernel-controlled mode, there would be
> > no effect for click/release. This can be achieved by userspace by emitting
> > EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
>
> What is wrong for not having effect for press/release if userspace did
> not bother to set it up? I think this is reasonably to expect that if
> user enabled support for haptic touchpad in kernel they should also have
> userspace that knows how to handle it. If we go with this requirement I
> think we will reduce a lot of complexity.
>
> Benjamin, Jiri, Peter, I'd like you to chime in please.

[FWIW, lei saved me on this one for not being Cc-ed since the
beginning of this thread]

I think we should keep it simple:
- the device configuration should be static (i.e.
ABS_PRESSURE/ABS_MT_PRESSURE, pointer emulation, button emulation,
...) always present
- userspace should pick up what it needs based on its own state:
  if there is a need to compute a total pressure, userspace is capable
of computing itself, and generates its own button press/release
- the haptic is a global state of the device, so any decision you make
is going to have corner cases with more than one userspace (or if the
userspace daemon/lib is restarted)

So to me, we should keep the kernel device emulation, export what
needs to be for userspace to make its own decision and have the haptic
side as a "nice to have" feature but distinct from the event
processing.

I didn't want to chime into this thread because I am currently working
on 2 big series that might also be helpful here:
- the first one, which is almost ready, consists in rethinking how the
HID events are processed, meaning we can ensure that some events are
always processed before others. The net benefit is that I can now
express the Win8 multitouch protocol in hid-generic without too much
pain, meaning that hid-haptic.c could be a leaf driver instead of
being an API.
The net benefit of not having hid-haptic.c as an API is that we can
always rmmod it to disable the entire haptic system if there is
something wrong.

- the second one is the eBPF bindings for HID (see
https://lore.kernel.org/all/20211215134220.1735144-1-tero.kristo@linux.intel.com/
and the other versions for some more discussions)
Basically BPF allows to avoid specific kernel APIs and userspace is in
charge of loading the bridge between its API and the device. It
definitely has the potential to solve many limitations we are seeing
now in all the various input/ff protocols IMO.

>
> >
> > This also allows for the case where userspace may want to send haptics for UX
> > effects, while still relying on the device for traditional press and release
> > haptics (in the case where the device doesn't define press/release
> > waveforms).
>
> Again, what is the difference between press/release and other UX
> effects? They seem to be the same to me...
>
> > >
> > > Now, the question would be where BTN_LEFT events should be generated.
> > > Normally it happens in hid-multitouch and I override it in hid-haptic.c
> > > This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
> > > Does anyone mind such behaviour?

Again, why is there a need to have some complex behavior there? Just
let userspace do its own fancy computation and keep it simple in the
kernel.
Well, with eBPF, you could let userspace put the BTN_LEFT emulation in
the kernel by loading a specific program, but that would be in charge
of the userspace to make this choice, not the kernel.

Cheers,
Benjamin

> > >
> > > > > Currently it
> > > > > also means
> > > > > that the driver stops generating ABS_PRESSURE events on its own and so
> > > > > input-mt layer may/should be used again (i.e. mt report pointer emulation).
> > > > > Anyhow, if it would be actually better to calculate the true force in
> > > > > input_mt_sync_frame()/input_mt_report_pointer_emulation()
> > > >
> > > (I suppose I wanted to say I would implement it in such case)
>
> Thanks.
>
> --
> Dmitry
>

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Angela Czubak]

On Wed, Jan 12, 2022 at 10:17 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Jan 12, 2022 at 3:52 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > > >
> > > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > > Hi Dmitry,
> > > > > >
> > > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Angela,
> > > > > > >
> > > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > > >
> > > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > > in input_mt_sync_frame().
> > > > > > >
> > > > > > > Or did I misunderstand the point?
> > > > > > >
> > > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > > static behaviour,
> > > > >
> > > > > It should be, otherwise how will userspace know the meaning of the
> > > > > event?
> > > > >
> > > > Fair point.
> > > >
> > > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > > Are flags intended to be modified at runtime?
> > > > >
> > > > > No.
> > > > >
> > > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > > the haptic handling from kernel mode back to device mode.
> > > > >
> > > > > Why? I think if user removes effects then they do not want to have
> > > > > haptics effects. I am wondering if this whole thing made too complex.
> > > > >
> > > > > In my mind we have following cases:
> > > > >
> > > > > - OS does not know about these haptics devices (touchpads). They work in
> > > > >   device (?) mode and provide haptic feedback on their own.
> > > > >
> > > > > - OS does know about haptics devices (that includes having both kernel
> > > > >   *and* userspace support for them. If one is missing then the other
> > > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > > >   are there). In this case OS controls haptics effects all the time,
> > > > >   except:
> > > > >
> > > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > > >   effect playback when waking up.
> > > > >
> > > > Perhaps switching between modes should be a separate discussion.
> > > > Right now it seems to me that your suggestion could be that if
> > > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > > across all slots. Does it sound right?
> > > > If so, I suppose I will implement it. It should be completely independent from
> > > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > > the pressure sum still gets calculated.
> >
> > I'd say that if hid_haptic_init() fails we should not say that the
> > device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> > the device instantiation, which we probably should not).
>
> Agree. Userspace should know that the device is a pressure pad based
> on the unit provided in ABS_MT_PRESSURE IIRC.
> So setting the resolution is enough for userspace to emulate the
> button clicks based on the pressure. libinput already has code for
> that.
>

A quick glance [1] and it seems that libinput chooses to ignore ABS_MT_PRESSURE
it the resolution is non-zero (though I might have been looking in a
wrong place).
Mentioning just because someone might lead me to a proper place/library actually
using ABS_MT_PRESSURE as force.

> So basically, INPUT_PROP_HAPTIC_TOUCHPAD is only an indication that
> the haptic is configurable. And if haptic_init() fails, it should not
> expose that property.
>
> And BTW, why "TOUCHPAD" in INPUT_PROP_HAPTIC_TOUCHPAD? The Surface
> Dial could benefit from that implementation and it is not a
> touchpad...
>

Ok, so looking back at the old discussion it seems to me that the
property originally
suggested is INPUT_PROP_FORCEPAD and it was initially intended to mean that
ABS_MT_PRESSURE events should be interpreted as force and not area/pressure.
In case units are grams or newtons I calculate the resolution, but it
seems that Peter
has previously stated it is not enough:

>And we can't just assume "if resolution is set, units are $foo" because
>nothing written in the last decade or so will assume that. Some extra flag
>is needed, like INPUT_PROP_FORCEPAD.

I think Benjamin originally suggested this flag so that userspace knows
ABS_MT_PRESSURE should mean force.

However, it was a very long time ago. It seems that about a year ago it was
defined that non-zero pressure resolution means units/grams is used.

It seems to me that we could assume that reporting FF_HID events implicates
how ABS_MT_PRESSURE should be interpreted, so I could get rid of this
flag, if that is what you prefer.

> >
> > > >
> > > > Sean, is it OK for the device to keep kernel mode in the event no
> > > > default press/release
> > > > waveform is defined in the waveform list and the user removes relevant effects
> > > > (after having uploaded them)? I think it was desired to remain in the
> > > > device mode
> > > > if no such waveforms/effects are defined and, thus, I assumed that removing
> > > > following effects (in case no press/release waveforms in the waveform
> > > > list) should
> > > > trigger coming back to device mode.
> > > > Right now it seems that switching back to device mode should be
> > > > allowed only when
> > > > suspending the device.
> > >
> > > I agree that we should switch to device-controlled mode if press/release are
> > > not defined by the device, and userspace has not supplied alternative
> > > waveforms for either. If we kept it in kernel-controlled mode, there would be
> > > no effect for click/release. This can be achieved by userspace by emitting
> > > EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
> >
> > What is wrong for not having effect for press/release if userspace did
> > not bother to set it up? I think this is reasonably to expect that if
> > user enabled support for haptic touchpad in kernel they should also have
> > userspace that knows how to handle it. If we go with this requirement I
> > think we will reduce a lot of complexity.
> >
> > Benjamin, Jiri, Peter, I'd like you to chime in please.
>
> [FWIW, lei saved me on this one for not being Cc-ed since the
> beginning of this thread]
>
> I think we should keep it simple:
> - the device configuration should be static (i.e.
> ABS_PRESSURE/ABS_MT_PRESSURE, pointer emulation, button emulation,
> ...) always present
> - userspace should pick up what it needs based on its own state:
>   if there is a need to compute a total pressure, userspace is capable
> of computing itself, and generates its own button press/release
> - the haptic is a global state of the device, so any decision you make
> is going to have corner cases with more than one userspace (or if the
> userspace daemon/lib is restarted)
>
> So to me, we should keep the kernel device emulation, export what
> needs to be for userspace to make its own decision and have the haptic
> side as a "nice to have" feature but distinct from the event
> processing.
>
> I didn't want to chime into this thread because I am currently working
> on 2 big series that might also be helpful here:
> - the first one, which is almost ready, consists in rethinking how the
> HID events are processed, meaning we can ensure that some events are
> always processed before others. The net benefit is that I can now
> express the Win8 multitouch protocol in hid-generic without too much
> pain, meaning that hid-haptic.c could be a leaf driver instead of
> being an API.
> The net benefit of not having hid-haptic.c as an API is that we can
> always rmmod it to disable the entire haptic system if there is
> something wrong.
>
> - the second one is the eBPF bindings for HID (see
> https://lore.kernel.org/all/20211215134220.1735144-1-tero.kristo@linux.intel.com/
> and the other versions for some more discussions)
> Basically BPF allows to avoid specific kernel APIs and userspace is in
> charge of loading the bridge between its API and the device. It
> definitely has the potential to solve many limitations we are seeing
> now in all the various input/ff protocols IMO.
>
> >
> > >
> > > This also allows for the case where userspace may want to send haptics for UX
> > > effects, while still relying on the device for traditional press and release
> > > haptics (in the case where the device doesn't define press/release
> > > waveforms).
> >
> > Again, what is the difference between press/release and other UX
> > effects? They seem to be the same to me...
> >
> > > >
> > > > Now, the question would be where BTN_LEFT events should be generated.
> > > > Normally it happens in hid-multitouch and I override it in hid-haptic.c
> > > > This means I calculate the pressure sum as well in hid-haptic/hid-multitouch.
> > > > Does anyone mind such behaviour?
>
> Again, why is there a need to have some complex behavior there? Just
> let userspace do its own fancy computation and keep it simple in the
> kernel.

I thought it was requested based on the following discussion [2]:

>>
>> ABS_PRESSURE may be optionally reported as the total force applied to the
>> forcepad.
>>
>> The device/driver shouldn’t detect button clicks, this is left to the userspace
>> gesture library. Accordingly, the driver should not sent BTN_* events to
>> userspace in normal operating mode. However it should still report the ability
>> to produce such events, for use in autonomous mode.
>
> For backward compatibility, and to be able to debug it properly, you
> should keep the BTN_* events emulated in all cases.
> The userspace can ignore the events it doesn't want this way, but you
> will be able to debug the btn emulations on your current session
> without having to kill your compositor.
> There shouldn't be much of a head over forwarding those events, as it
> will never come alone, and will always be with an other one at least
> (pressure being 0 or less).
>
> Also, not sending BTN_TOUCH and BTN_LEFT might give some headaches to
> legacy applications.

I can remove such behaviour if I misunderstood or it is no longer valid.

> Well, with eBPF, you could let userspace put the BTN_LEFT emulation in
> the kernel by loading a specific program, but that would be in charge
> of the userspace to make this choice, not the kernel.
>
> Cheers,
> Benjamin
>
> > > >
> > > > > > Currently it
> > > > > > also means
> > > > > > that the driver stops generating ABS_PRESSURE events on its own and so
> > > > > > input-mt layer may/should be used again (i.e. mt report pointer emulation).
> > > > > > Anyhow, if it would be actually better to calculate the true force in
> > > > > > input_mt_sync_frame()/input_mt_report_pointer_emulation()
> > > > >
> > > > (I suppose I wanted to say I would implement it in such case)
> >
> > Thanks.
> >
> > --
> > Dmitry
> >
>
Regards,
Angela

[1] https://gitlab.freedesktop.org/libinput/libinput/-/issues/562
[2] https://www.spinics.net/lists/linux-input/msg60983.html

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Peter Hutterer]

On Tue, Jan 11, 2022 at 06:52:27PM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > >
> > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > >
> > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > Hi Dmitry,
> > > > >
> > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > >
> > > > > > Hi Angela,
> > > > > >
> > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > >
> > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > in input_mt_sync_frame().
> > > > > >
> > > > > > Or did I misunderstand the point?
> > > > > >
> > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > static behaviour,
> > > >
> > > > It should be, otherwise how will userspace know the meaning of the
> > > > event?
> > > >
> > > Fair point.
> > >
> > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > Are flags intended to be modified at runtime?
> > > >
> > > > No.
> > > >
> > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > the haptic handling from kernel mode back to device mode.
> > > >
> > > > Why? I think if user removes effects then they do not want to have
> > > > haptics effects. I am wondering if this whole thing made too complex.
> > > >
> > > > In my mind we have following cases:
> > > >
> > > > - OS does not know about these haptics devices (touchpads). They work in
> > > >   device (?) mode and provide haptic feedback on their own.
> > > >
> > > > - OS does know about haptics devices (that includes having both kernel
> > > >   *and* userspace support for them. If one is missing then the other
> > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > >   are there). In this case OS controls haptics effects all the time,
> > > >   except:
> > > >
> > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > >   effect playback when waking up.
> > > >
> > > Perhaps switching between modes should be a separate discussion.
> > > Right now it seems to me that your suggestion could be that if
> > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > across all slots. Does it sound right?
> > > If so, I suppose I will implement it. It should be completely independent from
> > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > the pressure sum still gets calculated.
> 
> I'd say that if hid_haptic_init() fails we should not say that the
> device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> the device instantiation, which we probably should not).
> 
> > >
> > > Sean, is it OK for the device to keep kernel mode in the event no
> > > default press/release
> > > waveform is defined in the waveform list and the user removes relevant effects
> > > (after having uploaded them)? I think it was desired to remain in the
> > > device mode
> > > if no such waveforms/effects are defined and, thus, I assumed that removing
> > > following effects (in case no press/release waveforms in the waveform
> > > list) should
> > > trigger coming back to device mode.
> > > Right now it seems that switching back to device mode should be
> > > allowed only when
> > > suspending the device.
> > 
> > I agree that we should switch to device-controlled mode if press/release are
> > not defined by the device, and userspace has not supplied alternative
> > waveforms for either. If we kept it in kernel-controlled mode, there would be
> > no effect for click/release. This can be achieved by userspace by emitting
> > EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
> 
> What is wrong for not having effect for press/release if userspace did
> not bother to set it up? I think this is reasonably to expect that if
> user enabled support for haptic touchpad in kernel they should also have
> userspace that knows how to handle it. If we go with this requirement I
> think we will reduce a lot of complexity.
> 
> Benjamin, Jiri, Peter, I'd like you to chime in please.
> 
> > 
> > This also allows for the case where userspace may want to send haptics for UX
> > effects, while still relying on the device for traditional press and release
> > haptics (in the case where the device doesn't define press/release
> > waveforms).
> 
> Again, what is the difference between press/release and other UX
> effects? They seem to be the same to me...

Agree with Dmitry here - have a sensible default in the kernel and if
userspace changes it, it's now userspace's problem to do it right. Anything
more complex is just making things more complicated for niche cases that may
never happen.

Cheers,
  Peter

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Angela Czubak]

Hi Peter, Dmitry, Benjamin, Sean,

On Fri, Jan 21, 2022 at 7:10 AM Peter Hutterer <peter.hutterer@who-t.net> wrote:
>
> On Tue, Jan 11, 2022 at 06:52:27PM -0800, Dmitry Torokhov wrote:
> > On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > > >
> > > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > > Hi Dmitry,
> > > > > >
> > > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > >
> > > > > > > Hi Angela,
> > > > > > >
> > > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > > >
> > > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > > in input_mt_sync_frame().
> > > > > > >
> > > > > > > Or did I misunderstand the point?
> > > > > > >
> > > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > > static behaviour,
> > > > >
> > > > > It should be, otherwise how will userspace know the meaning of the
> > > > > event?
> > > > >
> > > > Fair point.
> > > >
> > > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > > Are flags intended to be modified at runtime?
> > > > >
> > > > > No.
> > > > >
> > > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > > the haptic handling from kernel mode back to device mode.
> > > > >
> > > > > Why? I think if user removes effects then they do not want to have
> > > > > haptics effects. I am wondering if this whole thing made too complex.
> > > > >
> > > > > In my mind we have following cases:
> > > > >
> > > > > - OS does not know about these haptics devices (touchpads). They work in
> > > > >   device (?) mode and provide haptic feedback on their own.
> > > > >
> > > > > - OS does know about haptics devices (that includes having both kernel
> > > > >   *and* userspace support for them. If one is missing then the other
> > > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > > >   are there). In this case OS controls haptics effects all the time,
> > > > >   except:
> > > > >
> > > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > > >   effect playback when waking up.
> > > > >
> > > > Perhaps switching between modes should be a separate discussion.
> > > > Right now it seems to me that your suggestion could be that if
> > > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > > across all slots. Does it sound right?
> > > > If so, I suppose I will implement it. It should be completely independent from
> > > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > > the pressure sum still gets calculated.
> >
> > I'd say that if hid_haptic_init() fails we should not say that the
> > device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> > the device instantiation, which we probably should not).
> >
> > > >
> > > > Sean, is it OK for the device to keep kernel mode in the event no
> > > > default press/release
> > > > waveform is defined in the waveform list and the user removes relevant effects
> > > > (after having uploaded them)? I think it was desired to remain in the
> > > > device mode
> > > > if no such waveforms/effects are defined and, thus, I assumed that removing
> > > > following effects (in case no press/release waveforms in the waveform
> > > > list) should
> > > > trigger coming back to device mode.
> > > > Right now it seems that switching back to device mode should be
> > > > allowed only when
> > > > suspending the device.
> > >
> > > I agree that we should switch to device-controlled mode if press/release are
> > > not defined by the device, and userspace has not supplied alternative
> > > waveforms for either. If we kept it in kernel-controlled mode, there would be
> > > no effect for click/release. This can be achieved by userspace by emitting
> > > EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
> >
> > What is wrong for not having effect for press/release if userspace did
> > not bother to set it up? I think this is reasonably to expect that if
> > user enabled support for haptic touchpad in kernel they should also have
> > userspace that knows how to handle it. If we go with this requirement I
> > think we will reduce a lot of complexity.
> >
> > Benjamin, Jiri, Peter, I'd like you to chime in please.
> >
> > >
> > > This also allows for the case where userspace may want to send haptics for UX
> > > effects, while still relying on the device for traditional press and release
> > > haptics (in the case where the device doesn't define press/release
> > > waveforms).
> >
> > Again, what is the difference between press/release and other UX
> > effects? They seem to be the same to me...
>
> Agree with Dmitry here - have a sensible default in the kernel and if
> userspace changes it, it's now userspace's problem to do it right. Anything
> more complex is just making things more complicated for niche cases that may
> never happen.
>

Could you please relate to the following statements/questions? I would like to
make sure I am nearer to your understanding of how the things should be.
I wouldn't say they constitute my plan, I am just wondering if shared effects
are acceptable at all since their handling seems questionable.

1. Kernel mode - is it OK to have any default at all? Or would you rather say
   it's userspace's responsibility to issue force feedback entirely? I am just
   wondering how much simplification you would actually prefer to have.
   In the current patchset the kernel can issue haptic feedback itself
   (based on the pressure/force sums calculated).
2. The patches introduce shared effects. This allows userspace to modify
   kernel mode behaviour, i.e. the waveforms it issues when press/release
   has been detected, which means both uploading and erasing those
   effects is possible.
   On the other hand, closing event fd triggers removing effects uploaded for
   that fd. I would assume removing shared effects is allowed as well
   since we can update them with upload. Should it be disallowed/prohibited?
   I mean that perhaps erasing shared effects should never really take place
   as we may end up removing something that has not been altered by
   userspace.
   I am worried since simply opening and closing the event file could possibly
   cause a change in behaviour if we actually let effects be completely
   removed.
3. Switching to kernel mode should happen at the instantiation and then only
   during suspend/resume cycle. If the shared press/release effect gets
   removed (even caused by input device flush), then we don't want any haptic
   feedback in kernel mode anyway.
4. Should I just not care and not sum the pressures across all slots? It just
   seemed to me there was a reason to choose one slot and pass it as
   ABS_PRESSURE in input-mt.c, and I just suspected it would be more
   logical to pass the sum of forces if the unit suggests it is force.

> Cheers,
>   Peter

Re: [PATCH 13/18] Input: MT - toggle ABS_PRESSURE pointer emulation

[Peter Hutterer]

On Tue, Jan 25, 2022 at 05:56:17PM +0100, Angela Czubak wrote:
> Hi Peter, Dmitry, Benjamin, Sean,
> 
> On Fri, Jan 21, 2022 at 7:10 AM Peter Hutterer <peter.hutterer@who-t.net> wrote:
> >
> > On Tue, Jan 11, 2022 at 06:52:27PM -0800, Dmitry Torokhov wrote:
> > > On Tue, Jan 11, 2022 at 09:19:19PM -0500, Sean O'Brien wrote:
> > > > On Tue, Jan 11, 2022 at 12:07 PM Angela Czubak <acz@semihalf.com> wrote:
> > > > >
> > > > > On Mon, Jan 10, 2022 at 10:02 PM Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Jan 10, 2022 at 08:43:28PM +0100, Angela Czubak wrote:
> > > > > > > Hi Dmitry,
> > > > > > >
> > > > > > > On Fri, Jan 7, 2022 at 11:07 PM Dmitry Torokhov
> > > > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > > >
> > > > > > > > Hi Angela,
> > > > > > > >
> > > > > > > > On Tue, Dec 21, 2021 at 07:17:38PM +0000, Angela Czubak wrote:
> > > > > > > > > Add a function to switch off ABS_PRESSURE generation if necessary.
> > > > > > > > > This may be helpful in case drivers want to generate ABS_PRESSURE events
> > > > > > > > > themselves from ABS_MT_PRESSURE.
> > > > > > > >
> > > > > > > > This needs better explanation for why it is needed. I assume this is to
> > > > > > > > use ABS_PRESSURE to report "true force" for devices. If this is correct
> > > > > > > > then I believe we should define a new flag for input_mt_init_slots()
> > > > > > > > and check it here and also use it to calculate the force across contacts
> > > > > > > > in input_mt_sync_frame().
> > > > > > > >
> > > > > > > > Or did I misunderstand the point?
> > > > > > > >
> > > > > > > I would say you understood it correctly, though to my mind it is not a
> > > > > > > static behaviour,
> > > > > >
> > > > > > It should be, otherwise how will userspace know the meaning of the
> > > > > > event?
> > > > > >
> > > > > Fair point.
> > > > >
> > > > > > > i.e. we may want to switch this kind of calculation on and off.
> > > > > > > Are flags intended to be modified at runtime?
> > > > > >
> > > > > > No.
> > > > > >
> > > > > > > For instance, if user decides to remove the release or press effect (previously
> > > > > > > uploaded by them) and there is no default one per device, then we should switch
> > > > > > > the haptic handling from kernel mode back to device mode.
> > > > > >
> > > > > > Why? I think if user removes effects then they do not want to have
> > > > > > haptics effects. I am wondering if this whole thing made too complex.
> > > > > >
> > > > > > In my mind we have following cases:
> > > > > >
> > > > > > - OS does not know about these haptics devices (touchpads). They work in
> > > > > >   device (?) mode and provide haptic feedback on their own.
> > > > > >
> > > > > > - OS does know about haptics devices (that includes having both kernel
> > > > > >   *and* userspace support for them. If one is missing then the other
> > > > > >   should not be enabled, it is up to the distro to make sure all pieces
> > > > > >   are there). In this case OS controls haptics effects all the time,
> > > > > >   except:
> > > > > >
> > > > > > - OS supports haptics, but switched it to device mode to allow haptics
> > > > > >   effect playback when waking up.
> > > > > >
> > > > > Perhaps switching between modes should be a separate discussion.
> > > > > Right now it seems to me that your suggestion could be that if
> > > > > INPUT_PROP_HAPTIC_TOUCHPAD is set it should be followed by setting
> > > > > something like INPUT_MT_PRESSURE_SUM in mt_flags, which should mean
> > > > > every ABS_PRESSURE event should actually be a sum of pressures/true forces
> > > > > across all slots. Does it sound right?
> > > > > If so, I suppose I will implement it. It should be completely independent from
> > > > > device/kernel mode and, what is more, if hid_haptic_init() fails for any reason
> > > > > the pressure sum still gets calculated.
> > >
> > > I'd say that if hid_haptic_init() fails we should not say that the
> > > device is INPUT_PROP_HAPTIC_TOUCHPAD (if we even decide to continue with
> > > the device instantiation, which we probably should not).
> > >
> > > > >
> > > > > Sean, is it OK for the device to keep kernel mode in the event no
> > > > > default press/release
> > > > > waveform is defined in the waveform list and the user removes relevant effects
> > > > > (after having uploaded them)? I think it was desired to remain in the
> > > > > device mode
> > > > > if no such waveforms/effects are defined and, thus, I assumed that removing
> > > > > following effects (in case no press/release waveforms in the waveform
> > > > > list) should
> > > > > trigger coming back to device mode.
> > > > > Right now it seems that switching back to device mode should be
> > > > > allowed only when
> > > > > suspending the device.
> > > >
> > > > I agree that we should switch to device-controlled mode if press/release are
> > > > not defined by the device, and userspace has not supplied alternative
> > > > waveforms for either. If we kept it in kernel-controlled mode, there would be
> > > > no effect for click/release. This can be achieved by userspace by emitting
> > > > EVIOCFFTAKECONTROL for click and release, and never sending haptic commands.
> > >
> > > What is wrong for not having effect for press/release if userspace did
> > > not bother to set it up? I think this is reasonably to expect that if
> > > user enabled support for haptic touchpad in kernel they should also have
> > > userspace that knows how to handle it. If we go with this requirement I
> > > think we will reduce a lot of complexity.
> > >
> > > Benjamin, Jiri, Peter, I'd like you to chime in please.
> > >
> > > >
> > > > This also allows for the case where userspace may want to send haptics for UX
> > > > effects, while still relying on the device for traditional press and release
> > > > haptics (in the case where the device doesn't define press/release
> > > > waveforms).
> > >
> > > Again, what is the difference between press/release and other UX
> > > effects? They seem to be the same to me...
> >
> > Agree with Dmitry here - have a sensible default in the kernel and if
> > userspace changes it, it's now userspace's problem to do it right. Anything
> > more complex is just making things more complicated for niche cases that may
> > never happen.
> >
> 
> Could you please relate to the following statements/questions? I would like to
> make sure I am nearer to your understanding of how the things should be.
> I wouldn't say they constitute my plan, I am just wondering if shared effects
> are acceptable at all since their handling seems questionable.
> 
> 1. Kernel mode - is it OK to have any default at all? Or would you rather say
>    it's userspace's responsibility to issue force feedback entirely? I am just
>    wondering how much simplification you would actually prefer to have.
>    In the current patchset the kernel can issue haptic feedback itself
>    (based on the pressure/force sums calculated).

IMO the kernel should have a default. without any force feedback the devices
are going to be difficult to use.

> 2. The patches introduce shared effects. This allows userspace to modify
>    kernel mode behaviour, i.e. the waveforms it issues when press/release
>    has been detected, which means both uploading and erasing those
>    effects is possible.
>    On the other hand, closing event fd triggers removing effects uploaded for
>    that fd. I would assume removing shared effects is allowed as well
>    since we can update them with upload. Should it be disallowed/prohibited?
>    I mean that perhaps erasing shared effects should never really take place
>    as we may end up removing something that has not been altered by
>    userspace.
>    I am worried since simply opening and closing the event file could possibly
>    cause a change in behaviour if we actually let effects be completely
>    removed.

if you remove effects on fd close, you're effectively enforcing that some
process always needs to keep the fd to this device open just to have it
respond correctly. It may also prevent the device from going to sleep, right?

So I think there's an argument to leaving the configured waveforms there after
closing the effect - similar things already happen with other evdev ioctls.

> 3. Switching to kernel mode should happen at the instantiation and then only
>    during suspend/resume cycle. If the shared press/release effect gets
>    removed (even caused by input device flush), then we don't want any haptic
>    feedback in kernel mode anyway.
> 4. Should I just not care and not sum the pressures across all slots? It just
>    seemed to me there was a reason to choose one slot and pass it as
>    ABS_PRESSURE in input-mt.c, and I just suspected it would be more
>    logical to pass the sum of forces if the unit suggests it is force.

I'm not quite keeping up with the details in the patches but - whatever is the
most accurate physical measurement? :)

Cheers,
  Peter

[PATCH 14/18] HID: haptic: add hid_haptic_switch_mode

[Angela Czubak]

Function hid_haptic_switch_mode() can be used to turn off and on the
autonomoums mode for the device. If the device supports press and release
waveforms, let the kernel handle generation of haptic feedback instead of
the device itself.
Implement hid_haptic_resume() and hid_haptic_suspend() so that the
autonomous mode gets switched off at resume and switched on at suspend.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/hid-haptic.c | 89 +++++++++++++++++++++++++++++++++++++---
 drivers/hid/hid-haptic.h | 10 +++++
 2 files changed, 93 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
index 85c4711f685e..e7fd53d1a7c1 100644
--- a/drivers/hid/hid-haptic.c
+++ b/drivers/hid/hid-haptic.c
@@ -8,6 +8,7 @@
 /*
  */
 
+#include <linux/input/mt.h>
 #include <linux/module.h>
 
 #include "hid-haptic.h"
@@ -200,9 +201,61 @@ static void fill_effect_buf(struct hid_haptic_device *haptic,
 	mutex_unlock(&haptic->manual_trigger_mutex);
 }
 
+static void switch_mode(struct hid_device *hdev, struct hid_haptic_device *haptic,
+			int mode)
+{
+	struct hid_report *rep = haptic->auto_trigger_report;
+	struct hid_field *field;
+	s32 value;
+	int i, j;
+
+	if (mode == HID_HAPTIC_MODE_KERNEL) {
+		value = HID_HAPTIC_ORDINAL_WAVEFORMSTOP;
+		input_mt_pressure_toggle(haptic->input_dev, 0);
+	} else {
+		value = haptic->default_auto_trigger;
+		input_mt_pressure_toggle(haptic->input_dev, 1);
+	}
+
+	mutex_lock(&haptic->auto_trigger_mutex);
+	for (i = 0; i < rep->maxfield; i++) {
+		field = rep->field[i];
+		/* Ignore if report count is out of bounds. */
+		if (field->report_count < 1)
+			continue;
+
+		for (j = 0; j < field->maxusage; j++) {
+			if (field->usage[j].hid == HID_HP_AUTOTRIGGER)
+				field->value[j] = value;
+		}
+	}
+
+	/* send the report */
+	hid_hw_request(hdev, rep, HID_REQ_SET_REPORT);
+	mutex_unlock(&haptic->auto_trigger_mutex);
+	haptic->mode = mode;
+}
+
+#ifdef CONFIG_PM
+void hid_haptic_resume(struct hid_device *hdev, struct hid_haptic_device *haptic)
+{
+	if (haptic->press_ordinal_cur && haptic->release_ordinal_cur)
+		switch_mode(hdev, haptic, HID_HAPTIC_MODE_KERNEL);
+}
+EXPORT_SYMBOL_GPL(hid_haptic_resume);
+
+void hid_haptic_suspend(struct hid_device *hdev, struct hid_haptic_device *haptic)
+{
+	if (haptic->press_ordinal_cur && haptic->release_ordinal_cur)
+		switch_mode(hdev, haptic, HID_HAPTIC_MODE_DEVICE);
+}
+EXPORT_SYMBOL_GPL(hid_haptic_suspend);
+#endif
+
 static int hid_haptic_upload_effect(struct input_dev *dev, struct ff_effect *effect,
 				    struct ff_effect *old)
 {
+	struct hid_device *hdev = input_get_drvdata(dev);
 	struct ff_device *ff = dev->ff;
 	struct hid_haptic_device *haptic = ff->private;
 	int i, ordinal = 0;
@@ -228,6 +281,20 @@ static int hid_haptic_upload_effect(struct input_dev *dev, struct ff_effect *eff
 	fill_effect_buf(haptic, &effect->u.hid, &haptic->effect[effect->id],
 			ordinal);
 
+	if (effect->id == HID_HAPTIC_RELEASE_EFFECT_ID) {
+		if (haptic->press_ordinal_cur &&
+		    haptic->mode == HID_HAPTIC_MODE_DEVICE) {
+			switch_mode(hdev, haptic, HID_HAPTIC_MODE_KERNEL);
+		}
+		haptic->release_ordinal_cur = ordinal;
+	} else if (effect->id == HID_HAPTIC_PRESS_EFFECT_ID) {
+		if (haptic->release_ordinal_cur &&
+		    haptic->mode == HID_HAPTIC_MODE_DEVICE) {
+			switch_mode(hdev, haptic, HID_HAPTIC_MODE_KERNEL);
+		}
+		haptic->press_ordinal_cur = ordinal;
+	}
+
 	return 0;
 }
 
@@ -294,6 +361,7 @@ static void effect_set_default(struct ff_effect *effect)
 static int hid_haptic_erase(struct input_dev *dev, int effect_id)
 {
 	struct hid_haptic_device *haptic = dev->ff->private;
+	struct hid_device *hdev = input_get_drvdata(dev);
 	struct ff_effect effect;
 	int ordinal;
 
@@ -301,21 +369,29 @@ static int hid_haptic_erase(struct input_dev *dev, int effect_id)
 	switch (effect_id) {
 	case HID_HAPTIC_RELEASE_EFFECT_ID:
 		ordinal = haptic->release_ordinal_orig;
-		if (!ordinal)
+		haptic->release_ordinal_cur = ordinal;
+		if (!ordinal) {
 			ordinal = HID_HAPTIC_ORDINAL_WAVEFORMNONE;
-		else
+			if (haptic->mode == HID_HAPTIC_MODE_KERNEL)
+				switch_mode(hdev, haptic, HID_HAPTIC_MODE_DEVICE);
+		} else {
 			effect.u.hid.hid_usage = HID_HP_WAVEFORMRELEASE &
 				HID_USAGE;
+		}
 		fill_effect_buf(haptic, &effect.u.hid, &haptic->effect[effect_id],
 				ordinal);
 		break;
 	case HID_HAPTIC_PRESS_EFFECT_ID:
 		ordinal = haptic->press_ordinal_orig;
-		if (!ordinal)
+		haptic->press_ordinal_cur = ordinal;
+		if (!ordinal) {
 			ordinal = HID_HAPTIC_ORDINAL_WAVEFORMNONE;
-		else
+			if (haptic->mode == HID_HAPTIC_MODE_KERNEL)
+				switch_mode(hdev, haptic, HID_HAPTIC_MODE_DEVICE);
+		} else {
 			effect.u.hid.hid_usage = HID_HP_WAVEFORMPRESS &
 				HID_USAGE;
+		}
 		fill_effect_buf(haptic, &effect.u.hid, &haptic->effect[effect_id],
 				ordinal);
 		break;
@@ -408,6 +484,7 @@ int hid_haptic_init(struct hid_device *hdev,
 	haptic->hid_usage_map[HID_HAPTIC_ORDINAL_WAVEFORMSTOP] =
 		HID_HP_WAVEFORMSTOP & HID_USAGE;
 
+	mutex_init(&haptic->auto_trigger_mutex);
 	for (r = 0; r < haptic->auto_trigger_report->maxfield; r++)
 		parse_auto_trigger_field(haptic, haptic->auto_trigger_report->field[r]);
 
@@ -583,13 +660,13 @@ void hid_haptic_handle_press_release(struct hid_haptic_device *haptic)
 	if (!prev_pressed_state && haptic->pressed_state &&
 	    haptic->mode == HID_HAPTIC_MODE_KERNEL) {
 		spin_lock_irqsave(&input->event_lock, flags);
-		input->ff->playback(input, PRESS_HID_EFFECT_ID, 1);
+		input->ff->playback(input, HID_HAPTIC_PRESS_EFFECT_ID, 1);
 		spin_unlock_irqrestore(&input->event_lock, flags);
 	}
 	if (prev_pressed_state && !haptic->pressed_state &&
 	    haptic->mode == HID_HAPTIC_MODE_KERNEL) {
 		spin_lock_irqsave(&input->event_lock, flags);
-		input->ff->playback(input, RELEASE_HID_EFFECT_ID, 1);
+		input->ff->playback(input, HID_HAPTIC_RELEASE_EFFECT_ID, 1);
 		spin_unlock_irqrestore(&input->event_lock, flags);
 	}
 }
diff --git a/drivers/hid/hid-haptic.h b/drivers/hid/hid-haptic.h
index 27ae1ed576c4..7a4571075a21 100644
--- a/drivers/hid/hid-haptic.h
+++ b/drivers/hid/hid-haptic.h
@@ -82,6 +82,10 @@ int hid_haptic_input_mapping(struct hid_device *hdev,
 int hid_haptic_input_configured(struct hid_device *hdev,
 				struct hid_haptic_device *haptic,
 				struct hid_input *hi);
+#ifdef CONFIG_PM
+void hid_haptic_resume(struct hid_device *hdev, struct hid_haptic_device *haptic);
+void hid_haptic_suspend(struct hid_device *hdev, struct hid_haptic_device *haptic);
+#endif
 int hid_haptic_init(struct hid_device *hdev, struct hid_haptic_device **haptic_ptr);
 void hid_haptic_handle_press_release(struct hid_haptic_device *haptic);
 bool hid_haptic_handle_input(struct hid_haptic_device *haptic);
@@ -117,6 +121,12 @@ int hid_haptic_input_configured(struct hid_device *hdev,
 {
 	return 0;
 }
+#ifdef CONFIG_PM
+static inline
+void hid_haptic_resume(struct hid_device *hdev, struct hid_haptic_device *haptic) {}
+static inline
+void hid_haptic_suspend(struct hid_device *hdev, struct hid_haptic_device *haptic) {}
+#endif
 static inline
 int hid_haptic_init(struct hid_device *hdev, struct hid_haptic_device **haptic_ptr)
 {
-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 15/18] HID: multitouch: add haptic multitouch support

[Angela Czubak]

Add new option (MULTITOUCH_HAPTIC) to mark whether hid-multitouch
should try and configure simple haptic device.
Once this option is configured, and the device is recognized to have simple
haptic capabilities, check input frames for pressure and handle it using
hid_haptic_* API.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/Kconfig          | 11 ++++++
 drivers/hid/hid-multitouch.c | 71 +++++++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 8d1eb4491a7f..d64e316eb2ca 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -742,6 +742,17 @@ config HID_MULTITOUCH
 	  To compile this driver as a module, choose M here: the
 	  module will be called hid-multitouch.
 
+config MULTITOUCH_HAPTIC
+	bool "Simple haptic multitouch support"
+	depends on HID_MULTITOUCH
+	select HID_HAPTIC
+	default n
+	help
+	Support for simple multitouch haptic devices.
+	Adds extra parsing and FF device for the hid multitouch driver.
+	It can be used for Elan 2703 haptic touchpad.
+	To enable, say Y.
+
 config HID_NINTENDO
 	tristate "Nintendo Joy-Con and Pro Controller support"
 	depends on HID
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 7beb3dfc3e67..260e5d9b891d 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -48,6 +48,8 @@ MODULE_LICENSE("GPL");
 
 #include "hid-ids.h"
 
+#include "hid-haptic.h"
+
 /* quirks to control the device */
 #define MT_QUIRK_NOT_SEEN_MEANS_UP	BIT(0)
 #define MT_QUIRK_SLOT_IS_CONTACTID	BIT(1)
@@ -159,11 +161,13 @@ struct mt_report_data {
 struct mt_device {
 	struct mt_class mtclass;	/* our mt device class */
 	struct timer_list release_timer;	/* to release sticky fingers */
+	struct hid_haptic_device *haptic;	/* haptic related configuration */
 	struct hid_device *hdev;	/* hid_device we're attached to */
 	unsigned long mt_io_flags;	/* mt flags (MT_IO_FLAGS_*) */
 	__u8 inputmode_value;	/* InputMode HID feature value */
 	__u8 maxcontacts;
 	bool is_buttonpad;	/* is this device a button pad? */
+	bool is_haptic_touchpad;	/* is this device a haptic touchpad? */
 	bool serial_maybe;	/* need to check for serial protocol */
 
 	struct list_head applications;
@@ -469,6 +473,8 @@ static void mt_feature_mapping(struct hid_device *hdev,
 			hid_get_feature(hdev, field->report);
 		break;
 	}
+
+	hid_haptic_feature_mapping(hdev, td->haptic, field, usage);
 }
 
 static void set_abs(struct input_dev *input, unsigned int code,
@@ -799,6 +805,9 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 		case HID_DG_TIPPRESSURE:
 			set_abs(hi->input, ABS_MT_PRESSURE, field,
 				cls->sn_pressure);
+			td->is_haptic_touchpad =
+				hid_haptic_check_pressure_unit(td->haptic,
+							       hi, field);
 			MT_STORE_FIELD(p);
 			return 1;
 		case HID_DG_SCANTIME:
@@ -912,8 +921,16 @@ static void mt_release_pending_palms(struct mt_device *td,
 static void mt_sync_frame(struct mt_device *td, struct mt_application *app,
 			  struct input_dev *input)
 {
-	if (app->quirks & MT_QUIRK_WIN8_PTP_BUTTONS)
-		input_event(input, EV_KEY, BTN_LEFT, app->left_button_state);
+	if (td->is_haptic_touchpad)
+		hid_haptic_handle_press_release(td->haptic);
+
+	if (app->quirks & MT_QUIRK_WIN8_PTP_BUTTONS) {
+		if (!(td->is_haptic_touchpad &&
+		    hid_haptic_handle_input(td->haptic))) {
+			input_event(input, EV_KEY, BTN_LEFT,
+				    app->left_button_state);
+		}
+	}
 
 	input_mt_sync_frame(input);
 	input_event(input, EV_MSC, MSC_TIMESTAMP, app->timestamp);
@@ -923,6 +940,8 @@ static void mt_sync_frame(struct mt_device *td, struct mt_application *app,
 
 	app->num_received = 0;
 	app->left_button_state = 0;
+	if (td->is_haptic_touchpad)
+		hid_haptic_pressure_reset(td->haptic);
 
 	if (test_bit(MT_IO_FLAGS_ACTIVE_SLOTS, &td->mt_io_flags))
 		set_bit(MT_IO_FLAGS_PENDING_SLOTS, &td->mt_io_flags);
@@ -1072,6 +1091,9 @@ static int mt_process_slot(struct mt_device *td, struct input_dev *input,
 			minor = minor >> 1;
 		}
 
+		if (td->is_haptic_touchpad)
+			hid_haptic_pressure_increase(td->haptic, *slot->p);
+
 		input_event(input, EV_ABS, ABS_MT_POSITION_X, *slot->x);
 		input_event(input, EV_ABS, ABS_MT_POSITION_Y, *slot->y);
 		input_event(input, EV_ABS, ABS_MT_TOOL_X, *slot->cx);
@@ -1281,6 +1303,7 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	struct mt_device *td = hid_get_drvdata(hdev);
 	struct mt_application *application;
 	struct mt_report_data *rdata;
+	int ret;
 
 	rdata = mt_find_report_data(td, field->report);
 	if (!rdata) {
@@ -1343,6 +1366,11 @@ static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	if (field->physical == HID_DG_STYLUS)
 		hi->application = HID_DG_STYLUS;
 
+	ret = hid_haptic_input_mapping(hdev, td->haptic, hi, field, usage, bit,
+				       max);
+	if (ret != 0)
+		return ret;
+
 	/* let hid-core decide for the others */
 	return 0;
 }
@@ -1593,6 +1621,14 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 		}
 	}
 
+	if (td->is_haptic_touchpad && (td->mtclass.name == MT_CLS_WIN_8 ||
+	    td->mtclass.name == MT_CLS_WIN_8_FORCE_MULTI_INPUT)) {
+		if (hid_haptic_input_configured(hdev, td->haptic, hi) == 0)
+			td->is_haptic_touchpad = false;
+	} else {
+		td->is_haptic_touchpad = false;
+	}
+
 	return 0;
 }
 
@@ -1684,6 +1720,9 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		dev_err(&hdev->dev, "cannot allocate multitouch data\n");
 		return -ENOMEM;
 	}
+	td->haptic = kzalloc(sizeof(*(td->haptic)), GFP_KERNEL);
+	if (!td->haptic)
+		return -ENOMEM;
 	td->hdev = hdev;
 	td->mtclass = *mtclass;
 	td->inputmode_value = MT_INPUTMODE_TOUCHSCREEN;
@@ -1735,6 +1774,17 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
 
+	if (td->is_haptic_touchpad) {
+		if (hid_haptic_init(hdev, &td->haptic)) {
+			dev_warn(&hdev->dev, "Cannot allocate haptic for %s\n",
+				 hdev->name);
+			td->is_haptic_touchpad = false;
+			kfree(td->haptic);
+		}
+	} else {
+		kfree(td->haptic);
+	}
+
 	return 0;
 }
 
@@ -1742,6 +1792,7 @@ static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
 static int mt_suspend(struct hid_device *hdev, pm_message_t state)
 {
 	struct mt_device *td = hid_get_drvdata(hdev);
+	struct hid_haptic_device *haptic = td->haptic;
 
 	/* High latency is desirable for power savings during S3/S0ix */
 	if ((td->mtclass.quirks & MT_QUIRK_DISABLE_WAKEUP) ||
@@ -1750,18 +1801,31 @@ static int mt_suspend(struct hid_device *hdev, pm_message_t state)
 	else
 		mt_set_modes(hdev, HID_LATENCY_HIGH, true, true);
 
+	if (td->is_haptic_touchpad)
+		hid_haptic_resume(hdev, haptic);
+
 	return 0;
 }
 
 static int mt_reset_resume(struct hid_device *hdev)
 {
+	struct mt_device *td = hid_get_drvdata(hdev);
+	struct hid_haptic_device *haptic = td->haptic;
+
 	mt_release_contacts(hdev);
 	mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
+
+	if (td->is_haptic_touchpad)
+		hid_haptic_resume(hdev, haptic);
+
 	return 0;
 }
 
 static int mt_resume(struct hid_device *hdev)
 {
+	struct mt_device *td = hid_get_drvdata(hdev);
+	struct hid_haptic_device *haptic = td->haptic;
+
 	/* Some Elan legacy devices require SET_IDLE to be set on resume.
 	 * It should be safe to send it to other devices too.
 	 * Tested on 3M, Stantum, Cypress, Zytronic, eGalax, and Elan panels. */
@@ -1770,6 +1834,9 @@ static int mt_resume(struct hid_device *hdev)
 
 	mt_set_modes(hdev, HID_LATENCY_NORMAL, true, true);
 
+	if (td->is_haptic_touchpad)
+		hid_haptic_suspend(hdev, haptic);
+
 	return 0;
 }
 #endif
-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 16/18] Input: introduce EVIOCFF(TAKE|RELEASE)CONTROL

[Angela Czubak]

Add new ioctls which can be used for simple haptic force feedback effects.
Once the control is taken over the effect the kernel does not generate it
on its own (EVIOCFFTAKECONTROL).
To revert this action use EVIOCFFRELEASECONTROL.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/input/evdev.c      |  6 +++
 drivers/input/ff-core.c    | 89 +++++++++++++++++++++++++++++++++++++-
 include/linux/input.h      |  5 +++
 include/uapi/linux/input.h |  4 ++
 4 files changed, 103 insertions(+), 1 deletion(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index 95f90699d2b1..6d25eb19e28e 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -1076,6 +1076,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 	case EVIOCRMFF:
 		return input_ff_erase(dev, (int)(unsigned long) p, file);
 
+	case EVIOCFFTAKECONTROL:
+		return input_ff_take_control(dev, (int)(unsigned long) p, file);
+
+	case EVIOCFFRELEASECONTROL:
+		return input_ff_release_control(dev, (int)(unsigned long) p, file);
+
 	case EVIOCGEFFECTS:
 		i = test_bit(EV_FF, dev->evbit) ?
 				dev->ff->max_effects : 0;
diff --git a/drivers/input/ff-core.c b/drivers/input/ff-core.c
index 960ae0e29348..7536274141e7 100644
--- a/drivers/input/ff-core.c
+++ b/drivers/input/ff-core.c
@@ -252,6 +252,91 @@ int input_ff_erase(struct input_dev *dev, int effect_id, struct file *file)
 }
 EXPORT_SYMBOL_GPL(input_ff_erase);
 
+/*
+ * Take control over the effect if the requester is also the effect owner.
+ * The mutex should already be locked before calling this function.
+ */
+static int control_effect(struct input_dev *dev, int effect_id,
+			  struct file *file, int take)
+{
+	struct ff_device *ff = dev->ff;
+	int error;
+
+	error = check_effect_access(ff, effect_id, file);
+	if (error) {
+		error = check_effect_shared(ff, effect_id);
+		if (error)
+			return error;
+	}
+
+	if (ff->change_control) {
+		error = ff->change_control(dev, effect_id, file, take);
+		if (error)
+			return error;
+	}
+
+	return 0;
+}
+/**
+ * input_ff_take_control - take control over a force-feedback effect from kernel
+ * @dev: input device to take control over effect from
+ * @effect_id: id of the effect to take control over
+ * @file: purported owner of the request
+ *
+ * This function switches user-controlled mode on for the given force-feedback
+ * effect. The user-mode will persist unitl the last caller releases control.
+ * The effect will only be taken control of if it was uploaded through the same
+ * file handle that is requesting taking control or for simple haptic effects
+ * 0 and 1.
+ * Valid only for simple haptic effects (ff_hid_effect).
+ */
+int input_ff_take_control(struct input_dev *dev, int effect_id,
+			  struct file *file)
+{
+	struct ff_device *ff = dev->ff;
+	int ret;
+
+	if (!test_bit(EV_FF, dev->evbit))
+		return -EINVAL;
+
+	mutex_lock(&ff->mutex);
+	ret = control_effect(dev, effect_id, file, 1);
+	mutex_unlock(&ff->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(input_ff_take_control);
+
+/**
+ * input_ff_release_control - release control over a force-feedback effect
+ * @dev: input device to release control over effect to
+ * @effect_id: id of the effect to release control
+ * @file: purported owner of the request
+ *
+ * This function switches user-controlled mode off for the given force-feedback
+ * effect. The user-mode will persist unitl the last caller releases control.
+ * The control will be released of if it was uploaded through the same
+ * file handle that is requesting taking control or for simple haptic effects
+ * 0 and 1.
+ * Valid only for simple haptic effects (ff_hid_effect).
+ */
+int input_ff_release_control(struct input_dev *dev, int effect_id,
+			     struct file *file)
+{
+	struct ff_device *ff = dev->ff;
+	int ret;
+
+	if (!test_bit(EV_FF, dev->evbit))
+		return -EINVAL;
+
+	mutex_lock(&ff->mutex);
+	ret = control_effect(dev, effect_id, file, 0);
+	mutex_unlock(&ff->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(input_ff_release_control);
+
 /*
  * input_ff_flush - erase all effects owned by a file handle
  * @dev: input device to erase effect from
@@ -270,8 +355,10 @@ int input_ff_flush(struct input_dev *dev, struct file *file)
 
 	mutex_lock(&ff->mutex);
 
-	for (i = 0; i < ff->max_effects; i++)
+	for (i = 0; i < ff->max_effects; i++) {
+		control_effect(dev, i, file, 0);
 		erase_effect(dev, i, file);
+	}
 
 	mutex_unlock(&ff->mutex);
 
diff --git a/include/linux/input.h b/include/linux/input.h
index 0354b298d874..cc432ff6a427 100644
--- a/include/linux/input.h
+++ b/include/linux/input.h
@@ -519,6 +519,7 @@ extern struct class input_class;
  * @upload: Called to upload an new effect into device
  * @erase: Called to erase an effect from device
  * @playback: Called to request device to start playing specified effect
+ * @change_control: Called to change control over specified effect
  * @set_gain: Called to set specified gain
  * @set_autocenter: Called to auto-center device
  * @destroy: called by input core when parent input device is being
@@ -547,6 +548,8 @@ struct ff_device {
 	int (*erase)(struct input_dev *dev, int effect_id);
 
 	int (*playback)(struct input_dev *dev, int effect_id, int value);
+	int (*change_control)(struct input_dev *dev, int effect_id,
+			      struct file *file, int take);
 	void (*set_gain)(struct input_dev *dev, u16 gain);
 	void (*set_autocenter)(struct input_dev *dev, u16 magnitude);
 
@@ -570,6 +573,8 @@ int input_ff_event(struct input_dev *dev, unsigned int type, unsigned int code,
 
 int input_ff_upload(struct input_dev *dev, struct ff_effect *effect, struct file *file);
 int input_ff_erase(struct input_dev *dev, int effect_id, struct file *file);
+int input_ff_take_control(struct input_dev *dev, int effect_id, struct file *file);
+int input_ff_release_control(struct input_dev *dev, int effect_id, struct file *file);
 int input_ff_flush(struct input_dev *dev, struct file *file);
 
 int input_ff_create_memless(struct input_dev *dev, void *data,
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 0d4d426cf75a..12acc3bb73d7 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -178,6 +178,10 @@ struct input_mask {
 
 #define EVIOCSFF		_IOW('E', 0x80, struct ff_effect)	/* send a force effect to a force feedback device */
 #define EVIOCRMFF		_IOW('E', 0x81, int)			/* Erase a force effect */
+/* Take control over a force effect */
+#define EVIOCFFTAKECONTROL	_IOW('E', 0x82, int)
+/* Release control over a force effect */
+#define EVIOCFFRELEASECONTROL	_IOW('E', 0x83, int)
 #define EVIOCGEFFECTS		_IOR('E', 0x84, int)			/* Report number of effects playable at the same time */
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 17/18] HID: haptic: add hid_haptic_change_control

[Angela Czubak]

Implement change_control callbacks for simple haptic device.
If anybody has requested control over an effect, do not generate it
in kernel.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/hid-haptic.c | 50 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
index e7fd53d1a7c1..366b7f8a2c19 100644
--- a/drivers/hid/hid-haptic.c
+++ b/drivers/hid/hid-haptic.c
@@ -9,6 +9,7 @@
  */
 
 #include <linux/input/mt.h>
+#include <linux/mm.h>
 #include <linux/module.h>
 
 #include "hid-haptic.h"
@@ -348,6 +349,46 @@ static int hid_haptic_playback(struct input_dev *dev, int effect_id, int value)
 	return 0;
 }
 
+static int hid_haptic_change_control(struct input_dev *dev, int effect_id,
+				     struct file *file, int take)
+{
+	struct hid_haptic_device *haptic = dev->ff->private;
+	struct hid_haptic_effect_node *effect_node;
+	struct hid_haptic_effect *effect;
+	bool found = false;
+	int ret = 0;
+
+	effect = &haptic->effect[effect_id];
+	mutex_lock(&effect->control_mutex);
+	list_for_each_entry(effect_node, &effect->control, node) {
+		if (effect_node->file == file) {
+			found = true;
+			break;
+		}
+	}
+	if (take) {
+		if (!found) {
+			effect_node = kvzalloc(sizeof(struct hid_haptic_effect),
+					       GFP_KERNEL);
+			if (!effect_node) {
+				ret = -ENOMEM;
+				goto exit;
+			}
+			effect_node->file = file;
+		}
+		list_add(&effect_node->node, &effect->control);
+	} else {
+		if (found) {
+			list_del(&effect_node->node);
+			kvfree(effect_node);
+		}
+	}
+exit:
+	mutex_unlock(&effect->control_mutex);
+
+	return ret;
+}
+
 static void effect_set_default(struct ff_effect *effect)
 {
 	effect->type = FF_HID;
@@ -533,6 +574,8 @@ int hid_haptic_init(struct hid_device *hdev,
 		}
 		haptic->effect[r].input_dev = dev;
 		INIT_WORK(&haptic->effect[r].work, haptic_work_handler);
+		INIT_LIST_HEAD(&haptic->effect[r].control);
+		mutex_init(&haptic->effect[r].control_mutex);
 	}
 	haptic->stop_effect.report_buf =
 		hid_alloc_report_buf(haptic->manual_trigger_report,
@@ -569,6 +612,7 @@ int hid_haptic_init(struct hid_device *hdev,
 	ff->private = haptic;
 	ff->upload = hid_haptic_upload_effect;
 	ff->playback = hid_haptic_playback;
+	ff->change_control = hid_haptic_change_control;
 	ff->erase = hid_haptic_erase;
 	ff->destroy = hid_haptic_destroy;
 	if (!try_module_get(THIS_MODULE)) {
@@ -658,13 +702,15 @@ void hid_haptic_handle_press_release(struct hid_haptic_device *haptic)
 	else if (haptic->pressure_sum < haptic->release_threshold)
 		haptic->pressed_state = 0;
 	if (!prev_pressed_state && haptic->pressed_state &&
-	    haptic->mode == HID_HAPTIC_MODE_KERNEL) {
+	    haptic->mode == HID_HAPTIC_MODE_KERNEL &&
+	    list_empty(&haptic->effect[HID_HAPTIC_PRESS_EFFECT_ID].control)) {
 		spin_lock_irqsave(&input->event_lock, flags);
 		input->ff->playback(input, HID_HAPTIC_PRESS_EFFECT_ID, 1);
 		spin_unlock_irqrestore(&input->event_lock, flags);
 	}
 	if (prev_pressed_state && !haptic->pressed_state &&
-	    haptic->mode == HID_HAPTIC_MODE_KERNEL) {
+	    haptic->mode == HID_HAPTIC_MODE_KERNEL &&
+	    list_empty(&haptic->effect[HID_HAPTIC_RELEASE_EFFECT_ID].control)) {
 		spin_lock_irqsave(&input->event_lock, flags);
 		input->ff->playback(input, HID_HAPTIC_RELEASE_EFFECT_ID, 1);
 		spin_unlock_irqrestore(&input->event_lock, flags);
-- 
2.34.1.307.g9b7440fafd-goog

[PATCH 18/18] HID: i2c-hid: fix i2c_hid_set_or_send_report

[Angela Czubak]

If command & data registers are to be used and report ID >= 0xF
use the sentinel value for report ID in the command.
Do not alter the report ID itself as it needs to be inserted into the args
buffer. If output register is to be used there is no need to insert
report IDs >= 0xF.

Signed-off-by: Angela Czubak <acz@semihalf.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 517141138b00..8cb925c86bbf 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -341,6 +341,7 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
 	u16 size;
 	int args_len;
 	int index = 0;
+	u8 cmdReportID = reportID;
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
 
@@ -357,16 +358,15 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
 	if (!use_data && maxOutputLength == 0)
 		return -ENOSYS;
 
-	if (reportID >= 0x0F) {
-		args[index++] = reportID;
-		reportID = 0x0F;
-	}
-
 	/*
 	 * use the data register for feature reports or if the device does not
 	 * support the output register
 	 */
 	if (use_data) {
+		if (reportID >= 0x0F) {
+			args[index++] = reportID;
+			cmdReportID = 0x0F;
+		}
 		args[index++] = dataRegister & 0xFF;
 		args[index++] = dataRegister >> 8;
 		hidcmd = &hid_set_report_cmd;
@@ -384,7 +384,7 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
 
 	memcpy(&args[index], buf, data_len);
 
-	ret = __i2c_hid_command(client, hidcmd, reportID,
+	ret = __i2c_hid_command(client, hidcmd, cmdReportID,
 		reportType, args, args_len, NULL, 0);
 	if (ret) {
 		dev_err(&client->dev, "failed to set a report to device.\n");
-- 
2.34.1.307.g9b7440fafd-goog

Re: [PATCH 18/18] HID: i2c-hid: fix i2c_hid_set_or_send_report

[Dmitry Torokhov]

Hi Angela,

On Tue, Dec 21, 2021 at 07:17:43PM +0000, Angela Czubak wrote:
> If command & data registers are to be used and report ID >= 0xF
> use the sentinel value for report ID in the command.
> Do not alter the report ID itself as it needs to be inserted into the args
> buffer. If output register is to be used there is no need to insert
> report IDs >= 0xF.

OK, I see what you mean, but I believe that i2c_hid_set_or_send_report()
is the wrong place to implement this handling to begin with. How about a
modified version of your patch that I am pasting below (not tested)?

Thanks,
Dmitry

-- >8 -- >8 --


HID: i2c-hid: fix handling numbered reports with IDs of 15 and above

From: Angela Czubak <acz@semihalf.com>

Special handling of numbered reports with IDs of 15 and above is only
needed when executing what HID-I2C spec is calling "Class Specific
Requests", and not when simply sending output reports.

Additionally, our mangling of report ID in i2c_hid_set_or_send_report()
resulted in incorrect report ID being written into SET_REPORT command
payload.

To solve it let's move all the report ID manipulation into
__i2c_hid_command() where we form the command data structure.

Signed-off-by: Angela Czubak <acz@semihalf.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid-core.c |   22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 517141138b00..9381a70b2948 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -97,6 +97,7 @@ union command {
 		__le16 reg;
 		__u8 reportTypeID;
 		__u8 opcode;
+		__u8 reportID;
 	} __packed c;
 };
 
@@ -232,7 +233,13 @@ static int __i2c_hid_command(struct i2c_client *client,
 
 	if (length > 2) {
 		cmd->c.opcode = command->opcode;
-		cmd->c.reportTypeID = reportID | reportType << 4;
+		if (reportID < 0x0F) {
+			cmd->c.reportTypeID = reportType << 4 | reportID;
+		} else {
+			cmd->c.reportTypeID = reportType << 4 | 0x0F;
+			cmd->c.reportID = reportID;
+			length++;
+		}
 	}
 
 	memcpy(cmd->data + length, args, args_len);
@@ -300,11 +307,6 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
 
 	i2c_hid_dbg(ihid, "%s\n", __func__);
 
-	if (reportID >= 0x0F) {
-		args[args_len++] = reportID;
-		reportID = 0x0F;
-	}
-
 	args[args_len++] = readRegister & 0xFF;
 	args[args_len++] = readRegister >> 8;
 
@@ -350,18 +352,12 @@ static int i2c_hid_set_or_send_report(struct i2c_client *client, u8 reportType,
 	size =		2			/* size */ +
 			(reportID ? 1 : 0)	/* reportID */ +
 			data_len		/* buf */;
-	args_len =	(reportID >= 0x0F ? 1 : 0) /* optional third byte */ +
-			2			/* dataRegister */ +
+	args_len =	2			/* dataRegister */ +
 			size			/* args */;
 
 	if (!use_data && maxOutputLength == 0)
 		return -ENOSYS;
 
-	if (reportID >= 0x0F) {
-		args[index++] = reportID;
-		reportID = 0x0F;
-	}
-
 	/*
 	 * use the data register for feature reports or if the device does not
 	 * support the output register

Re: [PATCH 18/18] HID: i2c-hid: fix i2c_hid_set_or_send_report

[Angela Czubak]

Hi Dmitry,

I tested the patch and it works fine.
What is more, I like your design better.
Thanks!

Regards,
Angela

Re: [PATCH 00/18] *** Implement simple haptic HID support ***

[Roderick Colenbrander]

Hi Angela,

Thanks for sharing. I would like to have a look as well at this
series. As I briefly mentioned in another thread at Sony we have a
need for haptics as well for our DualSense controller for the
PlayStation 5. There is some overlap and differences. For our use
case, FF is really showing its limits (it really doesn't work). The
key question I have is whether FF is really a good fit for your use
case or not. I don't understand this type of device well enough yet.
There might be enough demand for either a new framework (or it is the
end of the road for evdev).

Thanks,
Roderick

On Wed, Dec 22, 2021 at 7:59 AM Angela Czubak <acz@semihalf.com> wrote:
>
> This patch series introduces changes necessary to support devices
> using simple haptic HID pages.
> Implementation attempts to follow the discussion below:
> https://www.spinics.net/lists/linux-input/msg61091.html
>
> Introduce new haptic defines as specified in HID Usage Tables.
>
> Add new force feedback effect type in order to facilitate using
> simple haptic force feedback.
>
> Add INPUT_PROP_HAPTIC_TOUCHPAD to mark touchpad exposing simple haptic
> support.
>
> Add new struct hid_haptic_device so as to gather simple haptic related
> configuration and current state of the device.
>
> Function mt_get_feature() gets renamed to hid_get_feature() and is moved
> to hid-core.c as it is not specific to hid multitouch driver and may be
> reused, for instance by simple haptic specific source.
>
> Add new functions to be triggered during HID input mapping and
> configuration in order to detect simple haptic devices.
>
> Modify HID input so that haptic output reports are parsed.
>
> Initialize a haptic device.
>
> Modify FF core so that effect IDs can be shared between multiple open file
> handles.
>
> Add shared release and press effects for a simple haptic device.
>
> Calculate pressure resolution if units are grams or newtons.
>
> Add support for kernel-driven mode of simple haptic device.
>
> Toggle ABS_PRESSURE generation by input-mt on request.
>
> Implement functions allowing switching between kernel-managed mode
> and autonomous mode.
>
> Add simple haptic support for hid-multitouch driver.
>
> Implement EVIOCFF(TAKE|RELEASE)CONTROL ioctls so that userspace can take
> and release control of shared release and press effects.
>
> Fix i2c_hid_set_or_send_report so that report IDs larger than 0xF are
> handled correctly.
>
> Angela Czubak (18):
>   HID: add haptics page defines
>   Input: add FF_HID effect type
>   Input: add INPUT_PROP_HAPTIC_TOUCHPAD
>   HID: haptic: introduce hid_haptic_device
>   HID: introduce hid_get_feature
>   HID: haptic: add functions for mapping and configuration
>   HID: input: allow mapping of haptic output
>   HID: haptic: initialize haptic device
>   Input: add shared effects
>   HID: haptic: implement release and press effects
>   HID: input: calculate resolution for pressure
>   HID: haptic: add functions handling events
>   Input: MT - toggle ABS_PRESSURE pointer emulation
>   HID: haptic: add hid_haptic_switch_mode
>   HID: multitouch: add haptic multitouch support
>   Input: introduce EVIOCFF(TAKE|RELEASE)CONTROL
>   HID: haptic: add hid_haptic_change_control
>   HID: i2c-hid: fix i2c_hid_set_or_send_report
>
>  drivers/hid/Kconfig                    |  15 +
>  drivers/hid/Makefile                   |   1 +
>  drivers/hid/hid-core.c                 |  39 ++
>  drivers/hid/hid-haptic.c               | 745 +++++++++++++++++++++++++
>  drivers/hid/hid-haptic.h               | 150 +++++
>  drivers/hid/hid-input.c                |  18 +-
>  drivers/hid/hid-multitouch.c           | 109 ++--
>  drivers/hid/i2c-hid/i2c-hid-core.c     |  12 +-
>  drivers/input/evdev.c                  |   6 +
>  drivers/input/ff-core.c                | 129 ++++-
>  drivers/input/input-mt.c               |  18 +-
>  include/linux/hid.h                    |  24 +
>  include/linux/input.h                  |   5 +
>  include/linux/input/mt.h               |   4 +
>  include/uapi/linux/input-event-codes.h |   1 +
>  include/uapi/linux/input.h             |  26 +-
>  16 files changed, 1247 insertions(+), 55 deletions(-)
>  create mode 100644 drivers/hid/hid-haptic.c
>  create mode 100644 drivers/hid/hid-haptic.h
>
> --
> 2.34.1.307.g9b7440fafd-goog
>