Hi Kenny, Lukasz,
On Sat, Apr 27, 2024 at 12:47:07AM +0200, Kenny Levinsen wrote:
> To avoid error messages when a device is not present, b3a81b6c4fc6 added
> an initial bus probe using a dummy i2c_smbus_read_byte() call.
>
> Without this probe, i2c_hid_fetch_hid_descriptor() will fail internally
> on a bus error and log. Treat the bus error as a missing device and
> remove the error log so we can do away with the probe.
>
> Tested-by: Lukasz Majczak <lma@xxxxxxxxxxxx>
> Reviewed-by: Lukasz Majczak <lma@xxxxxxxxxxxx>
> Signed-off-by: Kenny Levinsen <kl@xxxxxx>
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 21 ++++++---------------
> 1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index d965382196c6..6ffa43d245b4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -872,12 +872,11 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> ihid->wHIDDescRegister,
> &ihid->hdesc,
> sizeof(ihid->hdesc));
> - if (error) {
> - dev_err(&ihid->client->dev,
> - "failed to fetch HID descriptor: %d\n",
> - error);
> - return -ENODEV;
> - }
> +
> + /* The i2c drivers are a bit inconsistent with their error
> + * codes, so treat everything as -ENXIO for now. */
> + if (error)
> + return -ENXIO;
I really think we should differentiate the cases "we do not know if
there is a device" vs "we do known that there is a device and we have
strong expectation of what that device is, and we do not expect
communication to fail".
Because of that I think we should have separate retry for the original
i2c_smbus_read_byte() (if you want you can make a wrapper around it,
something like i2c_hid_check_device_present()", and if there is a
concern that we will run into similar issue on resume (either from
suspend or from hibernate) then we can have similar retry in
i2c_hid_fetch_hid_descriptor().
But I do think that i2c_hid_fetch_hid_descriptor() should not try to
clobber the error but rather log the true one.
> }
>
> /* Validate the length of HID descriptor, the 4 first bytes:
> @@ -992,17 +991,9 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
> struct hid_device *hid = ihid->hid;
> int ret;
>
> - /* Make sure there is something at this address */
> - ret = i2c_smbus_read_byte(client);
> - if (ret < 0) {
> - i2c_hid_dbg(ihid, "nothing at this address: %d\n", ret);
> - return -ENXIO;
> - }
> -
> ret = i2c_hid_fetch_hid_descriptor(ihid);
> if (ret < 0) {
> - dev_err(&client->dev,
> - "Failed to fetch the HID Descriptor\n");
> + i2c_hid_dbg(ihid, "failed to fetch HID descriptor: %d\n", ret);
> return ret;
> }
>
> --
> 2.44.0
>
>
Thanks.
--
Dmitry
