Date:	Thu, 12 Apr 2007 09:27:57 +0200
From:	Hans de Goede <j.w.r.degoede@....nl>
To:	Krzysztof Helt <krzysztof.h1@...pl>
CC:	"Mark M. Hoffman" <mhoffman@...htlink.com>,
	LKML <linux-kernel@...r.kernel.org>,
	LM Sensors <lm-sensors@...sensors.org>
Subject: Re: [lm-sensors] Hardware monitoring subsystem maintainer	positionis
 open

Krzysztof Helt wrote:
> This is comments from someone who is newbie to this list.
> 
>> 1a) We need a set of review guidelines / a review checklist.
> Here is a start:
> 
> Maybe these guidelines can be described in more details and with
> links or names of documents with more description.
> 

Yes they should, this was just a quick list. Feel free to help writing a better 
list. And I guess we should put this list in the wiki somewhere.

>> * Must follow kernel coding style guidelines
> 
> Is there any tool to check this? If there is one, a basic
> instruction how to use it would be great.
> 

No tool.

>> * sysfs interface must be in accordance with
> Documentation/hwmon/sysfs
> 
> The documentation is still confusing to me. Can someone of you
> update it to answer these questions directly?
> 
> A. What is meaning of 0 and 1 values in pwmX_enable ?
> B. How to stop the fan (pwmX_enable = 1 and pwmX = 0 was
> suggested to someone on the list)?
> C. How t o handle situation if the pwm register minimum value (0)
> does not stop the fan, but there is a separate bit to do it? (do
> stop emulate with the bit when 0 is written to pwmX?)
> 

I think this is best answered by Jean and/or Mark

>> * proper locking to avoid 2 simultaneous attempts to
> communicate with the
>>    device when for example multiple sysfs files get read at once.
> 
> An example or two common errors would be great help.
> 

Erm, if  someone doesn't know what this means / how to look for this he 
shouldn't be reviewing a driver.

>> * check the code for any obvious programming errors, like:
>>    -not freeing resources (in error paths and in general)
>>    -overrunning an array
>>    -not checking return values of calls to other parts of the
> kernel
>>    -of by one errors / bad loops
>>    -etc.
> 
> List them, so a newbie can check the code against it.
> 

The etc. was because I'm sure there are more but I couldn't come up with any 
right there and then. And again a newbie shouldn't be reviewing, he should 
start reading some book in device drivers writing a driver or two himself get 
those reviewed and then he should no longer be a newbie :)

>> 1b) We need to decide somehow who can do reviews. For now I say
> anyone who
>>    already maintains atleast one hwmon driver. But thats just a
> wild shot better
>>    ideas are welcome.
>>
> 
> There are volunteers already. In order to lessen their work you
> can require to follow the guidelines (if they got described) by
> authors of patches .

Yes ofcourse authors should follow the guidelines, and check they did 
themselves before submitting. Also its great that there are volunteers, but 
reviewing does require a certain level of competence. There are many other ways 
to help out for those without the necessary skills todo the reviewing.

For example you could check out the 3.0.0 branch:
svn checkout http://lm-sensors.org/svn/lm-sensors/branches/lm-sensors-3.0.0

Edit lib/chips.c, goto the long array at the end and comment any entries for 
chips you have. Optionally edit prog/sensors/main.c goto the array near the end 
and again comment entries for chips you have.

Build and install lm-sensors-3.0.0 and let us know how it works, despite the 
commenting it should still recognize your cips and print the same info as usual 
(it can now dynamicly recognize new/unknown chips as long as the kernel 
supports them). When you skipped the optional step run the sensors command as 
'sensors -g', the normal sensors command will be borked if you skipped this step.

Thanks & Regards,

Hans

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists