| Message ID | 1660641649-11929-1-git-send-email-marge.yang@synaptics.corp-partner.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V4] Input: synaptics-rmi4 - filter incomplete relative packet. | expand |
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;
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;
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;