[lm-sensors] [PATCH v2] hwmon: Add submitting-patches checklist to documentation

[Jean Delvare]

On Tue, 5 Apr 2011 11:40:22 -0700, Guenter Roeck wrote:
> When writing hardware monitoring drivers, there are some common pitfalls which
> keep coming up in code reviews. This patch provides a document describing all
> those pitfalls and how to avoid them.
> 
> Signed-off-by: Guenter Roeck <guenter.roeck at ericsson.com>
> ---
> v2: Renamed and reorganized document. Added more items and clarifications
>     from review feedback.
> 
>  Documentation/hwmon/submitting-patches |   83 ++++++++++++++++++++++++++++++++
>  1 files changed, 83 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/hwmon/submitting-patches
> 
> diff --git a/Documentation/hwmon/submitting-patches b/Documentation/hwmon/submitting-patches
> new file mode 100644
> index 0000000..b98c8cd
> --- /dev/null
> +++ b/Documentation/hwmon/submitting-patches
> @@ -0,0 +1,83 @@

What about a short presentation of what this document is all about? One
line should to, two tops.

> +1. General

I think some decoration of the paragraph titles should improve
readability. E.g. use of uppercase and/or lines of dashes below the
text.

> +
> +* It should be unnecessary to mention, but please read and follow 
> +    Documentation/SubmitChecklist
> +    Documentation/SubmittingDrivers
> +    Documentation/SubmittingPatches
> +    Documentation/CodingStyle
> +
> +* If your patch generates checkpatch warnings, please refrain from explanations
> +  such as "I don't like that coding style". Keep in mind that each unnecessary
> +  warning helps hiding a real problem. If you don't like the kernel coding
> +  style, don't write kernel drivers.
> +
> +* Please test your patch thoroughly. We are not your test group.

Sometimes one can't test the patch because he/she has no hardware.
That's fine, but he/she should at least test-build the code on at least
one architecture  If run-time testing wasn't achieved, it should be
written explicitly below the patch header.

> +
> +* If your patch (or the driver) supports configuration options such as

s/supports/is affected by/ ?

> +  CONFIG_SMP or CONFIG_HOTPLUG, make sure it compiles for all configuration
> +  variants.
> +
> +
> +2. Adding functionality to existing drivers
> +
> +* Make sure the documentation in Documentation/hwmon/<driver_name> is up to
> +  date.
> +
> +* Make sure the information in Kconfig is up to date.
> +
> +* If the added functionality requires some cleanup or structural changes, split
> +  your patch into a cleanup part and the actual addition. This makes it easier
> +  to review your changes, and to bisect any resulting problems.
> +
> +* Never mix bug fixes, cleanup, and functional enhancements in a single patch.
> +
> +
> +3. New drivers
> +
> +* Running your patch or driver file(s) through checkpatch does not mean its
> +  formatting is clean. If unsure about formatting in your new driver, run it
> +  through Lindent. Lindent is not perfect, and you may have to do some minor
> +  cleanup, but it is a good start.
> +
> +* Consider adding yourself to MAINTAINERS.
> +
> +* Document the driver in Documentation/hwmon/<driver_name>.
> +
> +* Add the driver to Kconfig and Makefile in alphabetical order.
> +
> +* Make sure that all dependencies are listed in Kconfig. For new drivers, it
> +  is most likely prudent to add a dependency on EXPERIMENTAL.
> +
> +* Avoid forward declarations if you can. Rearrange the code if necessary.
> +
> +* Avoid calculations in macros and macro-generated functions. While such macros
> +  may save a line or so in the source, it obfuscates the code and makes code
> +  review more difficult. It may also result in code which is more complicated
> +  than necessary. Use inline functions or just regular functions instead.
> +
> +* If the driver has a detect function, make sure it is silent. Debug messages
> +  and messages printed after a successful detection are acceptable, but it
> +  must not print messages such as "Chip XXX not found/supported".
> +
> +  Keep in mind that the detect function will run for all drivers supporting an
> +  address if a chip is detected on that address. Unnecessary messages will just
> +  pollute the kernel log and not provide any value.
> +
> +* Avoid writing to chip registers in the _detect function. If you have to write,
> +  only do it after you have already gathered enough data to be certain that the
> +  detection is going to be successful.
> +  
> +  Keep in mind that the chip might not be what your driver believes it is, and
> +  writing to it might cause a bad misconfiguration.
> +
> +* Make sure there are no race conditions in the probe function. Specifically,
> +  completely initialize your chip first, then create sysfs entries and register
> +  with the hwmon subsystem.
> +
> +* Do not provide deprecated sysfs attributes.
> +
> +* Do not create non-standard attributes unless really needed. If you have to use
> +  non-standard attributes, or you believe you do, discuss it on the mailing list
> +  first. Either case, provide a detailed explanation why you need the
> +  non-standard attribute(s).
> +  Standard attributes are specified in Documentation/hwmon/sysfs-interface.

Other than this, looks good now.

-- 
Jean Delvare