Received-SPF: pass (google.com: domain of linux-kernel+bounces-144911-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <81e1b870-37f9-4ef2-9a3d-87015f32546b@kl.wtf>
Date: Mon, 15 Apr 2024 12:28:28 +0200
Precedence: bulk
MIME-Version: 1.0
To: lma@chromium.org
Cc: benjamin.tissoires@redhat.com, dianders@chromium.org, dtor@chromium.org,
 hdegoede@redhat.com, jikos@kernel.org, johan+linaro@kernel.org,
 johan@kernel.org, kai.heng.feng@canonical.com, linux-input@vger.kernel.org,
 linux-kernel@vger.kernel.org, mripard@kernel.org, rad@chromium.org
References: <CAE5UKNqufWZfKLAXLcpBYKQpJEVt6jPD4Xtr=Nesh34VkNOETg@mail.gmail.com>
Subject: Re: [PATCH v2] HID: i2c-hid: wait for i2c touchpad deep-sleep to
 power-up transition
Content-Language: en-US
From: Kenny Levinsen <kl@kl.wtf>
In-Reply-To: <CAE5UKNqufWZfKLAXLcpBYKQpJEVt6jPD4Xtr=Nesh34VkNOETg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

> The problem is that the probe function calling i2c_smbus_read_byte()
> is not aware that
> uC on the other end is in a deep sleep state so the first read will
> fail and so the whole probe.
Well, the probe was just added to "avoid scary messages", so we could 
just do away with it and fix the "scary messages" instead.

I think it would be better to handle the wake-up near the command being 
sent that requires the device to be awake, just like is done for 
i2c_hid_set_power(). This would mean removing the smbus probe 
altogether, extending the HID descriptor fetch code to retry on 
EREMOTEIO, and to avoid the "scary messages", print something nice if 
the second attempt also fails with EREMOTEIO.

If the device can enter deep-sleep arbitrarily, then we presumably also 
have problems in i2c_hid_output_raw_report() and 
i2c_hid_get_raw_report() which could happen after the device has gone to 
sleep from inactivity. These places would also need EREMOTEIO retry logic.

All these places should have the same sleeping behavior as they are 
working around the same device glitch. I imagine the client ACK timeout 
is longer than your required 400µs, in which case you don't need any 
sleep on retry at all, as is the case in the current i2c_hid_set_power() 
implementation.

However, as we litter retry-code all over the place, Johan's suggestion 
about doing this in the I2C driver does become a bit more relevant...

Best regards,
Kenny Levinsen