From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753535AbbESHuy (ORCPT ); Tue, 19 May 2015 03:50:54 -0400 Received: from cantor2.suse.de ([195.135.220.15]:41929 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750742AbbESHuu (ORCPT ); Tue, 19 May 2015 03:50:50 -0400 Subject: Re: [PATCH v5 WIP 3/5] i2c-parport: define ports to connect From: Jean Delvare To: Sudip Mukherjee Cc: Greg KH , Dan Carpenter , One Thousand Gnomes , linux-kernel@vger.kernel.org In-Reply-To: <1430907377-17147-3-git-send-email-sudipm.mukherjee@gmail.com> References: <1430907377-17147-1-git-send-email-sudipm.mukherjee@gmail.com> <1430907377-17147-3-git-send-email-sudipm.mukherjee@gmail.com> Content-Type: text/plain; charset="UTF-8" Organization: Suse Linux Date: Tue, 19 May 2015 09:50:47 +0200 Message-ID: <1432021847.24979.15.camel@chaos.site> Mime-Version: 1.0 X-Mailer: Evolution 2.28.2 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Sudip, Le Wednesday 06 May 2015 à 15:46 +0530, Sudip Mukherjee a écrit : > as of now i2c-parport was connecting to all the available parallel > ports. Lets limit that to maximum of 4 instances and at the same time > define which instance connects to which parallel port A leading capital and a trailing dot would look better. > Signed-off-by: Sudip Mukherjee > --- > drivers/i2c/busses/i2c-parport.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/i2c/busses/i2c-parport.c b/drivers/i2c/busses/i2c-parport.c > index a1fac5a..a9b25c3 100644 > --- a/drivers/i2c/busses/i2c-parport.c > +++ b/drivers/i2c/busses/i2c-parport.c > @@ -46,6 +46,9 @@ struct i2c_par { > > static LIST_HEAD(adapter_list); > static DEFINE_MUTEX(adapter_list_lock); > +#define MAX_DEVICE 4 > +static int parport[MAX_DEVICE] = {0, -1, -1, -1}; > + > > /* ----- Low-level parallel port access ----------------------------------- */ > > @@ -163,6 +166,18 @@ static void i2c_parport_irq(void *data) > static void i2c_parport_attach(struct parport *port) > { > struct i2c_par *adapter; > + int i; > + > + for (i = 0; i < MAX_DEVICE; i++) { > + if (parport[i] == -1) > + continue; > + if (port->number == parport[i]) > + break; > + } > + if (i == MAX_DEVICE) { > + pr_err("port mentioned not found\n"); This error message needs to be improved. Someone seeing this in his/her logs would have no idea where it comes from and what it means exactly. You want to add "i2c-parport: " at the beginning, and say which port number was specified. > + return; > + } > > adapter = kzalloc(sizeof(struct i2c_par), GFP_KERNEL); > if (adapter == NULL) { > @@ -298,5 +313,11 @@ MODULE_AUTHOR("Jean Delvare "); > MODULE_DESCRIPTION("I2C bus over parallel port"); > MODULE_LICENSE("GPL"); > > +module_param_array(parport, int, NULL, 0); > +MODULE_PARM_DESC(parport, "Atmost 4 instances are allowed.\n" You should first say what the parameter does, before going into the details. Please use __stringify(MAX_DEVICE) instead of hard-coding 4, so that it doesn't need to be updated if MAX_DEVICE ever changes. > + "Mention portnumbers in array.\n" Missing space between "port" and "numbers". Don't mention that this is an array here, "modinfo" already prints the parameter type. For improved readability, please also add a leading space to the second and following lines (as done for parameter "type" in i2c-parport.h.) > + "If the port is not to be used mention -1.\n" > + "Default is one instance connected to parport0\n"); Missing trailing dot. > + > module_init(i2c_parport_init); > module_exit(i2c_parport_exit); I tested this patch and it works fine. Tested-by: Jean Delvare -- Jean Delvare SUSE L3 Support