Re: [PATCH] staging: comedi: remove this_board macro in the s526 driver

[Ian Abbott <abbotti@mev.co.uk>]

On 2012-05-23 17:28, H Hartley Sweeten wrote:
> On Wednesday, May 23, 2012 2:18 AM, Ian Abbott wrote:
>> On 2012-05-23 06:48, Dan Carpenter wrote:
>>> On Tue, May 22, 2012 at 06:20:10PM -0700, H Hartley Sweeten wrote:
>>>> -/*
>>>> - * Initialize dev->board_name.  Note that we can use the "thisboard"
>>>> - * macro now, since we just initialized it in the last line.
>>>> - */
>>>> -	dev->board_ptr =&s526_boards[0];
>>>
>>> Was this intended?  Most of the boards have auto probing so the
>>> ->board_ptr gets set automatically.  We already called
>>> comedi_board() so I wonder if the autoprobed board is the same as
>>> the&s526_boards[0];?  NULL pointer perhaps?  I don't know.
>>
>> If .num_names in struct comedi_driver is non-zero, the core comedi
>> module will set dev->board_ptr to something non-NULL and meaningful to
>> the driver before it calls the driver's ->attach() method.  (This is
>> done by the comedi_recognize() function.)  The driver can change
>> dev->board_ptr to something else if it wants.  In this case,
>> dev->board_ptr will already be set to&s526_boards[0] so there is no
>> point setting it again.
>
> Ian,
>
> Side-note on the boardinfo stuff.
>
> I wonder if it makes sense to create a common boardinfo struct
> and modify the comedi_driver struct a bit. Something like:
>
> struct comedi_boardinfo {
> 	const char *name;
> 	unsigned short	vendor;
> 	unsigned short device;
> 	void *private;
> };
>
> struct comedi_driver {
> 	struct comedi_driver *next;
>
> 	const char *driver_name;
> 	struct module *module;
> 	int (*attach) (struct comedi_device *, struct comedi_devconfig *);
> 	void (*detach) (struct comedi_device *);
> 	int (*attach_pci) (struct comedi_device *, struct pci_dev *);
> 	int (*attach_usb) (struct comedi_device *, struct usb_interface *);
>
> 	u32 num_boards;
> 	struct comedi_boardinfo *board;
> };
>
> This would allow comedi_recognize() to walk the boardinfo
> to find the match without all the ugly casts. It should also
> allow creating a common comedi_find_pcidev() for all the
> comedi pci drivers to use when probing for the board_ptr.
>
> This would create a bit of churn but I think it would be a
> lot cleaner in the end.
>
> What do you think?

Please don't do this yet.  We might want to implement auto configuration 
for device types that have no concept of vendor ID or product ID, such 
as PCMCIA, platform devices, SPI devices etc., either by adding extra 
attach_xxx hooks to struct comedi_driver for the more common bus types, 
or a more generic attach_generic hook for the less common bus types.

Also, it would waste space in the ISA-only drivers.

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