Re: [lm-sensors] Hardware monitoring subsystem maintainer positionis open

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux