[PATCH v2] Staging: speakup: Add helper macro for spk_synth boilerplate

[PATCH v2] Staging: speakup: Add helper macro for spk_synth boilerplate

[Vaishali Thakkar]

For simple modules that contain a single spk_synth without
any additional setup code then ends up being a block of
duplicated boilerplate. This patch adds a new macro,
module_spk_synth(), which replaces the
module_init()/module_exit() registrations with template
functions.

Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
---
v1 : Here, I know, I can go for changing all files
     and make them use module_spk_synth at a single point.
     But as I am sending this patch as a sample patch, I
     am changing only one file to show the use of helper
     macro. I will go for changing all other such cases in
     this driver if this patch will be accepted.
v2 : Use module_driver macro in definition of helper macro

 drivers/staging/speakup/speakup_audptr.c | 12 +-----------
 drivers/staging/speakup/spk_types.h      | 12 ++++++++++++
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/speakup/speakup_audptr.c b/drivers/staging/speakup/speakup_audptr.c
index 5cbaec8..ea89e36 100644
--- a/drivers/staging/speakup/speakup_audptr.c
+++ b/drivers/staging/speakup/speakup_audptr.c
@@ -177,18 +177,8 @@ module_param_named(start, synth_audptr.startup, short, S_IRUGO);
 MODULE_PARM_DESC(ser, "Set the serial port for the synthesizer (0-based).");
 MODULE_PARM_DESC(start, "Start the synthesizer once it is loaded.");
 
-static int __init audptr_init(void)
-{
-	return synth_add(&synth_audptr);
-}
-
-static void __exit audptr_exit(void)
-{
-	synth_remove(&synth_audptr);
-}
+module_spk_synth(synth_audptr);
 
-module_init(audptr_init);
-module_exit(audptr_exit);
 MODULE_AUTHOR("Kirk Reiser <kirk@braille.uwo.ca>");
 MODULE_AUTHOR("David Borowski");
 MODULE_DESCRIPTION("Speakup support for Audapter synthesizer");
diff --git a/drivers/staging/speakup/spk_types.h b/drivers/staging/speakup/spk_types.h
index 8c565c9..67aa5e9 100644
--- a/drivers/staging/speakup/spk_types.h
+++ b/drivers/staging/speakup/spk_types.h
@@ -16,6 +16,7 @@
 #include <linux/spinlock.h>
 #include <linux/mutex.h>
 #include <linux/io.h>		/* for inb_p, outb_p, inb, outb, etc... */
