[[PATCH 0/3] 8250: Split Fintek PCIE to UART to independent file] On 19/01/2016 (Tue 10:41) Peter Hung wrote: > Fintek F81504/508/512 is a multi-functional PCIE device. It contains > GPIO and serial port with high baudrate & RTS auto direction for RS485. > Some general high level comments (meaning I've not looked at the code in detail, but have looked at this series) and I wonder about some issues: > The serial ports support from 50bps to 1.5Mbps with Linux baudrate > define excluding 1.0Mbps due to not support 16MHz clock source. How does this differ from what was achieved or possible with the old way of things? What was the limitation in the existing 8250 code sharing that required Fintek code to fork and become independent? How much code was just copied 8250 boilerplate vs. being a new implementation? The diffstat shows approx 500 lines of new code. What does that add vs. just copying? Don't get me wrong -- forking workarounds for buggy hardware into smaller workaround files and/or Kconfigs can be a win for everyone else, but we should be clear on why we do them. > > IC function list: > F81504: Max 2x8 GPIOs and max 4 serial ports > port2/3 are multi-function > F81508: Max 6x8 GPIOs and max 8 serial ports > port2/3 are multi-function, port8/9/10/11 are gpio only > F81512: Max 6x8 GPIOs and max 12 serial ports > port2/3/8/9/10/11 are multi-function > > We'll spilt from 8250_pci.c to new file 8250_fintek_pci.c and make it > as a kernel module with first & second patch, implements GPIOLIB with > third patch. > > Peter Hung (3): > serial: 8250_pci: Remove Fintek PCIE UART driver If someone had 8250 (PCI) builtin before, and Fintek stops working, they will most guaranteed bisect to this commit above where you remove support. That is less than ideal. We try to avoid code deletions or Kconfig addtions that will be obvious bisect magnets. > 8250_fintek_pci: Add Fintek PCIE UART driver This creates a new Kconfig var. which is default=m. How does that work if people were using these for built-in early console support in the past? Are these cards universal, or should it be default=m if (...) based on a Kconfig where this hardware exists? > 8250_fintek_pci: Add GPIOLIB support What does this add? The commit log is not at all clear. Leaving me to ask if it does belong in the core PCI support code at all? I honestly don't know, since I don't know the hardware details here. The commit long logs could go a long way to closing this knowledge gap if the 0/N listed the shortcomings and the 3/3 here indicated what the GPIO magic had managed to add. Again, this may be obvious to others, but the long logs should try and give a hint to people on the fringe who maybe don't have all the specific Fintek hardware details when reading the logs. P. -- > > drivers/tty/serial/8250/8250_fintek_pci.c | 767 ++++++++++++++++++++++++++++++ > drivers/tty/serial/8250/8250_pci.c | 201 -------- > drivers/tty/serial/8250/Kconfig | 9 + > drivers/tty/serial/8250/Makefile | 1 + > 4 files changed, 777 insertions(+), 201 deletions(-) > create mode 100644 drivers/tty/serial/8250/8250_fintek_pci.c > > -- > 1.9.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
