Re: [PATCH v4 1/1] pps: generators: pps_gen_parport: Switch to use module_parport_driver()

Greg Kroah-Hartman
2022-02-07T14:46:47Z

On Mon, Feb 07, 2022 at 04:19:44PM +0200, Andy Shevchenko wrote:
> Switch to use module_parport_driver() to reduce boilerplate code.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Rodolfo Giometti <giometti@enneenne.com>
> ---
>
> v4: made sure the proper version is taken into consideration
> v3: added Ack (Rodolfo), Cc'ed to Greg for picking it up
>
> drivers/pps/generators/pps_gen_parport.c | 42 ++++--------------------
> 1 file changed, 7 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/pps/generators/pps_gen_parport.c b/drivers/pps/generators/pps_gen_parport.c
> index 6a1af7664f3b..b3e084b75c23 100644
> --- a/drivers/pps/generators/pps_gen_parport.c
> +++ b/drivers/pps/generators/pps_gen_parport.c
> @@ -20,8 +20,6 @@
> #include <linux/hrtimer.h>
> #include <linux/parport.h>
>
> -#define DRVDESC "parallel port PPS signal generator"
> -
> #define SIGNAL 0
> #define NO_SIGNAL PARPORT_CONTROL_STROBE
>
> @@ -180,6 +178,11 @@ static void parport_attach(struct parport *port)
> {
> struct pardev_cb pps_cb;
>
> + if (send_delay > SEND_DELAY_MAX) {
> + pr_err("delay value should be not greater then %d\n", SEND_DELAY_MAX);
> + return;
> + }
> +
> if (attached) {
> /* we already have a port */
> return;
> @@ -231,39 +234,8 @@ static struct parport_driver pps_gen_parport_driver = {
> .detach = parport_detach,
> .devmodel = true,
> };
> -
> -/* module staff */
> -
> -static int __init pps_gen_parport_init(void)
> -{
> - int ret;
> -
> - pr_info(DRVDESC "\n");
> -
> - if (send_delay > SEND_DELAY_MAX) {
> - pr_err("delay value should be not greater"
> - " then %d\n", SEND_DELAY_MAX);
> - return -EINVAL;
> - }
> -
> - ret = parport_register_driver(&pps_gen_parport_driver);
> - if (ret) {
> - pr_err("unable to register with parport\n");
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> -static void __exit pps_gen_parport_exit(void)
> -{
> - parport_unregister_driver(&pps_gen_parport_driver);
> - pr_info("hrtimer avg error is %ldns\n", hrtimer_error);
> -}
> -
> -module_init(pps_gen_parport_init);
> -module_exit(pps_gen_parport_exit);
> +module_parport_driver(pps_gen_parport_driver);
>
> MODULE_AUTHOR("Alexander Gordeev <lasaine@lvk.cs.msu.su>");
> -MODULE_DESCRIPTION(DRVDESC);
> +MODULE_DESCRIPTION("parallel port PPS signal generator");
> MODULE_LICENSE("GPL");
> --
> 2.34.1
>

You changed the functionality here :(

Currently if send_delay is too big, loading the module will fail.

Now it succeeds but binding to a device fails, that's a functional
change. Are you _SURE_ that is ok to do?

And why did you not mention any of this in the changelog text? You
passed this off as a mere code cleanup, no functional change :(

thanks,

greg k-h