[PATCH] staging: comedi: amplc_pc263: split out PCI support

[Ian Abbott]

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 at mev.co.uk>
>> ---
>>   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 at visionengravers.com>

Thanks!

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-