Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190308123432.20520-1-saumah@gmail.com>
In-Reply-To: <20190308123432.20520-1-saumah@gmail.com>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Mon, 11 Mar 2019 16:05:28 +0100
Message-ID: <CAK8P3a2yKZqB1CtYSSrhHQmRDdE-GpwdHMifgPB5Ledv-UJzWw@mail.gmail.com>
Subject: Re: [PATCH 2/4] Add SUNIX Multi-I/O card device driver
To:     Morris Ku <saumah@gmail.com>
Cc:     gregkh <gregkh@linuxfoundation.org>, morris_ku@sunix.com,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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