[PATCH] staging: comedi: Add helper macro for comedi pci driver boilerplate

[PATCH] staging: comedi: Add helper macro for comedi pci driver boilerplate

[H Hartley Sweeten]

Introduce the module_comedi_pci_driver macro, and the
associated register/unregister functions, which is a
convenience macro for comedi pci driver modules similar
to module_platform_driver. It is intended to be used by
drivers where the init/exit section does nothing but
register/unregister the comedi driver and associated pci
driver. By using this macro it is possible to eliminate
a few lines of boilerplate code per comedi pci driver.

Also, when registering the pci driver check for failure
and unregister the comedi driver.

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/comedidev.h b/drivers/staging/comedi/comedidev.h
index 8828609..92a1199 100644
--- a/drivers/staging/comedi/comedidev.h
+++ b/drivers/staging/comedi/comedidev.h
@@ -310,6 +310,25 @@ int comedi_driver_unregister(struct comedi_driver *);
 	module_driver(__comedi_driver, comedi_driver_register, \
 			comedi_driver_unregister)
 
+struct pci_driver;
+
+int comedi_pci_driver_register(struct comedi_driver *, struct pci_driver *);
+void comedi_pci_driver_unregister(struct comedi_driver *, struct pci_driver *);
+
+/**
+ * module_comedi_pci_driver() - Helper macro for registering a comedi PCI driver
+ * @__comedi_driver: comedi_driver struct
+ * @__pci_driver: pci_driver struct
+ *
+ * Helper macro for comedi PCI drivers which do not do anything special
+ * in module init/exit. This eliminates a lot of boilerplate. Each
+ * module may only use this macro once, and calling it replaces
+ * module_init() and module_exit()
+ */
+#define module_comedi_pci_driver(__comedi_driver, __pci_driver) \
+	module_driver(__comedi_driver, comedi_pci_driver_register, \
+			comedi_pci_driver_unregister, &(__pci_driver))
+
 void init_polling(void);
 void cleanup_polling(void);
 void start_polling(struct comedi_device *);
diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
index 49681a1..7071109 100644
--- a/drivers/staging/comedi/drivers.c
+++ b/drivers/staging/comedi/drivers.c
@@ -950,6 +950,34 @@ void comedi_pci_auto_unconfig(struct pci_dev *pcidev)
 }
 EXPORT_SYMBOL_GPL(comedi_pci_auto_unconfig);
 
