| Message ID | 20120831135658.GC24820@alittletooquiet.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 31, 2012 at 09:56:58AM -0400, Forest Bond wrote: > From: Forest Bond <forest.bond@rapidrollout.com> > > Certain eGalax devices expose an interface with class HID and protocol > None. Some work with usbhid and some work with usbtouchscreen, but > there is no easy way to differentiate. Sending an eGalax diagnostic > packet seems to kick them all into using the right protocol for > usbtouchscreen, so we can continue to bind them all there (as opposed to > handing some off to usbhid). > > This fixes a regression for devices that were claimed by (and worked > with) usbhid prior to commit 139ebe8dc80dd74cb2ac9f5603d18fbf5cff049f, > which made usbtouchscreen claim them instead. With this patch they will > still be claimed by usbtouchscreen, but they will actually report events > usbtouchscreen can understand. Note that these devices will be limited > to the usbtouchscreen feature set so e.g. dual touch features are not > supported. > > I have the distinct pleasure of needing to support devices of both types > and have tested accordingly. > > Signed-off-by: Forest Bond <forest.bond@rapidrollout.com> Applied, thanks Forest. > --- > drivers/input/touchscreen/usbtouchscreen.c | 25 +++++++++++++++++++++++++ > 1 files changed, 25 insertions(+), 0 deletions(-) > > diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c > index e32709e..2ce5308 100644 > --- a/drivers/input/touchscreen/usbtouchscreen.c > +++ b/drivers/input/touchscreen/usbtouchscreen.c > @@ -304,6 +304,30 @@ static int e2i_read_data(struct usbtouch_usb *dev, unsigned char *pkt) > #define EGALAX_PKT_TYPE_REPT 0x80 > #define EGALAX_PKT_TYPE_DIAG 0x0A > > +static int egalax_init(struct usbtouch_usb *usbtouch) > +{ > + int ret, i; > + struct usb_device *udev = interface_to_usbdev(usbtouch->interface); > + > + /* An eGalax diagnostic packet kicks the device into using the right > + * protocol. */ > + for (i = 0; i < 3; i++) { > + /* Send a "check active" packet. The response will be read > + * later and ignored. */ > + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), > + 0, > + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > + 0, 0, "\x0A\x01A", 0, > + USB_CTRL_SET_TIMEOUT); > + if (ret >= 0) > + break; > + if (ret != -EPIPE) > + return ret; > + } > + > + return 0; > +} > + > static int egalax_read_data(struct usbtouch_usb *dev, unsigned char *pkt) > { > if ((pkt[0] & EGALAX_PKT_TYPE_MASK) != EGALAX_PKT_TYPE_REPT) > @@ -1056,6 +1080,7 @@ static struct usbtouch_device_info usbtouch_dev_info[] = { > .process_pkt = usbtouch_process_multi, > .get_pkt_len = egalax_get_pkt_len, > .read_data = egalax_read_data, > + .init = egalax_init, > }, > #endif > > -- > 1.7.0.4
Hello. On 08/31/2012 05:56 PM, Forest Bond wrote: > From: Forest Bond <forest.bond@rapidrollout.com> > Certain eGalax devices expose an interface with class HID and protocol > None. Some work with usbhid and some work with usbtouchscreen, but > there is no easy way to differentiate. Sending an eGalax diagnostic > packet seems to kick them all into using the right protocol for > usbtouchscreen, so we can continue to bind them all there (as opposed to > handing some off to usbhid). > This fixes a regression for devices that were claimed by (and worked > with) usbhid prior to commit 139ebe8dc80dd74cb2ac9f5603d18fbf5cff049f, Please also specify that commit's summary ion parens. > which made usbtouchscreen claim them instead. With this patch they will > still be claimed by usbtouchscreen, but they will actually report events > usbtouchscreen can understand. Note that these devices will be limited > to the usbtouchscreen feature set so e.g. dual touch features are not > supported. > I have the distinct pleasure of needing to support devices of both types > and have tested accordingly. > Signed-off-by: Forest Bond <forest.bond@rapidrollout.com> > --- > drivers/input/touchscreen/usbtouchscreen.c | 25 +++++++++++++++++++++++++ > 1 files changed, 25 insertions(+), 0 deletions(-) > diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c > index e32709e..2ce5308 100644 > --- a/drivers/input/touchscreen/usbtouchscreen.c > +++ b/drivers/input/touchscreen/usbtouchscreen.c > @@ -304,6 +304,30 @@ static int e2i_read_data(struct usbtouch_usb *dev, unsigned char *pkt) > #define EGALAX_PKT_TYPE_REPT 0x80 > #define EGALAX_PKT_TYPE_DIAG 0x0A > > +static int egalax_init(struct usbtouch_usb *usbtouch) > +{ > + int ret, i; > + struct usb_device *udev = interface_to_usbdev(usbtouch->interface); > + > + /* An eGalax diagnostic packet kicks the device into using the right > + * protocol. */ The preferred multi-line comment style is: /* * bla * bla */ > + for (i = 0; i < 3; i++) { > + /* Send a "check active" packet. The response will be read > + * later and ignored. */ > + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), > + 0, > + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > + 0, 0, "\x0A\x01A", 0, You probably can't send data from the .const section (as well as off the stack) -- they can be DMA'ed and there'll be issues with cache consistency on non-x86 arches. You should allocate the data with kmalloc(). Although, on the second thought, maybe I'm wrong in this case... not really sure about sending -- receiving (to the .data section) could certainly be harmful... WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 31, 2012 at 10:50:38PM +0400, Sergei Shtylyov wrote: > Hello. > > On 08/31/2012 05:56 PM, Forest Bond wrote: > > > From: Forest Bond <forest.bond@rapidrollout.com> > > > Certain eGalax devices expose an interface with class HID and protocol > > None. Some work with usbhid and some work with usbtouchscreen, but > > there is no easy way to differentiate. Sending an eGalax diagnostic > > packet seems to kick them all into using the right protocol for > > usbtouchscreen, so we can continue to bind them all there (as opposed to > > handing some off to usbhid). > > > This fixes a regression for devices that were claimed by (and worked > > with) usbhid prior to commit 139ebe8dc80dd74cb2ac9f5603d18fbf5cff049f, > > Please also specify that commit's summary ion parens. > > > which made usbtouchscreen claim them instead. With this patch they will > > still be claimed by usbtouchscreen, but they will actually report events > > usbtouchscreen can understand. Note that these devices will be limited > > to the usbtouchscreen feature set so e.g. dual touch features are not > > supported. > > > I have the distinct pleasure of needing to support devices of both types > > and have tested accordingly. > > > Signed-off-by: Forest Bond <forest.bond@rapidrollout.com> > > --- > > drivers/input/touchscreen/usbtouchscreen.c | 25 +++++++++++++++++++++++++ > > 1 files changed, 25 insertions(+), 0 deletions(-) > > > diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c > > index e32709e..2ce5308 100644 > > --- a/drivers/input/touchscreen/usbtouchscreen.c > > +++ b/drivers/input/touchscreen/usbtouchscreen.c > > @@ -304,6 +304,30 @@ static int e2i_read_data(struct usbtouch_usb *dev, unsigned char *pkt) > > #define EGALAX_PKT_TYPE_REPT 0x80 > > #define EGALAX_PKT_TYPE_DIAG 0x0A > > > > +static int egalax_init(struct usbtouch_usb *usbtouch) > > +{ > > + int ret, i; > > + struct usb_device *udev = interface_to_usbdev(usbtouch->interface); > > + > > + /* An eGalax diagnostic packet kicks the device into using the right > > + * protocol. */ > > The preferred multi-line comment style is: > > /* > * bla > * bla > */ > > > + for (i = 0; i < 3; i++) { > > + /* Send a "check active" packet. The response will be read > > + * later and ignored. */ > > + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), > > + 0, > > + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > > + 0, 0, "\x0A\x01A", 0, > > You probably can't send data from the .const section (as well as off the > stack) -- they can be DMA'ed and there'll be issues with cache consistency on > non-x86 arches. You should allocate the data with kmalloc(). > Although, on the second thought, maybe I'm wrong in this case... not really > sure about sending -- receiving (to the .data section) could certainly be harmful... Hmm, do we actually send anything here? The "size" passed to usb_control_msg() is 0 so I don't think we use that data at all... Thanks.
On Fri, 31 Aug 2012, Dmitry Torokhov wrote: > > > + /* Send a "check active" packet. The response will be read > > > + * later and ignored. */ > > > + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), > > > + 0, > > > + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > > > + 0, 0, "\x0A\x01A", 0, > > > > You probably can't send data from the .const section (as well as off the > > stack) -- they can be DMA'ed and there'll be issues with cache consistency on > > non-x86 arches. You should allocate the data with kmalloc(). > > Although, on the second thought, maybe I'm wrong in this case... not really > > sure about sending -- receiving (to the .data section) could certainly be harmful... > > Hmm, do we actually send anything here? The "size" passed to > usb_control_msg() is 0 so I don't think we use that data at all... Good point. Perhaps the 0 is a typo, in which case data does get sent and the buffer must be kmalloc'ed. If the 0 is correct then the buffer should be NULL, not "\x0A\x01A" (and what's the purpose of the leading '0' in the second byte?). In addition, although the bRequestType specifies USB_DIR_OUT, the pipe value is usb_rcvctrlpipe. Is this transfer meant to be IN or OUT? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Fri, Aug 31, 2012 at 04:04:58PM -0400, Alan Stern wrote: > On Fri, 31 Aug 2012, Dmitry Torokhov wrote: > > > > > + /* Send a "check active" packet. The response will be read > > > > + * later and ignored. */ > > > > + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), > > > > + 0, > > > > + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > > > > + 0, 0, "\x0A\x01A", 0, > > > > > > You probably can't send data from the .const section (as well as off the > > > stack) -- they can be DMA'ed and there'll be issues with cache consistency on > > > non-x86 arches. You should allocate the data with kmalloc(). > > > Although, on the second thought, maybe I'm wrong in this case... not really > > > sure about sending -- receiving (to the .data section) could certainly be harmful... > > > > Hmm, do we actually send anything here? The "size" passed to > > usb_control_msg() is 0 so I don't think we use that data at all... > > Good point. Perhaps the 0 is a typo, in which case data does get sent > and the buffer must be kmalloc'ed. If the 0 is correct then the buffer > should be NULL, not "\x0A\x01A" (and what's the purpose of the leading > '0' in the second byte?). > > In addition, although the bRequestType specifies USB_DIR_OUT, the pipe > value is usb_rcvctrlpipe. Is this transfer meant to be IN or OUT? The primary issue is that I'm a USB newb working on a deadline. ;) My intention with this call was to send those three bytes out to the device. I tested this in Python using a libusb binding. Oddly enough, the code as written does in fact work in the sense that the device talks to the driver correctly after initialization (and without it it doesn't). Anyway, thanks for the review and sorry for being a bonehead. I'll resend. Thanks, Forest
diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c index e32709e..2ce5308 100644 --- a/drivers/input/touchscreen/usbtouchscreen.c +++ b/drivers/input/touchscreen/usbtouchscreen.c @@ -304,6 +304,30 @@ static int e2i_read_data(struct usbtouch_usb *dev, unsigned char *pkt) #define EGALAX_PKT_TYPE_REPT 0x80 #define EGALAX_PKT_TYPE_DIAG 0x0A +static int egalax_init(struct usbtouch_usb *usbtouch) +{ + int ret, i; + struct usb_device *udev = interface_to_usbdev(usbtouch->interface); + + /* An eGalax diagnostic packet kicks the device into using the right + * protocol. */ + for (i = 0; i < 3; i++) { + /* Send a "check active" packet. The response will be read + * later and ignored. */ + ret = usb_control_msg(udev, usb_rcvctrlpipe(udev, 0), + 0, + USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE, + 0, 0, "\x0A\x01A", 0, + USB_CTRL_SET_TIMEOUT); + if (ret >= 0) + break; + if (ret != -EPIPE) + return ret; + } + + return 0; +} + static int egalax_read_data(struct usbtouch_usb *dev, unsigned char *pkt) { if ((pkt[0] & EGALAX_PKT_TYPE_MASK) != EGALAX_PKT_TYPE_REPT) @@ -1056,6 +1080,7 @@ static struct usbtouch_device_info usbtouch_dev_info[] = { .process_pkt = usbtouch_process_multi, .get_pkt_len = egalax_get_pkt_len, .read_data = egalax_read_data, + .init = egalax_init, }, #endif