[lm-sensors] [patch 00/36] New w83795 driver

[lm-sensors] [patch 00/36] New w83795 driver

[Jean Delvare]

This is a new driver for the Winbond/Nuvoton W83795G/ADG hardware
monitoring chips. The original code was contributed by Wei Song, a
former employee of Nuvoton. I've fixed and improved a lot of things
on top of his work.

-- 
Jean Delvare

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

Re: [lm-sensors] [patch 00/36] New w83795 driver

[Henrique de Moraes Holschuh]

On Wed, 15 Sep 2010, Jean Delvare wrote:
> This is a new driver for the Winbond/Nuvoton W83795G/ADG hardware
> monitoring chips. The original code was contributed by Wei Song, a
> former employee of Nuvoton. I've fixed and improved a lot of things
> on top of his work.

You've gotten yourself a tester: I have one of those in my main desktop
(SuperMicro C7X58 v1.1).  I just have to backport the driver to 2.6.32.y
to test it there.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

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

Re: [lm-sensors] [patch 00/36] New w83795 driver

[Jean Delvare]

Hi Henrique,

On Wed, 15 Sep 2010 12:52:06 -0300, Henrique de Moraes Holschuh wrote:
> On Wed, 15 Sep 2010, Jean Delvare wrote:
> > This is a new driver for the Winbond/Nuvoton W83795G/ADG hardware
> > monitoring chips. The original code was contributed by Wei Song, a
> > former employee of Nuvoton. I've fixed and improved a lot of things
> > on top of his work.
> 
> You've gotten yourself a tester: I have one of those in my main desktop
> (SuperMicro C7X58 v1.1).  I just have to backport the driver to 2.6.32.y
> to test it there.

You don't even have to, as I did it for you already:
http://khali.linux-fr.org/devel/misc/w83795/

If you find any problem with the driver, please let me know!

(Note: monitoring should work OK now, but I didn't have the time to
work on fan speed control yet, so I expect bugs in this area.)

-- 
Jean Delvare
http://khali.linux-fr.org/wishlist.html

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

Re: [lm-sensors] [patch 00/36] New w83795 driver

[Henrique de Moraes Holschuh]

On Wed, 15 Sep 2010, Jean Delvare wrote:
> On Wed, 15 Sep 2010 12:52:06 -0300, Henrique de Moraes Holschuh wrote:
> > On Wed, 15 Sep 2010, Jean Delvare wrote:
> > > This is a new driver for the Winbond/Nuvoton W83795G/ADG hardware
> > > monitoring chips. The original code was contributed by Wei Song, a
> > > former employee of Nuvoton. I've fixed and improved a lot of things
> > > on top of his work.
> > 
> > You've gotten yourself a tester: I have one of those in my main desktop
> > (SuperMicro C7X58 v1.1).  I just have to backport the driver to 2.6.32.y
> > to test it there.
> 
> You don't even have to, as I did it for you already:
> http://khali.linux-fr.org/devel/misc/w83795/

Thanks.  Will use.

> If you find any problem with the driver, please let me know!

I will even try to send patches :-)

> (Note: monitoring should work OK now, but I didn't have the time to
> work on fan speed control yet, so I expect bugs in this area.)

I can try to compare the montoring results with the userspace driver
included in SuperODoctor II.  The first version of the driver (before
your patches) had inconsistentices.

I will have to read the chip's datasheet to check if my hardware setup
will let me help test PWM control.

The C7X58/S8SAX BIOS is supposed to put the chip in some sort of
closed-loop fan control mode if you tell it to, but there are issues
when mixing 3-pin and 4-pin fans in the same system (or at least
SuperMicro cautions agains such a setup.  It might be just a lazy BIOS,
or it might be a chip restriction)... and I have a mix of 3-pin and
4-pin fans.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

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

Re: [lm-sensors] [patch 00/36] New w83795 driver

[Jean Delvare]

On Wed, 15 Sep 2010 13:07:40 -0300, Henrique de Moraes Holschuh wrote:
> On Wed, 15 Sep 2010, Jean Delvare wrote:
> > On Wed, 15 Sep 2010 12:52:06 -0300, Henrique de Moraes Holschuh wrote:
> > > You've gotten yourself a tester: I have one of those in my main desktop
> > > (SuperMicro C7X58 v1.1).  I just have to backport the driver to 2.6.32.y
> > > to test it there.
> > 
> > You don't even have to, as I did it for you already:
> > http://khali.linux-fr.org/devel/misc/w83795/
> 
> Thanks.  Will use.
> 
> > If you find any problem with the driver, please let me know!
> 
> I will even try to send patches :-)

Thanks :)