+int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
+		struct pci_driver *pci_driver)
+{
+	int ret;
+
+	ret = comedi_driver_register(comedi_driver);
+	if (ret < 0)
+		return ret;
+
+	pci_driver->name = comedi_driver->driver_name;
+	ret = pci_register_driver(pci_driver);
+	if (ret < 0) {
+		comedi_driver_unregister(comedi_driver);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(comedi_pci_driver_register);
+
+void comedi_pci_driver_unregister(struct comedi_driver *comedi_driver,
+		struct pci_driver *pci_driver)
+{
+	pci_unregister_driver(pci_driver);
+	comedi_driver_unregister(comedi_driver);
+}
+EXPORT_SYMBOL(comedi_pci_driver_unregister);
+
 static int comedi_old_usb_auto_config(struct usb_interface *intf,
 				      struct comedi_driver *driver)
 {

Re: [PATCH] staging: comedi: Add helper macro for comedi pci driver boilerplate

[Ian Abbott]

On 2012-05-11 18:07, H Hartley Sweeten wrote:
> +int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
> +		struct pci_driver *pci_driver)
> +{
> +	int ret;
> +
> +	ret = comedi_driver_register(comedi_driver);
> +	if (ret<  0)
> +		return ret;
> +
> +	pci_driver->name = comedi_driver->driver_name;

I think it would be better if the driver modules that call this function 
already had pci_driver->name initialized statically.

-- 
-=( 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: Add helper macro for comedi pci driver boilerplate

[H Hartley Sweeten]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 839 bytes --]

On Friday, May 11, 2012 10:32 AM, Ian Abbott wrote:
> On 2012-05-11 18:07, H Hartley Sweeten wrote:
>> +int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
>> +		struct pci_driver *pci_driver)
>> +{
>> +	int ret;
>> +
>> +	ret = comedi_driver_register(comedi_driver);
>> +	if (ret<  0)
>> +		return ret;
>> +
>> +	pci_driver->name = comedi_driver->driver_name;
>
> I think it would be better if the driver modules that call this function 
> already had pci_driver->name initialized statically.

I agree, but this is how all the pci drivers do it now.

To change this will require auditing all the comedi pci drivers and adding
the static name.

Regards,
Hartley

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] staging: comedi: Add helper macro for comedi pci driver boilerplate

[Ian Abbott]

On 2012-05-11 18:34, H Hartley Sweeten wrote:
> On Friday, May 11, 2012 10:32 AM, Ian Abbott wrote:
>> On 2012-05-11 18:07, H Hartley Sweeten wrote:
>>> +int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
>>> +		struct pci_driver *pci_driver)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = comedi_driver_register(comedi_driver);
>>> +	if (ret<   0)
>>> +		return ret;
>>> +
>>> +	pci_driver->name = comedi_driver->driver_name;
>>
>> I think it would be better if the driver modules that call this function
>> already had pci_driver->name initialized statically.
>
> I agree, but this is how all the pci drivers do it now.
>
> To change this will require auditing all the comedi pci drivers and adding
> the static name.

But if you're going to be calling this function from those drivers 
anyway, you might as well add the static names at the same time!  Saves 
another round of patches.

-- 
-=( 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: Add helper macro for comedi pci driver boilerplate

[H Hartley Sweeten]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1868 bytes --]

On Friday, May 11, 2012 10:42 AM, Ian Abbott wrote:
> On 2012-05-11 18:34, H Hartley Sweeten wrote:
>> On Friday, May 11, 2012 10:32 AM, Ian Abbott wrote:
>>> On 2012-05-11 18:07, H Hartley Sweeten wrote:
>>>> +int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
>>>> +		struct pci_driver *pci_driver)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = comedi_driver_register(comedi_driver);
>>>> +	if (ret<   0)
>>>> +		return ret;
>>>> +
>>>> +	pci_driver->name = comedi_driver->driver_name;
>>>
>>> I think it would be better if the driver modules that call this function
>>> already had pci_driver->name initialized statically.
>>
>> I agree, but this is how all the pci drivers do it now.
>>
>> To change this will require auditing all the comedi pci drivers and adding
>> the static name.
>
> But if you're going to be calling this function from those drivers 
> anyway, you might as well add the static names at the same time!  Saves 
> another round of patches.

True, but many of the pci drivers still need quite a bit of cleanup.
After this patch was accepted I was going to just change the ones
that have already had the init/exit stuff cleaned up. Then as I hit
all the others I would change them.

I guess I could do it this way:

1) add the "name" to all the pci drivers and remove the
"pci_driver->name = comedi_driver->name" from all the
module_init functions.

2) add the module_comedi_pci_driver stuff, without the
"pci_driver->name = comedi_driver->name"

3) Update all the refactored comedi pci drivers to use
the module_comedi_pci_driver macro.

4) As the other comedi pci drivers are refactored, use
the module_comedi_pci_driver macro.

How does that sound?

Regards,
Hartley

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] staging: comedi: Add helper macro for comedi pci driver boilerplate

[Ian Abbott]

On 2012-05-11 18:50, H Hartley Sweeten wrote:
> On Friday, May 11, 2012 10:42 AM, Ian Abbott wrote:
>> On 2012-05-11 18:34, H Hartley Sweeten wrote:
>>> On Friday, May 11, 2012 10:32 AM, Ian Abbott wrote:
>>>> On 2012-05-11 18:07, H Hartley Sweeten wrote:
>>>>> +int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
>>>>> +		struct pci_driver *pci_driver)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = comedi_driver_register(comedi_driver);
>>>>> +	if (ret<    0)
>>>>> +		return ret;
>>>>> +
>>>>> +	pci_driver->name = comedi_driver->driver_name;
>>>>
>>>> I think it would be better if the driver modules that call this function
>>>> already had pci_driver->name initialized statically.
>>>
>>> I agree, but this is how all the pci drivers do it now.
>>>
>>> To change this will require auditing all the comedi pci drivers and adding
>>> the static name.
>>
>> But if you're going to be calling this function from those drivers
>> anyway, you might as well add the static names at the same time!  Saves
>> another round of patches.
>
> True, but many of the pci drivers still need quite a bit of cleanup.
> After this patch was accepted I was going to just change the ones
> that have already had the init/exit stuff cleaned up. Then as I hit
> all the others I would change them.
>
> I guess I could do it this way:
>
> 1) add the "name" to all the pci drivers and remove the
> "pci_driver->name = comedi_driver->name" from all the
> module_init functions.
>
> 2) add the module_comedi_pci_driver stuff, without the
> "pci_driver->name = comedi_driver->name"
>
> 3) Update all the refactored comedi pci drivers to use
> the module_comedi_pci_driver macro.
>
> 4) As the other comedi pci drivers are refactored, use
> the module_comedi_pci_driver macro.
>
> How does that sound?

