Linux Input Archive on lore.kernel.org
 help / color / Atom feed
From: "dbasehore ." <dbasehore@chromium.org>
To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: jiada_wang@mentor.com, jeffrey.l.hugo@gmail.com,
	benjamin.tissoires@redhat.com, linux-input@vger.kernel.org,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] Input: elan_i2c - only increment wakeup count on touch
Date: Tue, 30 Jun 2020 14:44:48 -0700
Message-ID: <CAGAzgsoqsETR79hzdcWjyrPYeLJeM5aUNnxUXsdRQowjsdwLZg@mail.gmail.com> (raw)
In-Reply-To: <20200630051619.GG248110@dtor-ws>

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

      reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-30  0:57 Derek Basehore
2020-06-30  5:16 ` Dmitry Torokhov
2020-06-30 21:44   ` dbasehore . [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGAzgsoqsETR79hzdcWjyrPYeLJeM5aUNnxUXsdRQowjsdwLZg@mail.gmail.com \
    --to=dbasehore@chromium.org \
    --cc=benjamin.tissoires@redhat.com \
    --cc=dmitry.torokhov@gmail.com \
    --cc=jeffrey.l.hugo@gmail.com \
    --cc=jiada_wang@mentor.com \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux Input Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-input/0 linux-input/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-input linux-input/ https://lore.kernel.org/linux-input \
		linux-input@vger.kernel.org
	public-inbox-index linux-input

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-input


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git