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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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@xxxxxxxxxxxx> 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@xxxxxxxxx>
>>
>> ---
>> 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@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors



[Video for Linux]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Photos]     [Yosemite Photos]     [Free Singles Community]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]

Add to Google Powered by Linux