On Fri, Jan 14, 2022 at 7:24 PM Angela Czubak <acz@xxxxxxxxxxxx> wrote:
>
> Hi Benjamin,
>
> On Thu, Jan 13, 2022 at 10:54 AM Benjamin Tissoires
> <benjamin.tissoires@xxxxxxxxxx> wrote:
> >
> > On Wed, Jan 12, 2022 at 12:26 PM Angela Czubak <acz@xxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Jan 12, 2022 at 10:43 AM Benjamin Tissoires
> > > <benjamin.tissoires@xxxxxxxxxx> wrote:
> > > >
> > > > On 1/10/22 20:43, Angela Czubak wrote:
> > > > > On Fri, Jan 7, 2022 at 11:01 PM Dmitry Torokhov
> > > > > <dmitry.torokhov@xxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > >>> ---
> > > > >>> 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.
>
The thing it does is to release the semaphore
hid->driver_input_lock(), which is then in turn used in hid-core.c, in
hid_input_report(). Without this, hid_input_report() aborts earlier
and the field->values[] are not populated because HID considers it
should ignore the incoming reports.
So without this, you can not access the new values apart from doing
what hid-multitouch does: trick the system and force inject an event
in the processing pipeline.
Cheers,
Benjamin
