diff mbox

Input: usbtouchscreen - initialize eGalax devices

Message ID 20120831225353.GE24820@alittletooquiet.net (mailing list archive)
State New, archived
Headers show

Commit Message

Forest Bond Aug. 31, 2012, 10:53 p.m. UTC 
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?

Thanks again to all for the review.  My theory for why the previous patch worked
in spite of its wrongness is that the device actually switches modes when it
receives a control message with USB_TYPE_VENDOR even though the documentation
suggests an actual diagnostic packet must be received.

Does this (untested) patch look more reasonable?



Thanks,
Forest

Comments

Dmitry Torokhov Aug. 31, 2012, 11:10 p.m. UTC | #1 
On Fri, Aug 31, 2012 at 06:53:53PM -0400, Forest Bond wrote:
> 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?
> 
> Thanks again to all for the review.  My theory for why the previous patch worked
> in spite of its wrongness is that the device actually switches modes when it
> receives a control message with USB_TYPE_VENDOR even though the documentation
> suggests an actual diagnostic packet must be received.
> 
> Does this (untested) patch look more reasonable?

Yes, but we still need it tested, please.

> 
> 
> diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
> index e32709e..64b4b17 100644
> --- a/drivers/input/touchscreen/usbtouchscreen.c
> +++ b/drivers/input/touchscreen/usbtouchscreen.c
> @@ -304,6 +304,41 @@ 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;
> +	unsigned char *buf;
> +	struct usb_device *udev = interface_to_usbdev(usbtouch->interface);
> +
> +	/* An eGalax diagnostic packet kicks the device into using the right
> +	 * protocol.  We send a "check active" packet.  The response will be
> +	 * read later and ignored.
> +	 */
> +
> +	buf = kmalloc(3, GFP_KERNEL);
> +	buf[0] = EGALAX_PKT_TYPE_DIAG;
> +	buf[1] = 1;	/* length */
> +	buf[2] = 'A';	/* command - check active */
> +
> +	for (i = 0; i < 3; i++) {
> +		ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
> +				      0,
> +				      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
> +				      0, 0, buf, 3,
> +				      USB_CTRL_SET_TIMEOUT);
> +		if (ret >= 0) {
> +			ret = 0;
> +			break;
> +		}
> +		if (ret != -EPIPE)
> +			break;
> +	}
> +
> +	kfree(buf);
> +
> +	return ret;
> +}
> +
>  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 +1091,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
>  
> 
> Thanks,
> Forest
> -- 
> Forest Bond
> http://www.alittletooquiet.net
> http://www.rapidrollout.com
Forest Bond Aug. 31, 2012, 11:23 p.m. UTC | #2 
Hi Dmitry,

On Fri, Aug 31, 2012 at 04:10:47PM -0700, Dmitry Torokhov wrote:
> On Fri, Aug 31, 2012 at 06:53:53PM -0400, Forest Bond wrote:
> > 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?
> > 
> > Thanks again to all for the review.  My theory for why the previous patch worked
> > in spite of its wrongness is that the device actually switches modes when it
> > receives a control message with USB_TYPE_VENDOR even though the documentation
> > suggests an actual diagnostic packet must be received.
> > 
> > Does this (untested) patch look more reasonable?
> 
> Yes, but we still need it tested, please.

Great, I'll test it later this evening when I am back in front of the hardware
and follow up with a properly submitted patch.

Thanks again,
Forest
diff mbox

Patch

diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
index e32709e..64b4b17 100644
--- a/drivers/input/touchscreen/usbtouchscreen.c
+++ b/drivers/input/touchscreen/usbtouchscreen.c
@@ -304,6 +304,41 @@  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;
+	unsigned char *buf;
+	struct usb_device *udev = interface_to_usbdev(usbtouch->interface);
+
+	/* An eGalax diagnostic packet kicks the device into using the right
+	 * protocol.  We send a "check active" packet.  The response will be
+	 * read later and ignored.
+	 */
+
+	buf = kmalloc(3, GFP_KERNEL);
+	buf[0] = EGALAX_PKT_TYPE_DIAG;
+	buf[1] = 1;	/* length */
+	buf[2] = 'A';	/* command - check active */
+
+	for (i = 0; i < 3; i++) {
+		ret = usb_control_msg(udev, usb_sndctrlpipe(udev, 0),
+				      0,
+				      USB_DIR_OUT | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
+				      0, 0, buf, 3,
+				      USB_CTRL_SET_TIMEOUT);
+		if (ret >= 0) {
+			ret = 0;
+			break;
+		}
+		if (ret != -EPIPE)
+			break;
+	}
+
+	kfree(buf);
+
+	return ret;
+}
+
 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 +1091,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