Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 21 Jul 2021 10:21:46 +0200
From:   Jean Delvare <jdelvare@suse.de>
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc:     linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/1] i2c: parport: Switch to use
 module_parport_driver()
Message-ID: <20210721102146.66a486bc@endymion>
In-Reply-To: <20210712141119.22426-1-andriy.shevchenko@linux.intel.com>
References: <20210712141119.22426-1-andriy.shevchenko@linux.intel.com>
Organization: SUSE Linux
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Andy,

On Mon, 12 Jul 2021 17:11:19 +0300, Andy Shevchenko wrote:
> Switch to use module_parport_driver() to reduce boilerplate code.

This has the downside of moving the sanity check of the type parameter
to run time, instead of driver load time. In particular this means that
loading the i2c-parport driver without specifying the type will no
longer fail. The driver will load successfully, but won't do anything.

While I prefer user errors to be reported as soon as possible, I don't
really mind here, as parallel port drivers are not something worth
debating over at this point in time. As a matter of fact, I can't
possibly test this change as I no longer have a parallel port on any of
my systems.

So if that's the direction we want to take then so be it.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> v2: fixed compilation error (Jean, LKP)
>  drivers/i2c/busses/i2c-parport.c | 36 ++++++++++----------------------
>  1 file changed, 11 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c
> index a535889acca6..231145c48728 100644
> --- a/drivers/i2c/busses/i2c-parport.c
> +++ b/drivers/i2c/busses/i2c-parport.c
> @@ -267,6 +267,16 @@ static void i2c_parport_attach(struct parport *port)
>  	int i;
>  	struct pardev_cb i2c_parport_cb;
>  
> +	if (type < 0) {
> +		pr_warn("adapter type unspecified\n");
> +		return;
> +	}
> +
> +	if (type >= ARRAY_SIZE(adapter_parm)) {
> +		pr_warn("invalid type (%d)\n", type);
> +		return;
> +	}
> +
>  	for (i = 0; i < MAX_DEVICE; i++) {
>  		if (parport[i] == -1)
>  			continue;
> @@ -392,32 +402,8 @@ static struct parport_driver i2c_parport_driver = {
>  	.detach = i2c_parport_detach,
>  	.devmodel = true,
>  };
> -
> -/* ----- Module loading, unloading and information ------------------------ */
> -
> -static int __init i2c_parport_init(void)
> -{
> -	if (type < 0) {
> -		pr_warn("adapter type unspecified\n");
> -		return -ENODEV;
> -	}
> -
> -	if (type >= ARRAY_SIZE(adapter_parm)) {
> -		pr_warn("invalid type (%d)\n", type);
> -		return -ENODEV;
> -	}
> -
> -	return parport_register_driver(&i2c_parport_driver);
> -}
> -
> -static void __exit i2c_parport_exit(void)
> -{
> -	parport_unregister_driver(&i2c_parport_driver);
> -}
> +module_parport_driver(i2c_parport_driver);
>  
>  MODULE_AUTHOR("Jean Delvare <jdelvare@suse.de>");
>  MODULE_DESCRIPTION("I2C bus over parallel port");
>  MODULE_LICENSE("GPL");
> -
> -module_init(i2c_parport_init);
> -module_exit(i2c_parport_exit);


-- 
Jean Delvare
SUSE L3 Support