Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200630005707.2181126-1-dbasehore@chromium.org> <20200630051619.GG248110@dtor-ws>
In-Reply-To: <20200630051619.GG248110@dtor-ws>
From:   "dbasehore ." <dbasehore@chromium.org>
Date:   Tue, 30 Jun 2020 14:44:48 -0700
Message-ID: <CAGAzgsoqsETR79hzdcWjyrPYeLJeM5aUNnxUXsdRQowjsdwLZg@mail.gmail.com>
Subject: Re: [PATCH] Input: elan_i2c - only increment wakeup count on touch
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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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