Okay, but I don't think it's worth doing three patches for each driver 
where one would do.  Adding a one-line .name = "foo", to the single 
patch for each driver you were planning to do anyway seems like a 
trivial addition that could be slotted into the patch without too much 
argument from the "one change per patch" folks, especially if you 
mention the purpose of that line in the commit message.  (I'd defer to 
the better judgement of Greg though.)

-- 
-=( 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: Add helper macro for comedi pci driver boilerplate

[H Hartley Sweeten]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1525 bytes --]

On Friday, May 11, 2012 11:14 AM, Ian Abbott wrote:
> Okay, but I don't think it's worth doing three patches for each driver 
> where one would do.  Adding a one-line .name = "foo", to the single 
> patch for each driver you were planning to do anyway seems like a 
> trivial addition that could be slotted into the patch without too much 
> argument from the "one change per patch" folks, especially if you 
> mention the purpose of that line in the commit message.  (I'd defer to 
> the better judgement of Greg though.)

With this patch my main goal was to not "break" any of the drivers.

As a follow-up to this patch I was going to use the macro in all the
pci drivers that have been refactored. Similar to what I did with the
module_comedi_driver macro. When I do that I can add the static
name field to those drivers.

How about doing this in the register function:

	If (!pci_driver->name)
		pci_driver->name = comedi_driver->driver_name;

This way all the drivers that still need to be refactored will still work.

After all the drivers have been updated, those two lines can be
removed.

So we have this patch and one big patch updating all the refactored
drivers. Some number of patches refactoring the remaining drivers
and using the module_comedi_pci_driver macro. Then one last
patch removing the check above.

How does that sound?

Regards,
Hartley

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] staging: comedi: Add helper macro for comedi pci driver boilerplate

[gregkh]

On Fri, May 11, 2012 at 01:26:37PM -0500, H Hartley Sweeten wrote:
> On Friday, May 11, 2012 11:14 AM, Ian Abbott wrote:
> > Okay, but I don't think it's worth doing three patches for each driver 
> > where one would do.  Adding a one-line .name = "foo", to the single 
> > patch for each driver you were planning to do anyway seems like a 
> > trivial addition that could be slotted into the patch without too much 
> > argument from the "one change per patch" folks, especially if you 
> > mention the purpose of that line in the commit message.  (I'd defer to 
> > the better judgement of Greg though.)
> 
> With this patch my main goal was to not "break" any of the drivers.
> 
> As a follow-up to this patch I was going to use the macro in all the
> pci drivers that have been refactored. Similar to what I did with the
> module_comedi_driver macro. When I do that I can add the static
> name field to those drivers.
> 
> How about doing this in the register function:
> 
> 	If (!pci_driver->name)
> 		pci_driver->name = comedi_driver->driver_name;
> 
> This way all the drivers that still need to be refactored will still work.
> 
> After all the drivers have been updated, those two lines can be
> removed.
> 
> So we have this patch and one big patch updating all the refactored
> drivers. Some number of patches refactoring the remaining drivers
> and using the module_comedi_pci_driver macro. Then one last
> patch removing the check above.
> 
> How does that sound?

That sounds reasonable to me.

greg k-h

Re: [PATCH] staging: comedi: Add helper macro for comedi pci driver boilerplate

[Ian Abbott]

On 11/05/2012 22:59, gregkh@linuxfoundation.org wrote:
> On Fri, May 11, 2012 at 01:26:37PM -0500, H Hartley Sweeten wrote:
>> How about doing this in the register function:
>>
>> 	If (!pci_driver->name)
>> 		pci_driver->name = comedi_driver->driver_name;
>>
>> This way all the drivers that still need to be refactored will still work.
>>
>> After all the drivers have been updated, those two lines can be
>> removed.
>>
>> So we have this patch and one big patch updating all the refactored
>> drivers. Some number of patches refactoring the remaining drivers
>> and using the module_comedi_pci_driver macro. Then one last
>> patch removing the check above.
>>
>> How does that sound?
>
> That sounds reasonable to me.

