[Q]staging/comedi: Considation of *_find_boardinfo possible?

[Q]staging/comedi: Considation of *_find_boardinfo possible?

[Peter Hüwe]

Hi,

while analyzing the comedi drivers, I noticed that quite a lot of them use a 
more or less similar find_boardinfo function.
e.g.: 
cb_pcidas64.c 

static const struct pcidas64_board
*cb_pcidas64_find_pci_board(struct pci_dev *pcidev)
{
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(pcidas64_boards); i++)
		if (pcidev->device == pcidas64_boards[i].device_id)
			return &pcidas64_boards[i];
	return NULL;
}

and ni_6527.c:
static const struct ni6527_board *
ni6527_find_boardinfo(struct pci_dev *pcidev)
{
	unsigned int dev_id = pcidev->device;
	unsigned int n;

	for (n = 0; n < ARRAY_SIZE(ni6527_boards); n++) {
		const struct ni6527_board *board = &ni6527_boards[n];
		if (board->dev_id == dev_id)
			return board;
	}
	return NULL;
}




The names and the exact implementation differ slightly, but in most cases it 
boils down to:
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(__BOARD_ARRAY__); i++)
		if (pcidev->device == __BOARD_ARRAY__[i].device_id)
			return &__BOARD_ARRAY__[i];
	return NULL;

unfortunately the __BOARD_ARRAY__ is always of a different type (but all 
contain the device_id field) and size.


---> is there a way to consolidate these functions into one function (which 
can operate on the different types) ?  It's almost a bit like 'templates'.
Maybe with some gcc extensions or kernel magic functions ?

I already thought about passing a void pointer to the __BOARD_ARRAY__ and the 
size of one element of the __BOARD_ARRAY__ and doing pointer calculations - 
but I guess there must be a better way.

Or is the only option to write a macro ?


Looking forward to your replies.

Thanks,
Peter

RE: [Q]staging/comedi: Considation of *_find_boardinfo possible?

[H Hartley Sweeten]

On Tuesday, January 29, 2013 4:42 PM, Peter Hüwe wrote:
> Hi,
>
> while analyzing the comedi drivers, I noticed that quite a lot of them use a 
> more or less similar find_boardinfo function.

<snip>

> The names and the exact implementation differ slightly, but in most cases it 
> boils down to:
>	unsigned int i;
>
>	for (i = 0; i < ARRAY_SIZE(__BOARD_ARRAY__); i++)
>		if (pcidev->device == __BOARD_ARRAY__[i].device_id)
>			return &__BOARD_ARRAY__[i];
>	return NULL;
>
> unfortunately the __BOARD_ARRAY__ is always of a different type (but all 
> contain the device_id field) and size.
>
>
> ---> is there a way to consolidate these functions into one function (which 
> can operate on the different types) ?  It's almost a bit like 'templates'.
> Maybe with some gcc extensions or kernel magic functions ?
>
> I already thought about passing a void pointer to the __BOARD_ARRAY__ and the 
> size of one element of the __BOARD_ARRAY__ and doing pointer calculations - 
> but I guess there must be a better way.
>
> Or is the only option to write a macro ?

As you noticed, the problem is the driver specific definition of the struct used
to hold the boardinfo.

In drivers.c, the comedi_recognize() function actually access the boardinfo
in order to support the COMEDI_DEVCONFIG ioctl. There is a comment above
it giving a description of how it works.

There might be a way to do this in a generic way. The problem is that the
drivers use different names for "common" information and the data is
packed in the structs differently so accessing it generically is a bit difficult,
if not impossible.

I have been trying to remove as much of this boardinfo stuff from the drivers
as possible. For now I think the current implementation is fairly clean.

For the PnP bus drivers that use the auto_attach mechanism I have some ideas
to get rid of the find_boardinfo functions but I need to work out the kinks first.

Please wait on "fixing" any of this until a good solution is worked out.

Thanks,
Hartley

Re: [Q]staging/comedi: Considation of *_find_boardinfo possible?

[Joe Perches]

On Wed, 2013-01-30 at 00:41 +0100, Peter Hüwe wrote:
> ---> is there a way to consolidate these functions into one function (which 
> can operate on the different types) ?  It's almost a bit like 'templates'.
> Maybe with some gcc extensions or kernel magic functions ?

Nothing wrong with a macro.

Maybe something like:

