Re: [PATCH v5 1/1] applesmc: Re-work SMC comms
On 2020-11-11 14:06, Brad Campbell wrote:
Commit fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
introduced an issue whereby communication with the SMC became
unreliable with write errors like :
[ 120.378614] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.378621] applesmc: LKSB: write data fail
[ 120.512782] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.512787] applesmc: LKSB: write data fail
The original code appeared to be timing sensitive and was not reliable
with the timing changes in the aforementioned commit.
This patch re-factors the SMC communication to remove the timing
dependencies and restore function with the changes previously
committed. Logic changes based on inspection of the Apple SMC kext.
Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
MacBookAir3,1
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Reported-by: Andreas Kemnade
Tested-by: Andreas Kemnade # MacBookAir6,2
Acked-by: Arnd Bergmann
Signed-off-by: Brad Campbell
Signed-off-by: Henrik Rydberg
---
Changelog :
v1 : Initial attempt
v2 : Address logic and coding style
v3 : Removed some debug hangover. Added tested-by. Modifications for
MacBookAir1,1
v4 : Re-factored logic based on Apple driver. Simplified wait_status loop
v5 : Re-wrote status loop. Simplified busy check in send_byte(). Fixed
formatting
Hi Brad,
This version is still working fine on the MBA1,1, at 50 reads per second.
Thanks,
Henrik
Re: [PATCH v3] applesmc: Re-work SMC comms
Hi Brad, Out of morbid curiosity I grabbed an older MacOS AppleSMC.kext (10.7) and ran it through the disassembler. Every read/write to the SMC starts the same way with a check to make sure the SMC is in a sane state. If it's not, a read command is sent to try and kick it back into line : Wait for 0x04 to clear. This is 1,000,000 iterations of "read status, check if 0x04 is set, delay 10uS". If it clears, move on. If it doesn't, try and send a read command (just the command 0x10) and wait for the busy flag to clear again with the same loop. So in theory if the SMC was locked up, it'd be into the weeds for 20 seconds before it pushed the error out. So, lets say we've waited long enough and the busy flag dropped : Each command write is : Wait for 0x02 to clear. This is 1,000,000 iterations of "read status, check if 0x02 is set, delay 10uS". Send command Each data byte write is : Wait for 0x02 to clear. This is 1,000,000 iterations of "read status, check if 0x02 is set, delay 10uS". Immediate and single status read, check 0x04. If not set, abort. Send data byte Each data byte read is : read staus, wait for 0x01 and 0x04 to be set. delay 10uS and repeat. Abort if fail. Each timeout is 1,000,000 loops with a 10uS delay. So aside from the startup set which occurs on *every* read or write set, status checks happen before a command or data write, and not at all after. Under no circumstances are writes of any kind re-tried, but these timeouts are up to 10 seconds! Great findings here. But from this, it would seem we are doing almost the right thing already, no? The essential difference seems to be that where the kext does a read to wake up the SMC, while we retry the first command until it works. If would of course be very interesting to know if that makes a difference. That would indicate that the requirement for retries on the early Mac means we're not waiting long enough somewhere. Not that I'm suggesting we do another re-work, but when I get back in front of my iMac which does 17 transactions per second with this driver, I might re-work it similar to the Apple driver and see what happens. Oh, and it looks like the 0x40 flag that is on mine is the "I have an interrupt pending" flag, and the result should be able to be read from 0x31F. I'll play with that when I get time. That probably explains why IRQ9 screams until the kernel gags it on this machine as it's not being given any love. Sounds good, getting interrupts working would have been nice. Henrik
Re: [PATCH v3] applesmc: Re-work SMC comms
On 2020-11-08 12:57, Brad Campbell wrote: On 8/11/20 9:14 pm, Henrik Rydberg wrote: On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote: Hi Brad, On 2020-11-08 02:00, Brad Campbell wrote: G'day Henrik, I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in read_smc(). I assume that causes problems on the early Macbook. This is revised on the one sent earlier. If you could test this on your Air1,1 it'd be appreciated. No, I managed to screw up the patch; you can see that I carefully added the same treatment for the read argument, being unsure if the BUSY state would remain during the AVAILABLE data phase. I can check that again, but unfortunately the patch in this email shows the same problem. I think it may be worthwhile to rethink the behavior of wait_status() here. If one machine shows no change after a certain status bit change, then perhaps the others share that behavior, and we are waiting in vain. Just imagine how many years of cpu that is, combined. ;-) Here is a modification along that line. Compared to your latest version, this one has wait_status() return the actual status on success. Instead of waiting for BUSY, it waits for the other status bits, and checks BUSY afterwards. So as not to wait unneccesarily, the udelay() is placed together with the single outb(). The return value of send_byte_data() is augmented with -EAGAIN, which is then used in send_command() to create the resend loop. I reach 41 reads per second on the MBA1,1 with this version, which is getting close to the performance prior to the problems. G'day Henrik, I like this one. It's slower on my laptop (40 rps vs 50 on the MacbookPro11,1) and the same 17 rps on the iMac 12,2 but it's as reliable and if it works for both of yours then I think it's a winner. I can't really diagnose the iMac properly as I'm 2,800KM away and have nobody to reboot it if I kill it. 5.7.2 gives 20 rps, so 17 is ok for me. Andreas, could I ask you to test this one? Odd my original version worked on your Air3,1 and the other 3 machines without retry. I wonder how many commands require retries, how many retires are actually required, and what we are going wrong on the Air1,1 that requires one or more retries. I just feels like a brute force approach because there's something we're missing. I would think you are right. There should be a way to follow the status changes in realtime, so one can determine handshake and processing from that information. At least, with this change, we are making the blunt instrument a little smaller. Cheers, Henrik
Re: [PATCH v3] applesmc: Re-work SMC comms
On Sun, Nov 08, 2020 at 09:35:28AM +0100, Henrik Rydberg wrote:
> Hi Brad,
>
> On 2020-11-08 02:00, Brad Campbell wrote:
> > G'day Henrik,
> >
> > I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in
> > read_smc(). I assume
> > that causes problems on the early Macbook. This is revised on the one sent
> > earlier.
> > If you could test this on your Air1,1 it'd be appreciated.
>
> No, I managed to screw up the patch; you can see that I carefully added the
> same treatment for the read argument, being unsure if the BUSY state would
> remain during the AVAILABLE data phase. I can check that again, but
> unfortunately the patch in this email shows the same problem.
>
> I think it may be worthwhile to rethink the behavior of wait_status() here.
> If one machine shows no change after a certain status bit change, then
> perhaps the others share that behavior, and we are waiting in vain. Just
> imagine how many years of cpu that is, combined. ;-)
Here is a modification along that line.
Compared to your latest version, this one has wait_status() return the
actual status on success. Instead of waiting for BUSY, it waits for
the other status bits, and checks BUSY afterwards. So as not to wait
unneccesarily, the udelay() is placed together with the single
outb(). The return value of send_byte_data() is augmented with
-EAGAIN, which is then used in send_command() to create the resend
loop.
I reach 41 reads per second on the MBA1,1 with this version, which is
getting close to the performance prior to the problems.
>From b4405457f4ba07cff7b7e4f48c47668bee176a25 Mon Sep 17 00:00:00 2001
From: Brad Campbell
Date: Sun, 8 Nov 2020 12:00:03 +1100
Subject: [PATCH] hwmon: (applesmc) Re-work SMC comms
Commit fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
introduced an issue whereby communication with the SMC became
unreliable with write errors like :
[ 120.378614] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.378621] applesmc: LKSB: write data fail
[ 120.512782] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.512787] applesmc: LKSB: write data fail
The original code appeared to be timing sensitive and was not reliable
with the timing changes in the aforementioned commit.
This patch re-factors the SMC communication to remove the timing
dependencies and restore function with the changes previously
committed.
Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2, MacBookAir1,1,
MacBookAir3,1
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Reported-by: Andreas Kemnade
Tested-by: Andreas Kemnade # MacBookAir6,2
Acked-by: Arnd Bergmann
Signed-off-by: Brad Campbell
Signed-off-by: Henrik Rydberg
---
Changelog :
v1 : Inital attempt
v2 : Address logic and coding style
v3 : Removed some debug hangover. Added tested-by. Modifications for
MacBookAir1,1
v4 : Do not expect busy state to appear without other state changes
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..ea7c66d5788e 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
/* data port used by Apple SMC */
#define APPLESMC_DATA_PORT 0x300
@@ -42,6 +43,11 @@
#define APPLESMC_MAX_DATA_LENGTH 32
+/* Apple SMC status bits */
+#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting */
+#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
+#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
+
/* wait up to 128 ms for a status change. */
#define APPLESMC_MIN_WAIT 0x0010
#define APPLESMC_RETRY_WAIT0x0100
@@ -151,65 +157,78 @@ static unsigned int key_at_index;
static struct workqueue_struct *applesmc_led_wq;
/*
- * wait_read - Wait for a byte to appear on SMC port. Callers must
- * hold applesmc_lock.
+ * Wait for specific status bits with a mask on the SMC
+ * Used before and after writes, and before reads
+ * On success, returns the full status
+ * On failure, returns a negative error
*/
-static int wait_read(void)
+
+static int wait_status(u8 val, u8 mask)
{
unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
u8 status;
int us;
for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
status = inb(APPLESMC_CMD_PORT);
- /* read: wait for smc to settle */
- if (status & 0x01)
- return 0;
+ if ((status & mask) == val)
+ return status;
/* timeout: give up */
if (time_after(jiffies, end))
break;
+ usleep_range(us, us * 16);
}
-
- pr_warn("wait_read() fail: 0x%02x\n", status);
return -EIO;
}
/*
- *
Re: [PATCH v3] applesmc: Re-work SMC comms
Hi Brad,
On 2020-11-08 02:00, Brad Campbell wrote:
G'day Henrik,
I noticed you'd also loosened up the requirement for SMC_STATUS_BUSY in
read_smc(). I assume
that causes problems on the early Macbook. This is revised on the one sent
earlier.
If you could test this on your Air1,1 it'd be appreciated.
No, I managed to screw up the patch; you can see that I carefully added
the same treatment for the read argument, being unsure if the BUSY state
would remain during the AVAILABLE data phase. I can check that again,
but unfortunately the patch in this email shows the same problem.
I think it may be worthwhile to rethink the behavior of wait_status()
here. If one machine shows no change after a certain status bit change,
then perhaps the others share that behavior, and we are waiting in vain.
Just imagine how many years of cpu that is, combined. ;-)
Henrik
Commit fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") introduced
an issue whereby communication with the SMC became unreliable with write
errors like :
[ 120.378614] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.378621] applesmc: LKSB: write data fail
[ 120.512782] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.512787] applesmc: LKSB: write data fail
The original code appeared to be timing sensitive and was not reliable with
the timing changes in the aforementioned commit.
This patch re-factors the SMC communication to remove the timing
dependencies and restore function with the changes previously committed.
Tested on : MacbookAir6,2 MacBookPro11,1 iMac12,2
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Reported-by: Andreas Kemnade
Tested-by: Andreas Kemnade # MacBookAir6,2
Acked-by: Arnd Bergmann
Signed-off-by: Brad Campbell
Signed-off-by: Henrik Rydberg
---
Changelog :
v1 : Inital attempt
v2 : Address logic and coding style
v3 : Removed some debug hangover. Added tested-by. Modifications for
MacBookAir1,1
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..3e968abb37aa 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
/* data port used by Apple SMC */
#define APPLESMC_DATA_PORT0x300
@@ -42,6 +43,11 @@
#define APPLESMC_MAX_DATA_LENGTH 32
+/* Apple SMC status bits */
+#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting */
+#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
+#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
+
/* wait up to 128 ms for a status change. */
#define APPLESMC_MIN_WAIT 0x0010
#define APPLESMC_RETRY_WAIT 0x0100
@@ -151,65 +157,73 @@ static unsigned int key_at_index;
static struct workqueue_struct *applesmc_led_wq;
/*
- * wait_read - Wait for a byte to appear on SMC port. Callers must
- * hold applesmc_lock.
+ * Wait for specific status bits with a mask on the SMC
+ * Used before and after writes, and before reads
*/
-static int wait_read(void)
+
+static int wait_status(u8 val, u8 mask)
{
unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
u8 status;
int us;
+ udelay(APPLESMC_MIN_WAIT);
for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
status = inb(APPLESMC_CMD_PORT);
- /* read: wait for smc to settle */
- if (status & 0x01)
+ if ((status & mask) == val)
return 0;
/* timeout: give up */
if (time_after(jiffies, end))
break;
+ usleep_range(us, us * 16);
}
-
- pr_warn("wait_read() fail: 0x%02x\n", status);
return -EIO;
}
/*
- * send_byte - Write to SMC port, retrying when necessary. Callers
+ * send_byte_data - Write to SMC data port. Callers
* must hold applesmc_lock.
+ * Parameter skip must be true on the last write of any
+ * command or it'll time out.
*/
-static int send_byte(u8 cmd, u16 port)
+
+static int send_byte_data(u8 cmd, u16 port, bool skip)
{
- u8 status;
- int us;
- unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
+ int ret;
+ ret = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED);
+ if (ret)
+ return ret;
outb(cmd, port);
- for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
- status = inb(APPLESMC_CMD_PORT);
- /* write: wait for smc to settle */
- if (status & 0x02)
- continue;
- /* ready: cmd accepted, return */
- if (status & 0x04)
- return 0;
- /* timeout: give up */
-
Re: [PATCH] applesmc: Re-work SMC comms v2
Hi Brad, G'day Henrik, Which kernel was this based on? It won't apply to my 5.9 tree. I was being lazy and applied the diff to linus/master on top of my current stable branch. More importantly, I sent the mail out from an email client that may not format the patch properly; I'll fix that. I assume the sprinkling of udelay(APPLESMC_MIN_WAIT) means the SMC is slow in getting its status register set up. Could we instead just put a single one of those up-front in wait_status? That works fine, just a matter of taste. Any chance you could try this one? I've added a retry to send_command and added a single global APPLESMC_MIN_WAIT before each status read. From looking at your modified send_command, it appears the trigger for a retry is sending a command and the SMC doing absolutely nothing. This should do the same thing. Not quite, unfortunately. The patch that works waits for a drop of IB_CLOSED, then checks the BUSY status. If not seen, it resends immediately, never expecting to see it. The patch in this email creates a dreadfully sluggish probe, and the occasional failure. Interestingly enough, by adding the udelay to wait_status on my machine I've gone from 24 reads/s to 50 reads/s. Yep, I experience the same positive effect. I've left out the remainder of the cleanups. Once we get a minimally working patch I was going to look at a few cleanups, and I have some patches pending to allow writing to the SMC from userspace (for setting BCLM and BFCL mainly) All fine. I will respond to the v3 mail separately. Henrik
Re: [PATCH] applesmc: Re-work SMC comms v2
On 2020-11-06 21:02, Henrik Rydberg wrote:
So as it stands, it does not work at all. I will continue to check
another machine, and see if I can get something working.
On the MacBookAir3,1 the situation is somewhat better.
The first three tree positions result in zero failures and 10 reads per
second. The fourth yields zero failues and 11 reads per second, within
the margin of similarity.
So, the patch appears to have no apparent effect on the 3,1 series.
Now onto fixing the 1,1 behavior.
Hi again,
This patch, v3, works for me, on both MBA1,1 and MBA3,1. Both machines
yields 25 reads per second.
It turns out that the origin code has a case that was not carried over
to the v2 patch; the command byte needs to be resent upon the wrong
status code. I added that back. Also, there seems to be a basic response
time that needs to be respected, so I added back a small fixed delay
after each write operation. I also took the liberty to reduce the number
of status reads, and clean up error handling. Checkpatch is happy with
this version.
The code obviously needs to be retested on the other machines, but the
logic still follows what you wrote, Brad, and I have also checked it
against the VirtualSMC code. It appears to make sense, so hopefully
there wont be additional issues.
Thanks,
Henrik
From be4a32620b2b611472af3e35f9b50004e678efd5 Mon Sep 17 00:00:00 2001
From: Brad Campbell
Date: Thu, 5 Nov 2020 18:26:24 +1100
Subject: [PATCH] applesmc: Re-work SMC comms v3
Commit fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
introduced an issue whereby communication with the SMC became
unreliable with write errors like:
[ 120.378614] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.378621] applesmc: LKSB: write data fail
[ 120.512782] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.512787] applesmc: LKSB: write data fail
The original code appeared to be timing sensitive and was not reliable with
the timing changes in the aforementioned commit.
This patch re-factors the SMC communication to remove the timing
dependencies and restore function with the changes previously committed.
v2 : Address logic and coding style
v3 : Modifications for MacBookAir1,1
Reported-by: Andreas Kemnade
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Signed-off-by: Brad Campbell
Signed-off-by: Henrik Rydberg
---
drivers/hwmon/applesmc.c | 132 +--
1 file changed, 70 insertions(+), 62 deletions(-)
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..08289827da1e 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -42,6 +42,11 @@
#define APPLESMC_MAX_DATA_LENGTH 32
+/* Apple SMC status bits */
+#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting */
+#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
+#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
+
/* wait up to 128 ms for a status change. */
#define APPLESMC_MIN_WAIT 0x0010
#define APPLESMC_RETRY_WAIT0x0100
@@ -151,65 +156,76 @@ static unsigned int key_at_index;
static struct workqueue_struct *applesmc_led_wq;
/*
- * wait_read - Wait for a byte to appear on SMC port. Callers must
- * hold applesmc_lock.
+ * Wait for specific status bits with a mask on the SMC
+ * Used before and after writes, and before reads
*/
-static int wait_read(void)
+
+static int wait_status(u8 val, u8 mask)
{
unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
u8 status;
int us;
for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
status = inb(APPLESMC_CMD_PORT);
- /* read: wait for smc to settle */
- if (status & 0x01)
+ if ((status & mask) == val)
return 0;
/* timeout: give up */
if (time_after(jiffies, end))
break;
+ usleep_range(us, us * 16);
}
- pr_warn("wait_read() fail: 0x%02x\n", status);
+ if (debug)
+ pr_warn("%s fail: 0x%02x 0x%02x 0x%02x\n", __func__, val, mask,
status);
return -EIO;
}
/*
- * send_byte - Write to SMC port, retrying when necessary. Callers
+ * send_byte_data - Write to SMC data port. Callers
* must hold applesmc_lock.
+ * Parameter skip must be true on the last write of any
+ * command or it'll time out.
*/
-static int send_byte(u8 cmd, u16 port)
+
+static int send_byte_data(u8 cmd, u16 port, bool skip)
+{
+ outb(cmd, port);
+ udelay(APPLESMC_MIN_WAIT);
+ return wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY |
SMC_STATUS_IB_CLOSED);
+}
+
+/*
+ * send_command - Write a command to the SMC. Callers must hold
applesmc_lock.
+ * If SMC is in undefined state, any new command write resets th
Re: [PATCH] applesmc: Re-work SMC comms v2
So as it stands, it does not work at all. I will continue to check another machine, and see if I can get something working. On the MacBookAir3,1 the situation is somewhat better. The first three tree positions result in zero failures and 10 reads per second. The fourth yields zero failues and 11 reads per second, within the margin of similarity. So, the patch appears to have no apparent effect on the 3,1 series. Now onto fixing the 1,1 behavior. Henrik
Re: [PATCH] applesmc: Re-work SMC comms v2
I can't guarantee it won't break older machines which is why I've asked for help testing it. I only have a MacbookPro 11,1 and an iMac 12,2. It fixes both of those. Help testing would be much appreciated. I see, this makes much more sense. I may be able to run some tests tonight. Meanwhile, looking at the patch, the status variable in send_command looks superfluous now that there is a wait_status() before it. Ok, it took some time to get the machines up to speed, but so far I have managed to run some tests on an MacBookAir1,1. I only managed to upgrade up to 4.15, so I had to revert the inputdev polling patch, but the rest applied without problems. I recovered an old test program I used to use (attached), and checked for failures and reads per second *** hwmon: (applesmc) switch to using input device polling mode At this point in the tree, with this reverted, I see 0 failures and 55 reads per second. *** hwmon: (applesmc) avoid overlong udelay() With this applied, I see 0 failures and 16 reads per second. *** hwmon: (applesmc) check status earlier. With this applied, I see 0 failures and 16 reads per second. *** (HEAD -> stable) applesmc: Re-work SMC comms v2 With this applied, the kernel hangs in module probe, and the kernel log is flooded with read failures. So as it stands, it does not work at all. I will continue to check another machine, and see if I can get something working. Henrik applesmc-test.sh Description: application/shellscript
Re: [PATCH] applesmc: Re-work SMC comms v2
On 2020-11-05 09:30, Brad Campbell wrote: On 5/11/20 6:56 pm, Henrik Rydberg wrote: Hi Brad, Great to see this effort, it is certainly an area which could be improved. After having seen several generations of Macbooks while modifying much of that code, it became clear that the SMC communication got refreshed a few times over the years. Every tiny change had to be tested on all machines, or kept separate for a particular generation, or something would break. I have not followed the back story here, but I imagine the need has arisen because of a new refresh, and so this patch only needs to strictly apply to a new generation. I would therefore advice that you write the patch in that way, reducing the actual change to zero for earlier generations. It also makes it easier to test the effect of the new approach on older systems. I should be able to help testing on a 2008 and 2011 model once we get to that stage. G'day Henrik, Unfortunately I didn't make these changes to accommodate a "new generation". Changes made in kernel 5.9 broke it on my machine and in looking at why didn't identify any obvious causes, so I re-worked some of the comms. I can't guarantee it won't break older machines which is why I've asked for help testing it. I only have a MacbookPro 11,1 and an iMac 12,2. It fixes both of those. Help testing would be much appreciated. I see, this makes much more sense. I may be able to run some tests tonight. Meanwhile, looking at the patch, the status variable in send_command looks superfluous now that there is a wait_status() before it. Thanks, Henrik
Re: [PATCH] applesmc: Re-work SMC comms v2
Hi Brad,
Great to see this effort, it is certainly an area which could be
improved. After having seen several generations of Macbooks while
modifying much of that code, it became clear that the SMC communication
got refreshed a few times over the years. Every tiny change had to be
tested on all machines, or kept separate for a particular generation, or
something would break.
I have not followed the back story here, but I imagine the need has
arisen because of a new refresh, and so this patch only needs to
strictly apply to a new generation. I would therefore advice that you
write the patch in that way, reducing the actual change to zero for
earlier generations. It also makes it easier to test the effect of the
new approach on older systems. I should be able to help testing on a
2008 and 2011 model once we get to that stage.
Thanks,
Henrik
On 2020-11-05 08:26, Brad Campbell wrote:
Commit fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()") introduced
an issue whereby communication with the SMC became unreliable with write
errors like :
[ 120.378614] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.378621] applesmc: LKSB: write data fail
[ 120.512782] applesmc: send_byte(0x00, 0x0300) fail: 0x40
[ 120.512787] applesmc: LKSB: write data fail
The original code appeared to be timing sensitive and was not reliable with
the timing changes in the aforementioned commit.
This patch re-factors the SMC communication to remove the timing
dependencies and restore function with the changes previously committed.
v2 : Address logic and coding style
Reported-by: Andreas Kemnade
Fixes: fff2d0f701e6 ("hwmon: (applesmc) avoid overlong udelay()")
Signed-off-by: Brad Campbell
---
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index a18887990f4a..de890f3ec12f 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -42,6 +42,11 @@
#define APPLESMC_MAX_DATA_LENGTH 32
+/* Apple SMC status bits */
+#define SMC_STATUS_AWAITING_DATA BIT(0) /* SMC has data waiting */
+#define SMC_STATUS_IB_CLOSED BIT(1) /* Will ignore any input */
+#define SMC_STATUS_BUSY BIT(2) /* Command in progress */
+
/* wait up to 128 ms for a status change. */
#define APPLESMC_MIN_WAIT 0x0010
#define APPLESMC_RETRY_WAIT 0x0100
@@ -151,65 +156,69 @@ static unsigned int key_at_index;
static struct workqueue_struct *applesmc_led_wq;
/*
- * wait_read - Wait for a byte to appear on SMC port. Callers must
- * hold applesmc_lock.
+ * Wait for specific status bits with a mask on the SMC
+ * Used before and after writes, and before reads
*/
-static int wait_read(void)
+
+static int wait_status(u8 val, u8 mask)
{
unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
u8 status;
int us;
for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
status = inb(APPLESMC_CMD_PORT);
- /* read: wait for smc to settle */
- if (status & 0x01)
+ if ((status & mask) == val)
return 0;
/* timeout: give up */
if (time_after(jiffies, end))
break;
- }
-
- pr_warn("wait_read() fail: 0x%02x\n", status);
+ usleep_range(us, us * 16);
+ }
return -EIO;
}
/*
- * send_byte - Write to SMC port, retrying when necessary. Callers
+ * send_byte_data - Write to SMC data port. Callers
* must hold applesmc_lock.
+ * Parameter skip must be true on the last write of any
+ * command or it'll time out.
*/
-static int send_byte(u8 cmd, u16 port)
+
+static int send_byte_data(u8 cmd, u16 port, bool skip)
{
- u8 status;
- int us;
- unsigned long end = jiffies + (APPLESMC_MAX_WAIT * HZ) / USEC_PER_SEC;
+ int ret;
+ ret = wait_status(SMC_STATUS_BUSY, SMC_STATUS_BUSY | SMC_STATUS_IB_CLOSED);
+ if (ret)
+ return ret;
outb(cmd, port);
- for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
- usleep_range(us, us * 16);
- status = inb(APPLESMC_CMD_PORT);
- /* write: wait for smc to settle */
- if (status & 0x02)
- continue;
- /* ready: cmd accepted, return */
- if (status & 0x04)
- return 0;
- /* timeout: give up */
- if (time_after(jiffies, end))
- break;
- /* busy: long wait and resend */
- udelay(APPLESMC_RETRY_WAIT);
- outb(cmd, port);
- }
+ return wait_status(skip ? 0 : SMC_STATUS_BUSY, SMC_STATUS_BUSY);
+}
- pr_warn("send_byte(0x%02x, 0x%04x) fail: 0x%02x\n", cmd, port, status);
- return -EIO;
+static int send_byte(u8 cmd, u16 port)
+{
+ return send_byte_data(cmd, port, false);
}
Re: [PATCH] hwmon: applesmc: check status earlier.
Hi Tom,
From: Tom Rix
clang static analysis reports this representative problem
applesmc.c:758:10: warning: 1st function call argument is an
uninitialized value
left = be16_to_cpu(*(__be16 *)(buffer + 6)) >> 2;
^~~~
buffer is filled by the earlier call
ret = applesmc_read_key(LIGHT_SENSOR_LEFT_KEY, ...
This problem is reported because a goto skips the status check.
Other similar problems use data from applesmc_read_key before checking
the status. So move the checks to before the use.
Signed-off-by: Tom Rix
---
drivers/hwmon/applesmc.c | 31 ---
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 316618409315..a18887990f4a 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -753,15 +753,18 @@ static ssize_t applesmc_light_show(struct device *dev,
}
ret = applesmc_read_key(LIGHT_SENSOR_LEFT_KEY, buffer, data_length);
+ if (ret)
+ goto out;
/* newer macbooks report a single 10-bit bigendian value */
if (data_length == 10) {
left = be16_to_cpu(*(__be16 *)(buffer + 6)) >> 2;
goto out;
}
left = buffer[2];
+
+ ret = applesmc_read_key(LIGHT_SENSOR_RIGHT_KEY, buffer, data_length);
if (ret)
goto out;
- ret = applesmc_read_key(LIGHT_SENSOR_RIGHT_KEY, buffer, data_length);
right = buffer[2];
out:
@@ -810,12 +813,11 @@ static ssize_t applesmc_show_fan_speed(struct device *dev,
to_index(attr));
ret = applesmc_read_key(newkey, buffer, 2);
- speed = ((buffer[0] << 8 | buffer[1]) >> 2);
-
if (ret)
return ret;
- else
- return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", speed);
+
+ speed = ((buffer[0] << 8 | buffer[1]) >> 2);
+ return snprintf(sysfsbuf, PAGE_SIZE, "%u\n", speed);
}
static ssize_t applesmc_store_fan_speed(struct device *dev,
@@ -851,12 +853,11 @@ static ssize_t applesmc_show_fan_manual(struct device
*dev,
u8 buffer[2];
ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
- manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
-
if (ret)
return ret;
- else
- return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", manual);
+
+ manual = ((buffer[0] << 8 | buffer[1]) >> to_index(attr)) & 0x01;
+ return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", manual);
}
static ssize_t applesmc_store_fan_manual(struct device *dev,
@@ -872,10 +873,11 @@ static ssize_t applesmc_store_fan_manual(struct device
*dev,
return -EINVAL;
ret = applesmc_read_key(FANS_MANUAL, buffer, 2);
- val = (buffer[0] << 8 | buffer[1]);
if (ret)
goto out;
+ val = (buffer[0] << 8 | buffer[1]);
+
if (input)
val = val | (0x01 << to_index(attr));
else
@@ -951,13 +953,12 @@ static ssize_t applesmc_key_count_show(struct device *dev,
u32 count;
ret = applesmc_read_key(KEY_COUNT_KEY, buffer, 4);
- count = ((u32)buffer[0]<<24) + ((u32)buffer[1]<<16) +
- ((u32)buffer[2]<<8) + buffer[3];
-
if (ret)
return ret;
- else
- return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", count);
+
+ count = ((u32)buffer[0]<<24) + ((u32)buffer[1]<<16) +
+ ((u32)buffer[2]<<8) + buffer[3];
+ return snprintf(sysfsbuf, PAGE_SIZE, "%d\n", count);
}
static ssize_t applesmc_key_at_index_read_show(struct device *dev,
Looks good, thank you.
Reviewed-by: Henrik Rydberg
Henrik
Re: [PATCH v3 02/49] Input: introduce input_mt_report_slot_inactive
Hi Jiada, input_mt_report_slot_state() ignores the tool when the slot is closed. which has caused a bit of confusion. This patch introduces input_mt_report_slot_inactive() to report slot inactive state. replaces all input_mt_report_slot_state() with input_mt_report_slot_inactive() in case of close of slot. This patch looks very odd, I am afraid. When a driver needs to use input_mt functions, it first calls input_mt_init_slots() during setup. The MT state then remains in effect until the driver is destroyed. Thus, there is no valid case when input_mt_report_slot_state() would fail to execute the line input_event(dev, EV_ABS, ABS_MT_TRACKING_ID, -1) when active == false. What input_mt_report_slot_state() does do, however, is to ignore the event when no MT state has been set, which does happen for some drivers handling both normal and MT devices. Changing such a driver in the way you suggest would introduce new events in existing, working cases, and possibly break userspace. We should try very hard to avoid it. Thanks, Henrik
Re: [PATCH 0/2] Add Apple SPI keyboard and trackpad driver
Hi Ronald, This changeset adds a driver for the SPI keyboard and trackpad on recent MacBook's and MacBook Pro's. The driver has seen a fair amount of use over the last 2 years (basically anybody running linux on these machines), with only relatively small changes in the last year or so. For those interested, the driver development has been hosted at https://github.com/cb22/macbook12-spi-driver/ (as well as my clone at https://github.com/roadrunner2/macbook12-spi-driver/). The first patch is just a placeholder for now and is provided in case somebody wants to compile the driver while it's being reviewed here; the real patch has been submitted to dri-devel and is being discussed there, with the intent/hope that I can get an Ack and permission to merge it through the input subsystem tree here as part of this patch series. Great to see this upstream. The patch obviously has a lot in common with the previous keyboard and touchpad drivers; foremost this is a change of transport protocol and not functionality. That said, the code is compact and clear enough to make it hard to motivate any major effort to share more of existing code, at least initially. Barring detailed comments that are likely to produce new revisions, this looks like really good work. Thanks, Henrik
Re: [PATCH] [v3] HID: add support for Apple Magic Trackpad 2
Hi Sean, Gentle reminder, thank you! On Thu, Sep 20, 2018 at 4:13 PM Sean O'Brien wrote: USB device Vendor 05ac (Apple) Device 0265 (Magic Trackpad 2) Bluetooth device Vendor 004c (Apple) Device 0265 (Magic Trackpad 2) Add support for Apple Magic Trackpad 2 over USB and bluetooth, putting the device in multi-touch mode. Signed-off-by: Claudio Mettler Signed-off-by: Marek Wyborski Signed-off-by: Sean O'Brien --- drivers/hid/hid-ids.h| 1 + drivers/hid/hid-magicmouse.c | 149 +++ 2 files changed, 134 insertions(+), 16 deletions(-) This version looks good. Reviewed-by: Henrik Rydberg Thank you! Henrik
Re: [PATCH] [v3] HID: add support for Apple Magic Trackpad 2
Hi Sean, Gentle reminder, thank you! On Thu, Sep 20, 2018 at 4:13 PM Sean O'Brien wrote: USB device Vendor 05ac (Apple) Device 0265 (Magic Trackpad 2) Bluetooth device Vendor 004c (Apple) Device 0265 (Magic Trackpad 2) Add support for Apple Magic Trackpad 2 over USB and bluetooth, putting the device in multi-touch mode. Signed-off-by: Claudio Mettler Signed-off-by: Marek Wyborski Signed-off-by: Sean O'Brien --- drivers/hid/hid-ids.h| 1 + drivers/hid/hid-magicmouse.c | 149 +++ 2 files changed, 134 insertions(+), 16 deletions(-) This version looks good. Reviewed-by: Henrik Rydberg Thank you! Henrik
Re: [PATCH] [v2] HID: add support for Apple Magic Trackpad 2
Hi Sean,
Thanks for the driver. Looking good, but I do have some comments below.
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 79bdf0c7e351..d6d0b20cc015 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -88,9 +88,11 @@
> #define USB_DEVICE_ID_ANTON_TOUCH_PAD0x3101
>
> #define USB_VENDOR_ID_APPLE 0x05ac
> +#define BT_VENDOR_ID_APPLE 0x004c
> #define USB_DEVICE_ID_APPLE_MIGHTYMOUSE 0x0304
> #define USB_DEVICE_ID_APPLE_MAGICMOUSE 0x030d
> #define USB_DEVICE_ID_APPLE_MAGICTRACKPAD0x030e
> +#define USB_DEVICE_ID_APPLE_MAGICTRACKPAD2 0x0265
> #define USB_DEVICE_ID_APPLE_FOUNTAIN_ANSI0x020e
> #define USB_DEVICE_ID_APPLE_FOUNTAIN_ISO 0x020f
> #define USB_DEVICE_ID_APPLE_GEYSER_ANSI 0x0214
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index b454c4386157..7f14866ea3c7 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -54,6 +54,8 @@ module_param(report_undeciphered, bool, 0644);
> MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state
> field using a MSC_RAW event");
>
> #define TRACKPAD_REPORT_ID 0x28
> +#define TRACKPAD2_USB_REPORT_ID 0x02
> +#define TRACKPAD2_BT_REPORT_ID 0x31
> #define MOUSE_REPORT_ID0x29
> #define DOUBLE_REPORT_ID 0xf7
> /* These definitions are not precise, but they're close enough. (Bits
> @@ -91,6 +93,19 @@ MODULE_PARM_DESC(report_undeciphered, "Report undeciphered
> multi-touch state fie
> #define TRACKPAD_RES_Y \
> ((TRACKPAD_MAX_Y - TRACKPAD_MIN_Y) / (TRACKPAD_DIMENSION_Y / 100))
>
> +#define TRACKPAD2_DIMENSION_X (float)16000
> +#define TRACKPAD2_MIN_X -3678
> +#define TRACKPAD2_MAX_X 3934
> +#define TRACKPAD2_RES_X \
> + ((TRACKPAD2_MAX_X - TRACKPAD2_MIN_X) / (TRACKPAD2_DIMENSION_X / 100))
> +#define TRACKPAD2_DIMENSION_Y (float)11490
> +#define TRACKPAD2_MIN_Y -2478
> +#define TRACKPAD2_MAX_Y 2587
> +#define TRACKPAD2_RES_Y \
> + ((TRACKPAD2_MAX_Y - TRACKPAD2_MIN_Y) / (TRACKPAD2_DIMENSION_Y / 100))
> +
> +#define MAX_TOUCHES 16
> +
> /**
> * struct magicmouse_sc - Tracks Magic Mouse-specific data.
> * @input: Input device through which we report events.
> @@ -115,8 +130,8 @@ struct magicmouse_sc {
> short scroll_x;
> short scroll_y;
> u8 size;
> - } touches[16];
> - int tracking_ids[16];
> + } touches[MAX_TOUCHES];
> + int tracking_ids[MAX_TOUCHES];
> };
>
> static int magicmouse_firm_touch(struct magicmouse_sc *msc)
> @@ -183,6 +198,7 @@ static void magicmouse_emit_touch(struct magicmouse_sc
> *msc, int raw_id, u8 *tda
> {
> struct input_dev *input = msc->input;
> int id, x, y, size, orientation, touch_major, touch_minor, state, down;
> + int pressure = 0;
>
> if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
> @@ -194,7 +210,7 @@ static void magicmouse_emit_touch(struct magicmouse_sc
> *msc, int raw_id, u8 *tda
> touch_minor = tdata[4];
> state = tdata[7] & TOUCH_STATE_MASK;
> down = state != TOUCH_STATE_NONE;
> - } else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> + } else if (input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD) {
> id = (tdata[7] << 2 | tdata[6] >> 6) & 0xf;
> x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
> y = -((tdata[3] << 30 | tdata[2] << 22 | tdata[1] << 14) >> 19);
> @@ -204,6 +220,18 @@ static void magicmouse_emit_touch(struct magicmouse_sc
> *msc, int raw_id, u8 *tda
> touch_minor = tdata[5];
> state = tdata[8] & TOUCH_STATE_MASK;
> down = state != TOUCH_STATE_NONE;
> + } else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD2 */
The else clause is ill-suited for your specific addition, since any
odd device out there if they existed, would suddenly be treated as
something different. Better reverse the logic here.
> + id = tdata[8] & 0xf;
> + x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
> + y = -((tdata[3] << 30 | tdata[2] << 22 | tdata[1] << 14) >> 19);
> + size = tdata[6];
> + orientation = (tdata[8] >> 5) - 4;
> + touch_major = tdata[4];
> + touch_minor = tdata[5];
> + /* Prevent zero and low pressure values */
> + pressure = tdata[7] + 30;
Same question as Benjamin: what does + 30 stand for here?
> + state = tdata[3] & 0xC0;
> + down = state == 0x80;
> }
>
> /* Store tracking ID and other fields. */
> @@ -215,7 +243,8 @@ static void magicmouse_emit_touch(struct magicmouse_sc
> *msc, int raw_id, u8 *tda
> /* If requested, emulate a scroll wheel by detecting small
>* vertical touch motions.
>*/
> - if (emulate_scroll_wheel) {
> + if
Re: [PATCH] [v2] HID: add support for Apple Magic Trackpad 2
Hi Sean,
Thanks for the driver. Looking good, but I do have some comments below.
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 79bdf0c7e351..d6d0b20cc015 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -88,9 +88,11 @@
> #define USB_DEVICE_ID_ANTON_TOUCH_PAD0x3101
>
> #define USB_VENDOR_ID_APPLE 0x05ac
> +#define BT_VENDOR_ID_APPLE 0x004c
> #define USB_DEVICE_ID_APPLE_MIGHTYMOUSE 0x0304
> #define USB_DEVICE_ID_APPLE_MAGICMOUSE 0x030d
> #define USB_DEVICE_ID_APPLE_MAGICTRACKPAD0x030e
> +#define USB_DEVICE_ID_APPLE_MAGICTRACKPAD2 0x0265
> #define USB_DEVICE_ID_APPLE_FOUNTAIN_ANSI0x020e
> #define USB_DEVICE_ID_APPLE_FOUNTAIN_ISO 0x020f
> #define USB_DEVICE_ID_APPLE_GEYSER_ANSI 0x0214
> diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
> index b454c4386157..7f14866ea3c7 100644
> --- a/drivers/hid/hid-magicmouse.c
> +++ b/drivers/hid/hid-magicmouse.c
> @@ -54,6 +54,8 @@ module_param(report_undeciphered, bool, 0644);
> MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state
> field using a MSC_RAW event");
>
> #define TRACKPAD_REPORT_ID 0x28
> +#define TRACKPAD2_USB_REPORT_ID 0x02
> +#define TRACKPAD2_BT_REPORT_ID 0x31
> #define MOUSE_REPORT_ID0x29
> #define DOUBLE_REPORT_ID 0xf7
> /* These definitions are not precise, but they're close enough. (Bits
> @@ -91,6 +93,19 @@ MODULE_PARM_DESC(report_undeciphered, "Report undeciphered
> multi-touch state fie
> #define TRACKPAD_RES_Y \
> ((TRACKPAD_MAX_Y - TRACKPAD_MIN_Y) / (TRACKPAD_DIMENSION_Y / 100))
>
> +#define TRACKPAD2_DIMENSION_X (float)16000
> +#define TRACKPAD2_MIN_X -3678
> +#define TRACKPAD2_MAX_X 3934
> +#define TRACKPAD2_RES_X \
> + ((TRACKPAD2_MAX_X - TRACKPAD2_MIN_X) / (TRACKPAD2_DIMENSION_X / 100))
> +#define TRACKPAD2_DIMENSION_Y (float)11490
> +#define TRACKPAD2_MIN_Y -2478
> +#define TRACKPAD2_MAX_Y 2587
> +#define TRACKPAD2_RES_Y \
> + ((TRACKPAD2_MAX_Y - TRACKPAD2_MIN_Y) / (TRACKPAD2_DIMENSION_Y / 100))
> +
> +#define MAX_TOUCHES 16
> +
> /**
> * struct magicmouse_sc - Tracks Magic Mouse-specific data.
> * @input: Input device through which we report events.
> @@ -115,8 +130,8 @@ struct magicmouse_sc {
> short scroll_x;
> short scroll_y;
> u8 size;
> - } touches[16];
> - int tracking_ids[16];
> + } touches[MAX_TOUCHES];
> + int tracking_ids[MAX_TOUCHES];
> };
>
> static int magicmouse_firm_touch(struct magicmouse_sc *msc)
> @@ -183,6 +198,7 @@ static void magicmouse_emit_touch(struct magicmouse_sc
> *msc, int raw_id, u8 *tda
> {
> struct input_dev *input = msc->input;
> int id, x, y, size, orientation, touch_major, touch_minor, state, down;
> + int pressure = 0;
>
> if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
> id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
> @@ -194,7 +210,7 @@ static void magicmouse_emit_touch(struct magicmouse_sc
> *msc, int raw_id, u8 *tda
> touch_minor = tdata[4];
> state = tdata[7] & TOUCH_STATE_MASK;
> down = state != TOUCH_STATE_NONE;
> - } else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
> + } else if (input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD) {
> id = (tdata[7] << 2 | tdata[6] >> 6) & 0xf;
> x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
> y = -((tdata[3] << 30 | tdata[2] << 22 | tdata[1] << 14) >> 19);
> @@ -204,6 +220,18 @@ static void magicmouse_emit_touch(struct magicmouse_sc
> *msc, int raw_id, u8 *tda
> touch_minor = tdata[5];
> state = tdata[8] & TOUCH_STATE_MASK;
> down = state != TOUCH_STATE_NONE;
> + } else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD2 */
The else clause is ill-suited for your specific addition, since any
odd device out there if they existed, would suddenly be treated as
something different. Better reverse the logic here.
> + id = tdata[8] & 0xf;
> + x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
> + y = -((tdata[3] << 30 | tdata[2] << 22 | tdata[1] << 14) >> 19);
> + size = tdata[6];
> + orientation = (tdata[8] >> 5) - 4;
> + touch_major = tdata[4];
> + touch_minor = tdata[5];
> + /* Prevent zero and low pressure values */
> + pressure = tdata[7] + 30;
Same question as Benjamin: what does + 30 stand for here?
> + state = tdata[3] & 0xC0;
> + down = state == 0x80;
> }
>
> /* Store tracking ID and other fields. */
> @@ -215,7 +243,8 @@ static void magicmouse_emit_touch(struct magicmouse_sc
> *msc, int raw_id, u8 *tda
> /* If requested, emulate a scroll wheel by detecting small
>* vertical touch motions.
>*/
> - if (emulate_scroll_wheel) {
> + if
Re: [RFC/PATCH] Input: make input_report_slot_state() return boolean
On 06/05/2018 07:16 PM, Dmitry Torokhov wrote:
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
---
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 = >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);
Reviewed-by: Henrik Rydberg
Thanks, Dmitry.
Henrik
Re: [RFC/PATCH] Input: make input_report_slot_state() return boolean
On 06/05/2018 07:16 PM, Dmitry Torokhov wrote:
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
---
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 = >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);
Reviewed-by: Henrik Rydberg
Thanks, Dmitry.
Henrik
Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
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
Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
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
Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
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
Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
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
Re: [PATCH v3] input: bcm5974 - Add driver for Apple Magic Trackpad 2
Hi Stephan, I would like to have Touchpad 2 properly supported. You will find proper prior support for Magic Trackpads in drivers/hid/hid-magicmouse.c. Henrik
Re: [PATCH v3] input: bcm5974 - Add driver for Apple Magic Trackpad 2
Hi Stephan, I would like to have Touchpad 2 properly supported. You will find proper prior support for Magic Trackpads in drivers/hid/hid-magicmouse.c. Henrik
Re: [PATCH v5] HID: hid-multitouch: support fine-grain orientation reporting
On 10/12/2017 08:21 AM, Wei-Ning Huang wrote: From: Wei-Ning Huang <wnhu...@chromium.org> The current hid-multitouch driver only allow the report of two orientations, vertical and horizontal. We use the Azimuth orientation usage 0x3F under the Digitizer usage page to report orientation if the device supports it. Changelog: v1 -> v2: - Fix commit message. - Remove resolution reporting for ABS_MT_ORIENTATION. v2 -> v3: - Fix commit message. v3 -> v4: - Fix ABS_MT_ORIENTATION ABS param range. - Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already set by ABS_DG_AZIMUTH. v4 -> v5: - Improve multi-touch-protocol.rst documentation. Signed-off-by: Wei-Ning Huang <wnhu...@chromium.org> Signed-off-by: Wei-Ning Huang <wnhu...@google.com> Reviewed-by: Dmitry Torokhov <d...@chromium.org> --- Documentation/input/multi-touch-protocol.rst | 9 ++--- drivers/hid/hid-multitouch.c | 52 +--- include/linux/hid.h | 1 + 3 files changed, 54 insertions(+), 8 deletions(-) Reviewed-by: Henrik Rydberg <rydb...@bitmath.org> Thank you, Wei-Ning, and sorry for the delay. Dmitry, did you plan to add this to your pull request already? Henrik
Re: [PATCH v5] HID: hid-multitouch: support fine-grain orientation reporting
On 10/12/2017 08:21 AM, Wei-Ning Huang wrote: From: Wei-Ning Huang The current hid-multitouch driver only allow the report of two orientations, vertical and horizontal. We use the Azimuth orientation usage 0x3F under the Digitizer usage page to report orientation if the device supports it. Changelog: v1 -> v2: - Fix commit message. - Remove resolution reporting for ABS_MT_ORIENTATION. v2 -> v3: - Fix commit message. v3 -> v4: - Fix ABS_MT_ORIENTATION ABS param range. - Don't set ABS_MT_ORIENTATION in ABS_DG_HEIGHT when it is already set by ABS_DG_AZIMUTH. v4 -> v5: - Improve multi-touch-protocol.rst documentation. Signed-off-by: Wei-Ning Huang Signed-off-by: Wei-Ning Huang Reviewed-by: Dmitry Torokhov --- Documentation/input/multi-touch-protocol.rst | 9 ++--- drivers/hid/hid-multitouch.c | 52 +--- include/linux/hid.h | 1 + 3 files changed, 54 insertions(+), 8 deletions(-) Reviewed-by: Henrik Rydberg Thank you, Wei-Ning, and sorry for the delay. Dmitry, did you plan to add this to your pull request already? Henrik
Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting
> > > , but I'd go for fixing the documentation. And re-reading it, it's not > > > clear that the doc tells us to have [0,90]. It mentions negative values > > > and out of ranges too, so we might just as well simply clarify that we > > > rather have [-90,90], with 0 being "north". > > > > ... I'd like the documentation fix to go in together in one go with this > > patch if possible. > > > > Sounds like a plan. How about this patch? Henrik --- >From b14f92066dfab3f8a255ec7b5a30cb1a864dc62f Mon Sep 17 00:00:00 2001 From: Henrik Rydberg <rydb...@bitmath.org> Date: Wed, 11 Oct 2017 22:41:39 +0200 Subject: [PATCH] Input: docs - clarify the usage of ABS_MT_ORIENTATION As more drivers start to support touch orientation, clarify how the value range should be set to match the expected behavior in userland. Signed-off-by: Henrik Rydberg <rydb...@bitmath.org> --- Documentation/input/multi-touch-protocol.rst | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Documentation/input/multi-touch-protocol.rst b/Documentation/input/multi-touch-protocol.rst index 8035868..a0c5c03 100644 --- a/Documentation/input/multi-touch-protocol.rst +++ b/Documentation/input/multi-touch-protocol.rst @@ -269,15 +269,17 @@ ABS_MT_ORIENTATION The orientation of the touching ellipse. The value should describe a signed quarter of a revolution clockwise around the touch center. The signed value range is arbitrary, but zero should be returned for an ellipse aligned with -the Y axis of the surface, a negative value when the ellipse is turned to -the left, and a positive value when the ellipse is turned to the -right. When completely aligned with the X axis, the range max should be -returned. +the Y axis of the surface (north). A negative value should be returned when +the ellipse is turned to the left (west), with the smallest value reported +when aligned with the negative X axis. The largest value should be returned +when aligned with the positive X axis. + +The value range should be specified as [-range_max, range_max]. Touch ellipsis are symmetrical by default. For devices capable of true 360 -degree orientation, the reported orientation must exceed the range max to +degree orientation, the reported orientation will exceed range_max, in order to indicate more than a quarter of a revolution. For an upside-down finger, -range max * 2 should be returned. ++- 2 * range_max should be returned. Orientation can be omitted if the touch area is circular, or if the information is not available in the kernel driver. Partial orientation -- 2.14.1
Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting
> > > , but I'd go for fixing the documentation. And re-reading it, it's not > > > clear that the doc tells us to have [0,90]. It mentions negative values > > > and out of ranges too, so we might just as well simply clarify that we > > > rather have [-90,90], with 0 being "north". > > > > ... I'd like the documentation fix to go in together in one go with this > > patch if possible. > > > > Sounds like a plan. How about this patch? Henrik --- >From b14f92066dfab3f8a255ec7b5a30cb1a864dc62f Mon Sep 17 00:00:00 2001 From: Henrik Rydberg Date: Wed, 11 Oct 2017 22:41:39 +0200 Subject: [PATCH] Input: docs - clarify the usage of ABS_MT_ORIENTATION As more drivers start to support touch orientation, clarify how the value range should be set to match the expected behavior in userland. Signed-off-by: Henrik Rydberg --- Documentation/input/multi-touch-protocol.rst | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Documentation/input/multi-touch-protocol.rst b/Documentation/input/multi-touch-protocol.rst index 8035868..a0c5c03 100644 --- a/Documentation/input/multi-touch-protocol.rst +++ b/Documentation/input/multi-touch-protocol.rst @@ -269,15 +269,17 @@ ABS_MT_ORIENTATION The orientation of the touching ellipse. The value should describe a signed quarter of a revolution clockwise around the touch center. The signed value range is arbitrary, but zero should be returned for an ellipse aligned with -the Y axis of the surface, a negative value when the ellipse is turned to -the left, and a positive value when the ellipse is turned to the -right. When completely aligned with the X axis, the range max should be -returned. +the Y axis of the surface (north). A negative value should be returned when +the ellipse is turned to the left (west), with the smallest value reported +when aligned with the negative X axis. The largest value should be returned +when aligned with the positive X axis. + +The value range should be specified as [-range_max, range_max]. Touch ellipsis are symmetrical by default. For devices capable of true 360 -degree orientation, the reported orientation must exceed the range max to +degree orientation, the reported orientation will exceed range_max, in order to indicate more than a quarter of a revolution. For an upside-down finger, -range max * 2 should be returned. ++- 2 * range_max should be returned. Orientation can be omitted if the touch area is circular, or if the information is not available in the kernel driver. Partial orientation -- 2.14.1
Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting
mt_complete_slot(struct mt_device *td, struct
> input_dev *input)
> input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
> input_event(input, EV_ABS, ABS_MT_DISTANCE,
> !s->touch_state);
> - input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
> + input_event(input, EV_ABS, ABS_MT_ORIENTATION,
> + orientation);
> input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
> input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
> input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
> @@ -789,6 +817,22 @@ static void mt_process_mt_event(struct hid_device *hid,
> struct hid_field *field,
> break;
> case HID_DG_CONTACTCOUNT:
> break;
> + case HID_DG_AZIMUTH:
> + /*
> + * Azimuth is counter-clockwise and ranges from [0, MAX)
> + * (a full revolution). Convert it to clockwise ranging
> + * [-MAX/2, MAX/2].
> + *
> + * Note that ABS_MT_ORIENTATION require us to report
> + * the limit of [-MAX/4, MAX/4], but the value can go
> + * out of range to [-MAX/2, MAX/2] to report an upside
> + * down ellipsis.
> + */
> + if (value > field->logical_maximum / 2)
> + value -= field->logical_maximum;
> + td->curdata.a = -value;
> + td->curdata.has_azimuth = true;
> + break;
> case HID_DG_TOUCH:
> /* do nothing */
> break;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index ab05a86269dc..d81b9b6fd83a 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -281,6 +281,7 @@ struct hid_item {
>
> #define HID_DG_DEVICECONFIG 0x000d000e
> #define HID_DG_DEVICESETTINGS0x000d0023
> +#define HID_DG_AZIMUTH 0x000d003f
> #define HID_DG_CONFIDENCE0x000d0047
> #define HID_DG_WIDTH 0x000d0048
> #define HID_DG_HEIGHT0x000d0049
> --
> 2.12.2
>
Acked-by: Henrik Rydberg <rydb...@bitmath.org>
This version looks good - thank you Wei-Ning, thank you Benjamin.
Henrik
Re: [PATCH v4] HID: hid-multitouch: support fine-grain orientation reporting
input_event(input, EV_ABS, ABS_MT_TOOL_Y, s->cy);
> input_event(input, EV_ABS, ABS_MT_DISTANCE,
> !s->touch_state);
> - input_event(input, EV_ABS, ABS_MT_ORIENTATION, wide);
> + input_event(input, EV_ABS, ABS_MT_ORIENTATION,
> + orientation);
> input_event(input, EV_ABS, ABS_MT_PRESSURE, s->p);
> input_event(input, EV_ABS, ABS_MT_TOUCH_MAJOR, major);
> input_event(input, EV_ABS, ABS_MT_TOUCH_MINOR, minor);
> @@ -789,6 +817,22 @@ static void mt_process_mt_event(struct hid_device *hid,
> struct hid_field *field,
> break;
> case HID_DG_CONTACTCOUNT:
> break;
> + case HID_DG_AZIMUTH:
> + /*
> + * Azimuth is counter-clockwise and ranges from [0, MAX)
> + * (a full revolution). Convert it to clockwise ranging
> + * [-MAX/2, MAX/2].
> + *
> + * Note that ABS_MT_ORIENTATION require us to report
> + * the limit of [-MAX/4, MAX/4], but the value can go
> + * out of range to [-MAX/2, MAX/2] to report an upside
> + * down ellipsis.
> + */
> + if (value > field->logical_maximum / 2)
> + value -= field->logical_maximum;
> + td->curdata.a = -value;
> + td->curdata.has_azimuth = true;
> + break;
> case HID_DG_TOUCH:
> /* do nothing */
> break;
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index ab05a86269dc..d81b9b6fd83a 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -281,6 +281,7 @@ struct hid_item {
>
> #define HID_DG_DEVICECONFIG 0x000d000e
> #define HID_DG_DEVICESETTINGS0x000d0023
> +#define HID_DG_AZIMUTH 0x000d003f
> #define HID_DG_CONFIDENCE0x000d0047
> #define HID_DG_WIDTH 0x000d0048
> #define HID_DG_HEIGHT0x000d0049
> --
> 2.12.2
>
Acked-by: Henrik Rydberg
This version looks good - thank you Wei-Ning, thank you Benjamin.
Henrik
Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
Hi Dmitry, The meaning of confidence is literally "contact is too large to be a finger", so it is not touch state, but really tool identity. We do allow tool identity to change over time. What I am arguing is rather that since "palm" is a property, just like contact size, there should be no need to confuse that property with the touch state, which is, as you state, what happens in userland when the tool type is modified. Using a different event for the palm property ought to remove that confusion. The additional state is simply because we have never updated the tool type on release events and userspace is not expecting it and is likely to ignore any data in the slot that is accompanied with ABS_TRACKING_ID == -1. So we synthesize an extra event to have distinct tool change and release. We update all other properties of a contact freely at release, so logically there is no good reason to treat palm, the binary version of max contact size, differently. Mostly because with BTN_TOOL_PALM we are not able to decide which contact is turning into palm. Also, other drivers (RMI) use MT_TOOL_PALM and I would like to report palm state in unified way. Precedent certainly matters, but in this case, I think the modification promises problems down the road. I would rather suggest to add a new binary palm property, with the precise meaning "contact size = max contact size", and take it from there. I dont mind writing a patch for it if you agree. Henrik
Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
Hi Dmitry, The meaning of confidence is literally "contact is too large to be a finger", so it is not touch state, but really tool identity. We do allow tool identity to change over time. What I am arguing is rather that since "palm" is a property, just like contact size, there should be no need to confuse that property with the touch state, which is, as you state, what happens in userland when the tool type is modified. Using a different event for the palm property ought to remove that confusion. The additional state is simply because we have never updated the tool type on release events and userspace is not expecting it and is likely to ignore any data in the slot that is accompanied with ABS_TRACKING_ID == -1. So we synthesize an extra event to have distinct tool change and release. We update all other properties of a contact freely at release, so logically there is no good reason to treat palm, the binary version of max contact size, differently. Mostly because with BTN_TOOL_PALM we are not able to decide which contact is turning into palm. Also, other drivers (RMI) use MT_TOOL_PALM and I would like to report palm state in unified way. Precedent certainly matters, but in this case, I think the modification promises problems down the road. I would rather suggest to add a new binary palm property, with the precise meaning "contact size = max contact size", and take it from there. I dont mind writing a patch for it if you agree. Henrik
Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
Hi Dmitry, On 08/11/2017 02:44 AM, Dmitry Torokhov 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). Changing the tool identity to signal the tool property of low confidence does not seem quite right to me. Using MT_TOOL_PALM forces a semantic distinction between tool identity and touch state, which userland seems unprepared for. The additional kernel state needed to make it work raises the question if more considerations will turn up over with time. Why not add a property event, like BTN_TOOL_PALM, instead? In other words, modifying the definition of "active" as you propose, but then use a BTN_TOOL_PALM property to signal "s->confidence_state"? It perhaps creates a different oddity for applications unaware of palm, but AFAICT, it would not complicate the notion of touch state. Or? Henrik
Re: [PATCH 1/2] HID: multitouch: report MT_TOOL_PALM for non-confident touches
Hi Dmitry, On 08/11/2017 02:44 AM, Dmitry Torokhov 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). Changing the tool identity to signal the tool property of low confidence does not seem quite right to me. Using MT_TOOL_PALM forces a semantic distinction between tool identity and touch state, which userland seems unprepared for. The additional kernel state needed to make it work raises the question if more considerations will turn up over with time. Why not add a property event, like BTN_TOOL_PALM, instead? In other words, modifying the definition of "active" as you propose, but then use a BTN_TOOL_PALM property to signal "s->confidence_state"? It perhaps creates a different oddity for applications unaware of palm, but AFAICT, it would not complicate the notion of touch state. Or? Henrik
Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data
Hi Aniroop, >> I am not sure what the urgency is. It is more of a theoretical problem >> ans so far the proposed solutions were actually introducing more >> problems than they were solving. >> >> I am sorry, bit this particular topic is not a priority for me. >> > > There is no hurry at all. :-) As you know request is made a long time ago, > so I am only very curious to complete it. This kind of patch is not liked by any maintainer, because it does not solve any immediate problem, but instead may create one. If such a simple patch takes three of four tries to look right, it only adds to the perception that the code is best left alone. I think the solution at this stage is to say no to this patch. If there is ever a driver for which the input_estimate_events_per_packet() function returns less than the actual maximum number of events per frame, this issue can be revisited and resolved in a number of different ways. Sorry, and thanks for your work. Henrik
Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data
Hi Aniroop, >> I am not sure what the urgency is. It is more of a theoretical problem >> ans so far the proposed solutions were actually introducing more >> problems than they were solving. >> >> I am sorry, bit this particular topic is not a priority for me. >> > > There is no hurry at all. :-) As you know request is made a long time ago, > so I am only very curious to complete it. This kind of patch is not liked by any maintainer, because it does not solve any immediate problem, but instead may create one. If such a simple patch takes three of four tries to look right, it only adds to the perception that the code is best left alone. I think the solution at this stage is to say no to this patch. If there is ever a driver for which the input_estimate_events_per_packet() function returns less than the actual maximum number of events per frame, this issue can be revisited and resolved in a number of different ways. Sorry, and thanks for your work. Henrik
Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data
Hi Dmitry,
>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> index 8806059..262ef77 100644
>> --- a/drivers/input/input.c
>> +++ b/drivers/input/input.c
>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>> if (dev->num_vals >= 2)
>> input_pass_values(dev, dev->vals, dev->num_vals);
>> dev->num_vals = 0;
>> - } else if (dev->num_vals >= dev->max_vals - 2) {
>> - dev->vals[dev->num_vals++] = input_value_sync;
>> + } else if (dev->num_vals >= dev->max_vals - 1) {
>> input_pass_values(dev, dev->vals, dev->num_vals);
>> dev->num_vals = 0;
>> }
>
> This makes sense to me. Henrik?
I went through the commits that made these changes, and I cannot see any strong
reason to keep it. However, this code path only triggers if no SYN events are
seen, as in a driver that fails to emit them and consequently fills up the
buffer. In other words, this change would only affect a device that is already,
to some degree, broken.
So, the question to Aniroop is: do you see this problem in practise, and in that
case, for what driver?
Henrik
Re: [PATCH] Input: Do not add SYN_REPORT in between a single packet data
Hi Dmitry,
>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> index 8806059..262ef77 100644
>> --- a/drivers/input/input.c
>> +++ b/drivers/input/input.c
>> @@ -401,8 +401,7 @@ static void input_handle_event(struct input_dev *dev,
>> if (dev->num_vals >= 2)
>> input_pass_values(dev, dev->vals, dev->num_vals);
>> dev->num_vals = 0;
>> - } else if (dev->num_vals >= dev->max_vals - 2) {
>> - dev->vals[dev->num_vals++] = input_value_sync;
>> + } else if (dev->num_vals >= dev->max_vals - 1) {
>> input_pass_values(dev, dev->vals, dev->num_vals);
>> dev->num_vals = 0;
>> }
>
> This makes sense to me. Henrik?
I went through the commits that made these changes, and I cannot see any strong
reason to keep it. However, this code path only triggers if no SYN events are
seen, as in a driver that fails to emit them and consequently fills up the
buffer. In other words, this change would only affect a device that is already,
to some degree, broken.
So, the question to Aniroop is: do you see this problem in practise, and in that
case, for what driver?
Henrik
[PATCH 2/3] HID: apple: Add support for the 2015 Macbook Pro
This patch adds keyboard support for MacbookPro12,1 as WELLSPRING9
(0x0272, 0x0273, 0x0274). The touchpad is handled in a separate
bcm5974 patch, as usual.
Tested-by: John Horan
Tested-by: Jochen Radmacher
Tested-by: Yang Hongyang
Tested-by: Yen-Chin, Lee
Tested-by: George Hilios
Tested-by: Janez Urevc
Signed-off-by: Henrik Rydberg
---
drivers/hid/hid-apple.c | 6 ++
drivers/hid/hid-core.c | 6 ++
drivers/hid/hid-ids.h | 3 +++
3 files changed, 15 insertions(+)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index f822fd2..884d82f 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -546,6 +546,12 @@ static const struct hid_device_id apple_devices[] = {
.driver_data = APPLE_HAS_FN | APPLE_ISO_KEYBOARD },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_JIS),
.driver_data = APPLE_HAS_FN | APPLE_RDESC_JIS },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI),
+ .driver_data = APPLE_HAS_FN },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ISO),
+ .driver_data = APPLE_HAS_FN | APPLE_ISO_KEYBOARD },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_JIS),
+ .driver_data = APPLE_HAS_FN | APPLE_RDESC_JIS },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_ANSI),
.driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_ISO),
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 157c627..e6fce23 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1782,6 +1782,9 @@ static const struct hid_device_id
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_ISO) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_JIS) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ISO) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_JIS) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_ANSI) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_ISO) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_JIS) },
@@ -2463,6 +2466,9 @@ static const struct hid_device_id hid_mouse_ignore_list[]
= {
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_ISO) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_JIS) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ISO) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_JIS) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
{ }
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index b04b082..b3b225b 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -142,6 +142,9 @@
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI 0x0290
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ISO0x0291
#define USB_DEVICE_ID_APPLE_WELLSPRING8_JIS0x0292
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI 0x0272
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_ISO0x0273
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_JIS0x0274
#define USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY 0x030a
#define USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY0x030b
#define USB_DEVICE_ID_APPLE_IRCONTROL 0x8240
--
2.4.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/3] Input: bcm5974 - Prepare for a new trackpad generation
With the advent of the Macbook Pro 12, we see a new generation of
trackpads, capable of force sensoring and haptic feedback.
This patch prepares for the new device by adding configuration data
for the code paths that would otherwise look different.
Tested-by: John Horan
Tested-by: Jochen Radmacher
Tested-by: Yang Hongyang
Tested-by: Yen-Chin, Lee
Tested-by: George Hilios
Tested-by: Janez Urevc
Signed-off-by: Henrik Rydberg
---
drivers/input/mouse/bcm5974.c | 132 ++
1 file changed, 81 insertions(+), 51 deletions(-)
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index b10709f..a596b9b 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -184,17 +184,37 @@ enum tp_type {
};
/* trackpad finger data offsets, le16-aligned */
-#define FINGER_TYPE1 (13 * sizeof(__le16))
-#define FINGER_TYPE2 (15 * sizeof(__le16))
-#define FINGER_TYPE3 (19 * sizeof(__le16))
+#define HEADER_TYPE1 (13 * sizeof(__le16))
+#define HEADER_TYPE2 (15 * sizeof(__le16))
+#define HEADER_TYPE3 (19 * sizeof(__le16))
/* trackpad button data offsets */
+#define BUTTON_TYPE1 0
#define BUTTON_TYPE2 15
#define BUTTON_TYPE3 23
/* list of device capability bits */
#define HAS_INTEGRATED_BUTTON 1
+/* trackpad finger data block size */
+#define FSIZE_TYPE1(14 * sizeof(__le16))
+#define FSIZE_TYPE2(14 * sizeof(__le16))
+#define FSIZE_TYPE3(14 * sizeof(__le16))
+
+/* offset from header to finger struct */
+#define DELTA_TYPE1(0 * sizeof(__le16))
+#define DELTA_TYPE2(0 * sizeof(__le16))
+#define DELTA_TYPE3(0 * sizeof(__le16))
+
+/* usb control message mode switch data */
+#define USBMSG_TYPE1 8, 0x300, 0, 0, 0x1, 0x8
+#define USBMSG_TYPE2 8, 0x300, 0, 0, 0x1, 0x8
+#define USBMSG_TYPE3 8, 0x300, 0, 0, 0x1, 0x8
+
+/* Wellspring initialization constants */
+#define BCM5974_WELLSPRING_MODE_READ_REQUEST_ID1
+#define BCM5974_WELLSPRING_MODE_WRITE_REQUEST_ID 9
+
/* trackpad finger structure, le16-aligned */
struct tp_finger {
__le16 origin; /* zero when switching track finger */
@@ -213,8 +233,6 @@ struct tp_finger {
/* trackpad finger data size, empirically at least ten fingers */
#define MAX_FINGERS16
-#define SIZEOF_FINGER sizeof(struct tp_finger)
-#define SIZEOF_ALL_FINGERS (MAX_FINGERS * SIZEOF_FINGER)
#define MAX_FINGER_ORIENTATION 16384
/* device-specific parameters */
@@ -232,8 +250,17 @@ struct bcm5974_config {
int bt_datalen; /* data length of the button interface */
int tp_ep; /* the endpoint of the trackpad interface */
enum tp_type tp_type; /* type of trackpad interface */
- int tp_offset; /* offset to trackpad finger data */
+ int tp_header; /* bytes in header block */
int tp_datalen; /* data length of the trackpad interface */
+ int tp_button; /* offset to button data */
+ int tp_fsize; /* bytes in single finger block */
+ int tp_delta; /* offset from header to finger struct */
+ int um_size;/* usb control message length */
+ int um_req_val; /* usb control message value */
+ int um_req_idx; /* usb control message index */
+ int um_switch_idx; /* usb control message mode switch index */
+ int um_switch_on; /* usb control message mode switch on */
+ int um_switch_off; /* usb control message mode switch off */
struct bcm5974_param p; /* finger pressure limits */
struct bcm5974_param w; /* finger width limits */
struct bcm5974_param x; /* horizontal limits */
@@ -259,6 +286,24 @@ struct bcm5974 {
int slots[MAX_FINGERS]; /* slot assignments */
};
+/* trackpad finger block data, le16-aligned */
+static const struct tp_finger *get_tp_finger(const struct bcm5974 *dev, int i)
+{
+ const struct bcm5974_config *c = >cfg;
+ u8 *f_base = dev->tp_data + c->tp_header + c->tp_delta;
+
+ return (const struct tp_finger *)(f_base + i * c->tp_fsize);
+}
+
+#define DATAFORMAT(type) \
+ type, \
+ HEADER_##type, \
+ HEADER_##type + (MAX_FINGERS) * (FSIZE_##type), \
+ BUTTON_##type, \
+ FSIZE_##type, \
+ DELTA_##type, \
+ USBMSG_##type
+
/* logical signal quality */
#define SN_PRESSURE45 /* pressure signal-to-noise ratio */
#define SN_WIDTH 25 /* width signal-to-noise ratio */
@@ -273,7 +318,7
[PATCH 3/3] Input: bcm597 - Add support for the 2015 Macbook Pro
From: John Horan
Add support for the MacBookPro12,1 model. This patch needs to be
applied together with the accompanied HID patch, as usual.
Tested-by: John Horan
Tested-by: Jochen Radmacher
Tested-by: Yang Hongyang
Tested-by: Yen-Chin, Lee
Tested-by: George Hilios
Tested-by: Janez Urevc
Signed-off-by: Henrik Rydberg
---
drivers/input/mouse/bcm5974.c | 33 +++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index a596b9b..30e3442 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -2,6 +2,7 @@
* Apple USB BCM5974 (Macbook Air and Penryn Macbook Pro) multitouch driver
*
* Copyright (C) 2008 Henrik Rydberg (rydb...@euromail.se)
+ * Copyright (C) 2015 John Horan (knas...@gmail.com)
*
* The USB initialization and package decoding was made by
* Scott Shawcroft as part of the touchd user-space driver project:
@@ -91,6 +92,10 @@
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI 0x0290
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ISO0x0291
#define USB_DEVICE_ID_APPLE_WELLSPRING8_JIS0x0292
+/* MacbookPro12,1 (2015) */
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI 0x0272
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_ISO0x0273
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_JIS0x0274
#define BCM5974_DEVICE(prod) { \
.match_flags = (USB_DEVICE_ID_MATCH_DEVICE |\
@@ -152,6 +157,10 @@ static const struct usb_device_id bcm5974_table[] = {
BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI),
BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING8_ISO),
BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING8_JIS),
+ /* MacbookPro12,1 */
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI),
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING9_ISO),
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING9_JIS),
/* Terminating entry */
{}
};
@@ -180,18 +189,21 @@ struct bt_data {
enum tp_type {
TYPE1, /* plain trackpad */
TYPE2, /* button integrated in trackpad */
- TYPE3 /* additional header fields since June 2013 */
+ TYPE3, /* additional header fields since June 2013 */
+ TYPE4 /* additional header field for pressure data */
};
/* trackpad finger data offsets, le16-aligned */
#define HEADER_TYPE1 (13 * sizeof(__le16))
#define HEADER_TYPE2 (15 * sizeof(__le16))
#define HEADER_TYPE3 (19 * sizeof(__le16))
+#define HEADER_TYPE4 (23 * sizeof(__le16))
/* trackpad button data offsets */
#define BUTTON_TYPE1 0
#define BUTTON_TYPE2 15
#define BUTTON_TYPE3 23
+#define BUTTON_TYPE4 31
/* list of device capability bits */
#define HAS_INTEGRATED_BUTTON 1
@@ -200,16 +212,19 @@ enum tp_type {
#define FSIZE_TYPE1(14 * sizeof(__le16))
#define FSIZE_TYPE2(14 * sizeof(__le16))
#define FSIZE_TYPE3(14 * sizeof(__le16))
+#define FSIZE_TYPE4(15 * sizeof(__le16))
/* offset from header to finger struct */
#define DELTA_TYPE1(0 * sizeof(__le16))
#define DELTA_TYPE2(0 * sizeof(__le16))
#define DELTA_TYPE3(0 * sizeof(__le16))
+#define DELTA_TYPE4(1 * sizeof(__le16))
/* usb control message mode switch data */
#define USBMSG_TYPE1 8, 0x300, 0, 0, 0x1, 0x8
#define USBMSG_TYPE2 8, 0x300, 0, 0, 0x1, 0x8
#define USBMSG_TYPE3 8, 0x300, 0, 0, 0x1, 0x8
+#define USBMSG_TYPE4 2, 0x302, 2, 1, 0x1, 0x0
/* Wellspring initialization constants */
#define BCM5974_WELLSPRING_MODE_READ_REQUEST_ID1
@@ -227,7 +242,8 @@ struct tp_finger {
__le16 orientation; /* 16384 when point, else 15 bit angle */
__le16 touch_major; /* touch area, major axis */
__le16 touch_minor; /* touch area, minor axis */
- __le16 unused[3]; /* zeros */
+ __le16 unused[2]; /* zeros */
+ __le16 pressure;/* pressure on forcetouch touchpad */
__le16 multi; /* one finger: varies, more fingers: constant */
} __attribute__((packed,aligned(2)));
@@ -468,6 +484,19 @@ static const struct bcm5974_config bcm5974_config_table[]
= {
{ SN_COORD, -150, 6600 },
{ SN_ORIENT, -MAX_FINGER_ORIENTATION, MAX_FINGER_ORIENTATION }
},
+ {
+ USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI,
+ USB_DEVICE_ID_APPLE_WELLSPRING9_ISO,
+ USB_DEVICE_ID_APPLE_WELLSPRING9_JIS,
+ HAS_INTEGRATED_BUTTON,
+ 0, sizeof(struct bt_data),
+ 0x83, DATAFORMAT(TYPE4),
+ { SN_PRESSURE, 0, 300 },
+ { SN_WIDTH, 0, 2048 },
+ { SN_COORD, -4828, 5345
[PATCH 3/3] Input: bcm597 - Add support for the 2015 Macbook Pro
From: John Horan knas...@gmail.com
Add support for the MacBookPro12,1 model. This patch needs to be
applied together with the accompanied HID patch, as usual.
Tested-by: John Horan knas...@gmail.com
Tested-by: Jochen Radmacher jradmac...@gmx.de
Tested-by: Yang Hongyang bur...@gmail.com
Tested-by: Yen-Chin, Lee coldnew...@gmail.com
Tested-by: George Hilios ghil...@gmail.com
Tested-by: Janez Urevc ja...@janezurevc.name
Signed-off-by: Henrik Rydberg rydb...@bitmath.org
---
drivers/input/mouse/bcm5974.c | 33 +++--
1 file changed, 31 insertions(+), 2 deletions(-)
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index a596b9b..30e3442 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -2,6 +2,7 @@
* Apple USB BCM5974 (Macbook Air and Penryn Macbook Pro) multitouch driver
*
* Copyright (C) 2008 Henrik Rydberg (rydb...@euromail.se)
+ * Copyright (C) 2015 John Horan (knas...@gmail.com)
*
* The USB initialization and package decoding was made by
* Scott Shawcroft as part of the touchd user-space driver project:
@@ -91,6 +92,10 @@
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI 0x0290
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ISO0x0291
#define USB_DEVICE_ID_APPLE_WELLSPRING8_JIS0x0292
+/* MacbookPro12,1 (2015) */
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI 0x0272
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_ISO0x0273
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_JIS0x0274
#define BCM5974_DEVICE(prod) { \
.match_flags = (USB_DEVICE_ID_MATCH_DEVICE |\
@@ -152,6 +157,10 @@ static const struct usb_device_id bcm5974_table[] = {
BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI),
BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING8_ISO),
BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING8_JIS),
+ /* MacbookPro12,1 */
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI),
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING9_ISO),
+ BCM5974_DEVICE(USB_DEVICE_ID_APPLE_WELLSPRING9_JIS),
/* Terminating entry */
{}
};
@@ -180,18 +189,21 @@ struct bt_data {
enum tp_type {
TYPE1, /* plain trackpad */
TYPE2, /* button integrated in trackpad */
- TYPE3 /* additional header fields since June 2013 */
+ TYPE3, /* additional header fields since June 2013 */
+ TYPE4 /* additional header field for pressure data */
};
/* trackpad finger data offsets, le16-aligned */
#define HEADER_TYPE1 (13 * sizeof(__le16))
#define HEADER_TYPE2 (15 * sizeof(__le16))
#define HEADER_TYPE3 (19 * sizeof(__le16))
+#define HEADER_TYPE4 (23 * sizeof(__le16))
/* trackpad button data offsets */
#define BUTTON_TYPE1 0
#define BUTTON_TYPE2 15
#define BUTTON_TYPE3 23
+#define BUTTON_TYPE4 31
/* list of device capability bits */
#define HAS_INTEGRATED_BUTTON 1
@@ -200,16 +212,19 @@ enum tp_type {
#define FSIZE_TYPE1(14 * sizeof(__le16))
#define FSIZE_TYPE2(14 * sizeof(__le16))
#define FSIZE_TYPE3(14 * sizeof(__le16))
+#define FSIZE_TYPE4(15 * sizeof(__le16))
/* offset from header to finger struct */
#define DELTA_TYPE1(0 * sizeof(__le16))
#define DELTA_TYPE2(0 * sizeof(__le16))
#define DELTA_TYPE3(0 * sizeof(__le16))
+#define DELTA_TYPE4(1 * sizeof(__le16))
/* usb control message mode switch data */
#define USBMSG_TYPE1 8, 0x300, 0, 0, 0x1, 0x8
#define USBMSG_TYPE2 8, 0x300, 0, 0, 0x1, 0x8
#define USBMSG_TYPE3 8, 0x300, 0, 0, 0x1, 0x8
+#define USBMSG_TYPE4 2, 0x302, 2, 1, 0x1, 0x0
/* Wellspring initialization constants */
#define BCM5974_WELLSPRING_MODE_READ_REQUEST_ID1
@@ -227,7 +242,8 @@ struct tp_finger {
__le16 orientation; /* 16384 when point, else 15 bit angle */
__le16 touch_major; /* touch area, major axis */
__le16 touch_minor; /* touch area, minor axis */
- __le16 unused[3]; /* zeros */
+ __le16 unused[2]; /* zeros */
+ __le16 pressure;/* pressure on forcetouch touchpad */
__le16 multi; /* one finger: varies, more fingers: constant */
} __attribute__((packed,aligned(2)));
@@ -468,6 +484,19 @@ static const struct bcm5974_config bcm5974_config_table[]
= {
{ SN_COORD, -150, 6600 },
{ SN_ORIENT, -MAX_FINGER_ORIENTATION, MAX_FINGER_ORIENTATION }
},
+ {
+ USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI,
+ USB_DEVICE_ID_APPLE_WELLSPRING9_ISO,
+ USB_DEVICE_ID_APPLE_WELLSPRING9_JIS,
+ HAS_INTEGRATED_BUTTON,
+ 0, sizeof(struct bt_data),
+ 0x83
[PATCH 1/3] Input: bcm5974 - Prepare for a new trackpad generation
With the advent of the Macbook Pro 12, we see a new generation of
trackpads, capable of force sensoring and haptic feedback.
This patch prepares for the new device by adding configuration data
for the code paths that would otherwise look different.
Tested-by: John Horan knas...@gmail.com
Tested-by: Jochen Radmacher jradmac...@gmx.de
Tested-by: Yang Hongyang bur...@gmail.com
Tested-by: Yen-Chin, Lee coldnew...@gmail.com
Tested-by: George Hilios ghil...@gmail.com
Tested-by: Janez Urevc ja...@janezurevc.name
Signed-off-by: Henrik Rydberg rydb...@bitmath.org
---
drivers/input/mouse/bcm5974.c | 132 ++
1 file changed, 81 insertions(+), 51 deletions(-)
diff --git a/drivers/input/mouse/bcm5974.c b/drivers/input/mouse/bcm5974.c
index b10709f..a596b9b 100644
--- a/drivers/input/mouse/bcm5974.c
+++ b/drivers/input/mouse/bcm5974.c
@@ -184,17 +184,37 @@ enum tp_type {
};
/* trackpad finger data offsets, le16-aligned */
-#define FINGER_TYPE1 (13 * sizeof(__le16))
-#define FINGER_TYPE2 (15 * sizeof(__le16))
-#define FINGER_TYPE3 (19 * sizeof(__le16))
+#define HEADER_TYPE1 (13 * sizeof(__le16))
+#define HEADER_TYPE2 (15 * sizeof(__le16))
+#define HEADER_TYPE3 (19 * sizeof(__le16))
/* trackpad button data offsets */
+#define BUTTON_TYPE1 0
#define BUTTON_TYPE2 15
#define BUTTON_TYPE3 23
/* list of device capability bits */
#define HAS_INTEGRATED_BUTTON 1
+/* trackpad finger data block size */
+#define FSIZE_TYPE1(14 * sizeof(__le16))
+#define FSIZE_TYPE2(14 * sizeof(__le16))
+#define FSIZE_TYPE3(14 * sizeof(__le16))
+
+/* offset from header to finger struct */
+#define DELTA_TYPE1(0 * sizeof(__le16))
+#define DELTA_TYPE2(0 * sizeof(__le16))
+#define DELTA_TYPE3(0 * sizeof(__le16))
+
+/* usb control message mode switch data */
+#define USBMSG_TYPE1 8, 0x300, 0, 0, 0x1, 0x8
+#define USBMSG_TYPE2 8, 0x300, 0, 0, 0x1, 0x8
+#define USBMSG_TYPE3 8, 0x300, 0, 0, 0x1, 0x8
+
+/* Wellspring initialization constants */
+#define BCM5974_WELLSPRING_MODE_READ_REQUEST_ID1
+#define BCM5974_WELLSPRING_MODE_WRITE_REQUEST_ID 9
+
/* trackpad finger structure, le16-aligned */
struct tp_finger {
__le16 origin; /* zero when switching track finger */
@@ -213,8 +233,6 @@ struct tp_finger {
/* trackpad finger data size, empirically at least ten fingers */
#define MAX_FINGERS16
-#define SIZEOF_FINGER sizeof(struct tp_finger)
-#define SIZEOF_ALL_FINGERS (MAX_FINGERS * SIZEOF_FINGER)
#define MAX_FINGER_ORIENTATION 16384
/* device-specific parameters */
@@ -232,8 +250,17 @@ struct bcm5974_config {
int bt_datalen; /* data length of the button interface */
int tp_ep; /* the endpoint of the trackpad interface */
enum tp_type tp_type; /* type of trackpad interface */
- int tp_offset; /* offset to trackpad finger data */
+ int tp_header; /* bytes in header block */
int tp_datalen; /* data length of the trackpad interface */
+ int tp_button; /* offset to button data */
+ int tp_fsize; /* bytes in single finger block */
+ int tp_delta; /* offset from header to finger struct */
+ int um_size;/* usb control message length */
+ int um_req_val; /* usb control message value */
+ int um_req_idx; /* usb control message index */
+ int um_switch_idx; /* usb control message mode switch index */
+ int um_switch_on; /* usb control message mode switch on */
+ int um_switch_off; /* usb control message mode switch off */
struct bcm5974_param p; /* finger pressure limits */
struct bcm5974_param w; /* finger width limits */
struct bcm5974_param x; /* horizontal limits */
@@ -259,6 +286,24 @@ struct bcm5974 {
int slots[MAX_FINGERS]; /* slot assignments */
};
+/* trackpad finger block data, le16-aligned */
+static const struct tp_finger *get_tp_finger(const struct bcm5974 *dev, int i)
+{
+ const struct bcm5974_config *c = dev-cfg;
+ u8 *f_base = dev-tp_data + c-tp_header + c-tp_delta;
+
+ return (const struct tp_finger *)(f_base + i * c-tp_fsize);
+}
+
+#define DATAFORMAT(type) \
+ type, \
+ HEADER_##type, \
+ HEADER_##type + (MAX_FINGERS) * (FSIZE_##type), \
+ BUTTON_##type, \
+ FSIZE_##type, \
+ DELTA_##type, \
+ USBMSG_##type
+
/* logical signal quality */
#define SN_PRESSURE45 /* pressure signal
[PATCH 2/3] HID: apple: Add support for the 2015 Macbook Pro
This patch adds keyboard support for MacbookPro12,1 as WELLSPRING9
(0x0272, 0x0273, 0x0274). The touchpad is handled in a separate
bcm5974 patch, as usual.
Tested-by: John Horan knas...@gmail.com
Tested-by: Jochen Radmacher jradmac...@gmx.de
Tested-by: Yang Hongyang bur...@gmail.com
Tested-by: Yen-Chin, Lee coldnew...@gmail.com
Tested-by: George Hilios ghil...@gmail.com
Tested-by: Janez Urevc ja...@janezurevc.name
Signed-off-by: Henrik Rydberg rydb...@bitmath.org
---
drivers/hid/hid-apple.c | 6 ++
drivers/hid/hid-core.c | 6 ++
drivers/hid/hid-ids.h | 3 +++
3 files changed, 15 insertions(+)
diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index f822fd2..884d82f 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -546,6 +546,12 @@ static const struct hid_device_id apple_devices[] = {
.driver_data = APPLE_HAS_FN | APPLE_ISO_KEYBOARD },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_JIS),
.driver_data = APPLE_HAS_FN | APPLE_RDESC_JIS },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI),
+ .driver_data = APPLE_HAS_FN },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ISO),
+ .driver_data = APPLE_HAS_FN | APPLE_ISO_KEYBOARD },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_JIS),
+ .driver_data = APPLE_HAS_FN | APPLE_RDESC_JIS },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_ANSI),
.driver_data = APPLE_NUMLOCK_EMULATION | APPLE_HAS_FN },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_ISO),
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 157c627..e6fce23 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1782,6 +1782,9 @@ static const struct hid_device_id
hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_ISO) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_JIS) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ISO) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_JIS) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_ANSI) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_ISO) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2009_JIS) },
@@ -2463,6 +2466,9 @@ static const struct hid_device_id hid_mouse_ignore_list[]
= {
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_ISO) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING8_JIS) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_ISO) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_WELLSPRING9_JIS) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY) },
{ HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY) },
{ }
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index b04b082..b3b225b 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -142,6 +142,9 @@
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ANSI 0x0290
#define USB_DEVICE_ID_APPLE_WELLSPRING8_ISO0x0291
#define USB_DEVICE_ID_APPLE_WELLSPRING8_JIS0x0292
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_ANSI 0x0272
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_ISO0x0273
+#define USB_DEVICE_ID_APPLE_WELLSPRING9_JIS0x0274
#define USB_DEVICE_ID_APPLE_FOUNTAIN_TP_ONLY 0x030a
#define USB_DEVICE_ID_APPLE_GEYSER1_TP_ONLY0x030b
#define USB_DEVICE_ID_APPLE_IRCONTROL 0x8240
--
2.4.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Input - synaptics: pin 3 touches when the firmware reports 3 fingers
Benjamin, For old kernels this is not a problem because max_slots was 2 and libinput/ xorg-synaptics knew how to deal with that. Now that max_slot is 3, the clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP information, and goes wild. Maybe the cr48 sensor should be classified as MT_SEMI instead. Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] Input - synaptics: pin 3 touches when the firmware reports 3 fingers
Benjamin, For old kernels this is not a problem because max_slots was 2 and libinput/ xorg-synaptics knew how to deal with that. Now that max_slot is 3, the clients ignore BTN_TOOL_TRIPLETAP and count the actual used slots (so 2). It then gets confused when receiving the BTN_TOOL_TRIPLETAP and DOUBLETAP information, and goes wild. Maybe the cr48 sensor should be classified as MT_SEMI instead. Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Input - mt: Fix input_mt_get_slot_by_key
Peter, It may be a long time ago now, but we had very vocal discussions regarding the MT protocol back then, and I am quite sure all the subtleties are well understood. In order to fully appreciate the simplicity of the protocol, one only needs to stop misintepreting it. In order to do that, please imagine a large piece of papers, a set of brushes, and a set of colors. The MT protocol tracks brushes. When lines are drawn, positions are updated. When the color on the brush changes, the tracking id changes. When the brush is lifted, the tracking id becomes -1, the "no color" color. There is a fixed set of brushes. The old test application mtview works precisely this way. The add/remove protocol tracks colors. Technichally it tracks contacts, which are the combined (color, brush) objects, but in this analogy, colors and tracking ids are interchangeable. When lines a drawn, a color is first assigned to the color and the brush attributes on the color are updated. The positions on the color are subsequently updated. To change color, the brush is deassign from the first color and then the brush is assign to the new color. To lift, the brush is deassign from the color. This is the abstraction the seems to prevail in userland at the moment. > can't you just slot in an extra event that contains only the > ABS_MT_TRACKING_ID -1 for the needed slots, followed by an SYN_REPORT and > whatever BTN_TOOL_* updates are needed? You don't need extra slots here, > you're just putting one extra event in the queue before handling the new > touches as usual. So you want to add a rule saying that before a brush changes color, it first has to be cleaned. That may look simple enough, but it misses out on several subtleties. 1. It is no longer possible to create beautiful contiuous tracks of varying color. 2. When a brush moves quickly, it will sometimes restart the track with a new color. This happens on all hardware with tracking support when you approach the sampling limit. It happens in the kernel tracking as well, for the same reasons. Incompatible with 1. 3. There is a difference between losing track of a brush and lifting a brush. This is one of the situations where the add/remove protocol has to create very nonintuitive restrictions and rules to cope. The reason starts with 1. Forcing tracks (brushes) into the add/remove protocol creates problems that are on a more fundamental level than the subtle issues one may hope to resolve. > thing is: I've always assumed that a touch is terminated by a -1 event > and this hasn't been a problem until very recently. We have talked a lot about the differences, they can hardly have escaped anyone deeply involved. It is true that this is not normally a problem. It only becomes important when the sampling rate is too low to resolve all the actions we deem important. > so anything I've ever > touched will be broken if we start switching tracking IDs directly. > That > includes xorg input, libinput and anything that uses libevdev. sorry. This has been in the mainline kernel for the last five years, and obvisouly still works well most of the time. I sometimes experience glitches in the trackpad usage on my laptop, and it probably stems from this issues. It is slightly annoying, but not broken. No reason to panic. > if the kernel switches from one tracking ID to another directly, > libevdev will actually discard the new tracking ID. > http://cgit.freedesktop.org/libevdev/tree/libevdev/libevdev.c#n968 > (sorry again) aside from the warning, it's safe to switch directly though, > there shouldn't be any side-effects. > as for fixing this: I can add something to libevdev to allow it but I'll > also need to fix up every caller to handle this sequence then, they all rely > on the -1. so some stuff will simply break. > plus we still have synaptics up to 1.7.x and evdev up to 2.8.x that are > pre-libevdev. Perhaps this is worth looking at in conjunction with the problem of handling lost touches. I am thinking of suspend/resume issues in particular. If the system could handle the distinction between a lift and a lost touch, some logic would be less complicated and more correct. > for other event processing it's tricky as well. if you go from two > touches to two new touches you need to send out a BTN_TOOL_DOUBLETAP 0, then > 1. if not, a legacy process missed the event completely (synaptics would > suffer from that). likewise, without the BTN_TOUCH going to 0 for one frame > you'll get coordinate jumps on the pointer emulation. I am not so sure about this - the movement in synaptics checks if there has been any identity changes. > having the tracking ID go -1 and then to a real one in the same frame makes > this even worse, because now even the MT-capable processes need to attach > flags to each touch whether it intermediately terminated or not. This is simply the result of a poor abstraction to begin with. The state tracked through the input subsystem is the slot state. > The
Re: [PATCH v2] Input - mt: Fix input_mt_get_slot_by_key
Peter, It may be a long time ago now, but we had very vocal discussions regarding the MT protocol back then, and I am quite sure all the subtleties are well understood. In order to fully appreciate the simplicity of the protocol, one only needs to stop misintepreting it. In order to do that, please imagine a large piece of papers, a set of brushes, and a set of colors. The MT protocol tracks brushes. When lines are drawn, positions are updated. When the color on the brush changes, the tracking id changes. When the brush is lifted, the tracking id becomes -1, the no color color. There is a fixed set of brushes. The old test application mtview works precisely this way. The add/remove protocol tracks colors. Technichally it tracks contacts, which are the combined (color, brush) objects, but in this analogy, colors and tracking ids are interchangeable. When lines a drawn, a color is first assigned to the color and the brush attributes on the color are updated. The positions on the color are subsequently updated. To change color, the brush is deassign from the first color and then the brush is assign to the new color. To lift, the brush is deassign from the color. This is the abstraction the seems to prevail in userland at the moment. can't you just slot in an extra event that contains only the ABS_MT_TRACKING_ID -1 for the needed slots, followed by an SYN_REPORT and whatever BTN_TOOL_* updates are needed? You don't need extra slots here, you're just putting one extra event in the queue before handling the new touches as usual. So you want to add a rule saying that before a brush changes color, it first has to be cleaned. That may look simple enough, but it misses out on several subtleties. 1. It is no longer possible to create beautiful contiuous tracks of varying color. 2. When a brush moves quickly, it will sometimes restart the track with a new color. This happens on all hardware with tracking support when you approach the sampling limit. It happens in the kernel tracking as well, for the same reasons. Incompatible with 1. 3. There is a difference between losing track of a brush and lifting a brush. This is one of the situations where the add/remove protocol has to create very nonintuitive restrictions and rules to cope. The reason starts with 1. Forcing tracks (brushes) into the add/remove protocol creates problems that are on a more fundamental level than the subtle issues one may hope to resolve. thing is: I've always assumed that a touch is terminated by a -1 event and this hasn't been a problem until very recently. We have talked a lot about the differences, they can hardly have escaped anyone deeply involved. It is true that this is not normally a problem. It only becomes important when the sampling rate is too low to resolve all the actions we deem important. so anything I've ever touched will be broken if we start switching tracking IDs directly. That includes xorg input, libinput and anything that uses libevdev. sorry. This has been in the mainline kernel for the last five years, and obvisouly still works well most of the time. I sometimes experience glitches in the trackpad usage on my laptop, and it probably stems from this issues. It is slightly annoying, but not broken. No reason to panic. if the kernel switches from one tracking ID to another directly, libevdev will actually discard the new tracking ID. http://cgit.freedesktop.org/libevdev/tree/libevdev/libevdev.c#n968 (sorry again) aside from the warning, it's safe to switch directly though, there shouldn't be any side-effects. as for fixing this: I can add something to libevdev to allow it but I'll also need to fix up every caller to handle this sequence then, they all rely on the -1. so some stuff will simply break. plus we still have synaptics up to 1.7.x and evdev up to 2.8.x that are pre-libevdev. Perhaps this is worth looking at in conjunction with the problem of handling lost touches. I am thinking of suspend/resume issues in particular. If the system could handle the distinction between a lift and a lost touch, some logic would be less complicated and more correct. for other event processing it's tricky as well. if you go from two touches to two new touches you need to send out a BTN_TOOL_DOUBLETAP 0, then 1. if not, a legacy process missed the event completely (synaptics would suffer from that). likewise, without the BTN_TOUCH going to 0 for one frame you'll get coordinate jumps on the pointer emulation. I am not so sure about this - the movement in synaptics checks if there has been any identity changes. having the tracking ID go -1 and then to a real one in the same frame makes this even worse, because now even the MT-capable processes need to attach flags to each touch whether it intermediately terminated or not. This is simply the result of a poor abstraction to begin with. The state tracked through the input subsystem is the slot state. The event ordering is not guaranteed,
Re: [PATCH v2] Input - mt: Fix input_mt_get_slot_by_key
> My guess is that none of these drivers are > able to handle a silent closing of a slot and the easiest solution might > be to simply change the documentation if in fact nobody uses the > compressed notation (which removes just one ABS_SLOT event within the > frame, so not sure we can call it compressed BTW). No, this is a very bad idea. As is obvious from the patch and the problem, there are cases that the typical add/remove interface cannot handle, without either adding more frames or support twice as many slots. How else would you report a complete change of all N contacts into N new contacts, for instance? The kernel protocol is solid and works for all cases. Userland thus has access to a complete solution. Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Input - mt: Fix input_mt_get_slot_by_key
> "Creation, replacement and destruction of contacts is achieved by > modifying the ABS_MT_TRACKING_ID of the associated slot. A > non-negative tracking id is interpreted as a contact, and the value -1 > denotes an unused slot. A tracking id not previously present is > considered new, and a tracking id no longer present is considered > removed." > > If some userspace is confused with missing -1 tracking ID for that > slot, that userspace should be fixed. I agree. Some userland applications work with add/remove out of convenience, and cannot handle the more compressed notation the kernel slot handling allows. Fixing those applications will be a good thing. Unfortunately the patch already appeared in Linus' tree... Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Input - mt: Fix input_mt_get_slot_by_key
Creation, replacement and destruction of contacts is achieved by modifying the ABS_MT_TRACKING_ID of the associated slot. A non-negative tracking id is interpreted as a contact, and the value -1 denotes an unused slot. A tracking id not previously present is considered new, and a tracking id no longer present is considered removed. If some userspace is confused with missing -1 tracking ID for that slot, that userspace should be fixed. I agree. Some userland applications work with add/remove out of convenience, and cannot handle the more compressed notation the kernel slot handling allows. Fixing those applications will be a good thing. Unfortunately the patch already appeared in Linus' tree... Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2] Input - mt: Fix input_mt_get_slot_by_key
My guess is that none of these drivers are able to handle a silent closing of a slot and the easiest solution might be to simply change the documentation if in fact nobody uses the compressed notation (which removes just one ABS_SLOT event within the frame, so not sure we can call it compressed BTW). No, this is a very bad idea. As is obvious from the patch and the problem, there are cases that the typical add/remove interface cannot handle, without either adding more frames or support twice as many slots. How else would you report a complete change of all N contacts into N new contacts, for instance? The kernel protocol is solid and works for all cases. Userland thus has access to a complete solution. Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/3] Balanced slots, attempt #2
On 03/31/2015 05:07 PM, Benjamin Tissoires wrote: > Hi, > > so this is the v2 with Hans' ack and Henrik's respin of the first patch. The whole series looks good, thank you Benjamin. Acked-by: Henrik Rydberg Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Input: mt - prevent balanced slot assignment to assign twice the slot
Hi Benjamin,
> If two touches are under the dmax distance, it looks like they can now
> be assigned to the same slot. Add a band aid to prevent such situation
> and be able to use the balanced slot assignment.
Yes, great find. You patch is correct, but it is not a band aid, but a
result stemming from a limitation in how equality constraints (read
unique assignments) can be handled in the iterative algorithm. This
cannot happen in the original algorithm, because of the extra
penalization for overcovers.
Here is an alternative version which cleans up the loop logic
somewhat, together with a different commit message. I did not have any
opportunity to check this in hardware. I you like it and find it
working, then please take over the authorship and just add a
signed-off from me.
Thanks,
Henrik
---
>From e00d63f6cadf20d871bed763b3531428b34d785c Mon Sep 17 00:00:00 2001
From: Henrik Rydberg
Date: Tue, 31 Mar 2015 09:10:02 +0200
Subject: [PATCH] Input: MT - make slot assignment work for overcovered
solutions
The recent inclusion of a deassignment cost in the slot assignment
algorithm did not properly account for the corner cases where the
solutions are overcovered. This patch makes sure the resulting assignment
is unique, allocating new slots when necessary.
UNTESTED
---
drivers/input/input-mt.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index fbe29fc..17e80a6 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -363,25 +363,28 @@ static void input_mt_set_slots(struct input_mt *mt,
int *slots, int num_pos)
{
struct input_mt_slot *s;
- int *w = mt->red, *p;
+ int *w = mt->red, j;
- for (p = slots; p != slots + num_pos; p++)
- *p = -1;
+ for (j = 0; j != num_pos; j++)
+ slots[j] = -1;
for (s = mt->slots; s != mt->slots + mt->num_slots; s++) {
if (!input_mt_is_active(s))
continue;
- for (p = slots; p != slots + num_pos; p++)
- if (*w++ < 0)
- *p = s - mt->slots;
+ for (j = 0; j != num_pos; j++)
+ if (w[j] < 0) {
+ slots[j] = s - mt->slots;
+ break;
+ }
+ w += num_pos;
}
for (s = mt->slots; s != mt->slots + mt->num_slots; s++) {
if (input_mt_is_active(s))
continue;
- for (p = slots; p != slots + num_pos; p++)
- if (*p < 0) {
- *p = s - mt->slots;
+ for (j = 0; j != num_pos; j++)
+ if (slots[j] < 0) {
+ slots[j] = s - mt->slots;
break;
}
}
--
2.3.0
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] Input: mt - prevent balanced slot assignment to assign twice the slot
Hi Benjamin,
If two touches are under the dmax distance, it looks like they can now
be assigned to the same slot. Add a band aid to prevent such situation
and be able to use the balanced slot assignment.
Yes, great find. You patch is correct, but it is not a band aid, but a
result stemming from a limitation in how equality constraints (read
unique assignments) can be handled in the iterative algorithm. This
cannot happen in the original algorithm, because of the extra
penalization for overcovers.
Here is an alternative version which cleans up the loop logic
somewhat, together with a different commit message. I did not have any
opportunity to check this in hardware. I you like it and find it
working, then please take over the authorship and just add a
signed-off from me.
Thanks,
Henrik
---
From e00d63f6cadf20d871bed763b3531428b34d785c Mon Sep 17 00:00:00 2001
From: Henrik Rydberg rydb...@bitmath.org
Date: Tue, 31 Mar 2015 09:10:02 +0200
Subject: [PATCH] Input: MT - make slot assignment work for overcovered
solutions
The recent inclusion of a deassignment cost in the slot assignment
algorithm did not properly account for the corner cases where the
solutions are overcovered. This patch makes sure the resulting assignment
is unique, allocating new slots when necessary.
UNTESTED
---
drivers/input/input-mt.c | 21 -
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index fbe29fc..17e80a6 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -363,25 +363,28 @@ static void input_mt_set_slots(struct input_mt *mt,
int *slots, int num_pos)
{
struct input_mt_slot *s;
- int *w = mt-red, *p;
+ int *w = mt-red, j;
- for (p = slots; p != slots + num_pos; p++)
- *p = -1;
+ for (j = 0; j != num_pos; j++)
+ slots[j] = -1;
for (s = mt-slots; s != mt-slots + mt-num_slots; s++) {
if (!input_mt_is_active(s))
continue;
- for (p = slots; p != slots + num_pos; p++)
- if (*w++ 0)
- *p = s - mt-slots;
+ for (j = 0; j != num_pos; j++)
+ if (w[j] 0) {
+ slots[j] = s - mt-slots;
+ break;
+ }
+ w += num_pos;
}
for (s = mt-slots; s != mt-slots + mt-num_slots; s++) {
if (input_mt_is_active(s))
continue;
- for (p = slots; p != slots + num_pos; p++)
- if (*p 0) {
- *p = s - mt-slots;
+ for (j = 0; j != num_pos; j++)
+ if (slots[j] 0) {
+ slots[j] = s - mt-slots;
break;
}
}
--
2.3.0
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 0/3] Balanced slots, attempt #2
On 03/31/2015 05:07 PM, Benjamin Tissoires wrote: Hi, so this is the v2 with Hans' ack and Henrik's respin of the first patch. The whole series looks good, thank you Benjamin. Acked-by: Henrik Rydberg rydb...@bitmath.org Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Input - mt: Fix input_mt_get_slot_by_key
Hi Benjamin,
> Slot 0 is released correclty, but when we look for Contact ID 2, the slot
> 0 is then picked up again because it is marked as inactive (trackingID < 0).
>
> This is wrong, and we should not reuse a slot in the same frame.
> The test should also check for input_mt_is_used().
Good catch! However...
> @@ -453,7 +456,7 @@ int input_mt_get_slot_by_key(struct input_dev *dev, int
> key)
> return s - mt->slots;
>
> for (s = mt->slots; s != mt->slots + mt->num_slots; s++)
> - if (!input_mt_is_active(s)) {
> + if (!input_mt_is_active(s) && !input_mt_is_used(mt, s)) {
> s->key = key;
> return s - mt->slots;
> }
>
Here, you are changing the preconditions of the function without explicit
reference to all its users. For one, it is now assumed that input_mt_is_used()
is up-to-date, which requires either input_mt_drop_unused() or
input_mt_sync_frame(), which does not seem to be true for all users of
input_mt_get_slot_by_key(). After a couple of iterations with
input_mt_report_slot_state() in those drivers, input_mt_is_used() will be true
for all slots, and the driver will stop working.
How about defering the deassignments until the end of the loop instead? That
would remove possible reuse.
Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Input - mt: Fix input_mt_get_slot_by_key
Hi Benjamin,
Slot 0 is released correclty, but when we look for Contact ID 2, the slot
0 is then picked up again because it is marked as inactive (trackingID 0).
This is wrong, and we should not reuse a slot in the same frame.
The test should also check for input_mt_is_used().
Good catch! However...
@@ -453,7 +456,7 @@ int input_mt_get_slot_by_key(struct input_dev *dev, int
key)
return s - mt-slots;
for (s = mt-slots; s != mt-slots + mt-num_slots; s++)
- if (!input_mt_is_active(s)) {
+ if (!input_mt_is_active(s) !input_mt_is_used(mt, s)) {
s-key = key;
return s - mt-slots;
}
Here, you are changing the preconditions of the function without explicit
reference to all its users. For one, it is now assumed that input_mt_is_used()
is up-to-date, which requires either input_mt_drop_unused() or
input_mt_sync_frame(), which does not seem to be true for all users of
input_mt_get_slot_by_key(). After a couple of iterations with
input_mt_report_slot_state() in those drivers, input_mt_is_used() will be true
for all slots, and the driver will stop working.
How about defering the deassignments until the end of the loop instead? That
would remove possible reuse.
Thanks,
Henrik
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Input: MT - Add support for balanced slot assignment
Hi Benjamin,
> Tested this morning, and yes, it solves the problem.
> I assumed that the dmax was in mm, and used "10 * priv->x_res", which
> seemed to do the trick:
> - tapping with two fingers side by side triggered the jumps
> - in the general case (switching from the index to the thumb to
> click), the jumps disappeared.
Great, thanks for testing.
> Maybe we should also add a doc telling which units the dmax should be.
I added an enhanced description, which hopefully clarifies the units
of dmax.
> When you'll sign this, you can add my tested-by.
Done, and tested against Linus' master. I take it you have a patch on
this coming up as well?
Thanks,
Henrik
>From 2a4986420e634df2f0ba09198cb1e55714f61577 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg
Date: Sun, 1 Feb 2015 09:16:10 +0100
Subject: [PATCH v2] Input: MT - Add support for balanced slot assignment
Some devices are not fast enough to differentiate between a
fast-moving contact and a new contact. This problem cannot be fully
resolved because information is truly missing, but it is possible
to safe-guard against obvious mistakes by restricting movement with
a maximum displacement.
The new problem formulation for dmax > 0 cannot benefit from the
speedup for positive definite matrices, but since the convergence is
faster, the result is about the same. For a handful of contacts, the
latency difference is truly negligible.
Suggested-by: Benjamin Tissoires
Tested-by: Benjamin Tissoires
Signed-off-by: Henrik Rydberg
---
drivers/input/input-mt.c | 31 ---
drivers/input/mouse/alps.c| 2 +-
drivers/input/mouse/bcm5974.c | 2 +-
drivers/input/mouse/cypress_ps2.c | 2 +-
drivers/input/mouse/synaptics.c | 2 +-
drivers/input/touchscreen/pixcir_i2c_ts.c | 2 +-
include/linux/input/mt.h | 3 ++-
7 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index fbe29fc..cb150a1 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -293,7 +293,7 @@ void input_mt_sync_frame(struct input_dev *dev)
}
EXPORT_SYMBOL(input_mt_sync_frame);
-static int adjust_dual(int *begin, int step, int *end, int eq)
+static int adjust_dual(int *begin, int step, int *end, int eq, int mu)
{
int f, *p, s, c;
@@ -311,9 +311,10 @@ static int adjust_dual(int *begin, int step, int *end, int
eq)
s = *p;
c = (f + s + 1) / 2;
- if (c == 0 || (c > 0 && !eq))
+ if (c == 0 || (c > mu && (!eq || mu > 0)))
return 0;
- if (s < 0)
+ /* Improve convergence for positive matrices by penalizing overcovers */
+ if (s < 0 && mu <= 0)
c *= 2;
for (p = begin; p != end; p += step)
@@ -322,23 +323,24 @@ static int adjust_dual(int *begin, int step, int *end,
int eq)
return (c < s && s <= 0) || (f >= 0 && f < c);
}
-static void find_reduced_matrix(int *w, int nr, int nc, int nrc)
+static void find_reduced_matrix(int *w, int nr, int nc, int nrc, int mu)
{
int i, k, sum;
for (k = 0; k < nrc; k++) {
for (i = 0; i < nr; i++)
- adjust_dual(w + i, nr, w + i + nrc, nr <= nc);
+ adjust_dual(w + i, nr, w + i + nrc, nr <= nc, mu);
sum = 0;
for (i = 0; i < nrc; i += nr)
- sum += adjust_dual(w + i, 1, w + i + nr, nc <= nr);
+ sum += adjust_dual(w + i, 1, w + i + nr, nc <= nr, mu);
if (!sum)
break;
}
}
static int input_mt_set_matrix(struct input_mt *mt,
- const struct input_mt_pos *pos, int num_pos)
+ const struct input_mt_pos *pos, int num_pos,
+ int mu)
{
const struct input_mt_pos *p;
struct input_mt_slot *s;
@@ -352,7 +354,7 @@ static int input_mt_set_matrix(struct input_mt *mt,
y = input_mt_get_value(s, ABS_MT_POSITION_Y);
for (p = pos; p != pos + num_pos; p++) {
int dx = x - p->x, dy = y - p->y;
- *w++ = dx * dx + dy * dy;
+ *w++ = dx * dx + dy * dy - mu;
}
}
@@ -393,17 +395,24 @@ static void input_mt_set_slots(struct input_mt *mt,
* @slots: the slot assignment to be filled
* @pos: the position array to match
* @num_pos: number of positions
+ * @dmax: maximum ABS_MT_POSITION displacement (zero for infinite)
*
* Performs a best match against the current contacts and returns
* the slot assignment list. New contacts are assigned to unused
* slots.
*
+ * The assignments are balanced so that all coordinate
Re: [PATCH] Input: MT - Add support for balanced slot assignment
Hi Benjamin,
Tested this morning, and yes, it solves the problem.
I assumed that the dmax was in mm, and used 10 * priv-x_res, which
seemed to do the trick:
- tapping with two fingers side by side triggered the jumps
- in the general case (switching from the index to the thumb to
click), the jumps disappeared.
Great, thanks for testing.
Maybe we should also add a doc telling which units the dmax should be.
I added an enhanced description, which hopefully clarifies the units
of dmax.
When you'll sign this, you can add my tested-by.
Done, and tested against Linus' master. I take it you have a patch on
this coming up as well?
Thanks,
Henrik
From 2a4986420e634df2f0ba09198cb1e55714f61577 Mon Sep 17 00:00:00 2001
From: Henrik Rydberg rydb...@bitmath.org
Date: Sun, 1 Feb 2015 09:16:10 +0100
Subject: [PATCH v2] Input: MT - Add support for balanced slot assignment
Some devices are not fast enough to differentiate between a
fast-moving contact and a new contact. This problem cannot be fully
resolved because information is truly missing, but it is possible
to safe-guard against obvious mistakes by restricting movement with
a maximum displacement.
The new problem formulation for dmax 0 cannot benefit from the
speedup for positive definite matrices, but since the convergence is
faster, the result is about the same. For a handful of contacts, the
latency difference is truly negligible.
Suggested-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
Tested-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
Signed-off-by: Henrik Rydberg rydb...@bitmath.org
---
drivers/input/input-mt.c | 31 ---
drivers/input/mouse/alps.c| 2 +-
drivers/input/mouse/bcm5974.c | 2 +-
drivers/input/mouse/cypress_ps2.c | 2 +-
drivers/input/mouse/synaptics.c | 2 +-
drivers/input/touchscreen/pixcir_i2c_ts.c | 2 +-
include/linux/input/mt.h | 3 ++-
7 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index fbe29fc..cb150a1 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -293,7 +293,7 @@ void input_mt_sync_frame(struct input_dev *dev)
}
EXPORT_SYMBOL(input_mt_sync_frame);
-static int adjust_dual(int *begin, int step, int *end, int eq)
+static int adjust_dual(int *begin, int step, int *end, int eq, int mu)
{
int f, *p, s, c;
@@ -311,9 +311,10 @@ static int adjust_dual(int *begin, int step, int *end, int
eq)
s = *p;
c = (f + s + 1) / 2;
- if (c == 0 || (c 0 !eq))
+ if (c == 0 || (c mu (!eq || mu 0)))
return 0;
- if (s 0)
+ /* Improve convergence for positive matrices by penalizing overcovers */
+ if (s 0 mu = 0)
c *= 2;
for (p = begin; p != end; p += step)
@@ -322,23 +323,24 @@ static int adjust_dual(int *begin, int step, int *end,
int eq)
return (c s s = 0) || (f = 0 f c);
}
-static void find_reduced_matrix(int *w, int nr, int nc, int nrc)
+static void find_reduced_matrix(int *w, int nr, int nc, int nrc, int mu)
{
int i, k, sum;
for (k = 0; k nrc; k++) {
for (i = 0; i nr; i++)
- adjust_dual(w + i, nr, w + i + nrc, nr = nc);
+ adjust_dual(w + i, nr, w + i + nrc, nr = nc, mu);
sum = 0;
for (i = 0; i nrc; i += nr)
- sum += adjust_dual(w + i, 1, w + i + nr, nc = nr);
+ sum += adjust_dual(w + i, 1, w + i + nr, nc = nr, mu);
if (!sum)
break;
}
}
static int input_mt_set_matrix(struct input_mt *mt,
- const struct input_mt_pos *pos, int num_pos)
+ const struct input_mt_pos *pos, int num_pos,
+ int mu)
{
const struct input_mt_pos *p;
struct input_mt_slot *s;
@@ -352,7 +354,7 @@ static int input_mt_set_matrix(struct input_mt *mt,
y = input_mt_get_value(s, ABS_MT_POSITION_Y);
for (p = pos; p != pos + num_pos; p++) {
int dx = x - p-x, dy = y - p-y;
- *w++ = dx * dx + dy * dy;
+ *w++ = dx * dx + dy * dy - mu;
}
}
@@ -393,17 +395,24 @@ static void input_mt_set_slots(struct input_mt *mt,
* @slots: the slot assignment to be filled
* @pos: the position array to match
* @num_pos: number of positions
+ * @dmax: maximum ABS_MT_POSITION displacement (zero for infinite)
*
* Performs a best match against the current contacts and returns
* the slot assignment list. New contacts are assigned to unused
* slots.
*
+ * The assignments are balanced so that all coordinate displacements are
+ * below the euclidian distance dmax. If no such assignment can be found
Re: [PATCH] Input: MT - Add support for balanced slot assignment
Hi Dmitry,
On 01/22/2015 09:02 PM, Dmitry Torokhov wrote:
> On Thu, Jan 22, 2015 at 08:52:25PM +0100, Henrik Rydberg wrote:
>> int input_mt_assign_slots(struct input_dev *dev, int *slots,
>> - const struct input_mt_pos *pos, int num_pos)
>> + const struct input_mt_pos *pos, int num_pos,
>> + int dmax)
>
> Should dmax be unsigned and do we really need to treat 0 specially or we
> could use UNIT_MAX as "don't care" value?
We could have dmax unsigned, but it does not have to be from a branching
perspective, since the square is what gets used anyways.
>> {
>> struct input_mt *mt = dev->mt;
>> +int mu = 2 * dmax * dmax;
>
> For my education, what does "mu" stand for?
I chose mu because of the mathematical similarity to the chemical potential in
statistical mechanics, where it denotes the energy per particle. Here, it
denotes the energy per contact assignment.
> Ideally, if someone could create a
> write-up on the contact matching that would be most awesome.
Heh, I guess I will have to write something at some point, without requiring
prior knowledge of Lagrange relaxation or the like. Time is a luxury these
days...
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Input: MT - Add support for balanced slot assignment
Some devices are not fast enough to differentiate between a
fast-moving contact and a new contact. This problem cannot be fully
resolved because information is truly missing, but it is possible
to safe-guard against obvious mistakes by restricting movement with
a maximum displacement.
The new problem formulation for dmax > 0 cannot benefit from the
speedup for positive definite matrices, but since the convergence is
faster, the result is about the same. For a handful of contacts, the
latency difference is truly negligible.
Suggested-by: Benjamin Tissoires
Not-yet-signed-off-by: Henrik Rydberg
---
Hi Dmitry, Benjamin,
This patch incorporates the notion of a speed limit in the slot
assignment function, without changing the behavior for existing code.
There are multi-touch movement cases where the difference between this
algorithm and a simple deassign-overspeeding-contacts algorithm which
warrants this approach. One example is when a whole array of contacts
are moved one step to the right in one frame. With the simple
algorithm, all contacts will be deassigned, whereas with this
algorithm, the leftmost contact is deassigned and a new contact
created on the right. This is the interpretation of smallest change,
and is consistent with the behavior one would get from two contacts,
for both algorithms.
I have tested this on a number of testcases in my tree, and I am
running the patch with a speed limit on my device as I write this. If
this patch solves Benjamin's problem, I think we are ok.
Henrik
drivers/input/input-mt.c | 31 ---
drivers/input/mouse/alps.c| 2 +-
drivers/input/mouse/bcm5974.c | 2 +-
drivers/input/mouse/cypress_ps2.c | 2 +-
drivers/input/mouse/synaptics.c | 2 +-
drivers/input/touchscreen/pixcir_i2c_ts.c | 2 +-
include/linux/input/mt.h | 3 ++-
7 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index fbe29fc..990509e 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -293,7 +293,7 @@ void input_mt_sync_frame(struct input_dev *dev)
}
EXPORT_SYMBOL(input_mt_sync_frame);
-static int adjust_dual(int *begin, int step, int *end, int eq)
+static int adjust_dual(int *begin, int step, int *end, int eq, int mu)
{
int f, *p, s, c;
@@ -311,9 +311,10 @@ static int adjust_dual(int *begin, int step, int *end, int
eq)
s = *p;
c = (f + s + 1) / 2;
- if (c == 0 || (c > 0 && !eq))
+ if (c == 0 || (c > mu && (!eq || mu > 0)))
return 0;
- if (s < 0)
+ /* Improve convergence for positive matrices by penalizing overcovers */
+ if (s < 0 && mu <= 0)
c *= 2;
for (p = begin; p != end; p += step)
@@ -322,23 +323,24 @@ static int adjust_dual(int *begin, int step, int *end,
int eq)
return (c < s && s <= 0) || (f >= 0 && f < c);
}
-static void find_reduced_matrix(int *w, int nr, int nc, int nrc)
+static void find_reduced_matrix(int *w, int nr, int nc, int nrc, int mu)
{
int i, k, sum;
for (k = 0; k < nrc; k++) {
for (i = 0; i < nr; i++)
- adjust_dual(w + i, nr, w + i + nrc, nr <= nc);
+ adjust_dual(w + i, nr, w + i + nrc, nr <= nc, mu);
sum = 0;
for (i = 0; i < nrc; i += nr)
- sum += adjust_dual(w + i, 1, w + i + nr, nc <= nr);
+ sum += adjust_dual(w + i, 1, w + i + nr, nc <= nr, mu);
if (!sum)
break;
}
}
static int input_mt_set_matrix(struct input_mt *mt,
- const struct input_mt_pos *pos, int num_pos)
+ const struct input_mt_pos *pos, int num_pos,
+ int mu)
{
const struct input_mt_pos *p;
struct input_mt_slot *s;
@@ -352,7 +354,7 @@ static int input_mt_set_matrix(struct input_mt *mt,
y = input_mt_get_value(s, ABS_MT_POSITION_Y);
for (p = pos; p != pos + num_pos; p++) {
int dx = x - p->x, dy = y - p->y;
- *w++ = dx * dx + dy * dy;
+ *w++ = dx * dx + dy * dy - mu;
}
}
@@ -393,17 +395,24 @@ static void input_mt_set_slots(struct input_mt *mt,
* @slots: the slot assignment to be filled
* @pos: the position array to match
* @num_pos: number of positions
+ * @dmax: maximum sensor displacement (zero for infinite)
*
* Performs a best match against the current contacts and returns
* the slot assignment list. New contacts are assigned to unused
* slots.
*
+ * The assignments are balanced so that all displacements are below
+ * dmax. If no such assi
Re: Fixing touch point jumps in the kernel (was Re: [PATCH] MAINTAINERS: Update rydberg's addresses)
Hi Peter, > from the testing I've done, you cannot trigger this except by lifting a > finger and raising a finger. There is no per-use-case requirement, it's a > hardware deficiency that the hardware simply cannot detect this specific > case of finger change. This is a good selling point, but... > again, you cannot trigger this by moving. or at least if you do your > pointer would be in the bottom right corner anyway because you have to move > the finger so insanely fast that pointer control is clearly not in the > picture anymore. this is what puts me off again. It takes me back to the old arcade days, and watching kids hammer at the controls. What is insane for one person may not be insane for another. That said, I understand and agree that there exists an upper move velocity that makes sense for any particular touchpad. So I will accept two patches on this: one to set the speed limit in the MT layer, and one which uses that limit to solve a real problem, for the touchpad in question. Let's end this thread here and leave unrelated people in peace. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Fixing touch point jumps in the kernel (was Re: [PATCH] MAINTAINERS: Update rydberg's addresses)
Hi Peter, from the testing I've done, you cannot trigger this except by lifting a finger and raising a finger. There is no per-use-case requirement, it's a hardware deficiency that the hardware simply cannot detect this specific case of finger change. This is a good selling point, but... again, you cannot trigger this by moving. or at least if you do your pointer would be in the bottom right corner anyway because you have to move the finger so insanely fast that pointer control is clearly not in the picture anymore. this is what puts me off again. It takes me back to the old arcade days, and watching kids hammer at the controls. What is insane for one person may not be insane for another. That said, I understand and agree that there exists an upper move velocity that makes sense for any particular touchpad. So I will accept two patches on this: one to set the speed limit in the MT layer, and one which uses that limit to solve a real problem, for the touchpad in question. Let's end this thread here and leave unrelated people in peace. Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Input: MT - Add support for balanced slot assignment
Some devices are not fast enough to differentiate between a
fast-moving contact and a new contact. This problem cannot be fully
resolved because information is truly missing, but it is possible
to safe-guard against obvious mistakes by restricting movement with
a maximum displacement.
The new problem formulation for dmax 0 cannot benefit from the
speedup for positive definite matrices, but since the convergence is
faster, the result is about the same. For a handful of contacts, the
latency difference is truly negligible.
Suggested-by: Benjamin Tissoires benjamin.tissoi...@redhat.com
Not-yet-signed-off-by: Henrik Rydberg rydb...@bitmath.org
---
Hi Dmitry, Benjamin,
This patch incorporates the notion of a speed limit in the slot
assignment function, without changing the behavior for existing code.
There are multi-touch movement cases where the difference between this
algorithm and a simple deassign-overspeeding-contacts algorithm which
warrants this approach. One example is when a whole array of contacts
are moved one step to the right in one frame. With the simple
algorithm, all contacts will be deassigned, whereas with this
algorithm, the leftmost contact is deassigned and a new contact
created on the right. This is the interpretation of smallest change,
and is consistent with the behavior one would get from two contacts,
for both algorithms.
I have tested this on a number of testcases in my tree, and I am
running the patch with a speed limit on my device as I write this. If
this patch solves Benjamin's problem, I think we are ok.
Henrik
drivers/input/input-mt.c | 31 ---
drivers/input/mouse/alps.c| 2 +-
drivers/input/mouse/bcm5974.c | 2 +-
drivers/input/mouse/cypress_ps2.c | 2 +-
drivers/input/mouse/synaptics.c | 2 +-
drivers/input/touchscreen/pixcir_i2c_ts.c | 2 +-
include/linux/input/mt.h | 3 ++-
7 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index fbe29fc..990509e 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -293,7 +293,7 @@ void input_mt_sync_frame(struct input_dev *dev)
}
EXPORT_SYMBOL(input_mt_sync_frame);
-static int adjust_dual(int *begin, int step, int *end, int eq)
+static int adjust_dual(int *begin, int step, int *end, int eq, int mu)
{
int f, *p, s, c;
@@ -311,9 +311,10 @@ static int adjust_dual(int *begin, int step, int *end, int
eq)
s = *p;
c = (f + s + 1) / 2;
- if (c == 0 || (c 0 !eq))
+ if (c == 0 || (c mu (!eq || mu 0)))
return 0;
- if (s 0)
+ /* Improve convergence for positive matrices by penalizing overcovers */
+ if (s 0 mu = 0)
c *= 2;
for (p = begin; p != end; p += step)
@@ -322,23 +323,24 @@ static int adjust_dual(int *begin, int step, int *end,
int eq)
return (c s s = 0) || (f = 0 f c);
}
-static void find_reduced_matrix(int *w, int nr, int nc, int nrc)
+static void find_reduced_matrix(int *w, int nr, int nc, int nrc, int mu)
{
int i, k, sum;
for (k = 0; k nrc; k++) {
for (i = 0; i nr; i++)
- adjust_dual(w + i, nr, w + i + nrc, nr = nc);
+ adjust_dual(w + i, nr, w + i + nrc, nr = nc, mu);
sum = 0;
for (i = 0; i nrc; i += nr)
- sum += adjust_dual(w + i, 1, w + i + nr, nc = nr);
+ sum += adjust_dual(w + i, 1, w + i + nr, nc = nr, mu);
if (!sum)
break;
}
}
static int input_mt_set_matrix(struct input_mt *mt,
- const struct input_mt_pos *pos, int num_pos)
+ const struct input_mt_pos *pos, int num_pos,
+ int mu)
{
const struct input_mt_pos *p;
struct input_mt_slot *s;
@@ -352,7 +354,7 @@ static int input_mt_set_matrix(struct input_mt *mt,
y = input_mt_get_value(s, ABS_MT_POSITION_Y);
for (p = pos; p != pos + num_pos; p++) {
int dx = x - p-x, dy = y - p-y;
- *w++ = dx * dx + dy * dy;
+ *w++ = dx * dx + dy * dy - mu;
}
}
@@ -393,17 +395,24 @@ static void input_mt_set_slots(struct input_mt *mt,
* @slots: the slot assignment to be filled
* @pos: the position array to match
* @num_pos: number of positions
+ * @dmax: maximum sensor displacement (zero for infinite)
*
* Performs a best match against the current contacts and returns
* the slot assignment list. New contacts are assigned to unused
* slots.
*
+ * The assignments are balanced so that all displacements are below
+ * dmax. If no such assignment can be found, some contacts are
+ * assigned to unused slots.
+ *
* Returns
Re: [PATCH] Input: MT - Add support for balanced slot assignment
Hi Dmitry,
On 01/22/2015 09:02 PM, Dmitry Torokhov wrote:
On Thu, Jan 22, 2015 at 08:52:25PM +0100, Henrik Rydberg wrote:
int input_mt_assign_slots(struct input_dev *dev, int *slots,
- const struct input_mt_pos *pos, int num_pos)
+ const struct input_mt_pos *pos, int num_pos,
+ int dmax)
Should dmax be unsigned and do we really need to treat 0 specially or we
could use UNIT_MAX as don't care value?
We could have dmax unsigned, but it does not have to be from a branching
perspective, since the square is what gets used anyways.
{
struct input_mt *mt = dev-mt;
+int mu = 2 * dmax * dmax;
For my education, what does mu stand for?
I chose mu because of the mathematical similarity to the chemical potential in
statistical mechanics, where it denotes the energy per particle. Here, it
denotes the energy per contact assignment.
Ideally, if someone could create a
write-up on the contact matching that would be most awesome.
Heh, I guess I will have to write something at some point, without requiring
prior knowledge of Lagrange relaxation or the like. Time is a luxury these
days...
Henrik
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MAINTAINERS: Update rydberg's addresses
Hi Benjamin, > - there is a fragmentation problem: we would have to fix the bug in > xorg-synaptics (which is slowly waiting for its death), libinput, > ChromeOS, Qt Embedded, Kivy (I think), etc... Indeed, this is the problem I wanted to highlight. As the fragmentation problem grows (graphics, input, compositors, toolkits), the need for a common denominator grows as well. However, I do not think the kernel should be the single common denominator for all the world's problems. Rather, the purpose of the kernel is to convey hardware information and control as accurately, effectively and generically as possible. > - it means that the mt protocol B can not be relied upon, because even > if we state that each touch has its own slot, then it is false in this > case. The case we are talking about is due to information missing in the hardware. At low enough sampling frequencies, there is no way to distinguish between a moving finger and a lift-and-press action. We could flag this hardware deficiency somehow, but making shit up in order to maintain the statue that we do have enough information is just asking for trouble. I agree that this point is valid: we cannot always trust the interpretation of touchpoints for certain hardware. However, there is nothing we can do about that, except flag for it. > Also, if you compare the libinput implementation of the handling of > the cursors jumps and the kernel implementation I proposed, there is a > big difference in term of simplicity. No, this is wrong. > In the kernel, while we are assigning the tracking IDs, we detect that > there is a jump, and we "just" have to generate a new slot and close > the first (done by 1 assignment of -1 to the current tracking ID). The kernel case would have to be accompanied by parameters, under the control of some user process, where adjustments are made to accomodate different usecases such as painting, gaming, air guitar playing, flick gestures, multi-user tablets, etc, etc. That is complex and unwanted. > In Libinput, well, you receive a slot, there is a jump, you detect it, > then you have to create a new fake kernel event to stop the current > slot, create a new one, and you then have to rewind the current state > of the buttons, the hysteresis, add special case handling and > hopefully, you did not introduced a bug in all the complex code. So > you need to write unit tests (not an argument, I concede, but this is > extra work), and in the future, someone will not understand what this > is all about because the kernel should guarantee that the slots are > sane. You do not need to do any of this (except the test cases, which would be needed anyway given the context-dependent interpretation of scarse data) if you intercept the touch points as they come in from the kernel, before the contact dynamics is fully trusted. Last time I checked that was mtdev or the touch frame layer or Xinput. > If I were grumpy (and I can be, ask Peter), I would say that sure, we > can add such a case in the mtdev library, but the point of having the > in-kernel tracking system was to slowly get away from the head over > added by mtdev. No, this was not the reason. The tree main reasons were actually latency and power and code reduction. The mtdev layer still provides the functional bridge needed. However, the latency and number of cpu cycles involved in transferring the data to userland before throwing most of it away were much reduced by adding the in-kernel tracking. Collecting the diversity of solutions for older hardware was a maintainability bonus. Regarding the practical problem at hand, that double taps sometimes get misinterpreted as a "flash move", perhaps the problem really is in how we define a double tap. When I was writing a certain gesture engine, I realized I got issues with multi-finger taps. In my case the problem was not due to misinterpreted finger data, but simply because pressing four fingers simultaneously is not easy, and really not needed in order to define the gesture per se. So in order to correctly interpret gestures, I had to involve time in various ways; checking for overlaps between fingers over a short time span, looking for the maximum number of fingers within a certain timespan, etc. Not to mention palm detection; in this context, the individual touch points really lose some meaning. The double tap is no different in this regard. Nor is a flash move. But we would not want a real flash move to be interpreted as double tap, would we? My point is that the gesture context is where the ultimate decision can best be made - where there is enough information to best approximate what the imperfect hardware is trying to tell us. If the time between touch down and a fast movement to a nearby point is consistent with a tap, then maybe it is a tap. End rant. I hope it helps one way or the other. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to
Re: [PATCH] MAINTAINERS: Update rydberg's addresses
Hi Peter, > it wasn't the first reaction. it was the third, after we hacked up > synaptics [1], then libinput [2], both rather unsatisfactory. Not sure what > chromeos does but they'll probably would need similar code. And any other > users of the evdev API of course. So if this approach did not work very well in userland, why would it work any better in the kernel? Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MAINTAINERS: Update rydberg's addresses
Hi Benjamin, - there is a fragmentation problem: we would have to fix the bug in xorg-synaptics (which is slowly waiting for its death), libinput, ChromeOS, Qt Embedded, Kivy (I think), etc... Indeed, this is the problem I wanted to highlight. As the fragmentation problem grows (graphics, input, compositors, toolkits), the need for a common denominator grows as well. However, I do not think the kernel should be the single common denominator for all the world's problems. Rather, the purpose of the kernel is to convey hardware information and control as accurately, effectively and generically as possible. - it means that the mt protocol B can not be relied upon, because even if we state that each touch has its own slot, then it is false in this case. The case we are talking about is due to information missing in the hardware. At low enough sampling frequencies, there is no way to distinguish between a moving finger and a lift-and-press action. We could flag this hardware deficiency somehow, but making shit up in order to maintain the statue that we do have enough information is just asking for trouble. I agree that this point is valid: we cannot always trust the interpretation of touchpoints for certain hardware. However, there is nothing we can do about that, except flag for it. Also, if you compare the libinput implementation of the handling of the cursors jumps and the kernel implementation I proposed, there is a big difference in term of simplicity. No, this is wrong. In the kernel, while we are assigning the tracking IDs, we detect that there is a jump, and we just have to generate a new slot and close the first (done by 1 assignment of -1 to the current tracking ID). The kernel case would have to be accompanied by parameters, under the control of some user process, where adjustments are made to accomodate different usecases such as painting, gaming, air guitar playing, flick gestures, multi-user tablets, etc, etc. That is complex and unwanted. In Libinput, well, you receive a slot, there is a jump, you detect it, then you have to create a new fake kernel event to stop the current slot, create a new one, and you then have to rewind the current state of the buttons, the hysteresis, add special case handling and hopefully, you did not introduced a bug in all the complex code. So you need to write unit tests (not an argument, I concede, but this is extra work), and in the future, someone will not understand what this is all about because the kernel should guarantee that the slots are sane. You do not need to do any of this (except the test cases, which would be needed anyway given the context-dependent interpretation of scarse data) if you intercept the touch points as they come in from the kernel, before the contact dynamics is fully trusted. Last time I checked that was mtdev or the touch frame layer or Xinput. If I were grumpy (and I can be, ask Peter), I would say that sure, we can add such a case in the mtdev library, but the point of having the in-kernel tracking system was to slowly get away from the head over added by mtdev. No, this was not the reason. The tree main reasons were actually latency and power and code reduction. The mtdev layer still provides the functional bridge needed. However, the latency and number of cpu cycles involved in transferring the data to userland before throwing most of it away were much reduced by adding the in-kernel tracking. Collecting the diversity of solutions for older hardware was a maintainability bonus. Regarding the practical problem at hand, that double taps sometimes get misinterpreted as a flash move, perhaps the problem really is in how we define a double tap. When I was writing a certain gesture engine, I realized I got issues with multi-finger taps. In my case the problem was not due to misinterpreted finger data, but simply because pressing four fingers simultaneously is not easy, and really not needed in order to define the gesture per se. So in order to correctly interpret gestures, I had to involve time in various ways; checking for overlaps between fingers over a short time span, looking for the maximum number of fingers within a certain timespan, etc. Not to mention palm detection; in this context, the individual touch points really lose some meaning. The double tap is no different in this regard. Nor is a flash move. But we would not want a real flash move to be interpreted as double tap, would we? My point is that the gesture context is where the ultimate decision can best be made - where there is enough information to best approximate what the imperfect hardware is trying to tell us. If the time between touch down and a fast movement to a nearby point is consistent with a tap, then maybe it is a tap. End rant. I hope it helps one way or the other. Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More
Re: [PATCH] MAINTAINERS: Update rydberg's addresses
Hi Peter, it wasn't the first reaction. it was the third, after we hacked up synaptics [1], then libinput [2], both rather unsatisfactory. Not sure what chromeos does but they'll probably would need similar code. And any other users of the evdev API of course. So if this approach did not work very well in userland, why would it work any better in the kernel? Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MAINTAINERS: Update rydberg's addresses
Hi Benjamin, > Henrik, I just used the get_maintainer to add you on CC to an input-mt > patch series, and it ended up using the @euromail.se instead of your > still valid one. I can resend to you the patch series if you want. Thanks, that won't be necessary. I looked through the the patchset, and it strikes me as mostly renames without deeper explanations, which all in all puts them in the not-needed category, I am afraid. An in-depth explanation could be added as a text block somewhere without touching the current code. The only patch that does something is the last one, and what it does could easily be performed in userland, by further processing of the contacts there. I therefore see no use for any of those patches in the kernel. Is there a good reason for the first reaction to be to add special tweaks such as this one in the kernel, rather than in a dedicated userland input system? I am genuinely curious. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] MAINTAINERS: Update rydberg's addresses
Hi Benjamin, Henrik, I just used the get_maintainer to add you on CC to an input-mt patch series, and it ended up using the @euromail.se instead of your still valid one. I can resend to you the patch series if you want. Thanks, that won't be necessary. I looked through the the patchset, and it strikes me as mostly renames without deeper explanations, which all in all puts them in the not-needed category, I am afraid. An in-depth explanation could be added as a text block somewhere without touching the current code. The only patch that does something is the last one, and what it does could easily be performed in userland, by further processing of the contacts there. I therefore see no use for any of those patches in the kernel. Is there a good reason for the first reaction to be to add special tweaks such as this one in the kernel, rather than in a dedicated userland input system? I am genuinely curious. Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] MAINTAINERS: Update rydberg's addresses
My ISP finally gave up on the old mail address, so I am moving things over to bitmath.org instead. Also change the status fields to better reflect reality. Signed-off-by: Henrik Rydberg --- MAINTAINERS | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index c721042..62b53e8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -714,15 +714,15 @@ F:include/uapi/linux/apm_bios.h F: drivers/char/apm-emulation.c APPLE BCM5974 MULTITOUCH DRIVER -M: Henrik Rydberg +M: Henrik Rydberg L: linux-in...@vger.kernel.org -S: Maintained +S: Odd fixes F: drivers/input/mouse/bcm5974.c APPLE SMC DRIVER -M: Henrik Rydberg +M: Henrik Rydberg L: lm-sens...@lm-sensors.org -S: Maintained +S: Odd fixes F: drivers/hwmon/applesmc.c APPLETALK NETWORK LAYER @@ -4813,10 +4813,10 @@ F: include/uapi/linux/input.h F: include/linux/input/ INPUT MULTITOUCH (MT) PROTOCOL -M: Henrik Rydberg +M: Henrik Rydberg L: linux-in...@vger.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/rydberg/input-mt.git -S: Maintained +S: Odd fixes F: Documentation/input/multi-touch-protocol.txt F: drivers/input/input-mt.c K: \b(ABS|SYN)_MT_ -- 2.1.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] MAINTAINERS: Update rydberg's addresses
My ISP finally gave up on the old mail address, so I am moving things over to bitmath.org instead. Also change the status fields to better reflect reality. Signed-off-by: Henrik Rydberg rydb...@bitmath.org --- MAINTAINERS | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/MAINTAINERS b/MAINTAINERS index c721042..62b53e8 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -714,15 +714,15 @@ F:include/uapi/linux/apm_bios.h F: drivers/char/apm-emulation.c APPLE BCM5974 MULTITOUCH DRIVER -M: Henrik Rydberg rydb...@euromail.se +M: Henrik Rydberg rydb...@bitmath.org L: linux-in...@vger.kernel.org -S: Maintained +S: Odd fixes F: drivers/input/mouse/bcm5974.c APPLE SMC DRIVER -M: Henrik Rydberg rydb...@euromail.se +M: Henrik Rydberg rydb...@bitmath.org L: lm-sens...@lm-sensors.org -S: Maintained +S: Odd fixes F: drivers/hwmon/applesmc.c APPLETALK NETWORK LAYER @@ -4813,10 +4813,10 @@ F: include/uapi/linux/input.h F: include/linux/input/ INPUT MULTITOUCH (MT) PROTOCOL -M: Henrik Rydberg rydb...@euromail.se +M: Henrik Rydberg rydb...@bitmath.org L: linux-in...@vger.kernel.org T: git git://git.kernel.org/pub/scm/linux/kernel/git/rydberg/input-mt.git -S: Maintained +S: Odd fixes F: Documentation/input/multi-touch-protocol.txt F: drivers/input/input-mt.c K: \b(ABS|SYN)_MT_ -- 2.1.3 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86, ia64: Do not lose track of the EFI default VGA device
Hi Bruno, > So it would need to at least be select VGA_ARB if (PCI && !S390) > in order to not have broken kernel configuration (in more or less > exotic cases) while depends on VGA_ARB would be the only correct option > if the rule 'select only allowed for leafs' is enforced. Here is a tested patch that does just that, thanks for the suggestion. Henrik >From 43c16bbc7adbcb17aac73d09f046bf2779771c4c Mon Sep 17 00:00:00 2001 From: Henrik Rydberg Date: Fri, 14 Nov 2014 20:01:21 +0100 Subject: [PATCH v2] x86, ia64: Do not lose track of the EFI default VGA device MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since commit 20cde694 in the 3.17 merge window, the EFI framebuffer depends on the VGA arbitration layer. However, the configuration does not reflect this, which leads to a hard-to-find bug when FB_EFI is configured without VGA_ARB. Add a select clause to remedy this. Cc: Bruno Prémont Signed-off-by: Henrik Rydberg --- drivers/video/fbdev/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index c7bf606..1615a1b 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -761,6 +761,7 @@ config FB_EFI select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT + select VGA_ARB if (PCI && !S390) help This is the EFI frame buffer device driver. If the firmware on your platform is EFI 1.10 or UEFI 2.0, select Y to add support for -- 2.1.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86, ia64: Do not lose track of the EFI default VGA device
On 11/14/2014 03:42 PM, Bruno Prémont wrote:
> On Fri, 14 Nov 2014 13:53:30 +0100 Henrik Rydberg wrote:
>> Since commit 20cde694027e ("x86, ia64: Move EFI_FB
>> vga_default_device() initialization to pci_vga_fixup()") in the 3.17
>> merge window, the EFI framebuffer depends on the VGA arbitration
>> layer. However, the configuration does not reflect this, which leads
>> to a hard-to-find bug when FB_EFI is configured without VGA_ARB. Add a
>> select clause to remedy this.
>
> Could you be more verbose in why it depends on/needs VGA_ARB?
When the EFI framebuffer is configured but not VGA_ARB, the kernel manages to
lose track of the default VGA device in some cases. As a result, the X11
nouveau driver fails on my MacBookAir3,1 (GeForce 320M, nv50, 0xaf), which
is booting in EFI_STUB mode.
The code to select the right PCI device was literally moved from efifb.c to the
internals of vgaarb. The PCI sysfs layer seems to depend on
vga_default_device(), which is only defined when VGA_ARB is set.
> With EFI starting to show up on ARM this is not necessarily true
> (no PCI -> no VGA_ARB arbitration).
>
> So it would need to at least be select VGA_ARB if (PCI && !S390)
> in order to not have broken kernel configuration (in more or less
> exotic cases) while depends on VGA_ARB would be the only correct option
> if the rule 'select only allowed for leafs' is enforced.
I agree that the tie probably should be somewhere else. I am fine with any
combination of flags that respects the fact that efifb used to define
vga_default_device(), apparently for good reason.
Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] x86, ia64: Do not lose track of the EFI default VGA device
Since commit 20cde694027e ("x86, ia64: Move EFI_FB
vga_default_device() initialization to pci_vga_fixup()") in the 3.17
merge window, the EFI framebuffer depends on the VGA arbitration
layer. However, the configuration does not reflect this, which leads
to a hard-to-find bug when FB_EFI is configured without VGA_ARB. Add a
select clause to remedy this.
Cc: Bruno Prémont
Signed-off-by: Henrik Rydberg
---
Hi Peter,
I stumbled upon this bug from the 3.17 merge window when updating to
Linus's 3.18 git head yesterday. The patch has been tested on two
different EFI machines; one that needs the patch and one that does not.
Thanks,
Henrik
drivers/video/fbdev/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index c7bf606..81b21bc 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -761,6 +761,7 @@ config FB_EFI
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
+ select VGA_ARB
help
This is the EFI frame buffer device driver. If the firmware on
your platform is EFI 1.10 or UEFI 2.0, select Y to add support for
--
2.1.3
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] x86, ia64: Do not lose track of the EFI default VGA device
Since commit 20cde694027e (x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()) in the 3.17 merge window, the EFI framebuffer depends on the VGA arbitration layer. However, the configuration does not reflect this, which leads to a hard-to-find bug when FB_EFI is configured without VGA_ARB. Add a select clause to remedy this. Cc: Bruno Prémont bonb...@linux-vserver.org Signed-off-by: Henrik Rydberg rydb...@euromail.se --- Hi Peter, I stumbled upon this bug from the 3.17 merge window when updating to Linus's 3.18 git head yesterday. The patch has been tested on two different EFI machines; one that needs the patch and one that does not. Thanks, Henrik drivers/video/fbdev/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index c7bf606..81b21bc 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -761,6 +761,7 @@ config FB_EFI select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT + select VGA_ARB help This is the EFI frame buffer device driver. If the firmware on your platform is EFI 1.10 or UEFI 2.0, select Y to add support for -- 2.1.3 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86, ia64: Do not lose track of the EFI default VGA device
On 11/14/2014 03:42 PM, Bruno Prémont wrote: On Fri, 14 Nov 2014 13:53:30 +0100 Henrik Rydberg wrote: Since commit 20cde694027e (x86, ia64: Move EFI_FB vga_default_device() initialization to pci_vga_fixup()) in the 3.17 merge window, the EFI framebuffer depends on the VGA arbitration layer. However, the configuration does not reflect this, which leads to a hard-to-find bug when FB_EFI is configured without VGA_ARB. Add a select clause to remedy this. Could you be more verbose in why it depends on/needs VGA_ARB? When the EFI framebuffer is configured but not VGA_ARB, the kernel manages to lose track of the default VGA device in some cases. As a result, the X11 nouveau driver fails on my MacBookAir3,1 (GeForce 320M, nv50, 0xaf), which is booting in EFI_STUB mode. The code to select the right PCI device was literally moved from efifb.c to the internals of vgaarb. The PCI sysfs layer seems to depend on vga_default_device(), which is only defined when VGA_ARB is set. With EFI starting to show up on ARM this is not necessarily true (no PCI - no VGA_ARB arbitration). So it would need to at least be select VGA_ARB if (PCI !S390) in order to not have broken kernel configuration (in more or less exotic cases) while depends on VGA_ARB would be the only correct option if the rule 'select only allowed for leafs' is enforced. I agree that the tie probably should be somewhere else. I am fine with any combination of flags that respects the fact that efifb used to define vga_default_device(), apparently for good reason. Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] x86, ia64: Do not lose track of the EFI default VGA device
Hi Bruno, So it would need to at least be select VGA_ARB if (PCI !S390) in order to not have broken kernel configuration (in more or less exotic cases) while depends on VGA_ARB would be the only correct option if the rule 'select only allowed for leafs' is enforced. Here is a tested patch that does just that, thanks for the suggestion. Henrik From 43c16bbc7adbcb17aac73d09f046bf2779771c4c Mon Sep 17 00:00:00 2001 From: Henrik Rydberg rydb...@euromail.se Date: Fri, 14 Nov 2014 20:01:21 +0100 Subject: [PATCH v2] x86, ia64: Do not lose track of the EFI default VGA device MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since commit 20cde694 in the 3.17 merge window, the EFI framebuffer depends on the VGA arbitration layer. However, the configuration does not reflect this, which leads to a hard-to-find bug when FB_EFI is configured without VGA_ARB. Add a select clause to remedy this. Cc: Bruno Prémont bonb...@linux-vserver.org Signed-off-by: Henrik Rydberg rydb...@euromail.se --- drivers/video/fbdev/Kconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig index c7bf606..1615a1b 100644 --- a/drivers/video/fbdev/Kconfig +++ b/drivers/video/fbdev/Kconfig @@ -761,6 +761,7 @@ config FB_EFI select FB_CFB_FILLRECT select FB_CFB_COPYAREA select FB_CFB_IMAGEBLIT + select VGA_ARB if (PCI !S390) help This is the EFI frame buffer device driver. If the firmware on your platform is EFI 1.10 or UEFI 2.0, select Y to add support for -- 2.1.3 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Input: synaptics - properly initialize slots for semi-MT
Hi Dmitry,
On 07/26/2014 02:27 AM, Dmitry Torokhov wrote:
> Semi-MT devices are pointers too, so let's tell that to
> input_mt_init_slots(), as well as let it set up the devices as semi-MT,
> instead of us doing it manually.
>
> Reviewed-by: Daniel Kurtz
> Reviewed-by: Benson Leung
> Signed-off-by: Dmitry Torokhov
> ---
> drivers/input/mouse/synaptics.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
> index ef9e0b8..fe607e9 100644
> --- a/drivers/input/mouse/synaptics.c
> +++ b/drivers/input/mouse/synaptics.c
> @@ -1371,11 +1371,11 @@ static void set_input_params(struct psmouse *psmouse,
> __set_bit(BTN_TOOL_QUADTAP, dev->keybit);
> __set_bit(BTN_TOOL_QUINTTAP, dev->keybit);
> } else if (SYN_CAP_ADV_GESTURE(priv->ext_cap_0c)) {
> - /* Non-image sensors with AGM use semi-mt */
> - __set_bit(INPUT_PROP_SEMI_MT, dev->propbit);
> - input_mt_init_slots(dev, 2, 0);
> set_abs_position_params(dev, priv, ABS_MT_POSITION_X,
> ABS_MT_POSITION_Y);
> + /* Non-image sensors with AGM use semi-mt */
> + input_mt_init_slots(dev, 2,
> + INPUT_MT_POINTER | INPUT_MT_SEMI_MT);
> }
As long as DOUBLE_TAP is set unconditionally in this path anyways, it should be
ok. I have a vague memory of refraining from this exact change at some point,
because of the many branches of settings.
>
> if (SYN_CAP_PALMDETECT(priv->capabilities))
>
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Input: synaptics - properly initialize slots for semi-MT
Hi Dmitry,
On 07/26/2014 02:27 AM, Dmitry Torokhov wrote:
Semi-MT devices are pointers too, so let's tell that to
input_mt_init_slots(), as well as let it set up the devices as semi-MT,
instead of us doing it manually.
Reviewed-by: Daniel Kurtz djku...@chromium.org
Reviewed-by: Benson Leung ble...@chromium.org
Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com
---
drivers/input/mouse/synaptics.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index ef9e0b8..fe607e9 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -1371,11 +1371,11 @@ static void set_input_params(struct psmouse *psmouse,
__set_bit(BTN_TOOL_QUADTAP, dev-keybit);
__set_bit(BTN_TOOL_QUINTTAP, dev-keybit);
} else if (SYN_CAP_ADV_GESTURE(priv-ext_cap_0c)) {
- /* Non-image sensors with AGM use semi-mt */
- __set_bit(INPUT_PROP_SEMI_MT, dev-propbit);
- input_mt_init_slots(dev, 2, 0);
set_abs_position_params(dev, priv, ABS_MT_POSITION_X,
ABS_MT_POSITION_Y);
+ /* Non-image sensors with AGM use semi-mt */
+ input_mt_init_slots(dev, 2,
+ INPUT_MT_POINTER | INPUT_MT_SEMI_MT);
}
As long as DOUBLE_TAP is set unconditionally in this path anyways, it should be
ok. I have a vague memory of refraining from this exact change at some point,
because of the many branches of settings.
if (SYN_CAP_PALMDETECT(priv-capabilities))
Henrik
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Input: fix defuzzing logic
Hi Dmitry, > We attempt to remove noise from coordinates reported by devices in > input_handle_abs_event(), unfortunately, unless we were dropping the > event altogether, we were ignoring the adjusted value and were passing > on the original value instead. > > Reviewed-by: Andrew de los Reyes > Reviewed-by: Benson Leung > Signed-off-by: Dmitry Torokhov > --- > drivers/input/input.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) Oh my, that was a bad one. Reviewed-by: Henrik Rydberg Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Input: fix defuzzing logic
Hi Dmitry, We attempt to remove noise from coordinates reported by devices in input_handle_abs_event(), unfortunately, unless we were dropping the event altogether, we were ignoring the adjusted value and were passing on the original value instead. Reviewed-by: Andrew de los Reyes a...@chromium.org Reviewed-by: Benson Leung ble...@chromium.org Signed-off-by: Dmitry Torokhov dmitry.torok...@gmail.com --- drivers/input/input.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) Oh my, that was a bad one. Reviewed-by: Henrik Rydberg rydb...@euromail.se Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/7] Input: pixcir_i2c_ts: Use Type-B Multi-Touch protocol
TION_X, touch->x);
> + input_event(ts->input, EV_ABS, ABS_MT_POSITION_Y, touch->y);
> +
> + dev_dbg(dev, "%d: slot %d, x %d, y %d\n",
> + i, slot, touch->x, touch->y);
> + }
> +
> + input_mt_sync_frame(ts->input);
> + input_sync(ts->input);
> }
>
> static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
> {
> struct pixcir_i2c_ts_data *tsdata = dev_id;
> const struct pixcir_ts_platform_data *pdata = tsdata->chip;
> + struct pixcir_report_data report;
>
> while (tsdata->running) {
> - pixcir_ts_poscheck(tsdata);
> -
> - if (gpio_get_value(pdata->gpio_attb))
> + /* parse packet */
> + pixcir_ts_parse(tsdata, );
> +
> + /* report it */
> + pixcir_ts_report(tsdata, );
> +
> + if (gpio_get_value(pdata->gpio_attb)) {
> + if (report.num_touches) {
> + /*
> + * Last report with no finger up?
> + * Do it now then.
> + */
> + input_mt_sync_frame(tsdata->input);
> + input_sync(tsdata->input);
I think this construct is alright for this particular patch. If anything, it
points at a need for a better model of interrupted contacts in the core.
> + }
> break;
> + }
>
> msleep(20);
> }
> @@ -333,6 +389,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
> input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata->x_max, 0, 0);
> input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata->y_max, 0, 0);
>
> + error = input_mt_init_slots(input, PIXCIR_MAX_SLOTS,
> + INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
> + if (error) {
> + dev_err(dev, "Error initializing Multi-Touch slots\n");
> + return error;
> + }
> +
> input_set_drvdata(input, tsdata);
>
> error = devm_gpio_request_one(dev, pdata->gpio_attb,
>
Reviewed-by: Henrik Rydberg
Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v5 4/7] Input: pixcir_i2c_ts: Use Type-B Multi-Touch protocol
) {
- pixcir_ts_poscheck(tsdata);
-
- if (gpio_get_value(pdata-gpio_attb))
+ /* parse packet */
+ pixcir_ts_parse(tsdata, report);
+
+ /* report it */
+ pixcir_ts_report(tsdata, report);
+
+ if (gpio_get_value(pdata-gpio_attb)) {
+ if (report.num_touches) {
+ /*
+ * Last report with no finger up?
+ * Do it now then.
+ */
+ input_mt_sync_frame(tsdata-input);
+ input_sync(tsdata-input);
I think this construct is alright for this particular patch. If anything, it
points at a need for a better model of interrupted contacts in the core.
+ }
break;
+ }
msleep(20);
}
@@ -333,6 +389,13 @@ static int pixcir_i2c_ts_probe(struct i2c_client *client,
input_set_abs_params(input, ABS_MT_POSITION_X, 0, pdata-x_max, 0, 0);
input_set_abs_params(input, ABS_MT_POSITION_Y, 0, pdata-y_max, 0, 0);
+ error = input_mt_init_slots(input, PIXCIR_MAX_SLOTS,
+ INPUT_MT_DIRECT | INPUT_MT_DROP_UNUSED);
+ if (error) {
+ dev_err(dev, Error initializing Multi-Touch slots\n);
+ return error;
+ }
+
input_set_drvdata(input, tsdata);
error = devm_gpio_request_one(dev, pdata-gpio_attb,
Reviewed-by: Henrik Rydberg rydb...@euromail.se
Thanks,
Henrik
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/8] Input: pixcir_i2c_ts: Use Type-B Multi-Touch protocol
Hi Roger,
the MT implementation seems mostly fine, just one curiosity:
> static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
> {
> struct pixcir_i2c_ts_data *tsdata = dev_id;
> const struct pixcir_ts_platform_data *pdata = tsdata->chip;
> + struct pixcir_report_data report;
>
> while (!tsdata->exiting) {
> - pixcir_ts_poscheck(tsdata);
> -
> - if (gpio_get_value(pdata->gpio_attb))
> + /* parse packet */
> + pixcir_ts_parse(tsdata, );
> +
> + /* report it */
> + pixcir_ts_report(tsdata, );
> +
> + if (gpio_get_value(pdata->gpio_attb)) {
> + if (report.num_touches) {
> + /*
> + * Last report with no finger up?
> + * Do it now then.
> + */
> + input_mt_sync_frame(tsdata->input);
> + input_sync(tsdata->input);
Why is this special handling needed?
Thanks,
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 4/8] Input: pixcir_i2c_ts: Use Type-B Multi-Touch protocol
Hi Roger,
the MT implementation seems mostly fine, just one curiosity:
static irqreturn_t pixcir_ts_isr(int irq, void *dev_id)
{
struct pixcir_i2c_ts_data *tsdata = dev_id;
const struct pixcir_ts_platform_data *pdata = tsdata-chip;
+ struct pixcir_report_data report;
while (!tsdata-exiting) {
- pixcir_ts_poscheck(tsdata);
-
- if (gpio_get_value(pdata-gpio_attb))
+ /* parse packet */
+ pixcir_ts_parse(tsdata, report);
+
+ /* report it */
+ pixcir_ts_report(tsdata, report);
+
+ if (gpio_get_value(pdata-gpio_attb)) {
+ if (report.num_touches) {
+ /*
+ * Last report with no finger up?
+ * Do it now then.
+ */
+ input_mt_sync_frame(tsdata-input);
+ input_sync(tsdata-input);
Why is this special handling needed?
Thanks,
Henrik
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] input: mt: Add helper function to send end events
Hi Dmitry, >> What problematic scenario is this supposed to solve? >> >> The 'release on suspend' is only an approximation to the 'close >> laptop' scenario, it is certainly not correct in the coffee table >> case, > > Why would it not be correct for coffee table? Do we expect the touches > to remain valid while the device is asleep? In some scenarios with placed objects (like a cup or a marker), that would be the case, yes. >> for instance. Instead of >> hardcoding this in the kernel, userland can easily detect long intervals >> directly using the event time. > > But with slots it is not only problem with old events being sent out > later, it is that we have mix of old (pre-sleep) and new state. The new state is not really a problem, it will look exactly the same regardless of how 'old' releases are handled. > We do that for keys (release them when we transition to system sleep) > and I think it might be worthwhile to do the same for touch data. I agree that keyboard applications seldom look at time intervals and hence are well helped by such a strategy, even though it is not strictly 'correct'. However, touch interfaces are quite dependent on time intervals, and it therefore makes a lot of sense to resolve touch timeouts directly in the application. It also puts less restrictions on what can be done. Regarding notifications in general, perhaps it would be interesting to be able to send a notification event via the input interface when a device comes back from sleep. It could help resolve other complex issues, if there were any. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] input: mt: Add helper function to send end events
Hi Dmitry, What problematic scenario is this supposed to solve? The 'release on suspend' is only an approximation to the 'close laptop' scenario, it is certainly not correct in the coffee table case, Why would it not be correct for coffee table? Do we expect the touches to remain valid while the device is asleep? In some scenarios with placed objects (like a cup or a marker), that would be the case, yes. for instance. Instead of hardcoding this in the kernel, userland can easily detect long intervals directly using the event time. But with slots it is not only problem with old events being sent out later, it is that we have mix of old (pre-sleep) and new state. The new state is not really a problem, it will look exactly the same regardless of how 'old' releases are handled. We do that for keys (release them when we transition to system sleep) and I think it might be worthwhile to do the same for touch data. I agree that keyboard applications seldom look at time intervals and hence are well helped by such a strategy, even though it is not strictly 'correct'. However, touch interfaces are quite dependent on time intervals, and it therefore makes a lot of sense to resolve touch timeouts directly in the application. It also puts less restrictions on what can be done. Regarding notifications in general, perhaps it would be interesting to be able to send a notification event via the input interface when a device comes back from sleep. It could help resolve other complex issues, if there were any. Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] input: mt: Add helper function to send end events
Hi Felipe, > Adds a helper function to send end touch events for active tracking ids. > And also implements it in egalax driver. What problematic scenario is this supposed to solve? The 'release on suspend' is only an approximation to the 'close laptop' scenario, it is certainly not correct in the coffee table case, for instance. Instead of hardcoding this in the kernel, userland can easily detect long intervals directly using the event time. Unless there is already a reasonable mechanism in your user application which deals with cases like this but fails to account for your particular problem scenario, the patch will not be applied. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 0/2] input: mt: Add helper function to send end events
Hi Felipe, Adds a helper function to send end touch events for active tracking ids. And also implements it in egalax driver. What problematic scenario is this supposed to solve? The 'release on suspend' is only an approximation to the 'close laptop' scenario, it is certainly not correct in the coffee table case, for instance. Instead of hardcoding this in the kernel, userland can easily detect long intervals directly using the event time. Unless there is already a reasonable mechanism in your user application which deals with cases like this but fails to account for your particular problem scenario, the patch will not be applied. Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 3/3] HID: multitouch: add support of other generic collections in hid-mt
Hi Benjamin, > The ANTEC Touch Pad is a device which can switch from a multitouch > touchpad to a mouse. It thus presents several generic collections which > are currently ignored by hid-multitouch. Enable them by using the generic > protocol. Adding also a suffix for them depending on their application. In what way does this and the preceeding patches differ from "else if (is_pen || export_all_inputs)"? Adding a functional pattern which then is converted to a generic case, such that the usual branching is bound to occur anyways, seems unnecessary. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/9] Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting
Hi Roger, > It seems the controller the original driver was written for does not report > touch ID and just reports 2 touch co-ordinates. I'm not sure how this fits > into > Type B reporting model. Look in drivers/input/mouse/cypress_ps2.c for an example of how to deal with anonymous touch coordinates. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 7/9] Input: pixcir_i2c_ts: Implement Type B Multi Touch reporting
Hi Roger, It seems the controller the original driver was written for does not report touch ID and just reports 2 touch co-ordinates. I'm not sure how this fits into Type B reporting model. Look in drivers/input/mouse/cypress_ps2.c for an example of how to deal with anonymous touch coordinates. Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [patch 3/3] HID: multitouch: add support of other generic collections in hid-mt
Hi Benjamin, The ANTEC Touch Pad is a device which can switch from a multitouch touchpad to a mouse. It thus presents several generic collections which are currently ignored by hid-multitouch. Enable them by using the generic protocol. Adding also a suffix for them depending on their application. In what way does this and the preceeding patches differ from else if (is_pen || export_all_inputs)? Adding a functional pattern which then is converted to a generic case, such that the usual branching is bound to occur anyways, seems unnecessary. Thanks, Henrik -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: small regression: hwmon: (applesmc) Check key count before proceeding - 5f4513864304672e6ea9eac60583eeac32e679f2
Hi Chris, > Well, it seems to be a another one off event. It's the same hardware as before, and it was booting from a USB stick containing Fedora 20 final test candidate 2 which uses kernel 3.11.8. An immediate reboot did not reproduce the problem, nor multiple subsequent reboots. I think I previously mentioned a preponderance of these events happen when booting from USB sticks. Ok, thanks, that makes sense. So at least one problem is still there, but possibly very difficult to hit. > It would be nice to have an identical model for this testing. I suspect most > users wouldn't go to the trouble to report the occasional, seemingly one off, events like this. So unfortunately it's uncertain if the hardware I have has a unique problem, or if it's a model specific behavior. I will keep this in mind next time I see this particular model. It is also possible that the SMC needs to be reset after having testing various more or less successful patches. Given the tiny cross-section, it looks like this one can rest for now. Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: small regression: hwmon: (applesmc) Check key count before proceeding - 5f4513864304672e6ea9eac60583eeac32e679f2
Hi Michele, > The issue Chris has seen in Fedora on one MacBookPro4,1 > (https://bugzilla.redhat.com/show_bug.cgi?id=1033414) is that this > machine returns a huge number from read_register_count() so now we will > try to allocate an insane amount of memory and we will barf: > [8.603053] applesmc: key count changed from 261 to 1392508929 I was under the impression that this machine was tested before, and that commit 25f2bd7f5add608c1d1405938f39c96927b275ca Author: Henrik Rydberg Date: Wed Oct 2 19:15:03 2013 +0200 hwmon: (applesmc) Always read until end of data resolved this problem? But if the kernel under test is 3.11.8, both patches are already present... Chris, could you please sched some light on this before moving on? Thanks, Henrik -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
