| Message ID | 20211221191743.1893185-6-acz@semihalf.com |
|---|---|
| State | New |
| Headers | show |
| Series | *** Implement simple haptic HID support *** | expand |
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. > > -- > Dmitry
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). Cheers, Benjamin > >> >> -- >> Dmitry
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? 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. > Cheers, > Benjamin > > > > >> > >> -- > >> Dmitry >
On Fri, Jan 14, 2022 at 7:24 PM Angela Czubak <acz@semihalf.com> wrote: > > 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. > 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
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) +{ + int ret; + u32 size = hid_report_len(report); + u8 *buf; + + /* + * Do not fetch the feature report if the device has been explicitly + * marked as non-capable. + */ + if (hdev->quirks & HID_QUIRK_NO_INIT_REPORTS) + return; + + buf = hid_alloc_report_buf(report, GFP_KERNEL); + if (!buf) + return; + + ret = hid_hw_raw_request(hdev, report->id, buf, size, + HID_FEATURE_REPORT, HID_REQ_GET_REPORT); + if (ret < 0) { + dev_warn(&hdev->dev, "failed to fetch feature %d\n", + report->id); + } else { + ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, + size, 0); + if (ret) + dev_warn(&hdev->dev, "failed to report feature\n"); + } + + kfree(buf); +} +EXPORT_SYMBOL_GPL(hid_get_feature); + /** * hid_input_report - report data from lower layer (usb, bt...) * diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index 082376a6cb3d..7beb3dfc3e67 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -434,38 +434,6 @@ static const struct attribute_group mt_attribute_group = { .attrs = sysfs_attrs }; -static void mt_get_feature(struct hid_device *hdev, struct hid_report *report) -{ - int ret; - u32 size = hid_report_len(report); - u8 *buf; - - /* - * Do not fetch the feature report if the device has been explicitly - * marked as non-capable. - */ - if (hdev->quirks & HID_QUIRK_NO_INIT_REPORTS) - return; - - buf = hid_alloc_report_buf(report, GFP_KERNEL); - if (!buf) - return; - - ret = hid_hw_raw_request(hdev, report->id, buf, size, - HID_FEATURE_REPORT, HID_REQ_GET_REPORT); - if (ret < 0) { - dev_warn(&hdev->dev, "failed to fetch feature %d\n", - report->id); - } else { - ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, - size, 0); - if (ret) - dev_warn(&hdev->dev, "failed to report feature\n"); - } - - kfree(buf); -} - static void mt_feature_mapping(struct hid_device *hdev, struct hid_field *field, struct hid_usage *usage) { @@ -473,7 +441,7 @@ static void mt_feature_mapping(struct hid_device *hdev, switch (usage->hid) { case HID_DG_CONTACTMAX: - mt_get_feature(hdev, field->report); + hid_get_feature(hdev, field->report); td->maxcontacts = field->value[0]; if (!td->maxcontacts && @@ -490,7 +458,7 @@ static void mt_feature_mapping(struct hid_device *hdev, break; } - mt_get_feature(hdev, field->report); + hid_get_feature(hdev, field->report); if (field->value[usage->usage_index] == MT_BUTTONTYPE_CLICKPAD) td->is_buttonpad = true; @@ -498,7 +466,7 @@ static void mt_feature_mapping(struct hid_device *hdev, case 0xff0000c5: /* Retrieve the Win8 blob once to enable some devices */ if (usage->usage_index == 0) - mt_get_feature(hdev, field->report); + hid_get_feature(hdev, field->report); break; } } diff --git a/include/linux/hid.h b/include/linux/hid.h index 70679bf820ce..fce7966234de 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -1219,6 +1219,7 @@ static inline u32 hid_report_len(struct hid_report *report) int hid_report_raw_event(struct hid_device *hid, int type, u8 *data, u32 size, int interrupt); +void hid_get_feature(struct hid_device *hdev, struct hid_report *report); /* HID quirks API */ unsigned long hid_lookup_quirk(const struct hid_device *hdev);
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(-)