#define comedi_find_board(array, board_id)		\
({							\
	int i;						\
	typeof array *p = array;			\
	typeof array *rtn = NULL;			\
	for (i = 0; i < ARRAY_SIZE(array); i++, p++) {	\
		if (p->device_id == board_id) {		\
			rtn = p;			\
			break;				\
		}					\
	}						\
	rtn;						\
})

Re: [Q]staging/comedi: Considation of *_find_boardinfo possible?

[Ian Abbott]

On 2013-01-29 23:56, H Hartley Sweeten wrote:
> On Tuesday, January 29, 2013 4:42 PM, Peter Hüwe wrote:
>> Hi,
>>
>> while analyzing the comedi drivers, I noticed that quite a lot of them use a
>> more or less similar find_boardinfo function.
>
> <snip>
>
>> The names and the exact implementation differ slightly, but in most cases it
>> boils down to:
>> 	unsigned int i;
>>
>> 	for (i = 0; i < ARRAY_SIZE(__BOARD_ARRAY__); i++)
>> 		if (pcidev->device == __BOARD_ARRAY__[i].device_id)
>> 			return &__BOARD_ARRAY__[i];
>> 	return NULL;
>>
>> unfortunately the __BOARD_ARRAY__ is always of a different type (but all
>> contain the device_id field) and size.
>>
>>
>> ---> is there a way to consolidate these functions into one function (which
>> can operate on the different types) ?  It's almost a bit like 'templates'.
>> Maybe with some gcc extensions or kernel magic functions ?
>>
>> I already thought about passing a void pointer to the __BOARD_ARRAY__ and the
>> size of one element of the __BOARD_ARRAY__ and doing pointer calculations -
>> but I guess there must be a better way.
>>
>> Or is the only option to write a macro ?
>
> As you noticed, the problem is the driver specific definition of the struct used
> to hold the boardinfo.
>
> In drivers.c, the comedi_recognize() function actually access the boardinfo
> in order to support the COMEDI_DEVCONFIG ioctl. There is a comment above
> it giving a description of how it works.
>
> There might be a way to do this in a generic way. The problem is that the
> drivers use different names for "common" information and the data is
> packed in the structs differently so accessing it generically is a bit difficult,
> if not impossible.
>
> I have been trying to remove as much of this boardinfo stuff from the drivers
> as possible. For now I think the current implementation is fairly clean.
>
> For the PnP bus drivers that use the auto_attach mechanism I have some ideas
> to get rid of the find_boardinfo functions but I need to work out the kinks first.
>
> Please wait on "fixing" any of this until a good solution is worked out.

One way is to enumerate the possible indices of the custom board array 
as a set of enum constants, initialize the custom board array using 
designated element initializers (indexed by the enum constants) and 
include the same enum constant in the 'driver_data' member of the struct 
pci_device_id elements of the module's PCI device table.  Then the 
module's PCI driver's probe function can index directly into the custom 
board array without searching.

The missing link in the above is passing the index from the 
'driver_data' through to the modules' comedi_driver's 'auto_attach' 
function. The 'comedi_pci_auto_config()' function does not currently 
have a parameter for passing this information, but the underlying 
'comedi_auto_config()' function does.  Either the existing 
'comedi_pci_auto_config()' function could be extended, or a separate 
extended version of the function could be added (maybe as an inline 
function in comedidev.h), or the modules could call 
'comedi_auto_config()' directly.

We have posted patches to add extra context to 
'comedi_pci_auto_config()' before, but they weren't applied because we 
didn't have a clear use case for them.  Now we have, but I wouldn't mind 
leaving the existing function alone and adding a new one.

The nice thing is that it's all under the control of the individual drivers.

Here's some code to illustrate what I'm on about in the above description:

struct foobar_board {
	const char *name;
	unsigned int ai_chans;
	unsigned int ai_bits;
};

enum foobar_board_nums {
	bn_foo,
	bn_bar,
	bn_baz
};

static const struct foobar_board foobar_boards[] = {
	[bn_foo] = {
		.name = "foo",
		.ai_chans = 4,
		.ai_bits = 12,
	},
	[bn_bar] = {
		.name = "bar",
		.ai_chans = 4,
		.ai_bits = 16,
	},
	[bn_baz] = {
		.name = "baz",
		.ai_chans = 8,
		.ai_bits = 16,
	},
};

