Re: [PATCH 001/001] I2C: AD7414 I2C chip driver for Linux-

From: Jean Delvare
Date: Sun Aug 06 2006 - 07:29:19 EST

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@xxxxxxxxxxxxxxxx>
> > ---
> > 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-
> > (...)
> > [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-
> > linux-
> > linux-
> > --- linux- 1970-01-01
> > 12:00:00.000000000 +1200
> > +++ linux- 2006-08-04
> > 10:28:37.000000000 +1200

[Randy Dunlap]
> 1/ Thunderbird is breaking (splitting) your lines for you. :(
> See if
> 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.

Jean Delvare
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at