Hi Theuns, Randy,
[Theuns Verwoerd]
> > AD7414 Temperature Sensor I2C driver. I2C chip driver for the Analog
> > Devices AD7414 device, exposes raw and decoded registers via sysfs.
> > Signed-off-by: Theuns Verwoerd <theuns.verwoerd@bluewatersys.com>
> > ---
> > Tested on a custom EP9315-based board developed in-house. Fairly trivial
> > driver; really just exposes the raw registers and temperature reading
> > to userspace.
> > Patch is relative to stock Linux-2.6.17.7
> > (...)
> > [This is my first go at a kernel submission, so feel free to point out
> > anything
> > that should be done differently]
> > ---
> > diff -uprN -X linux-2.6.17.7-vanilla/Documentation/dontdiff
> > linux-2.6.17.7-vanilla/drivers/i2c/chips/ad7414.c
> > linux-2.6.17.7/drivers/i2c/chips/ad7414.c
> > --- linux-2.6.17.7-vanilla/drivers/i2c/chips/ad7414.c 1970-01-01
> > 12:00:00.000000000 +1200
> > +++ linux-2.6.17.7/drivers/i2c/chips/ad7414.c 2006-08-04
> > 10:28:37.000000000 +1200
[Randy Dunlap]
> 1/ Thunderbird is breaking (splitting) your lines for you. :(
> See if http://mbligh.org/linuxdocs/Email/Clients/Thunderbird
> helps any. Try sending a patch to yourself and test if you
> can apply it. Or use a different mail client/app.
> Those 7 lines above should be 3 lines.
> You may need to unset some "flowed" attribute in tbird
> (I'm not sure of its full, correct name.)
>
> 2/ Either thunderbird is converting tabs to spaces or the
> driver source does not contain tabs -- just lots of spaces.
> Please indent struct fields with one tab, embedded struct fields
> with 2 tabs, etc.
>
> 3/ Macro continuation lines should be indented.
>
> 4/ sysfs files should contain only one value per file. Use
> multiple files for multiple values.
And please see Documentation/hwmon/sysfs-interface for proper file
names and units.
> 5/ Code labels should not be indented or maybe indented one space,
> and certainly not indented more than the following source lines.
>
> 6/ device_create_file() and friends can return errors. They need
> to be checked for success/fail.
>
> 7/ Header files should be included in linux/ alpha order, then
> asm/ alpha order, then local.h files unless there are reasons
> that this order won't work.
> (Yes, only linux/ applies here.)
The "alpha order" point is moot. I see nothing about that in
Documentation/CodingStyle, and also no rationale for it. Header files
should work no matter the order of inclusion. And I don't think we have
a single driver respecting that rule, do we?
I do agree with the linux/asm/local ordering rule, though, and I think
pretty much everyone does, as there is a rationale for it (local
defines shouldn't possibly affect standard header files.)
I second every other point Randy made, and add the following ones:
8* Theuns' driver belongs to drivers/hwmon, not drivers/i2c/chips.
9* The driver should be submitted for review to the lm-sensors list
(see MAINTAINERS) rather than the LKML.
Thanks,
--
Jean Delvare
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
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]