static int foobar_auto_attach(struct comedi_device *dev,
			      unsigned long context_bn)
{
	struct pci_dev *pcidev = comedi_to_pci_dev(dev);
	struct foobar_board *thisboard = &foobar_boards[bn_foo];

	dev->board_ptr = thisboard;	/* no searching! */

	/* just return error for now */
	return -ENODEV;
}

static void foobar_detach(struct comedi_device *dev)
{
	/* nothing to do for now */
}

static struct comedi_driver foobar_comedi_driver = {
	.driver_name	= "foobar",
	.module		= THIS_MODULE,
	.auto_attach	= foobar_auto_attach,
	.detach		= foobar_detach,
};

static DEFINE_PCI_DEVICE_TABLE(foobar_pci_table) = {
	{
		PCI_DEVICE(PCI_VENDOR_ID_FOOBAR,
			   PCI_DEVICE_ID_FOOBAR_FOO),
		.driver_data = bn_foo,
	},
	{
		PCI_DEVICE(PCI_VENDOR_ID_FOOBAR,
			   PCI_DEVICE_ID_FOOBAR_BAR),
		.driver_data = bn_bar,
	},
	{
		PCI_DEVICE(PCI_VENDOR_ID_FOOBAR,
			   PCI_DEVICE_ID_FOOBAR_BAZ),
		.driver_data = bn_baz,
	},
	{ 0 }
};
MODULE_DEVICE_TABLE(pci, foobar_pci_table);

static int foobar_pci_probe(struct pci_dev *pdev,
			    const struct pci_device_id *ent)
{
	/* could use a variant of comedi_pci_auto_config() here */
	return comedi_auto_config(&pdev->dev, &foobar_comedi_driver,
				  ent->driver_data);
}

static struct pci_driver foobar_pci_driver = {
	.name		= "foobar",
	.id_table	= foobar_pci_table,
	.probe		= foobar_pci_probe,
	.remove		= comedi_pci_auto_unconfig,
};
module_comedi_pci_driver(foobar_comedi_driver, foobar_pci_driver);

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

Re: [Q]staging/comedi: Considation of *_find_boardinfo possible?

[Ian Abbott]

On 2013-01-30 11:04, Ian Abbott wrote:
> static int foobar_auto_attach(struct comedi_device *dev,
> 			      unsigned long context_bn)
> {
> 	struct pci_dev *pcidev = comedi_to_pci_dev(dev);
> 	struct foobar_board *thisboard = &foobar_boards[bn_foo];

Oops, I meant &foobar_boards[context_bn] there!

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

RE: [Q]staging/comedi: Considation of *_find_boardinfo possible?

[H Hartley Sweeten]

On Wednesday, January 30, 2013 4:04 AM, Ian Abbott wrote:
> One way is to enumerate the possible indices of the custom board array 
> as a set of enum constants, initialize the custom board array using 
> designated element initializers (indexed by the enum constants) and 
> include the same enum constant in the 'driver_data' member of the struct 
> pci_device_id elements of the module's PCI device table.  Then the 
> module's PCI driver's probe function can index directly into the custom 
> board array without searching.

Ian,

The method you describe is what I was also considering. The only
problem is it will introduce a lot of churn in the drivers.

I'm hoping to eliminate all the unnecessary boardinfo's from the
drivers before going down this path.

> The missing link in the above is passing the index from the 
> 'driver_data' through to the modules' comedi_driver's 'auto_attach' 
> function. The 'comedi_pci_auto_config()' function does not currently 
> have a parameter for passing this information, but the underlying 
> 'comedi_auto_config()' function does.  Either the existing 
> 'comedi_pci_auto_config()' function could be extended, or a separate 
> extended version of the function could be added (maybe as an inline 
> function in comedidev.h), or the modules could call 
> 'comedi_auto_config()' directly.
>
> We have posted patches to add extra context to 
> 'comedi_pci_auto_config()' before, but they weren't applied because we 
> didn't have a clear use case for them.  Now we have, but I wouldn't mind 
> leaving the existing function alone and adding a new one.

Yah, that was the intention of my patches. They just weren't clear. Also,
my patches changed the type on the 'context' which appears to not be
needed.

> The nice thing is that it's all under the control of the individual drivers.
>
> Here's some code to illustrate what I'm on about in the above description:
>
> struct foobar_board {
>	const char *name;
>	unsigned int ai_chans;
>	unsigned int ai_bits;
> };

I would also like to make a common "boardinfo" struct that the comedi
core can then use in the comedi_recognize() and comedi_report_boards()
functions to remove the need for the pointer math. Something like:

struct comedi_board {
	const char *name;
	const void *private;
};

The comedi_driver then could be changed to:

+	const struct comedi_board *boards;
-	/* number of elements in board_name and board_id arrays */
-	unsigned int num_names;
-	const char *const *board_name;
-	/* offset in bytes from one board name pointer to the next */
-	int offset;
};