Sounds reasonable to me too.

-- 
-=( 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: Add helper macro for comedi pci driver boilerplate

[Greg KH]

On Fri, May 11, 2012 at 10:07:43AM -0700, H Hartley Sweeten wrote:
> --- a/drivers/staging/comedi/drivers.c
> +++ b/drivers/staging/comedi/drivers.c
> @@ -950,6 +950,34 @@ void comedi_pci_auto_unconfig(struct pci_dev *pcidev)
>  }
>  EXPORT_SYMBOL_GPL(comedi_pci_auto_unconfig);
>  
> +int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
> +		struct pci_driver *pci_driver)
> +{
> +	int ret;
> +
> +	ret = comedi_driver_register(comedi_driver);
> +	if (ret < 0)
> +		return ret;
> +
> +	pci_driver->name = comedi_driver->driver_name;
> +	ret = pci_register_driver(pci_driver);
> +	if (ret < 0) {
> +		comedi_driver_unregister(comedi_driver);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(comedi_pci_driver_register);
> +
> +void comedi_pci_driver_unregister(struct comedi_driver *comedi_driver,
> +		struct pci_driver *pci_driver)
> +{
> +	pci_unregister_driver(pci_driver);
> +	comedi_driver_unregister(comedi_driver);
> +}
> +EXPORT_SYMBOL(comedi_pci_driver_unregister);

EXPORT_SYMBOL_GPL() for these two functions?

thanks,

greg k-h

RE: [PATCH] staging: comedi: Add helper macro for comedi pci driver boilerplate

[H Hartley Sweeten]

On Friday, May 11, 2012 11:00 AM, Greg KH wrote:
> On Fri, May 11, 2012 at 10:07:43AM -0700, H Hartley Sweeten wrote:
>> --- a/drivers/staging/comedi/drivers.c
>> +++ b/drivers/staging/comedi/drivers.c

<snip>

>> +EXPORT_SYMBOL(comedi_pci_driver_register);

<snip>

>> +EXPORT_SYMBOL(comedi_pci_driver_unregister);
>
> EXPORT_SYMBOL_GPL() for these two functions?

I wasn't sure about that...

Question.. when "should" EXPORT_SYMBOL be used and
when should it be EXPORT_SYMBOL_GPL?

Also, do you have any comments on Ian Abbott's comment
about the:

> +	pci_driver->name = comedi_driver->driver_name;

In the register function?

Regards,
Hartley

Re: [PATCH] staging: comedi: Add helper macro for comedi pci driver boilerplate

[Ian Abbott]

On 2012-05-11 19:04, H Hartley Sweeten wrote:
> On Friday, May 11, 2012 11:00 AM, Greg KH wrote:
>> On Fri, May 11, 2012 at 10:07:43AM -0700, H Hartley Sweeten wrote:
>>> +EXPORT_SYMBOL(comedi_pci_driver_unregister);
>>
>> EXPORT_SYMBOL_GPL() for these two functions?
>
> I wasn't sure about that...
>
> Question.. when "should" EXPORT_SYMBOL be used and
> when should it be EXPORT_SYMBOL_GPL?

Adding EXPORT_SYMBOL_GPL shouldn't remove previous functionality.  It 
doesn't in this case, so that's okay!

-- 
-=( 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: Add helper macro for comedi pci driver boilerplate

[H Hartley Sweeten]

On Friday, May 11, 2012 11:15 AM, Ian Abbott wrote:
> On 2012-05-11 19:04, H Hartley Sweeten wrote:
>> On Friday, May 11, 2012 11:00 AM, Greg KH wrote:
>>> On Fri, May 11, 2012 at 10:07:43AM -0700, H Hartley Sweeten wrote:
>>>> +EXPORT_SYMBOL(comedi_pci_driver_unregister);
>>>
>>> EXPORT_SYMBOL_GPL() for these two functions?
>>
>> I wasn't sure about that...
>>
>> Question.. when "should" EXPORT_SYMBOL be used and
>> when should it be EXPORT_SYMBOL_GPL?
>
> Adding EXPORT_SYMBOL_GPL shouldn't remove previous functionality.  It 
> doesn't in this case, so that's okay!

OK. I'll change it.

Regards,
Hartley