Received-SPF: pass (google.com: domain of linux-kernel+bounces-145276-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 15 Apr 2024 15:37:37 +0200
From: Johan Hovold <johan@kernel.org>
To: Radoslaw Biernacki <biernacki@google.com>
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>
Subject: Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to
 power-up transition
Message-ID: <Zh0toTnoGBkJwOq7@hovoldconsulting.com>
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>
 <CAM4=RnLQRPmYDt_c+dpsia4WFZVA1vukXa35a0Uupu7jMZxtVQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAM4=RnLQRPmYDt_c+dpsia4WFZVA1vukXa35a0Uupu7jMZxtVQ@mail.gmail.com>

On Mon, Apr 15, 2024 at 02:26:42PM +0200, Radoslaw Biernacki wrote:
> On Mon, Apr 15, 2024 at 11:08 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, Apr 11, 2024 at 04:23:27PM +0200, Łukasz Majczak wrote:
> > > > Sure, but what about other transactions that are initiated by the host
> > > > (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.

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

Ok, and do I understand you correctly that it is indeed the SET_POWER
command on shutdown() that makes the device enter the sleep state and
which then needs an extra transaction on next boot to be woken up?

And that the firmware will not enter that low power state on its own
based on lack of activity as the current commit message suggests?

If so, then it seems a variant of this patch would be ok as long as the
commit message and a comment in the code explains the problem correctly.

Johan