* Re: [PATCH] HID: steam: use hid_device.driver_data instead of hid_set_drvdata()
From: Rodrigo Rivas Costa @ 2018-06-01 18:27 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A. Griffais,
Clément VUCHENER, lkml, linux-input
In-Reply-To: <20180522201006.14003-1-rodrigorivascosta@gmail.com>
Hi, all!
Could you check my previous patch to see if it makes any sense?
My machine currently crashes without it, but I'm not sure why. If you
think it is worth it I can try and bisect it.
Regards
--
Rodrigo Rivas Costa.
On Tue, May 22, 2018 at 10:10:06PM +0200, Rodrigo Rivas Costa wrote:
> When creating the low-level hidraw device, the reference to steam_device
> was stored using hid_set_drvdata(). But this value is not guaranteed to
> be kept when set before calling probe. If this pointer is reset, it
> crashes when opening the emulated hidraw device.
>
> It looks like hid_set_drvdata() is for users "avobe" this hid_device,
> while hid_device.driver_data it for users "below" this one.
>
> In this case, we are creating a virtual hidraw device, so we must use
> hid_device.driver_data.
>
> Signed-off-by: Rodrigo Rivas Costa <rodrigorivascosta@gmail.com>
> ---
>
> This patch is to be applied over hid/for-4.18/hid-steam. Is this the
> proper way to signal it?
>
> I don't know exactly when the problem started. I am pretty sure that it
> worked with 4.16.2, but it failed with 4.16.9. Or maybe it is caused by
> upgrading the firmware of the controller, although the protocol seems
> identical.
>
> drivers/hid/hid-steam.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index cb86cc834201..0422ec2b13d2 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -573,7 +573,7 @@ static bool steam_is_valve_interface(struct hid_device *hdev)
>
> static int steam_client_ll_parse(struct hid_device *hdev)
> {
> - struct steam_device *steam = hid_get_drvdata(hdev);
> + struct steam_device *steam = hdev->driver_data;
>
> return hid_parse_report(hdev, steam->hdev->dev_rdesc,
> steam->hdev->dev_rsize);
> @@ -590,7 +590,7 @@ static void steam_client_ll_stop(struct hid_device *hdev)
>
> static int steam_client_ll_open(struct hid_device *hdev)
> {
> - struct steam_device *steam = hid_get_drvdata(hdev);
> + struct steam_device *steam = hdev->driver_data;
> int ret;
>
> ret = hid_hw_open(steam->hdev);
> @@ -605,7 +605,7 @@ static int steam_client_ll_open(struct hid_device *hdev)
>
> static void steam_client_ll_close(struct hid_device *hdev)
> {
> - struct steam_device *steam = hid_get_drvdata(hdev);
> + struct steam_device *steam = hdev->driver_data;
>
> mutex_lock(&steam->mutex);
> steam->client_opened = false;
> @@ -623,7 +623,7 @@ static int steam_client_ll_raw_request(struct hid_device *hdev,
> size_t count, unsigned char report_type,
> int reqtype)
> {
> - struct steam_device *steam = hid_get_drvdata(hdev);
> + struct steam_device *steam = hdev->driver_data;
>
> return hid_hw_raw_request(steam->hdev, reportnum, buf, count,
> report_type, reqtype);
> @@ -710,7 +710,7 @@ static int steam_probe(struct hid_device *hdev,
> ret = PTR_ERR(steam->client_hdev);
> goto client_hdev_fail;
> }
> - hid_set_drvdata(steam->client_hdev, steam);
> + steam->client_hdev->driver_data = steam;
>
> /*
> * With the real steam controller interface, do not connect hidraw.
> --
> 2.17.0
>
^ permalink raw reply
* Re: [PATCH v1 1/3] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
From: Dmitry Torokhov @ 2018-06-01 18:33 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jeffy Chen, linux-input, linux-kernel
In-Reply-To: <20180601083120.40352-1-andriy.shevchenko@linux.intel.com>
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. How do we merge
this?
> ---
> 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);
?
> +}
> +
> +static inline void bitmap_free(const unsigned long *bitmap)
> +{
> + kfree(bitmap);
> +}
> +
> /*
> * lib/bitmap.c provides these functions:
> */
> --
> 2.17.0
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v1 2/3] Input: gpio-keys - Switch to bitmap_zalloc()
From: Dmitry Torokhov @ 2018-06-01 18:34 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jeffy Chen, linux-input, linux-kernel
In-Reply-To: <20180601083120.40352-2-andriy.shevchenko@linux.intel.com>
On Fri, Jun 01, 2018 at 11:31:19AM +0300, Andy Shevchenko wrote:
> Switch to bitmap_zalloc() to show clearly what we are allocating.
> Besides that it returns pointer of bitmap type instead of opaque void *.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/keyboard/gpio_keys.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> index 052e37675086..492a971b95b5 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -196,7 +196,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
> ssize_t ret;
> int i;
>
> - bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
> + bits = bitmap_zalloc(n_events, GFP_KERNEL);
> if (!bits)
> return -ENOMEM;
>
> @@ -216,7 +216,7 @@ static ssize_t gpio_keys_attr_show_helper(struct gpio_keys_drvdata *ddata,
> buf[ret++] = '\n';
> buf[ret] = '\0';
>
> - kfree(bits);
> + bitmap_free(bits);
>
> return ret;
> }
> @@ -240,7 +240,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
> ssize_t error;
> int i;
>
> - bits = kcalloc(BITS_TO_LONGS(n_events), sizeof(*bits), GFP_KERNEL);
> + bits = bitmap_zalloc(n_events, GFP_KERNEL);
> if (!bits)
> return -ENOMEM;
>
> @@ -284,7 +284,7 @@ static ssize_t gpio_keys_attr_store_helper(struct gpio_keys_drvdata *ddata,
> mutex_unlock(&ddata->disable_lock);
>
> out:
> - kfree(bits);
> + bitmap_free(bits);
> return error;
> }
>
> --
> 2.17.0
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH v1 3/3] Input: evdev - Switch to bitmap_zalloc()
From: Dmitry Torokhov @ 2018-06-01 18:34 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jeffy Chen, linux-input, linux-kernel
In-Reply-To: <20180601083120.40352-3-andriy.shevchenko@linux.intel.com>
On Fri, Jun 01, 2018 at 11:31:20AM +0300, Andy Shevchenko wrote:
> Switch to bitmap_zalloc() to show clearly what we are allocating.
> Besides that it returns pointer of bitmap type instead of opaque void *.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/evdev.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index c81c79d01d93..3f87ef973bc7 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c
> @@ -481,7 +481,7 @@ static int evdev_release(struct inode *inode, struct file *file)
> evdev_detach_client(evdev, client);
>
> for (i = 0; i < EV_CNT; ++i)
> - kfree(client->evmasks[i]);
> + bitmap_free(client->evmasks[i]);
>
> kvfree(client);
>
> @@ -925,17 +925,15 @@ static int evdev_handle_get_val(struct evdev_client *client,
> {
> int ret;
> unsigned long *mem;
> - size_t len;
>
> - len = BITS_TO_LONGS(maxbit) * sizeof(unsigned long);
> - mem = kmalloc(len, GFP_KERNEL);
> + mem = bitmap_malloc(maxbit, GFP_KERNEL);
> if (!mem)
> return -ENOMEM;
>
> spin_lock_irq(&dev->event_lock);
> spin_lock(&client->buffer_lock);
>
> - memcpy(mem, bits, len);
> + bitmap_copy(mem, bits, maxbit);
>
> spin_unlock(&dev->event_lock);
>
> @@ -947,7 +945,7 @@ static int evdev_handle_get_val(struct evdev_client *client,
> if (ret < 0)
> evdev_queue_syn_dropped(client);
>
> - kfree(mem);
> + bitmap_free(mem);
>
> return ret;
> }
> @@ -1003,13 +1001,13 @@ static int evdev_set_mask(struct evdev_client *client,
> if (!cnt)
> return 0;
>
> - mask = kcalloc(sizeof(unsigned long), BITS_TO_LONGS(cnt), GFP_KERNEL);
> + mask = bitmap_zalloc(cnt, GFP_KERNEL);
> if (!mask)
> return -ENOMEM;
>
> error = bits_from_user(mask, cnt - 1, codes_size, codes, compat);
> if (error < 0) {
> - kfree(mask);
> + bitmap_free(mask);
> return error;
> }
>
> @@ -1018,7 +1016,7 @@ static int evdev_set_mask(struct evdev_client *client,
> client->evmasks[type] = mask;
> spin_unlock_irqrestore(&client->buffer_lock, flags);
>
> - kfree(oldmask);
> + bitmap_free(oldmask);
>
> return 0;
> }
> --
> 2.17.0
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Dmitry Torokhov @ 2018-06-01 18:43 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Henrik Rydberg, Jason Gerecke, Dennis Kempin,
Andrew de los Reyes, open list:HID CORE LAYER, lkml
In-Reply-To: <CAO-hwJJ8E4mQqu0RYaZrmawud4wbSVE4GHEvg7oxUzEc-B5nvQ@mail.gmail.com>
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.
>
> Do you have any precise example of reports where you need that feature?
It was observed on Pixelbooks which use Wacom digitizers IIRC.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Henrik Rydberg @ 2018-06-01 19:03 UTC (permalink / raw)
To: Dmitry Torokhov, Benjamin Tissoires
Cc: Jiri Kosina, Jason Gerecke, Dennis Kempin, Andrew de los Reyes,
open list:HID CORE LAYER, lkml
In-Reply-To: <20180601184330.GD222005@dtor-ws>
>> 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.
This is in part why I objected to this patch on August 11th, 2017.
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.
Henrik
^ permalink raw reply
* [PATCH V2] i8042: Increment wakeup_count for the respective port.
From: Ravi Chandra Sadineni @ 2018-06-02 1:07 UTC (permalink / raw)
To: dmitry.torokhov, chenhong3, ravisadineni, ravisadineni, dtor
Cc: tbroch, linux-kernel, linux-input, rajatja, bleung
In-Reply-To: <20180601180053.120202-1-ravisadineni@chromium.org>
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);
+
filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
port_no, irq,
dfl & SERIO_PARITY ? ", bad parity" : "",
--
2.17.1.1185.g55be947832-goog
^ permalink raw reply related
* Re: [PATCH v7 0/9] Add support for SAMA5D2 touchscreen
From: Eugen Hristev @ 2018-06-04 6:15 UTC (permalink / raw)
To: Jonathan Cameron
Cc: ludovic.desroches, alexandre.belloni, linux-arm-kernel,
devicetree, linux-kernel, linux-iio, linux-input, nicolas.ferre,
dmitry.torokhov, robh
In-Reply-To: <20180522185732.0336d29c@archlinux>
On 22.05.2018 20:57, Jonathan Cameron wrote:
> On Tue, 22 May 2018 10:52:30 +0300
> Eugen Hristev <eugen.hristev@microchip.com> wrote:
>
>> Hello,
>>
>> This patch series is a rework of my previous series named:
>> [PATCH 00/14] iio: triggers: add consumer support
>>
>> This is the version 7 of the series, and addresses the received feedback
>> on the v2 series named:
>> [PATCH v2 00/10] Add support for SAMA5D2 touchscreen
>> and the v3 series named
>> [PATCH v3 00/11] Add support for SAMA5D2 touchscreen
>> and the v4 series named
>> [PATCH v4 0/9] Add support for SAMA5D2 touchscreen
>> and fixes one bug found in series named
>> [PATCH v5 0/9] Add support for SAMA5D2 touchscreen
>> and addresses comments in series named
>> [PATCH v6 0/9] Add support for SAMA5D2 touchscreen
>>
>> This series applies on top of fixes-togreg branch of iio.git,
>> specifically on top of commit:
>> "f0c8d1f" : iio: adc: at91-sama5d2_adc:
>> fix channel configuration for differential channels
>>
>> Jonathan, if you need me to rebase this on top of testing, let me know.
>>
>> Changes in previous versions are presented at the end of the cover letter below.
>> Thanks everyone for the feedback. Below is the original v2 cover letter:
>>
>> In few words, this is the implementation of splitting the functionality
>> of the IP block ADC device in SAMA5D2 SoC from ADC with touchscreen
>> support. In order to avoid having a MFD device, two separate
>> drivers that would work on same register base and split the IRQ,etc,
>> as advised on the mailing list, I created a consumer driver for the
>> channels, that will connect to the ADC as described in the device tree.
>>
>> I have collected feedback from everyone and here is the result:
>> I have added a new generic resistive touchscreen driver, which acts
>> as a iio consumer for the given channels and will create an input
>> device and report the events. It uses a callback buffer to register
>> to the IIO device and waits for data to be pushed.
>> Inside the IIO device, I have kept a similar approach with the first version
>> of the series, except that now the driver can take multiple buffers, and
>> will configure the touchscreen part of the hardware device if the specific
>> channels are requested.
>>
>> The SAMA5D2 ADC driver registers three new channels: two for the
>> position on the X and Y axis, and one for the touch pressure.
>> When channels are requested, it will check if the touchscreen channel mask
>> includes the requested channels (it is possible that the consumer driver
>> will not request pressure for example). If it's the case, it will work
>> in touchscreen mode, and will refuse to do usual analog-digital conversion,
>> because we have a single trigger and the touchscreen needs it.
>> When the scan mask will include only old channels, the driver will function
>> in the same way as before. If the scan mask somehow is a mix of the two (the
>> masks intersect), the driver will refuse to work whatsoever (cannot have both
>> in the same time).
>> The driver allows reading raw data for the new channels, if claim direct
>> mode works: no touchscreen driver requested anything. The new channels can
>> act like the old ones. However, when requesting these channels, the usual
>> trigger will not work and will not be enabled. The touchscreen channels
>> require special trigger and irq configuration: pen detect, no pen detect
>> and a periodic trigger to sample the touchscreen position and pressure.
>> If the user attempts to use another trigger while there is a buffer
>> that already requested the touchscreen channels (thus the trigger), the
>> driver will refuse to comply.
>>
>> In order to have defines for the channel numbers, I added a bindings include
>> file that goes on a separate commit :
>> dt-bindings: iio: adc: at91-sama5d2_adc: add channel specific consumer info
>> This should go in the same tree with the following commits :
>> ARM: dts: at91: sama5d2: add channel cells for ADC device
>> ARM: dts: at91: sama5d2: Add resistive touch device
>>
>> as build will break because these commits depend on the binding one
>> which creates the included header file.
>> V5 update: After discussing with Alexandre Belloni on Mailing list, the two
>> DTS patches are to be taken in the next version after bindings reach mainline.
>>
>> Changes in v7:
>> - Addressed some feedback from Dmitry, explained in input driver patch
>> changelog.
>> - Added Acked-by Dmitry.
>>
>> Changes in v6:
>> - Fixed a crash in ADC driver , explained in driver patch changelog.
>> - changed a dev_err to dev_dbg in input driver.
>> - added Reviewed-by Rob Herring.
>>
>> Changes in v5:
>> - renamed property touchscreen-threshold-pressure to touchscreen-min-pressure
>> - added one return in touchscreen driver
>>
>> Changes in v4:
>> - removed patch for inkern module get/set kref
>> - addressed feedback on both the ADC and the touchscreen driver. each
>> patch has a history inside the patch file for the specific changes.
>> - patch that fixes the channel fix
>> [PATCH v3 01/11] iio: adc: at91-sama5d2_adc:
>> fix channel configuration for differential channels
>> was accepted in fixes-togreg branch thus removed from this series.
>> - added Reviewed-by for the bindings by Rob Herring
>>
>> Changes in v3:
>> - changed input driver name according to feedback and reworked in commits
>> to adapt to binding changes and new name.
>> - moved channel index fix in at91-sama5d2_adc at the beginning of the series
>> (PATCH 01/11)
>> - created a new optional binding for the touchscreen as a separate commit
>> and added it to the series :
>> [PATCH v3 04/11] dt-bindings: input: touchscreen: add pressure
>> threshold touchscreen property
>> - changed at91-sama5d2_adc driver patch to address the comments. Exact changes
>> are in the patch file for the driver source file.
>>
>> Eugen Hristev (9):
>> MAINTAINERS: add generic resistive touchscreen adc
>> iio: Add channel for Position Relative
>> dt-bindings: input: touchscreen: add minimum pressure touchscreen
>> property
>> dt-bindings: input: touchscreen: resistive-adc-touch: create bindings
>> iio: adc: at91-sama5d2_adc: add support for position and pressure
>> channels
>> input: touchscreen: resistive-adc-touch: add generic resistive ADC
>> touchscreen
>> dt-bindings: iio: adc: at91-sama5d2_adc: add channel specific consumer
>> info
>> ARM: dts: at91: sama5d2: add channel cells for ADC device
>> ARM: dts: at91: sama5d2: Add resistive touch device
>>
>> Documentation/ABI/testing/sysfs-bus-iio | 12 +
>> .../bindings/iio/adc/at91-sama5d2_adc.txt | 9 +
>> .../input/touchscreen/resistive-adc-touch.txt | 30 +
>> .../bindings/input/touchscreen/touchscreen.txt | 3 +
>> MAINTAINERS | 6 +
>> arch/arm/boot/dts/sama5d2.dtsi | 12 +
>> drivers/iio/adc/at91-sama5d2_adc.c | 609 +++++++++++++++++++--
>> drivers/iio/industrialio-core.c | 1 +
>> drivers/input/touchscreen/Kconfig | 13 +
>> drivers/input/touchscreen/Makefile | 1 +
>> drivers/input/touchscreen/resistive-adc-touch.c | 204 +++++++
>> include/dt-bindings/iio/adc/at91-sama5d2_adc.h | 16 +
>> include/uapi/linux/iio/types.h | 1 +
>> tools/iio/iio_event_monitor.c | 2 +
>> 14 files changed, 861 insertions(+), 58 deletions(-)
>> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/resistive-adc-touch.txt
>> create mode 100644 drivers/input/touchscreen/resistive-adc-touch.c
>> create mode 100644 include/dt-bindings/iio/adc/at91-sama5d2_adc.h
>>
>
> Hi All,
>
> I'm happy to take this, but there is a slight issue that we have a fix working
> it's way in which this is dependent on.
>
> I'll see if we can get this sorted before the merge window, but we may be
> cutting it fine.
>
> Jonathan
>
Hello Jonathan,
I can see the dependency fix made it to 4.17. What is the plan for this
series? Getting into this merge window ?
Thanks,
Eugen
^ permalink raw reply
* Re: [PATCH V2] cros_ec_keyb: Mark cros_ec_keyb driver as wake enabled device.
From: Lee Jones @ 2018-06-04 6:18 UTC (permalink / raw)
To: Ravi Chandra Sadineni
Cc: dmitry.torokhov, ravisadineni, dtor, briannorris, tbroch,
linux-kernel, linux-input, rajatja, bleung
In-Reply-To: <20180526011440.102417-1-ravisadineni@chromium.org>
On Fri, 25 May 2018, 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.
>
> drivers/input/keyboard/cros_ec_keyb.c | 21 +++++++++++++++++----
> drivers/mfd/cros_ec.c | 19 +++++++------------
For my own reference:
Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH] mfd: intel-lpss: Fix Intel Cannon Lake LPSS I2C input clock
From: Lee Jones @ 2018-06-04 7:39 UTC (permalink / raw)
To: Jarkko Nikula
Cc: linux-kernel, Andy Shevchenko, Mika Westerberg, linux-i2c,
linux-input, Jian-Hong Pan, Chris Chiu, Daniel Drake, stable
In-Reply-To: <20180518083827.20626-1-jarkko.nikula@linux.intel.com>
On Fri, 18 May 2018, Jarkko Nikula wrote:
> Intel Cannon Lake PCH has much higher 216 MHz input clock to LPSS I2C
> than Sunrisepoint which uses 120 MHz. Preliminary information was that
> both share the same clock rate but actual silicon implements elevated
> rate for better support for 3.4 MHz high-speed I2C.
>
> This incorrect input clock rate results too high I2C bus clock in case
> ACPI doesn't provide tuned I2C timing parameters since I2C host
> controller driver calculates them from input clock rate.
>
> Fix this by using the correct rate. We still share the same 230 ns SDA
> hold time value than Sunrisepoint.
>
> Cc: stable@vger.kernel.org
> Fixes: b418bbff36dd ("mfd: intel-lpss: Add Intel Cannonlake PCI IDs")
> Reported-by: Jian-Hong Pan <jian-hong@endlessm.com>
> Reported-by: Chris Chiu <chiu@endlessm.com>
> Reported-by: Daniel Drake <drake@endlessm.com>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> Hi Jian-Hong, Chris and Daniel. Could you test does this fix your
> touchpad issue?
> ---
> drivers/mfd/intel-lpss-pci.c | 25 +++++++++++++++----------
> 1 file changed, 15 insertions(+), 10 deletions(-)
Applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH v1 1/3] bitmap: Add bitmap_alloc(), bitmap_zalloc() and bitmap_free()
From: Andy Shevchenko @ 2018-06-04 9:59 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Jeffy Chen, linux-input, linux-kernel
In-Reply-To: <20180601183343.GA222005@dtor-ws>
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.
> > ---
> > 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
you have a chance to do that when applying it would be appreciated.
>
> > +}
> > +
> > +static inline void bitmap_free(const unsigned long *bitmap)
> > +{
> > + kfree(bitmap);
> > +}
> > +
> > /*
> > * lib/bitmap.c provides these functions:
> > */
> > --
> > 2.17.0
> >
>
> Thanks.
>
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v7 0/9] Add support for SAMA5D2 touchscreen
From: Jonathan Cameron @ 2018-06-04 10:04 UTC (permalink / raw)
To: Eugen Hristev, Jonathan Cameron
Cc: devicetree, alexandre.belloni, robh, linux-iio, dmitry.torokhov,
linux-kernel, ludovic.desroches, linux-input, linux-arm-kernel
In-Reply-To: <45ff713b-7811-22c6-2124-4fbcad59b7e8@microchip.com>
On 4 June 2018 07:15:57 BST, Eugen Hristev <eugen.hristev@microchip.com> wrote:
>
>
>On 22.05.2018 20:57, Jonathan Cameron wrote:
>> On Tue, 22 May 2018 10:52:30 +0300
>> Eugen Hristev <eugen.hristev@microchip.com> wrote:
>>
>>> Hello,
>>>
>>> This patch series is a rework of my previous series named:
>>> [PATCH 00/14] iio: triggers: add consumer support
>>>
>>> This is the version 7 of the series, and addresses the received
>feedback
>>> on the v2 series named:
>>> [PATCH v2 00/10] Add support for SAMA5D2 touchscreen
>>> and the v3 series named
>>> [PATCH v3 00/11] Add support for SAMA5D2 touchscreen
>>> and the v4 series named
>>> [PATCH v4 0/9] Add support for SAMA5D2 touchscreen
>>> and fixes one bug found in series named
>>> [PATCH v5 0/9] Add support for SAMA5D2 touchscreen
>>> and addresses comments in series named
>>> [PATCH v6 0/9] Add support for SAMA5D2 touchscreen
>>>
>>> This series applies on top of fixes-togreg branch of iio.git,
>>> specifically on top of commit:
>>> "f0c8d1f" : iio: adc: at91-sama5d2_adc:
>>> fix channel configuration for differential channels
>>>
>>> Jonathan, if you need me to rebase this on top of testing, let me
>know.
>>>
>>> Changes in previous versions are presented at the end of the cover
>letter below.
>>> Thanks everyone for the feedback. Below is the original v2 cover
>letter:
>>>
>>> In few words, this is the implementation of splitting the
>functionality
>>> of the IP block ADC device in SAMA5D2 SoC from ADC with touchscreen
>>> support. In order to avoid having a MFD device, two separate
>>> drivers that would work on same register base and split the IRQ,etc,
>>> as advised on the mailing list, I created a consumer driver for the
>>> channels, that will connect to the ADC as described in the device
>tree.
>>>
>>> I have collected feedback from everyone and here is the result:
>>> I have added a new generic resistive touchscreen driver, which acts
>>> as a iio consumer for the given channels and will create an input
>>> device and report the events. It uses a callback buffer to register
>>> to the IIO device and waits for data to be pushed.
>>> Inside the IIO device, I have kept a similar approach with the first
>version
>>> of the series, except that now the driver can take multiple buffers,
>and
>>> will configure the touchscreen part of the hardware device if the
>specific
>>> channels are requested.
>>>
>>> The SAMA5D2 ADC driver registers three new channels: two for the
>>> position on the X and Y axis, and one for the touch pressure.
>>> When channels are requested, it will check if the touchscreen
>channel mask
>>> includes the requested channels (it is possible that the consumer
>driver
>>> will not request pressure for example). If it's the case, it will
>work
>>> in touchscreen mode, and will refuse to do usual analog-digital
>conversion,
>>> because we have a single trigger and the touchscreen needs it.
>>> When the scan mask will include only old channels, the driver will
>function
>>> in the same way as before. If the scan mask somehow is a mix of the
>two (the
>>> masks intersect), the driver will refuse to work whatsoever (cannot
>have both
>>> in the same time).
>>> The driver allows reading raw data for the new channels, if claim
>direct
>>> mode works: no touchscreen driver requested anything. The new
>channels can
>>> act like the old ones. However, when requesting these channels, the
>usual
>>> trigger will not work and will not be enabled. The touchscreen
>channels
>>> require special trigger and irq configuration: pen detect, no pen
>detect
>>> and a periodic trigger to sample the touchscreen position and
>pressure.
>>> If the user attempts to use another trigger while there is a buffer
>>> that already requested the touchscreen channels (thus the trigger),
>the
>>> driver will refuse to comply.
>>>
>>> In order to have defines for the channel numbers, I added a bindings
>include
>>> file that goes on a separate commit :
>>> dt-bindings: iio: adc: at91-sama5d2_adc: add channel specific
>consumer info
>>> This should go in the same tree with the following commits :
>>> ARM: dts: at91: sama5d2: add channel cells for ADC device
>>> ARM: dts: at91: sama5d2: Add resistive touch device
>>>
>>> as build will break because these commits depend on the binding one
>>> which creates the included header file.
>>> V5 update: After discussing with Alexandre Belloni on Mailing list,
>the two
>>> DTS patches are to be taken in the next version after bindings reach
>mainline.
>>>
>>> Changes in v7:
>>> - Addressed some feedback from Dmitry, explained in input driver
>patch
>>> changelog.
>>> - Added Acked-by Dmitry.
>>>
>>> Changes in v6:
>>> - Fixed a crash in ADC driver , explained in driver patch
>changelog.
>>> - changed a dev_err to dev_dbg in input driver.
>>> - added Reviewed-by Rob Herring.
>>>
>>> Changes in v5:
>>> - renamed property touchscreen-threshold-pressure to
>touchscreen-min-pressure
>>> - added one return in touchscreen driver
>>>
>>> Changes in v4:
>>> - removed patch for inkern module get/set kref
>>> - addressed feedback on both the ADC and the touchscreen driver.
>each
>>> patch has a history inside the patch file for the specific changes.
>>> - patch that fixes the channel fix
>>> [PATCH v3 01/11] iio: adc: at91-sama5d2_adc:
>>> fix channel configuration for differential channels
>>> was accepted in fixes-togreg branch thus removed from this series.
>>> - added Reviewed-by for the bindings by Rob Herring
>>>
>>> Changes in v3:
>>> - changed input driver name according to feedback and reworked in
>commits
>>> to adapt to binding changes and new name.
>>> - moved channel index fix in at91-sama5d2_adc at the beginning of
>the series
>>> (PATCH 01/11)
>>> - created a new optional binding for the touchscreen as a separate
>commit
>>> and added it to the series :
>>> [PATCH v3 04/11] dt-bindings: input: touchscreen: add pressure
>>> threshold touchscreen property
>>> - changed at91-sama5d2_adc driver patch to address the comments.
>Exact changes
>>> are in the patch file for the driver source file.
>>>
>>> Eugen Hristev (9):
>>> MAINTAINERS: add generic resistive touchscreen adc
>>> iio: Add channel for Position Relative
>>> dt-bindings: input: touchscreen: add minimum pressure touchscreen
>>> property
>>> dt-bindings: input: touchscreen: resistive-adc-touch: create
>bindings
>>> iio: adc: at91-sama5d2_adc: add support for position and pressure
>>> channels
>>> input: touchscreen: resistive-adc-touch: add generic resistive
>ADC
>>> touchscreen
>>> dt-bindings: iio: adc: at91-sama5d2_adc: add channel specific
>consumer
>>> info
>>> ARM: dts: at91: sama5d2: add channel cells for ADC device
>>> ARM: dts: at91: sama5d2: Add resistive touch device
>>>
>>> Documentation/ABI/testing/sysfs-bus-iio | 12 +
>>> .../bindings/iio/adc/at91-sama5d2_adc.txt | 9 +
>>> .../input/touchscreen/resistive-adc-touch.txt | 30 +
>>> .../bindings/input/touchscreen/touchscreen.txt | 3 +
>>> MAINTAINERS | 6 +
>>> arch/arm/boot/dts/sama5d2.dtsi | 12 +
>>> drivers/iio/adc/at91-sama5d2_adc.c | 609
>+++++++++++++++++++--
>>> drivers/iio/industrialio-core.c | 1 +
>>> drivers/input/touchscreen/Kconfig | 13 +
>>> drivers/input/touchscreen/Makefile | 1 +
>>> drivers/input/touchscreen/resistive-adc-touch.c | 204 +++++++
>>> include/dt-bindings/iio/adc/at91-sama5d2_adc.h | 16 +
>>> include/uapi/linux/iio/types.h | 1 +
>>> tools/iio/iio_event_monitor.c | 2 +
>>> 14 files changed, 861 insertions(+), 58 deletions(-)
>>> create mode 100644
>Documentation/devicetree/bindings/input/touchscreen/resistive-adc-touch.txt
>>> create mode 100644 drivers/input/touchscreen/resistive-adc-touch.c
>>> create mode 100644 include/dt-bindings/iio/adc/at91-sama5d2_adc.h
>>>
>>
>> Hi All,
>>
>> I'm happy to take this, but there is a slight issue that we have a
>fix working
>> it's way in which this is dependent on.
>>
>> I'll see if we can get this sorted before the merge window, but we
>may be
>> cutting it fine.
>>
>> Jonathan
>>
>
>Hello Jonathan,
>
>I can see the dependency fix made it to 4.17. What is the plan for this
>
>series? Getting into this merge window ?
Sorry, no. We were to tight on time. Will get it into linux-next in a few weeks, ready for the next merge window.
Jonathan
>
>Thanks,
>Eugen
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Benjamin Tissoires @ 2018-06-04 12:57 UTC (permalink / raw)
To: Henrik Rydberg
Cc: Dmitry Torokhov, Jiri Kosina, Jason Gerecke, Dennis Kempin,
Andrew de los Reyes, open list:HID CORE LAYER, lkml
In-Reply-To: <72b7120a-d304-0b2f-d04a-473631623f72@bitmath.org>
On Fri, Jun 1, 2018 at 9:03 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
>
>>> 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.
>
> This is in part why I objected to this patch on August 11th, 2017.
> 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 :/
Cheers,
Benjamin
>
> Henrik
>
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Benjamin Tissoires @ 2018-06-04 13:18 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: <20180601184330.GD222005@dtor-ws>
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.
>
>>
>> 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?
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.
If you agree, I'll rebase your patch on top of my series as rebasing
my series on top of yours will take more effort.
I am trying to be cautious in the generic path because I want to merge
the cleanest multitouch implementation in hid-core/hid-input, and
leave all the quirks in hid-multitouch for the devices in need.
Cheers,
Benjamin
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Dmitry Torokhov @ 2018-06-04 17:27 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Henrik Rydberg, Jiri Kosina, Jason Gerecke, Dennis Kempin,
Andrew de los Reyes, open list:HID CORE LAYER, lkml
In-Reply-To: <CAO-hwJKibKvqviLZ5SSYR_X3yLTzJrr0G4d6svM3FtgJ6AfvQQ@mail.gmail.com>
On Mon, Jun 04, 2018 at 02:57:54PM +0200, Benjamin Tissoires wrote:
> On Fri, Jun 1, 2018 at 9:03 PM, Henrik Rydberg <rydberg@bitmath.org> wrote:
> >
> >>> 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.
> >
> > This is in part why I objected to this patch on August 11th, 2017.
> > 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 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.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Dmitry Torokhov @ 2018-06-04 17:33 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Henrik Rydberg, Jason Gerecke, Dennis Kempin,
Andrew de los Reyes, open list:HID CORE LAYER, lkml
In-Reply-To: <CAO-hwJJPGJzgX+7H1nnpd=OkLZu0q0TuKhvroJ1K3jtnv7t6dg@mail.gmail.com>
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.
>
> If you agree, I'll rebase your patch on top of my series as rebasing
> my series on top of yours will take more effort.
That would be great.
>
> I am trying to be cautious in the generic path because I want to merge
> the cleanest multitouch implementation in hid-core/hid-input, and
> leave all the quirks in hid-multitouch for the devices in need.
>
> Cheers,
> Benjamin
>
> >
> > Thanks.
> >
> > --
> > Dmitry
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Henrik Rydberg @ 2018-06-04 17:55 UTC (permalink / raw)
To: Dmitry Torokhov, Benjamin Tissoires
Cc: Jiri Kosina, Jason Gerecke, Dennis Kempin, Andrew de los Reyes,
open list:HID CORE LAYER, lkml
In-Reply-To: <20180604172759.GA164893@dtor-ws>
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.
> 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 also to a pen lying down on a touchpad, for instance.
Henrik
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Dmitry Torokhov @ 2018-06-04 18:26 UTC (permalink / raw)
To: Henrik Rydberg
Cc: Benjamin Tissoires, Jiri Kosina, Jason Gerecke, Dennis Kempin,
Andrew de los Reyes, open list:HID CORE LAYER, lkml
In-Reply-To: <994e5698-fbdf-05c8-b0b6-3df6af1b3dbc@bitmath.org>
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 it is kernel task to not signal transitions that do not make
sense, such as finger->pen or palm->pen etc.
>
> > 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
> 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?
Thanks.
--
Dmitry
^ permalink raw reply
* RE: [PATCH] HID: core: allow concurrent registration of drivers
From: Mario.Limonciello @ 2018-06-04 20:37 UTC (permalink / raw)
To: benjamin.tissoires, jikos; +Cc: linux-input, linux-kernel
In-Reply-To: <20180531114929.30545-1-benjamin.tissoires@redhat.com>
Benjamin,
I had to make a minor change since I didn't see
> #define HID_STAT_DUP_DETECTED BIT(2)
In Linus's tree.
I tested with master today though and it seems to be working now with your patch across
several boot attempts.
Tested-by: Mario Limonciello <mario.limonciello@dell.com>
Thanks,
> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Sent: Thursday, May 31, 2018 6:49 AM
> To: Jiri Kosina; Limonciello, Mario
> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; Benjamin Tissoires
> Subject: [PATCH] HID: core: allow concurrent registration of drivers
>
> Detected on the Dell XPS 9365.
> The laptop has 2 devices that benefit from the hid-generic auto-unbinding.
> When those 2 devices are presented to the userspace, udev loads both
> wacom and hid-multitouch. When this happens, the code in
> __hid_bus_reprobe_drivers() is called concurrently and the second device
> gets reprobed twice.
> An other bug in the power_supply subsystem prevent to remove the wacom
> driver if it just finished its initialization, which basically kills
> the wacom node.
>
> Fixes c17a7476e4c4 ("HID: core: rewrite the hid-generic automatic unbind")
>
> Cc: stable@vger.kernel.org # v4.17
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> Hi Mario,
>
> can you please test this on your faulty XPS?
> I think it'll fix the issue, but I can not reproduce, so better wait for
> your confirmation.
>
> Jiri, ideally I would love to see this in v4.17 final, but Mario seems
> to be on PTO until next week. I guess we'll just push this in v4.17.1
> then. Also, if you can double check that it makes sense, that would be
> nice :)
>
> Cheers,
> Benjamin
>
> drivers/hid/hid-core.c | 5 ++++-
> include/linux/hid.h | 3 ++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 2f7367b1de00..7afed0c0f9e5 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1960,6 +1960,8 @@ static int hid_device_probe(struct device *dev)
> }
> hdev->io_started = false;
>
> + clear_bit(ffs(HID_STAT_REPROBED), &hdev->status);
> +
> if (!hdev->driver) {
> id = hid_match_device(hdev, hdrv);
> if (id == NULL) {
> @@ -2223,7 +2225,8 @@ static int __hid_bus_reprobe_drivers(struct device *dev,
> void *data)
> struct hid_device *hdev = to_hid_device(dev);
>
> if (hdev->driver == hdrv &&
> - !hdrv->match(hdev, hid_ignore_special_drivers))
> + !hdrv->match(hdev, hid_ignore_special_drivers) &&
> + !test_and_set_bit(ffs(HID_STAT_REPROBED), &hdev->status))
> return device_reprobe(dev);
>
> return 0;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index ee2510019033..aee281522c6d 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -517,6 +517,7 @@ struct hid_output_fifo {
> #define HID_STAT_ADDED BIT(0)
> #define HID_STAT_PARSED BIT(1)
> #define HID_STAT_DUP_DETECTED BIT(2)
> +#define HID_STAT_REPROBED BIT(3)
>
> struct hid_input {
> struct list_head list;
> @@ -585,7 +586,7 @@ struct hid_device {
> /* device report descriptor */
> bool battery_avoid_query;
> #endif
>
> - unsigned int status; /* see
> STAT flags above */
> + unsigned long status; /* see
> STAT flags above */
> unsigned claimed; /*
> Claimed by hidinput, hiddev? */
> unsigned quirks; /* Various
> quirks the device can pull on us */
> bool io_started; /* If IO has started
> */
> --
> 2.14.3
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Benjamin Tissoires @ 2018-06-04 20:42 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: <20180604173339.GB164893@dtor-ws>
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).
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.
>
>>
>> If you agree, I'll rebase your patch on top of my series as rebasing
>> my series on top of yours will take more effort.
>
> That would be great.
\o/
Cheers,
Benjamin
>
>>
>> I am trying to be cautious in the generic path because I want to merge
>> the cleanest multitouch implementation in hid-core/hid-input, and
>> leave all the quirks in hid-multitouch for the devices in need.
>>
>> Cheers,
>> Benjamin
>>
>> >
>> > Thanks.
>> >
>> > --
>> > Dmitry
>
> 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 20:59 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: <20180604182658.GC164893@dtor-ws>
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?
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
>
> 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.
>
>>
>> > 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.
>> 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.
Cheers,
Benjamin
>
> Thanks.
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Dmitry Torokhov @ 2018-06-04 21:19 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Henrik Rydberg, Jason Gerecke, Dennis Kempin,
Andrew de los Reyes, open list:HID CORE LAYER, lkml
In-Reply-To: <CAO-hwJ+nzC=i1Jv_yu4vLL6Quw=850HJkOZBKCjFaZaw0-GWOQ@mail.gmail.com>
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...
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
From: Dmitry Torokhov @ 2018-06-04 21:32 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Henrik Rydberg, Jiri Kosina, Jason Gerecke, Dennis Kempin,
Andrew de los Reyes, open list:HID CORE LAYER, lkml
In-Reply-To: <CAO-hwJK3h8xjuO=GQRT0prdbXURCTinkQg4i7N8ScPOig1KTYw@mail.gmail.com>
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.
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.
>
> >> 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...
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 related
* Re: [PATCH V2] i8042: Increment wakeup_count for the respective port.
From: Dmitry Torokhov @ 2018-06-04 21:53 UTC (permalink / raw)
To: Ravi Chandra Sadineni, Rafael J. Wysocki
Cc: chenhong3, ravisadineni, dtor, tbroch, linux-kernel, linux-input,
rajatja, bleung
In-Reply-To: <20180602010708.152025-1-ravisadineni@chromium.org>
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. Rafael, can we move the check into
pm_wakeup_dev_event()?
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?
> +
> filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
> port_no, irq,
> dfl & SERIO_PARITY ? ", bad parity" : "",
> --
> 2.17.1.1185.g55be947832-goog
>
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