Re: [PATCH V2 1/1] usb:serial:f81534 Add F81532/534 Driver

[Johan Hovold <johan@kernel.org>]

On Thu, Jun 25, 2015 at 10:06:07AM +0200, Johan Hovold wrote:
> Hi Peter,
> 
> On Thu, Jun 25, 2015 at 01:16:56PM +0800, Peter Hung wrote:
> > Hello Johan,
> > 
> > Peter Hung 於 2015/6/15 上午 09:54 寫道:
> > > This driver is for Fintek F81532/F81534 USB to Serial Ports IC.
> > > 
> > > Features:
> > > 1. F81534 is 1-to-4 & F81532 is 1-to-2 serial ports IC
> > > 2. Support Baudrate from B50 to B1500000 (excluding B1000000).
> > > 3. The RTS signal can be transformed their behavior with configuration
> > >     for transceiver (for RS232/RS485/RS422) (/sys/class/ttyUSBx/uart_mode)
> > > 4. There are 4x3 output-only GPIOs to control transceiver mode. It's
> > >     can be controlled via sysfs (/sys/class/ttyUSBx/gpio)
> > > 
> > 
> > Do you receive my patch?
> 
> Yes, I did. I just haven't had time to review it yet.
> 
> > Are there anything should I do to improve it ?
> 
> There are, including
> 
>  - your custom read and write implementations look odd, you should be
>    able to reuse a lot more of the generic framework
>  - you'll also need to implement open/and close in some way
>    (enable/disable in hardware or flag in software) as you should not
>    push data to a closed tty
>  - stack allocated buffers used for DMA (register accessors)
>  - DMA buffers allocated as part of larger struct (read/write buffers)
>  - lots of magic constants (e.g. use defines for the registers)
>  - As Greg already mentioned, you need to implement gpio support using
>    gpiolib, not a custom sysfs interface
> 
> I don't have time to look closer at the architectural bits until next
> week I'm afraid, but perhaps you could start with the above.

I took a closer look at the protocol, and you should definitely be able
to reuse more of the generic implementation, at least for the read path.

You should also make sure to document the bulk-message layout somewhere
in the driver. Do you always have to send full 512-byte messages?

Use the read-urbs already allocated by usb-serial core, and take a look
at how this is handled in mxuport (e.g. submit port0 read urbs at
attach, and demultiplex in the process_read_urb callback). Your
message-parsing code needs to be cleaned up.

For the write-path, you at least need to use a fifo per port (usb-serial
core will have allocated one for the first port, again see mxuport).
Depending on the firmware implementation you may be able to reuse the
generic implementation as mxuport does, or you need to keep a custom
implementation. Could you elaborate on what buffers you have in the
firmware? What happens if you submit data for a port before receiving a
"tx empty" notification over the bulk-in endpoints?

Also make sure to remove all unused code (e.g. the READ_AND_SET macro)
before resubmitting.

Thanks,
Johan