Re: [PATCH] Driver for GE PIO2 VME Card

[Martyn Welch <martyn.welch@ge.com>]

On 04/11/11 19:55, Paul Bolle wrote:
> A few random remarks follow. Ie, things that came up while quickly
> scanning this.
> 
> On Fri, 2011-11-04 at 17:33 +0000, Martyn Welch wrote:
>> diff --git a/drivers/staging/vme/devices/Kconfig b/drivers/staging/vme/devices/Kconfig
>> index ca5ba89..99f5414 100644
>> --- a/drivers/staging/vme/devices/Kconfig
>> +++ b/drivers/staging/vme/devices/Kconfig
>> @@ -6,3 +6,13 @@ config VME_USER
>>  	  If you say Y here you want to be able to access a limited number of
>>  	  VME windows in a manner at least semi-compatible with the interface
>>  	  provided with the original driver at http://vmelinux.org/.
> 
> Blank line here.
> 

ok

>> +config VME_PIO2
>> +	tristate "GE PIO2 VME"
>> +	help
>> +	  If you say Y here you have a GE PIO2. The PIO2 is a 6U VME Card,
> 
> Maybe something like: "Say Y here if you have a GE PIO2."?
> 

Yup, thats better :-)

>> +	  implementing 32 solid-state relay switched IO lines, in 4 groups of 8.
>> +	  The IO lines are provided as input, output or both as a build time
>> +	  option.
> 
> What option would that be?
> 

I think "Each bank of IO lines is pre-configured as input, output or both
depending on the variant of the card" may be a bit clearer?

>> +	  Otherwise it is safe to say N here.
>> +
> 
> A lot of modules have the "if you say M here the module will be called"
> boilerplate here (which actually doesn't match the line I just made up).
> 

ok

>> +/* These are required for each board */
>> +MODULE_PARM_DESC(bus, "Enumeration of VMEbus to which the board is connected");
>> +module_param_array(bus, int, &bus_num, 0);
>> +
>> +MODULE_PARM_DESC(base, "Base VME address for PIO2 Registers");
>> +module_param_array(base, long, &base_num, 0);
>> +
>> +MODULE_PARM_DESC(vector, "VME IRQ Vector (Lower 4 bits masked)");
>> +module_param_array(vector, int, &vector_num, 0);
>> +
>> +MODULE_PARM_DESC(level, "VME IRQ Level");
>> +module_param_array(level, int, &level_num, 0);
>> +
>> +MODULE_PARM_DESC(variant, "Last 4 characters of PIO2 board variant");
>> +module_param_array(variant, charp, &variant_num, 0);
>> +
>> +/* This is for debugging */
>> +MODULE_PARM_DESC(loopback, "Enable loopback mode on all cards");
>> +module_param(loopback, bool, 0);
> 
> All module parameters have a sysfs visibility (or permission) of zero.
> Why is that? (This might very well be a naive question. But I often
> wonder why a certain parameter's permission isn't at least 400, just to
> allow a quick check of that parameter.) Are arrays tricky in sysfs?
> 

Hadn't really thought about it to be honest. There seems to be plenty of
examples where it's set to non-zero.

Martyn

> I can't remember ever having seen guidelines for this. It's possible
> they exist and my question is answered after reading those.
> 
> 
> Paul Bolle
> 


-- 
Martyn Welch (Principal Software Engineer) | Registered in England and
GE Intelligent Platforms                   | Wales (3828642) at 100
T +44(0)1327322748                         | Barbirolli Square, Manchester,
E martyn.welch@ge.com                      | M2 3AB  VAT:GB 927559189