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 :)
Cheers,
Benjamin
