[PATCH 0/3] opticon driver fixes

[PATCH 0/3] opticon driver fixes

[Alon Ziv]

These patches get the opticon driver to actually work using my OPN2001.

A userspace library is still needed; a good start can be had using the
Python script from
http://majid.info/blog/a-python-driver-for-the-symbol-cs-1504-bar-code-scanner/

Alon Ziv (3):
  Fix long-standing bugs in opticon driver
  Add Opticon OPN2001 write support
  Whitespace fixes in opticon driver

 drivers/usb/serial/opticon.c |   44 +++++++++++++++++++++++++++++++----------
 1 files changed, 33 insertions(+), 11 deletions(-)

[PATCH 1/3] Fix long-standing bugs in opticon driver

[Alon Ziv]

The bulk-read callback had two bugs:
a) The bulk-in packet's leading two zeros were returned (and the two last
   bytes truncated)
b) The wrong URB was transmitted for the second (and later) read requests,
   causing further reads to return the entire packet (including leading
   zeros)

Signed-off-by: Alon Ziv <alon-git@nolaviz.org>
---
 drivers/usb/serial/opticon.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
index ed01f3b..9ff19c8 100644
--- a/drivers/usb/serial/opticon.c
+++ b/drivers/usb/serial/opticon.c
@@ -96,8 +96,8 @@ static void opticon_bulk_callback(struct urb *urb)
 			/* real data, send it to the tty layer */
 			tty = tty_port_tty_get(&port->port);
 			if (tty) {
-				tty_insert_flip_string(tty, data,
-							       data_length);
+				tty_insert_flip_string(tty, data + 2,
+						       data_length);
 				tty_flip_buffer_push(tty);
 				tty_kref_put(tty);
 			}
@@ -130,7 +130,7 @@ exit:
 						  priv->bulk_address),
 				  priv->bulk_in_buffer, priv->buffer_size,
 				  opticon_bulk_callback, priv);
