[lm-sensors] http://www.lm-sensors.org/ticket/2188

[lm-sensors] http://www.lm-sensors.org/ticket/2188

[Juerg Haefliger]

any takers?

Regards
...juerg

[lm-sensors] http://www.lm-sensors.org/ticket/2188

[Hans de Goede]

Juerg Haefliger wrote:
> any takers?
> 

I'll review your driver if you review mine:
http://lists.lm-sensors.org/pipermail/lm-sensors/2007-January/018703.html
:) Deal?

Jean,

Would you be ok with accepting patches / new drivers reviewed by other 
contributers then you and / Mark? I think that could greatly help to speed up 
the inclusion of newer drivers.

Regards,

Hans

[lm-sensors] http://www.lm-sensors.org/ticket/2188

[Jean Delvare]

Hi Hans,

On Thu, 15 Mar 2007 19:05:00 +0100, Hans de Goede wrote:
> Jean,
> 
> Would you be ok with accepting patches / new drivers reviewed by other 
> contributers then you and / Mark? I think that could greatly help to speed up 
> the inclusion of newer drivers.

If I would be OK? I'm _asking_ for this for years!

-- 
Jean Delvare

[lm-sensors] http://www.lm-sensors.org/ticket/2188

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 
> On Thu, 15 Mar 2007 19:05:00 +0100, Hans de Goede wrote:
>> Jean,
>>
>> Would you be ok with accepting patches / new drivers reviewed by other 
>> contributers then you and / Mark? I think that could greatly help to speed up 
>> the inclusion of newer drivers.
> 
> If I would be OK? I'm _asking_ for this for years!
> 

Excellent,

Juerg do we have a deal? My time is scarce (like for all of us). But I'm more 
then willing to invest it in a review, if I get a review in return :)

Regards,

Hans

[lm-sensors] http://www.lm-sensors.org/ticket/2188

[Juerg Haefliger]

Hans,

Yes, the deal is on.
Do you have a datasheet you could share?
Unfortunately I can't share the datasheet for the DME1737, I'm under NDA.

I'll try to take a look at your patch next week.

...juerg



On 3/16/07, Hans de Goede <j.w.r.degoede at hhs.nl> wrote:
> Jean Delvare wrote:
> > Hi Hans,
> >
> > On Thu, 15 Mar 2007 19:05:00 +0100, Hans de Goede wrote:
> >> Jean,
> >>
> >> Would you be ok with accepting patches / new drivers reviewed by other
> >> contributers then you and / Mark? I think that could greatly help to speed up
> >> the inclusion of newer drivers.
> >
> > If I would be OK? I'm _asking_ for this for years!
> >
>
> Excellent,
>
> Juerg do we have a deal? My time is scarce (like for all of us). But I'm more
> then willing to invest it in a review, if I get a review in return :)
>
> Regards,
>
> Hans
>
>

[lm-sensors] http://www.lm-sensors.org/ticket/2188

[Hans de Goede]

Juerg Haefliger wrote:
> Hans,
> 
> Yes, the deal is on.
> Do you have a datasheet you could share?
Abit doesn't release (probably doesn't even have) a datasheet for this chip, my 
driver is reverse engineered, but has been tested successfully by about 6 
people on 5 different abit motherboards.

> Unfortunately I can't share the datasheet for the DME1737, I'm under NDA.
> 
> I'll try to take a look at your patch next week.
> 

Some first comments as a result of looking at the documentation, do we really 
want these options: ?
+* force_temp_map: bool	Force the temp-to-zone mapping to the default, which is
+			temp1->zone1, temp2->zone2, temp3->zone3. The driver
+			currently only supports the default mapping and will
+			not load if the mapping is different, unless this
+			parameter is used.
+
+* force_fan_map: bool	Force the fan-to-pwm mapping to the default which is
+			fan1->pwm1, fan2->pwm2, fan3->pwm3. The driver
+			currently only supports the default mapping and will
+			not load if the mapping is different, unless this
+			parameter is used.

I find them rather dangerous both could mess up the cooling. If one must pass 
force_temp_map, then one must probably also change pwm[1-3]_auto_channels_temp 
to match, or otherwise the wrong temp is used for regulating the fan speed!
Maybe your driver could automaticly do this adjusting? Then it  would become 
less dangerous imho.

force_fan_map is even worse, then if indeed a fan is attached to fan sensor 1, 
but pwm output 2, the speed regulation becomes totally broken! I must say I 
cannot imagine a motherboard manufacturer doing something like this, and thus I 
question the usefullness of this option.

