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

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

On Tue, May 19, 2015 at 11:28:15AM +0200, Jean Delvare wrote:
> Le Tuesday 19 May 2015 à 14:14 +0530, Sudip Mukherjee a écrit :
> > 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 :
<snip>
> > 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 suggest formatting this in such a way that "\n"s are at the end of
> lines (as your patch had.) This makes this much more readable and
> unexpected embedded line breaks and spaces as you have here (the output
> of modinfo would look ugly.)
> 
> Also the initial description is still misleading, and there are too many
> occurrences of "mention" to my taste. I'd suggest the following:
> 
> MODULE_PARM_DESC(parport,
> 	"List of parallel ports to bind to, by index.\n"
> 	" Atmost " __stringify(MAX_DEVICE) " devices are supported.\n"
> 	" Default is one device connected to parport0.\n"
> );
> 
> I have removed the mention of -1 as this is an internal implementation
> detail and I can't think of any reason why the user would ever want to
> pass it.
will use this.
> 
> 
> > > 
> > <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?
> 
> Yes.

Thanks Jean.

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