[PATCH] HID: i2c-hid: Stop querying for init reports

[PATCH] HID: i2c-hid: Stop querying for init reports

[Bibek Basu]

According to specifications, HID over I2C devices
are not bound to respond to query for INPUT
REPORTS. Thus dropping the call during init
as many devices does not respond causing error
messages during boot.

Signed-off-by: Bibek Basu <bbasu@nvidia.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index fd7ce37..ae48d18 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -455,10 +455,6 @@ static void i2c_hid_init_reports(struct hid_device *hid)
 	}
 
 	list_for_each_entry(report,
-		&hid->report_enum[HID_INPUT_REPORT].report_list, list)
-		i2c_hid_init_report(report, inbuf, ihid->bufsize);
-
-	list_for_each_entry(report,
 		&hid->report_enum[HID_FEATURE_REPORT].report_list, list)
 		i2c_hid_init_report(report, inbuf, ihid->bufsize);
 
-- 
1.8.1.5

Re: [PATCH] HID: i2c-hid: Stop querying for init reports

[Jiri Kosina]

On Mon, 21 Oct 2013, Bibek Basu wrote:

> According to specifications, HID over I2C devices
> are not bound to respond to query for INPUT
> REPORTS. Thus dropping the call during init
> as many devices does not respond causing error
> messages during boot.
> 
> Signed-off-by: Bibek Basu <bbasu@nvidia.com>

I am queuing this for 3.13. Thanks,

-- 
Jiri Kosina
SUSE Labs

[PATCH] HID: i2c-hid: Stop querying for init reports

[Bibek Basu]

According to specifications, HID over I2C devices
are not bound to respond to query for INPUT
REPORTS. Thus dropping the call during init
as many devices does not respond causing error
messages during boot.

Signed-off-by: Bibek Basu <bbasu@nvidia.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 59 -------------------------------------------
 1 file changed, 59 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index fd7ce37..58a4f12 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -409,62 +409,6 @@ static int i2c_hid_get_report_length(struct hid_report *report)
 		report->device->report_enum[report->type].numbered + 2;
 }
 
-static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
-	size_t bufsize)
-{
-	struct hid_device *hid = report->device;
-	struct i2c_client *client = hid->driver_data;
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
-	unsigned int size, ret_size;
-
-	size = i2c_hid_get_report_length(report);
-	if (i2c_hid_get_report(client,
-			report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
-			report->id, buffer, size))
-		return;
-
-	i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
-
-	ret_size = buffer[0] | (buffer[1] << 8);
-
-	if (ret_size != size) {
-		dev_err(&client->dev, "error in %s size:%d / ret_size:%d\n",
-			__func__, size, ret_size);
-		return;
-	}
-
-	/* hid->driver_lock is held as we are in probe function,
-	 * we just need to setup the input fields, so using
-	 * hid_report_raw_event is safe. */
-	hid_report_raw_event(hid, report->type, buffer + 2, size - 2, 1);
-}
-
-/*
- * Initialize all reports
- */
-static void i2c_hid_init_reports(struct hid_device *hid)
-{
-	struct hid_report *report;
-	struct i2c_client *client = hid->driver_data;
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
-	u8 *inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
-
-	if (!inbuf) {
-		dev_err(&client->dev, "can not retrieve initial reports\n");
-		return;
-	}
-
-	list_for_each_entry(report,
-		&hid->report_enum[HID_INPUT_REPORT].report_list, list)
-		i2c_hid_init_report(report, inbuf, ihid->bufsize);
-
-	list_for_each_entry(report,
-		&hid->report_enum[HID_FEATURE_REPORT].report_list, list)
-		i2c_hid_init_report(report, inbuf, ihid->bufsize);
-
-	kfree(inbuf);
-}
-
 /*
  * Traverse the supplied list of reports and find the longest
  */
@@ -683,9 +627,6 @@ static int i2c_hid_start(struct hid_device *hid)
 			return ret;
 	}
 
