Messages in this thread | | | From | Arnd Bergmann <> | Date | Mon, 11 Mar 2019 16:05:28 +0100 | Subject | Re: [PATCH 2/4] Add SUNIX Multi-I/O card device driver |
| |
On Fri, Mar 8, 2019 at 1:34 PM Morris Ku <saumah@gmail.com> wrote: > > Driver for SUNIX Multi-I/O card. > Based on driver/char/serial.c by Linus Torvalds, Theodore Ts'o. > > SUNIX serial card designed with SUNIX UART controller and > compatible with 16C950 UART specification. > > Signed-off-by: Morris Ku <saumah@gmail.com>
Hi Morris,
Thanks for your submission!
Enrico has already replied with the most important comments. Here are just some more high-level thoughts from me:
* We want the functionality to be present in subsystem specific standalone drivers, i.e. drivers/tty/serial/, drivers/parport/, drivers/gpio etc, as Enrico said
* It looks like you may need some glue logic to tie those together for the multi-I/O cards. This is a bit tricky but doable. Many other multi-I/O cards simply appear as a set of PCI functions that are connected to a single slot. This is the ideal case, as you can simply have one driver per function and they do not have to be linked together at all. If this is not possible, what you may need to do here is to have a small driver in drivers/mfd/ that handles the PCI function and creates platform_device instances that the individual drivers can bind to. Have a look at the various files in drivers/mfd that come with a 'struct pci_driver' instance to see what I mean.
* In any way, new drivers must probing according to our driver module if possible. This means you should have a 'pci_driver' structure listing the possible vendor/device ID combinations, and register that with 'module_pci_driver'. If you use mfd_cell to register sub-drivers, use 'module_platform_driver' to register the driver for those.
* do not implement your own ioctl handlers. If you need a custom interface for something hardware specific, split that out into a separate patch, so the base support can be reviewed independently.
* If the functions (in particular serial and parport) have a similar register layout to existing drivers, try to reuse the existing code and extend it, rather than duplicating the implementation.
Arnd
| |