LM87: Add code to retry reads on error
PATCH 1/1 LM87: Add code to retry reads on error
This patch adds code to retry if there are any read errors. This code has been copied almost verbatim from the LM93 driver (Thanks, Mark!).
Just as in the LM93, it's not clear what the right thing would be to do if the retries all fail, so I return 0, just as the LM93 does.
David
kernel/chips/lm87.c renamed from kernel/chips/lm87.c.orig +21 -3
@@ -27,6 +27,7 @@#include <linux/i2c.h> #include <linux/i2c-proc.h> #include <linux/init.h> +#include <linux/delay.h> #include "version.h" #include "sensors_vid.h"
@@ -487,7 +488,7 @@if ((err =i2c_detach_client (client))) {printk - ("lm87.o: Client deregistration failed, client not detached.\n" ); + (KERN_ERR "lm87.o: Client deregistration failed, client not detached.\n" );return err; }
@@ -496,9 +497,26 @@return 0 ; } +#define MAX_RETRIES 5 +static int lm87_read_value (struct i2c_client *client,u8 reg) { -return 0xFF &i2c_smbus_read_byte_data (client, reg); +int value, i; + +/* retry in case of read errors */ +for (i=1 ; i<=MAX_RETRIES ; i++) { +if ((value =i2c_smbus_read_byte_data (client, reg)) >=0 ) { +return value; + }else { +printk (KERN_WARNING "lm87.o: read byte data failed, " +"address 0x%02x.\n" , reg); +mdelay (i); + } + + } + +/* <TODO> what to return in case of error? */ +return 0 ; }static int lm87_write_value (struct i2c_client *client,u8 reg,u8 value)
@@ -964,7 +982,7 @@static int __init sm_lm87_init (void ) { -printk ("lm87.o version %s (%s)\n" ,LM_VERSION ,LM_DATE ); +printk (KERN_INFO "lm87.o version %s (%s)\n" ,LM_VERSION ,LM_DATE );return i2c_add_driver (&LM87_driver); }
Jean Delvare
AppliedPATCH 1/1 LM87: Add code to retry reads on error
Hi David,
Quoted 3 lines
This patch adds code to retry if there are any read errors. This
code has been copied almost verbatim from the LM93 driver (Thanks, Mark!).
Thanks, applied (with some edition).
I suspect Mark copied it from my w83l785ts driver ;) Open source code rules.
BTW this may make us think of integrating the mechanism into the i2c layer itself. If more chips and hardware setups need a similar function, it may be preferable that we avoid duplicating code.
Quoted 2 lines
Just as in the LM93, it's not clear what the right thing would be to do if the retries all fail, so I return 0, just as the LM93 does.
In the w83l785ts driver I do return the last know value for that register, passed as an additional parameter by the caller. It doesn't help for the first read, of course, and tends to hide errors, but is IMHO better than arbitrarily returning 0.
There's something strange in your patch, methinks. You issue warnings on read errors, but you don't actually issue an error message if you are not able to recover from the successive errors. You may provide a patch against CVS that fixes that and I'll apply it.
You'll see I have commited my own changes to the lm87 driver too. They mostly fix brokenesses in macros, which make the driver handle negative temperatures correctly, and, more interestingly, make it round voltages properly.
I plan to commit more changes soon. In particular, the init step is totally weird. Reseting the chip makes it forget the way the BIOS had configured it. The lm87 driver also has the drawback that you need to change a couple defines to get it to work according to your physical setup. When I ported the driver to Linux 2.6, I changed that and had the driver read the configuration from the chip instead. On my board, the BIOS configures it as it should, so it works really great.
Are you in a similar case? Did you have to change the defines before compiling the driver? Did you ever try to dump the chips contents before loading the lm87 driver and check if the BIOS had configured it for you? The main configuration register is at 0x16; if it doesn't read 0x00 at boot time, then it means that your BIOS changed it. It reads 0x04 for me, because the chip is wired for 4 voltages and 3 temperatures (as opposed to the default 6 voltages and 2 temperatures).
Thanks.
Signature · 2 lines
Jean Delvare http://khali.linux-fr.org/
PATCH 1/1 LM87: Add code to retry reads on error
In reply to David KnierimQuoted 8 lines · Jean Delvare
Hi David,
Quoted 3 lines
This patch adds code to retry if there are any read errors. This
code has been copied almost verbatim from the LM93 driver (Thanks, Mark!).
Quoted 4 lines
Thanks, applied (with some edition).
I suspect Mark copied it from my w83l785ts driver ;) Open source code rules.
:^)
Quoted 4 lines
BTW this may make us think of integrating the mechanism into the i2c layer itself. If more chips and hardware setups need a similar function, it may be preferable that we avoid duplicating code.
For us, we need the functionality. I agree that it probably belongs in
the i2c layer, but this works (and was amazingly easy to implement).
Quoted 10 lines
Quoted 4 lines
Just as in the LM93, it's not clear what the right thing would be to do if the retries all fail, so I return 0, just as the LM93 does.
In the w83l785ts driver I do return the last know value for that register, passed as an additional parameter by the caller. It doesn't help for the first read, of course, and tends to hide errors, but is IMHO better than arbitrarily returning 0.
I see what you did. I'm not a C coder, so I'm not sure I can pull it off. I will see what I can do.
Quoted 5 lines
There's something strange in your patch, methinks. You issue warnings on read errors, but you don't actually issue an error message if you are not able to recover from the successive errors. You may provide a patch against CVS that fixes that and I'll apply it.
Yeah, I thought of that after I sent the patch off. The patch I have
been using logs when the maximum number of retries has been exhausted. I held back from sending a patch to my patch until I got some feedback :^)
Quoted 5 lines
You'll see I have commited my own changes to the lm87 driver too. They mostly fix brokenesses in macros, which make the driver handle negative temperatures correctly, and, more interestingly, make it round voltages properly.
Sounds like good stuff. I hadn't noticed any problems, but that doesn't mean they weren't there.
Quoted 11 lines
I plan to commit more changes soon. In particular, the init step is totally weird. Reseting the chip makes it forget the way the BIOS had configured it. The lm87 driver also has the drawback that you need to change a couple defines to get it to work according to your physical setup. When I ported the driver to Linux 2.6, I changed that and had the driver read the configuration from the chip instead. On my board, the BIOS configures it as it should, so it works really great.
Are you in a similar case? Did you have to change the defines before compiling the driver?
Yes, I have the same case. We have two products that use the LM87 (and
another on the way). One uses one configuration, and the other uses the second configuration. At this time, I have a total hack in the spec file that makes a copy of the lm87.c file, patches it for the additional temperature input and adds and entry to the make file to build it when
the rest of the modules are built. Ugly, but since I don't code in C
(yet), doing this was something well within my skills (and it does work fine, as long as you remember to load the right driver version).
Quoted 2 lines
Did you ever try to dump the chips contents before
loading the lm87 driver and check if the BIOS had configured it for you?
I haven't tried this, so I really don't know. I expect it be correct on one board. The second board is still in development and has more BIOS bugs than you can count, so I'm pretty sure setting up the sensor chips isn't done correctly yet.
Quoted 4 lines
The main configuration register is at
0x16; if it doesn't read0x00at boot time, then it means that your BIOS changed it. It reads0x04for me, because the chip is wired for 4 voltages and 3 temperatures (as opposed to the default 6 voltages and 2 temperatures).
I'll give a peek and see what I can see.
PATCH 1/1 LM87: Add code to retry reads on error
In reply to David KnierimQuoted 8 lines
Quoted 5 lines
In the w83l785ts driver I do return the last know value for that register, passed as an additional parameter by the caller. It doesn't help for the first read, of course, and tends to hide errors, but is IMHO better than arbitrarily returning 0.
I see what you did. I'm not a C coder, so I'm not sure I can pull it off. I will see what I can do.
My method needs more changes to the code since it alters the function prototype. If you don't feel a need for it in your case, let's not change anything.
Quoted 8 lines
Quoted 4 lines
There's something strange in your patch, methinks. You issue warnings on read errors, but you don't actually issue an error message if you are not able to recover from the successive errors. You may provide a patch against CVS that fixes that and I'll apply it.
Yeah, I thought of that after I sent the patch off. The patch I have
been using logs when the maximum number of retries has been exhausted. I held back from sending a patch to my patch until I got some feedback :^)
And does it actually happen? The delay between retries may be experimented with. I came with a very simple model but other approaches could lead to better results in some cases. Feel free to alter the delays and see if it helps.
Quoted 7 lines
Quoted 4 lines
You'll see I have commited my own changes to the lm87 driver too. They mostly fix brokenesses in macros, which make the driver handle negative temperatures correctly, and, more interestingly, make it round voltages properly.
Sounds like good stuff. I hadn't noticed any problems, but that doesn't mean they weren't there.
The negative temperature problem was obviously not important, I wonder who needs this. The voltage rounding issues were rather clean though. If you set limits in /etc/sensors.conf, "sensors -s && sensors" would yield different values, off by a few mV each time. For example, my VCore limits are 1.7V +/- 5%. They were reading (IIRC) 1.56V-1.77V, and after the fixes read 1.60V-1.79V, which is nearer from what I asked for.
Quoted 8 lines
Yes, I have the same case. We have two products that use the LM87 (and
another on the way). One uses one configuration, and the other uses the second configuration. At this time, I have a total hack in the spec file that makes a copy of thelm87.cfile, patches it for the additional temperature input and adds and entry to the make file to build it when
the rest of the modules are built. Ugly, but since I don't code in C
(yet), doing this was something well within my skills (and it does work fine, as long as you remember to load the right driver version).
I suspect that you can simply set CFLAGS to -DLM87_EXT2 to alter the output. It might be easier than to copy/patch the file.
Quoted 4 lines
I haven't tried this, so I really don't know. I expect it be correct on one board. The second board is still in development and has more BIOS bugs than you can count, so I'm pretty sure setting up the sensor chips isn't done correctly yet.
Do you suggest that you have a new product using LM87 chips? I thought these chips were pretty old and not used for new designs anymore.
Thanks,
Jean Delvare
PATCH 1/1 LM87: Add code to retry reads on error
In reply to David KnierimQuoted 5 lines · Jean Delvare
Quoted 5 lines
Quoted 4 lines
In the w83l785ts driver I do return the last know value for that register, passed as an additional parameter by the caller. It doesn't help for the first read, of course, and tends to hide errors, but is IMHO better than arbitrarily returning 0.
Quoted 7 lines
Quoted 2 lines
I see what you did. I'm not a C coder, so I'm not sure I can pull it off. I will see what I can do.
My method needs more changes to the code since it alters the function prototype. If you don't feel a need for it in your case, let's not change anything.
Well, it works fine the way it is. I would like to enhance it, but I
have a lot of other stuff that are higher priority...
Quoted 12 lines
Quoted 10 lines
Quoted 6 lines
There's something strange in your patch, methinks. You issue warnings on read errors, but you don't actually issue an error message if you are not able to recover from the successive errors. You may provide a patch against CVS that fixes that and I'll apply it.
Yeah, I thought of that after I sent the patch off. The patch I have
been using logs when the maximum number of retries has been exhausted. I held back from sending a patch to my patch until I got some feedback :^)
Quoted 4 lines
And does it actually happen? The delay between retries may be experimented with. I came with a very simple model but other approaches could lead to better results in some cases. Feel free to alter the delays and see if it helps.
I have only ever seen the maximum number of retries reached once, and that was under extreme system stress. In fact, all other times that I have seen, exactly 3 retries required if there is an error. I'll try a bit longer wait and see what happens.
Quoted 5 lines
Do you suggest that you have a new product using LM87 chips? I thought these chips were pretty old and not used for new designs anymore.
Yes, we have new products that are using the LM87. I don't select the hardware, so I don't know much about how that particular chip was selected. They do check for single sources and get input from the vendor concerning predicted lifespan, so they must feel it will be available for a long time (we build telco products with a minimum 5 year support life).
David Knierim