* Re: [PATCH v2 5/8] Input: cros_ec_keyb - Switch to SPDX identifier.
From: Enric Balletbo i Serra @ 2018-06-05 18:45 UTC (permalink / raw)
To: Dmitry Torokhov, Fabio Estevam
Cc: linux-kernel, Gwendal Grignou, kernel, linux-input
In-Reply-To: <20180605182919.GF202428@dtor-ws>
Hi Dmitry,
On 05/06/18 20:29, Dmitry Torokhov wrote:
> On Tue, Jun 05, 2018 at 03:16:40PM -0300, Fabio Estevam wrote:
>> Hi Enric,
>>
>> On Tue, Jun 5, 2018 at 3:14 PM, Enric Balletbo i Serra
>> <enric.balletbo@collabora.com> wrote:
>>
>>> Right, but the module license is set to GPL which means GNU Public License v2 or
>>> later, see [1]. So, there is a mismatch. In such cases I assumed GPL-2.0+ as the
>>> default. These mismatches are common so I think that should be fine for the
>>> authors, if someone is disagree just let me know and I will change.
>>
>> Ok, but I think you should add this explanation in the commit log to
>> make it clearer.
>
> If there is a conflict between the license notice and MODULE_LICENSE()
> we should go by the license notice. The license note is usually approved
> by companies, whereas MIODULE_LICENSE()s get moved, adjusted, etc.
>
> For ChromeOS kernel changes license notice is GPL v2 as documented at:
>
> https://www.chromium.org/chromium-os/how-tos-and-troubleshooting/kernel-faq
>
> section "Which copyright header should I use?"
>
Many thanks to share and clarify this.
There is lots of mismatches so I'll send another version fixing this and
changing all to GPL v2. I assume I should also modify the MODULE_LICENSE when
it's wrong.
Thanks.
Enric
> Thanks.
>
^ permalink raw reply
* Re: [PATCH v2 5/8] Input: cros_ec_keyb - Switch to SPDX identifier.
From: Dmitry Torokhov @ 2018-06-05 18:29 UTC (permalink / raw)
To: Fabio Estevam
Cc: Enric Balletbo i Serra, linux-kernel, Gwendal Grignou, kernel,
linux-input
In-Reply-To: <CAOMZO5Cod=jo5gC1ibQx5ZHU5xtUZDvfZt899=nPLgH4bhy+_g@mail.gmail.com>
On Tue, Jun 05, 2018 at 03:16:40PM -0300, Fabio Estevam wrote:
> Hi Enric,
>
> On Tue, Jun 5, 2018 at 3:14 PM, Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>
> > Right, but the module license is set to GPL which means GNU Public License v2 or
> > later, see [1]. So, there is a mismatch. In such cases I assumed GPL-2.0+ as the
> > default. These mismatches are common so I think that should be fine for the
> > authors, if someone is disagree just let me know and I will change.
>
> Ok, but I think you should add this explanation in the commit log to
> make it clearer.
If there is a conflict between the license notice and MODULE_LICENSE()
we should go by the license notice. The license note is usually approved
by companies, whereas MIODULE_LICENSE()s get moved, adjusted, etc.
For ChromeOS kernel changes license notice is GPL v2 as documented at:
https://www.chromium.org/chromium-os/how-tos-and-troubleshooting/kernel-faq
section "Which copyright header should I use?"
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 5/8] Input: cros_ec_keyb - Switch to SPDX identifier.
From: Fabio Estevam @ 2018-06-05 18:16 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: linux-kernel, Gwendal Grignou, kernel, Dmitry Torokhov,
linux-input
In-Reply-To: <7085d0bb-557c-0796-d3ae-756e000a7950@collabora.com>
Hi Enric,
On Tue, Jun 5, 2018 at 3:14 PM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Right, but the module license is set to GPL which means GNU Public License v2 or
> later, see [1]. So, there is a mismatch. In such cases I assumed GPL-2.0+ as the
> default. These mismatches are common so I think that should be fine for the
> authors, if someone is disagree just let me know and I will change.
Ok, but I think you should add this explanation in the commit log to
make it clearer.
^ permalink raw reply
* Re: [PATCH v2 5/8] Input: cros_ec_keyb - Switch to SPDX identifier.
From: Enric Balletbo i Serra @ 2018-06-05 18:14 UTC (permalink / raw)
To: Fabio Estevam
Cc: linux-kernel, Gwendal Grignou, kernel, Dmitry Torokhov,
linux-input
In-Reply-To: <CAOMZO5ByhMV9_gRHLmGiSNJtCvKEOk==ZMz2O2=tokZh2ps4xg@mail.gmail.com>
Hi Fabio,
On 05/06/18 20:04, Fabio Estevam wrote:
> Hi Enric,
>
> On Tue, Jun 5, 2018 at 2:54 PM, Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
>> Adopt the SPDX license identifier headers to ease license compliance
>> management.
>>
>> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
>> ---
>>
>> Changes in v2:
>> - [6/9] Do not remove last paragraph.
>>
>> drivers/input/keyboard/cros_ec_keyb.c | 34 ++++++++++-----------------
>> 1 file changed, 12 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
>> index 79eb29550c34..91b2839c12df 100644
>> --- a/drivers/input/keyboard/cros_ec_keyb.c
>> +++ b/drivers/input/keyboard/cros_ec_keyb.c
>> @@ -1,25 +1,15 @@
>> -/*
>> - * ChromeOS EC keyboard driver
>> - *
>> - * Copyright (C) 2012 Google, Inc
>> - *
>> - * This software is licensed under the terms of the GNU General Public
>> - * License version 2, as published by the Free Software Foundation, and
>
> Original text says GPL-2.0...
>
>> - * may be copied, distributed, and modified under those terms.
>> - *
>> - * This program is distributed in the hope that it will be useful,
>> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> - * GNU General Public License for more details.
>> - *
>> - * This driver uses the Chrome OS EC byte-level message-based protocol for
>> - * communicating the keyboard state (which keys are pressed) from a keyboard EC
>> - * to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
>> - * but everything else (including deghosting) is done here. The main
>> - * motivation for this is to keep the EC firmware as simple as possible, since
>> - * it cannot be easily upgraded and EC flash/IRAM space is relatively
>> - * expensive.
>> - */
>> +// SPDX-License-Identifier: GPL-2.0+
>
> but here you add a GPL-2.0+ type.
>
Right, but the module license is set to GPL which means GNU Public License v2 or
later, see [1]. So, there is a mismatch. In such cases I assumed GPL-2.0+ as the
default. These mismatches are common so I think that should be fine for the
authors, if someone is disagree just let me know and I will change.
Best regards,
Enric
[1]
https://elixir.bootlin.com/linux/v4.17/source/drivers/input/keyboard/cros_ec_keyb.c#L677
^ permalink raw reply
* Re: [PATCH v2 5/8] Input: cros_ec_keyb - Switch to SPDX identifier.
From: Fabio Estevam @ 2018-06-05 18:04 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: linux-kernel, Gwendal Grignou, kernel, Dmitry Torokhov,
linux-input
In-Reply-To: <20180605175429.9072-6-enric.balletbo@collabora.com>
Hi Enric,
On Tue, Jun 5, 2018 at 2:54 PM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> Adopt the SPDX license identifier headers to ease license compliance
> management.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> Changes in v2:
> - [6/9] Do not remove last paragraph.
>
> drivers/input/keyboard/cros_ec_keyb.c | 34 ++++++++++-----------------
> 1 file changed, 12 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 79eb29550c34..91b2839c12df 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -1,25 +1,15 @@
> -/*
> - * ChromeOS EC keyboard driver
> - *
> - * Copyright (C) 2012 Google, Inc
> - *
> - * This software is licensed under the terms of the GNU General Public
> - * License version 2, as published by the Free Software Foundation, and
Original text says GPL-2.0...
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> - * This driver uses the Chrome OS EC byte-level message-based protocol for
> - * communicating the keyboard state (which keys are pressed) from a keyboard EC
> - * to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
> - * but everything else (including deghosting) is done here. The main
> - * motivation for this is to keep the EC firmware as simple as possible, since
> - * it cannot be easily upgraded and EC flash/IRAM space is relatively
> - * expensive.
> - */
> +// SPDX-License-Identifier: GPL-2.0+
but here you add a GPL-2.0+ type.
^ permalink raw reply
* [PATCH v2 5/8] Input: cros_ec_keyb - Switch to SPDX identifier.
From: Enric Balletbo i Serra @ 2018-06-05 17:54 UTC (permalink / raw)
To: linux-kernel; +Cc: Gwendal Grignou, kernel, Dmitry Torokhov, linux-input
In-Reply-To: <20180605175429.9072-1-enric.balletbo@collabora.com>
Adopt the SPDX license identifier headers to ease license compliance
management.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes in v2:
- [6/9] Do not remove last paragraph.
drivers/input/keyboard/cros_ec_keyb.c | 34 ++++++++++-----------------
1 file changed, 12 insertions(+), 22 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 79eb29550c34..91b2839c12df 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -1,25 +1,15 @@
-/*
- * ChromeOS EC keyboard driver
- *
- * Copyright (C) 2012 Google, Inc
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * This driver uses the Chrome OS EC byte-level message-based protocol for
- * communicating the keyboard state (which keys are pressed) from a keyboard EC
- * to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
- * but everything else (including deghosting) is done here. The main
- * motivation for this is to keep the EC firmware as simple as possible, since
- * it cannot be easily upgraded and EC flash/IRAM space is relatively
- * expensive.
- */
+// SPDX-License-Identifier: GPL-2.0+
+// ChromeOS EC keyboard driver
+//
+// Copyright (C) 2012 Google, Inc.
+//
+// This driver uses the ChromeOS EC byte-level message-based protocol for
+// communicating the keyboard state (which keys are pressed) from a keyboard EC
+// to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
+// but everything else (including deghosting) is done here. The main
+// motivation for this is to keep the EC firmware as simple as possible, since
+// it cannot be easily upgraded and EC flash/IRAM space is relatively
+// expensive.
#include <linux/module.h>
#include <linux/bitops.h>
--
2.17.1
^ permalink raw reply related
* [PATCH v2 0/8] cros_ec: Switch to SPDX identifier.
From: Enric Balletbo i Serra @ 2018-06-05 17:54 UTC (permalink / raw)
To: linux-kernel
Cc: Gwendal Grignou, kernel, Tony Luck, Paolo Cretaro, linux-iio,
linux-i2c, Olof Johansson, Anton Vorontsov, Chanwoo Choi,
Benson Leung, linux-input, Dmitry Torokhov, Jonathan Cameron,
Peter Meerwald-Stadler, linux-pwm, Thierry Reding, MyungJoo Ham,
Lee Jones, Hartmut Knaack, Colin Cross, Kees Cook, Lars-Pete
Dear all,
This patchset is just to adopt the SPDX license identifier for all
ChromeOS Embedded Controller related drivers. The patches touches
different subsystems but every patch can be picked from their respective
maintainer independently.
This second version fixes some copy paste errors in the copyright year
and takes care to no remove the paragraphs that have nothing to do with
license notice.
Thanks,
Enric
Changes in v2:
- Removed '[PATCH 5/9] rtc: cros-ec: Switch to SPDX identifier' from
series as it was applied.
- s/Chrome OS/ChromeOS/
- [1/9] Fixed wrong copyright year.
- [6/9] Do not remove last paragraph.
- [8/9] Fixed wrong copyright year.
Enric Balletbo i Serra (8):
platform/chrome: cros_ec: Switch to SPDX identifier.
platform/chrome: pstore: Switch to SPDX identifier.
mfd: cros_ec: Add or fix SPDX-License-Identifier in all files.
iio: cros_ec: Switch to SPDX identifier.
Input: cros_ec_keyb - Switch to SPDX identifier.
pwm: cros-ec: Switch to SPDX identifier.
extcon: usbc-cros-ec: Switch to SPDX identifier.
i2c: i2c-cros-ec-tunnel: Switch to SPDX identifier.
drivers/extcon/extcon-usbc-cros-ec.c | 20 +++--------
drivers/i2c/busses/i2c-cros-ec-tunnel.c | 14 +++-----
drivers/iio/accel/cros_ec_accel_legacy.c | 27 +++++----------
.../common/cros_ec_sensors/cros_ec_sensors.c | 24 ++++---------
.../cros_ec_sensors/cros_ec_sensors_core.c | 18 +++-------
.../cros_ec_sensors/cros_ec_sensors_core.h | 12 ++-----
drivers/iio/light/cros_ec_light_prox.c | 18 +++-------
drivers/iio/pressure/cros_ec_baro.c | 18 +++-------
drivers/input/keyboard/cros_ec_keyb.c | 34 +++++++------------
drivers/mfd/cros_ec.c | 26 +++++---------
drivers/mfd/cros_ec_dev.c | 23 +++----------
drivers/mfd/cros_ec_dev.h | 16 ++-------
drivers/mfd/cros_ec_i2c.c | 18 +++-------
drivers/mfd/cros_ec_spi.c | 20 +++--------
drivers/platform/chrome/chromeos_pstore.c | 13 +++----
drivers/platform/chrome/cros_ec_debugfs.c | 22 +++---------
drivers/platform/chrome/cros_ec_lightbar.c | 22 +++---------
drivers/platform/chrome/cros_ec_lpc.c | 34 +++++++------------
drivers/platform/chrome/cros_ec_lpc_mec.c | 34 +++++++------------
drivers/platform/chrome/cros_ec_lpc_reg.c | 34 +++++++------------
drivers/platform/chrome/cros_ec_proto.c | 19 +++--------
drivers/platform/chrome/cros_ec_sysfs.c | 22 +++---------
drivers/platform/chrome/cros_ec_vbc.c | 24 +++----------
.../platform/chrome/cros_kbd_led_backlight.c | 19 +++--------
drivers/pwm/pwm-cros-ec.c | 13 +++----
include/linux/mfd/cros_ec.h | 10 +-----
include/linux/mfd/cros_ec_commands.h | 10 +-----
include/linux/mfd/cros_ec_lpc_mec.h | 14 ++------
include/linux/mfd/cros_ec_lpc_reg.h | 14 ++------
29 files changed, 152 insertions(+), 440 deletions(-)
--
2.17.1
^ permalink raw reply
* [RFC/PATCH] Input: make input_report_slot_state() return boolean
From: Dmitry Torokhov @ 2018-06-05 17:16 UTC (permalink / raw)
To: linux-input; +Cc: Henrik Rydberg, Benjamin Tissoires, linux-kernel
Let's make input_report_slot_state() return boolean representing whether
the contact is active or not. This will allow writing code like:
if (input_mt_report_slot_state(input, obj->mt_tool,
obj->type != RMI_2D_OBJECT_NONE) {
input_event(sensor->input, EV_ABS, ABS_MT_POSITION_X, obj->x);
input_event(sensor->input, EV_ABS, ABS_MT_POSITION_Y, obj->y);
...
}
instead of:
input_mt_report_slot_state(input, obj->mt_tool,
obj->type != RMI_2D_OBJECT_NONE);
if (obj->type != RMI_2D_OBJECT_NONE) {
input_event(sensor->input, EV_ABS, ABS_MT_POSITION_X, obj->x);
input_event(sensor->input, EV_ABS, ABS_MT_POSITION_Y, obj->y);
...
}
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/input-mt.c | 10 +++++++---
include/linux/input/mt.h | 2 +-
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index 7ca4b318ed419..4a69716e54614 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -131,8 +131,10 @@ EXPORT_SYMBOL(input_mt_destroy_slots);
* inactive, or if the tool type is changed, a new tracking id is
* assigned to the slot. The tool type is only reported if the
* corresponding absbit field is set.
+ *
+ * Returns true if contact is active.
*/
-void input_mt_report_slot_state(struct input_dev *dev,
+bool input_mt_report_slot_state(struct input_dev *dev,
unsigned int tool_type, bool active)
{
struct input_mt *mt = dev->mt;
@@ -140,14 +142,14 @@ void input_mt_report_slot_state(struct input_dev *dev,
int id;
if (!mt)
- return;
+ return false;
slot = &mt->slots[mt->slot];
slot->frame = mt->frame;
if (!active) {
input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1);
- return;
+ return false;
}
id = input_mt_get_value(slot, ABS_MT_TRACKING_ID);
@@ -156,6 +158,8 @@ void input_mt_report_slot_state(struct input_dev *dev,
input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, id);
input_event(dev, EV_ABS, ABS_MT_TOOL_TYPE, tool_type);
+
+ return true;
}
EXPORT_SYMBOL(input_mt_report_slot_state);
diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
index d7188de4db968..3f4bf60b0bb55 100644
--- a/include/linux/input/mt.h
+++ b/include/linux/input/mt.h
@@ -100,7 +100,7 @@ static inline bool input_is_mt_axis(int axis)
return axis == ABS_MT_SLOT || input_is_mt_value(axis);
}
-void input_mt_report_slot_state(struct input_dev *dev,
+bool input_mt_report_slot_state(struct input_dev *dev,
unsigned int tool_type, bool active);
void input_mt_report_finger_count(struct input_dev *dev, int count);
--
2.17.1.1185.g55be947832-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Dmitry Torokhov @ 2018-06-05 17:05 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Peter Hutterer, Jiri Kosina, Henrik Rydberg, Jason Gerecke,
Dennis Kempin, Andrew de los Reyes, open list:HID CORE LAYER,
lkml
In-Reply-To: <CAO-hwJLzc_bVDLKTExTZLSEYON3zu54Q0COe57xGGQ5AS6OPRw@mail.gmail.com>
On Tue, Jun 05, 2018 at 03:50:15PM +0200, Benjamin Tissoires wrote:
> On Tue, Jun 5, 2018 at 12:55 AM, Peter Hutterer
> <peter.hutterer@who-t.net> wrote:
> > On Mon, Jun 04, 2018 at 02:19:44PM -0700, Dmitry Torokhov wrote:
> >> On Mon, Jun 04, 2018 at 10:42:31PM +0200, Benjamin Tissoires wrote:
> >> > On Mon, Jun 4, 2018 at 7:33 PM, Dmitry Torokhov
> >> > <dmitry.torokhov@gmail.com> wrote:
> >> > > On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
> >> > >> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
> >> > >> <dmitry.torokhov@gmail.com> wrote:
> >> > >> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
> >> > >> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
> >> > >> >> <dmitry.torokhov@gmail.com> wrote:
> >> > >> >> > According to Microsoft specification [1] for Precision Touchpads (and
> >> > >> >> > Touchscreens) the devices use "confidence" reports to signal accidental
> >> > >> >> > touches, or contacts that are "too large to be a finger". Instead of
> >> > >> >> > simply marking contact inactive in this case (which causes issues if
> >> > >> >> > contact was originally proper and we lost confidence in it later, as
> >> > >> >> > this results in accidental clicks, drags, etc), let's report such
> >> > >> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
> >> > >> >> > Additionally, let's report contact size for such touches as maximum
> >> > >> >> > allowed for major/minor, which should help userspace that is not yet
> >> > >> >> > aware of MT_TOOL_PALM to still perform palm rejection.
> >> > >> >> >
> >> > >> >> > An additional complication, is that some firmwares do not report
> >> > >> >> > non-confident touches as active. To cope with this we delay release of
> >> > >> >> > such contact (i.e. if contact was active we first report it as still
> >> > >> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
> >> > >> >> > frame).
> >> > >> >>
> >> > >> >> I am not sure I agree with this part. The spec says that "Once a
> >> > >> >> device has determined that a contact is unintentional, it should clear
> >> > >> >> the confidence bit for that contact report and all subsequent
> >> > >> >> reports."
> >> > >> >> So in theory the spec says that if a touch has been detected as a
> >> > >> >> palm, the flow of events should not stop (tested on the PTP of the
> >> > >> >> Dell XPS 9360).
> >> > >> >>
> >> > >> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
> >> > >> >> and then (confidence 0, tip switch 0) a simple release, and the
> >> > >> >> confidence bit should not be relayed.
> >> > >> >
> >> > >> > This unfortunately leads to false clicks: you start with finger, so
> >> > >> > confidence is 1, then you transition the same touch to palm (use your
> >> > >> > thumb and "roll" your hand until heel of it comes into contact with the
> >> > >> > screen). The firmware reports "no-confidence" and "release" in the same
> >> > >> > report and userspace seeing release does not pay attention to confidence
> >> > >> > (i.e. it does exactly "simple release" logic) and this results in UI
> >> > >> > interpreting this as a click. With splitting no-confidence
> >> > >> > (MT_TOOL_PALM) and release event into separate frames we help userspace
> >> > >> > to recognize that the contact should be discarded.
> >> > >>
> >> > >> After further thoughts, I would consider this to be a firmware bug,
> >> > >> and not how the firmware is supposed to be reporting palm.
> >> > >> For the precision touchpads, the spec says that the device "should
> >> > >> clear the confidence bit for that contact report and all subsequent
> >> > >> reports.". And it is how the Dell device I have here reports palms.
> >> > >> The firmware is not supposed to cut the event stream.
> >> > >>
> >> > >> There is a test for that:
> >> > >> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
> >> > >> which tells me that I am right here for PTP.
> >> > >>
> >> > >> The touchscreen spec is blurrier however.
> >> > >
> >> > > OK, that is great to know.
> >> > >
> >> > >>
> >> > >> >
> >> > >> >>
> >> > >> >> Do you have any precise example of reports where you need that feature?
> >> > >> >
> >> > >> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
> >> > >>
> >> > >> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
> >> > >> guessing the device did not went through Microsoft certification
> >> > >> process?
> >> > >
> >> > > That would be correct ;) At least the firmware that is shipping with
> >> > > Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
> >> > > their MSWin devices.
> >> > >
> >> > >>
> >> > >> I am in favor of splitting the patch in 2. One for the generic
> >> > >> processing of confidence bit, and one for this spurious release. For
> >> > >> the spurious release, I'm more in favor of explicitly quirking the
> >> > >> devices in need of such quirk.
> >> > >
> >> > > Hmm, I am not sure about having specific quirk. It will be hard for
> >> > > users to accurately diagnose the issue if firmware is broken in this way
> >> > > so we could add a new quirk for a new device.
> >> >
> >> > One thing we can do is keep the quirked mechanism as default in
> >> > hid-multitouch, but remove it in hid-core. If people need the quirk,
> >> > they can just use hid-multitouch instead (talking about the long run
> >> > here).
> >>
> >> Hmm, I am confused. My patch did not touch hid-core or hid-input, only
> >> hid-multitouch... So we are already doing what you are proposing?..
> >>
> >> >
> >> > However, I really believe this might only be required for a handful of
> >> > devices, and probably only touchscreens. So I would be tempted to not
> >> > make it default and see how many bug reports we have.
> >>
> >> Up to you but it is hard to detect for users. If just sometimes there
> >> are stray clicks...
> >
> > fwiw, from my POV, if you give me MT_TOOL_PALM in the same frame as the
> > ABS_MT_TRACKING_ID -1 I can work that into libinput to do the right thing.
>
> This would be a one line change in the kernel, so you got my attention :)
Umm, there are other input stacks beyond libinput.
>
> > Not 100% whether that already works anyway but probably not. I'd prefer it
> > being fixed in the kernel though, less work for me :)
>
> What do you mean by "fixed"?
> Is it incorrect to send a tool while tracking ID is set to -1?
> From what I read on multi-touch-protocol.rst this shouldn't be
> violating the protocol, and this would save quite a mess in the kernel
> in which we need to add an artificial event in the queue for the
> release.
Well, we say "A non-negative tracking id is interpreted as a contact,
and the value -1 denotes an unused slot." Unless you are a protocol
lawyer, the most sensible way of interpreting it is to ignore whatever
is transmitted for the slot once receiving tracking ID of -1.
Given that this is particular firmware quirk that sends confidence and
release in the same report, I'd prefer if we had a quirk in driver
rather than pushing the responsibility to userspace.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 6/9] Input: cros_ec_keyb - Switch to SPDX identifier.
From: Dmitry Torokhov @ 2018-06-05 16:37 UTC (permalink / raw)
To: Enric Balletbo i Serra; +Cc: linux-kernel, Gwendal Grignou, kernel, linux-input
In-Reply-To: <20180605092209.22887-7-enric.balletbo@collabora.com>
On Tue, Jun 05, 2018 at 11:22:06AM +0200, Enric Balletbo i Serra wrote:
> Adopt the SPDX license identifier headers to ease license compliance
> management.
>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
>
> drivers/input/keyboard/cros_ec_keyb.c | 26 ++++----------------------
> 1 file changed, 4 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 79eb29550c34..342513ec0ede 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -1,25 +1,7 @@
> -/*
> - * ChromeOS EC keyboard driver
> - *
> - * Copyright (C) 2012 Google, Inc
> - *
> - * This software is licensed under the terms of the GNU General Public
> - * License version 2, as published by the Free Software Foundation, and
> - * may be copied, distributed, and modified under those terms.
> - *
> - * This program is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> - * GNU General Public License for more details.
> - *
> - * This driver uses the Chrome OS EC byte-level message-based protocol for
> - * communicating the keyboard state (which keys are pressed) from a keyboard EC
> - * to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
> - * but everything else (including deghosting) is done here. The main
> - * motivation for this is to keep the EC firmware as simple as possible, since
> - * it cannot be easily upgraded and EC flash/IRAM space is relatively
> - * expensive.
> - */
Ugh, why did you remove this last paragraph? It has nothing to do with
license notice and should have been kept.
> +// SPDX-License-Identifier: GPL-2.0+
> +// ChromeOS EC keyboard driver
> +//
> +// Copyright (C) 2012 Google, Inc.
>
> #include <linux/module.h>
> #include <linux/bitops.h>
> --
> 2.17.1
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Benjamin Tissoires @ 2018-06-05 13:50 UTC (permalink / raw)
To: Peter Hutterer
Cc: Dmitry Torokhov, Jiri Kosina, Henrik Rydberg, Jason Gerecke,
Dennis Kempin, Andrew de los Reyes, open list:HID CORE LAYER,
lkml
In-Reply-To: <20180604225539.GA13447@jelly>
On Tue, Jun 5, 2018 at 12:55 AM, Peter Hutterer
<peter.hutterer@who-t.net> wrote:
> On Mon, Jun 04, 2018 at 02:19:44PM -0700, Dmitry Torokhov wrote:
>> On Mon, Jun 04, 2018 at 10:42:31PM +0200, Benjamin Tissoires wrote:
>> > On Mon, Jun 4, 2018 at 7:33 PM, Dmitry Torokhov
>> > <dmitry.torokhov@gmail.com> wrote:
>> > > On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
>> > >> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
>> > >> <dmitry.torokhov@gmail.com> wrote:
>> > >> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
>> > >> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
>> > >> >> <dmitry.torokhov@gmail.com> wrote:
>> > >> >> > According to Microsoft specification [1] for Precision Touchpads (and
>> > >> >> > Touchscreens) the devices use "confidence" reports to signal accidental
>> > >> >> > touches, or contacts that are "too large to be a finger". Instead of
>> > >> >> > simply marking contact inactive in this case (which causes issues if
>> > >> >> > contact was originally proper and we lost confidence in it later, as
>> > >> >> > this results in accidental clicks, drags, etc), let's report such
>> > >> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
>> > >> >> > Additionally, let's report contact size for such touches as maximum
>> > >> >> > allowed for major/minor, which should help userspace that is not yet
>> > >> >> > aware of MT_TOOL_PALM to still perform palm rejection.
>> > >> >> >
>> > >> >> > An additional complication, is that some firmwares do not report
>> > >> >> > non-confident touches as active. To cope with this we delay release of
>> > >> >> > such contact (i.e. if contact was active we first report it as still
>> > >> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
>> > >> >> > frame).
>> > >> >>
>> > >> >> I am not sure I agree with this part. The spec says that "Once a
>> > >> >> device has determined that a contact is unintentional, it should clear
>> > >> >> the confidence bit for that contact report and all subsequent
>> > >> >> reports."
>> > >> >> So in theory the spec says that if a touch has been detected as a
>> > >> >> palm, the flow of events should not stop (tested on the PTP of the
>> > >> >> Dell XPS 9360).
>> > >> >>
>> > >> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
>> > >> >> and then (confidence 0, tip switch 0) a simple release, and the
>> > >> >> confidence bit should not be relayed.
>> > >> >
>> > >> > This unfortunately leads to false clicks: you start with finger, so
>> > >> > confidence is 1, then you transition the same touch to palm (use your
>> > >> > thumb and "roll" your hand until heel of it comes into contact with the
>> > >> > screen). The firmware reports "no-confidence" and "release" in the same
>> > >> > report and userspace seeing release does not pay attention to confidence
>> > >> > (i.e. it does exactly "simple release" logic) and this results in UI
>> > >> > interpreting this as a click. With splitting no-confidence
>> > >> > (MT_TOOL_PALM) and release event into separate frames we help userspace
>> > >> > to recognize that the contact should be discarded.
>> > >>
>> > >> After further thoughts, I would consider this to be a firmware bug,
>> > >> and not how the firmware is supposed to be reporting palm.
>> > >> For the precision touchpads, the spec says that the device "should
>> > >> clear the confidence bit for that contact report and all subsequent
>> > >> reports.". And it is how the Dell device I have here reports palms.
>> > >> The firmware is not supposed to cut the event stream.
>> > >>
>> > >> There is a test for that:
>> > >> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
>> > >> which tells me that I am right here for PTP.
>> > >>
>> > >> The touchscreen spec is blurrier however.
>> > >
>> > > OK, that is great to know.
>> > >
>> > >>
>> > >> >
>> > >> >>
>> > >> >> Do you have any precise example of reports where you need that feature?
>> > >> >
>> > >> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
>> > >>
>> > >> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
>> > >> guessing the device did not went through Microsoft certification
>> > >> process?
>> > >
>> > > That would be correct ;) At least the firmware that is shipping with
>> > > Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
>> > > their MSWin devices.
>> > >
>> > >>
>> > >> I am in favor of splitting the patch in 2. One for the generic
>> > >> processing of confidence bit, and one for this spurious release. For
>> > >> the spurious release, I'm more in favor of explicitly quirking the
>> > >> devices in need of such quirk.
>> > >
>> > > Hmm, I am not sure about having specific quirk. It will be hard for
>> > > users to accurately diagnose the issue if firmware is broken in this way
>> > > so we could add a new quirk for a new device.
>> >
>> > One thing we can do is keep the quirked mechanism as default in
>> > hid-multitouch, but remove it in hid-core. If people need the quirk,
>> > they can just use hid-multitouch instead (talking about the long run
>> > here).
>>
>> Hmm, I am confused. My patch did not touch hid-core or hid-input, only
>> hid-multitouch... So we are already doing what you are proposing?..
>>
>> >
>> > However, I really believe this might only be required for a handful of
>> > devices, and probably only touchscreens. So I would be tempted to not
>> > make it default and see how many bug reports we have.
>>
>> Up to you but it is hard to detect for users. If just sometimes there
>> are stray clicks...
>
> fwiw, from my POV, if you give me MT_TOOL_PALM in the same frame as the
> ABS_MT_TRACKING_ID -1 I can work that into libinput to do the right thing.
This would be a one line change in the kernel, so you got my attention :)
> Not 100% whether that already works anyway but probably not. I'd prefer it
> being fixed in the kernel though, less work for me :)
What do you mean by "fixed"?
Is it incorrect to send a tool while tracking ID is set to -1?
>From what I read on multi-touch-protocol.rst this shouldn't be
violating the protocol, and this would save quite a mess in the kernel
in which we need to add an artificial event in the queue for the
release.
Cheers,
Benjamin
^ permalink raw reply
* [PATCH 6/9] Input: cros_ec_keyb - Switch to SPDX identifier.
From: Enric Balletbo i Serra @ 2018-06-05 9:22 UTC (permalink / raw)
To: linux-kernel; +Cc: Gwendal Grignou, kernel, Dmitry Torokhov, linux-input
In-Reply-To: <20180605092209.22887-1-enric.balletbo@collabora.com>
Adopt the SPDX license identifier headers to ease license compliance
management.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/input/keyboard/cros_ec_keyb.c | 26 ++++----------------------
1 file changed, 4 insertions(+), 22 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 79eb29550c34..342513ec0ede 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -1,25 +1,7 @@
-/*
- * ChromeOS EC keyboard driver
- *
- * Copyright (C) 2012 Google, Inc
- *
- * This software is licensed under the terms of the GNU General Public
- * License version 2, as published by the Free Software Foundation, and
- * may be copied, distributed, and modified under those terms.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
- *
- * This driver uses the Chrome OS EC byte-level message-based protocol for
- * communicating the keyboard state (which keys are pressed) from a keyboard EC
- * to the AP over some bus (such as i2c, lpc, spi). The EC does debouncing,
- * but everything else (including deghosting) is done here. The main
- * motivation for this is to keep the EC firmware as simple as possible, since
- * it cannot be easily upgraded and EC flash/IRAM space is relatively
- * expensive.
- */
+// SPDX-License-Identifier: GPL-2.0+
+// ChromeOS EC keyboard driver
+//
+// Copyright (C) 2012 Google, Inc.
#include <linux/module.h>
#include <linux/bitops.h>
--
2.17.1
^ permalink raw reply related
* [PATCH 0/9] croc_ec: Switch to SPDX identifier
From: Enric Balletbo i Serra @ 2018-06-05 9:22 UTC (permalink / raw)
To: linux-kernel
Cc: Gwendal Grignou, kernel, Tony Luck, Alessandro Zummo,
Paolo Cretaro, linux-iio, Alexandre Belloni, linux-i2c,
Olof Johansson, linux-rtc, Anton Vorontsov, Chanwoo Choi,
Benson Leung, linux-input, Dmitry Torokhov, Jonathan Cameron,
Peter Meerwald-Stadler, linux-pwm, Thierry Reding, MyungJoo Ham,
Lee Jones
Dear all,
This patchset is just to adopt the SPDX license identifier for all
ChromeOS Embedded Controller related drivers. The patches touches
different subsystems but every patch can be picked from their respective
maintainer independently.
Thanks,
Enric
Enric Balletbo i Serra (9):
platform/chrome: cros_ec: Switch to SPDX identifier.
platform/chrome: pstore: Switch to SPDX identifier.
mfd: cros_ec: Add or fix SPDX-License-Identifier in all files.
iio: cros_ec: Switch to SPDX identifier.
rtc: cros-ec: Switch to SPDX identifier.
Input: cros_ec_keyb - Switch to SPDX identifier.
pwm: cros-ec: Switch to SPDX identifier.
extcon: usbc-cros-ec: Switch to SPDX identifier.
i2c: i2c-cros-ec-tunnel: Switch to SPDX identifier.
drivers/extcon/extcon-usbc-cros-ec.c | 20 ++++----------
drivers/i2c/busses/i2c-cros-ec-tunnel.c | 14 +++-------
drivers/iio/accel/cros_ec_accel_legacy.c | 23 ++++------------
.../common/cros_ec_sensors/cros_ec_sensors.c | 24 +++++------------
.../cros_ec_sensors/cros_ec_sensors_core.c | 18 +++----------
.../cros_ec_sensors/cros_ec_sensors_core.h | 12 ++-------
drivers/iio/light/cros_ec_light_prox.c | 18 +++----------
drivers/iio/pressure/cros_ec_baro.c | 18 +++----------
drivers/input/keyboard/cros_ec_keyb.c | 26 +++---------------
drivers/mfd/cros_ec.c | 27 +++++++------------
drivers/mfd/cros_ec_dev.c | 24 +++++------------
drivers/mfd/cros_ec_dev.h | 16 ++---------
drivers/mfd/cros_ec_i2c.c | 19 ++++---------
drivers/mfd/cros_ec_spi.c | 21 +++++----------
drivers/platform/chrome/chromeos_pstore.c | 13 +++------
drivers/platform/chrome/cros_ec_debugfs.c | 22 +++------------
drivers/platform/chrome/cros_ec_lightbar.c | 22 +++------------
drivers/platform/chrome/cros_ec_lpc.c | 26 +++---------------
drivers/platform/chrome/cros_ec_lpc_mec.c | 26 +++---------------
drivers/platform/chrome/cros_ec_lpc_reg.c | 26 +++---------------
drivers/platform/chrome/cros_ec_proto.c | 19 +++----------
drivers/platform/chrome/cros_ec_sysfs.c | 22 +++------------
drivers/platform/chrome/cros_ec_vbc.c | 24 ++++-------------
.../platform/chrome/cros_kbd_led_backlight.c | 19 +++----------
drivers/pwm/pwm-cros-ec.c | 13 +++------
drivers/rtc/rtc-cros-ec.c | 21 ++++-----------
include/linux/mfd/cros_ec.h | 10 +------
include/linux/mfd/cros_ec_commands.h | 10 +------
include/linux/mfd/cros_ec_lpc_mec.h | 12 ++-------
include/linux/mfd/cros_ec_lpc_reg.h | 12 ++-------
30 files changed, 123 insertions(+), 454 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH V2] i8042: Increment wakeup_count for the respective port.
From: Rafael J. Wysocki @ 2018-06-05 9:14 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Ravi Chandra Sadineni, Rafael J. Wysocki, chenhong3,
Ravi Chandra Sadineni, Dmitry Torokhov, Todd Broch,
Linux Kernel Mailing List, linux-input, Rajat Jain, Benson Leung
In-Reply-To: <20180604215339.GG164893@dtor-ws>
On Mon, Jun 4, 2018 at 11:53 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Jun 01, 2018 at 06:07:08PM -0700, Ravi Chandra Sadineni wrote:
>> Call pm_wakeup_event on every irq. This should help us in identifying if
>> keyboard was a potential wake reason for the last resume.
>>
>> Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
>> ---
>> V2: Increment the wakeup count only when there is a irq and not when the
>> method is called internally.
>>
>> drivers/input/serio/i8042.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
>> index 824f4c1c1f310..2bd6f2633e29a 100644
>> --- a/drivers/input/serio/i8042.c
>> +++ b/drivers/input/serio/i8042.c
>> @@ -573,6 +573,9 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
>> port = &i8042_ports[port_no];
>> serio = port->exists ? port->serio : NULL;
>>
>> + if (irq && serio && device_may_wakeup(&serio->dev))
>> + pm_wakeup_event(&serio->dev, 0);
>
> The constant checks for device_may_wakeup() before calling
> pm_wakeup_event()needed to avoid warnings in wakeup_source_activate()
> (?) are annoying.
I'm not following you here.
pm_wakeup_event() ->
pm_wakeup_dev_event() ->
pm_wakeup_ws_event(dev->power.wakeup, ...)
Checks if the first arg is NULL and returns quietly if so.
I don't see why you need the device_may_wakeup() check.
> Rafael, can we move the check into pm_wakeup_dev_event()?
That would be redundant, wouldn't it?
> I am also confused when pm_wakeup_event() vs pm_wakeup_hard_event() vs
> pm_wakeup_dev_event() should be used, if any. Is there any guidance?
First off, the "hard" variant is for when you want to abort suspends
in progress or wake up from suspend to idle regardless of whether or
not wakeup source tracking is enabled.
Second, use pm_wakeup_dev_event() if the decision on "hard" vs "soft"
needs to be made at run time.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device.
From: Brian Norris @ 2018-06-05 1:32 UTC (permalink / raw)
To: Ravi Chandra Sadineni
Cc: dmitry.torokhov, lee.jones, ravisadineni, dtor, tbroch,
linux-kernel, linux-input, rajatja, bleung
In-Reply-To: <20180526011440.102417-1-ravisadineni@chromium.org>
Hi,
On Fri, May 25, 2018 at 06:14:40PM -0700, Ravi Chandra Sadineni wrote:
> Mark cros_ec_keyb has wake enabled by default. If we see a MKBP event
> related to keyboard, call pm_wakeup_event() to make sure wakeup
> triggers are accounted to keyb during suspend resume path.
>
> Signed-off-by: Ravi Chandra Sadineni <ravisadineni@chromium.org>
> ---
> V2: Marked the ckdev as wake enabled instead of input devices.
I'm not sure I saw v1? But FYI, if you're not marking the input devices
as wake-enabled, then you only have one knob for disabling wakeup on the
buttons and switches (e.g., power button) vs. the keyboard. I know you
were previously concerned about this, but given that the EC itself
usually has full knowledge of these situations (e.g., it knows to
disable keyboard wakeup when a convertible is in a tablet orientation,
but leave power-button wakeup enabled), this may not be a problem.
Anyway, just wanted to highlight that part.
> drivers/input/keyboard/cros_ec_keyb.c | 21 +++++++++++++++++----
> drivers/mfd/cros_ec.c | 19 +++++++------------
> 2 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 79eb29550c348..a7c96f0317123 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -245,12 +245,17 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> switch (ckdev->ec->event_data.event_type) {
> case EC_MKBP_EVENT_KEY_MATRIX:
> /*
> - * If EC is not the wake source, discard key state changes
> + * If Keyb is not wake enabled, discard key state changes
We can use the full word; text is cheap:
s/Keyb/keyboard/
> * during suspend.
> */
> - if (queued_during_suspend)
> + if (queued_during_suspend
> + && !device_may_wakeup(ckdev->dev))
> return NOTIFY_OK;
>
> + if (device_may_wakeup(ckdev->dev))
> + pm_wakeup_event(ckdev->dev, 0);
> +
> +
I don't think you need two blank lines here. One is enough.
> if (ckdev->ec->event_size != ckdev->cols) {
> dev_err(ckdev->dev,
> "Discarded incomplete key matrix event.\n");
> @@ -265,18 +270,25 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> val = get_unaligned_le32(&ckdev->ec->event_data.data.sysrq);
> dev_dbg(ckdev->dev, "sysrq code from EC: %#x\n", val);
> handle_sysrq(val);
> +
> + if (device_may_wakeup(ckdev->dev))
> + pm_wakeup_event(ckdev->dev, 0);
> break;
>
> case EC_MKBP_EVENT_BUTTON:
> case EC_MKBP_EVENT_SWITCH:
> /*
> - * If EC is not the wake source, discard key state
> + * If keyb is not wake enabled, discard key state
s/keyb/keyboard/
Or, since this is talking about buttons and switches (which don't
technically require the "keyboard" part of this driver), you might just
leave that off ("If not wake enabled...").
Otherwise, I believe this looks good, though I may have overlooked
something:
Reviewed-by: Brian Norris <briannorris@chromium.org>
And given Lee acked this, and it's mostly a keyboard change, it should
probably go through Dmitry? And I'd expect he'd be a better reviewer
than me anyway.
Brian
> * changes during suspend. Switches will be re-checked in
> * cros_ec_keyb_resume() to be sure nothing is lost.
> */
> - if (queued_during_suspend)
> + if (queued_during_suspend
> + && !device_may_wakeup(ckdev->dev))
> return NOTIFY_OK;
>
> + if (device_may_wakeup(ckdev->dev))
> + pm_wakeup_event(ckdev->dev, 0);
> +
> if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
> val = get_unaligned_le32(
> &ckdev->ec->event_data.data.buttons);
> @@ -639,6 +651,7 @@ static int cros_ec_keyb_probe(struct platform_device *pdev)
> return err;
> }
>
> + device_init_wakeup(ckdev->dev, true);
> return 0;
> }
>
> diff --git a/drivers/mfd/cros_ec.c b/drivers/mfd/cros_ec.c
> index d61024141e2b6..36156a41499c9 100644
> --- a/drivers/mfd/cros_ec.c
> +++ b/drivers/mfd/cros_ec.c
> @@ -229,7 +229,7 @@ int cros_ec_suspend(struct cros_ec_device *ec_dev)
> }
> EXPORT_SYMBOL(cros_ec_suspend);
>
> -static void cros_ec_drain_events(struct cros_ec_device *ec_dev)
> +static void cros_ec_report_events_during_suspend(struct cros_ec_device *ec_dev)
> {
> while (cros_ec_get_next_event(ec_dev, NULL) > 0)
> blocking_notifier_call_chain(&ec_dev->event_notifier,
> @@ -253,21 +253,16 @@ int cros_ec_resume(struct cros_ec_device *ec_dev)
> dev_dbg(ec_dev->dev, "Error %d sending resume event to ec",
> ret);
>
> - /*
> - * In some cases, we need to distinguish between events that occur
> - * during suspend if the EC is not a wake source. For example,
> - * keypresses during suspend should be discarded if it does not wake
> - * the system.
> - *
> - * If the EC is not a wake source, drain the event queue and mark them
> - * as "queued during suspend".
> - */
> if (ec_dev->wake_enabled) {
> disable_irq_wake(ec_dev->irq);
> ec_dev->wake_enabled = 0;
> - } else {
> - cros_ec_drain_events(ec_dev);
> }
> + /*
> + * Let the mfd devices know about events that occur during
> + * suspend. This way the clients know what to do with them.
> + */
> + cros_ec_report_events_during_suspend(ec_dev);
> +
>
> return 0;
> }
> --
> 2.17.0.921.gf22659ad46-goog
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Dmitry Torokhov @ 2018-06-04 23:54 UTC (permalink / raw)
To: Peter Hutterer
Cc: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Jason Gerecke,
Dennis Kempin, Andrew de los Reyes, open list:HID CORE LAYER,
lkml
In-Reply-To: <20180604235114.GA13763@jelly>
On Tue, Jun 05, 2018 at 09:51:14AM +1000, Peter Hutterer wrote:
> On Mon, Jun 04, 2018 at 04:28:01PM -0700, Dmitry Torokhov wrote:
> > On Tue, Jun 05, 2018 at 09:06:24AM +1000, Peter Hutterer wrote:
> > > On Mon, Jun 04, 2018 at 02:32:55PM -0700, Dmitry Torokhov wrote:
> > > > On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
> > > > > On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
> > > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
> > > > > >> Hi Dmitry,
> > > > > >>
> > > > > >> > > > Logically, the confidence state is a property of a contact, not a new type
> > > > > >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
> > > > > >> > > >
> > > > > >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
> > > > > >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> > > > > >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> > > > > >> > > > I can't find any other users in the current upstream tree, but those
> > > > > >> > > > two are already making a precedent and changing the semantic is a
> > > > > >> > > > little bit late :/
> > > > > >> > I am sorry I did not respond and lost track of this issue back then, but
> > > > > >> > I disagree with Henrik here. While confidence is a property of contact,
> > > > > >> > so is the type of contact and it can and will change throughout life of
> > > > > >> > a contact, especially if we will continue adding new types, such as, for
> > > > > >> > example, thumb. In this case the firmware can transition through
> > > > > >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
> > > > > >> > contact becomes better understood. Still it is the same contact and we
> > > > > >> > should not attempt to signal userspace differently.
> > > > > >> We agree that the contact should stay the same, but the fear, and I think
> > > > > >> somewhere along the blurry history of this thread, the problem was that
> > > > > >> userspace interpreted the property change as a new contact (lift up/double
> > > > > >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
> > > > > >> Pen? It would be hard for an application to consider a switch from finger to
> > > > > >> pen as the same contact, which is where the natural implementation starts to
> > > > > >> diverge from the intention.
> > > > > >
> > > > > > I think the userspace has to trust our tracking ID to decide whether it
> > > > > > is a same contact or not. The current issue is that kernel is forcing
> > > > > > tracking ID change on tool type change, and one of the 2 patches that I
> > > > > > posted fixed that, allowing us to keep the tracking ID for finger->palm
> > > > > > transitions.
> > > > >
> > > > > I think I missed those 2 patches, can you point a LKML link?
> > > >
> > > > Sorry, I thought I sent it out with the patch we are talking about here,
> > > > but I did not. See below. Note that it doe snot have any protections on
> > > > finger->pen transitions and I am not sure any are needed at the moment.
> > > > We can add them wither to MT core or to drivers if we see issues with
> > > > devices.
> > > >
> > > > > Also, note that libevdev discards the tracking ID change now (it
> > > > > shouts at the user in the logs). So that means that it will now be
> > > > > hard to force libevdev to trust the kernel again for the tracking ID.
> > > > > The current rule is:
> > > > > - tracking ID >= 0 -> new touch
> > > > > - any subsequent tracking ID >= 0 -> discarded
> > > > > - tracking ID == -1 -> end of touch
> > > >
> > > > Well, I guess it is like synaptics driver that used to dump core
> > > > whenever it saw tracking ID change for the same slot (not going though
> > > > -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> > > > us having to produce weird results when users would use fancy gestures
> > > > with 3+ fingers.
> > >
> > > yeah, my mistake, sorry. I always assumed a transition from M to -1 to N,
> > > never M to N. This assumption made its way into libevdev (where the tracking
> > > ID is transparently discarded, albeit with a warning). There are libevdev
> > > patches to get rid of that but whatever device needed it got fixed in some
> > > other way, so the patch didn't get pushed.
> > >
> > > fwiw, dump core was just "print the backtrace to the log" here, there was no
> > > actual core dump.
> >
> > Hmm, I do not recall what version I was playing with, but I tried
> > changing Synaptics kernel driver to not insert the fake -1 tracking ID
> > for a slot when rolling 3 fingers on a 2-slot device (so removing finger
> > 1 while holding finger 2 and adding finger 3 does not appear to
> > userspace as 2 - 1 - 2 fingers on the surface, but 2 - 2 - 2 instead)
> > and xf86-input-synaptics-1.7.8 would scream about too many slots and
> > stop working.
> >
> > That was a while ago though, before libinput I think.
> >
> > >
> > > > It probably does not matter with devices with 5+ slots. We should pretty
> > > > much always have free slot for new contact.
> > > >
> > > > >
> > > > > >
> > > > > > I think it is kernel task to not signal transitions that do not make
> > > > > > sense, such as finger->pen or palm->pen etc.
> > > > >
> > > > > I fully agree, though there is currently no such guard in the kernel
> > > > > (maybe it's part of your series). I am worried about the RMI4 F12
> > > > > driver that automatically forward the info from the firmware, so if
> > > > > the firmware does something crazy, it will be exported to user space.
> > > > > But I guess it might be better to treat that on a per driver basis.
> > > >
> > > > Yeah, I think so.
> > > >
> > > > >
> > > > > >
> > > > > >>
> > > > > >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> > > > > >> > complement ABS_MT_TOOL, but that would not really solve the issue with
> > > > > >> > Wacom firmware (declaring contact non-confident and releasing it right
> > > > > >> > away) and given MS explanation of the confidence as "contact is too big"
> > > > > >> > MT_TOOL_PALM fits it perfectly.
> > > > > >> Indeed, the Wacom firmware seems to need some special handling, which should
> > > > > >> be fine by everyone. I do think it would make sense to add
> > > > > >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
> > > > >
> > > > > Except we are already running out of ABS_* axes.
> > > >
> > > > Sorry, meants MT_TOOL_TO_BIG, not a new axis.
> > >
> > > bikeshed: MT_TOOL_IGNORE is a more generic name and does not imply size. A
> > > pen that's lying on its side doesn't have a size but should still be
> > > ignored.
> >
> > OK, when we start seeing this for non finger/thumb/palm objects we can
> > add this tool type. For current devices we are dealing with palms.
> >
> > >
> > > > >
> > > > > >> also to a pen lying down on a touchpad, for instance.
> > > > > >
> > > > > > OK, I can see that for Pens, if we have firmware that would recognize
> > > > > > such condition, it would be weird to report PALM. We could indeed have
> > > > > > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
> > > > > > the hardware can recognize it as such). Maybe we'd be better off just
> > > > > > having userspace going by contact size for pens. Peter, any suggestions
> > > > > > here?
> > > > >
> > > > > I don't think we have size handling in the tablet implementation in
> > > > > libinput. I do not see it as a big issue to add such axes from a
> > > > > libinput point of view. However, there is no existing hardware that
> > > > > would provide such information, so I guess this will be a 'no' until
> > > > > actual hardware comes in.
> > >
> > > correct on all counts :)
> > >
> > >
> > > > > Also note that the MT_TOOL_PEN implementation is limited (even
> > > > > non-existant if I remember correctly). Peter and I do not have access
> > > > > to any device that support such multi pen, so AFAICT, there is no code
> > > > > to handle this in libinput.
> > >
> > > Yep, correct. On this note: libinput very much follows a "no hardware, no
> > > implementation" rule. I played the game of trying to support everything in a
> > > generic manner with the X drivers and it's a nightmare to maintain. libinput
> > > instead takes a use case and tries to make it sensible - but for that to
> > > work we need to know both the hardware and the use-cases. That's why tablet
> > > handling coming out of libinput is very different to the handling we have in
> > > X but, afaict, everyone is better off for it so far.
> > >
> > > This means that if you give me a MT_TOOL_FINGER → MT_TOOL_PEN transition,
> > > I'll handle it, but only after you also give me the use-case for it and the
> > > promise of real devices that need it.
> > >
> > > > > One last point from libinput, the pen device would need to be on its
> > > > > separate kernel node for the protocol to be smoothly handled. So
> > > > > basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
> > > > > would not be handled properly right now. The Pen event will be treated
> > > > > as a touch.
> > > >
> > > > I think normally pen and touch a separate controllers, so we have that
> > > > going for us...
> > >
> > > Side-effect of this is: the tablet interface doesn't handle touch at all
> > > because it didn't need to yet. So while technically possible, it requires a
> > > fair bit of re-arranging.
> >
> > What about things like Bamboo touch? It is a Wacom tablet with both
> > multitouch finger and stylus.
>
> these are on two different event nodes though, isn't it? If not, then no-one
> has tested them with libinput so far...
I'll have to plug it in and see... It was a while since I used it.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Peter Hutterer @ 2018-06-04 23:51 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Jason Gerecke,
Dennis Kempin, Andrew de los Reyes, open list:HID CORE LAYER,
lkml
In-Reply-To: <20180604232801.GJ164893@dtor-ws>
On Mon, Jun 04, 2018 at 04:28:01PM -0700, Dmitry Torokhov wrote:
> On Tue, Jun 05, 2018 at 09:06:24AM +1000, Peter Hutterer wrote:
> > On Mon, Jun 04, 2018 at 02:32:55PM -0700, Dmitry Torokhov wrote:
> > > On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
> > > > On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > > > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
> > > > >> Hi Dmitry,
> > > > >>
> > > > >> > > > Logically, the confidence state is a property of a contact, not a new type
> > > > >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
> > > > >> > > >
> > > > >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
> > > > >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> > > > >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> > > > >> > > > I can't find any other users in the current upstream tree, but those
> > > > >> > > > two are already making a precedent and changing the semantic is a
> > > > >> > > > little bit late :/
> > > > >> > I am sorry I did not respond and lost track of this issue back then, but
> > > > >> > I disagree with Henrik here. While confidence is a property of contact,
> > > > >> > so is the type of contact and it can and will change throughout life of
> > > > >> > a contact, especially if we will continue adding new types, such as, for
> > > > >> > example, thumb. In this case the firmware can transition through
> > > > >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
> > > > >> > contact becomes better understood. Still it is the same contact and we
> > > > >> > should not attempt to signal userspace differently.
> > > > >> We agree that the contact should stay the same, but the fear, and I think
> > > > >> somewhere along the blurry history of this thread, the problem was that
> > > > >> userspace interpreted the property change as a new contact (lift up/double
> > > > >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
> > > > >> Pen? It would be hard for an application to consider a switch from finger to
> > > > >> pen as the same contact, which is where the natural implementation starts to
> > > > >> diverge from the intention.
> > > > >
> > > > > I think the userspace has to trust our tracking ID to decide whether it
> > > > > is a same contact or not. The current issue is that kernel is forcing
> > > > > tracking ID change on tool type change, and one of the 2 patches that I
> > > > > posted fixed that, allowing us to keep the tracking ID for finger->palm
> > > > > transitions.
> > > >
> > > > I think I missed those 2 patches, can you point a LKML link?
> > >
> > > Sorry, I thought I sent it out with the patch we are talking about here,
> > > but I did not. See below. Note that it doe snot have any protections on
> > > finger->pen transitions and I am not sure any are needed at the moment.
> > > We can add them wither to MT core or to drivers if we see issues with
> > > devices.
> > >
> > > > Also, note that libevdev discards the tracking ID change now (it
> > > > shouts at the user in the logs). So that means that it will now be
> > > > hard to force libevdev to trust the kernel again for the tracking ID.
> > > > The current rule is:
> > > > - tracking ID >= 0 -> new touch
> > > > - any subsequent tracking ID >= 0 -> discarded
> > > > - tracking ID == -1 -> end of touch
> > >
> > > Well, I guess it is like synaptics driver that used to dump core
> > > whenever it saw tracking ID change for the same slot (not going though
> > > -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> > > us having to produce weird results when users would use fancy gestures
> > > with 3+ fingers.
> >
> > yeah, my mistake, sorry. I always assumed a transition from M to -1 to N,
> > never M to N. This assumption made its way into libevdev (where the tracking
> > ID is transparently discarded, albeit with a warning). There are libevdev
> > patches to get rid of that but whatever device needed it got fixed in some
> > other way, so the patch didn't get pushed.
> >
> > fwiw, dump core was just "print the backtrace to the log" here, there was no
> > actual core dump.
>
> Hmm, I do not recall what version I was playing with, but I tried
> changing Synaptics kernel driver to not insert the fake -1 tracking ID
> for a slot when rolling 3 fingers on a 2-slot device (so removing finger
> 1 while holding finger 2 and adding finger 3 does not appear to
> userspace as 2 - 1 - 2 fingers on the surface, but 2 - 2 - 2 instead)
> and xf86-input-synaptics-1.7.8 would scream about too many slots and
> stop working.
>
> That was a while ago though, before libinput I think.
>
> >
> > > It probably does not matter with devices with 5+ slots. We should pretty
> > > much always have free slot for new contact.
> > >
> > > >
> > > > >
> > > > > I think it is kernel task to not signal transitions that do not make
> > > > > sense, such as finger->pen or palm->pen etc.
> > > >
> > > > I fully agree, though there is currently no such guard in the kernel
> > > > (maybe it's part of your series). I am worried about the RMI4 F12
> > > > driver that automatically forward the info from the firmware, so if
> > > > the firmware does something crazy, it will be exported to user space.
> > > > But I guess it might be better to treat that on a per driver basis.
> > >
> > > Yeah, I think so.
> > >
> > > >
> > > > >
> > > > >>
> > > > >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> > > > >> > complement ABS_MT_TOOL, but that would not really solve the issue with
> > > > >> > Wacom firmware (declaring contact non-confident and releasing it right
> > > > >> > away) and given MS explanation of the confidence as "contact is too big"
> > > > >> > MT_TOOL_PALM fits it perfectly.
> > > > >> Indeed, the Wacom firmware seems to need some special handling, which should
> > > > >> be fine by everyone. I do think it would make sense to add
> > > > >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
> > > >
> > > > Except we are already running out of ABS_* axes.
> > >
> > > Sorry, meants MT_TOOL_TO_BIG, not a new axis.
> >
> > bikeshed: MT_TOOL_IGNORE is a more generic name and does not imply size. A
> > pen that's lying on its side doesn't have a size but should still be
> > ignored.
>
> OK, when we start seeing this for non finger/thumb/palm objects we can
> add this tool type. For current devices we are dealing with palms.
>
> >
> > > >
> > > > >> also to a pen lying down on a touchpad, for instance.
> > > > >
> > > > > OK, I can see that for Pens, if we have firmware that would recognize
> > > > > such condition, it would be weird to report PALM. We could indeed have
> > > > > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
> > > > > the hardware can recognize it as such). Maybe we'd be better off just
> > > > > having userspace going by contact size for pens. Peter, any suggestions
> > > > > here?
> > > >
> > > > I don't think we have size handling in the tablet implementation in
> > > > libinput. I do not see it as a big issue to add such axes from a
> > > > libinput point of view. However, there is no existing hardware that
> > > > would provide such information, so I guess this will be a 'no' until
> > > > actual hardware comes in.
> >
> > correct on all counts :)
> >
> >
> > > > Also note that the MT_TOOL_PEN implementation is limited (even
> > > > non-existant if I remember correctly). Peter and I do not have access
> > > > to any device that support such multi pen, so AFAICT, there is no code
> > > > to handle this in libinput.
> >
> > Yep, correct. On this note: libinput very much follows a "no hardware, no
> > implementation" rule. I played the game of trying to support everything in a
> > generic manner with the X drivers and it's a nightmare to maintain. libinput
> > instead takes a use case and tries to make it sensible - but for that to
> > work we need to know both the hardware and the use-cases. That's why tablet
> > handling coming out of libinput is very different to the handling we have in
> > X but, afaict, everyone is better off for it so far.
> >
> > This means that if you give me a MT_TOOL_FINGER → MT_TOOL_PEN transition,
> > I'll handle it, but only after you also give me the use-case for it and the
> > promise of real devices that need it.
> >
> > > > One last point from libinput, the pen device would need to be on its
> > > > separate kernel node for the protocol to be smoothly handled. So
> > > > basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
> > > > would not be handled properly right now. The Pen event will be treated
> > > > as a touch.
> > >
> > > I think normally pen and touch a separate controllers, so we have that
> > > going for us...
> >
> > Side-effect of this is: the tablet interface doesn't handle touch at all
> > because it didn't need to yet. So while technically possible, it requires a
> > fair bit of re-arranging.
>
> What about things like Bamboo touch? It is a Wacom tablet with both
> multitouch finger and stylus.
these are on two different event nodes though, isn't it? If not, then no-one
has tested them with libinput so far...
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Dmitry Torokhov @ 2018-06-04 23:28 UTC (permalink / raw)
To: Peter Hutterer
Cc: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Jason Gerecke,
Dennis Kempin, Andrew de los Reyes, open list:HID CORE LAYER,
lkml
In-Reply-To: <20180604230624.GA13197@jelly>
On Tue, Jun 05, 2018 at 09:06:24AM +1000, Peter Hutterer wrote:
> On Mon, Jun 04, 2018 at 02:32:55PM -0700, Dmitry Torokhov wrote:
> > On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
> > > On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
> > > <dmitry.torokhov@gmail.com> wrote:
> > > > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
> > > >> Hi Dmitry,
> > > >>
> > > >> > > > Logically, the confidence state is a property of a contact, not a new type
> > > >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
> > > >> > > >
> > > >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
> > > >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> > > >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> > > >> > > > I can't find any other users in the current upstream tree, but those
> > > >> > > > two are already making a precedent and changing the semantic is a
> > > >> > > > little bit late :/
> > > >> > I am sorry I did not respond and lost track of this issue back then, but
> > > >> > I disagree with Henrik here. While confidence is a property of contact,
> > > >> > so is the type of contact and it can and will change throughout life of
> > > >> > a contact, especially if we will continue adding new types, such as, for
> > > >> > example, thumb. In this case the firmware can transition through
> > > >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
> > > >> > contact becomes better understood. Still it is the same contact and we
> > > >> > should not attempt to signal userspace differently.
> > > >> We agree that the contact should stay the same, but the fear, and I think
> > > >> somewhere along the blurry history of this thread, the problem was that
> > > >> userspace interpreted the property change as a new contact (lift up/double
> > > >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
> > > >> Pen? It would be hard for an application to consider a switch from finger to
> > > >> pen as the same contact, which is where the natural implementation starts to
> > > >> diverge from the intention.
> > > >
> > > > I think the userspace has to trust our tracking ID to decide whether it
> > > > is a same contact or not. The current issue is that kernel is forcing
> > > > tracking ID change on tool type change, and one of the 2 patches that I
> > > > posted fixed that, allowing us to keep the tracking ID for finger->palm
> > > > transitions.
> > >
> > > I think I missed those 2 patches, can you point a LKML link?
> >
> > Sorry, I thought I sent it out with the patch we are talking about here,
> > but I did not. See below. Note that it doe snot have any protections on
> > finger->pen transitions and I am not sure any are needed at the moment.
> > We can add them wither to MT core or to drivers if we see issues with
> > devices.
> >
> > > Also, note that libevdev discards the tracking ID change now (it
> > > shouts at the user in the logs). So that means that it will now be
> > > hard to force libevdev to trust the kernel again for the tracking ID.
> > > The current rule is:
> > > - tracking ID >= 0 -> new touch
> > > - any subsequent tracking ID >= 0 -> discarded
> > > - tracking ID == -1 -> end of touch
> >
> > Well, I guess it is like synaptics driver that used to dump core
> > whenever it saw tracking ID change for the same slot (not going though
> > -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> > us having to produce weird results when users would use fancy gestures
> > with 3+ fingers.
>
> yeah, my mistake, sorry. I always assumed a transition from M to -1 to N,
> never M to N. This assumption made its way into libevdev (where the tracking
> ID is transparently discarded, albeit with a warning). There are libevdev
> patches to get rid of that but whatever device needed it got fixed in some
> other way, so the patch didn't get pushed.
>
> fwiw, dump core was just "print the backtrace to the log" here, there was no
> actual core dump.
Hmm, I do not recall what version I was playing with, but I tried
changing Synaptics kernel driver to not insert the fake -1 tracking ID
for a slot when rolling 3 fingers on a 2-slot device (so removing finger
1 while holding finger 2 and adding finger 3 does not appear to
userspace as 2 - 1 - 2 fingers on the surface, but 2 - 2 - 2 instead)
and xf86-input-synaptics-1.7.8 would scream about too many slots and
stop working.
That was a while ago though, before libinput I think.
>
> > It probably does not matter with devices with 5+ slots. We should pretty
> > much always have free slot for new contact.
> >
> > >
> > > >
> > > > I think it is kernel task to not signal transitions that do not make
> > > > sense, such as finger->pen or palm->pen etc.
> > >
> > > I fully agree, though there is currently no such guard in the kernel
> > > (maybe it's part of your series). I am worried about the RMI4 F12
> > > driver that automatically forward the info from the firmware, so if
> > > the firmware does something crazy, it will be exported to user space.
> > > But I guess it might be better to treat that on a per driver basis.
> >
> > Yeah, I think so.
> >
> > >
> > > >
> > > >>
> > > >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> > > >> > complement ABS_MT_TOOL, but that would not really solve the issue with
> > > >> > Wacom firmware (declaring contact non-confident and releasing it right
> > > >> > away) and given MS explanation of the confidence as "contact is too big"
> > > >> > MT_TOOL_PALM fits it perfectly.
> > > >> Indeed, the Wacom firmware seems to need some special handling, which should
> > > >> be fine by everyone. I do think it would make sense to add
> > > >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
> > >
> > > Except we are already running out of ABS_* axes.
> >
> > Sorry, meants MT_TOOL_TO_BIG, not a new axis.
>
> bikeshed: MT_TOOL_IGNORE is a more generic name and does not imply size. A
> pen that's lying on its side doesn't have a size but should still be
> ignored.
OK, when we start seeing this for non finger/thumb/palm objects we can
add this tool type. For current devices we are dealing with palms.
>
> > >
> > > >> also to a pen lying down on a touchpad, for instance.
> > > >
> > > > OK, I can see that for Pens, if we have firmware that would recognize
> > > > such condition, it would be weird to report PALM. We could indeed have
> > > > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
> > > > the hardware can recognize it as such). Maybe we'd be better off just
> > > > having userspace going by contact size for pens. Peter, any suggestions
> > > > here?
> > >
> > > I don't think we have size handling in the tablet implementation in
> > > libinput. I do not see it as a big issue to add such axes from a
> > > libinput point of view. However, there is no existing hardware that
> > > would provide such information, so I guess this will be a 'no' until
> > > actual hardware comes in.
>
> correct on all counts :)
>
>
> > > Also note that the MT_TOOL_PEN implementation is limited (even
> > > non-existant if I remember correctly). Peter and I do not have access
> > > to any device that support such multi pen, so AFAICT, there is no code
> > > to handle this in libinput.
>
> Yep, correct. On this note: libinput very much follows a "no hardware, no
> implementation" rule. I played the game of trying to support everything in a
> generic manner with the X drivers and it's a nightmare to maintain. libinput
> instead takes a use case and tries to make it sensible - but for that to
> work we need to know both the hardware and the use-cases. That's why tablet
> handling coming out of libinput is very different to the handling we have in
> X but, afaict, everyone is better off for it so far.
>
> This means that if you give me a MT_TOOL_FINGER → MT_TOOL_PEN transition,
> I'll handle it, but only after you also give me the use-case for it and the
> promise of real devices that need it.
>
> > > One last point from libinput, the pen device would need to be on its
> > > separate kernel node for the protocol to be smoothly handled. So
> > > basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
> > > would not be handled properly right now. The Pen event will be treated
> > > as a touch.
> >
> > I think normally pen and touch a separate controllers, so we have that
> > going for us...
>
> Side-effect of this is: the tablet interface doesn't handle touch at all
> because it didn't need to yet. So while technically possible, it requires a
> fair bit of re-arranging.
What about things like Bamboo touch? It is a Wacom tablet with both
multitouch finger and stylus.
Thanks.
--
Dmitry
^ permalink raw reply
* [git pull] Input updates for v4.18-rc0
From: Dmitry Torokhov @ 2018-06-04 23:14 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-input
Hi Linus,
Please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
to receive updates for the input subsystem. You will get:
- a new driver to ChipOne icn8505 based touchscreens
- on certain systems with Elan touch controllers they will be switched
away form PS/2 emulation and over to native SMbus mode.
- assorted driver fixups and improvements.
Changelog:
---------
Benjamin Tissoires (6):
Input: elan_i2c - add trackstick report
Input: elantech - split device info into a separate structure
Input: elantech - query the resolution in query_info
Input: elantech - add support for SMBus devices
Input: elantech - detect new ICs and setup Host Notify for them
Input: psmouse-smbus - allow to control psmouse_deactivate
Dmitry Torokhov (1):
Input: atmel_mxt_ts - require device properties present when probing
Ethan Lee (2):
Input: xpad - add GPD Win 2 Controller USB IDs
Input: goodix - add new ACPI id for GPD Win 2 touch screen
Grygorii Strashko (2):
Input: ti_am335x_tsc - ack pending IRQs at probe and before suspend
Input: ti_am335x_tsc - prevent system suspend when TSC is in use
Hans de Goede (1):
Input: add support for ChipOne icn8505 based touchscreens
Johannes Wienke (1):
Input: elan_i2c - add ELAN0612 (Lenovo v330 14IKB) ACPI ID
Leo Sperling (1):
Input: xpad - fix some coding style issues
Marcus Folkesson (3):
Input: xpad - avoid using __set_bit() for capabilities
Input: as5011 - avoid using __set_bit() for capabilities
Input: gamecon - avoid using __set_bit() for capabilities
Martin Kepplinger (1):
Input: mk712 - update documentation web link
Nick Dyer (1):
Input: usbtouchscreen - add sysfs attribute for 3M MTouch firmware rev
Nick Simonov (1):
Input: replace hard coded string with __func__ in pr_err()
Pali Rohár (1):
Input: alps - demystify trackstick initialization for v3 and v6 protocols
Ravi Chandra Sadineni (1):
Input: cros_ec_keyb - mark cros_ec_keyb driver as wake enabled device.
Sebastian Reichel (1):
Input: atmel_mxt_ts - fix reset-gpio for level based irqs
Wolfram Sang (1):
Input: ati_remote2 - fix typo 'can by' to 'can be'
Diffstat:
--------
.../devicetree/bindings/input/elan_i2c.txt | 1 +
MAINTAINERS | 6 +
drivers/input/input.c | 3 +-
drivers/input/joystick/as5011.c | 4 +-
drivers/input/joystick/gamecon.c | 27 +-
drivers/input/joystick/xpad.c | 35 +-
drivers/input/keyboard/cros_ec_keyb.c | 33 +-
drivers/input/misc/ati_remote2.c | 2 +-
drivers/input/mouse/Kconfig | 12 +
drivers/input/mouse/alps.c | 80 ++--
drivers/input/mouse/elan_i2c_core.c | 89 +++-
drivers/input/mouse/elantech.c | 479 ++++++++++++++-----
drivers/input/mouse/elantech.h | 69 ++-
drivers/input/mouse/psmouse-base.c | 21 +-
drivers/input/mouse/psmouse-smbus.c | 24 +-
drivers/input/mouse/psmouse.h | 2 +
drivers/input/mouse/synaptics.c | 2 +-
drivers/input/touchscreen/Kconfig | 11 +
drivers/input/touchscreen/Makefile | 1 +
drivers/input/touchscreen/atmel_mxt_ts.c | 170 +------
drivers/input/touchscreen/chipone_icn8505.c | 520 +++++++++++++++++++++
drivers/input/touchscreen/goodix.c | 1 +
drivers/input/touchscreen/mk712.c | 2 +-
drivers/input/touchscreen/ti_am335x_tsc.c | 7 +
drivers/input/touchscreen/usbtouchscreen.c | 94 ++++
drivers/mfd/cros_ec.c | 19 +-
26 files changed, 1325 insertions(+), 389 deletions(-)
create mode 100644 drivers/input/touchscreen/chipone_icn8505.c
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Peter Hutterer @ 2018-06-04 23:06 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Benjamin Tissoires, Henrik Rydberg, Jiri Kosina, Jason Gerecke,
Dennis Kempin, Andrew de los Reyes, open list:HID CORE LAYER,
lkml
In-Reply-To: <20180604213255.GF164893@dtor-ws>
On Mon, Jun 04, 2018 at 02:32:55PM -0700, Dmitry Torokhov wrote:
> On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
> > On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
> > >> Hi Dmitry,
> > >>
> > >> > > > Logically, the confidence state is a property of a contact, not a new type
> > >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
> > >> > > >
> > >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
> > >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
> > >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
> > >> > > > I can't find any other users in the current upstream tree, but those
> > >> > > > two are already making a precedent and changing the semantic is a
> > >> > > > little bit late :/
> > >> > I am sorry I did not respond and lost track of this issue back then, but
> > >> > I disagree with Henrik here. While confidence is a property of contact,
> > >> > so is the type of contact and it can and will change throughout life of
> > >> > a contact, especially if we will continue adding new types, such as, for
> > >> > example, thumb. In this case the firmware can transition through
> > >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
> > >> > contact becomes better understood. Still it is the same contact and we
> > >> > should not attempt to signal userspace differently.
> > >> We agree that the contact should stay the same, but the fear, and I think
> > >> somewhere along the blurry history of this thread, the problem was that
> > >> userspace interpreted the property change as a new contact (lift up/double
> > >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
> > >> Pen? It would be hard for an application to consider a switch from finger to
> > >> pen as the same contact, which is where the natural implementation starts to
> > >> diverge from the intention.
> > >
> > > I think the userspace has to trust our tracking ID to decide whether it
> > > is a same contact or not. The current issue is that kernel is forcing
> > > tracking ID change on tool type change, and one of the 2 patches that I
> > > posted fixed that, allowing us to keep the tracking ID for finger->palm
> > > transitions.
> >
> > I think I missed those 2 patches, can you point a LKML link?
>
> Sorry, I thought I sent it out with the patch we are talking about here,
> but I did not. See below. Note that it doe snot have any protections on
> finger->pen transitions and I am not sure any are needed at the moment.
> We can add them wither to MT core or to drivers if we see issues with
> devices.
>
> > Also, note that libevdev discards the tracking ID change now (it
> > shouts at the user in the logs). So that means that it will now be
> > hard to force libevdev to trust the kernel again for the tracking ID.
> > The current rule is:
> > - tracking ID >= 0 -> new touch
> > - any subsequent tracking ID >= 0 -> discarded
> > - tracking ID == -1 -> end of touch
>
> Well, I guess it is like synaptics driver that used to dump core
> whenever it saw tracking ID change for the same slot (not going though
> -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> us having to produce weird results when users would use fancy gestures
> with 3+ fingers.
yeah, my mistake, sorry. I always assumed a transition from M to -1 to N,
never M to N. This assumption made its way into libevdev (where the tracking
ID is transparently discarded, albeit with a warning). There are libevdev
patches to get rid of that but whatever device needed it got fixed in some
other way, so the patch didn't get pushed.
fwiw, dump core was just "print the backtrace to the log" here, there was no
actual core dump.
> It probably does not matter with devices with 5+ slots. We should pretty
> much always have free slot for new contact.
>
> >
> > >
> > > I think it is kernel task to not signal transitions that do not make
> > > sense, such as finger->pen or palm->pen etc.
> >
> > I fully agree, though there is currently no such guard in the kernel
> > (maybe it's part of your series). I am worried about the RMI4 F12
> > driver that automatically forward the info from the firmware, so if
> > the firmware does something crazy, it will be exported to user space.
> > But I guess it might be better to treat that on a per driver basis.
>
> Yeah, I think so.
>
> >
> > >
> > >>
> > >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
> > >> > complement ABS_MT_TOOL, but that would not really solve the issue with
> > >> > Wacom firmware (declaring contact non-confident and releasing it right
> > >> > away) and given MS explanation of the confidence as "contact is too big"
> > >> > MT_TOOL_PALM fits it perfectly.
> > >> Indeed, the Wacom firmware seems to need some special handling, which should
> > >> be fine by everyone. I do think it would make sense to add
> > >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
> >
> > Except we are already running out of ABS_* axes.
>
> Sorry, meants MT_TOOL_TO_BIG, not a new axis.
bikeshed: MT_TOOL_IGNORE is a more generic name and does not imply size. A
pen that's lying on its side doesn't have a size but should still be
ignored.
> >
> > >> also to a pen lying down on a touchpad, for instance.
> > >
> > > OK, I can see that for Pens, if we have firmware that would recognize
> > > such condition, it would be weird to report PALM. We could indeed have
> > > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
> > > the hardware can recognize it as such). Maybe we'd be better off just
> > > having userspace going by contact size for pens. Peter, any suggestions
> > > here?
> >
> > I don't think we have size handling in the tablet implementation in
> > libinput. I do not see it as a big issue to add such axes from a
> > libinput point of view. However, there is no existing hardware that
> > would provide such information, so I guess this will be a 'no' until
> > actual hardware comes in.
correct on all counts :)
> > Also note that the MT_TOOL_PEN implementation is limited (even
> > non-existant if I remember correctly). Peter and I do not have access
> > to any device that support such multi pen, so AFAICT, there is no code
> > to handle this in libinput.
Yep, correct. On this note: libinput very much follows a "no hardware, no
implementation" rule. I played the game of trying to support everything in a
generic manner with the X drivers and it's a nightmare to maintain. libinput
instead takes a use case and tries to make it sensible - but for that to
work we need to know both the hardware and the use-cases. That's why tablet
handling coming out of libinput is very different to the handling we have in
X but, afaict, everyone is better off for it so far.
This means that if you give me a MT_TOOL_FINGER → MT_TOOL_PEN transition,
I'll handle it, but only after you also give me the use-case for it and the
promise of real devices that need it.
> > One last point from libinput, the pen device would need to be on its
> > separate kernel node for the protocol to be smoothly handled. So
> > basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
> > would not be handled properly right now. The Pen event will be treated
> > as a touch.
>
> I think normally pen and touch a separate controllers, so we have that
> going for us...
Side-effect of this is: the tablet interface doesn't handle touch at all
because it didn't need to yet. So while technically possible, it requires a
fair bit of re-arranging.
> Input: do not assign new tracking ID when changing tool type
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> We allow changing tool type (from MT_TOOL_FINGER to MT_TOOL_PALM) so we
> should not be forcing new tracking ID for the slot.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Cheers,
Peter
> ---
> drivers/input/input-mt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index a1bbec9cda8d4..7ca4b318ed419 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -151,7 +151,7 @@ void input_mt_report_slot_state(struct input_dev *dev,
> }
>
> id = input_mt_get_value(slot, ABS_MT_TRACKING_ID);
> - if (id < 0 || input_mt_get_value(slot, ABS_MT_TOOL_TYPE) != tool_type)
> + if (id < 0)
> id = input_mt_new_trkid(mt);
>
> input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, id);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Peter Hutterer @ 2018-06-04 22:55 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Benjamin Tissoires, Jiri Kosina, Henrik Rydberg, Jason Gerecke,
Dennis Kempin, Andrew de los Reyes, open list:HID CORE LAYER,
lkml
In-Reply-To: <20180604211944.GE164893@dtor-ws>
On Mon, Jun 04, 2018 at 02:19:44PM -0700, Dmitry Torokhov wrote:
> On Mon, Jun 04, 2018 at 10:42:31PM +0200, Benjamin Tissoires wrote:
> > On Mon, Jun 4, 2018 at 7:33 PM, Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> > > On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
> > >> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
> > >> <dmitry.torokhov@gmail.com> wrote:
> > >> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
> > >> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
> > >> >> <dmitry.torokhov@gmail.com> wrote:
> > >> >> > According to Microsoft specification [1] for Precision Touchpads (and
> > >> >> > Touchscreens) the devices use "confidence" reports to signal accidental
> > >> >> > touches, or contacts that are "too large to be a finger". Instead of
> > >> >> > simply marking contact inactive in this case (which causes issues if
> > >> >> > contact was originally proper and we lost confidence in it later, as
> > >> >> > this results in accidental clicks, drags, etc), let's report such
> > >> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
> > >> >> > Additionally, let's report contact size for such touches as maximum
> > >> >> > allowed for major/minor, which should help userspace that is not yet
> > >> >> > aware of MT_TOOL_PALM to still perform palm rejection.
> > >> >> >
> > >> >> > An additional complication, is that some firmwares do not report
> > >> >> > non-confident touches as active. To cope with this we delay release of
> > >> >> > such contact (i.e. if contact was active we first report it as still
> > >> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
> > >> >> > frame).
> > >> >>
> > >> >> I am not sure I agree with this part. The spec says that "Once a
> > >> >> device has determined that a contact is unintentional, it should clear
> > >> >> the confidence bit for that contact report and all subsequent
> > >> >> reports."
> > >> >> So in theory the spec says that if a touch has been detected as a
> > >> >> palm, the flow of events should not stop (tested on the PTP of the
> > >> >> Dell XPS 9360).
> > >> >>
> > >> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
> > >> >> and then (confidence 0, tip switch 0) a simple release, and the
> > >> >> confidence bit should not be relayed.
> > >> >
> > >> > This unfortunately leads to false clicks: you start with finger, so
> > >> > confidence is 1, then you transition the same touch to palm (use your
> > >> > thumb and "roll" your hand until heel of it comes into contact with the
> > >> > screen). The firmware reports "no-confidence" and "release" in the same
> > >> > report and userspace seeing release does not pay attention to confidence
> > >> > (i.e. it does exactly "simple release" logic) and this results in UI
> > >> > interpreting this as a click. With splitting no-confidence
> > >> > (MT_TOOL_PALM) and release event into separate frames we help userspace
> > >> > to recognize that the contact should be discarded.
> > >>
> > >> After further thoughts, I would consider this to be a firmware bug,
> > >> and not how the firmware is supposed to be reporting palm.
> > >> For the precision touchpads, the spec says that the device "should
> > >> clear the confidence bit for that contact report and all subsequent
> > >> reports.". And it is how the Dell device I have here reports palms.
> > >> The firmware is not supposed to cut the event stream.
> > >>
> > >> There is a test for that:
> > >> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
> > >> which tells me that I am right here for PTP.
> > >>
> > >> The touchscreen spec is blurrier however.
> > >
> > > OK, that is great to know.
> > >
> > >>
> > >> >
> > >> >>
> > >> >> Do you have any precise example of reports where you need that feature?
> > >> >
> > >> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
> > >>
> > >> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
> > >> guessing the device did not went through Microsoft certification
> > >> process?
> > >
> > > That would be correct ;) At least the firmware that is shipping with
> > > Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
> > > their MSWin devices.
> > >
> > >>
> > >> I am in favor of splitting the patch in 2. One for the generic
> > >> processing of confidence bit, and one for this spurious release. For
> > >> the spurious release, I'm more in favor of explicitly quirking the
> > >> devices in need of such quirk.
> > >
> > > Hmm, I am not sure about having specific quirk. It will be hard for
> > > users to accurately diagnose the issue if firmware is broken in this way
> > > so we could add a new quirk for a new device.
> >
> > One thing we can do is keep the quirked mechanism as default in
> > hid-multitouch, but remove it in hid-core. If people need the quirk,
> > they can just use hid-multitouch instead (talking about the long run
> > here).
>
> Hmm, I am confused. My patch did not touch hid-core or hid-input, only
> hid-multitouch... So we are already doing what you are proposing?..
>
> >
> > However, I really believe this might only be required for a handful of
> > devices, and probably only touchscreens. So I would be tempted to not
> > make it default and see how many bug reports we have.
>
> Up to you but it is hard to detect for users. If just sometimes there
> are stray clicks...
fwiw, from my POV, if you give me MT_TOOL_PALM in the same frame as the
ABS_MT_TRACKING_ID -1 I can work that into libinput to do the right thing.
Not 100% whether that already works anyway but probably not. I'd prefer it
being fixed in the kernel though, less work for me :)
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH v1 1/3] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
From: Dmitry Torokhov @ 2018-06-04 22:14 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jeffy Chen, linux-input, linux-kernel
In-Reply-To: <20180604215723.GH164893@dtor-ws>
On Mon, Jun 04, 2018 at 02:57:23PM -0700, Dmitry Torokhov wrote:
> On Mon, Jun 04, 2018 at 12:59:54PM +0300, Andy Shevchenko wrote:
> > On Fri, 2018-06-01 at 11:33 -0700, Dmitry Torokhov wrote:
> > > Hi Andy,
> > >
> > > On Fri, Jun 01, 2018 at 11:31:18AM +0300, Andy Shevchenko wrote:
> > > > A lot of code become ugly because of open coding allocations for
> > > > bitmaps.
> > > >
> > > > Introduce three helpers to allow users be more clear of intention
> > > > and keep their code neat.
> > > >
> > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > >
> > > This looks nice and I like how it simplifies drivers.
> >
> > Thanks!
> >
> > > How do we merge
> > > this?
> >
> > I suppose through 'input' tree if there is no objections.
>
> OK, let's wait for objections for a few days.
>
> >
> > > > ---
> > > > include/linux/bitmap.h | 16 ++++++++++++++++
> > > > 1 file changed, 16 insertions(+)
> > > >
> > > > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > > > index 1ee46f492267..845822425393 100644
> > > > --- a/include/linux/bitmap.h
> > > > +++ b/include/linux/bitmap.h
> > > > @@ -6,6 +6,7 @@
> > > >
> > > > #include <linux/types.h>
> > > > #include <linux/bitops.h>
> > > > +#include <linux/slab.h>
> > > > #include <linux/string.h>
> > > > #include <linux/kernel.h>
> > > >
> > > > @@ -104,6 +105,21 @@
> > > > * contain all bit positions from 0 to 'bits' - 1.
> > > > */
> > > >
> > > > +static inline unsigned long *bitmap_alloc(unsigned int nbits, gfp_t
> > > > flags)
> > > > +{
> > > > + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned
> > > > long), flags);
> > > > +}
> > > > +
> > > > +static inline unsigned long *bitmap_zalloc(unsigned int nbits,
> > > > gfp_t flags)
> > > > +{
> > > > + return kcalloc(BITS_TO_LONGS(nbits), sizeof(unsigned long),
> > > > flags);
> > >
> > > retrun bitmap_alloc(nbits, flags | __GFP_ZERO);
> > >
> > > ?
> >
> > I though about this, but decide not to rely on linux/gfp.h.
> > If you think explicit __GFP_ZERO is better, I can replace in v2, or if
>
> I like it ;) and we already do it for kcalloc done via kmalloc_array
> with __GFP_ZERO or kzalloc (kmalloc with __GFP_ZERO).
>
> > you have a chance to do that when applying it would be appreciated.
>
> Yeah, I can do that, no problem.
Ugh, there is circular dependency:
CC arch/x86/kernel/asm-offsets.s
In file included from ./include/linux/cpumask.h:12:0,
from ./arch/x86/include/asm/cpumask.h:5,
from ./arch/x86/include/asm/msr.h:11,
from ./arch/x86/include/asm/processor.h:21,
from ./arch/x86/include/asm/cpufeature.h:5,
from ./arch/x86/include/asm/thread_info.h:53,
from ./include/linux/thread_info.h:38,
from ./arch/x86/include/asm/preempt.h:7,
from ./include/linux/preempt.h:81,
from ./include/linux/spinlock.h:51,
from ./include/linux/mmzone.h:8,
from ./include/linux/gfp.h:6,
from ./include/linux/slab.h:15,
from ./include/linux/crypto.h:24,
from arch/x86/kernel/asm-offsets.c:9:
./include/linux/bitmap.h: In function ‘bitmap_alloc’:
./include/linux/bitmap.h:110:9: error: implicit declaration of function ‘kmalloc_array’ [-Werror=implicit-function-declaration]
return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
^~~~~~~~~~~~~
./include/linux/bitmap.h:110:9: warning: return makes pointer from integer without a cast [-Wint-conversion]
return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned long), flags);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
So we have slab.h ... -> cpumask.h -> bitmap.h -> slab.h -> BOOM
We will probably have to move implementation into lib/bitmap.h. I think
that should be OK. Can you make the change?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Benjamin Tissoires @ 2018-06-04 22:14 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Henrik Rydberg, Jiri Kosina, Jason Gerecke, Dennis Kempin,
Andrew de los Reyes, open list:HID CORE LAYER, lkml
In-Reply-To: <20180604213255.GF164893@dtor-ws>
On Mon, Jun 4, 2018 at 11:32 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jun 04, 2018 at 10:59:16PM +0200, Benjamin Tissoires wrote:
>> On Mon, Jun 4, 2018 at 8:26 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Mon, Jun 04, 2018 at 07:55:57PM +0200, Henrik Rydberg wrote:
>> >> Hi Dmitry,
>> >>
>> >> > > > Logically, the confidence state is a property of a contact, not a new type
>> >> > > > of contact. Trying to use it in any other way is bound to lead to confusion.
>> >> > > >
>> >> > > > Problem is that MT_TOOL_PALM has been introduced in the kernel since
>> >> > > > v4.0 (late 2015 by a736775db683 "Input: add MT_TOOL_PALM").
>> >> > > > It's been used in the Synaptics RMI4 driver since and by hid-asus in late 2016.
>> >> > > > I can't find any other users in the current upstream tree, but those
>> >> > > > two are already making a precedent and changing the semantic is a
>> >> > > > little bit late :/
>> >> > I am sorry I did not respond and lost track of this issue back then, but
>> >> > I disagree with Henrik here. While confidence is a property of contact,
>> >> > so is the type of contact and it can and will change throughout life of
>> >> > a contact, especially if we will continue adding new types, such as, for
>> >> > example, thumb. In this case the firmware can transition through
>> >> > finger->thumb or finger->thumb->palm or finger->palm as the nature of
>> >> > contact becomes better understood. Still it is the same contact and we
>> >> > should not attempt to signal userspace differently.
>> >> We agree that the contact should stay the same, but the fear, and I think
>> >> somewhere along the blurry history of this thread, the problem was that
>> >> userspace interpreted the property change as a new contact (lift up/double
>> >> click/etc). Finger/thumb/palm is one set of hand properties, but what about
>> >> Pen? It would be hard for an application to consider a switch from finger to
>> >> pen as the same contact, which is where the natural implementation starts to
>> >> diverge from the intention.
>> >
>> > I think the userspace has to trust our tracking ID to decide whether it
>> > is a same contact or not. The current issue is that kernel is forcing
>> > tracking ID change on tool type change, and one of the 2 patches that I
>> > posted fixed that, allowing us to keep the tracking ID for finger->palm
>> > transitions.
>>
>> I think I missed those 2 patches, can you point a LKML link?
>
> Sorry, I thought I sent it out with the patch we are talking about here,
> but I did not. See below. Note that it doe snot have any protections on
> finger->pen transitions and I am not sure any are needed at the moment.
> We can add them wither to MT core or to drivers if we see issues with
> devices.
For what it worth, after a quick reading of the patch, it would be:
Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
I can't remember exactly why we checked for the tool in the first place.
>
>> Also, note that libevdev discards the tracking ID change now (it
>> shouts at the user in the logs). So that means that it will now be
>> hard to force libevdev to trust the kernel again for the tracking ID.
>> The current rule is:
>> - tracking ID >= 0 -> new touch
>> - any subsequent tracking ID >= 0 -> discarded
>> - tracking ID == -1 -> end of touch
>
> Well, I guess it is like synaptics driver that used to dump core
> whenever it saw tracking ID change for the same slot (not going though
> -1 sequence). It only mattered to Synaptics PS/2 having only 2 slots and
> us having to produce weird results when users would use fancy gestures
> with 3+ fingers.
>
> It probably does not matter with devices with 5+ slots. We should pretty
> much always have free slot for new contact.
On Win 8 devices, we can safely expect the firmware to have a correct
maximum contact number information (there is no point reporting 5
fingers and saying you can support only 2). So if we try to add a new
slot and we are out of them, it's pretty much down to a firmware bug
or a touch we missed to release.
>
>>
>> >
>> > I think it is kernel task to not signal transitions that do not make
>> > sense, such as finger->pen or palm->pen etc.
>>
>> I fully agree, though there is currently no such guard in the kernel
>> (maybe it's part of your series). I am worried about the RMI4 F12
>> driver that automatically forward the info from the firmware, so if
>> the firmware does something crazy, it will be exported to user space.
>> But I guess it might be better to treat that on a per driver basis.
>
> Yeah, I think so.
>
>>
>> >
>> >>
>> >> > We could introduce the ABS_MT_CONFIDENCE (0/1 or even 0..n range), to
>> >> > complement ABS_MT_TOOL, but that would not really solve the issue with
>> >> > Wacom firmware (declaring contact non-confident and releasing it right
>> >> > away) and given MS explanation of the confidence as "contact is too big"
>> >> > MT_TOOL_PALM fits it perfectly.
>> >> Indeed, the Wacom firmware seems to need some special handling, which should
>> >> be fine by everyone. I do think it would make sense to add
>> >> ABS_MT_TOOL_TOO_BIG, or something, and use it if it exists. This would apply
>>
>> Except we are already running out of ABS_* axes.
>
> Sorry, meants MT_TOOL_TO_BIG, not a new axis.
>
>>
>> >> also to a pen lying down on a touchpad, for instance.
>> >
>> > OK, I can see that for Pens, if we have firmware that would recognize
>> > such condition, it would be weird to report PALM. We could indeed have
>> > ABS_MT_TOOL_TOO_BIG, but on the other hand it is still a pen (as long as
>> > the hardware can recognize it as such). Maybe we'd be better off just
>> > having userspace going by contact size for pens. Peter, any suggestions
>> > here?
>>
>> I don't think we have size handling in the tablet implementation in
>> libinput. I do not see it as a big issue to add such axes from a
>> libinput point of view. However, there is no existing hardware that
>> would provide such information, so I guess this will be a 'no' until
>> actual hardware comes in.
>>
>> Also note that the MT_TOOL_PEN implementation is limited (even
>> non-existant if I remember correctly). Peter and I do not have access
>> to any device that support such multi pen, so AFAICT, there is no code
>> to handle this in libinput.
>>
>> One last point from libinput, the pen device would need to be on its
>> separate kernel node for the protocol to be smoothly handled. So
>> basically, even the transition from MT_TOOL_FINGER to MT_TOOL_PEN
>> would not be handled properly right now. The Pen event will be treated
>> as a touch.
>
> I think normally pen and touch a separate controllers, so we have that
> going for us...
The problem is for devices like the N-trig DualSense or the Wacom AES
where only one sensor handles both pen and touch. For now, given that
MS says it supports only one pen we are on the safe side, but things
might gets scarier in the future :)
Cheers,
Benjamin
>
> Thanks.
>
> --
> Dmitry
>
>
> Input: do not assign new tracking ID when changing tool type
>
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> We allow changing tool type (from MT_TOOL_FINGER to MT_TOOL_PALM) so we
> should not be forcing new tracking ID for the slot.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/input-mt.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index a1bbec9cda8d4..7ca4b318ed419 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -151,7 +151,7 @@ void input_mt_report_slot_state(struct input_dev *dev,
> }
>
> id = input_mt_get_value(slot, ABS_MT_TRACKING_ID);
> - if (id < 0 || input_mt_get_value(slot, ABS_MT_TOOL_TYPE) != tool_type)
> + if (id < 0)
> id = input_mt_new_trkid(mt);
>
> input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, id);
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Benjamin Tissoires @ 2018-06-04 22:03 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Henrik Rydberg, Jason Gerecke, Dennis Kempin,
Andrew de los Reyes, open list:HID CORE LAYER, lkml
In-Reply-To: <20180604211944.GE164893@dtor-ws>
On Mon, Jun 4, 2018 at 11:19 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Jun 04, 2018 at 10:42:31PM +0200, Benjamin Tissoires wrote:
>> On Mon, Jun 4, 2018 at 7:33 PM, Dmitry Torokhov
>> <dmitry.torokhov@gmail.com> wrote:
>> > On Mon, Jun 04, 2018 at 03:18:12PM +0200, Benjamin Tissoires wrote:
>> >> On Fri, Jun 1, 2018 at 8:43 PM, Dmitry Torokhov
>> >> <dmitry.torokhov@gmail.com> wrote:
>> >> > On Fri, Jun 01, 2018 at 04:16:09PM +0200, Benjamin Tissoires wrote:
>> >> >> On Fri, Aug 11, 2017 at 2:44 AM, Dmitry Torokhov
>> >> >> <dmitry.torokhov@gmail.com> wrote:
>> >> >> > According to Microsoft specification [1] for Precision Touchpads (and
>> >> >> > Touchscreens) the devices use "confidence" reports to signal accidental
>> >> >> > touches, or contacts that are "too large to be a finger". Instead of
>> >> >> > simply marking contact inactive in this case (which causes issues if
>> >> >> > contact was originally proper and we lost confidence in it later, as
>> >> >> > this results in accidental clicks, drags, etc), let's report such
>> >> >> > contacts as MT_TOOL_PALM and let userspace decide what to do.
>> >> >> > Additionally, let's report contact size for such touches as maximum
>> >> >> > allowed for major/minor, which should help userspace that is not yet
>> >> >> > aware of MT_TOOL_PALM to still perform palm rejection.
>> >> >> >
>> >> >> > An additional complication, is that some firmwares do not report
>> >> >> > non-confident touches as active. To cope with this we delay release of
>> >> >> > such contact (i.e. if contact was active we first report it as still
>> >> >> > active MT+TOOL_PALM and then synthesize the release event in a separate
>> >> >> > frame).
>> >> >>
>> >> >> I am not sure I agree with this part. The spec says that "Once a
>> >> >> device has determined that a contact is unintentional, it should clear
>> >> >> the confidence bit for that contact report and all subsequent
>> >> >> reports."
>> >> >> So in theory the spec says that if a touch has been detected as a
>> >> >> palm, the flow of events should not stop (tested on the PTP of the
>> >> >> Dell XPS 9360).
>> >> >>
>> >> >> However, I interpret a firmware that send (confidence 1, tip switch 1)
>> >> >> and then (confidence 0, tip switch 0) a simple release, and the
>> >> >> confidence bit should not be relayed.
>> >> >
>> >> > This unfortunately leads to false clicks: you start with finger, so
>> >> > confidence is 1, then you transition the same touch to palm (use your
>> >> > thumb and "roll" your hand until heel of it comes into contact with the
>> >> > screen). The firmware reports "no-confidence" and "release" in the same
>> >> > report and userspace seeing release does not pay attention to confidence
>> >> > (i.e. it does exactly "simple release" logic) and this results in UI
>> >> > interpreting this as a click. With splitting no-confidence
>> >> > (MT_TOOL_PALM) and release event into separate frames we help userspace
>> >> > to recognize that the contact should be discarded.
>> >>
>> >> After further thoughts, I would consider this to be a firmware bug,
>> >> and not how the firmware is supposed to be reporting palm.
>> >> For the precision touchpads, the spec says that the device "should
>> >> clear the confidence bit for that contact report and all subsequent
>> >> reports.". And it is how the Dell device I have here reports palms.
>> >> The firmware is not supposed to cut the event stream.
>> >>
>> >> There is a test for that:
>> >> https://docs.microsoft.com/en-us/previous-versions/windows/hardware/hck/dn456905%28v%3dvs.85%29
>> >> which tells me that I am right here for PTP.
>> >>
>> >> The touchscreen spec is blurrier however.
>> >
>> > OK, that is great to know.
>> >
>> >>
>> >> >
>> >> >>
>> >> >> Do you have any precise example of reports where you need that feature?
>> >> >
>> >> > It was observed on Pixelbooks which use Wacom digitizers IIRC.
>> >>
>> >> Pixelbooks + Wacom means that it was likely a touchscreen. I am right
>> >> guessing the device did not went through Microsoft certification
>> >> process?
>> >
>> > That would be correct ;) At least the firmware that is shipping with
>> > Pixlebooks hasn't, I do now if anyone else sourced these Wacom parts for
>> > their MSWin devices.
>> >
>> >>
>> >> I am in favor of splitting the patch in 2. One for the generic
>> >> processing of confidence bit, and one for this spurious release. For
>> >> the spurious release, I'm more in favor of explicitly quirking the
>> >> devices in need of such quirk.
>> >
>> > Hmm, I am not sure about having specific quirk. It will be hard for
>> > users to accurately diagnose the issue if firmware is broken in this way
>> > so we could add a new quirk for a new device.
>>
>> One thing we can do is keep the quirked mechanism as default in
>> hid-multitouch, but remove it in hid-core. If people need the quirk,
>> they can just use hid-multitouch instead (talking about the long run
>> here).
>
> Hmm, I am confused. My patch did not touch hid-core or hid-input, only
> hid-multitouch... So we are already doing what you are proposing?..
It's the long run solution. I am trying in my last series to
streamline hid-multitouch so I can merge it in hid-core. I am planning
on having a few revision with hid-multitouch as an external module,
and then switch the win 8 (touchscreens and PTPs) devices to use a
generic processing in hid-input.c. So that's why I'd rather keep the
quirked devices separately and iron out the generic ones. It's easier
to override hid-multiouch for users than it is to override hid.ko.
>
>>
>> However, I really believe this might only be required for a handful of
>> devices, and probably only touchscreens. So I would be tempted to not
>> make it default and see how many bug reports we have.
>
> Up to you but it is hard to detect for users. If just sometimes there
> are stray clicks...
Users might not directly target the bugs toward me, but will likely
complain to Peter first :)
We can also add some automatic sanity check in libinput to relay the
information that there is a FW/kernel bug for such devices.
Cheers,
Benjamin
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH v1 1/3] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
From: Dmitry Torokhov @ 2018-06-04 21:57 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jeffy Chen, linux-input, linux-kernel
In-Reply-To: <cfe3fa909edd73ba84dd367a11a570866647a3f4.camel@linux.intel.com>
On Mon, Jun 04, 2018 at 12:59:54PM +0300, Andy Shevchenko wrote:
> On Fri, 2018-06-01 at 11:33 -0700, Dmitry Torokhov wrote:
> > Hi Andy,
> >
> > On Fri, Jun 01, 2018 at 11:31:18AM +0300, Andy Shevchenko wrote:
> > > A lot of code become ugly because of open coding allocations for
> > > bitmaps.
> > >
> > > Introduce three helpers to allow users be more clear of intention
> > > and keep their code neat.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >
> > This looks nice and I like how it simplifies drivers.
>
> Thanks!
>
> > How do we merge
> > this?
>
> I suppose through 'input' tree if there is no objections.
OK, let's wait for objections for a few days.
>
> > > ---
> > > include/linux/bitmap.h | 16 ++++++++++++++++
> > > 1 file changed, 16 insertions(+)
> > >
> > > diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> > > index 1ee46f492267..845822425393 100644
> > > --- a/include/linux/bitmap.h
> > > +++ b/include/linux/bitmap.h
> > > @@ -6,6 +6,7 @@
> > >
> > > #include <linux/types.h>
> > > #include <linux/bitops.h>
> > > +#include <linux/slab.h>
> > > #include <linux/string.h>
> > > #include <linux/kernel.h>
> > >
> > > @@ -104,6 +105,21 @@
> > > * contain all bit positions from 0 to 'bits' - 1.
> > > */
> > >
> > > +static inline unsigned long *bitmap_alloc(unsigned int nbits, gfp_t
> > > flags)
> > > +{
> > > + return kmalloc_array(BITS_TO_LONGS(nbits), sizeof(unsigned
> > > long), flags);
> > > +}
> > > +
> > > +static inline unsigned long *bitmap_zalloc(unsigned int nbits,
> > > gfp_t flags)
> > > +{
> > > + return kcalloc(BITS_TO_LONGS(nbits), sizeof(unsigned long),
> > > flags);
> >
> > retrun bitmap_alloc(nbits, flags | __GFP_ZERO);
> >
> > ?
>
> I though about this, but decide not to rely on linux/gfp.h.
> If you think explicit __GFP_ZERO is better, I can replace in v2, or if
I like it ;) and we already do it for kcalloc done via kmalloc_array
with __GFP_ZERO or kzalloc (kmalloc with __GFP_ZERO).
> you have a chance to do that when applying it would be appreciated.
Yeah, I can do that, no problem.
Thanks.
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox