[lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Chris]

Moved Fan Pulse per revolution into a loop so that both channel 1 and
channel 2 get set, instead of just channel 1


Signed-off-by: Chris D Schimp <silverchris@gmail.com>

---
diff -uprN -X vanilla/linux-3.2.5/Documentation/dontdiff
vanilla/linux-3.2.5/drivers/hwmon/max6639.c
devel/linux-3.2.5/drivers/hwmon/max6639.c
--- vanilla/linux-3.2.5/drivers/hwmon/max6639.c	2012-02-06
12:47:00.000000000 -0500
+++ devel/linux-3.2.5/drivers/hwmon/max6639.c	2012-02-11
21:40:02.399127171 -0500
@@ -439,25 +439,24 @@ static int max6639_init_client(struct i2
 	if (err)
 		goto exit;

-	/* Fans pulse per revolution is 2 by default */
-	if (max6639_info && max6639_info->ppr > 0 &&
-			max6639_info->ppr < 5)
-		data->ppr = max6639_info->ppr;
-	else
-		data->ppr = 2;
-	data->ppr -= 1;
-	err = i2c_smbus_write_byte_data(client,
-			MAX6639_REG_FAN_PPR(i),
-			data->ppr << 5);
-	if (err)
-		goto exit;
-
 	if (max6639_info)
 		rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
 	data->rpm_range = rpm_range;

 	for (i = 0; i < 2; i++) {

+		/* Fans pulse per revolution is 2 by default */
+		if (max6639_info && max6639_info->ppr > 0 &&
+				max6639_info->ppr < 5)
+			data->ppr = max6639_info->ppr;
+		else
+			data->ppr = 2;
+		data->ppr -= 1;
+		err = i2c_smbus_write_byte_data(client,
+			MAX6639_REG_FAN_PPR(i),
+			data->ppr << 5);
+		if (err)
+			goto exit;
 		/* Fans config PWM, RPM */
 		err = i2c_smbus_write_byte_data(client,
 			MAX6639_REG_FAN_CONFIG1(i),

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Jean Delvare]

Hi Chris,

On Sat, 11 Feb 2012 22:00:39 -0500, Chris wrote:
> Moved Fan Pulse per revolution into a loop so that both channel 1 and
> channel 2 get set, instead of just channel 1

You are right that the code is broken. However your fix is
unnecessarily expensive, as you end up setting data->ppr twice. What
needs to be moved inside the loop is the register write and only that.

Note that this bug wasn't spotted earlier because i (and err, for that
matter) are initialized at the beginning of the function when they
did not have to. I guess I will never repeat that enough: do not
initialize variables "just in case", or gcc can no longer help you when
you get things wrong.

Also note that I find it highly discussable that the driver initializes
the chip with arbitrary values in the absence of platform data. The
usual policy of hwmon drivers is to leave the chip untouched by
default, assuming that the hardware defaults or firmware/BIOS settings
are OK. Here some configuration bits are even set to 0 simply because
simple register writes are used instead of read-modify-write cycles.
I'm not even sure if this is intended.

> Signed-off-by: Chris D Schimp <silverchris@gmail.com>
> 
> ---
> diff -uprN -X vanilla/linux-3.2.5/Documentation/dontdiff
> vanilla/linux-3.2.5/drivers/hwmon/max6639.c
> devel/linux-3.2.5/drivers/hwmon/max6639.c
> --- vanilla/linux-3.2.5/drivers/hwmon/max6639.c	2012-02-06
> 12:47:00.000000000 -0500
> +++ devel/linux-3.2.5/drivers/hwmon/max6639.c	2012-02-11
> 21:40:02.399127171 -0500
> @@ -439,25 +439,24 @@ static int max6639_init_client(struct i2
>  	if (err)
>  		goto exit;
> 
> -	/* Fans pulse per revolution is 2 by default */
> -	if (max6639_info && max6639_info->ppr > 0 &&
> -			max6639_info->ppr < 5)
> -		data->ppr = max6639_info->ppr;
> -	else
> -		data->ppr = 2;
> -	data->ppr -= 1;
> -	err = i2c_smbus_write_byte_data(client,
> -			MAX6639_REG_FAN_PPR(i),
> -			data->ppr << 5);
> -	if (err)
> -		goto exit;
> -
>  	if (max6639_info)
>  		rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
>  	data->rpm_range = rpm_range;
> 
>  	for (i = 0; i < 2; i++) {
> 
> +		/* Fans pulse per revolution is 2 by default */
> +		if (max6639_info && max6639_info->ppr > 0 &&
> +				max6639_info->ppr < 5)
> +			data->ppr = max6639_info->ppr;
> +		else
> +			data->ppr = 2;
> +		data->ppr -= 1;
> +		err = i2c_smbus_write_byte_data(client,
> +			MAX6639_REG_FAN_PPR(i),
> +			data->ppr << 5);

I think there is a second bug here. According to the datasheet the
shift should be 6 not 5. I'm not sure how this managed to go unnoticed
so far. Maybe there is another bug somewhere in the driver? I see that
FAN_FROM_REG() takes data->ppr as a parameter, which it should not
according to the datasheet. If the PPR setting is set correctly for the
fan being used, then it doesn't show in the formula:

Tachometer count value = internal clock frequency * 60 / actual RPM

where internal clock frequency = fan rpm range / 2, so:

Actual RPM = fan rpm range * 30 / tachometer count value

> +		if (err)
> +			goto exit;
>  		/* Fans config PWM, RPM */
>  		err = i2c_smbus_write_byte_data(client,
>  			MAX6639_REG_FAN_CONFIG1(i),

Assuming I am correct, can you please send two patches, one clearing
the initialization of i and moving the write to MAX6639_REG_FAN_PPR(i)
inside the loop, and a second one fixing the bit shift and the
definition of FAN_FROM_REG?

Note that data->ppr will be unused at this point, so it might as well
be dropped, a local variable will be enough.

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] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Chris]

Fixed RPM calculations to not use ppr, and to be closer to what the
data sheet specifies.
Signed-off-by: Chris D Schimp <silverchris@gmail.com>
---
You would be very correct about the second bug. I have fixed that bug,
and removed the use of ppr in this patch attached. I have also changed
the behavior of the driver as to not change or initialize any
settings, so that it will leave all the bios or default settings in
place and clean up the things left over.


diff -uprN -X linux-3.2.5/Documentation/dontdiff
old/linux-3.2.5/drivers/hwmon/max6639.c
new/linux-3.2.5/drivers/hwmon/max6639.c
--- old/linux-3.2.5/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
+++ new/linux-3.2.5/drivers/hwmon/max6639.c	2012-02-12 23:16:21.770059276 -0500
@@ -72,8 +72,8 @@ static unsigned short normal_i2c[] = { 0

 static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };

-#define FAN_FROM_REG(val, div, rpm_range)	((val) == 0 ? -1 : \
-	(val) == 255 ? 0 : (rpm_ranges[rpm_range] * 30) / ((div + 1) * (val)))
+#define FAN_FROM_REG(val, rpm_range)	((val) == 0 ? -1 : \
+	(val) == 255 ? 0 : (rpm_ranges[rpm_range] * 30) / val)
 #define TEMP_LIMIT_TO_REG(val)	SENSORS_LIMIT((val) / 1000, 0, 255)

 /*
@@ -333,7 +334,7 @@ static ssize_t show_fan_input(struct dev
 		return PTR_ERR(data);

 	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
-		       data->ppr, data->rpm_range));
+		       data->rpm_range[attr->index]));
 }

 static ssize_t show_alarm(struct device *dev,


On Sun, Feb 12, 2012 at 4:30 AM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Chris,
>
> On Sat, 11 Feb 2012 22:00:39 -0500, Chris wrote:
>> Moved Fan Pulse per revolution into a loop so that both channel 1 and
>> channel 2 get set, instead of just channel 1
>
> You are right that the code is broken. However your fix is
> unnecessarily expensive, as you end up setting data->ppr twice. What
> needs to be moved inside the loop is the register write and only that.
>
> Note that this bug wasn't spotted earlier because i (and err, for that
> matter) are initialized at the beginning of the function when they
> did not have to. I guess I will never repeat that enough: do not
> initialize variables "just in case", or gcc can no longer help you when
> you get things wrong.
>
> Also note that I find it highly discussable that the driver initializes
> the chip with arbitrary values in the absence of platform data. The
> usual policy of hwmon drivers is to leave the chip untouched by
> default, assuming that the hardware defaults or firmware/BIOS settings
> are OK. Here some configuration bits are even set to 0 simply because
> simple register writes are used instead of read-modify-write cycles.
> I'm not even sure if this is intended.
>
>> Signed-off-by: Chris D Schimp <silverchris@gmail.com>
>>
>> ---
>> diff -uprN -X vanilla/linux-3.2.5/Documentation/dontdiff
>> vanilla/linux-3.2.5/drivers/hwmon/max6639.c
>> devel/linux-3.2.5/drivers/hwmon/max6639.c
>> --- vanilla/linux-3.2.5/drivers/hwmon/max6639.c       2012-02-06
>> 12:47:00.000000000 -0500
>> +++ devel/linux-3.2.5/drivers/hwmon/max6639.c 2012-02-11
>> 21:40:02.399127171 -0500
>> @@ -439,25 +439,24 @@ static int max6639_init_client(struct i2
>>       if (err)
>>               goto exit;
>>
>> -     /* Fans pulse per revolution is 2 by default */
>> -     if (max6639_info && max6639_info->ppr > 0 &&
>> -                     max6639_info->ppr < 5)
>> -             data->ppr = max6639_info->ppr;
>> -     else
>> -             data->ppr = 2;
>> -     data->ppr -= 1;
>> -     err = i2c_smbus_write_byte_data(client,
>> -                     MAX6639_REG_FAN_PPR(i),
>> -                     data->ppr << 5);
>> -     if (err)
>> -             goto exit;
>> -
>>       if (max6639_info)
>>               rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
>>       data->rpm_range = rpm_range;
>>
>>       for (i = 0; i < 2; i++) {
>>
>> +             /* Fans pulse per revolution is 2 by default */
>> +             if (max6639_info && max6639_info->ppr > 0 &&
>> +                             max6639_info->ppr < 5)
>> +                     data->ppr = max6639_info->ppr;
>> +             else
>> +                     data->ppr = 2;
>> +             data->ppr -= 1;
>> +             err = i2c_smbus_write_byte_data(client,
>> +                     MAX6639_REG_FAN_PPR(i),
>> +                     data->ppr << 5);
>
> I think there is a second bug here. According to the datasheet the
> shift should be 6 not 5. I'm not sure how this managed to go unnoticed
> so far. Maybe there is another bug somewhere in the driver? I see that
> FAN_FROM_REG() takes data->ppr as a parameter, which it should not
> according to the datasheet. If the PPR setting is set correctly for the
> fan being used, then it doesn't show in the formula:
>
> Tachometer count value = internal clock frequency * 60 / actual RPM
>
> where internal clock frequency = fan rpm range / 2, so:
>
> Actual RPM = fan rpm range * 30 / tachometer count value
>
>> +             if (err)
>> +                     goto exit;
>>               /* Fans config PWM, RPM */
>>               err = i2c_smbus_write_byte_data(client,
>>                       MAX6639_REG_FAN_CONFIG1(i),
>
> Assuming I am correct, can you please send two patches, one clearing
> the initialization of i and moving the write to MAX6639_REG_FAN_PPR(i)
> inside the loop, and a second one fixing the bit shift and the
> definition of FAN_FROM_REG?
>
> Note that data->ppr will be unused at this point, so it might as well
> be dropped, a local variable will be enough.
>
> 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] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Chris]

