Received-SPF: pass (google.com: domain of linux-kernel+bounces-145065-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240405102436.3479210-1-lma@chromium.org> <ZhOccGFkTFkUkRUI@hovoldconsulting.com>
 <CAE5UKNqufWZfKLAXLcpBYKQpJEVt6jPD4Xtr=Nesh34VkNOETg@mail.gmail.com>
 <ZhVix-HJrqQbiPrB@hovoldconsulting.com> <CAE5UKNp3uS9cqDbQjcP3SbfxVi3wPFG4LtP6z=WU_V+M9x6LtQ@mail.gmail.com>
 <ZhzuoWgA88CeenMC@hovoldconsulting.com>
In-Reply-To: <ZhzuoWgA88CeenMC@hovoldconsulting.com>
From: Radoslaw Biernacki <biernacki@google.com>
Date: Mon, 15 Apr 2024 14:26:42 +0200
Message-ID: <CAM4=RnLQRPmYDt_c+dpsia4WFZVA1vukXa35a0Uupu7jMZxtVQ@mail.gmail.com>
Subject: Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to
 power-up transition
To: Johan Hovold <johan@kernel.org>
Cc: =?UTF-8?Q?=C5=81ukasz_Majczak?= <lma@chromium.org>, 
	Jiri Kosina <jikos@kernel.org>, Dmitry Torokhov <dtor@chromium.org>, 
	Benjamin Tissoires <benjamin.tissoires@redhat.com>, Douglas Anderson <dianders@chromium.org>, 
	Hans de Goede <hdegoede@redhat.com>, Maxime Ripard <mripard@kernel.org>, 
	Kai-Heng Feng <kai.heng.feng@canonical.com>, Johan Hovold <johan+linaro@kernel.org>, 
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, 
	Radoslaw Biernacki <rad@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Johan,

On Mon, Apr 15, 2024 at 11:08=E2=80=AFAM Johan Hovold <johan@kernel.org> wr=
ote:
>
> On Thu, Apr 11, 2024 at 04:23:27PM +0200, =C5=81ukasz Majczak wrote:
> > > Sure, but what about other transactions that are initiated by the hos=
t
> > > (e.g. SET_POWER)?
> > >
> > Somehow it is problematic only on reboot and works just fine on
> > suspend/resume and
> > set_power.
> > I will dig more and try to find out what the difference is.
>
> Sounds like it may be related to the i2c_hid_set_power() on shutdown()
> then as Kai-Heng pointed out.
>
> That function already handles a similar retry for I2C_HID_PWR_ON during
> resume.
>
> > > Perhaps this hack at probe is enough for your use case, but is an
> > > incomplete hack and at a minimum you'd need to add a comment explaini=
ng
> > > why it is there.
> > >
> > You mean a comment in the code ?
>
> Yes, if this turns out to be needed then there should be a comment
> explaining why it is there (and currently also as the delays you used
> seem specific for your particular platform).
>
> But hopefully you can find a generic solution to this.

Yes, we might need a more generic solution though it is not yet clear
for us which would be the cleanest one.
As I wrote in the reply to Kenny, the design back in the day was made
to use events rather than
level driven IO line, to drive the power state of the device.
Consequence is we need to request
a low power state before the kernel goes down as there is no guarantee
the kernel will wake up soon
(prevent battery power leak). This event/level logic problem (event
design for level type problem).

Let us get back to you with more info as we look deeper into some
newly found power sequence limitations
of the second I2C node on this device.

>
> Johan