[09/13] comedi: ni_mio_common: Conditionally use I/O port or MMIO
Commit Message
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.)
Cc: Arnd Bergmann <arnd@kernel.org>
Cc: Niklas Schnelle <schnelle@linux.ibm.com>
Signed-off-by: Ian Abbott <abbotti@mev.co.uk>
---
drivers/comedi/drivers/ni_mio_common.c | 70 ++++++++++++++++++--------
1 file changed, 50 insertions(+), 20 deletions(-)
Comments
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
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!
On Wed, Sep 13, 2023, at 17:45, Ian Abbott wrote:
> On 13/09/2023 13:17, Arnd Bergmann wrote:
>> On Wed, Sep 13, 2023, at 13:20, Ian Abbott wrote:
>>>
>>> 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.
>
> 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!
Sure, that's fine, I just thought I'd mention it in case you
were looking for a nicer implementation. I wouldn't have done
the current series to start with since there is not a huge amount
of value in running these drivers on obscure architectures
without HAS_IOPORT, but your changes are otherwise completely
sensible.
Arnd
@@ -46,6 +46,12 @@
#include <linux/comedi/comedi_8255.h>
#include "mite.h"
+#ifdef PCIDMA
+#define IS_PCIMIO 1
+#else
+#define IS_PCIMIO 0
+#endif
+
/* A timeout count */
#define NI_TIMEOUT 1000
@@ -219,54 +225,72 @@ enum timebase_nanoseconds {
static const int num_adc_stages_611x = 3;
+#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);
}
static void ni_writew(struct comedi_device *dev, unsigned int data, int reg)
{
- if (dev->mmio)
- writew(data, dev->mmio + reg);
- else
- outw(data, dev->iobase + reg);
+ writew(data, dev->mmio + reg);
}
static void ni_writeb(struct comedi_device *dev, unsigned int data, int reg)
{
- if (dev->mmio)
- writeb(data, dev->mmio + reg);
- else
- outb(data, dev->iobase + reg);
+ writeb(data, dev->mmio + reg);
}
static unsigned int ni_readl(struct comedi_device *dev, int reg)
{
- if (dev->mmio)
- return readl(dev->mmio + reg);
+ return readl(dev->mmio + reg);
+}
+
+static unsigned int ni_readw(struct comedi_device *dev, int reg)
+{
+ return readw(dev->mmio + reg);
+}
+
+static unsigned int ni_readb(struct comedi_device *dev, int reg)
+{
+ return readb(dev->mmio + reg);
+}
+#else /* PCIDMA */
+
+static void ni_writel(struct comedi_device *dev, unsigned int data, int reg)
+{
+ outl(data, dev->iobase + reg);
+}
+
+static void ni_writew(struct comedi_device *dev, unsigned int data, int reg)
+{
+ outw(data, dev->iobase + reg);
+}
+
+static void ni_writeb(struct comedi_device *dev, unsigned int data, int reg)
+{
+ outb(data, dev->iobase + reg);
+}
+
+static unsigned int ni_readl(struct comedi_device *dev, int reg)
+{
return inl(dev->iobase + reg);
}
static unsigned int ni_readw(struct comedi_device *dev, int reg)
{
- if (dev->mmio)
- return readw(dev->mmio + reg);
-
return inw(dev->iobase + reg);
}
static unsigned int ni_readb(struct comedi_device *dev, int reg)
{
- if (dev->mmio)
- return readb(dev->mmio + reg);
-
return inb(dev->iobase + reg);
}
+#endif /* PCIDMA */
+
/*
* We automatically take advantage of STC registers that can be
* read/written directly in the I/O space of the board.
@@ -5977,6 +6001,12 @@ static int ni_E_init(struct comedi_device *dev,
int i;
const char *dev_family = devpriv->is_m_series ? "ni_mseries"
: "ni_eseries";
+ if (!IS_PCIMIO != !dev->mmio) {
+ dev_err(dev->class_dev,
+ "%s: bug! %s device not supported.\n",
+ KBUILD_MODNAME, board->name);
+ return -ENXIO;
+ }
/* prepare the device for globally-named routes. */
if (ni_assign_device_routes(dev_family, board->name,