Removed initialization to leave bios or hardware defaults alone.
---
diff -uprN -X linux-3.2.5/Documentation/dontdiff
old/linux-3.2.5/drivers/hwmon/max6639.c
new/linux-3.2.5/drivers/hwmon/max6639.c
--- old/linux-3.2.5/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
+++ new/linux-3.2.5/drivers/hwmon/max6639.c	2012-02-12 23:16:21.770059276 -0500
@@ -90,16 +90,13 @@ struct max6639_data {
 	bool temp_fault[2];	/* Detected temperature diode failure */
 	u8 fan[2];		/* Register value: TACH count for fans >=30 */
 	u8 status;		/* Detected channel alarms and fan failures */
+	u8 rpm_range[2];		/* Index in above rpm_ranges table */

 	/* Register values only written to */
 	u8 pwm[2];		/* Register value: Duty cycle 0..120 */
 	u8 temp_therm[2];	/* THERM Temperature, 0..255 C (->_max) */
 	u8 temp_alert[2];	/* ALERT Temperature, 0..255 C (->_crit) */
 	u8 temp_ot[2];		/* OT Temperature, 0..255 C (->_emergency) */
-
-	/* Register values initialized only once */
-	u8 ppr;			/* Pulses per rotation 0..3 for 1..4 ppr */
-	u8 rpm_range;		/* Index in above rpm_ranges table */
 };

 static struct max6639_data *max6639_update_device(struct device *dev)
@@ -136,6 +133,10 @@ static struct max6639_data *max6639_upda
 			data->fan[i] = res;

 			res = i2c_smbus_read_byte_data(client,
+					MAX6639_REG_FAN_CONFIG1(i));
+			data->rpm_range[i] = 0x03&res;
+
+			res = i2c_smbus_read_byte_data(client,
 					MAX6639_REG_TEMP_EXT(i));
 			if (res < 0) {
 				ret = ERR_PTR(res);
@@ -408,117 +409,6 @@ static const struct attribute_group max6
 	.attrs = max6639_attributes,
 };