Atleast the doc should be much more clear that this is dangerous and could 
cause once computer to overheat.


Regards,

Hans

[lm-sensors] http://www.lm-sensors.org/ticket/2188

[Hans de Goede]

Juerg Haefliger wrote:
> Hans,
> 
> Yes, the deal is on.
> Do you have a datasheet you could share?
> Unfortunately I can't share the datasheet for the DME1737, I'm under NDA.
> 

Ok, so here is my review of your driver:

General remarks:
- i2x writes / read status never gets checked anywhere, I dunno if this is done
   in other lm_sensors drivers similary, but IMHO these things should be
   checked!

---

+/* temperature input */
+static int TEMP_FROM_REG(int reg, int res)
+{
+	return (((reg & (1 << (res - 1))) ? reg - (1 << res) : reg) *
+		1000 / (1 << (res - 8)));
+}
+
+static int TEMP_TO_REG(int val, int res)
+{
+	int temp = val * (1 << (res - 8)) / 1000;
+	return SENSORS_LIMIT((temp < 0) ? temp + (1 << res) : temp,
+			     0, (1 << res) - 1 );
+}

Wouldn't it be better readable / clearer when instead of:
"x * 1000 / (1 << (res - 8))" you write "(x * 1000) >> (res - 8)" ?
And the same for TEMP_TO_REG, instead of: "val * (1 << (res - 8)) / 1000",
write: "(val << (res - 8)) / 1000" ?

Then its clear (to me) at once that you're changing a 8.x bits fixed resolution
value to a res bits resolution value and vica versa.

---

+static int TEMP_HYST_FROM_REG(int reg, int ix) {
+	return ((((ix = 1) ? reg : reg >> 4) & 0x0f) * 1000);
+}
+
+static int TEMP_HYST_TO_REG(int val, int ix, int reg)
+{
+	int hyst = SENSORS_LIMIT(val, 0, 15);
+	return ((ix = 1) ? (reg & 0xf0) | hyst : (reg & 0x0f) | (hyst << 4));
+}

This doesn't seem consistent, when getting a hyst from the reg you multiply it
by 1000 making it ready for sysfs, but when storing it to the reg, you don't
divide it by 1000 (which should be done before the limit, or the limit should
be adjusted)

Actually when using TEMP_HYST_TO_REG here:
+	case SYS_TEMP_AUTO_POINT1_TEMP_HYST:
+		data->zone_hyst[ix = 2] = TEMP_HYST_TO_REG(
+					TEMP_FROM_REG(data->zone_low[ix], 8) -
+					val, ix, dme1737_read(client,
+					DME1737_REG_ZONE_HYST(ix = 2)));
+		dme1737_write(client, DME1737_REG_ZONE_HYST(ix = 2),
+			      data->zone_hyst[ix = 2]);
+		break;

You don't divide the temp past to TEMP_HYST_TO_REG by 1000, so its not
inconsistent its actually a bug (fix please).

---

Please add a comment that the pwm_freq registers hold the pwm freq in the lower 
nibble and the temp_range for the speed control in the higher nibble, without 
such a comment, this code looks rather strange:
+	case SYS_TEMP_AUTO_POINT2_TEMP:
+		res = TEMP_FROM_REG(data->zone_low[ix], 8) +
+		      TEMP_RANGE_FROM_REG(data->pwm_freq[ix]);
+		break;

---

+	case SYS_TEMP_AUTO_POINT2_TEMP:
+		data->pwm_freq[ix] = TEMP_RANGE_TO_REG(val -
+					TEMP_FROM_REG(data->zone_low[ix], 8),
+					dme1737_read(client,
+					DME1737_REG_PWM_FREQ(ix)));

Why do you use the cached temp here, but not the cached pwm_freq reg contents?? 
Notice that you do this in several places.

---

+#define SYS_PWM				0
+#define SYS_PWM_FREQ			2
+#define SYS_PWM_ENABLE			3
+#define SYS_PWM_RAMP_RATE		4
+#define SYS_PWM_AUTO_CHANNELS_TEMP	5
+#define SYS_PWM_AUTO_POINT1_PWM		6
+#define SYS_PWM_AUTO_POINT2_PWM		7

Why is 1 not used in this "enum" ?

---