The board_ptr in comedi_device would then change to:

+	const struct comedi_board *board_ptr;
-	const void *board_ptr;

The comedi_board() helper would also need changed:

static inline const void *comedi_board(const struct comedi_device *dev)
{
+	return (dev->board_ptr) ? dev->board_ptr->private : NULL;
-	return dev->board_ptr;
}

It still returns the driver specific boardinfo as a const void *.

The common comedi_board would also allow removing the board_name
from the comedi_device. A helper function could just fetch it:

static const char *comedi_board_name(struct comedi_device *dev)
{
	return (dev->board_ptr) ? dev->board_ptr->name : dev->driver->driver_name;
}

> enum foobar_board_nums {
>	bn_foo,
>	bn_bar,
>	bn_baz
> };
>
> static const struct foobar_board foobar_boards[] = {
>	[bn_foo] = {
>		.name = "foo",
>		.ai_chans = 4,
>		.ai_bits = 12,
>	},
>	[bn_bar] = {
>		.name = "bar",
>		.ai_chans = 4,
>		.ai_bits = 16,
>	},
>	[bn_baz] = {
>		.name = "baz",
>		.ai_chans = 8,
>		.ai_bits = 16,
>	},
> };

Using the common comedi_board would change this a bit:

static const struct foobar_board[] = {
	[bn_foo] = {
		.ai_chans = 4,
		.ai_bits = 12,
	},
	[bn_bar] = {
		.ai_chans = 4,
		.ai_bits = 16,
	},
	[bn_baz] = {
		.ai_chans = 8,
		.ai_bits = 16,
	},
};

static const struct comedi_board foobar_boards[] = {
	[bn_foo] = {
		.name = "foo",
		.private = &foorbar_info[bn_foo],
	},
	[bn_bar] = {
		.name = "bar",
		.private = &foorbar_info[bn_bar],
	},
	[bn_baz] = {
		.name = "baz",
		.private = &foorbar_info[bn_baz],
	},
};

Any other "common" information that the comedi core needs to
access could be added to comedi_board. All the driver specific
information stays in the private struct.

> static int foobar_auto_attach(struct comedi_device *dev,
> 			      unsigned long context_bn)
> {
>	struct pci_dev *pcidev = comedi_to_pci_dev(dev);
>	struct foobar_board *thisboard = &foobar_boards[bn_foo];
>
>	dev->board_ptr = thisboard;	/* no searching! */

The dev->board_ptr should just be set in the comedi core before
calling the drivers auto_attach. Something like:

		comedi_set_hw_dev(comedi_dev, hardware_device);
		comedi_dev->driver = driver;
+		if (driver->boards)
+			comedi_dev->board_ptr = &driver->boards[context];
		ret = driver->auto_attach(comedi_dev, context);

Actually, if we go this route, the context should not be required in the
auto_attach since the core has already taken care of it.

Basically, once the auto_attach is called the comedi_device already has
the following fields initialized correctly:

driver		points to the comedi_driver
hw_dev	points to the underlying device (pci/usb/pcmcia/etc.)
board_name	no longer needed
board_ptr	points to the correct comedi_board 'context'

Other than that, the rest of your code follows what I'm thinking.

Opinion?

Regards,
Hartley

Re: [Q]staging/comedi: Considation of *_find_boardinfo possible?

[Ian Abbott]

On 2013-01-30 17:54, H Hartley Sweeten wrote:
> On Wednesday, January 30, 2013 4:04 AM, Ian Abbott wrote:
>> Here's some code to illustrate what I'm on about in the above description:
>>
>> struct foobar_board {
>> 	const char *name;
>> 	unsigned int ai_chans;
>> 	unsigned int ai_bits;
>> };
>
> I would also like to make a common "boardinfo" struct that the comedi
> core can then use in the comedi_recognize() and comedi_report_boards()
> functions to remove the need for the pointer math. Something like:
>
> struct comedi_board {
> 	const char *name;
> 	const void *private;
> };
>
> The comedi_driver then could be changed to:
>
> +	const struct comedi_board *boards;
> -	/* number of elements in board_name and board_id arrays */
> -	unsigned int num_names;
> -	const char *const *board_name;
> -	/* offset in bytes from one board name pointer to the next */
> -	int offset;
> };