-	if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS))
-		i2c_hid_init_reports(hid);
-
 	return 0;
 }
 
-- 
1.8.1.5

Re: [PATCH] HID: i2c-hid: Stop querying for init reports

[Benjamin Tissoires]

Hi Bibek,

On Tue, Oct 15, 2013 at 4:28 AM, Bibek Basu <bbasu@nvidia.com> wrote:
> According to specifications, HID over I2C devices
> are not bound to respond to query for INPUT
> REPORTS. Thus dropping the call during init
> as many devices does not respond causing error
> messages during boot.
>

This time, the patch is removing too many things that are correct. :)

see below to know what to remove and what to keep:

> Signed-off-by: Bibek Basu <bbasu@nvidia.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 59 -------------------------------------------
>  1 file changed, 59 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index fd7ce37..58a4f12 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -409,62 +409,6 @@ static int i2c_hid_get_report_length(struct hid_report *report)
>                 report->device->report_enum[report->type].numbered + 2;
>  }
>
> -static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
> -       size_t bufsize)
> -{
> -       struct hid_device *hid = report->device;
> -       struct i2c_client *client = hid->driver_data;
> -       struct i2c_hid *ihid = i2c_get_clientdata(client);
> -       unsigned int size, ret_size;
> -
> -       size = i2c_hid_get_report_length(report);
> -       if (i2c_hid_get_report(client,
> -                       report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
> -                       report->id, buffer, size))
> -               return;
> -
> -       i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
> -
> -       ret_size = buffer[0] | (buffer[1] << 8);
> -
> -       if (ret_size != size) {
> -               dev_err(&client->dev, "error in %s size:%d / ret_size:%d\n",
> -                       __func__, size, ret_size);
> -               return;
> -       }
> -
> -       /* hid->driver_lock is held as we are in probe function,
> -        * we just need to setup the input fields, so using
> -        * hid_report_raw_event is safe. */
> -       hid_report_raw_event(hid, report->type, buffer + 2, size - 2, 1);
> -}

This function should be kept

> -
> -/*
> - * Initialize all reports
> - */
> -static void i2c_hid_init_reports(struct hid_device *hid)
> -{
> -       struct hid_report *report;
> -       struct i2c_client *client = hid->driver_data;
> -       struct i2c_hid *ihid = i2c_get_clientdata(client);
> -       u8 *inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
> -
> -       if (!inbuf) {
> -               dev_err(&client->dev, "can not retrieve initial reports\n");
> -               return;
> -       }
> -

Above should be kept

> -       list_for_each_entry(report,
> -               &hid->report_enum[HID_INPUT_REPORT].report_list, list)
> -               i2c_hid_init_report(report, inbuf, ihid->bufsize);
> -

these for lines should be removed (they are the one giving the errors
in the logs)

and please keep the rest of the code as is.


> -       list_for_each_entry(report,
> -               &hid->report_enum[HID_FEATURE_REPORT].report_list, list)
> -               i2c_hid_init_report(report, inbuf, ihid->bufsize);

Actually, this part is very important because we have no spontaneous
events emitted by features, so we don't know the value of the feature
until we probed it. So please keep this part as mentioned above.

Cheers,
Benjamin

> -
> -       kfree(inbuf);
> -}
> -
>  /*
>   * Traverse the supplied list of reports and find the longest
>   */
> @@ -683,9 +627,6 @@ static int i2c_hid_start(struct hid_device *hid)
>                         return ret;
>         }
>
> -       if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS))
> -               i2c_hid_init_reports(hid);
> -
>         return 0;
>  }
>
> --
> 1.8.1.5
>

RE: [PATCH] HID: i2c-hid: Stop querying for init reports

[Bibek Basu]

Hi Benjamin,


> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@gmail.com]
> Sent: Thursday, October 17, 2013 11:27 PM
> To: Bibek Basu
> Cc: Jiri Kosina; linux-input; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] HID: i2c-hid: Stop querying for init reports
> 
> Hi Bibek,
> 
> On Tue, Oct 15, 2013 at 4:28 AM, Bibek Basu <bbasu@nvidia.com> wrote:
> > According to specifications, HID over I2C devices are not bound to
> > respond to query for INPUT REPORTS. Thus dropping the call during init
> > as many devices does not respond causing error messages during boot.
> >
> 
> This time, the patch is removing too many things that are correct. :)
> 
> see below to know what to remove and what to keep:
> 
> > Signed-off-by: Bibek Basu <bbasu@nvidia.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid.c | 59
> > -------------------------------------------
> >  1 file changed, 59 deletions(-)
> >
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c
> > b/drivers/hid/i2c-hid/i2c-hid.c index fd7ce37..58a4f12 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -409,62 +409,6 @@ static int i2c_hid_get_report_length(struct
> hid_report *report)
> >                 report->device->report_enum[report->type].numbered +
> > 2;  }
> >
> > -static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
> > -       size_t bufsize)
> > -{
> > -       struct hid_device *hid = report->device;
> > -       struct i2c_client *client = hid->driver_data;
> > -       struct i2c_hid *ihid = i2c_get_clientdata(client);
> > -       unsigned int size, ret_size;
> > -
> > -       size = i2c_hid_get_report_length(report);
> > -       if (i2c_hid_get_report(client,
> > -                       report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
> > -                       report->id, buffer, size))
> > -               return;
> > -
> > -       i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
> > -
> > -       ret_size = buffer[0] | (buffer[1] << 8);
> > -
> > -       if (ret_size != size) {
> > -               dev_err(&client->dev, "error in %s size:%d / ret_size:%d\n",
> > -                       __func__, size, ret_size);
> > -               return;
> > -       }
> > -
> > -       /* hid->driver_lock is held as we are in probe function,
> > -        * we just need to setup the input fields, so using
> > -        * hid_report_raw_event is safe. */
> > -       hid_report_raw_event(hid, report->type, buffer + 2, size - 2, 1);
> > -}
> 
> This function should be kept
> 
> > -
> > -/*
> > - * Initialize all reports
> > - */
> > -static void i2c_hid_init_reports(struct hid_device *hid) -{
> > -       struct hid_report *report;
> > -       struct i2c_client *client = hid->driver_data;
> > -       struct i2c_hid *ihid = i2c_get_clientdata(client);
> > -       u8 *inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
> > -
> > -       if (!inbuf) {
> > -               dev_err(&client->dev, "can not retrieve initial reports\n");
> > -               return;
> > -       }
> > -
> 
> Above should be kept
> 
> > -       list_for_each_entry(report,
> > -               &hid->report_enum[HID_INPUT_REPORT].report_list, list)
> > -               i2c_hid_init_report(report, inbuf, ihid->bufsize);
> > -
> 
> these for lines should be removed (they are the one giving the errors in the
> logs)
> 
> and please keep the rest of the code as is.
> 
> 
> > -       list_for_each_entry(report,
> > -               &hid->report_enum[HID_FEATURE_REPORT].report_list, list)
> > -               i2c_hid_init_report(report, inbuf, ihid->bufsize);
> 
> Actually, this part is very important because we have no spontaneous events
> emitted by features, so we don't know the value of the feature until we
> probed it. So please keep this part as mentioned above.

Thanks for the inputs. I am repushing  the patch. Unfortunately, looks like I need to upgrade the firmware of my HID keyboard, as it does not responds to the FEATURE_REPORT
query:(

regards
Bibek
> 
> Cheers,
> Benjamin
> 
> > -
> > -       kfree(inbuf);
> > -}
> > -
> >  /*
> >   * Traverse the supplied list of reports and find the longest
> >   */
> > @@ -683,9 +627,6 @@ static int i2c_hid_start(struct hid_device *hid)
> >                         return ret;
> >         }
> >
> > -       if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS))
> > -               i2c_hid_init_reports(hid);
> > -
> >         return 0;
> >  }
> >
> > --
> > 1.8.1.5
> >