On Saturday 03 May 2008, Jean Delvare wrote:
> > +enum lm75_type { /* keep sorted in alphabetical order */
> > + ds1775,
> > + ds75,
> > + LM75, /* UPPERCASE prevents INSMOD_1 conflict(!) */
>
> Huu, this is ugly. Not really your fault, of course, the
> I2C_CLIENT_INSMOD stuff is ugly and there's not much we can do at this
> point.
Right ...
> I'm not only worried by the use of uppercase, that's only a
> detail. What worries me more is the fact that a given chip type ends up
> being enumerated twice with different values, and a given type number
> corresponds to different chip types depending on which enum you look
> at. This has potential for future trouble. As this problem is likely to
> happen to all hybrid hardware monitoring drivers, I'd like to find a
> clean solution right now...
Fair enough.
> My proposal would be to only list in enum lm75_type, the types which
> are NOT declared by I2C_CLIENT_INSMOD. To prevent numbering conflict,
> you would start lm75_type at 9 (I2C_CLIENT_INSMOD_8 is the max.) See my
> proposed patch at the end of this post.
Looked OK to me, modulo a header comment (read infix below).
> > + lm75a,
> > + max6625,
> > + max6626,
> > + mcp980x,
> > + stds75,
> > + tcn75,
> > + tmp100,
> > + tmp101,
> > + tmp175,
> > + tmp275,
> > + tmp75,
> > +};
>
> Do you really need that many different types? I though that some of
> those parts were 100% compatible, at least as far as this driver is
> concerned. The more different types, the more tests you'll have to do
> at initialization time or even at run time.
ISTR most of them handle 12-bit encodings. And it turns
out that the probe() code has most need to know about the
differences: it's what will set up the state used by the rest
of the driver. (Albeit not in this particular patch...)
Re compatibility, I think explicit listings inside driver
code lead to a lot less confusion and time wasted on writing
needless drivers; and it also avoids wondering "is it *really*
compatible?". Plus it's easier to trust board init code that
matches the schematics (and other board docs).
> > /* Each client has this additional data */
> > struct lm75_data {
> > - struct i2c_client client;
> > + struct i2c_client *client;
> > + enum lm75_type type;
>
> You don't use the type field anywhere. If you use it in a later patch,
> then add it in that patch.
Fair enough. Your update doesn't remove it though...
> > struct device *hwmon_dev;
> > struct mutex update_lock;
> > + u8 orig_conf;
> > char valid; /* !=0 if registers are valid */
> > unsigned long last_updated; /* In jiffies */
> > - u16 temp[3]; /* Register values,
> > + s16 temp[3]; /* Register values,
>
> This too doesn't seem to belong to this patch any longer.
First time you commented on that, and that bugfix has been
in this patch for ages. :)
Yeah, that could stand to get pulled out and merged ASAP.
> > ...
> > + { "tmp75", tmp75, },
> > + { /* LIST END */ },
>
> Trailing comma not needed - you can't grow the list beyond its end.
Doesn't hurt either.
> > +};
> > +MODULE_DEVICE_TABLE(i2c, lm75_ids);
> > (...)
>
> Proposed patch:
ACK, modulo the comment below:
> ---
> drivers/hwmon/lm75.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> --- linux-2.6.26-rc0.orig/drivers/hwmon/lm75.c 2008-05-02 22:46:01.000000000 +0200
> +++ linux-2.6.26-rc0/drivers/hwmon/lm75.c 2008-05-03 10:01:15.000000000 +0200
> @@ -34,12 +34,14 @@
> * This driver handles the LM75 and compatible digital temperature sensors.
> * Compatibles include at least the DS75, DS1775, MCP980x, STDS75, TCN75,
> * TMP100, TMP101, TMP75, TMP175, and TMP275.
> + * Only types which are _not_ part of I2C_CLIENT_INSMOD below, need to be
> + * enumerated here. We start at 9 because I2C_CLIENT_INSMOD can be used to
> + * define up to 8 chip types.
> */
Better to also strike the "Compatibles include..." sentence;
it's already out of sync.
> enum lm75_type { /* keep sorted in alphabetical order */
> - ds1775,
> + ds1775 = 9,
> ds75,
> - LM75, /* UPPERCASE prevents INSMOD_1 conflict(!) */
> lm75a,
> max6625,
> max6626,
> @@ -72,7 +74,7 @@ static const u8 LM75_REG_TEMP[3] = {
> /* Each client has this additional data */
> struct lm75_data {
> struct i2c_client *client;
> - enum lm75_type type;
> + unsigned int type;
> struct device *hwmon_dev;
> struct mutex update_lock;
> u8 orig_conf;
> @@ -220,7 +222,7 @@ static int lm75_remove(struct i2c_client
> static const struct i2c_device_id lm75_ids[] = {
> { "ds1775", ds1775, },
> { "ds75", ds75, },
> - { "lm75", LM75, },
> + { "lm75", lm75, },
> { "lm75a", lm75a, },
> { "max6625", max6625, },
> { "max6626", max6626, },
>
> The advantages are: case consistency, each chip type has a single type
> number, and it is possible and easy to move types from the
> new-style-specific enum to the common one if needed. What do you think?
I think adding new chip support to legacy driver side
should be avoided. ;)
- Dave
