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

[Bibek Basu]

Hi Benjamin,


> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@xxxxxxxxx]
> Sent: Thursday, October 17, 2013 11:27 PM
> To: Bibek Basu
> Cc: Jiri Kosina; linux-input; linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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
> >
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—