> > (Note: monitoring should work OK now, but I didn't have the time to
> > work on fan speed control yet, so I expect bugs in this area.)
> 
> I can try to compare the montoring results with the userspace driver
> included in SuperODoctor II.  The first version of the driver (before
> your patches) had inconsistentices.
> 
> I will have to read the chip's datasheet to check if my hardware setup
> will let me help test PWM control.
> 
> The C7X58/S8SAX BIOS is supposed to put the chip in some sort of
> closed-loop fan control mode if you tell it to, but there are issues
> when mixing 3-pin and 4-pin fans in the same system (or at least
> SuperMicro cautions agains such a setup.  It might be just a lazy BIOS,
> or it might be a chip restriction)... and I have a mix of 3-pin and
> 4-pin fans.

I have similar restrictions on my Asus Z8NA-D6 board. Both CPU fans
have to be either 4-pin or 3-pin for control to work, they can't be
mixed. Same goes for case fans, they have to be all 3-pin or all 4-pin,
otherwise control doesn't work. There are jumpers on the board to
switch between 3-pin and 4-pin setups.

-- 
Jean Delvare

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

Re: [lm-sensors] [patch 00/36] New w83795 driver

[Guenter Roeck]

On Wed, Sep 15, 2010 at 09:40:05AM -0400, Jean Delvare wrote:
> This is a new driver for the Winbond/Nuvoton W83795G/ADG hardware
> monitoring chips. The original code was contributed by Wei Song, a
> former employee of Nuvoton. I've fixed and improved a lot of things
> on top of his work.
> 
Hi Jean,

highly unusual, I know, but I integrated all individual patches of this set
and then added my coments into the resulting file.
The result is a patch file with my comments.

I might be able to spend some more time on it, but this is what I have so far.
Hope it is useful.

Mostly nitpicks and suggested simplifications, but there may be one
real bug (see the comment about lsb).

Guenter

--
diff --git a/drivers/hwmon/w83795.c b/drivers/hwmon/w83795.c
index 76c7f24..e19e6c9 100644
--- a/drivers/hwmon/w83795.c
+++ b/drivers/hwmon/w83795.c
@@ -124,6 +124,14 @@ static const u8 W83795_REG_IN_HL_LSB[] = {
 	0xa4,	/* VSEN17 */
 };
 
+/*
+ * type is either 1 or 2.
+ * 1: index := index --> could use index + type - 1
+ * 2: index := index + 1 --> could use index + type - 1
+ * Suggestion:
+ *  #define IN_LSB_REG(index, type) W83795_REG_IN_HL_LSB[(index + type - 1)]
+ * or at least use type = IN_MAX instead of type = 1.
+ */
 #define IN_LSB_REG(index, type) \
 	(((type) = 1) ? W83795_REG_IN_HL_LSB[(index)] \
 	: (W83795_REG_IN_HL_LSB[(index)] + 1))
@@ -161,6 +169,7 @@ static const u8 IN_LSB_SHIFT_IDX[][2] = {
 #define W83795_REG_FAN_MIN_LSB(index)	(0xC4 + (index) / 2)
 #define W83795_REG_FAN_MIN_LSB_SHIFT(index) \
 	(((index) & 1) ? 4 : 0)
+/* could use ((index) & 1) << 2) */
 
 #define W83795_REG_VID_CTRL		0x6A
 
@@ -349,6 +358,9 @@ struct w83795_data {
 	u8 dts_read_vrlsb[8];	/* Register value */
 	s8 dts_ext[4];		/* Register value */
 
+	/* something like pwm_count would be better here. 
+	 * All other has_xxx variables are bit masks.
+	 */
 	u8 has_pwm;		/* 795g supports 8 pwm, 795adg only supports 2,
 				 * no config register, only affected by chip
 				 * type */
@@ -470,6 +482,12 @@ static void w83795_update_limits(struct i2c_client *client)
 
 		/* Each register contains LSB for 2 fans, but we want to
 		 * read it only once to save time */
+		/*
+		 * But that causes lsb to be undefined across loop instances,
+		 * doesn't it, since it is only defined inside the loop ?
+		 * And what happens if an even fan does not exist
+		 * but the odd one does ?
+		 */
 		if ((i & 1) = 0 && (data->has_fan & (3 << i)))
 			lsb = w83795_read(client, W83795_REG_FAN_MIN_LSB(i));
 
@@ -491,6 +509,7 @@ static void w83795_update_limits(struct i2c_client *client)
 	}
 
 	/* Read the DTS limits */
+	/* != 0 isn't really needed */
 	if (data->enable_dts != 0) {
 		for (limit = DTS_CRIT; limit <= DTS_WARN_HYST; limit++)
 			data->dts_ext[limit] @@ -544,6 +563,7 @@ static struct w83795_data *w83795_update_pwm_config(struct device *dev)
 		data->pwm_temp[i][TEMP_PWM_CTFS]  			w83795_read(client, W83795_REG_CTFS(i));
 		tmp = w83795_read(client, W83795_REG_HT(i));
+		/* & 0x0f doesn't hurt but also isn't needed. */
 		data->pwm_temp[i][TEMP_PWM_HCT] = (tmp >> 4) & 0x0f;
 		data->pwm_temp[i][TEMP_PWM_HOT] = tmp & 0x0f;
 	}
@@ -620,6 +640,7 @@ static struct w83795_data *w83795_update_device(struct device *dev)
 		data->fan[i] = w83795_read(client, W83795_REG_FAN(i)) << 4;
 		data->fan[i] | 		  (w83795_read(client, W83795_REG_VRLSB) >> 4) & 0x0F;
+		    /* & 0x0f doesn't hurt but isn't needed either. */
 	}
 
 	/* Update temperature */
