linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Input: elan_i2c - only increment wakeup count on touch
@ 2020-06-30  0:57 Derek Basehore
  2020-06-30  5:16 ` Dmitry Torokhov
  0 siblings, 1 reply; 3+ messages in thread
From: Derek Basehore @ 2020-06-30  0:57 UTC (permalink / raw)
  Cc: dmitry.torokhov, dbasehore, jiada_wang, jeffrey.l.hugo,
	benjamin.tissoires, linux-input, linux-kernel

This moves the wakeup increment for elan devices to the touch report.
This prevents the drivers from incorrectly reporting a wakeup when the
resume callback resets then device, which causes an interrupt to
occur. This also avoids error messages when these interrupts occur,
since this behavior is expected.

Signed-off-by: Derek Basehore <dbasehore@chromium.org>
---
 drivers/input/mouse/elan_i2c_core.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index cdbe6b38c73c1..6ad53a75f9807 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -49,6 +49,7 @@
 
 #define ETP_MAX_FINGERS		5
 #define ETP_FINGER_DATA_LEN	5
+#define ETP_REPORT_LEN_OFFSET	0
 #define ETP_REPORT_ID		0x5D
 #define ETP_TP_REPORT_ID	0x5E
 #define ETP_REPORT_ID_OFFSET	2
@@ -1018,6 +1019,8 @@ static void elan_report_absolute(struct elan_tp_data *data, u8 *packet)
 	u8 hover_info = packet[ETP_HOVER_INFO_OFFSET];
 	bool contact_valid, hover_event;
 
+	pm_wakeup_event(&data->client->dev, 0);
+
 	hover_event = hover_info & 0x40;
 	for (i = 0; i < ETP_MAX_FINGERS; i++) {
 		contact_valid = tp_info & (1U << (3 + i));
@@ -1041,6 +1044,8 @@ static void elan_report_trackpoint(struct elan_tp_data *data, u8 *report)
 	u8 *packet = &report[ETP_REPORT_ID_OFFSET + 1];
 	int x, y;
 
+	pm_wakeup_event(&data->client->dev, 0);
+
 	if (!data->tp_input) {
 		dev_warn_once(&data->client->dev,
 			      "received a trackpoint report while no trackpoint device has been created. Please report upstream.\n");
@@ -1065,7 +1070,6 @@ static void elan_report_trackpoint(struct elan_tp_data *data, u8 *report)
 static irqreturn_t elan_isr(int irq, void *dev_id)
 {
 	struct elan_tp_data *data = dev_id;
-	struct device *dev = &data->client->dev;
 	int error;
 	u8 report[ETP_MAX_REPORT_LEN];
 
@@ -1083,7 +1087,13 @@ static irqreturn_t elan_isr(int irq, void *dev_id)
 	if (error)
 		goto out;
 
-	pm_wakeup_event(dev, 0);
+	/*
+	 * Controllers may send a full length report on power on and reset
+	 * cases. There are only meaningless bytes in these reports except for
+	 * report[ETP_REPORT_LEN_OFFSET], which is 0.
+	 */
+	if (!report[ETP_REPORT_LEN_OFFSET])
+		goto out;
 
 	switch (report[ETP_REPORT_ID_OFFSET]) {
 	case ETP_REPORT_ID:
@@ -1093,7 +1103,7 @@ static irqreturn_t elan_isr(int irq, void *dev_id)
 		elan_report_trackpoint(data, report);
 		break;
 	default:
-		dev_err(dev, "invalid report id data (%x)\n",
+		dev_err(&data->client->dev, "invalid report id data (%x)\n",
 			report[ETP_REPORT_ID_OFFSET]);
 	}
 
-- 
2.27.0.212.ge8ba1cc988-goog


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] Input: elan_i2c - only increment wakeup count on touch
  2020-06-30  0:57 [PATCH] Input: elan_i2c - only increment wakeup count on touch Derek Basehore
@ 2020-06-30  5:16 ` Dmitry Torokhov
  2020-06-30 21:44   ` dbasehore .
  0 siblings, 1 reply; 3+ messages in thread
From: Dmitry Torokhov @ 2020-06-30  5:16 UTC (permalink / raw)
  To: Derek Basehore
  Cc: jiada_wang, jeffrey.l.hugo, benjamin.tissoires, linux-input,
	linux-kernel

On Mon, Jun 29, 2020 at 05:57:07PM -0700, Derek Basehore wrote:
> This moves the wakeup increment for elan devices to the touch report.
> This prevents the drivers from incorrectly reporting a wakeup when the
> resume callback resets then device, which causes an interrupt to
> occur. This also avoids error messages when these interrupts occur,
> since this behavior is expected.
> 
> Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> ---
>  drivers/input/mouse/elan_i2c_core.c | 16 +++++++++++++---
>  1 file changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index cdbe6b38c73c1..6ad53a75f9807 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -49,6 +49,7 @@
>  
>  #define ETP_MAX_FINGERS		5
>  #define ETP_FINGER_DATA_LEN	5
> +#define ETP_REPORT_LEN_OFFSET	0
>  #define ETP_REPORT_ID		0x5D
>  #define ETP_TP_REPORT_ID	0x5E
>  #define ETP_REPORT_ID_OFFSET	2
> @@ -1018,6 +1019,8 @@ static void elan_report_absolute(struct elan_tp_data *data, u8 *packet)
>  	u8 hover_info = packet[ETP_HOVER_INFO_OFFSET];
>  	bool contact_valid, hover_event;
>  
> +	pm_wakeup_event(&data->client->dev, 0);
> +
>  	hover_event = hover_info & 0x40;
>  	for (i = 0; i < ETP_MAX_FINGERS; i++) {
>  		contact_valid = tp_info & (1U << (3 + i));
> @@ -1041,6 +1044,8 @@ static void elan_report_trackpoint(struct elan_tp_data *data, u8 *report)
>  	u8 *packet = &report[ETP_REPORT_ID_OFFSET + 1];
>  	int x, y;
>  
> +	pm_wakeup_event(&data->client->dev, 0);
> +
>  	if (!data->tp_input) {
>  		dev_warn_once(&data->client->dev,
>  			      "received a trackpoint report while no trackpoint device has been created. Please report upstream.\n");
> @@ -1065,7 +1070,6 @@ static void elan_report_trackpoint(struct elan_tp_data *data, u8 *report)
>  static irqreturn_t elan_isr(int irq, void *dev_id)
>  {
>  	struct elan_tp_data *data = dev_id;
> -	struct device *dev = &data->client->dev;
>  	int error;
>  	u8 report[ETP_MAX_REPORT_LEN];
>  
> @@ -1083,7 +1087,13 @@ static irqreturn_t elan_isr(int irq, void *dev_id)
>  	if (error)
>  		goto out;
>  
> -	pm_wakeup_event(dev, 0);
> +	/*
> +	 * Controllers may send a full length report on power on and reset
> +	 * cases. There are only meaningless bytes in these reports except for
> +	 * report[ETP_REPORT_LEN_OFFSET], which is 0.
> +	 */

Is this true for all versions of firmware? Also, should we pay attention
to the value of this field for various types of reports?

> +	if (!report[ETP_REPORT_LEN_OFFSET])
> +		goto out;
>  
>  	switch (report[ETP_REPORT_ID_OFFSET]) {
>  	case ETP_REPORT_ID:
> @@ -1093,7 +1103,7 @@ static irqreturn_t elan_isr(int irq, void *dev_id)
>  		elan_report_trackpoint(data, report);
>  		break;
>  	default:
> -		dev_err(dev, "invalid report id data (%x)\n",
> +		dev_err(&data->client->dev, "invalid report id data (%x)\n",
>  			report[ETP_REPORT_ID_OFFSET]);
>  	}
>  
> -- 
> 2.27.0.212.ge8ba1cc988-goog
> 

Thanks.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] Input: elan_i2c - only increment wakeup count on touch
  2020-06-30  5:16 ` Dmitry Torokhov
@ 2020-06-30 21:44   ` dbasehore .
  0 siblings, 0 replies; 3+ messages in thread
From: dbasehore . @ 2020-06-30 21:44 UTC (permalink / raw)
  To: Dmitry Torokhov
  Cc: jiada_wang, jeffrey.l.hugo, benjamin.tissoires, linux-input,
	linux-kernel

On Mon, Jun 29, 2020 at 10:16 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Mon, Jun 29, 2020 at 05:57:07PM -0700, Derek Basehore wrote:
> > This moves the wakeup increment for elan devices to the touch report.
> > This prevents the drivers from incorrectly reporting a wakeup when the
> > resume callback resets then device, which causes an interrupt to
> > occur. This also avoids error messages when these interrupts occur,
> > since this behavior is expected.
> >
> > Signed-off-by: Derek Basehore <dbasehore@chromium.org>
> > ---
> >  drivers/input/mouse/elan_i2c_core.c | 16 +++++++++++++---
> >  1 file changed, 13 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> > index cdbe6b38c73c1..6ad53a75f9807 100644
> > --- a/drivers/input/mouse/elan_i2c_core.c
> > +++ b/drivers/input/mouse/elan_i2c_core.c
> > @@ -49,6 +49,7 @@
> >
> >  #define ETP_MAX_FINGERS              5
> >  #define ETP_FINGER_DATA_LEN  5
> > +#define ETP_REPORT_LEN_OFFSET        0
> >  #define ETP_REPORT_ID                0x5D
> >  #define ETP_TP_REPORT_ID     0x5E
> >  #define ETP_REPORT_ID_OFFSET 2
> > @@ -1018,6 +1019,8 @@ static void elan_report_absolute(struct elan_tp_data *data, u8 *packet)
> >       u8 hover_info = packet[ETP_HOVER_INFO_OFFSET];
> >       bool contact_valid, hover_event;
> >
> > +     pm_wakeup_event(&data->client->dev, 0);
> > +
> >       hover_event = hover_info & 0x40;
> >       for (i = 0; i < ETP_MAX_FINGERS; i++) {
> >               contact_valid = tp_info & (1U << (3 + i));
> > @@ -1041,6 +1044,8 @@ static void elan_report_trackpoint(struct elan_tp_data *data, u8 *report)
> >       u8 *packet = &report[ETP_REPORT_ID_OFFSET + 1];
> >       int x, y;
> >
> > +     pm_wakeup_event(&data->client->dev, 0);
> > +
> >       if (!data->tp_input) {
> >               dev_warn_once(&data->client->dev,
> >                             "received a trackpoint report while no trackpoint device has been created. Please report upstream.\n");
> > @@ -1065,7 +1070,6 @@ static void elan_report_trackpoint(struct elan_tp_data *data, u8 *report)
> >  static irqreturn_t elan_isr(int irq, void *dev_id)
> >  {
> >       struct elan_tp_data *data = dev_id;
> > -     struct device *dev = &data->client->dev;
> >       int error;
> >       u8 report[ETP_MAX_REPORT_LEN];
> >
> > @@ -1083,7 +1087,13 @@ static irqreturn_t elan_isr(int irq, void *dev_id)
> >       if (error)
> >               goto out;
> >
> > -     pm_wakeup_event(dev, 0);
> > +     /*
> > +      * Controllers may send a full length report on power on and reset
> > +      * cases. There are only meaningless bytes in these reports except for
> > +      * report[ETP_REPORT_LEN_OFFSET], which is 0.
> > +      */
>
> Is this true for all versions of firmware? Also, should we pay attention
> to the value of this field for various types of reports?
>

I wrote the patch with input from Elan on our bug tracker, and they
say that this will work. This is HID over I2C, so the first and second
byte are the length. Since the packets are never long, just the first
byte suffices for everything, but I think we should actually take the
first and second byte for the length. Just in case some new version of
the chip starts sending 256+ byte responses.

> > +     if (!report[ETP_REPORT_LEN_OFFSET])
> > +             goto out;
> >
> >       switch (report[ETP_REPORT_ID_OFFSET]) {
> >       case ETP_REPORT_ID:
> > @@ -1093,7 +1103,7 @@ static irqreturn_t elan_isr(int irq, void *dev_id)
> >               elan_report_trackpoint(data, report);
> >               break;
> >       default:
> > -             dev_err(dev, "invalid report id data (%x)\n",
> > +             dev_err(&data->client->dev, "invalid report id data (%x)\n",
> >                       report[ETP_REPORT_ID_OFFSET]);
> >       }
> >
> > --
> > 2.27.0.212.ge8ba1cc988-goog
> >
>
> Thanks.
>
> --
> Dmitry

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-30 21:45 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-30  0:57 [PATCH] Input: elan_i2c - only increment wakeup count on touch Derek Basehore
2020-06-30  5:16 ` Dmitry Torokhov
2020-06-30 21:44   ` dbasehore .

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).