Re: [lm-sensors] possible error in ams-i2c.c

Re: [lm-sensors] possible error in ams-i2c.c

[Jean Delvare]

On Fri, 30 Mar 2007 21:41:37 +0200, Stelian Pop wrote:
> Fix sleep and retry logic in ams-i2c.
> 
> Signed-off-by: Stelian Pop <stelian@popies.net>
> ---
>  drivers/hwmon/ams/ams-i2c.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/ams/ams-i2c.c b/drivers/hwmon/ams/ams-i2c.c
> index 0d24bdf..6f5b423 100644
> --- a/drivers/hwmon/ams/ams-i2c.c
> +++ b/drivers/hwmon/ams/ams-i2c.c
> @@ -85,17 +85,17 @@ static int ams_i2c_write(u8 reg, u8 value)
>  static int ams_i2c_cmd(enum ams_i2c_cmd cmd)
>  {
>  	s32 result;
> -	int remaining = HZ / 20;
> +	int count = 3;
>  
>  	ams_i2c_write(AMS_COMMAND, cmd);
> -	mdelay(5);
> +	msleep(5);
>  
> -	while (remaining) {
> +	while (count--) {
>  		result = ams_i2c_read(AMS_COMMAND);
>  		if (result = 0 || result & 0x80)
>  			return 0;
>  
> -		remaining = schedule_timeout(remaining);
> +		schedule_timeout_uninterruptible(HZ / 20);
>  	}
>  
>  	return -1;

Applied, thanks.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] possible error in ams-i2c.c

[Stelian Pop]

Le vendredi 30 mars 2007 à 19:26 +0200, Jean Delvare a écrit :
[...]
> > > > Moreover, the loop logic is wrong: the ams_i2c_read should be
> > > > attempted a few times if it fails, with some delay between each
> > > > read. This is not what the code does...
[...]
> > -	int remaining = HZ / 20;
> > +	int count = 3, delay = HZ / 20;

> You don't really need a local variable for delay, you use it only
> once.

Well, the compiler would have optimized this away anyway.

> >  	ams_i2c_write(AMS_COMMAND, cmd);
> >  	mdelay(5);
> 
> Not related with your patch, but whouldn't msleep() be more friendly
> with the rest of the system? It doesn't look like you need the delay to
> be exactly 5 ms.

The protocol was reverse engineered so I don't know for sure if there is
a need for a fixed timing here. But you're probably correct.

[...]

> Otherwise I'm fine with this patch, but please post it on the
> lm-sensors list so that others can comment.

Sure, I'm adding the CC:, but please keep me in CC: in replies since I'm
not subscribed.

>  I'll pick it there. Is it something we want in 2.6.21?

No, there is no need to hurry, it can wait for 2.6.22.

Thanks,

---

Fix sleep and retry logic in ams-i2c.

Signed-off-by: Stelian Pop <stelian@popies.net>
---
 drivers/hwmon/ams/ams-i2c.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/ams/ams-i2c.c b/drivers/hwmon/ams/ams-i2c.c
index 0d24bdf..6f5b423 100644
--- a/drivers/hwmon/ams/ams-i2c.c
+++ b/drivers/hwmon/ams/ams-i2c.c
@@ -85,17 +85,17 @@ static int ams_i2c_write(u8 reg, u8 value)
 static int ams_i2c_cmd(enum ams_i2c_cmd cmd)
 {
 	s32 result;
-	int remaining = HZ / 20;
+	int count = 3;
 
 	ams_i2c_write(AMS_COMMAND, cmd);
-	mdelay(5);
+	msleep(5);
 
-	while (remaining) {
+	while (count--) {
 		result = ams_i2c_read(AMS_COMMAND);
 		if (result = 0 || result & 0x80)
 			return 0;
 
-		remaining = schedule_timeout(remaining);
+		schedule_timeout_uninterruptible(HZ / 20);
 	}
 
 	return -1;
-- 
1.5.0.3


-- 
Stelian Pop <stelian@popies.net>


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors