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

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

[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 <hsweeten@visionengravers.com>
Cc: Ian Abbott <abbotti@mev.co.uk>
Cc: Mori Hess <fmhess@users.sourceforge.net>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---

diff --git a/drivers/staging/comedi/drivers/s526.c b/drivers/staging/comedi/drivers/s526.c
index 7a56434..3c8e979 100644
--- a/drivers/staging/comedi/drivers/s526.c
+++ b/drivers/staging/comedi/drivers/s526.c
@@ -200,11 +200,6 @@ static const struct s526_board s526_boards[] = {
 #define ADDR_REG(reg) (dev->iobase + (reg))
 #define ADDR_CHAN_REG(reg, chan) (dev->iobase + (reg) + (chan) * 8)
 
-/*
- * Useful for shorthand access to the particular board structure
- */
-#define thisboard ((const struct s526_board *)dev->board_ptr)
-
 /* this structure is for data unique to this hardware driver.  If
    several hardware drivers keep similar information in this structure,
    feel free to suggest moving the variable to the struct comedi_device
@@ -744,6 +739,7 @@ static int s526_dio_insn_config(struct comedi_device *dev,
 
 static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 {
+	const struct s526_board *board = comedi_board(dev);
 	struct comedi_subdevice *s;
 	int iobase;
 	int i, n;
@@ -754,7 +750,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	printk(KERN_INFO "comedi%d: s526: ", dev->minor);
 
 	iobase = it->options[0];
-	if (!iobase || !request_region(iobase, S526_IOSIZE, thisboard->name)) {
+	if (!iobase || !request_region(iobase, S526_IOSIZE, board->name)) {
 		comedi_error(dev, "I/O port conflict");
 		return -EIO;
 	}
@@ -769,13 +765,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	}
 	***/
 
-/*
- * 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];
-
-	dev->board_name = thisboard->name;
+	dev->board_name = board->name;
 
 /*
  * Allocate the private structure area.  alloc_private() is a
@@ -797,7 +787,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 	s->type = COMEDI_SUBD_COUNTER;
 	s->subdev_flags = SDF_READABLE | SDF_WRITABLE | SDF_LSAMPL;
 	/* KG: What does SDF_LSAMPL (see multiq3.c) mean? */
-	s->n_chan = thisboard->gpct_chans;
+	s->n_chan = board->gpct_chans;
 	s->maxdata = 0x00ffffff;	/* 24 bit counter */
 	s->insn_read = s526_gpct_rinsn;
 	s->insn_config = s526_gpct_insn_config;
@@ -838,7 +828,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
 
 	s = dev->subdevices + 3;
 	/* digital i/o subdevice */
-	if (thisboard->have_dio) {
+	if (board->have_dio) {
 		s->type = COMEDI_SUBD_DIO;
 		s->subdev_flags = SDF_READABLE | SDF_WRITABLE;
 		s->n_chan = 8;

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

[Dan Carpenter]

On Tue, May 22, 2012 at 06:20:10PM -0700, H Hartley Sweeten wrote:
> 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 <hsweeten@visionengravers.com>
> Cc: Ian Abbott <abbotti@mev.co.uk>
> Cc: Mori Hess <fmhess@users.sourceforge.net>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> ---
> @@ -769,13 +765,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
>  	}
>  	***/
>  
> -/*
> - * 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.

> -
> -	dev->board_name = thisboard->name;
> +	dev->board_name = board->name;
>  
>  /*
>   * Allocate the private structure area.  alloc_private() is a

regards,
dan carpenter

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

[Ian Abbott]

On 2012-05-23 06:48, Dan Carpenter wrote:
> On Tue, May 22, 2012 at 06:20:10PM -0700, H Hartley Sweeten wrote:
>> 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<hsweeten@visionengravers.com>
>> Cc: Ian Abbott<abbotti@mev.co.uk>
>> Cc: Mori Hess<fmhess@users.sourceforge.net>
>> Cc: Greg Kroah-Hartman<gregkh@linuxfoundation.org>
>>
>> ---
>> @@ -769,13 +765,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
>>   	}
>>   	***/
>>
>> -/*
>> - * 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 Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-

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

[H Hartley Sweeten]

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?

Regards,
Hartley

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

[Ian Abbott]

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         )=-

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

[H Hartley Sweeten]

On Wednesday, May 23, 2012 9:49 AM, Ian Abbott wrote:
> On 2012-05-23 17:28, H Hartley Sweeten wrote:
>> 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.

OK. I'll hold off on doing anything like this.

But, I think we would actually save space in all the drivers. I just need
to think about it a bit more.

Regards,
Hartley

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

[Dan Carpenter]

On Wed, May 23, 2012 at 11:28:32AM -0500, 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.

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.

regards,
dan carpenter

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

[Ian Abbott]

On 2012-05-23 20:07, Dan Carpenter wrote:
> On Wed, May 23, 2012 at 11:28:32AM -0500, H Hartley Sweeten wrote:
>> 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.

Well maybe some of the consts.  We could use const char **name_ptr 
instead of const char *const *name_ptr.  *name_ptr is never modified, 
but as name_ptr is just a local variable, the compiler can figure that 
out for itself without the extra const.

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

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

[H Hartley Sweeten]

On Tuesday, May 22, 2012 10:49 PM, Dan Carpenter wrote:
> On Tue, May 22, 2012 at 06:20:10PM -0700, H Hartley Sweeten wrote:
>> 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 <hsweeten@visionengravers.com>
>> Cc: Ian Abbott <abbotti@mev.co.uk>
>> Cc: Mori Hess <fmhess@users.sourceforge.net>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> 
>> ---
>> @@ -769,13 +765,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
>>  	}
>>  	***/
>>  
>> -/*
>> - * 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); */

The only comedi drivers that modify the dev->board_ptr are the
PCI ones. Hopefully I can figure out a way to clean them up...
The comedi_board() inline does not change the dev->board_ptr,
it's simply a helper to fetch it from the comedi_device.

Regards,
Hartley

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

[Dan Carpenter]

On Wed, May 23, 2012 at 11:06:39AM -0500, H Hartley Sweeten wrote:
> On Tuesday, May 22, 2012 10:49 PM, Dan Carpenter wrote:
> > On Tue, May 22, 2012 at 06:20:10PM -0700, H Hartley Sweeten wrote:
> >> 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 <hsweeten@visionengravers.com>
> >> Cc: Ian Abbott <abbotti@mev.co.uk>
> >> Cc: Mori Hess <fmhess@users.sourceforge.net>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> 
> >> ---
> >> @@ -769,13 +765,7 @@ static int s526_attach(struct comedi_device *dev, struct comedi_devconfig *it)
> >>  	}
> >>  	***/
> >>  
> >> -/*
> >> - * 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); */
> 

Heh.  Yeah.  I saw you removed a bunch of these comments and this
was the only one that wasn't commented so I wondered.

Ian already explained how the probing works.  Looks good.

regards,
dan carpenter