On 2013-04-12 18:33, H Hartley Sweeten wrote:
On Friday, April 12, 2013 9:02 AM, Ian Abbott wrote:
The "amplc_pc263" module is a hybrid driver for Amplicon PC263 (ISA) and
PCI263 (PCI) and uses conditional compilation (and compiler
optimization) to compile in the support for the different bus types.
Split out support for the PCI263 into a new module "amplc_pci263",
retaining support for the PC263 in the existing module "amplc_pc263".
Don't bother supporting the legacy attach mechanism for PCI board in the
new module as that is no longer in vogue for the comedi drivers and the
PCI263 board has no special configuration requirements.
Although the code to handle the single subdevice of each board is the
same for both drivers, this is only a small amount of code and I don't
think it's worth creating a common module to handle it.
Signed-off-by: Ian Abbott <abbotti@xxxxxxxxx>
---
drivers/staging/comedi/Kconfig | 13 +-
drivers/staging/comedi/drivers/Makefile | 3 +-
drivers/staging/comedi/drivers/amplc_pc263.c | 278 ++------------------------
drivers/staging/comedi/drivers/amplc_pci263.c | 127 ++++++++++++
4 files changed, 152 insertions(+), 269 deletions(-)
create mode 100644 drivers/staging/comedi/drivers/amplc_pci263.c
<snip>
diff --git a/drivers/staging/comedi/drivers/amplc_pc263.c b/drivers/staging/comedi/drivers/amplc_pc263.c
index 1b1c1aa..1ffe379 100644
--- a/drivers/staging/comedi/drivers/amplc_pc263.c
+++ b/drivers/staging/comedi/drivers/amplc_pc263.c
<snip>
struct pc263_board {
const char *name;
- unsigned short devid;
- enum pc263_bustype bustype;
- enum pc263_model model;
};
The boardinfo now only contains the 'name'. How about just removing it.
If the legacy matching is typically done against this name instead of the
driver_name, just change the driver_name.
The driver_name is usually the same as the module name. There are very
few comedi drivers where that isn't the case (the only one I spotted
with a quick grep was the "skel.ko" module which uses the comedi driver
name "dummy").
<snip>
static struct comedi_driver amplc_pc263_driver = {
.driver_name = PC263_DRIVER_NAME,
.module = THIS_MODULE,
.attach = pc263_attach,
- .auto_attach = pc263_auto_attach,
.detach = pc263_detach,
.board_name = &pc263_boards[0].name,
.offset = sizeof(struct pc263_board),
.num_names = ARRAY_SIZE(pc263_boards),
};
Then the boardinfo stuff can also be removed from the comedi_driver.
Also, I think PC263_DRIVER_NAME is only being used in the comedi_driver.
How about removing it and just open-coding the string here?
I have no strong opinions on the matter. Even KBUILD_MODNAME could be
used, though I prefer the string literal to appear somewhere in the
source file rather than being automatically generated from the module name.
<snip>
diff --git a/drivers/staging/comedi/drivers/amplc_pci263.c b/drivers/staging/comedi/drivers/amplc_pci263.c
new file mode 100644
index 0000000..8b57533
--- /dev/null
+++ b/drivers/staging/comedi/drivers/amplc_pci263.c
<snip>
+#define PCI263_DRIVER_NAME "amplc_pci263"
Again, is this define really necessary?
Not really, although it is used twice.
+
+/* PCI263 PCI configuration register information */
+#define PCI_DEVICE_ID_AMPLICON_PCI263 0x000c
This one also, it's only used in the id table. Just open code the value there.
Again, I have no strong opinion on the matter.
Other than these comments:
Reviewed-by: H Hartley Sweeten <hsweeten@xxxxxxxxxxxxxxxxxxx>
Thanks!
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
