Received-SPF: pass (google.com: domain of linux-kernel+bounces-29426-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240115045054.1170294-1-kai.heng.feng@canonical.com>
In-Reply-To: <20240115045054.1170294-1-kai.heng.feng@canonical.com>
From: Doug Anderson <dianders@chromium.org>
Date: Wed, 17 Jan 2024 12:35:23 -0800
Message-ID: <CAD=FV=VCoB_uBontRk3WVn-nqzZnutSMCHBpdBqNOFwrYv=8xw@mail.gmail.com>
Subject: Re: [PATCH v2] HID: i2c-hid: Skip SET_POWER SLEEP for Cirque touchpad
 on system suspend
To: Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com, 
	Hans de Goede <hdegoede@redhat.com>, Maxime Ripard <mripard@kernel.org>, 
	=?UTF-8?Q?Thomas_Wei=C3=9Fschuh?= <linux@weissschuh.net>, 
	Johan Hovold <johan+linaro@kernel.org>, linux-input@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Sun, Jan 14, 2024 at 8:51=E2=80=AFPM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> There's a Cirque touchpad that wakes system up without anything touched
> the touchpad. The input report is empty when this happens.
> The reason is stated in HID over I2C spec, 7.2.8.2:
> "If the DEVICE wishes to wake the HOST from its low power state, it can
> issue a wake by asserting the interrupt."
>
> This is fine if OS can put system back to suspend by identifying input
> wakeup count stays the same on resume, like Chrome OS Dark Resume [0].
> But for regular distro such policy is lacking.
>
> Though the change doesn't bring any impact on power consumption for
> touchpad is minimal, other i2c-hid device may depends on SLEEP control
> power. So use a quirk to limit the change scope.
>
> [0] https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/power_m=
anager/docs/dark_resume.md
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2:
>  - Use quirk instead of applying the change universally.
>
>  drivers/hid/hid-ids.h              | 3 +++
>  drivers/hid/i2c-hid/i2c-hid-core.c | 6 +++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)

This seems OK to me. As per my repy to v1, it doesn't feel fully
explained what's going on. Why does it only wake the system without
touches if you put it to sleep first? Is it truly a hardware quirk
with the Cirque touchpad or is it a bug on the board you have it
hooked up to? In any case, perhaps it doesn't matter since you said
you measured power here and, on this touchpad it doesn't seem to save
significant extra power to go into sleep mode. ...so I guess I'd be OK
w/

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[1] https://lore.kernel.org/r/20240115045054.1170294-1-kai.heng.feng@canoni=
cal.com