Re: [RFC] driver: Adding helper macro for platform_driver boilerplate.

Re: [RFC] driver: Adding helper macro for platform_driver boilerplate.

[Guenter Roeck]

On 5/8/20 4:39 AM, harshal chaudhari wrote:
> Hi Wim and Guenter,
> 
> 
> For simple module that contain a single platform_driver without any additional setup code then ends up being a block of duplicated boilerplate.
> 
> This patch add a new micro, module_platform_driver(), which replace the module_init()/module_exit() registrations with template functions.
> 
> 
> Signed-off-by: harshal chaudhari <harshalchau04@gmail.com <mailto:harshalchau04@gmail.com>>
> 

First, this is not in correct patch format. Second, I don't really see
the point of making such changes without converting the driver to use
the watchdog subsystem.

Guenter

> Patch as below:
> 
> diff --git a/drivers/watchdog/gef_wdt.c b/drivers/watchdog/gef_wdt.c
> index f6541d1b65e3..e1e9d5ecd31c 100644
> --- a/drivers/watchdog/gef_wdt.c
> +++ b/drivers/watchdog/gef_wdt.c
> @@ -300,6 +300,7 @@ static const struct of_device_id gef_wdt_ids[] = {
>         },
>         {},
>  };
> +
>  MODULE_DEVICE_TABLE(of, gef_wdt_ids);
>  
>  static struct platform_driver gef_wdt_driver = {
> @@ -311,19 +312,7 @@ static struct platform_driver gef_wdt_driver = {
>         .remove         = gef_wdt_remove,
>  };
>  
> -static int __init gef_wdt_init(void)
> -{
> -       pr_info("GE watchdog driver\n");
> -       return platform_driver_register(&gef_wdt_driver);
> -}
> -
> -static void __exit gef_wdt_exit(void)
> -{
> -       platform_driver_unregister(&gef_wdt_driver);
> -}
> -
> -module_init(gef_wdt_init);
> -module_exit(gef_wdt_exit);
> +module_platform_driver(gef_wdt_driver);
> 
> 
> 
> Thanks & Regards,
> 
> Harshal Chaudhari
> 

Re: [RFC] driver: Adding helper macro for platform_driver boilerplate.

[Guenter Roeck]

On 5/8/20 11:50 AM, harshal chaudhari wrote:
> Hi Guenter,
> 
> 
> Sorry for the last patch. please consider this new below patch.
> 
> 
> And yes driver is still running independently means without watchdog subsystem, but as init and exit functions just do a platform_driver_register and platform_driver_unregister, and nothing else, so its better to use the module_platform_driver macro rather replicating its implementation. and i have already tested it with this patch so its running fine.
> 

If you have the hardware, I would suggest to convert the driver to use
the watchdog subsystem. That would be much more valuable.

> 
> Signed-off-by: harshal chaudhari <harshalchau04@gmail.com <mailto:harshalchau04@gmail.com>>
> 

Please consult Documentation/process/submitting-patches.rst for guidelines
on how to submit patches.

Thanks,
Guenter

> From 11eeac26b352ad83fcf1d349c5e37040e6dc8c06 Mon Sep 17 00:00:00 2001
> From: Harshal <harshalchau04@gmail.com <mailto:harshalchau04@gmail.com>>
> Date: Fri, 8 May 2020 23:56:26 +0530
> Subject: [PATCH] [RFC] driver: Adding helper macro for platform_driver
>  boilerplate
> 
> ---
>  drivers/watchdog/gef_wdt.c | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/drivers/watchdog/gef_wdt.c b/drivers/watchdog/gef_wdt.c
> index f6541d1b65e3..4421a452d0f5 100644
> --- a/drivers/watchdog/gef_wdt.c
> +++ b/drivers/watchdog/gef_wdt.c
> @@ -311,19 +311,7 @@ static struct platform_driver gef_wdt_driver = {
>   .remove = gef_wdt_remove,
>  };
>  
> -static int __init gef_wdt_init(void)
> -{
> - pr_info("GE watchdog driver\n");
> - return platform_driver_register(&gef_wdt_driver);
> -}
> -
> -static void __exit gef_wdt_exit(void)
> -{
> - platform_driver_unregister(&gef_wdt_driver);
> -}
> -
> -module_init(gef_wdt_init);
> -module_exit(gef_wdt_exit);
> +module_platform_driver(gef_wdt_driver);
>  
>  MODULE_AUTHOR("Martyn Welch <martyn.welch@ge.com <mailto:martyn.welch@ge.com>>");
>  MODULE_DESCRIPTION("GE watchdog driver");
> -- 
> 2.17.1
> 
> 
> 
> 
> 
> 
> 
> 
> 
> On Fri, May 8, 2020 at 6:31 PM Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>> wrote:
> 
>     On 5/8/20 4:39 AM, harshal chaudhari wrote:
>     > Hi Wim and Guenter,
>     >
>     >
>     > For simple module that contain a single platform_driver without any additional setup code then ends up being a block of duplicated boilerplate.
>     >
>     > This patch add a new micro, module_platform_driver(), which replace the module_init()/module_exit() registrations with template functions.
>     >
>     >
>     > Signed-off-by: harshal chaudhari <harshalchau04@gmail.com <mailto:harshalchau04@gmail.com> <mailto:harshalchau04@gmail.com <mailto:harshalchau04@gmail.com>>>
>     >
> 
>     First, this is not in correct patch format. Second, I don't really see
>     the point of making such changes without converting the driver to use
>     the watchdog subsystem.
> 
>     Guenter
> 
>     > Patch as below:
>     >
>     > diff --git a/drivers/watchdog/gef_wdt.c b/drivers/watchdog/gef_wdt.c
>     > index f6541d1b65e3..e1e9d5ecd31c 100644
>     > --- a/drivers/watchdog/gef_wdt.c
>     > +++ b/drivers/watchdog/gef_wdt.c
>     > @@ -300,6 +300,7 @@ static const struct of_device_id gef_wdt_ids[] = {
>     >         },
>     >         {},
>     >  };
>     > +
>     >  MODULE_DEVICE_TABLE(of, gef_wdt_ids);
>     >  
>     >  static struct platform_driver gef_wdt_driver = {
>     > @@ -311,19 +312,7 @@ static struct platform_driver gef_wdt_driver = {
>     >         .remove         = gef_wdt_remove,
>     >  };
>     >  
>     > -static int __init gef_wdt_init(void)
>     > -{
>     > -       pr_info("GE watchdog driver\n");
>     > -       return platform_driver_register(&gef_wdt_driver);
>     > -}
>     > -
>     > -static void __exit gef_wdt_exit(void)
>     > -{
>     > -       platform_driver_unregister(&gef_wdt_driver);
>     > -}
>     > -
>     > -module_init(gef_wdt_init);
>     > -module_exit(gef_wdt_exit);
>     > +module_platform_driver(gef_wdt_driver);
>     >
>     >
>     >
>     > Thanks & Regards,
>     >
>     > Harshal Chaudhari
>     >
>