I think you'd still need the equivalent of num_names as the comedi core 
would need to know the length of the boards array.

I'm not excited about the idea of adding an extra layer of indirection 
to all the drivers for the sake of making a couple of functions in the 
core a little cleaner.  It was kind of done the way it is for the 
convenience of the driver in the first place.

> The board_ptr in comedi_device would then change to:
>
> +	const struct comedi_board *board_ptr;
> -	const void *board_ptr;
>
> The comedi_board() helper would also need changed:
>
> static inline const void *comedi_board(const struct comedi_device *dev)
> {
> +	return (dev->board_ptr) ? dev->board_ptr->private : NULL;
> -	return dev->board_ptr;
> }

...and probably renamed to avoid the confusion between comedi board and 
private board structures.

> It still returns the driver specific boardinfo as a const void *.
>
> The common comedi_board would also allow removing the board_name
> from the comedi_device. A helper function could just fetch it:
>
> static const char *comedi_board_name(struct comedi_device *dev)
> {
> 	return (dev->board_ptr) ? dev->board_ptr->name : dev->driver->driver_name;
> }
>
>> enum foobar_board_nums {
>> 	bn_foo,
>> 	bn_bar,
>> 	bn_baz
>> };
>>
>> static const struct foobar_board foobar_boards[] = {
>> 	[bn_foo] = {
>> 		.name = "foo",
>> 		.ai_chans = 4,
>> 		.ai_bits = 12,
>> 	},
>> 	[bn_bar] = {
>> 		.name = "bar",
>> 		.ai_chans = 4,
>> 		.ai_bits = 16,
>> 	},
>> 	[bn_baz] = {
>> 		.name = "baz",
>> 		.ai_chans = 8,
>> 		.ai_bits = 16,
>> 	},
>> };
>
> Using the common comedi_board would change this a bit:
>
> static const struct foobar_board[] = {
> 	[bn_foo] = {
> 		.ai_chans = 4,
> 		.ai_bits = 12,
> 	},
> 	[bn_bar] = {
> 		.ai_chans = 4,
> 		.ai_bits = 16,
> 	},
> 	[bn_baz] = {
> 		.ai_chans = 8,
> 		.ai_bits = 16,
> 	},
> };
>
> static const struct comedi_board foobar_boards[] = {
> 	[bn_foo] = {
> 		.name = "foo",
> 		.private = &foorbar_info[bn_foo],
> 	},
> 	[bn_bar] = {
> 		.name = "bar",
> 		.private = &foorbar_info[bn_bar],
> 	},
> 	[bn_baz] = {
> 		.name = "baz",
> 		.private = &foorbar_info[bn_baz],
> 	},
> };
>
> Any other "common" information that the comedi core needs to
> access could be added to comedi_board. All the driver specific
> information stays in the private struct.
>
>> static int foobar_auto_attach(struct comedi_device *dev,
>> 			      unsigned long context_bn)
>> {
>> 	struct pci_dev *pcidev = comedi_to_pci_dev(dev);
>> 	struct foobar_board *thisboard = &foobar_boards[bn_foo];
>>
>> 	dev->board_ptr = thisboard;	/* no searching! */
>
> The dev->board_ptr should just be set in the comedi core before
> calling the drivers auto_attach. Something like:
>
> 		comedi_set_hw_dev(comedi_dev, hardware_device);
> 		comedi_dev->driver = driver;
> +		if (driver->boards)
> +			comedi_dev->board_ptr = &driver->boards[context];
> 		ret = driver->auto_attach(comedi_dev, context);
>
> Actually, if we go this route, the context should not be required in the
> auto_attach since the core has already taken care of it.

The context should be under the control of the driver for its own 
nefarious purposes, not dictated by what the comedi core thinks it 
should be used for.

> Basically, once the auto_attach is called the comedi_device already has
> the following fields initialized correctly:
>
> driver		points to the comedi_driver
> hw_dev	points to the underlying device (pci/usb/pcmcia/etc.)
> board_name	no longer needed
> board_ptr	points to the correct comedi_board 'context'
>
> Other than that, the rest of your code follows what I'm thinking.
>
> Opinion?

Well I'm not keen!

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