Date:	Thu, 31 Jan 2013 16:43:10 +0000
From:	Ian Abbott <abbotti@....co.uk>
To:	H Hartley Sweeten <hartleys@...ionengravers.com>
CC:	Ian Abbott <ian.abbott@....co.uk>,
	Peter Hüwe <PeterHuewe@....de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Dan Carpenter <dan.carpenter@...cle.com>,
	"devel@...uxdriverproject.org" <devel@...uxdriverproject.org>
Subject: Re: [Q]staging/comedi: Considation of *_find_boardinfo possible?

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@....co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists