staging: comedi: remove this_board macro in the s526 driver

The 'thisboard' macro depends on having a local variable with a magic name. The CodingStyle document suggests not doing this to avoid confusion. Remove the macro and use the comedi_board() inline helper to get the dev->board_ptr information.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Ian Abbott <[email protected]>

Cc Mori Hess <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---

Quoted 15 lines · H Hartley Sweeten

The 'thisboard' macro depends on having a local variable with a magic name. The CodingStyle document suggests not doing this to avoid confusion. Remove the macro and use the comedi_board() inline helper to get the dev->board_ptr information.

Signed-off-by H Hartley Sweeten<[email protected]>

Cc Ian Abbott<[email protected]>

Cc Mori Hess<[email protected]>

Cc Greg Kroah-Hartman<[email protected]>

---

(quoted diff) +0 -0

@@ -769,13 +765,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
  	}
  	***/
 

drivers/staging/comedi/drivers/s526.c static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)

Quoted 5 lines

drivers/staging/comedi/drivers/s526.c +0 -5

-/*
- * 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.

Quoted 11 lines · Dan Carpenter

drivers/staging/comedi/drivers/s526.c static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)

Quoted 5 lines · H Hartley Sweeten

drivers/staging/comedi/drivers/s526.c +0 -5

-/*
- * 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.

Quoted 13 lines · Ian Abbott

Quoted 11 lines · Dan Carpenter

drivers/staging/comedi/drivers/s526.c static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)

Quoted 5 lines · H Hartley Sweeten

drivers/staging/comedi/drivers/s526.c +0 -5

-/*
- * 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.

Quoted 7 lines

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?

Quoted 31 lines · H Hartley Sweeten

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.

Quoted 4 lines

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.

Quoted 13 lines · Ian Abbott

Quoted 11 lines · Dan Carpenter

drivers/staging/comedi/drivers/s526.c static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)

Quoted 5 lines · H Hartley Sweeten

drivers/staging/comedi/drivers/s526.c +0 -5

-/*
- * 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.

Quoted 7 lines

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.

Quoted 2 lines · H Hartley Sweeten

This would allow comedi_recognize() to walk the boardinfo to find the match without all the ugly casts.

That function wouldn't be so bad if we just removed all the consts. There is no point because we drop all of them anyway when we return the struct.

Quoted 15 lines · H Hartley Sweeten

The 'thisboard' macro depends on having a local variable with a magic name. The CodingStyle document suggests not doing this to avoid confusion. Remove the macro and use the comedi_board() inline helper to get the dev->board_ptr information.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Ian Abbott <[email protected]>

Cc Mori Hess <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---

(quoted diff) +0 -0

@@ -769,13 +765,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	}
 	***/
 

drivers/staging/comedi/drivers/s526.c static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it) · line 776

Quoted 5 lines

drivers/staging/comedi/drivers/s526.c +0 -5

-/*
- * 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.

Quoted 16 lines · Dan Carpenter

Quoted 15 lines · H Hartley Sweeten

The 'thisboard' macro depends on having a local variable with a magic name. The CodingStyle document suggests not doing this to avoid confusion. Remove the macro and use the comedi_board() inline helper to get the dev->board_ptr information.

Signed-off-by H Hartley Sweeten <[email protected]>

Cc Ian Abbott <[email protected]>

Cc Mori Hess <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

---

(quoted diff) +0 -0

@@ -769,13 +765,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	}
 	***/
 

Quoted 10 lines

drivers/staging/comedi/drivers/s526.c static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it) · line 776

Quoted 5 lines

drivers/staging/comedi/drivers/s526.c +0 -5

-/*
- * 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.

Yes, removing the line was intended. Sorry I didn't mention it in the commit message.

The dev->board_ptr will already be set by comedi_device_attach() before the drivers attach() method is called.

I think the author of this driver misunderstood the skel driver and thought this was needed in order to simulate a "probe". See this comment in the skel.c driver:

/*
* If you can probe the device to determine what device in a series
* it is, this is the place to do it. Otherwise, dev->board_ptr
* should already be initialized.
*/

	/* dev->board_ptr = skel_probe(dev, it); */