Re: [PATCH v5 WIP 3/5] i2c-parport: define ports to connect

[Sudip Mukherjee <sudipm.mukherjee@gmail.com>]

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 <sudip@vectorindia.org>
> > ---
<snip>
> > +	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 <jdelvare@suse.de>");
> >  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");

> 
<snip>
> 
> I tested this patch and it works fine.
> 
> Tested-by: Jean Delvare <jdelvare@suse.de>

do i add your Tested-by: to the main patch and this patch?

regards
sudip	
> 
> -- 
> Jean Delvare
> SUSE L3 Support
>