Re: [PATCH 05/18] HID: introduce hid_get_feature

[Angela Czubak <acz@semihalf.com>]

Hi Benjamin,

On Thu, Jan 13, 2022 at 10:54 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Wed, Jan 12, 2022 at 12:26 PM Angela Czubak <acz@semihalf.com> wrote:
> >
> > On Wed, Jan 12, 2022 at 10:43 AM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > On 1/10/22 20:43, Angela Czubak wrote:
> > > > On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
> > > > <dmitry.torokhov@gmail.com> wrote:
> > > >>
> > > >> On Tue, Dec 21, 2021 at 07:17:30PM +0000, Angela Czubak wrote:
> > > >>> Move mt_get_feature from hid-multitouch to hid-core as it is a generic
> > > >>> function that can be used by other drivers as well.
> > > >>>
> > > >>> Signed-off-by: Angela Czubak <acz@semihalf.com>
> > > >>> ---
> > > >>>   drivers/hid/hid-core.c       | 39 ++++++++++++++++++++++++++++++++++++
> > > >>>   drivers/hid/hid-multitouch.c | 38 +++--------------------------------
> > > >>>   include/linux/hid.h          |  1 +
> > > >>>   3 files changed, 43 insertions(+), 35 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > >>> index dbed2524fd47..c11cb7324157 100644
> > > >>> --- a/drivers/hid/hid-core.c
> > > >>> +++ b/drivers/hid/hid-core.c
> > > >>> @@ -1796,6 +1796,45 @@ int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size,
> > > >>>   }
> > > >>>   EXPORT_SYMBOL_GPL(hid_report_raw_event);
> > > >>>
> > > >>> +/**
> > > >>> + * hid_get_feature - retrieve feature report from device
> > > >>> + *
> > > >>> + * @hdev: hid device
> > > >>> + * @report: hid report to retrieve
> > > >>> + */
> > > >>> +void hid_get_feature(struct hid_device *hdev, struct hid_report *report)
> > > >>
> > > >> If this is a generic API I believe it should return success/error code
> > > >> so that users can decide what to do.
> > > >>
> > > > Does it mean I should also modify hid-multitouch.c so that the return
> > > > value is actually checked? Currently it seems to ignore any failures.
> > > >> Thanks.
> > >
> > > Honestly that function is a hack in hid-multitouch. You can replace it by:
> > >
> > > ```
> > > hid_device_io_start(hid);
> > > hid_hw_request(hid, report, HID_REQ_GET_REPORT);
> > > hid_hw_wait(hid);
> > > hid_device_io_stop(hid);
> > > ```
> > >
> > > The hack allows to not have to use hid_device_io_{start|stop}(), which
> > > is probably not clean.
> > >
> > > As for the return value, hid_hw_request() can be used as asynchronous,
> > > which is why it returns void. However, returning an actual int would
> > > definitively be better because some cases are failing silently (like if
> > > the device is not io started).
> > >
> > I am slightly confused; it is hid_hw_raw_request() that is used and it does
> > not seem asynchronous to me; is there no guarantee that the response
> > has already been received?
>
> In the case of usbhid, hid_hw_request() calls directly
> __usbhid_submit_report() which is asynchronous.
> So no, we have no guarantees that the answer is there.
>
> >  It seemed to me that the main purpose of
> > this function is to retrieve information an have it correctly parsed.
> > I literally issue it once to learn if auto trigger has been set by default and
> > to know the durations of waveforms, learn ordinals etc.
> > I could introduce a new function for the purpose of haptic API, it just
> > seemed redundant as the one in hid-multitouch.c does what I need.
>
> Again, the one in hid-multitouch is a hack against
> hid_device_io_{start|stop}(). So if you need to change something, it's
> the hid-multitouch code, not reuse that hack :)
>

ACK, I will use hid_hw_request() and hid_hw_wait() instead as suggested.
BTW is hid_device_io_{start|stop} required to do anything meaningful?
It just seems to me that it currently sets some variable which is not really
useful anywhere else.

> Cheers,
> Benjamin
>