Wouldn't it be cleaner to instead of:
+#define SENSOR_ATTR_FAN_1TO4(ix) \
+	SENSOR_ATTR_2(fan##ix##_type, S_IRUGO | S_IWUSR, \
+		show_fan, set_fan, SYS_FAN_TYPE, ix-1)
write:
+#define SENSOR_ATTR_FAN_1TO4(ix) \
+       SENSOR_ATTR_FAN(ix) \
+	SENSOR_ATTR_2(fan##ix##_type, S_IRUGO | S_IWUSR, \
+		show_fan, set_fan, SYS_FAN_TYPE, ix-1)

And idem for 5to6, and the instead of:
+	/* fans */
+	SENSOR_ATTR_FAN(1),
+	SENSOR_ATTR_FAN(2),
+	SENSOR_ATTR_FAN(3),
+	SENSOR_ATTR_FAN(4),
+	SENSOR_ATTR_FAN(5),
+	SENSOR_ATTR_FAN(6),
+	/* fan[1-4] unique attrs */
+	SENSOR_ATTR_FAN_1TO4(1),
+	SENSOR_ATTR_FAN_1TO4(2),
+	SENSOR_ATTR_FAN_1TO4(3),
+	SENSOR_ATTR_FAN_1TO4(4),
+	/* fan[5-6] unique attrs */
+	SENSOR_ATTR_FAN_5TO6(5),
+	SENSOR_ATTR_FAN_5TO6(6),

write:
+	/* fans */
+	SENSOR_ATTR_FAN_1TO4(1),
+	SENSOR_ATTR_FAN_1TO4(2),
+	SENSOR_ATTR_FAN_1TO4(3),
+	SENSOR_ATTR_FAN_1TO4(4),
+	SENSOR_ATTR_FAN_5TO6(5),
+	SENSOR_ATTR_FAN_5TO6(6),



Well thats it all in all it looks like a clean driver to me.

Regards,

Hans

[lm-sensors] http://www.lm-sensors.org/ticket/2188

[Jean Delvare]

On Sun, 18 Mar 2007 15:16:30 +0100, Hans de Goede wrote:
> General remarks:
> - i2x writes / read status never gets checked anywhere, I dunno if this is done
>    in other lm_sensors drivers similary, but IMHO these things should be
>    checked!

Most other I2C/SMBus hardware monitoring drivers indeed do not check
the status on I2C transactions and assume they were successful. This is
bad, but OTOH in a vast majority of cases it doesn't cause problem.
SMBus appears to be very reliable in practice.

Not to say that I do not encourage driver authors to check the status
of I2C transactions, but this is definitely not mandatory to get your
driver accepted in mainline.

-- 
Jean Delvare

[lm-sensors] http://www.lm-sensors.org/ticket/2188

[Juerg Haefliger]

Hans,

> Some first comments as a result of looking at the documentation, do we really
> want these options: ?
> +* force_temp_map: bool Force the temp-to-zone mapping to the default, which is
> +                       temp1->zone1, temp2->zone2, temp3->zone3. The driver
> +                       currently only supports the default mapping and will
> +                       not load if the mapping is different, unless this
> +                       parameter is used.
> +
> +* force_fan_map: bool  Force the fan-to-pwm mapping to the default which is
> +                       fan1->pwm1, fan2->pwm2, fan3->pwm3. The driver
> +                       currently only supports the default mapping and will
> +                       not load if the mapping is different, unless this
> +                       parameter is used.
>
> I find them rather dangerous both could mess up the cooling. If one must pass
> force_temp_map, then one must probably also change pwm[1-3]_auto_channels_temp
> to match, or otherwise the wrong temp is used for regulating the fan speed!
> Maybe your driver could automaticly do this adjusting? Then it  would become
> less dangerous imho.
>
> force_fan_map is even worse, then if indeed a fan is attached to fan sensor 1,
> but pwm output 2, the speed regulation becomes totally broken! I must say I
> cannot imagine a motherboard manufacturer doing something like this, and thus I
> question the usefullness of this option.

Agreed. Originally I wanted the driver to be able to handle these
special conditions but it turned out to be too complicated and I
reasoned that not too many boards will be out there that use
non-standard mappings. At the time, for some reason I thought it would
be a good idea if there's a way to still load the driver in that
case... Not sure anymore why...
I'll remove them and just have the driver print a warning if it
detects any of these conditions.

Thanks
...juerg



>
> Atleast the doc should be much more clear that this is dangerous and could
> cause once computer to overheat.
>
>
> Regards,
>
> Hans
>