+#include <linux/device.h>	/* for struct device */
 
 enum var_type_t {
 	VAR_NUM = 0,
@@ -179,6 +180,17 @@ struct spk_synth {
 	struct attribute_group attributes;
 };
 
+/**
+ * module_spk_synth() - Helper macro for registering a speakup driver
+ * @__spk_synth: spk_synth struct
+ *
+ * Helper macro for speakup 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_spk_synth(__spk_synth) \
+	module_driver(__spk_synth, synth_add, \
+			synth_remove)
 struct speakup_info_t {
 	spinlock_t spinlock;
 	int port_tts;
-- 
1.9.1

Re: [Outreachy kernel] [PATCH v2] Staging: speakup: Add helper macro for spk_synth boilerplate

[Greg KH]

On Mon, Mar 16, 2015 at 07:59:01PM +0530, Vaishali Thakkar wrote:
> For simple modules that contain a single spk_synth without
> any additional setup code then ends up being a block of
> duplicated boilerplate. This patch adds a new macro,
> module_spk_synth(), which replaces the
> module_init()/module_exit() registrations with template
> functions.
> 
> Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
> ---
> v1 : Here, I know, I can go for changing all files
>      and make them use module_spk_synth at a single point.
>      But as I am sending this patch as a sample patch, I
>      am changing only one file to show the use of helper
>      macro. I will go for changing all other such cases in
>      this driver if this patch will be accepted.
> v2 : Use module_driver macro in definition of helper macro
> 
>  drivers/staging/speakup/speakup_audptr.c | 12 +-----------
>  drivers/staging/speakup/spk_types.h      | 12 ++++++++++++
>  2 files changed, 13 insertions(+), 11 deletions(-)

Please make this a patch series, the first one adding the new macro, and
the rest converting all of the different modules to use it.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH v2] Staging: speakup: Add helper macro for spk_synth boilerplate

[Vaishali Thakkar]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

On Mar 16, 2015 8:58 PM, "Greg KH" <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Mar 16, 2015 at 07:59:01PM +0530, Vaishali Thakkar wrote:
> > For simple modules that contain a single spk_synth without
> > any additional setup code then ends up being a block of
> > duplicated boilerplate. This patch adds a new macro,
> > module_spk_synth(), which replaces the
> > module_init()/module_exit() registrations with template
> > functions.
> >
> > Signed-off-by: Vaishali Thakkar <vthakkar1994@gmail.com>
> > ---
> > v1 : Here, I know, I can go for changing all files
> >      and make them use module_spk_synth at a single point.
> >      But as I am sending this patch as a sample patch, I
> >      am changing only one file to show the use of helper
> >      macro. I will go for changing all other such cases in
> >      this driver if this patch will be accepted.
> > v2 : Use module_driver macro in definition of helper macro
> >
> >  drivers/staging/speakup/speakup_audptr.c | 12 +-----------
> >  drivers/staging/speakup/spk_types.h      | 12 ++++++++++++
> >  2 files changed, 13 insertions(+), 11 deletions(-)
>
> Please make this a patch series, the first one adding the new macro, and
> the rest converting all of the different modules to use it.

Ok. Will do it.

Thank You

> thanks,
>
> greg k-h

[-- Attachment #2: Type: text/html, Size: 1766 bytes --]

Re: [Outreachy kernel] [PATCH v2] Staging: speakup: Add helper macro for spk_synth boilerplate

[Julia Lawall]

> +#define module_spk_synth(__spk_synth) \
> +	module_driver(__spk_synth, synth_add, \
> +			synth_remove)

Why the newline before synth_remove?

julia

Re: [Outreachy kernel] [PATCH v2] Staging: speakup: Add helper macro for spk_synth boilerplate

[Vaishali Thakkar]

On Mon, Mar 16, 2015 at 10:19 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> +#define module_spk_synth(__spk_synth) \
>> +     module_driver(__spk_synth, synth_add, \
>> +                     synth_remove)
>
> Why the newline before synth_remove?

Just for the consistency. I followed all such macro examples
like module_usb_driver, module_platform_driver, module_pci_driver
etc. And I guess may be this line will go over 80 characters though
I haven't checked that.

Should I go for writing like this in a single line
[if it will not go beyond 80 characters]?

module_driver(__spk_synth, synth_add,  synth_remove) \

> julia



-- 
Vaishali

Re: [Outreachy kernel] [PATCH v2] Staging: speakup: Add helper macro for spk_synth boilerplate

[Greg KH]

On Mon, Mar 16, 2015 at 10:34:34PM +0530, Vaishali Thakkar wrote:
> On Mon, Mar 16, 2015 at 10:19 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >> +#define module_spk_synth(__spk_synth) \
> >> +     module_driver(__spk_synth, synth_add, \
> >> +                     synth_remove)
> >
> > Why the newline before synth_remove?
> 
> Just for the consistency. I followed all such macro examples
> like module_usb_driver, module_platform_driver, module_pci_driver
> etc. And I guess may be this line will go over 80 characters though
> I haven't checked that.
> 
> Should I go for writing like this in a single line
> [if it will not go beyond 80 characters]?
> 
> module_driver(__spk_synth, synth_add,  synth_remove) \

Yes, please do so, the only reason I wrapped the module_usb_driver()
lines was because I had to.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH v2] Staging: speakup: Add helper macro for spk_synth boilerplate

[Vaishali Thakkar]

On Mon, Mar 16, 2015 at 10:38 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Mar 16, 2015 at 10:34:34PM +0530, Vaishali Thakkar wrote:
>> On Mon, Mar 16, 2015 at 10:19 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> >> +#define module_spk_synth(__spk_synth) \
>> >> +     module_driver(__spk_synth, synth_add, \
>> >> +                     synth_remove)
>> >
>> > Why the newline before synth_remove?
>>
>> Just for the consistency. I followed all such macro examples
>> like module_usb_driver, module_platform_driver, module_pci_driver
>> etc. And I guess may be this line will go over 80 characters though
>> I haven't checked that.
>>
>> Should I go for writing like this in a single line
>> [if it will not go beyond 80 characters]?
>>
>> module_driver(__spk_synth, synth_add,  synth_remove) \
>
> Yes, please do so, the only reason I wrapped the module_usb_driver()
> lines was because I had to.

Ok. Will go for it too.

> thanks,
>
> greg k-h



-- 
Vaishali

Re: [Outreachy kernel] [PATCH v2] Staging: speakup: Add helper macro for spk_synth boilerplate

[Julia Lawall]

On Mon, 16 Mar 2015, Vaishali Thakkar wrote:

> On Mon, Mar 16, 2015 at 10:19 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >> +#define module_spk_synth(__spk_synth) \
> >> +     module_driver(__spk_synth, synth_add, \
> >> +                     synth_remove)
> >
> > Why the newline before synth_remove?
> 
> Just for the consistency. I followed all such macro examples
> like module_usb_driver, module_platform_driver, module_pci_driver
> etc. And I guess may be this line will go over 80 characters though
> I haven't checked that.
> 
> Should I go for writing like this in a single line
> [if it will not go beyond 80 characters]?
> 
> module_driver(__spk_synth, synth_add,  synth_remove) \

I think this will look better (without the final \).

julia