-/*
- *  returns respective index in rpm_ranges table
- *  1 by default on invalid range
- */
-static int rpm_range_to_reg(int range)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(rpm_ranges); i++) {
-		if (rpm_ranges[i] == range)
-			return i;
-	}
-
-	return 1; /* default: 4000 RPM */
-}
-
-static int max6639_init_client(struct i2c_client *client)
-{
-	struct max6639_data *data = i2c_get_clientdata(client);
-	struct max6639_platform_data *max6639_info =
-		client->dev.platform_data;
-	int i = 0;
-	int rpm_range = 1; /* default: 4000 RPM */
-	int err = 0;
-
-	/* Reset chip to default values, see below for GCONFIG setup */
-	err = i2c_smbus_write_byte_data(client, MAX6639_REG_GCONFIG,
-				  MAX6639_GCONFIG_POR);
-	if (err)
-		goto exit;
-
-	/* Fans pulse per revolution is 2 by default */
-	if (max6639_info && max6639_info->ppr > 0 &&
-			max6639_info->ppr < 5)
-		data->ppr = max6639_info->ppr;
-	else
-		data->ppr = 2;
-	data->ppr -= 1;
-	err = i2c_smbus_write_byte_data(client,
-			MAX6639_REG_FAN_PPR(i),
-			data->ppr << 5);
-	if (err)
-		goto exit;
-
-	if (max6639_info)
-		rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
-	data->rpm_range = rpm_range;
-
-	for (i = 0; i < 2; i++) {
-
-		/* Fans config PWM, RPM */
-		err = i2c_smbus_write_byte_data(client,
-			MAX6639_REG_FAN_CONFIG1(i),
-			MAX6639_FAN_CONFIG1_PWM | rpm_range);
-		if (err)
-			goto exit;
-
-		/* Fans PWM polarity high by default */
-		if (max6639_info && max6639_info->pwm_polarity == 0)
-			err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_FAN_CONFIG2a(i), 0x00);
-		else
-			err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_FAN_CONFIG2a(i), 0x02);
-		if (err)
-			goto exit;
-
-		/*
-		 * /THERM full speed enable,
-		 * PWM frequency 25kHz, see also GCONFIG below
-		 */
-		err = i2c_smbus_write_byte_data(client,
-			MAX6639_REG_FAN_CONFIG3(i),
-			MAX6639_FAN_CONFIG3_THERM_FULL_SPEED | 0x03);
-		if (err)
-			goto exit;
-
-		/* Max. temp. 80C/90C/100C */
-		data->temp_therm[i] = 80;
-		data->temp_alert[i] = 90;
-		data->temp_ot[i] = 100;
-		err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_THERM_LIMIT(i),
-				data->temp_therm[i]);
-		if (err)
-			goto exit;
-		err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_ALERT_LIMIT(i),
-				data->temp_alert[i]);
-		if (err)
-			goto exit;
-		err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_OT_LIMIT(i), data->temp_ot[i]);
-		if (err)
-			goto exit;
-
-		/* PWM 120/120 (i.e. 100%) */
-		data->pwm[i] = 120;
-		err = i2c_smbus_write_byte_data(client,
-				MAX6639_REG_TARGTDUTY(i), data->pwm[i]);
-		if (err)
-			goto exit;
-	}
-	/* Start monitoring */
-	err = i2c_smbus_write_byte_data(client, MAX6639_REG_GCONFIG,
-		MAX6639_GCONFIG_DISABLE_TIMEOUT | MAX6639_GCONFIG_CH2_LOCAL |
-		MAX6639_GCONFIG_PWM_FREQ_HI);
-exit:
-	return err;
-}
-
 /* Return 0 if detection is successful, -ENODEV otherwise */
 static int max6639_detect(struct i2c_client *client,
 			  struct i2c_board_info *info)
