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

[Arnd Bergmann]

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