@@ -631,6 +652,7 @@ static struct w83795_data *w83795_update_device(struct device *dev)
 	}
 
 	/* Update dts temperature */
+	/* != 0 isn't really needed */
 	if (data->enable_dts != 0) {
 		for (i = 0; i < ARRAY_SIZE(data->dts); i++) {
 			if (!(data->has_dts & (1 << i)))
@@ -677,10 +699,11 @@ show_alarm_beep(struct device *dev, struct device_attribute *attr, char *buf)
 	int bit = sensor_attr->index & 0x07;
 	u8 val;
 
-	if (ALARM_STATUS = nr) {
-		val = (data->alarms[index] >> (bit)) & 1;
+	/* nr = ALARM_STATUS ? */
+	if (ALARM_STATUS = nr) {			    /* { } not needed ? */
+		val = (data->alarms[index] >> (bit)) & 1; /* (bit) --> bit */
 	} else {		/* BEEP_ENABLE */
-		val = (data->beeps[index] >> (bit)) & 1;
+		val = (data->beeps[index] >> (bit)) & 1; /* (bit) --> bit */
 	}
 
 	return sprintf(buf, "%u\n", val);
@@ -744,6 +767,18 @@ show_fan(struct device *dev, struct device_attribute *attr, char *buf)
 	struct w83795_data *data = w83795_update_device(dev);
 	u16 val;
 
+	/* Those reversed comparisons always confuse me.
+	 * I think the compiler should refuse to compile 
+	 * such code to make me happy. And, no, I don't buy
+	 * the argument that the compiler magically produces
+	 * better code this way.
+	 * They are used in this driver to compare variables against
+	 * defined constants (though not all the time).
+	 * Direct value comparisons are (most of the time) the other way.
+	 * Any reason ?
+	 * Maybe I shouldn't even ask since this one always create a lot of
+	 * heated discussion. Let me know.
+	 */
 	if (FAN_INPUT = nr)
 		val = data->fan[index] & 0x0fff;
 	else
@@ -855,6 +890,13 @@ show_pwm_enable(struct device *dev, struct device_attribute *attr, char *buf)
 	int index = sensor_attr->index;
 	u8 tmp;
 
+	/* so we are talking about
+	 *  (index = 0 && (data->pwm_fcms[0] & 1)),
+	 * correct ? Or maybe
+	 *  (index = 0 && (data->pwm_fcms[0] & (1 << index))),
+	 * Seems to me that would be less confusing and actually be less
+	 * expensive.
+	 */
 	if (1 = (data->pwm_fcms[0] & (1 << index))) {
 		tmp = 2;
 		goto out;
@@ -1011,6 +1053,7 @@ store_temp_pwm_enable(struct device *dev, struct device_attribute *attr,
 
 	switch (nr) {
 	case TEMP_PWM_ENABLE:
+		/* excessive () */
 		if ((tmp != 3) && (tmp != 4))
 			return -EINVAL;
 		tmp -= 3;
@@ -1075,6 +1118,7 @@ store_fanin(struct device *dev, struct device_attribute *attr,
 	case FANIN_TARGET:
 		val = fan_to_reg(SENSORS_LIMIT(val, 0, 0xfff));
 		w83795_write(client, W83795_REG_FTSH(index), (val >> 4) & 0xff);
+		    /* & 0xff doesn't hurt but isn't needed either. */
 		w83795_write(client, W83795_REG_FTSL(index), (val << 4) & 0xf0);
 		data->target_speed[index] = val;
 		break;
@@ -1234,6 +1278,7 @@ show_temp(struct device *dev, struct device_attribute *attr, char *buf)
 	struct w83795_data *data = w83795_update_device(dev);
 	long temp = temp_from_reg(data->temp[index][nr]);
 
+	/* reverse order ? */
 	if (TEMP_READ = nr)
 		temp += (data->temp_read_vrlsb[index] >> 6) * 250;
 	return sprintf(buf, "%ld\n", temp);
@@ -1810,6 +1855,7 @@ static int w83795_detect(struct i2c_client *client,
 
 	/* If Nuvoton chip, address of chip and W83795_REG_I2C_ADDR
 	   should match */
+	/* !((bank & 0x07) ? */
 	if ((bank & 0x07) = 0) {
 		i2c_addr = i2c_smbus_read_byte_data(client,
 						    W83795_REG_I2C_ADDR);
@@ -1825,6 +1871,8 @@ static int w83795_detect(struct i2c_client *client,
 	   Usually we don't write to chips during detection, but here we don't
 	   quite have the choice; hopefully it's OK, we are about to return
 	   success anyway */
+	/* != 0 isn't really needed.
+	 * Also, looks like a candicate for an else statement. */
 	if ((bank & 0x07) != 0)
 		i2c_smbus_write_byte_data(client, W83795_REG_BANKSEL,
 					  bank & ~0x07);
@@ -1891,6 +1939,7 @@ static int w83795_handle_files(struct device *dev, int (*fn)(struct device *,
 		}
 	}
 
+	/* != 0 isn't really needed */
 	if (data->enable_dts != 0) {
 		for (i = 0; i < ARRAY_SIZE(w83795_dts); i++) {
 			if (!(data->has_dts & (1 << i)))
@@ -1923,6 +1972,7 @@ static void w83795_check_dynamic_in_limits(struct i2c_client *client)
 	vid_ctl = w83795_read(client, W83795_REG_VID_CTRL);
 
 	/* Return immediately if VRM isn't configured */
+	/* !(vid_ctl & 0x07) ? */
 	if ((vid_ctl & 0x07) = 0x00 || (vid_ctl & 0x07) = 0x07)
 		return;
 
@@ -2021,6 +2071,8 @@ static int w83795_probe(struct i2c_client *client,
 			if (!(data->has_dts & (1 << i)))
 				continue;
 			tmp = w83795_read(client, W83795_REG_PECI_TBASE(i));
+			/* Is it common to report such values during boot ?
+			 * Or is it just excessive noise ? */
 			dev_info(&client->dev,
 				 "PECI agent %d Tbase temperature: %u\n",
 				 i + 1, (unsigned int)tmp & 0x7f);

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

Re: [lm-sensors] [patch 00/36] New w83795 driver

[Jean Delvare]

Hi Guenter,

On Thu, 16 Sep 2010 17:14:44 -0700, Guenter Roeck wrote:
> On Wed, Sep 15, 2010 at 09:40:05AM -0400, Jean Delvare wrote:
> > This is a new driver for the Winbond/Nuvoton W83795G/ADG hardware
> > monitoring chips. The original code was contributed by Wei Song, a
> > former employee of Nuvoton. I've fixed and improved a lot of things
> > on top of his work.
> > 
> Hi Jean,
> 
> highly unusual, I know, but I integrated all individual patches of this set
> and then added my coments into the resulting file.
> The result is a patch file with my comments.

Certainly unusual, but it should work, and I'm happy to receive
feedback no matter in which form.

> I might be able to spend some more time on it, but this is what I have so far.
> Hope it is useful.
> 
> Mostly nitpicks and suggested simplifications, but there may be one
> real bug (see the comment about lsb).

As this patch series is already very large, I don't plan to add any
non-needed fix at this point in time. I have refrained from adding some
work I've already done. But I keep further improvements for later, they
won't be lost.

> diff --git a/drivers/hwmon/w83795.c b/drivers/hwmon/w83795.c
> index 76c7f24..e19e6c9 100644
> --- a/drivers/hwmon/w83795.c
> +++ b/drivers/hwmon/w83795.c
> @@ -124,6 +124,14 @@ static const u8 W83795_REG_IN_HL_LSB[] = {
>  	0xa4,	/* VSEN17 */
>  };
>  
> +/*
> + * type is either 1 or 2.
> + * 1: index := index --> could use index + type - 1
> + * 2: index := index + 1 --> could use index + type - 1
> + * Suggestion:
> + *  #define IN_LSB_REG(index, type) W83795_REG_IN_HL_LSB[(index + type - 1)]
> + * or at least use type = IN_MAX instead of type = 1.
> + */
>  #define IN_LSB_REG(index, type) \
>  	(((type) = 1) ? W83795_REG_IN_HL_LSB[(index)] \
>  	: (W83795_REG_IN_HL_LSB[(index)] + 1))

I agree that (type) = 1 is ugly, given that all callers use symbolic
values rather than numeric constants. I have a pending patch changing
this and also reworking the handling of voltage LSBs at large, but it
wasn't ready in time for this series.

But your optimization proposal is interesting too, although your actual
implementation is bogus: we want [index] + 1, not [index + 1]. I will
consider it, assuming it doesn't interfere with my pending changes.

> @@ -161,6 +169,7 @@ static const u8 IN_LSB_SHIFT_IDX[][2] = {
>  #define W83795_REG_FAN_MIN_LSB(index)	(0xC4 + (index) / 2)
>  #define W83795_REG_FAN_MIN_LSB_SHIFT(index) \
>  	(((index) & 1) ? 4 : 0)
> +/* could use ((index) & 1) << 2) */
>  
>  #define W83795_REG_VID_CTRL		0x6A
>  

Good point, this is a small optimization, at the price of a lower
readability IMHO. I think I will prefer (((index) & 1) * 4), which is
as fast as your proposal, but easier to grasp.

> @@ -349,6 +358,9 @@ struct w83795_data {
>  	u8 dts_read_vrlsb[8];	/* Register value */
>  	s8 dts_ext[4];		/* Register value */
>  
> +	/* something like pwm_count would be better here. 
> +	 * All other has_xxx variables are bit masks.
> +	 */
>  	u8 has_pwm;		/* 795g supports 8 pwm, 795adg only supports 2,
>  				 * no config register, only affected by chip
>  				 * type */

This is on my to-do list. But there's another issue, which is that PWM7
and PWM8 can be disabled. So I will probably keep the name has_pwm, but
turn it into a bit mask. But this is low priority, as the ADG variant
of the chip has only 2 PWM outputs, and I haven't seen any G variant
(up to 8 PWM outputs) yet.

> @@ -470,6 +482,12 @@ static void w83795_update_limits(struct i2c_client *client)
>  
>  		/* Each register contains LSB for 2 fans, but we want to
>  		 * read it only once to save time */
> +		/*
> +		 * But that causes lsb to be undefined across loop instances,
> +		 * doesn't it, since it is only defined inside the loop ?
> +		 * And what happens if an even fan does not exist
> +		 * but the odd one does ?
> +		 */
>  		if ((i & 1) = 0 && (data->has_fan & (3 << i)))
>  			lsb = w83795_read(client, W83795_REG_FAN_MIN_LSB(i));
>  

I don't see any problem with lsb being undefined. lsb is always set if
it is going to be needed. I also don't see any problem with odd and
even fans: lsb is set if _any_ fan in a given pair is present (see the
3 << i).

If you can think of a specific scenario where it doesn't work, please
let me know. The code looks good to me.

> @@ -491,6 +509,7 @@ static void w83795_update_limits(struct i2c_client *client)
>  	}
>  
>  	/* Read the DTS limits */
> +	/* != 0 isn't really needed */
>  	if (data->enable_dts != 0) {
>  		for (limit = DTS_CRIT; limit <= DTS_WARN_HYST; limit++)
>  			data->dts_ext[limit] 
I agree. The code isn't mine...

> @@ -544,6 +563,7 @@ static struct w83795_data *w83795_update_pwm_config(struct device *dev)
>  		data->pwm_temp[i][TEMP_PWM_CTFS] >  			w83795_read(client, W83795_REG_CTFS(i));
>  		tmp = w83795_read(client, W83795_REG_HT(i));
> +		/* & 0x0f doesn't hurt but also isn't needed. */
>  		data->pwm_temp[i][TEMP_PWM_HCT] = (tmp >> 4) & 0x0f;
>  		data->pwm_temp[i][TEMP_PWM_HOT] = tmp & 0x0f;
>  	}

Ditto.

> @@ -620,6 +640,7 @@ static struct w83795_data *w83795_update_device(struct device *dev)
>  		data->fan[i] = w83795_read(client, W83795_REG_FAN(i)) << 4;
>  		data->fan[i] |>  		  (w83795_read(client, W83795_REG_VRLSB) >> 4) & 0x0F;
> +		    /* & 0x0f doesn't hurt but isn't needed either. */
>  	}
>  
>  	/* Update temperature */

Ditto.

> @@ -631,6 +652,7 @@ static struct w83795_data *w83795_update_device(struct device *dev)
>  	}
>  
>  	/* Update dts temperature */
> +	/* != 0 isn't really needed */
>  	if (data->enable_dts != 0) {
>  		for (i = 0; i < ARRAY_SIZE(data->dts); i++) {
>  			if (!(data->has_dts & (1 << i)))

Ditto.

> @@ -677,10 +699,11 @@ show_alarm_beep(struct device *dev, struct device_attribute *attr, char *buf)
>  	int bit = sensor_attr->index & 0x07;
>  	u8 val;
>  
> -	if (ALARM_STATUS = nr) {
> -		val = (data->alarms[index] >> (bit)) & 1;
> +	/* nr = ALARM_STATUS ? */
> +	if (ALARM_STATUS = nr) {			    /* { } not needed ? */
> +		val = (data->alarms[index] >> (bit)) & 1; /* (bit) --> bit */
>  	} else {		/* BEEP_ENABLE */
> -		val = (data->beeps[index] >> (bit)) & 1;
> +		val = (data->beeps[index] >> (bit)) & 1; /* (bit) --> bit */
>  	}
>  
>  	return sprintf(buf, "%u\n", val);

Ditto.

> @@ -744,6 +767,18 @@ show_fan(struct device *dev, struct device_attribute *attr, char *buf)
>  	struct w83795_data *data = w83795_update_device(dev);
>  	u16 val;
>  
> +	/* Those reversed comparisons always confuse me.
> +	 * I think the compiler should refuse to compile 
> +	 * such code to make me happy. And, no, I don't buy
> +	 * the argument that the compiler magically produces
> +	 * better code this way.
> +	 * They are used in this driver to compare variables against
> +	 * defined constants (though not all the time).
> +	 * Direct value comparisons are (most of the time) the other way.
> +	 * Any reason ?
> +	 * Maybe I shouldn't even ask since this one always create a lot of
> +	 * heated discussion. Let me know.
> +	 */
>  	if (FAN_INPUT = nr)
>  		val = data->fan[index] & 0x0fff;
>  	else

I fully agree with you. Again the code isn't mine.

> @@ -855,6 +890,13 @@ show_pwm_enable(struct device *dev, struct device_attribute *attr, char *buf)
>  	int index = sensor_attr->index;
>  	u8 tmp;
>  
> +	/* so we are talking about
> +	 *  (index = 0 && (data->pwm_fcms[0] & 1)),
> +	 * correct ? Or maybe
> +	 *  (index = 0 && (data->pwm_fcms[0] & (1 << index))),
> +	 * Seems to me that would be less confusing and actually be less
> +	 * expensive.
> +	 */
>  	if (1 = (data->pwm_fcms[0] & (1 << index))) {
>  		tmp = 2;
>  		goto out;

Seems to me that the code is simply buggy. There is no reason to handle
the case index = 0 (pwm1) any differently from the rest. Probably the
"1 =" shouldn't be there, but I'd rather review the function entirely
to make sure it's correct.

This is a part of the driver I didn't review yet, but quick testing a
few days ago suggested there where bugs to be fixed. Your code review
confirms this.

> @@ -1011,6 +1053,7 @@ store_temp_pwm_enable(struct device *dev, struct device_attribute *attr,
>  
>  	switch (nr) {
>  	case TEMP_PWM_ENABLE:
> +		/* excessive () */
>  		if ((tmp != 3) && (tmp != 4))
>  			return -EINVAL;
>  		tmp -= 3;
> @@ -1075,6 +1118,7 @@ store_fanin(struct device *dev, struct device_attribute *attr,
>  	case FANIN_TARGET:
>  		val = fan_to_reg(SENSORS_LIMIT(val, 0, 0xfff));
>  		w83795_write(client, W83795_REG_FTSH(index), (val >> 4) & 0xff);
> +		    /* & 0xff doesn't hurt but isn't needed either. */
>  		w83795_write(client, W83795_REG_FTSL(index), (val << 4) & 0xf0);
>  		data->target_speed[index] = val;
>  		break;
> @@ -1234,6 +1278,7 @@ show_temp(struct device *dev, struct device_attribute *attr, char *buf)
>  	struct w83795_data *data = w83795_update_device(dev);
>  	long temp = temp_from_reg(data->temp[index][nr]);
>  
> +	/* reverse order ? */
>  	if (TEMP_READ = nr)
>  		temp += (data->temp_read_vrlsb[index] >> 6) * 250;
>  	return sprintf(buf, "%ld\n", temp);

All agreed.

> @@ -1810,6 +1855,7 @@ static int w83795_detect(struct i2c_client *client,
>  
>  	/* If Nuvoton chip, address of chip and W83795_REG_I2C_ADDR
>  	   should match */
> +	/* !((bank & 0x07) ? */
>  	if ((bank & 0x07) = 0) {
>  		i2c_addr = i2c_smbus_read_byte_data(client,
>  						    W83795_REG_I2C_ADDR);

I prefer = 0 here, because 0 actually means something (bank 0).

> @@ -1825,6 +1871,8 @@ static int w83795_detect(struct i2c_client *client,
>  	   Usually we don't write to chips during detection, but here we don't
>  	   quite have the choice; hopefully it's OK, we are about to return
>  	   success anyway */
> +	/* != 0 isn't really needed.
> +	 * Also, looks like a candicate for an else statement. */
>  	if ((bank & 0x07) != 0)
>  		i2c_smbus_write_byte_data(client, W83795_REG_BANKSEL,
>  					  bank & ~0x07);

Even better would be to force the bank to 0 first, then the other test
becomes unconditional and we save one level of indentation. Thanks for
the idea :)

> @@ -1891,6 +1939,7 @@ static int w83795_handle_files(struct device *dev, int (*fn)(struct device *,
>  		}
>  	}
>  
> +	/* != 0 isn't really needed */
>  	if (data->enable_dts != 0) {
>  		for (i = 0; i < ARRAY_SIZE(w83795_dts); i++) {
>  			if (!(data->has_dts & (1 << i)))

Agreed.

> @@ -1923,6 +1972,7 @@ static void w83795_check_dynamic_in_limits(struct i2c_client *client)
>  	vid_ctl = w83795_read(client, W83795_REG_VID_CTRL);
>  
>  	/* Return immediately if VRM isn't configured */
> +	/* !(vid_ctl & 0x07) ? */
>  	if ((vid_ctl & 0x07) = 0x00 || (vid_ctl & 0x07) = 0x07)
>  		return;
>  

As for the bank, this 0x00 has a meaning, so I prefer that it stays.

> @@ -2021,6 +2071,8 @@ static int w83795_probe(struct i2c_client *client,
>  			if (!(data->has_dts & (1 << i)))
>  				continue;
>  			tmp = w83795_read(client, W83795_REG_PECI_TBASE(i));
> +			/* Is it common to report such values during boot ?
> +			 * Or is it just excessive noise ? */
>  			dev_info(&client->dev,
>  				 "PECI agent %d Tbase temperature: %u\n",
>  				 i + 1, (unsigned int)tmp & 0x7f);

I plan to export these values through sysfs in some way in the future,
but I couldn't decide how exactly, so for now the kernel log was the
only way. I need the values to investigate some strange PECI temperature
reports I got. So I don't plan to leave these messages in the driver
forever, but for the time being I want to keep them.

Thanks for the review!

-- 
Jean Delvare

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

Re: [lm-sensors] [patch 00/36] New w83795 driver

[Guenter Roeck]

Hi Jean,
On Sat, Sep 18, 2010 at 07:56:01AM -0400, Jean Delvare wrote:
> Hi Guenter,
> 
[ ... ] 
> >  
> > +/*
> > + * type is either 1 or 2.
> > + * 1: index := index --> could use index + type - 1
> > + * 2: index := index + 1 --> could use index + type - 1
> > + * Suggestion:
> > + *  #define IN_LSB_REG(index, type) W83795_REG_IN_HL_LSB[(index + type - 1)]
> > + * or at least use type = IN_MAX instead of type = 1.
> > + */
> >  #define IN_LSB_REG(index, type) \
> >  	(((type) = 1) ? W83795_REG_IN_HL_LSB[(index)] \
> >  	: (W83795_REG_IN_HL_LSB[(index)] + 1))
> 
> I agree that (type) = 1 is ugly, given that all callers use symbolic
> values rather than numeric constants. I have a pending patch changing
> this and also reworking the handling of voltage LSBs at large, but it
> wasn't ready in time for this series.
> 
> But your optimization proposal is interesting too, although your actual
> implementation is bogus: we want [index] + 1, not [index + 1]. I will
> consider it, assuming it doesn't interfere with my pending changes.
> 
oops ... so it should have been

	#define IN_LSB_REG(index, type) W83795_REG_IN_HL_LSB[index] + type - 1)

[ ... ] 

> > +		 * But that causes lsb to be undefined across loop instances,
> > +		 * doesn't it, since it is only defined inside the loop ?
> > +		 * And what happens if an even fan does not exist
> > +		 * but the odd one does ?
> > +		 */
> >  		if ((i & 1) = 0 && (data->has_fan & (3 << i)))
> >  			lsb = w83795_read(client, W83795_REG_FAN_MIN_LSB(i));
> >  
> 
> I don't see any problem with lsb being undefined. lsb is always set if
> it is going to be needed. I also don't see any problem with odd and
> even fans: lsb is set if _any_ fan in a given pair is present (see the
> 3 << i).
> 
> If you can think of a specific scenario where it doesn't work, please
> let me know. The code looks good to me.
> 
Looks like lsb survives across loop instances; at least the compiler doesn't complain 
about it. So I guess you are right.

[ ... ]

> >  
> > +	/* Those reversed comparisons always confuse me.
> > +	 * I think the compiler should refuse to compile 
> > +	 * such code to make me happy. And, no, I don't buy
> > +	 * the argument that the compiler magically produces
> > +	 * better code this way.
> > +	 * They are used in this driver to compare variables against
> > +	 * defined constants (though not all the time).
> > +	 * Direct value comparisons are (most of the time) the other way.
> > +	 * Any reason ?
> > +	 * Maybe I shouldn't even ask since this one always create a lot of
> > +	 * heated discussion. Let me know.
> > +	 */
> >  	if (FAN_INPUT = nr)
> >  		val = data->fan[index] & 0x0fff;
> >  	else
> 
> I fully agree with you. Again the code isn't mine.
> 
Would it make sense to add a cleanup patch on top of the other ones ?
I always find that such style issues are distracting, and make it difficult
to review it.

> > @@ -855,6 +890,13 @@ show_pwm_enable(struct device *dev, struct device_attribute *attr, char *buf)
> >  	int index = sensor_attr->index;
> >  	u8 tmp;
> >  
> > +	/* so we are talking about
> > +	 *  (index = 0 && (data->pwm_fcms[0] & 1)),
> > +	 * correct ? Or maybe
> > +	 *  (index = 0 && (data->pwm_fcms[0] & (1 << index))),
> > +	 * Seems to me that would be less confusing and actually be less
> > +	 * expensive.
> > +	 */
> >  	if (1 = (data->pwm_fcms[0] & (1 << index))) {
> >  		tmp = 2;
> >  		goto out;
> 
> Seems to me that the code is simply buggy. There is no reason to handle
> the case index = 0 (pwm1) any differently from the rest. Probably the
> "1 =" shouldn't be there, but I'd rather review the function entirely
> to make sure it's correct.
> 
> This is a part of the driver I didn't review yet, but quick testing a
> few days ago suggested there where bugs to be fixed. Your code review
> confirms this.
> 
Guess that explains a lot.

Guenter

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

Re: [lm-sensors] [patch 00/36] New w83795 driver

[Jean Delvare]

On Sat, 18 Sep 2010 06:34:33 -0700, Guenter Roeck wrote:
> Hi Jean,
> On Sat, Sep 18, 2010 at 07:56:01AM -0400, Jean Delvare wrote:
> > > +		 * But that causes lsb to be undefined across loop instances,
> > > +		 * doesn't it, since it is only defined inside the loop ?
> > > +		 * And what happens if an even fan does not exist
> > > +		 * but the odd one does ?
> > > +		 */
> > >  		if ((i & 1) = 0 && (data->has_fan & (3 << i)))
> > >  			lsb = w83795_read(client, W83795_REG_FAN_MIN_LSB(i));
> > >  
> > 
> > I don't see any problem with lsb being undefined. lsb is always set if
> > it is going to be needed. I also don't see any problem with odd and
> > even fans: lsb is set if _any_ fan in a given pair is present (see the
> > 3 << i).
> > 
> > If you can think of a specific scenario where it doesn't work, please
> > let me know. The code looks good to me.
> > 
> Looks like lsb survives across loop instances; at least the compiler doesn't complain 
> about it. So I guess you are right.

Of course it does. Why wouldn't it?

> > > (...)
> > > +	/* Those reversed comparisons always confuse me.
> > > +	 * I think the compiler should refuse to compile 
> > > +	 * such code to make me happy. And, no, I don't buy
> > > +	 * the argument that the compiler magically produces
> > > +	 * better code this way.
> > > +	 * They are used in this driver to compare variables against
> > > +	 * defined constants (though not all the time).
> > > +	 * Direct value comparisons are (most of the time) the other way.
> > > +	 * Any reason ?
> > > +	 * Maybe I shouldn't even ask since this one always create a lot of
> > > +	 * heated discussion. Let me know.
> > > +	 */
> > >  	if (FAN_INPUT = nr)
> > >  		val = data->fan[index] & 0x0fff;
> > >  	else
> > 
> > I fully agree with you. Again the code isn't mine.
>
> Would it make sense to add a cleanup patch on top of the other ones ?
> I always find that such style issues are distracting, and make it difficult
> to review it.

I've written that patch already, based on your comments. I'll post it
with the next batch of w83795 patches.

Thanks,
-- 
Jean Delvare

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

Re: [lm-sensors] [patch 00/36] New w83795 driver

[Guenter Roeck]

On Sat, Sep 18, 2010 at 10:16:14AM -0400, Jean Delvare wrote:
> On Sat, 18 Sep 2010 06:34:33 -0700, Guenter Roeck wrote:
> > Hi Jean,
> > On Sat, Sep 18, 2010 at 07:56:01AM -0400, Jean Delvare wrote:
> > > > +		 * But that causes lsb to be undefined across loop instances,
> > > > +		 * doesn't it, since it is only defined inside the loop ?
> > > > +		 * And what happens if an even fan does not exist
> > > > +		 * but the odd one does ?
> > > > +		 */
> > > >  		if ((i & 1) = 0 && (data->has_fan & (3 << i)))
> > > >  			lsb = w83795_read(client, W83795_REG_FAN_MIN_LSB(i));
> > > >  
> > > 
> > > I don't see any problem with lsb being undefined. lsb is always set if
> > > it is going to be needed. I also don't see any problem with odd and
> > > even fans: lsb is set if _any_ fan in a given pair is present (see the
> > > 3 << i).
> > > 
> > > If you can think of a specific scenario where it doesn't work, please
> > > let me know. The code looks good to me.
> > > 
> > Looks like lsb survives across loop instances; at least the compiler doesn't complain 
> > about it. So I guess you are right.
> 
> Of course it does. Why wouldn't it?
> 
Wrong understanding of C on my side. Still happens, as it is.
I thought the lifetime of a local variable inside a loop is only
for single instance of that loop. Apparently that is not the case.

Guenter

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