Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] comedi: Make ni_mio_common a standalone module
To:     =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <uwe@kleine-koenig.org>,
        H Hartley Sweeten <hsweeten@visionengravers.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc:     linux-kernel@vger.kernel.org, Lee Jones <lee.jones@linaro.org>,
        Dan Carpenter <dan.carpenter@oracle.com>
References: <20210522115345.821578-1-uwe@kleine-koenig.org>
From:   Ian Abbott <abbotti@mev.co.uk>
Message-ID: <6a09f2b6-afda-a901-abe6-1b45d51988e8@mev.co.uk>
Date:   Sat, 22 May 2021 18:54:38 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.0
MIME-Version: 1.0
In-Reply-To: <20210522115345.821578-1-uwe@kleine-koenig.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 22/05/2021 12:53, Uwe Kleine-König wrote:
> This allows to get rid of the ugly
> 
> 	#include "ni_mio_common.c"
> 
> in three modules.
> 
> For an amd64 allmodconfig this changed the size for the following object
> files:
>                                         |  before |  after
> drivers/comedi/drivers/ni_atmio.o      |  323944 |  20832
> drivers/comedi/drivers/ni_mio_cs.o     |  318376 |  15000
> drivers/comedi/drivers/ni_mio_common.o |       - | 325048
> drivers/comedi/drivers/ni_pcimio.o     |  389344 |  48216
> ---------------------------------------+---------+-------
>   sum                                   | 1031664 | 409096
> 
> So this results in a considerable decrease in binary size of more than
> 600 KiB.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello,
> 
> changes compared to (implicit) v1 can be seen below. I now make use of a
> module name space COMEDI_NI as suggested by Greg and moved the
> declaration for range_ni_E_ao_ext to a more sensible place. The numbers
> got a tad worse because of the module name space, but the order is still
> the same.
> 
> Best regards
> Uwe
> 
> Range-diff against v1:
> 1:  79b9c40b8aa1 ! 1:  37d109ed616a comedi: Make ni_mio_common a standalone module
>      @@ Commit message
>           For an amd64 allmodconfig this changed the size for the following object
>           files:
>                                                  |  before |  after
>      -    drivers/comedi/drivers/ni_atmio.o      |  323944 |  20752
>      -    drivers/comedi/drivers/ni_mio_cs.o     |  318376 |  14920
>      -    drivers/comedi/drivers/ni_mio_common.o |       - | 324872
>      -    drivers/comedi/drivers/ni_pcimio.o     |  389344 |  48168
>      +    drivers/comedi/drivers/ni_atmio.o      |  323944 |  20832
>      +    drivers/comedi/drivers/ni_mio_cs.o     |  318376 |  15000
>      +    drivers/comedi/drivers/ni_mio_common.o |       - | 325048
>      +    drivers/comedi/drivers/ni_pcimio.o     |  389344 |  48216
>           ---------------------------------------+---------+-------
>      -     sum                                   | 1031664 | 408712
>      +     sum                                   | 1031664 | 409096
>       
>           So this results in a considerable decrease in binary size of more than
>           600 KiB.
>      @@ drivers/comedi/drivers/ni_mio.h (new)
>       +extern int ni_E_init(struct comedi_device *dev,
>       +		     unsigned int interrupt_pin, unsigned int irq_polarity);
>       +extern void mio_common_detach(struct comedi_device *dev);
>      ++
>      ++extern const struct comedi_lrange range_ni_E_ao_ext;
>      ++
>      ++MODULE_IMPORT_NS(COMEDI_NI);
>       
>        ## drivers/comedi/drivers/ni_mio_common.c ##
>       @@
>      +  * fully tested as yet. Terry Barnaby, BEAM Ltd.
>         */
>        
>      ++#define DEFAULT_SYMBOL_NAMESPACE COMEDI_NI
>      ++
>        #include <linux/interrupt.h>
>       +#include <linux/module.h>
>        #include <linux/sched.h>
>      @@ drivers/comedi/drivers/ni_stc.h: struct ni_private {
>        };
>        
>       -static const struct comedi_lrange range_ni_E_ao_ext;
>      -+extern const struct comedi_lrange range_ni_E_ao_ext;
>      -
>      +-
>        #endif /* _COMEDI_NI_STC_H */
> 
>   drivers/comedi/Kconfig                 |  6 +++
>   drivers/comedi/drivers/Makefile        |  1 +
>   drivers/comedi/drivers/ni_atmio.c      |  6 +--
>   drivers/comedi/drivers/ni_mio.h        | 37 +++++++++++++++
>   drivers/comedi/drivers/ni_mio_common.c | 65 +++++++++++++-------------
>   drivers/comedi/drivers/ni_mio_cs.c     |  6 +--
>   drivers/comedi/drivers/ni_pcimio.c     |  3 +-
>   drivers/comedi/drivers/ni_stc.h        |  2 -
>   8 files changed, 81 insertions(+), 45 deletions(-)
>   create mode 100644 drivers/comedi/drivers/ni_mio.h
> 
> diff --git a/drivers/comedi/Kconfig b/drivers/comedi/Kconfig
> index 3cb61fa2c5c3..8137b94001ea 100644
> --- a/drivers/comedi/Kconfig
> +++ b/drivers/comedi/Kconfig
> @@ -473,9 +473,13 @@ config COMEDI_NI_AT_AO
>   	  To compile this driver as a module, choose M here: the module will be
>   	  called ni_at_ao.
>   
> +config COMEDI_NI_MIO
> +	tristate
> +
>   config COMEDI_NI_ATMIO
>   	tristate "NI AT-MIO E series ISA-PNP card support"
>   	select COMEDI_8255
> +	select COMEDI_NI_MIO
>   	select COMEDI_NI_TIO
>   	help
>   	  Enable support for National Instruments AT-MIO E series cards
> @@ -1053,6 +1057,7 @@ config COMEDI_NI_PCIDIO
>   config COMEDI_NI_PCIMIO
>   	tristate "NI PCI-MIO-E series and M series support"
>   	depends on HAS_DMA
> +	select COMEDI_NI_MIO
>   	select COMEDI_NI_TIOCMD
>   	select COMEDI_8255
>   	help
> @@ -1160,6 +1165,7 @@ config COMEDI_NI_LABPC_CS
>   
>   config COMEDI_NI_MIO_CS
>   	tristate "NI DAQCard E series PCMCIA support"
> +	select COMEDI_NI_MIO
>   	select COMEDI_NI_TIO
>   	select COMEDI_8255
>   	help
> diff --git a/drivers/comedi/drivers/Makefile b/drivers/comedi/drivers/Makefile
> index b24ac00cab73..725ad62f4c99 100644
> --- a/drivers/comedi/drivers/Makefile
> +++ b/drivers/comedi/drivers/Makefile
> @@ -49,6 +49,7 @@ obj-$(CONFIG_COMEDI_MPC624)		+= mpc624.o
>   obj-$(CONFIG_COMEDI_ADQ12B)		+= adq12b.o
>   obj-$(CONFIG_COMEDI_NI_AT_A2150)	+= ni_at_a2150.o
>   obj-$(CONFIG_COMEDI_NI_AT_AO)		+= ni_at_ao.o
> +obj-$(CONFIG_COMEDI_NI_MIO)		+= ni_mio_common.o
>   obj-$(CONFIG_COMEDI_NI_ATMIO)		+= ni_atmio.o
>   obj-$(CONFIG_COMEDI_NI_ATMIO16D)	+= ni_atmio16d.o
>   obj-$(CONFIG_COMEDI_NI_LABPC_ISA)	+= ni_labpc.o
> diff --git a/drivers/comedi/drivers/ni_atmio.c b/drivers/comedi/drivers/ni_atmio.c
> index 56c78da475e7..d2c211788109 100644
> --- a/drivers/comedi/drivers/ni_atmio.c
> +++ b/drivers/comedi/drivers/ni_atmio.c
> @@ -44,8 +44,7 @@
>    */
>   
>   /*
> - * The real guts of the driver is in ni_mio_common.c, which is included
> - * both here and in ni_pcimio.c
> + * The real guts of the driver is in ni_mio_common.c.
>    *
>    * Interrupt support added by Truxton Fulton <trux@truxton.com>
>    *
> @@ -78,6 +77,7 @@
>   #include <linux/isapnp.h>
>   
>   #include "ni_stc.h"
> +#include "ni_mio.h"
>   #include "8255.h"
>   
>   /* AT specific setup */
> @@ -205,8 +205,6 @@ static const int ni_irqpin[] = {
>   	-1, -1, -1, 0, 1, 2, -1, 3, -1, -1, 4, 5, 6, -1, -1, 7
>   };
>   
> -#include "ni_mio_common.c"
> -
>   static const struct pnp_device_id device_ids[] = {
>   	{.id = "NIC1900", .driver_data = 0},
>   	{.id = "NIC2400", .driver_data = 0},
> diff --git a/drivers/comedi/drivers/ni_mio.h b/drivers/comedi/drivers/ni_mio.h
> new file mode 100644
> index 000000000000..c9f20ade9dcc
> --- /dev/null
> +++ b/drivers/comedi/drivers/ni_mio.h
> @@ -0,0 +1,37 @@
> +#include <linux/interrupt.h>
> +
> +#define NI_GPCT_SUBDEV(x)	(NI_GPCT0_SUBDEV + (x))
> +
> +enum ni_common_subdevices {
> +	NI_AI_SUBDEV,
> +	NI_AO_SUBDEV,
> +	NI_DIO_SUBDEV,
> +	NI_8255_DIO_SUBDEV,
> +	NI_UNUSED_SUBDEV,
> +	NI_CALIBRATION_SUBDEV,
> +	NI_EEPROM_SUBDEV,
> +	NI_PFI_DIO_SUBDEV,
> +	NI_CS5529_CALIBRATION_SUBDEV,
> +	NI_SERIAL_SUBDEV,
> +	NI_RTSI_SUBDEV,
> +	NI_GPCT0_SUBDEV,
> +	NI_GPCT1_SUBDEV,
> +	NI_FREQ_OUT_SUBDEV,
> +	NI_NUM_SUBDEVICES
> +};
> +
> +extern void ni_writel(struct comedi_device *dev, unsigned int data, int reg);
> +extern void ni_writew(struct comedi_device *dev, unsigned int data, int reg);
> +extern void ni_writeb(struct comedi_device *dev, unsigned int data, int reg);
> +extern unsigned int ni_readb(struct comedi_device *dev, int reg);
> +extern void ni_stc_writew(struct comedi_device *dev, unsigned int data, int reg);
> +extern int ni_read_eeprom(struct comedi_device *dev, int addr);
> +extern irqreturn_t ni_E_interrupt(int irq, void *d);
> +extern int ni_alloc_private(struct comedi_device *dev);
> +extern int ni_E_init(struct comedi_device *dev,
> +		     unsigned int interrupt_pin, unsigned int irq_polarity);
> +extern void mio_common_detach(struct comedi_device *dev);
> +
> +extern const struct comedi_lrange range_ni_E_ao_ext;
> +
> +MODULE_IMPORT_NS(COMEDI_NI);
> diff --git a/drivers/comedi/drivers/ni_mio_common.c b/drivers/comedi/drivers/ni_mio_common.c
> index 4f80a4991f95..af480d03296c 100644
> --- a/drivers/comedi/drivers/ni_mio_common.c
> +++ b/drivers/comedi/drivers/ni_mio_common.c
> @@ -40,12 +40,19 @@
>    * fully tested as yet. Terry Barnaby, BEAM Ltd.
>    */
>   
> +#define DEFAULT_SYMBOL_NAMESPACE COMEDI_NI
> +
>   #include <linux/interrupt.h>
> +#include <linux/module.h>
>   #include <linux/sched.h>
>   #include <linux/delay.h>
>   #include "8255.h"
>   #include "mite.h"
>   
> +#include "ni_stc.h"
> +#include "ni_mio.h"
> +#include "../comedidev.h"
> +
>   /* A timeout count */
>   #define NI_TIMEOUT 1000
>   
> @@ -160,7 +167,7 @@ static const struct comedi_lrange range_ni_M_ai_628x = {
>   	}
>   };
>   
> -static const struct comedi_lrange range_ni_E_ao_ext = {
> +const struct comedi_lrange range_ni_E_ao_ext = {
>   	4, {
>   		BIP_RANGE(10),
>   		UNI_RANGE(10),
> @@ -168,6 +175,7 @@ static const struct comedi_lrange range_ni_E_ao_ext = {
>   		RANGE_ext(0, 1)
>   	}
>   };
> +EXPORT_SYMBOL_GPL(range_ni_E_ao_ext);
>   
>   static const struct comedi_lrange *const ni_range_lkup[] = {
>   	[ai_gain_16] = &range_ni_E_ai,
> @@ -187,26 +195,6 @@ enum aimodes {
>   	AIMODE_SAMPLE = 3,
>   };
>   
> -enum ni_common_subdevices {
> -	NI_AI_SUBDEV,
> -	NI_AO_SUBDEV,
> -	NI_DIO_SUBDEV,
> -	NI_8255_DIO_SUBDEV,
> -	NI_UNUSED_SUBDEV,
> -	NI_CALIBRATION_SUBDEV,
> -	NI_EEPROM_SUBDEV,
> -	NI_PFI_DIO_SUBDEV,
> -	NI_CS5529_CALIBRATION_SUBDEV,
> -	NI_SERIAL_SUBDEV,
> -	NI_RTSI_SUBDEV,
> -	NI_GPCT0_SUBDEV,
> -	NI_GPCT1_SUBDEV,
> -	NI_FREQ_OUT_SUBDEV,
> -	NI_NUM_SUBDEVICES
> -};
> -
> -#define NI_GPCT_SUBDEV(x)	(NI_GPCT0_SUBDEV + (x))
> -
>   enum timebase_nanoseconds {
>   	TIMEBASE_1_NS = 50,
>   	TIMEBASE_2_NS = 10000
> @@ -219,29 +207,32 @@ enum timebase_nanoseconds {
>   
>   static const int num_adc_stages_611x = 3;
>   
> -static void ni_writel(struct comedi_device *dev, unsigned int data, int reg)
> +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);
>   }
> +EXPORT_SYMBOL_GPL(ni_writel);
>   
> -static void ni_writew(struct comedi_device *dev, unsigned int data, int reg)
> +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);
>   }
> +EXPORT_SYMBOL_GPL(ni_writew);
>   
> -static void ni_writeb(struct comedi_device *dev, unsigned int data, int reg)
> +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);
>   }
> +EXPORT_SYMBOL_GPL(ni_writeb);
>   
>   static unsigned int ni_readl(struct comedi_device *dev, int reg)
>   {
> @@ -259,13 +250,14 @@ static unsigned int ni_readw(struct comedi_device *dev, int reg)
>   	return inw(dev->iobase + reg);
>   }
>   
> -static unsigned int ni_readb(struct comedi_device *dev, int reg)
> +unsigned int ni_readb(struct comedi_device *dev, int reg)
>   {
>   	if (dev->mmio)
>   		return readb(dev->mmio + reg);
>   
>   	return inb(dev->iobase + reg);
>   }
> +EXPORT_SYMBOL_GPL(ni_readb);
>   
>   /*
>    * We automatically take advantage of STC registers that can be
> @@ -435,8 +427,7 @@ static unsigned int m_series_stc_read(struct comedi_device *dev,
>   	}
>   }
>   
> -static void ni_stc_writew(struct comedi_device *dev,
> -			  unsigned int data, int reg)
> +void ni_stc_writew(struct comedi_device *dev, unsigned int data, int reg)
>   {
>   	struct ni_private *devpriv = dev->private;
>   	unsigned long flags;
> @@ -454,6 +445,7 @@ static void ni_stc_writew(struct comedi_device *dev,
>   		spin_unlock_irqrestore(&devpriv->window_lock, flags);
>   	}
>   }
> +EXPORT_SYMBOL_GPL(ni_stc_writew);
>   
>   static void ni_stc_writel(struct comedi_device *dev,
>   			  unsigned int data, int reg)
> @@ -4482,7 +4474,7 @@ static void caldac_setup(struct comedi_device *dev, struct comedi_subdevice *s)
>   	}
>   }
>   
> -static int ni_read_eeprom(struct comedi_device *dev, int addr)
> +int ni_read_eeprom(struct comedi_device *dev, int addr)
>   {
>   	unsigned int cmd = NI_E_SERIAL_CMD_EEPROM_CS;
>   	int bit;
> @@ -4511,6 +4503,7 @@ static int ni_read_eeprom(struct comedi_device *dev, int addr)
>   
>   	return bitstring;
>   }
> +EXPORT_SYMBOL_GPL(ni_read_eeprom);
>   
>   static int ni_eeprom_insn_read(struct comedi_device *dev,
>   			       struct comedi_subdevice *s,
> @@ -5885,7 +5878,7 @@ static int ni_gpct_cancel(struct comedi_device *dev, struct comedi_subdevice *s)
>   }
>   #endif
>   
> -static irqreturn_t ni_E_interrupt(int irq, void *d)
> +irqreturn_t ni_E_interrupt(int irq, void *d)
>   {
>   	struct comedi_device *dev = d;
>   	struct comedi_subdevice *s_ai = dev->read_subdev;
> @@ -5941,8 +5934,9 @@ static irqreturn_t ni_E_interrupt(int irq, void *d)
>   	spin_unlock_irqrestore(&dev->spinlock, flags);
>   	return IRQ_HANDLED;
>   }
> +EXPORT_SYMBOL_GPL(ni_E_interrupt);
>   
> -static int ni_alloc_private(struct comedi_device *dev)
> +int ni_alloc_private(struct comedi_device *dev)
>   {
>   	struct ni_private *devpriv;
>   
> @@ -5956,6 +5950,7 @@ static int ni_alloc_private(struct comedi_device *dev)
>   
>   	return 0;
>   }
> +EXPORT_SYMBOL_GPL(ni_alloc_private);
>   
>   static unsigned int _ni_get_valid_routes(struct comedi_device *dev,
>   					 unsigned int n_pairs,
> @@ -5967,8 +5962,8 @@ static unsigned int _ni_get_valid_routes(struct comedi_device *dev,
>   				   pair_data);
>   }
>   
> -static int ni_E_init(struct comedi_device *dev,
> -		     unsigned int interrupt_pin, unsigned int irq_polarity)
> +int ni_E_init(struct comedi_device *dev,
> +	      unsigned int interrupt_pin, unsigned int irq_polarity)
>   {
>   	const struct ni_board_struct *board = dev->board_ptr;
>   	struct ni_private *devpriv = dev->private;
> @@ -6331,11 +6326,15 @@ static int ni_E_init(struct comedi_device *dev,
>   
>   	return 0;
>   }
> +EXPORT_SYMBOL_GPL(ni_E_init);
>   
> -static void mio_common_detach(struct comedi_device *dev)
> +void mio_common_detach(struct comedi_device *dev)
>   {
>   	struct ni_private *devpriv = dev->private;
>   
>   	if (devpriv)
>   		ni_gpct_device_destroy(devpriv->counter_dev);
>   }
> +EXPORT_SYMBOL_GPL(mio_common_detach);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/comedi/drivers/ni_mio_cs.c b/drivers/comedi/drivers/ni_mio_cs.c
> index 4f37b4e58f09..9f11efde28f9 100644
> --- a/drivers/comedi/drivers/ni_mio_cs.c
> +++ b/drivers/comedi/drivers/ni_mio_cs.c
> @@ -19,8 +19,7 @@
>    */
>   
>   /*
> - * The real guts of the driver is in ni_mio_common.c, which is
> - * included by all the E series drivers.
> + * The real guts of the driver is in ni_mio_common.c
>    *
>    * References for specifications:
>    *	341080a.pdf  DAQCard E Series Register Level Programmer Manual
> @@ -31,6 +30,7 @@
>   
>   #include "../comedi_pcmcia.h"
>   #include "ni_stc.h"
> +#include "ni_mio.h"
>   #include "8255.h"
>   
>   /*
> @@ -112,8 +112,6 @@ static const struct ni_board_struct ni_boards[] = {
>   #endif
>   };
>   
> -#include "ni_mio_common.c"
> -
>   static const void *ni_getboardtype(struct comedi_device *dev,
>   				   struct pcmcia_device *link)
>   {
> diff --git a/drivers/comedi/drivers/ni_pcimio.c b/drivers/comedi/drivers/ni_pcimio.c
> index 6c813a490ba5..f74114e883be 100644
> --- a/drivers/comedi/drivers/ni_pcimio.c
> +++ b/drivers/comedi/drivers/ni_pcimio.c
> @@ -100,6 +100,7 @@
>   #include <asm/byteorder.h>
>   
>   #include "ni_stc.h"
> +#include "ni_mio.h"
>   #include "mite.h"
>   
>   #define PCIDMA
> @@ -1142,8 +1143,6 @@ static const struct ni_board_struct ni_boards[] = {
>   	},
>   };
>   
> -#include "ni_mio_common.c"
> -
>   static int pcimio_ai_change(struct comedi_device *dev,
>   			    struct comedi_subdevice *s)
>   {
> diff --git a/drivers/comedi/drivers/ni_stc.h b/drivers/comedi/drivers/ni_stc.h
> index fbc0b753a0f5..7837e4683c6d 100644
> --- a/drivers/comedi/drivers/ni_stc.h
> +++ b/drivers/comedi/drivers/ni_stc.h
> @@ -1137,6 +1137,4 @@ struct ni_private {
>   	u8 rgout0_usage;
>   };
>   
> -static const struct comedi_lrange range_ni_E_ao_ext;
> -
>   #endif /* _COMEDI_NI_STC_H */
> 
> base-commit: 6efb943b8616ec53a5e444193dccf1af9ad627b5
> 

NAK.

It's not as simple as that.  "ni_mio_common.c" contains condional code 
that depends on whether the PCIDMA macro is defined or not before it is 
included.  "ni_pcimio.c" defines the PCIDMA macro but "ni_atmio.c" and 
"ni_mio_cs.c" do not.

The "ni_pcimio" module depends on the "mite" and "ni_tiocmd" modules and 
most of the code that depends on those is in the PCIDMA parts of 
"ni_mio_common.c".

The "ni_atmio" and "ni_mio_cs" modules do not depend on the "mite" and 
"ni_tiocmd" modules, and should not compile any of the PCMDMA parts of 
"ni_mio_common.c" that depends on it.

So you see, there are really two versions of the "ni_mio_common.c" code 
currently, but this "ni_mio_common" module you have added only has one 
version of the code, and that version of the code is broken for "ni_pcimio".

-- 
-=( Ian Abbott <abbotti@mev.co.uk> || 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 )=-