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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