@@ -555,11 +445,6 @@ static int max6639_probe(struct i2c_clie
 	i2c_set_clientdata(client, data);
 	mutex_init(&data->update_lock);

-	/* Initialize the max6639 chip */
-	err = max6639_init_client(client);
-	if (err < 0)
-		goto error_free;
-
 	/* Register sysfs hooks */
 	err = sysfs_create_group(&client->dev.kobj, &max6639_group);
 	if (err)

On Sun, Feb 12, 2012 at 4:30 AM, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Chris,
>
> On Sat, 11 Feb 2012 22:00:39 -0500, Chris wrote:
>> Moved Fan Pulse per revolution into a loop so that both channel 1 and
>> channel 2 get set, instead of just channel 1
>
> You are right that the code is broken. However your fix is
> unnecessarily expensive, as you end up setting data->ppr twice. What
> needs to be moved inside the loop is the register write and only that.
>
> Note that this bug wasn't spotted earlier because i (and err, for that
> matter) are initialized at the beginning of the function when they
> did not have to. I guess I will never repeat that enough: do not
> initialize variables "just in case", or gcc can no longer help you when
> you get things wrong.
>
> Also note that I find it highly discussable that the driver initializes
> the chip with arbitrary values in the absence of platform data. The
> usual policy of hwmon drivers is to leave the chip untouched by
> default, assuming that the hardware defaults or firmware/BIOS settings
> are OK. Here some configuration bits are even set to 0 simply because
> simple register writes are used instead of read-modify-write cycles.
> I'm not even sure if this is intended.
>
>> Signed-off-by: Chris D Schimp <silverchris@gmail.com>
>>
>> ---
>> diff -uprN -X vanilla/linux-3.2.5/Documentation/dontdiff
>> vanilla/linux-3.2.5/drivers/hwmon/max6639.c
>> devel/linux-3.2.5/drivers/hwmon/max6639.c
>> --- vanilla/linux-3.2.5/drivers/hwmon/max6639.c       2012-02-06
>> 12:47:00.000000000 -0500
>> +++ devel/linux-3.2.5/drivers/hwmon/max6639.c 2012-02-11
>> 21:40:02.399127171 -0500
>> @@ -439,25 +439,24 @@ static int max6639_init_client(struct i2
>>       if (err)
>>               goto exit;
>>
>> -     /* Fans pulse per revolution is 2 by default */
>> -     if (max6639_info && max6639_info->ppr > 0 &&
>> -                     max6639_info->ppr < 5)
>> -             data->ppr = max6639_info->ppr;
>> -     else
>> -             data->ppr = 2;
>> -     data->ppr -= 1;
>> -     err = i2c_smbus_write_byte_data(client,
>> -                     MAX6639_REG_FAN_PPR(i),
>> -                     data->ppr << 5);
>> -     if (err)
>> -             goto exit;
>> -
>>       if (max6639_info)
>>               rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
>>       data->rpm_range = rpm_range;
>>
>>       for (i = 0; i < 2; i++) {
>>
>> +             /* Fans pulse per revolution is 2 by default */
>> +             if (max6639_info && max6639_info->ppr > 0 &&
>> +                             max6639_info->ppr < 5)
>> +                     data->ppr = max6639_info->ppr;
>> +             else
>> +                     data->ppr = 2;
>> +             data->ppr -= 1;
>> +             err = i2c_smbus_write_byte_data(client,
>> +                     MAX6639_REG_FAN_PPR(i),
>> +                     data->ppr << 5);
>
> I think there is a second bug here. According to the datasheet the
> shift should be 6 not 5. I'm not sure how this managed to go unnoticed
> so far. Maybe there is another bug somewhere in the driver? I see that
> FAN_FROM_REG() takes data->ppr as a parameter, which it should not
> according to the datasheet. If the PPR setting is set correctly for the
> fan being used, then it doesn't show in the formula:
>
> Tachometer count value = internal clock frequency * 60 / actual RPM
>
> where internal clock frequency = fan rpm range / 2, so:
>
> Actual RPM = fan rpm range * 30 / tachometer count value
>
>> +             if (err)
>> +                     goto exit;
>>               /* Fans config PWM, RPM */
>>               err = i2c_smbus_write_byte_data(client,
>>                       MAX6639_REG_FAN_CONFIG1(i),
>
> Assuming I am correct, can you please send two patches, one clearing
> the initialization of i and moving the write to MAX6639_REG_FAN_PPR(i)
> inside the loop, and a second one fixing the bit shift and the
> definition of FAN_FROM_REG?
>
> Note that data->ppr will be unused at this point, so it might as well
> be dropped, a local variable will be enough.
>
> 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] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Jean Delvare]

Hi Chris,

On Mon, 13 Feb 2012 00:53:42 -0500, Chris wrote:
> Fixed RPM calculations to not use ppr, and to be closer to what the
> data sheet specifies.
> Signed-off-by: Chris D Schimp <silverchris@gmail.com>
> ---
> You would be very correct about the second bug. I have fixed that bug,
> and removed the use of ppr in this patch attached. I have also changed
> the behavior of the driver as to not change or initialize any
> settings, so that it will leave all the bios or default settings in
> place and clean up the things left over.
> 
> 
> diff -uprN -X linux-3.2.5/Documentation/dontdiff
> old/linux-3.2.5/drivers/hwmon/max6639.c
> new/linux-3.2.5/drivers/hwmon/max6639.c
> --- old/linux-3.2.5/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
> +++ new/linux-3.2.5/drivers/hwmon/max6639.c	2012-02-12 23:16:21.770059276 -0500

This isn't a -p1 patch. Please send only -p1 patches.

> @@ -72,8 +72,8 @@ static unsigned short normal_i2c[] = { 0
> 
>  static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
> 
> -#define FAN_FROM_REG(val, div, rpm_range)	((val) = 0 ? -1 : \
> -	(val) = 255 ? 0 : (rpm_ranges[rpm_range] * 30) / ((div + 1) * (val)))
> +#define FAN_FROM_REG(val, rpm_range)	((val) = 0 ? -1 : \
> +	(val) = 255 ? 0 : (rpm_ranges[rpm_range] * 30) / val)
>  #define TEMP_LIMIT_TO_REG(val)	SENSORS_LIMIT((val) / 1000, 0, 255)
> 
>  /*
> @@ -333,7 +334,7 @@ static ssize_t show_fan_input(struct dev
>  		return PTR_ERR(data);
> 
>  	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
> -		       data->ppr, data->rpm_range));
> +		       data->rpm_range[attr->index]));

This change looks wrong and produces the following error at build
time:

drivers/hwmon/max6639.c: In function 'show_fan_input':
drivers/hwmon/max6639.c:335:30: error: subscripted value is neither array nor pointer nor vector

Care to build-test your patches before you send them?

-- 
Jean Delvare

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Jean Delvare]

On Mon, 13 Feb 2012 00:56:00 -0500, Chris wrote:
> Removed initialization to leave bios or hardware defaults alone.

Err, this is going too far. The original author of the code obviously
needed the driver to initialize the chip, otherwise he wouldn't have
defined a platform data for this purpose. Killing the initialization
function altogether makes no sense. All I said was that it seemed
curious to do it all _by default_.

Anyway, this is really a secondary issue at this point. The main
problems is the two functional bugs we spotted in the driver. We should
fix them first, and then we can discuss the rest if there is an
interest. I'm not using this driver and I don't know who is in
practice, so I don't care that much.

Please resubmit a (-p1) patch fixing the issue your originally spotted
instead.

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] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Guenter Roeck]

On Thu, Feb 16, 2012 at 04:29:22PM -0500, Jean Delvare wrote:
> On Mon, 13 Feb 2012 00:56:00 -0500, Chris wrote:
> > Removed initialization to leave bios or hardware defaults alone.
> 
> Err, this is going too far. The original author of the code obviously
> needed the driver to initialize the chip, otherwise he wouldn't have
> defined a platform data for this purpose. Killing the initialization
> function altogether makes no sense. All I said was that it seemed
> curious to do it all _by default_.
> 
> Anyway, this is really a secondary issue at this point. The main
> problems is the two functional bugs we spotted in the driver. We should
> fix them first, and then we can discuss the rest if there is an
> interest. I'm not using this driver and I don't know who is in
> practice, so I don't care that much.
> 
> Please resubmit a (-p1) patch fixing the issue your originally spotted
> instead.
> 
We really need input from Roland on the initialization problem.
Might make sense to kwwp him copied on this exchange.

Guenter

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Roland Stigge]

Hi!

On 02/20/2012 06:39 PM, Guenter Roeck wrote:
>> Please resubmit a (-p1) patch fixing the issue your originally spotted
>> instead.
>>
> We really need input from Roland on the initialization problem.
> Might make sense to kwwp him copied on this exchange.

Thanks for your notification and sorry for the delay!

Unfortunately, when I ported the driver from the other original author,
I kept the initialization procedure which is obviously wrong, doing
initialization only for one channel. (I adjusted the driver locally
platform-dependent, so didn't find a chance for mainline integration and
this way, the obvious problems in the mainline driver slipped.)

Therefore, a fix for doing this for both channels, possibly in a loop,
would be good, IMO.

Jean's note about the broken variable initialization is correct. Should
have done this differently.

The other note about initialization only with platform_data is also a
good idea.

I'm using the chip on a custom ARM board without BIOS initialization,
but providing platform_data in this case should be the correct way, anyway.

So Chris, if you are already at it, do it this way. Otherwise please
notify me and I can prepare patches.

Thanks for your work!

Roland

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Chris]

Patch to fix PPR register initialization to set both channels
Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>
---
diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/max6639.c
b/drivers/hwmon/max6639.c
--- a/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
+++ b/drivers/hwmon/max6639.c	2012-02-20 16:36:02.553668023 -0500
@@ -438,7 +438,6 @@ static int max6639_init_client(struct i2
 				  MAX6639_GCONFIG_POR);
 	if (err)
 		goto exit;
-
 	/* Fans pulse per revolution is 2 by default */
 	if (max6639_info && max6639_info->ppr > 0 &&
 			max6639_info->ppr < 5)