-		result = usb_submit_urb(port->read_urb, GFP_ATOMIC);
+		result = usb_submit_urb(priv->bulk_read_urb, GFP_ATOMIC);
 		if (result)
 			dev_err(&port->dev,
 			    "%s - failed resubmitting read urb, error %d\n",
-- 
1.6.3.3

[PATCH 2/3] Add Opticon OPN2001 write support

[Alon Ziv]

OPN2001 expects write operations to arrive as a vendor-specific command
through the control pipe (instead of using a separate bulk-out pipe).

Signed-off-by: Alon Ziv <alon-git@nolaviz.org>
---
 drivers/usb/serial/opticon.c |   30 ++++++++++++++++++++++++++----
 1 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
index 9ff19c8..4fe7c3d 100644
--- a/drivers/usb/serial/opticon.c
+++ b/drivers/usb/serial/opticon.c
@@ -187,6 +187,9 @@ static void opticon_write_bulk_callback(struct urb *urb)
 	/* free up the transfer buffer, as usb_free_urb() does not do this */
 	kfree(urb->transfer_buffer);
 
+	/* setup packet may be set if we're using it for writing */
+	kfree(urb->setup_packet);
+
 	if (status)
 		dbg("%s - nonzero write bulk status received: %d",
 		    __func__, status);
@@ -237,10 +240,29 @@ static int opticon_write(struct tty_struct *tty, struct usb_serial_port *port,
 
 	usb_serial_debug_data(debug, &port->dev, __func__, count, buffer);
 
-	usb_fill_bulk_urb(urb, serial->dev,
-			  usb_sndbulkpipe(serial->dev,
-					  port->bulk_out_endpointAddress),
-			  buffer, count, opticon_write_bulk_callback, priv);
+	if (port->bulk_out_endpointAddress) {
+		usb_fill_bulk_urb(urb, serial->dev,
+				  usb_sndbulkpipe(serial->dev,
+						  port->bulk_out_endpointAddress),
+				  buffer, count, opticon_write_bulk_callback, priv);
+	} else {
+		struct usb_ctrlrequest *dr;
+
+		dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
+		if (!dr)
+			return -ENOMEM;
+
+		dr->bRequestType = USB_TYPE_VENDOR | USB_RECIP_INTERFACE | USB_DIR_OUT;
+		dr->bRequest = 0x01;
+		dr->wValue = 0;
+		dr->wIndex = 0;
+		dr->wLength = cpu_to_le16(count);
+
+		usb_fill_control_urb(urb, serial->dev,
+			usb_sndctrlpipe(serial->dev, 0),
+			(unsigned char *)dr, buffer, count,
+			opticon_write_bulk_callback, priv);
+	}
 
 	/* send it down the pipe */
 	status = usb_submit_urb(urb, GFP_ATOMIC);
-- 
1.6.3.3

[PATCH 3/3] Whitespace fixes in opticon driver

[Alon Ziv]

Signed-off-by: Alon Ziv <alon-git@nolaviz.org>
---
 drivers/usb/serial/opticon.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
index 4fe7c3d..eda1f92 100644
--- a/drivers/usb/serial/opticon.c
+++ b/drivers/usb/serial/opticon.c
@@ -108,10 +108,10 @@ static void opticon_bulk_callback(struct urb *urb)
 				else
 					priv->rts = true;
 			} else {
-			dev_dbg(&priv->udev->dev,
-				"Unknown data packet received from the device:"
-				" %2x %2x\n",
-				data[0], data[1]);
+				dev_dbg(&priv->udev->dev,
+					"Unknown data packet received from the device:"
+					" %2x %2x\n",
+					data[0], data[1]);
 			}
 		}
 	} else {
-- 
1.6.3.3

Re: [PATCH 2/3] Add Opticon OPN2001 write support

[Sergei Shtylyov]

Hello.

On 10-10-2010 10:32, Alon Ziv wrote:

> OPN2001 expects write operations to arrive as a vendor-specific command
> through the control pipe (instead of using a separate bulk-out pipe).

> Signed-off-by: Alon Ziv<alon-git@nolaviz.org>
> ---
>   drivers/usb/serial/opticon.c |   30 ++++++++++++++++++++++++++----
>   1 files changed, 26 insertions(+), 4 deletions(-)

> diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
> index 9ff19c8..4fe7c3d 100644
> --- a/drivers/usb/serial/opticon.c
> +++ b/drivers/usb/serial/opticon.c
[...]
> @@ -237,10 +240,29 @@ static int opticon_write(struct tty_struct *tty, struct usb_serial_port *port,
>
>   	usb_serial_debug_data(debug,&port->dev, __func__, count, buffer);
>
> -	usb_fill_bulk_urb(urb, serial->dev,
> -			  usb_sndbulkpipe(serial->dev,
> -					  port->bulk_out_endpointAddress),
> -			  buffer, count, opticon_write_bulk_callback, priv);
> +	if (port->bulk_out_endpointAddress) {
> +		usb_fill_bulk_urb(urb, serial->dev,
> +				  usb_sndbulkpipe(serial->dev,
> +						  port->bulk_out_endpointAddress),
> +				  buffer, count, opticon_write_bulk_callback, priv);

    Please align all follow-up lines uniformly.

> +	} else {
> +		struct usb_ctrlrequest *dr;
> +
> +		dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);

    sizeof(*dr) is a preferred form.

> +		usb_fill_control_urb(urb, serial->dev,
> +			usb_sndctrlpipe(serial->dev, 0),
> +			(unsigned char *)dr, buffer, count,
> +			opticon_write_bulk_callback, priv);

    More tabs needed for alignment, I think...

WBR, Sergei

Re: [PATCH 2/3] Add Opticon OPN2001 write support

[Greg KH]

On Sun, Oct 10, 2010 at 02:53:19PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> On 10-10-2010 10:32, Alon Ziv wrote:
>
>> OPN2001 expects write operations to arrive as a vendor-specific command
>> through the control pipe (instead of using a separate bulk-out pipe).
>
>> Signed-off-by: Alon Ziv<alon-git@nolaviz.org>
>> ---
>>   drivers/usb/serial/opticon.c |   30 ++++++++++++++++++++++++++----
>>   1 files changed, 26 insertions(+), 4 deletions(-)
>
>> diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
>> index 9ff19c8..4fe7c3d 100644
>> --- a/drivers/usb/serial/opticon.c
>> +++ b/drivers/usb/serial/opticon.c
> [...]
>> @@ -237,10 +240,29 @@ static int opticon_write(struct tty_struct *tty, struct usb_serial_port *port,
>>
>>   	usb_serial_debug_data(debug,&port->dev, __func__, count, buffer);
>>
>> -	usb_fill_bulk_urb(urb, serial->dev,
>> -			  usb_sndbulkpipe(serial->dev,
>> -					  port->bulk_out_endpointAddress),
>> -			  buffer, count, opticon_write_bulk_callback, priv);
>> +	if (port->bulk_out_endpointAddress) {
>> +		usb_fill_bulk_urb(urb, serial->dev,
>> +				  usb_sndbulkpipe(serial->dev,
>> +						  port->bulk_out_endpointAddress),
>> +				  buffer, count, opticon_write_bulk_callback, priv);
>
>    Please align all follow-up lines uniformly.

Not a big deal.

>> +	} else {
>> +		struct usb_ctrlrequest *dr;
>> +
>> +		dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
>
>    sizeof(*dr) is a preferred form.

No, people disagree on this, there is no 'preferred' form, you are free
to use either way.

>> +		usb_fill_control_urb(urb, serial->dev,
>> +			usb_sndctrlpipe(serial->dev, 0),
>> +			(unsigned char *)dr, buffer, count,
>> +			opticon_write_bulk_callback, priv);
>
>    More tabs needed for alignment, I think...

It's fine, not a big deal.

thanks,

greg k-h

Re: [PATCH 2/3] Add Opticon OPN2001 write support

[Sergei Shtylyov]

Hello.

On 10/10/10 16:34, Greg KH wrote:

>>> OPN2001 expects write operations to arrive as a vendor-specific command
>>> through the control pipe (instead of using a separate bulk-out pipe).

>>> Signed-off-by: Alon Ziv<alon-git@nolaviz.org>
>>> ---
>>>    drivers/usb/serial/opticon.c |   30 ++++++++++++++++++++++++++----
>>>    1 files changed, 26 insertions(+), 4 deletions(-)

>>> diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
>>> index 9ff19c8..4fe7c3d 100644
>>> --- a/drivers/usb/serial/opticon.c
>>> +++ b/drivers/usb/serial/opticon.c
>> [...]

>>> +	} else {
>>> +		struct usb_ctrlrequest *dr;
>>> +
>>> +		dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);

>>     sizeof(*dr) is a preferred form.

> No, people disagree on this, there is no 'preferred' form, you are free
> to use either way.

    Please refer to the CodingStyle chapter 14. This is preferred form, 
according to it...

WBR, Sergei

Re: [PATCH 2/3] Add Opticon OPN2001 write support

[Greg KH]

On Sun, Oct 10, 2010 at 05:58:39PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> On 10/10/10 16:34, Greg KH wrote:
>
>>>> OPN2001 expects write operations to arrive as a vendor-specific command
>>>> through the control pipe (instead of using a separate bulk-out pipe).
>
>>>> Signed-off-by: Alon Ziv<alon-git@nolaviz.org>
>>>> ---
>>>>    drivers/usb/serial/opticon.c |   30 ++++++++++++++++++++++++++----
>>>>    1 files changed, 26 insertions(+), 4 deletions(-)
>
>>>> diff --git a/drivers/usb/serial/opticon.c b/drivers/usb/serial/opticon.c
>>>> index 9ff19c8..4fe7c3d 100644
>>>> --- a/drivers/usb/serial/opticon.c
>>>> +++ b/drivers/usb/serial/opticon.c
>>> [...]
>
>>>> +	} else {
>>>> +		struct usb_ctrlrequest *dr;
>>>> +
>>>> +		dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
>
>>>     sizeof(*dr) is a preferred form.
>
>> No, people disagree on this, there is no 'preferred' form, you are free
>> to use either way.
>
>    Please refer to the CodingStyle chapter 14. This is preferred form, 
> according to it...

Hm, I didn't realize it had been written down.

Anyway, again, no big deal, it's up to the author/maintainer which way
they prefer, and for me, I can handle it either way.

thanks,

greg k-h