Re: PATCH: ehci-q.c

["Felipe Balbi" <felipebalbi@xxxxxxxxxxxxxxxxxxxxx>]

Hi,

On Nov 23, 2007 8:04 AM, Sergio Arcangeli <s.arcangeli@xxxxxxxxxxxxx> wrote:
> Hi all,
>
>     I apologize for my previous unreadable message... sorry.
>
>
>     I'm workig on a porting on linux of a device driver for a double-side
> scanner.
>
>     This device is equipped by a Cypress FX2 usb2.0 controller that, by the
> OEM, is configured
> for bulk I/O with EndPoint "MaxPacketSize=1024", non usb2.0-compliant but
> supported by hardware and software device drivers on MS-Windows systems.
>
> This problem is known (cfr the J. C. Andrus -
> http://lkml.org/lkml/2007/4/13/199) but is still present also in the recent
> kernel release: When the device send packets which were 1024 bytes in size,
> the urb came back with a status set to -EOVERFLOW.
>
>     Instead of the solution proposed by J. C. Andrus, that is implemented
> for backward compatibility with a special urb flag that implies a more
> complex coding of bulk transfers, I made a simple "Endpoint Descriptor"
> driven solution, using the same logic alwais used for "INTERRUPT" EndPoint,
> that is still backward-compatible, but supports also new devices
> non-usb2.0-compliants.
>
>     This solution is also "transparent" to current usb libraries and no
> special urb-flags are required, permitting the use of the high-level
> functions of current usblibs.
>
>     In all tests I made, all USB devices working with Bulk transfer I
> connected (External HD&DVD, PenDrives, TV-Tuner and SkypePhone) are still
> working with their alredy installed drivers.
>
>     It's possible to include this patch (or an analogue but working
> solution) in next releases of usb kernel module ?
>
>
> All comments will be appreciated !
>
> Thanks and ciao,
>   Sergio
>
> _____________________________________________________________________________________________
>
> --- ehci-q.c_old 2007-07-09 01:32:17.000000000 +0200
> +++ ehci-q.c 2007-11-23 02:16:14.000000000 +0100
> @@ -20,6 +20,14 @@
>
> /*-------------------------------------------------------------------------*/
>
> +/* NOTE NOTE NOTE NOTE NOTE NOTE NOTE NOTE NOTE NOTE NOTE NOTE NOTE NOTE
> NOTE
> + * Patched by S.Arcangeli to let it work with Cypress FX2 chipset that
> allows
> + * non 2.0-compliant Bulk transfers. The queue always uses the
> MaxPacketSize
> + * value declared by the EndPoint Descriptor instead of the 512 Fixed
> value.
> + * By this way the queue is well working also with these non standard
> devices.
> + * See down rows 459-460, 764-765
> + */
> +

Remove the above comments, this will go as a git-log entry, no need to
put it inside c-file.

>  /*
>   * EHCI hardware queue manipulation ... the core.  QH/QTD manipulation.
>   *
> @@ -448,6 +456,8 @@
>  #define hb_mult(wMaxPacketSize) (1 + (((wMaxPacketSize) >> 11) & 0x03))
>  // ... and packet size, for any kind of endpoint descriptor

Comments in kernel are /* */

>  #define max_packet(wMaxPacketSize) ((wMaxPacketSize) & 0x07ff)
> +// ... but bulk endpoints can work only with packetsize of 512 or 1024
> bytes

dito and there's a line wrap here. Fix your mailer.

> +#define max_bulkpacket(wMaxPacketSize) (max_packet(wMaxPacketSize) > 512 ?
> 1024 : 512)

fix your mailer

>
>  /*
>   * reverse of qh_urb_transaction:  free a list of TDs.
> @@ -751,7 +761,8 @@
>                          info2 |= (EHCI_TUNE_MULT_HS << 30);
>                  } else if (type == PIPE_BULK) {
>                          info1 |= (EHCI_TUNE_RL_HS << 28);
> -                        info1 |= 512 << 16;     /* usb2 fixed maxpacket */
> +                        //info1 |= 512 << 16;   /* Original source:
> strictly usb2.0 compliant 512 bytes fixed maxpacket */
> +                        info1 |= max_bulkpacket(maxp) << 16;  /* Uses the
> maxpacket from EP Descriptor, can be only 1024 or 512*/

Commented code. No, no, no. We can always get the history of the file
by using git-show,
git-whatchanged, git-annotate, etc. Only change the code, don't keep
the older version


>                          info2 |= (EHCI_TUNE_MULT_HS << 30);
>                  } else {                /* PIPE_INTERRUPT */
>                          info1 |= max_packet (maxp) << 16;


Please read Documentation/SubmittingPatches and
Documentation/CodingStyle before sending
such patches. Also try to use scripts/checkpatch.pl to get usual
coding style issues.

-- 
Best Regards,

Felipe Balbi
felipebalbi@xxxxxxxxxxxxxxxxxxxxx

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-devel@xxxxxxxxxxxxxxxxxxxxx
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel