From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754644AbbESIpX (ORCPT ); Tue, 19 May 2015 04:45:23 -0400 Received: from mail-pd0-f177.google.com ([209.85.192.177]:34273 "EHLO mail-pd0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750916AbbESIok (ORCPT ); Tue, 19 May 2015 04:44:40 -0400 Date: Tue, 19 May 2015 14:14:30 +0530 From: Sudip Mukherjee To: Jean Delvare Cc: Greg KH , Dan Carpenter , One Thousand Gnomes , linux-kernel@vger.kernel.org Subject: Re: [PATCH v5 WIP 3/5] i2c-parport: define ports to connect Message-ID: <20150519084430.GA5751@sudip-PC> References: <1430907377-17147-1-git-send-email-sudipm.mukherjee@gmail.com> <1430907377-17147-3-git-send-email-sudipm.mukherjee@gmail.com> <1432021847.24979.15.camel@chaos.site> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1432021847.24979.15.camel@chaos.site> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 19, 2015 at 09:50:47AM +0200, Jean Delvare wrote: > 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. sure. > > > Signed-off-by: Sudip Mukherjee > > --- > > + 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. oops. sorry about it. I saw all the other messages are having "i2c-parport: " mentioned. I will modify it. And maybe later I will send you another patch to use pr_fmt. Or here it may be better if I mention: pr_err("i2c-parport: You have chosen not to use parport%d.\n", port->number); > > > + 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. then what about: MODULE_PARM_DESC(parport, "Mention number of i2c-parport instances you want.\n Atmost " __stringify(MAX_DEVICE) " instances are allowed.\n Mention port numbers you want to use.\n" " If the port is not to be used mention -1.\n" " Default is one instance connected to parport0.\n"); > > > I tested this patch and it works fine. > > Tested-by: Jean Delvare do i add your Tested-by: to the main patch and this patch? regards sudip > > -- > Jean Delvare > SUSE L3 Support >