* [PATCH V4] Input: synaptics-rmi4 - filter incomplete relative packet.
@ 2022-08-16 9:20 margeyang
2022-08-17 14:11 ` Hans de Goede
0 siblings, 1 reply; 3+ messages in thread
From: margeyang @ 2022-08-16 9:20 UTC (permalink / raw)
To: dmitry.torokhov, linux-input, linux-kernel, hdegoede, benjamin.tissoires
Cc: marge.yang, derek.cheng, vincent.huang, Marge Yang
From: Marge Yang <marge.yang@synaptics.corp-partner.google.com>
RMI4 F03 supports the Stick function,
it's designed to support relative packet.
This patch supports the following case.
When relative packet can't be reported completely,
it may miss one byte or two byte.
New Synaptics firmware will report PARITY error.
When timeout error or parity error happens,
RMI4 driver will sends 0xFE command and
ask FW to Re-send stick packet again.
Signed-off-by: Marge Yang<marge.yang@synaptics.corp-partner.google.com>
---
drivers/input/rmi4/rmi_f03.c | 74 +++++++++++++++++++++++++++++++++++-
1 file changed, 73 insertions(+), 1 deletion(-)
diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c
index c194b1664b10..563b40c2dc06 100644
--- a/drivers/input/rmi4/rmi_f03.c
+++ b/drivers/input/rmi4/rmi_f03.c
@@ -23,8 +23,12 @@
#define RMI_F03_BYTES_PER_DEVICE_SHIFT 4
#define RMI_F03_QUEUE_LENGTH 0x0F
+#define RMI_F03_RESET_STYK 0xFE
+
#define PSMOUSE_OOB_EXTRA_BTNS 0x01
+#define RELATIVE_PACKET_SIZE 3
+
struct f03_data {
struct rmi_function *fn;
@@ -33,6 +37,11 @@ struct f03_data {
unsigned int overwrite_buttons;
+ int iwritecommandcounter;
+ unsigned int ipacketindex;
+ unsigned int serio_flagsArry[RELATIVE_PACKET_SIZE];
+ u8 ob_dataArry[RELATIVE_PACKET_SIZE];
+
u8 device_count;
u8 rx_queue_length;
};
@@ -88,6 +97,7 @@ static int rmi_f03_pt_write(struct serio *id, unsigned char val)
return error;
}
+ f03->iwritecommandcounter++;
return 0;
}
@@ -107,6 +117,8 @@ static int rmi_f03_initialize(struct f03_data *f03)
return error;
}
+ f03->iwritecommandcounter = 0;
+ f03->ipacketindex = 0;
f03->device_count = query1 & RMI_F03_DEVICE_COUNT;
bytes_per_device = (query1 >> RMI_F03_BYTES_PER_DEVICE_SHIFT) &
RMI_F03_BYTES_PER_DEVICE;
@@ -284,6 +296,22 @@ static irqreturn_t rmi_f03_attention(int irq, void *ctx)
ob_data = obs[i + RMI_F03_OB_DATA_OFFSET];
serio_flags = 0;
+ if (ob_status & (RMI_F03_OB_FLAG_TIMEOUT | RMI_F03_OB_FLAG_PARITY)) {
+ // Send resend command to stick when timeout or parity error.
+ // Driver can receive the last stick packet.
+ unsigned char val = RMI_F03_RESET_STYK;
+
+ error = rmi_write(f03->fn->rmi_dev, f03->fn->fd.data_base_addr, val);
+ if (error) {
+ dev_err(&f03->fn->dev,
+ "%s: Failed to rmi_write to F03 TX register (%d).\n",
+ __func__, error);
+ return error;
+ }
+ f03->ipacketindex = 0;
+ break;
+ }
+
if (!(ob_status & RMI_F03_RX_DATA_OFB))
continue;
@@ -298,7 +326,51 @@ static irqreturn_t rmi_f03_attention(int irq, void *ctx)
serio_flags & SERIO_TIMEOUT ? 'Y' : 'N',
serio_flags & SERIO_PARITY ? 'Y' : 'N');
- serio_interrupt(f03->serio, ob_data, serio_flags);
+ if (f03->iwritecommandcounter > 0) {
+ // Read Acknowledge Byte after writing the PS2 command.
+ // It is not trackpoint data.
+ serio_interrupt(f03->serio, ob_data, serio_flags);
+ } else {
+ // The relative-mode PS/2 packet format is as follows:
+ //
+ // bit position position (as array of bytes)
+ // 7 6 5 4 3 2 1 0
+ // =================================+
+ // Yov Xov DY8 DX8 1 M R L | DATA[0]
+ // DX[7:0] | DATA[1]
+ // DY[7:0] | DATA[2]
+ // =================================+
+ // Yov: Y overflow
+ // Xov: X overflow
+ if ((f03->ipacketindex == 0) && (ob_data & ((BIT(7)|BIT(6))))) {
+ dev_err(&f03->fn->dev,
+ "%s: X or Y is overflow. (%x)\n",
+ __func__, ob_data);
+ break;
+ } else if ((f03->ipacketindex == 0) && !(ob_data & BIT(3))) {
+ dev_err(&f03->fn->dev,
+ "%s: New BIT 3 is not 1 for the first byte\n",
+ __func__);
+ break;
+ }
+ f03->ob_dataArry[f03->ipacketindex] = ob_data;
+ f03->serio_flagsArry[f03->ipacketindex] = serio_flags;
+ f03->ipacketindex++;
+
+ if (f03->ipacketindex == RELATIVE_PACKET_SIZE) {
+ serio_interrupt(f03->serio, f03->ob_dataArry[0],
+ f03->serio_flagsArry[0]);
+ serio_interrupt(f03->serio, f03->ob_dataArry[1],
+ f03->serio_flagsArry[1]);
+ serio_interrupt(f03->serio, f03->ob_dataArry[2],
+ f03->serio_flagsArry[2]);
+ f03->ipacketindex = 0;
+ }
+ }
+ }
+ if (f03->iwritecommandcounter > 0) {
+ f03->ipacketindex = 0;
+ f03->iwritecommandcounter = f03->iwritecommandcounter - 1;
}
return IRQ_HANDLED;
--
2.22.0.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH V4] Input: synaptics-rmi4 - filter incomplete relative packet.
2022-08-16 9:20 [PATCH V4] Input: synaptics-rmi4 - filter incomplete relative packet margeyang
@ 2022-08-17 14:11 ` Hans de Goede
2022-11-04 10:29 ` Marge Yang
0 siblings, 1 reply; 3+ messages in thread
From: Hans de Goede @ 2022-08-17 14:11 UTC (permalink / raw)
To: margeyang, dmitry.torokhov, linux-input, linux-kernel,
benjamin.tissoires
Cc: marge.yang, derek.cheng, vincent.huang
Hi,
On 8/16/22 11:20, margeyang wrote:
> From: Marge Yang <marge.yang@synaptics.corp-partner.google.com>
>
> RMI4 F03 supports the Stick function,
> it's designed to support relative packet.
> This patch supports the following case.
> When relative packet can't be reported completely,
> it may miss one byte or two byte.
> New Synaptics firmware will report PARITY error.
> When timeout error or parity error happens,
> RMI4 driver will sends 0xFE command and
> ask FW to Re-send stick packet again.
>
> Signed-off-by: Marge Yang<marge.yang@synaptics.corp-partner.google.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
note I see that Dmitry still has questions about this, so my
Reviewed-by is in no way a guarantee that this will get merged.
Please make sure to answer Dmitry's questions about this.
Regards,
Hans
> ---
> drivers/input/rmi4/rmi_f03.c | 74 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/rmi4/rmi_f03.c b/drivers/input/rmi4/rmi_f03.c
> index c194b1664b10..563b40c2dc06 100644
> --- a/drivers/input/rmi4/rmi_f03.c
> +++ b/drivers/input/rmi4/rmi_f03.c
> @@ -23,8 +23,12 @@
> #define RMI_F03_BYTES_PER_DEVICE_SHIFT 4
> #define RMI_F03_QUEUE_LENGTH 0x0F
>
> +#define RMI_F03_RESET_STYK 0xFE
> +
> #define PSMOUSE_OOB_EXTRA_BTNS 0x01
>
> +#define RELATIVE_PACKET_SIZE 3
> +
> struct f03_data {
> struct rmi_function *fn;
>
> @@ -33,6 +37,11 @@ struct f03_data {
>
> unsigned int overwrite_buttons;
>
> + int iwritecommandcounter;
> + unsigned int ipacketindex;
> + unsigned int serio_flagsArry[RELATIVE_PACKET_SIZE];
> + u8 ob_dataArry[RELATIVE_PACKET_SIZE];
> +
> u8 device_count;
> u8 rx_queue_length;
> };
> @@ -88,6 +97,7 @@ static int rmi_f03_pt_write(struct serio *id, unsigned char val)
> return error;
> }
>
> + f03->iwritecommandcounter++;
> return 0;
> }
>
> @@ -107,6 +117,8 @@ static int rmi_f03_initialize(struct f03_data *f03)
> return error;
> }
>
> + f03->iwritecommandcounter = 0;
> + f03->ipacketindex = 0;
> f03->device_count = query1 & RMI_F03_DEVICE_COUNT;
> bytes_per_device = (query1 >> RMI_F03_BYTES_PER_DEVICE_SHIFT) &
> RMI_F03_BYTES_PER_DEVICE;
> @@ -284,6 +296,22 @@ static irqreturn_t rmi_f03_attention(int irq, void *ctx)
> ob_data = obs[i + RMI_F03_OB_DATA_OFFSET];
> serio_flags = 0;
>
> + if (ob_status & (RMI_F03_OB_FLAG_TIMEOUT | RMI_F03_OB_FLAG_PARITY)) {
> + // Send resend command to stick when timeout or parity error.
> + // Driver can receive the last stick packet.
> + unsigned char val = RMI_F03_RESET_STYK;
> +
> + error = rmi_write(f03->fn->rmi_dev, f03->fn->fd.data_base_addr, val);
> + if (error) {
> + dev_err(&f03->fn->dev,
> + "%s: Failed to rmi_write to F03 TX register (%d).\n",
> + __func__, error);
> + return error;
> + }
> + f03->ipacketindex = 0;
> + break;
> + }
> +
> if (!(ob_status & RMI_F03_RX_DATA_OFB))
> continue;
>
> @@ -298,7 +326,51 @@ static irqreturn_t rmi_f03_attention(int irq, void *ctx)
> serio_flags & SERIO_TIMEOUT ? 'Y' : 'N',
> serio_flags & SERIO_PARITY ? 'Y' : 'N');
>
> - serio_interrupt(f03->serio, ob_data, serio_flags);
> + if (f03->iwritecommandcounter > 0) {
> + // Read Acknowledge Byte after writing the PS2 command.
> + // It is not trackpoint data.
> + serio_interrupt(f03->serio, ob_data, serio_flags);
> + } else {
> + // The relative-mode PS/2 packet format is as follows:
> + //
> + // bit position position (as array of bytes)
> + // 7 6 5 4 3 2 1 0
> + // =================================+
> + // Yov Xov DY8 DX8 1 M R L | DATA[0]
> + // DX[7:0] | DATA[1]
> + // DY[7:0] | DATA[2]
> + // =================================+
> + // Yov: Y overflow
> + // Xov: X overflow
> + if ((f03->ipacketindex == 0) && (ob_data & ((BIT(7)|BIT(6))))) {
> + dev_err(&f03->fn->dev,
> + "%s: X or Y is overflow. (%x)\n",
> + __func__, ob_data);
> + break;
> + } else if ((f03->ipacketindex == 0) && !(ob_data & BIT(3))) {
> + dev_err(&f03->fn->dev,
> + "%s: New BIT 3 is not 1 for the first byte\n",
> + __func__);
> + break;
> + }
> + f03->ob_dataArry[f03->ipacketindex] = ob_data;
> + f03->serio_flagsArry[f03->ipacketindex] = serio_flags;
> + f03->ipacketindex++;
> +
> + if (f03->ipacketindex == RELATIVE_PACKET_SIZE) {
> + serio_interrupt(f03->serio, f03->ob_dataArry[0],
> + f03->serio_flagsArry[0]);
> + serio_interrupt(f03->serio, f03->ob_dataArry[1],
> + f03->serio_flagsArry[1]);
> + serio_interrupt(f03->serio, f03->ob_dataArry[2],
> + f03->serio_flagsArry[2]);
> + f03->ipacketindex = 0;
> + }
> + }
> + }
> + if (f03->iwritecommandcounter > 0) {
> + f03->ipacketindex = 0;
> + f03->iwritecommandcounter = f03->iwritecommandcounter - 1;
> }
>
> return IRQ_HANDLED;
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH V4] Input: synaptics-rmi4 - filter incomplete relative packet.
2022-08-17 14:11 ` Hans de Goede
@ 2022-11-04 10:29 ` Marge Yang
0 siblings, 0 replies; 3+ messages in thread
From: Marge Yang @ 2022-11-04 10:29 UTC (permalink / raw)
To: Hans de Goede, margeyang, dmitry.torokhov, linux-input,
linux-kernel, benjamin.tissoires
Cc: Derek Cheng, Vincent Huang, Wayne Chang, Kevin Chu
Hi Dmitry,
Update Synaptics firmware's comment.
To address the transaction error case in the middle would potential lead to the unexpected data transaction and latency between styk device and host driver. F$03 didn't really parse any data packet but simply works as bridge functionality to bypass the command and response packets between styk device and driver.
Thanks
Marge Yang
-----Original Message-----
From: Hans de Goede <hdegoede@redhat.com>
Sent: Wednesday, August 17, 2022 10:12 PM
To: margeyang <marge.yang@synaptics.corp-partner.google.com>; dmitry.torokhov@gmail.com; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; benjamin.tissoires@redhat.com
Cc: Marge Yang <Marge.Yang@tw.synaptics.com>; Derek Cheng <derek.cheng@tw.synaptics.com>; Vincent Huang <Vincent.huang@tw.synaptics.com>
Subject: Re: [PATCH V4] Input: synaptics-rmi4 - filter incomplete relative packet.
CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe.
Hi,
On 8/16/22 11:20, margeyang wrote:
> From: Marge Yang <marge.yang@synaptics.corp-partner.google.com>
>
> RMI4 F03 supports the Stick function,
> it's designed to support relative packet.
> This patch supports the following case.
> When relative packet can't be reported completely, it may miss one
> byte or two byte.
> New Synaptics firmware will report PARITY error.
> When timeout error or parity error happens,
> RMI4 driver will sends 0xFE command and ask FW to Re-send stick packet
> again.
>
> Signed-off-by: Marge
> Yang<marge.yang@synaptics.corp-partner.google.com>
Thanks, patch looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
note I see that Dmitry still has questions about this, so my Reviewed-by is in no way a guarantee that this will get merged.
Please make sure to answer Dmitry's questions about this.
Regards,
Hans
> ---
> drivers/input/rmi4/rmi_f03.c | 74
> +++++++++++++++++++++++++++++++++++-
> 1 file changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/rmi4/rmi_f03.c
> b/drivers/input/rmi4/rmi_f03.c index c194b1664b10..563b40c2dc06 100644
> --- a/drivers/input/rmi4/rmi_f03.c
> +++ b/drivers/input/rmi4/rmi_f03.c
> @@ -23,8 +23,12 @@
> #define RMI_F03_BYTES_PER_DEVICE_SHIFT 4
> #define RMI_F03_QUEUE_LENGTH 0x0F
>
> +#define RMI_F03_RESET_STYK 0xFE
> +
> #define PSMOUSE_OOB_EXTRA_BTNS 0x01
>
> +#define RELATIVE_PACKET_SIZE 3
> +
> struct f03_data {
> struct rmi_function *fn;
>
> @@ -33,6 +37,11 @@ struct f03_data {
>
> unsigned int overwrite_buttons;
>
> + int iwritecommandcounter;
> + unsigned int ipacketindex;
> + unsigned int serio_flagsArry[RELATIVE_PACKET_SIZE];
> + u8 ob_dataArry[RELATIVE_PACKET_SIZE];
> +
> u8 device_count;
> u8 rx_queue_length;
> };
> @@ -88,6 +97,7 @@ static int rmi_f03_pt_write(struct serio *id, unsigned char val)
> return error;
> }
>
> + f03->iwritecommandcounter++;
> return 0;
> }
>
> @@ -107,6 +117,8 @@ static int rmi_f03_initialize(struct f03_data *f03)
> return error;
> }
>
> + f03->iwritecommandcounter = 0;
> + f03->ipacketindex = 0;
> f03->device_count = query1 & RMI_F03_DEVICE_COUNT;
> bytes_per_device = (query1 >> RMI_F03_BYTES_PER_DEVICE_SHIFT) &
> RMI_F03_BYTES_PER_DEVICE; @@ -284,6
> +296,22 @@ static irqreturn_t rmi_f03_attention(int irq, void *ctx)
> ob_data = obs[i + RMI_F03_OB_DATA_OFFSET];
> serio_flags = 0;
>
> + if (ob_status & (RMI_F03_OB_FLAG_TIMEOUT | RMI_F03_OB_FLAG_PARITY)) {
> + // Send resend command to stick when timeout or parity error.
> + // Driver can receive the last stick packet.
> + unsigned char val = RMI_F03_RESET_STYK;
> +
> + error = rmi_write(f03->fn->rmi_dev, f03->fn->fd.data_base_addr, val);
> + if (error) {
> + dev_err(&f03->fn->dev,
> + "%s: Failed to rmi_write to F03 TX register (%d).\n",
> + __func__, error);
> + return error;
> + }
> + f03->ipacketindex = 0;
> + break;
> + }
> +
> if (!(ob_status & RMI_F03_RX_DATA_OFB))
> continue;
>
> @@ -298,7 +326,51 @@ static irqreturn_t rmi_f03_attention(int irq, void *ctx)
> serio_flags & SERIO_TIMEOUT ? 'Y' : 'N',
> serio_flags & SERIO_PARITY ? 'Y' : 'N');
>
> - serio_interrupt(f03->serio, ob_data, serio_flags);
> + if (f03->iwritecommandcounter > 0) {
> + // Read Acknowledge Byte after writing the PS2 command.
> + // It is not trackpoint data.
> + serio_interrupt(f03->serio, ob_data, serio_flags);
> + } else {
> + // The relative-mode PS/2 packet format is as follows:
> + //
> + // bit position position (as array of bytes)
> + // 7 6 5 4 3 2 1 0
> + // =================================+
> + // Yov Xov DY8 DX8 1 M R L | DATA[0]
> + // DX[7:0] | DATA[1]
> + // DY[7:0] | DATA[2]
> + // =================================+
> + // Yov: Y overflow
> + // Xov: X overflow
> + if ((f03->ipacketindex == 0) && (ob_data & ((BIT(7)|BIT(6))))) {
> + dev_err(&f03->fn->dev,
> + "%s: X or Y is overflow. (%x)\n",
> + __func__, ob_data);
> + break;
> + } else if ((f03->ipacketindex == 0) && !(ob_data & BIT(3))) {
> + dev_err(&f03->fn->dev,
> + "%s: New BIT 3 is not 1 for the first byte\n",
> + __func__);
> + break;
> + }
> + f03->ob_dataArry[f03->ipacketindex] = ob_data;
> + f03->serio_flagsArry[f03->ipacketindex] = serio_flags;
> + f03->ipacketindex++;
> +
> + if (f03->ipacketindex == RELATIVE_PACKET_SIZE) {
> + serio_interrupt(f03->serio, f03->ob_dataArry[0],
> + f03->serio_flagsArry[0]);
> + serio_interrupt(f03->serio, f03->ob_dataArry[1],
> + f03->serio_flagsArry[1]);
> + serio_interrupt(f03->serio, f03->ob_dataArry[2],
> + f03->serio_flagsArry[2]);
> + f03->ipacketindex = 0;
> + }
> + }
> + }
> + if (f03->iwritecommandcounter > 0) {
> + f03->ipacketindex = 0;
> + f03->iwritecommandcounter = f03->iwritecommandcounter -
> + 1;
> }
>
> return IRQ_HANDLED;
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-11-04 10:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-16 9:20 [PATCH V4] Input: synaptics-rmi4 - filter incomplete relative packet margeyang
2022-08-17 14:11 ` Hans de Goede
2022-11-04 10:29 ` Marge Yang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).