@@ -446,11 +445,6 @@ static int max6639_init_client(struct i2
 	else
 		data->ppr = 2;
 	data->ppr -= 1;
-	err = i2c_smbus_write_byte_data(client,
-			MAX6639_REG_FAN_PPR(i),
-			data->ppr << 5);
-	if (err)
-		goto exit;

 	if (max6639_info)
 		rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
@@ -458,6 +452,13 @@ static int max6639_init_client(struct i2

 	for (i = 0; i < 2; i++) {

+		/* Set Fan pulse per revolution */
+		err = i2c_smbus_write_byte_data(client,
+				MAX6639_REG_FAN_PPR(i),
+				data->ppr << 6);
+		if (err)
+			goto exit;
+
 		/* Fans config PWM, RPM */
 		err = i2c_smbus_write_byte_data(client,
 			MAX6639_REG_FAN_CONFIG1(i),

On Mon, Feb 20, 2012 at 1:58 PM, Roland Stigge <stigge@antcom.de> wrote:
> Hi!
>
> On 02/20/2012 06:39 PM, Guenter Roeck wrote:
>>> Please resubmit a (-p1) patch fixing the issue your originally spotted
>>> instead.
>>>
>> We really need input from Roland on the initialization problem.
>> Might make sense to kwwp him copied on this exchange.
>
> Thanks for your notification and sorry for the delay!
>
> Unfortunately, when I ported the driver from the other original author,
> I kept the initialization procedure which is obviously wrong, doing
> initialization only for one channel. (I adjusted the driver locally
> platform-dependent, so didn't find a chance for mainline integration and
> this way, the obvious problems in the mainline driver slipped.)
>
> Therefore, a fix for doing this for both channels, possibly in a loop,
> would be good, IMO.
>
> Jean's note about the broken variable initialization is correct. Should
> have done this differently.
>
> The other note about initialization only with platform_data is also a
> good idea.
>
> I'm using the chip on a custom ARM board without BIOS initialization,
> but providing platform_data in this case should be the correct way, anyway.
>
> So Chris, if you are already at it, do it this way. Otherwise please
> notify me and I can prepare patches.
>
> Thanks for your work!
>
> Roland

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Chris]

Patch to fix FAN_FROM_REG calculations
Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>
---

diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/max6639.c
b/drivers/hwmon/max6639.c
--- a/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
+++ b/drivers/hwmon/max6639.c	2012-02-20 16:36:02.553668023 -0500
@@ -72,8 +72,8 @@ static unsigned short normal_i2c[] = { 0

 static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };

-#define FAN_FROM_REG(val, div, rpm_range)	((val) = 0 ? -1 : \
-	(val) = 255 ? 0 : (rpm_ranges[rpm_range] * 30) / ((div + 1) * (val)))
+#define FAN_FROM_REG(val, rpm_range)	((val) = 0 ? -1 : \
+	(val) = 255 ? 0 : (rpm_ranges[rpm_range] * 30) / val)
 #define TEMP_LIMIT_TO_REG(val)	SENSORS_LIMIT((val) / 1000, 0, 255)

 /*
@@ -333,7 +333,7 @@ static ssize_t show_fan_input(struct dev
 		return PTR_ERR(data);

 	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
-		       data->ppr, data->rpm_range));
+		       data->rpm_range));
 }

 static ssize_t show_alarm(struct device *dev,


On Mon, Feb 20, 2012 at 1:58 PM, Roland Stigge <stigge@antcom.de> wrote:
> Hi!
>
> On 02/20/2012 06:39 PM, Guenter Roeck wrote:
>>> Please resubmit a (-p1) patch fixing the issue your originally spotted
>>> instead.
>>>
>> We really need input from Roland on the initialization problem.
>> Might make sense to kwwp him copied on this exchange.
>
> Thanks for your notification and sorry for the delay!
>
> Unfortunately, when I ported the driver from the other original author,
> I kept the initialization procedure which is obviously wrong, doing
> initialization only for one channel. (I adjusted the driver locally
> platform-dependent, so didn't find a chance for mainline integration and
> this way, the obvious problems in the mainline driver slipped.)
>
> Therefore, a fix for doing this for both channels, possibly in a loop,
> would be good, IMO.
>
> Jean's note about the broken variable initialization is correct. Should
> have done this differently.
>
> The other note about initialization only with platform_data is also a
> good idea.
>
> I'm using the chip on a custom ARM board without BIOS initialization,
> but providing platform_data in this case should be the correct way, anyway.
>
> So Chris, if you are already at it, do it this way. Otherwise please
> notify me and I can prepare patches.
>
> Thanks for your work!
>
> Roland

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Chris]

The patches I just submitted fix the main issues with the driver on my
system. I didn't fix the variable initialization because it seems to
generate a warning when ever I move them closer to were they belong
in, it generates a compiler warning of "warning: ISO C90 forbids mixed
declarations and code". I am looking into how platform_data works to
see how it works to see if I can handle checking it and changing the
chip initialization to work with platform data if its provided and to
leave it alone if its not. Thanks for the patience with my patch
submissions, I am learning a lot from this

On Mon, Feb 20, 2012 at 1:58 PM, Roland Stigge <stigge@antcom.de> wrote:
> Hi!
>
> On 02/20/2012 06:39 PM, Guenter Roeck wrote:
>>> Please resubmit a (-p1) patch fixing the issue your originally spotted
>>> instead.
>>>
>> We really need input from Roland on the initialization problem.
>> Might make sense to kwwp him copied on this exchange.
>
> Thanks for your notification and sorry for the delay!
>
> Unfortunately, when I ported the driver from the other original author,
> I kept the initialization procedure which is obviously wrong, doing
> initialization only for one channel. (I adjusted the driver locally
> platform-dependent, so didn't find a chance for mainline integration and
> this way, the obvious problems in the mainline driver slipped.)
>
> Therefore, a fix for doing this for both channels, possibly in a loop,
> would be good, IMO.
>
> Jean's note about the broken variable initialization is correct. Should
> have done this differently.
>
> The other note about initialization only with platform_data is also a
> good idea.
>
> I'm using the chip on a custom ARM board without BIOS initialization,
> but providing platform_data in this case should be the correct way, anyway.
>
> So Chris, if you are already at it, do it this way. Otherwise please
> notify me and I can prepare patches.
>
> Thanks for your work!
>
> Roland

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Guenter Roeck]

On Mon, 2012-02-20 at 16:58 -0500, Chris wrote:
> Patch to fix PPR register initialization to set both channels
> Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>
> ---
> diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/max6639.c
> b/drivers/hwmon/max6639.c
> --- a/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
> +++ b/drivers/hwmon/max6639.c	2012-02-20 16:36:02.553668023 -0500
> @@ -438,7 +438,6 @@ static int max6639_init_client(struct i2
>  				  MAX6639_GCONFIG_POR);
>  	if (err)
>  		goto exit;
> -

Unnecessary whitespace change.

>  	/* Fans pulse per revolution is 2 by default */
>  	if (max6639_info && max6639_info->ppr > 0 &&
>  			max6639_info->ppr < 5)
> @@ -446,11 +445,6 @@ static int max6639_init_client(struct i2
>  	else
>  		data->ppr = 2;
>  	data->ppr -= 1;
> -	err = i2c_smbus_write_byte_data(client,
> -			MAX6639_REG_FAN_PPR(i),
> -			data->ppr << 5);
> -	if (err)
> -		goto exit;
> 
>  	if (max6639_info)
>  		rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
> @@ -458,6 +452,13 @@ static int max6639_init_client(struct i2
> 
>  	for (i = 0; i < 2; i++) {
> 
> +		/* Set Fan pulse per revolution */
> +		err = i2c_smbus_write_byte_data(client,
> +				MAX6639_REG_FAN_PPR(i),
> +				data->ppr << 6);

Just asking ... is this correct ? Original code is << 5.

> +		if (err)
> +			goto exit;
> +
>  		/* Fans config PWM, RPM */
>  		err = i2c_smbus_write_byte_data(client,
>  			MAX6639_REG_FAN_CONFIG1(i),
> 
> On Mon, Feb 20, 2012 at 1:58 PM, Roland Stigge <stigge@antcom.de> wrote:
> > Hi!
> >
> > On 02/20/2012 06:39 PM, Guenter Roeck wrote:
> >>> Please resubmit a (-p1) patch fixing the issue your originally spotted
> >>> instead.
> >>>
> >> We really need input from Roland on the initialization problem.
> >> Might make sense to kwwp him copied on this exchange.
> >
> > Thanks for your notification and sorry for the delay!
> >
> > Unfortunately, when I ported the driver from the other original author,
> > I kept the initialization procedure which is obviously wrong, doing
> > initialization only for one channel. (I adjusted the driver locally
> > platform-dependent, so didn't find a chance for mainline integration and
> > this way, the obvious problems in the mainline driver slipped.)
> >
> > Therefore, a fix for doing this for both channels, possibly in a loop,
> > would be good, IMO.
> >
> > Jean's note about the broken variable initialization is correct. Should
> > have done this differently.
> >
> > The other note about initialization only with platform_data is also a
> > good idea.
> >
> > I'm using the chip on a custom ARM board without BIOS initialization,
> > but providing platform_data in this case should be the correct way, anyway.
> >
> > So Chris, if you are already at it, do it this way. Otherwise please
> > notify me and I can prepare patches.
> >
> > Thanks for your work!
> >
> > Roland



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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Guenter Roeck]

On Mon, 2012-02-20 at 17:14 -0500, Chris wrote:
> The patches I just submitted fix the main issues with the driver on my
> system. I didn't fix the variable initialization because it seems to
> generate a warning when ever I move them closer to were they belong
> in, it generates a compiler warning of "warning: ISO C90 forbids mixed
> declarations and code". I am looking into how platform_data works to
> see how it works to see if I can handle checking it and changing the
> chip initialization to work with platform data if its provided and to
> leave it alone if its not. Thanks for the patience with my patch
> submissions, I am learning a lot from this
> 

The initializations are not needed.

	s/int i = 0/int i/
	s/int err = 0/int err/

should do it.

Guenter

> On Mon, Feb 20, 2012 at 1:58 PM, Roland Stigge <stigge@antcom.de> wrote:
> > Hi!
> >
> > On 02/20/2012 06:39 PM, Guenter Roeck wrote:
> >>> Please resubmit a (-p1) patch fixing the issue your originally spotted
> >>> instead.
> >>>
> >> We really need input from Roland on the initialization problem.
> >> Might make sense to kwwp him copied on this exchange.
> >
> > Thanks for your notification and sorry for the delay!
> >
> > Unfortunately, when I ported the driver from the other original author,
> > I kept the initialization procedure which is obviously wrong, doing
> > initialization only for one channel. (I adjusted the driver locally
> > platform-dependent, so didn't find a chance for mainline integration and
> > this way, the obvious problems in the mainline driver slipped.)
> >
> > Therefore, a fix for doing this for both channels, possibly in a loop,
> > would be good, IMO.
> >
> > Jean's note about the broken variable initialization is correct. Should
> > have done this differently.
> >
> > The other note about initialization only with platform_data is also a
> > good idea.
> >
> > I'm using the chip on a custom ARM board without BIOS initialization,
> > but providing platform_data in this case should be the correct way, anyway.
> >
> > So Chris, if you are already at it, do it this way. Otherwise please
> > notify me and I can prepare patches.
> >
> > Thanks for your work!
> >
> > Roland



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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Chris]

Fix variable intializations in max6639_init_client
Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>
---

diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/max6639.c
b/drivers/hwmon/max6639.c
--- a/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
+++ b/drivers/hwmon/max6639.c	2012-02-20 17:40:36.821422457 -0500
@@ -429,16 +429,15 @@ static int max6639_init_client(struct i2
 	struct max6639_data *data = i2c_get_clientdata(client);
 	struct max6639_platform_data *max6639_info =
 		client->dev.platform_data;
-	int i = 0;
+	int i;
 	int rpm_range = 1; /* default: 4000 RPM */
-	int err = 0;
+	int err;

 	/* Reset chip to default values, see below for GCONFIG setup */
 	err = i2c_smbus_write_byte_data(client, MAX6639_REG_GCONFIG,
 				  MAX6639_GCONFIG_POR);
 	if (err)
 		goto exit;
-
 	/* Fans pulse per revolution is 2 by default */
 	if (max6639_info && max6639_info->ppr > 0 &&
 			max6639_info->ppr < 5)

On Mon, Feb 20, 2012 at 5:31 PM, Guenter Roeck
<guenter.roeck@ericsson.com> wrote:
> On Mon, 2012-02-20 at 17:14 -0500, Chris wrote:
>> The patches I just submitted fix the main issues with the driver on my
>> system. I didn't fix the variable initialization because it seems to
>> generate a warning when ever I move them closer to were they belong
>> in, it generates a compiler warning of "warning: ISO C90 forbids mixed
>> declarations and code". I am looking into how platform_data works to
>> see how it works to see if I can handle checking it and changing the
>> chip initialization to work with platform data if its provided and to
>> leave it alone if its not. Thanks for the patience with my patch
>> submissions, I am learning a lot from this
>>
>
> The initializations are not needed.
>
>        s/int i = 0/int i/
>        s/int err = 0/int err/
>
> should do it.
>
> Guenter
>
>> On Mon, Feb 20, 2012 at 1:58 PM, Roland Stigge <stigge@antcom.de> wrote:
>> > Hi!
>> >
>> > On 02/20/2012 06:39 PM, Guenter Roeck wrote:
>> >>> Please resubmit a (-p1) patch fixing the issue your originally spotted
>> >>> instead.
>> >>>
>> >> We really need input from Roland on the initialization problem.
>> >> Might make sense to kwwp him copied on this exchange.
>> >
>> > Thanks for your notification and sorry for the delay!
>> >
>> > Unfortunately, when I ported the driver from the other original author,
>> > I kept the initialization procedure which is obviously wrong, doing
>> > initialization only for one channel. (I adjusted the driver locally
>> > platform-dependent, so didn't find a chance for mainline integration and
>> > this way, the obvious problems in the mainline driver slipped.)
>> >
>> > Therefore, a fix for doing this for both channels, possibly in a loop,
>> > would be good, IMO.
>> >
>> > Jean's note about the broken variable initialization is correct. Should
>> > have done this differently.
>> >
>> > The other note about initialization only with platform_data is also a
>> > good idea.
>> >
>> > I'm using the chip on a custom ARM board without BIOS initialization,
>> > but providing platform_data in this case should be the correct way, anyway.
>> >
>> > So Chris, if you are already at it, do it this way. Otherwise please
>> > notify me and I can prepare patches.
>> >
>> > Thanks for your work!
>> >
>> > Roland
>
>

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Roland Stigge]

On 20/02/12 22:58, Chris wrote:
> Patch to fix PPR register initialization to set both channels
> Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>

Looks good.

Acked-by: Roland Stigge <stigge@antcom.de>

> ---
> diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/max6639.c
> b/drivers/hwmon/max6639.c
> --- a/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
> +++ b/drivers/hwmon/max6639.c	2012-02-20 16:36:02.553668023 -0500
> @@ -438,7 +438,6 @@ static int max6639_init_client(struct i2
>  				  MAX6639_GCONFIG_POR);
>  	if (err)
>  		goto exit;
> -
>  	/* Fans pulse per revolution is 2 by default */
>  	if (max6639_info && max6639_info->ppr > 0 &&
>  			max6639_info->ppr < 5)
> @@ -446,11 +445,6 @@ static int max6639_init_client(struct i2
>  	else
>  		data->ppr = 2;
>  	data->ppr -= 1;
> -	err = i2c_smbus_write_byte_data(client,
> -			MAX6639_REG_FAN_PPR(i),
> -			data->ppr << 5);
> -	if (err)
> -		goto exit;
> 
>  	if (max6639_info)
>  		rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
> @@ -458,6 +452,13 @@ static int max6639_init_client(struct i2
> 
>  	for (i = 0; i < 2; i++) {
> 
> +		/* Set Fan pulse per revolution */
> +		err = i2c_smbus_write_byte_data(client,
> +				MAX6639_REG_FAN_PPR(i),
> +				data->ppr << 6);
> +		if (err)
> +			goto exit;
> +
>  		/* Fans config PWM, RPM */
>  		err = i2c_smbus_write_byte_data(client,
>  			MAX6639_REG_FAN_CONFIG1(i),

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Roland Stigge]

On 20/02/12 22:59, Chris wrote:
> Patch to fix FAN_FROM_REG calculations
> Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>

Thanks for your work!

Acked-by: Roland Stigge <stigge@antcom.de>

> ---
> 
> diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/max6639.c
> b/drivers/hwmon/max6639.c
> --- a/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
> +++ b/drivers/hwmon/max6639.c	2012-02-20 16:36:02.553668023 -0500
> @@ -72,8 +72,8 @@ static unsigned short normal_i2c[] = { 0
> 
>  static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
> 
> -#define FAN_FROM_REG(val, div, rpm_range)	((val) = 0 ? -1 : \
> -	(val) = 255 ? 0 : (rpm_ranges[rpm_range] * 30) / ((div + 1) * (val)))
> +#define FAN_FROM_REG(val, rpm_range)	((val) = 0 ? -1 : \
> +	(val) = 255 ? 0 : (rpm_ranges[rpm_range] * 30) / val)
>  #define TEMP_LIMIT_TO_REG(val)	SENSORS_LIMIT((val) / 1000, 0, 255)
> 
>  /*
> @@ -333,7 +333,7 @@ static ssize_t show_fan_input(struct dev
>  		return PTR_ERR(data);
> 
>  	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
> -		       data->ppr, data->rpm_range));
> +		       data->rpm_range));
>  }
> 
>  static ssize_t show_alarm(struct device *dev,

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Roland Stigge]

On 20/02/12 23:44, Chris wrote:
> Fix variable intializations in max6639_init_client
> Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>

Also looks good.

Acked-by: Roland Stigge <stigge@antcom.de>

> ---
> 
> diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/max6639.c
> b/drivers/hwmon/max6639.c
> --- a/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
> +++ b/drivers/hwmon/max6639.c	2012-02-20 17:40:36.821422457 -0500
> @@ -429,16 +429,15 @@ static int max6639_init_client(struct i2
>  	struct max6639_data *data = i2c_get_clientdata(client);
>  	struct max6639_platform_data *max6639_info >  		client->dev.platform_data;
> -	int i = 0;
> +	int i;
>  	int rpm_range = 1; /* default: 4000 RPM */
> -	int err = 0;
> +	int err;
> 
>  	/* Reset chip to default values, see below for GCONFIG setup */
>  	err = i2c_smbus_write_byte_data(client, MAX6639_REG_GCONFIG,
>  				  MAX6639_GCONFIG_POR);
>  	if (err)
>  		goto exit;
> -
>  	/* Fans pulse per revolution is 2 by default */
>  	if (max6639_info && max6639_info->ppr > 0 &&
>  			max6639_info->ppr < 5)
> 

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Guenter Roeck]

On Mon, Feb 20, 2012 at 05:44:59PM -0500, Chris wrote:
> Fix variable intializations in max6639_init_client
> Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>
> ---
> 
Can we somehow reach an agreement that patches are sent as patches and not in reply
to other e-mail or patches ? I for my part all but lost track about which e-mail
fixes which problem, since all have the same headline.

> diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/max6639.c
> b/drivers/hwmon/max6639.c
> --- a/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
> +++ b/drivers/hwmon/max6639.c	2012-02-20 17:40:36.821422457 -0500
> @@ -429,16 +429,15 @@ static int max6639_init_client(struct i2
>  	struct max6639_data *data = i2c_get_clientdata(client);
>  	struct max6639_platform_data *max6639_info =
>  		client->dev.platform_data;
> -	int i = 0;
> +	int i;
>  	int rpm_range = 1; /* default: 4000 RPM */
> -	int err = 0;
> +	int err;
> 
>  	/* Reset chip to default values, see below for GCONFIG setup */
>  	err = i2c_smbus_write_byte_data(client, MAX6639_REG_GCONFIG,
>  				  MAX6639_GCONFIG_POR);
>  	if (err)
>  		goto exit;
> -

... and I think I have seen this unnecessary line removal several times now,
suggesting that the patch won't apply in any sequence.

Please, do some homework. Applying this series of patches will/would require
a lot of unnecessary extra work from either me or Jean.

Guenter

>  	/* Fans pulse per revolution is 2 by default */
>  	if (max6639_info && max6639_info->ppr > 0 &&
>  			max6639_info->ppr < 5)
> 
> On Mon, Feb 20, 2012 at 5:31 PM, Guenter Roeck
> <guenter.roeck@ericsson.com> wrote:
> > On Mon, 2012-02-20 at 17:14 -0500, Chris wrote:
> >> The patches I just submitted fix the main issues with the driver on my
> >> system. I didn't fix the variable initialization because it seems to
> >> generate a warning when ever I move them closer to were they belong
> >> in, it generates a compiler warning of "warning: ISO C90 forbids mixed
> >> declarations and code". I am looking into how platform_data works to
> >> see how it works to see if I can handle checking it and changing the
> >> chip initialization to work with platform data if its provided and to
> >> leave it alone if its not. Thanks for the patience with my patch
> >> submissions, I am learning a lot from this
> >>
> >
> > The initializations are not needed.
> >
> >        s/int i = 0/int i/
> >        s/int err = 0/int err/
> >
> > should do it.
> >
> > Guenter
> >
> >> On Mon, Feb 20, 2012 at 1:58 PM, Roland Stigge <stigge@antcom.de> wrote:
> >> > Hi!
> >> >
> >> > On 02/20/2012 06:39 PM, Guenter Roeck wrote:
> >> >>> Please resubmit a (-p1) patch fixing the issue your originally spotted
> >> >>> instead.
> >> >>>
> >> >> We really need input from Roland on the initialization problem.
> >> >> Might make sense to kwwp him copied on this exchange.
> >> >
> >> > Thanks for your notification and sorry for the delay!
> >> >
> >> > Unfortunately, when I ported the driver from the other original author,
> >> > I kept the initialization procedure which is obviously wrong, doing
> >> > initialization only for one channel. (I adjusted the driver locally
> >> > platform-dependent, so didn't find a chance for mainline integration and
> >> > this way, the obvious problems in the mainline driver slipped.)
> >> >
> >> > Therefore, a fix for doing this for both channels, possibly in a loop,
> >> > would be good, IMO.
> >> >
> >> > Jean's note about the broken variable initialization is correct. Should
> >> > have done this differently.
> >> >
> >> > The other note about initialization only with platform_data is also a
> >> > good idea.
> >> >
> >> > I'm using the chip on a custom ARM board without BIOS initialization,
> >> > but providing platform_data in this case should be the correct way, anyway.
> >> >
> >> > So Chris, if you are already at it, do it this way. Otherwise please
> >> > notify me and I can prepare patches.
> >> >
> >> > Thanks for your work!
> >> >
> >> > Roland
> >
> >

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Guenter Roeck]

On Mon, Feb 20, 2012 at 05:56:08PM -0500, Roland Stigge wrote:
> On 20/02/12 22:59, Chris wrote:
> > Patch to fix FAN_FROM_REG calculations
> > Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>
> 
> Thanks for your work!
> 
> Acked-by: Roland Stigge <stigge@antcom.de>
> 
Patch #3 under the same headline .. unless I lost count :-(. I actually did
miss the original patch (thinking it was a reply), and only caught this one
since I wondered why you acked the other patch twice.

What is wrong with the original calculation ?

> > ---
> > 
> > diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/max6639.c
> > b/drivers/hwmon/max6639.c
> > --- a/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
> > +++ b/drivers/hwmon/max6639.c	2012-02-20 16:36:02.553668023 -0500
> > @@ -72,8 +72,8 @@ static unsigned short normal_i2c[] = { 0
> > 
> >  static const int rpm_ranges[] = { 2000, 4000, 8000, 16000 };
> > 
> > -#define FAN_FROM_REG(val, div, rpm_range)	((val) = 0 ? -1 : \
> > -	(val) = 255 ? 0 : (rpm_ranges[rpm_range] * 30) / ((div + 1) * (val)))
> > +#define FAN_FROM_REG(val, rpm_range)	((val) = 0 ? -1 : \
> > +	(val) = 255 ? 0 : (rpm_ranges[rpm_range] * 30) / val)
> >  #define TEMP_LIMIT_TO_REG(val)	SENSORS_LIMIT((val) / 1000, 0, 255)
> > 
> >  /*
> > @@ -333,7 +333,7 @@ static ssize_t show_fan_input(struct dev
> >  		return PTR_ERR(data);
> > 
> >  	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
> > -		       data->ppr, data->rpm_range));
> > +		       data->rpm_range));

This is a bit problematic. Always was, actually. If val=0 it returns a fan speed of -1,
which does not make much sense. It should either return 0, or some kind of error.

> >  }
> > 
> >  static ssize_t show_alarm(struct device *dev,

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Guenter Roeck]

On Mon, Feb 20, 2012 at 05:53:16PM -0500, Roland Stigge wrote:
> On 20/02/12 22:58, Chris wrote:
> > Patch to fix PPR register initialization to set both channels
> > Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>
> 
> Looks good.
> 
> Acked-by: Roland Stigge <stigge@antcom.de>
> 
So << 6 instead of << 5 is ok ? 

Thanks,
Guenter

> > ---
> > diff -uprN -X a/Documentation/dontdiff a/drivers/hwmon/max6639.c
> > b/drivers/hwmon/max6639.c
> > --- a/drivers/hwmon/max6639.c	2012-02-06 12:47:00.000000000 -0500
> > +++ b/drivers/hwmon/max6639.c	2012-02-20 16:36:02.553668023 -0500
> > @@ -438,7 +438,6 @@ static int max6639_init_client(struct i2
> >  				  MAX6639_GCONFIG_POR);
> >  	if (err)
> >  		goto exit;
> > -
> >  	/* Fans pulse per revolution is 2 by default */
> >  	if (max6639_info && max6639_info->ppr > 0 &&
> >  			max6639_info->ppr < 5)
> > @@ -446,11 +445,6 @@ static int max6639_init_client(struct i2
> >  	else
> >  		data->ppr = 2;
> >  	data->ppr -= 1;
> > -	err = i2c_smbus_write_byte_data(client,
> > -			MAX6639_REG_FAN_PPR(i),
> > -			data->ppr << 5);
> > -	if (err)
> > -		goto exit;
> > 
> >  	if (max6639_info)
> >  		rpm_range = rpm_range_to_reg(max6639_info->rpm_range);
> > @@ -458,6 +452,13 @@ static int max6639_init_client(struct i2
> > 
> >  	for (i = 0; i < 2; i++) {
> > 
> > +		/* Set Fan pulse per revolution */
> > +		err = i2c_smbus_write_byte_data(client,
> > +				MAX6639_REG_FAN_PPR(i),
> > +				data->ppr << 6);
> > +		if (err)
> > +			goto exit;
> > +
> >  		/* Fans config PWM, RPM */
> >  		err = i2c_smbus_write_byte_data(client,
> >  			MAX6639_REG_FAN_CONFIG1(i),

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Roland Stigge]

On 02/21/2012 12:58 AM, Guenter Roeck wrote:
> On Mon, Feb 20, 2012 at 05:53:16PM -0500, Roland Stigge wrote:
>> On 20/02/12 22:58, Chris wrote:
>>> Patch to fix PPR register initialization to set both channels
>>> Signed-off-by: Chris D Schimp <silverchris <at> gmail.com>
>>
>> Looks good.
>>
>> Acked-by: Roland Stigge <stigge@antcom.de>
>>
> So << 6 instead of << 5 is ok ?

Yes, see datasheet, Register Map, Reg. 24h+25h. It's the upper 2 bits of
the 8 bit register for the 2 bits of PPR.

Further, I agree that the patch style should be improved before submitting.

Thanks,

Roland

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

Re: [lm-sensors] [Patch] hwmon: (max6639) Set Pulse per revolution loop for both channels

[Roland Stigge]

Hi,

On 02/21/2012 12:57 AM, Guenter Roeck wrote:
> Patch #3 under the same headline .. unless I lost count :-(. I actually did
> miss the original patch (thinking it was a reply), and only caught this one
> since I wondered why you acked the other patch twice.
> 
> What is wrong with the original calculation ?

The original speed/RPM calculation included data->ppr in a wrong way. I
also needed to re-read the datasheet. See also "Table 7,
RPM-to-Tachometer Count Relationship Examples" and the printed formula
where the actual PPR value is removed from by "selected number of pulses
per revolution / actual fan pulses".


>>> -#define FAN_FROM_REG(val, div, rpm_range)	((val) = 0 ? -1 : \
>>> -	(val) = 255 ? 0 : (rpm_ranges[rpm_range] * 30) / ((div + 1) * (val)))
>>> +#define FAN_FROM_REG(val, rpm_range)	((val) = 0 ? -1 : \
>>> +	(val) = 255 ? 0 : (rpm_ranges[rpm_range] * 30) / val)
>>>  #define TEMP_LIMIT_TO_REG(val)	SENSORS_LIMIT((val) / 1000, 0, 255)
>>>
>>>  /*
>>> @@ -333,7 +333,7 @@ static ssize_t show_fan_input(struct dev
>>>  		return PTR_ERR(data);
>>>
>>>  	return sprintf(buf, "%d\n", FAN_FROM_REG(data->fan[attr->index],
>>> -		       data->ppr, data->rpm_range));
>>> +		       data->rpm_range));
> 
> This is a bit problematic. Always was, actually. If val=0 it returns a fan speed of -1,
> which does not make much sense. It should either return 0, or some kind of error.

Agreed!

Roland

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