Received-SPF: pass (google.com: domain of linux-kernel+bounces-134900-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Mon, 8 Apr 2024 09:27:44 +0200
From: Johan Hovold <johan@kernel.org>
To: Lukasz Majczak <lma@chromium.org>
Cc: 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: <ZhOccGFkTFkUkRUI@hovoldconsulting.com>
References: <20240405102436.3479210-1-lma@chromium.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240405102436.3479210-1-lma@chromium.org>

On Fri, Apr 05, 2024 at 10:24:36AM +0000, Lukasz Majczak wrote:
> This patch extends the early bailout for probing procedure introduced in
> commit b3a81b6c4fc6 ("HID: i2c-hid: check if device is there before
> really probing"), in order to cover devices
> based on STM microcontrollers. For touchpads based on STM uC,
> the probe sequence needs to take into account the increased response time
> for i2c transaction if the device already entered a deep power state.
> STM specify the wakeup time as 400us between positive strobe of
> the clock line. Deep sleep is controlled by Touchpad FW,
> though some devices enter it pretty early to conserve power
> in case of lack of activity on the i2c bus.
> Failing to follow this requirement will result in touchpad device not being
> detected due initial transaction being dropped by STM i2c slave controller.
> By adding additional try, first transaction will wake up the touchpad
> STM controller, and the second will produce proper detection response.

Can you please explain why this would not a problem for all future
transactions as well?

If it is, then it sounds like this needs to be addressed in the i2c
driver. If not, then perhaps the problem is really that you just need a
delay after enabling the power supplies? 
 
> v1->v2:
> * Updated commit message with short sha of a base commit and proper tags
> * Rearranged while loop to perform check only once
> * Loosened sleeping range
> 
> Co-developed-by: Radoslaw Biernacki <rad@chromium.org>
> Signed-off-by: Radoslaw Biernacki <rad@chromium.org>
> Signed-off-by: Lukasz Majczak <lma@chromium.org>
> ---
>  drivers/hid/i2c-hid/i2c-hid-core.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 2df1ab3c31cc..ece1a5815e0b 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -1013,9 +1013,17 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
>  	struct i2c_client *client = ihid->client;
>  	struct hid_device *hid = ihid->hid;
>  	int ret;
> +	int tries = 2;

Nit: move above the 'ret' declaration to maintain reverse xmas style
ordering.

Johan