Hi, Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> writes: > On Sat, Jan 8, 2022 at 2:10 AM Dmitry Antipov <dmanti@xxxxxxxxxxxxx> wrote: >> >> On Tue, Jan 4, 2022 at 7:52 AM Benjamin Tissoires >> <benjamin.tissoires@xxxxxxxxxx> wrote: >> > >> > On Tue, Jan 4, 2022 at 3:08 AM Dmitry Antipov <dmanti@xxxxxxxxxxxxx> >> > wrote: >> > > >> > > > -----Original Message----- >> > > > From: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> >> > > > Sent: Monday, January 3, 2022 7:27 AM >> > > > To: Dmitry Antipov <daantipov@xxxxxxxxx> >> > > > Cc: Jiri Kosina <jikos@xxxxxxxxxx>; open list:HID CORE LAYER <linux- >> > > > input@xxxxxxxxxxxxxxx>; Felipe Balbi <balbi@xxxxxxxxxx>; Dmitry >> > > > Antipov <dmanti@xxxxxxxxxxxxx> >> > > > Subject: [EXTERNAL] Re: [PATCH v1 3/5] HID: add on_transport_error() >> > > > field to struct hid_driver >> > > > >> > > > On Thu, Dec 30, 2021 at 12:11 AM Dmitry Antipov >> > > > <daantipov@xxxxxxxxx> >> > > > wrote: >> > > > > >> > > > > This new API allows a transport driver to notify the HID device >> > > > > driver about a transport layer error. >> > > > >> > > > I do not see entirely the purpose of this new callback: >> > > > >> > > > - when we receive the device initiated reset, this is a specific >> > > > device event, so it would make sense... >> > > > - but for things like >> > HID_TRANSPORT_ERROR_TYPE_BUS_OUTPUT_TRANSFER, >> > > > I would expect the caller to return that error code instead of >> > > > having an async function called. >> > > > >> > > > I think it might be simpler to add a more specific >> > > > .device_initiated_reset() callback instead of trying to be generic. >> > > > >> > > >> > > The intention of this new callback is to notify the device driver of a >> > > transport-layer error for at least two reasons: >> > > 1. Delegating the decision making. For certain types of errors the >> > > spec states that the host _may_ reset the device. Right now there are >> > > not many devices that support HID over SPI, but I wanted to allow the >> > > flexibility for each vendor to decide what cases to error-handle. >> > >> > Looking at section 9 (Error handling) of the HID SPI protocol spec, it seems that >> > the only time the host may (or not) decide to reset the device is when receiving >> > a timeout error. >> > And looking at the phrasing there, I think we ought to simply reset the device >> > anyway. >> > >> > So now that I have the spec under my eyes, I would think that for this part, the >> > host is expected to reset the device, which in turn makes this a spi-hid >> > responsibility. >> > >> > So I would suggest adding a callback notifying that the device has been reset, >> > and with a flag telling whether it's host or device initiated. >> > Then in hid-microsoft, hid-multitouch we can deal with that situation. >> > >> > Putting this at the transport layer allows for a common behavior which won't >> > depend on the leaf HID driver in use. >> >> Please note the "ready" flag that is wired to a sysfs attribute in >> spi-hid in patch 5/5. In our case the touch digitizer sends the raw >> data, so we process it and convert it into input events in a userspace >> service we call the touch daemon. The touch daemon detects digitizer >> resets via the ready flag: any time the flag goes from "not ready" to >> "ready", it is interpreted as digitizer coming out of reset and the >> touch daemon then sends some system state info to the digitizer, among >> other things. While the ready flag is "not ready", in our architecture, >> the userspace will not send ioctl's or write into the hidraw device. > > So that means that this device is forwarding the raw touch map? yes it is. Raw touch map for fingers, some other not-truly-raw reports for pen, and some vendor specific messages (mostly tuning-related and some telemetry/debug data). >> All this means that the code in hid-microsoft won't be implementing this >> new notify_of_reset() callback. Since in the final submission there >> won't be an implementation of this callback, is it worth adding at this >> stage? Can it go in as a REVISIT or a FIXME comment until such >> notification to the leaf driver is needed? > > If there is no users, then it's probably best to not implement it. We > could add a comment, yes, but maybe not a FIXME, just a regular > comment. +1 [snip] -- balbi
