lm75: add new-style driver binding
PATCH 1/1 lm75: add new-style driver binding
Quoted 3 lines · Jean Delvare
Alternatively, you could just move the addition of the min and max settings the next patch in the series, as I can't really see how they are related to converting the driver to new-style i2c
Done. This goes on top of
http://marc.info/?l=lm-sensors&m0880547009929&w=2
and *CURRENT* git (with updates to driver modle style binding).
IMO these two patches should go into the hwmon queue ASAP (first one seems overdue for merge mainstream, FWIW).
==== CUT HERE
More LM75 updates:
- Teach the LM75 driver to use new-style driver binding:
* Create a second driver struct, using new-style driver binding methods cribbed from the legacy code.
* Add a MODULE_DEVICE_TABLE (for "newER-style binding")
* The legacy probe logic delegates its work to this new code.
* The legacy driver uses the name "lm75_legacy".
- Sensor range bugfix: values are signed, not unsigned.
- More careful initialization. Chips are put into 9-bit mode so the current interconversion routines will never fail.
- Save the original chip configuration, and restore it on exit.
So the new-style code should catch all chips that boards declare, while the legacy code catches others. This particular coexistence strategy may need some work yet ... legacy modes might best be set up explicitly by some tool not unlike "sensors-detect". (Or else completely eradicated...)
---
UPDATED: applies against update first patch. Support for 12-bit conversions moves into a later patch.
drivers/hwmon/Kconfig | 7 + drivers/hwmon/lm75.c | 204 ++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 164 insertions(+), 47 deletions(-)
drivers/hwmon/Kconfig +6 -1
@@ -392,7 +392,12 @@ config SENSORS_LM75 - TelCom (now Microchip) TCN75 - Texas Instruments TMP100, TMP101, TMP75, TMP175, TMP275 - Most of these chips will require a "force" module parameter. + This driver supports driver model based binding through board + specific I2C device tables. + + It also supports the "legacy" style of driver binding. To use + that with some chips which don't replicate LM75 quirks exactly, + you may need the "force" module parameter. This driver can also be built as a module. If so, the module will be called lm75.
drivers/hwmon/lm75.c +158 -46
296 lines not on this page · raw message
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [patch 2.6.25-git] lm75: add new-style driver
Hi David,
Quoted 5 lines · David Brownell
Quoted 3 lines · Jean Delvare
Alternatively, you could just move the addition of the min and max settings the next patch in the series, as I can't really see how they are related to converting the driver to new-style i2c
Quoted 8 lines
Done. This goes on top of
http://marc.info/?l=lm-sensors&m0880547009929&w=2and *CURRENT* git (with updates to driver modle style binding).
IMO these two patches should go into the hwmon queue ASAP (first one seems overdue for merge mainstream, FWIW).
Agreed. But I still would like to suggest one more update to this second patch:
Quoted 41 lines
==== CUT HERE
More LM75 updates:- Teach the LM75 driver to use new-style driver binding:
* Create a second driver struct, using new-style driver binding methods cribbed from the legacy code.
* Add a
MODULE_DEVICE_TABLE(for "newER-style binding")* The legacy probe logic delegates its work to this new code.
* The legacy driver uses the name "
lm75_legacy".- Sensor range bugfix: values are signed, not unsigned.
- More careful initialization. Chips are put into 9-bit mode so the current interconversion routines will never fail.
- Save the original chip configuration, and restore it on exit.
So the new-style code should catch all chips that boards declare, while the legacy code catches others. This particular coexistence strategy may need some work yet ... legacy modes might best be set up explicitly by some tool not unlike "sensors-detect". (Or else completely eradicated...)
Signed-off-by David Brownell <[email protected]>---
UPDATED: applies against update first patch. Support for 12-bit conversions moves into a later patch.drivers/hwmon/Kconfig | 7 + drivers/hwmon/lm75.c | 204 ++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 164 insertions(+), 47 deletions(-)(...)
drivers/hwmon/lm75.c line 42
Quoted 7 lines
drivers/hwmon/lm75.c +4
-0@@ -36,6 +36,23 @@* TMP100, TMP101, TMP75, TMP175, and TMP275. */ +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. 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...
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.
drivers/hwmon/lm75.c line 54
Quoted 12 lines
drivers/hwmon/lm75.c +12
-0+ 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.
drivers/hwmon/lm75.c static const u8 LM75_REG_TEMP[3] = { · line 75
Quoted 11 lines
drivers/hwmon/lm75.c +3
-1+/* Addresses scanned by legacy style driver binding */ static const unsigned short normal_i2c[] = {0x48 ,0x49 ,0x4a ,0x4b ,0x4c ,0x4d ,0x4e ,0x4f ,I2C_CLIENT_END };@@ -54,18 +71,19 @@ static const u8 LM75_REG_TEMP[3] = {/* 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.
drivers/hwmon/lm75.c static const u8 LM75_REG_TEMP[3] = { · line 81
Quoted 7 lines
drivers/hwmon/lm75.c +2
-1struct 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.
Quoted 12 lines
drivers/hwmon/lm75.c +6
-00 = input 1 = max 2 = hyst */ }; +static const struct i2c_device_id lm75_ids[] = { + {"ds1775" , ds1775, }, + {"ds75" , ds75, }, + {"lm75" ,LM75 , }, + {"lm75a" , lm75a, }, + {"max6625" , max6625, },
drivers/hwmon/lm75.c static const u8 LM75_REG_TEMP[3] = {
Quoted 10 lines
drivers/hwmon/lm75.c +10
-0+ { "max6626", max6626, }, + { "mcp980x", mcp980x, }, + { "stds75", stds75, }, + { "tcn75", tcn75, }, + { "tmp100", tmp100, }, + { "tmp101", tmp101, }, + { "tmp175", tmp175, }, + { "tmp275", tmp275, }, + { "tmp75", tmp75, }, + { /* LIST END */ },
Trailing comma not needed - you can't grow the list beyond its end.
drivers/hwmon/lm75.c
Quoted 3 lines
drivers/hwmon/lm75.c +2
-0+}; +MODULE_DEVICE_TABLE (i2c, lm75_ids);
Proposed patch:
--- drivers/hwmon/lm75.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/hwmon/lm75.c +6 -4
@@ -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. */ 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_clientstatic 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?
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [patch 2.6.25-git] lm75: add new-style driver
In reply to David BrownellQuoted 8 lines · Jean Delvare
drivers/hwmon/lm75.c line 42
Quoted 4 lines
drivers/hwmon/lm75.c +4
-0+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_INSMODstuff is ugly and there's not much we can do at this point.
Right ...
Quoted 7 lines
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.
Quoted 4 lines
My proposal would be to only list in enum
lm75_type, the types which are NOT declared byI2C_CLIENT_INSMOD. To prevent numbering conflict, you would startlm75_typeat 9 (I2C_CLIENT_INSMOD_8is the max.) See my proposed patch at the end of this post.
Looked OK to me, modulo a header comment (read infix below).
Quoted 9 lines
Quoted 9 lines
drivers/hwmon/lm75.c +9
-0+ lm75a, + max6625, + max6626, + mcp980x, + stds75, + tcn75, + tmp100, + tmp101, + tmp175,
Quoted 8 lines
drivers/hwmon/lm75.c line 54
Quoted 3 lines
drivers/hwmon/lm75.c +3
-0+ 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).
Quoted 8 lines
drivers/hwmon/lm75.c static const u8 LM75_REG_TEMP[3] = { · line 75
Quoted 5 lines
/* Each client has this additional data */
structlm75_data{drivers/hwmon/lm75.c +2
-1-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...
Quoted 9 lines
drivers/hwmon/lm75.c static const u8 LM75_REG_TEMP[3] = { · line 81
Quoted 7 lines
drivers/hwmon/lm75.c +2
-1struct 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.
Quoted 5 lines
drivers/hwmon/lm75.c static const struct attribute_group lm75 · line 235
Quoted 3 lines
drivers/hwmon/lm75.c +2
-0+ { "tmp75", tmp75, }, + { /* LIST END */ },Trailing comma not needed - you can't grow the list beyond its end.
Doesn't hurt either.
Quoted 5 lines
drivers/hwmon/lm75.c static const struct attribute_group lm75 · line 237
Quoted 3 lines
drivers/hwmon/lm75.c +2
-0+}; +MODULE_DEVICE_TABLE (i2c, lm75_ids);Proposed patch:
ACK, modulo the comment below:
Quoted 7 lines
--- drivers/hwmon/lm75.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/hwmon/lm75.c line 40
Quoted 7 lines
drivers/hwmon/lm75.c +3
-0@@ -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.
Quoted 27 lines
drivers/hwmon/lm75.c +3
-4enum 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_clientstatic const struct i2c_device_id lm75_ids[] = { {"ds1775" , ds1775, }, {"ds75" , ds75, }, - {"lm75" ,LM75 , }, + {"lm75" , lm75, }, {"lm75a" , lm75a, }, {"max6625" , max6625, }, {"max6626" , max6626, },
Quoted 3 lines
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
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [patch 2.6.25-git] lm75: add new-style driver
In reply to David BrownellHi David,
Quoted 28 lines · David Brownell
Quoted 13 lines · Jean Delvare
Quoted 13 lines
drivers/hwmon/lm75.c +13
-0+enum lm75_type {/* keep sorted in alphabetical order */ + ds1775, + ds75, +LM75 ,/* UPPERCASE prevents INSMOD_1 conflict(!) */ + lm75a, + max6625, + max6626, + mcp980x, + stds75, + tcn75, + tmp100, + tmp101, + tmp175,Quoted 8 lines
drivers/hwmon/lm75.c line 54
Quoted 3 lines
drivers/hwmon/lm75.c +3
-0+ 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...)
Quoted 5 lines
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).
I didn't mean to object to having that many device names, only that many different enum values. When 2 parts are 100% compatible, I think it makes sense to have the two names map to the same enum value in lm75_ids, so that later code can only check for one enum value instead of 2.
But well it's up to you, that was only a suggestion, I don't care that much.
Quoted 7 lines
Quoted 4 lines
drivers/hwmon/lm75.c static const u8 LM75_REG_TEMP[3] = { · line 81
Quoted 2 lines
drivers/hwmon/lm75.c +1
-1-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. :)
Up to the previous version, the patch included min/max stuff and wrappers for the conversions, so it made some sense to include this change along. Now it no longer does.
Quoted 6 lines
Quoted 4 lines
drivers/hwmon/lm75.c static const struct attribute_group lm75 · line 235
Quoted 2 lines
drivers/hwmon/lm75.c +2
-0+ { "tmp75", tmp75, }, + { /* LIST END */ },Trailing comma not needed - you can't grow the list beyond its end.
Doesn't hurt either.
Well, I think it does. A trailing comma is an invitation to add new lines after it. The big fat comment you inserted somewhat mitigates this, but still.
Quoted 2 lines
I think adding new chip support to legacy driver side should be avoided. ;)
So do I, but as long as the new-style drivers do not offer a way to forcibly instantiate a device from user-space, we might have to do that.
Signature · 6 lines
Jean Delvare _______________________________________________ lm-sensors mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/lm-sensors
Re: [lm-sensors] [patch 2.6.25-git] lm75: add new-style driver
In reply to David BrownellWhat's the merge status of these LM75 patches?
Pending for around a year now, acked, ...
I notice a boatload of hwmon driver updates have
jumped past these in the queue.
I'm going to resend these, but this is the last
time I'm going to do that.
- Dave
_______________________________________________
lm-sensors mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors