Re: [PATCH 09/13] comedi: ni_mio_common: Conditionally use I/O port or MMIO

[Ian Abbott]

On 13/09/2023 13:17, Arnd Bergmann wrote:
> On Wed, Sep 13, 2023, at 13:20, Ian Abbott wrote:
> > In a future patch, the port I/O functions (`inb()`, `outb()`, and
> > friends will only be declared in the `HAS_IOPORT` configuration option
> > is enabled.
> >
> > The "ni_mio_common.c" file contains calls to both port I/O functions and
> > memory-mapped I/O functions.  The file is `#include`d by "ni_atmio.c",
> > "ni_mio_cs.c", and "ni_pcimio.c" for the ni_atmio, ni_mio_cs, and
> > ni_pcimio modules, respectively.  Only "ni_pcimio.c" defines the
> > `PCIDMA` macro before including "ni_mio_common.c" and various bits of
> > code in "ni_mio_common.c" is conditionally compiled according to whether
> > that macro is defined or not.  Currently, the port I/O function calls
> > are compiled in regardless of whether the `PCIDMA` macro is defined or
> > not.  However, the fact is that the ni_atmio and ni_mio_cs modules will
> > never call the memory-mapped I/O functions, and the ni_pcimio module
> > will never call the port I/O functions.
> >
> > Calls to the port I/O and memory-mapped I/O functions is confined to the
> > `ni_writel()`, `ni_writew()`, `ni_writeb()`, `ni_readl()`, `ni_readw()`,
> > and `ni_readb()` functions which do a run-time test to decide whether to
> > call the port I/O functions or the memory-mapped I/O functions.
> > Conditionally compile two variants of the functions so they only call
> > the port I/O functions if the `PCIDMA` macro is undefined (for the
> > ni_atmio and ni_mio_cs modules), and only call the memory-mapped I/O
> > functions if the `PCIDMA` macro is defined (for the ni_pcimio module).
> >
> > Add a run-time check in the `ni_E_init()` function to return an error if
> > the comedi device has been set up to use port I/O if `PCIDMA` is
> > defined, or has been set up to use memory-mapped I/O if `PCIDMA` is not
> > defined.
> >
> > The changes make it possible to build the ni_pcimio module even if the
> > port I/O functions have not been declared.  (The ni_atmio and ni_mio_cs
> > modules do still require the port I/O functions to be declared.)
>
> I think this all works, but there is probably a simpler way to
> achieve the same:
>   
> > +#ifdef PCIDMA
> > +
> >   static void ni_writel(struct comedi_device *dev, unsigned int data, int reg)
> >   {
> > -	if (dev->mmio)
> > -		writel(data, dev->mmio + reg);
> > -	else
> > -		outl(data, dev->iobase + reg);
> > +	writel(data, dev->mmio + reg);
> >   }
> >
> >
> > +#else /* PCIDMA */
> > +
> > +static void ni_writel(struct comedi_device *dev, unsigned int data, int reg)
> > +{
> > +	outl(data, dev->iobase + reg);
> > +}
>
> We already have an abstraction for this using iowrite32(),
> which turns into either writel() or outl() depending on the
> argument, so you could just use pci_iomap() or ioport_map()
> to turn port numbers into tokens suitable for the common
> helper.
>
>       Arnd

It would affect three modules, and there are similar changes that could 
be made to other modules, so I think I'll put that suggestion on hold 
for now.  I'd also like to get rid of the '#include "ni_mio_common.c"' 
nonsense at some point, but that's a big job!

--
-=( Ian Abbott <abbotti@xxxxxxxxx> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-