Hi Bjoern,
On Fri, Dec 02, 2011 at 05:09:36PM -0500, Bjoern Gerhart wrote:
> Hi Guenter,
>
> the mainboard I tested the module on is an Adlink NanoX-TC. However,
> please note that the f75387 chip is _not_ assembled on this board, but
> on the connected proprietary Interface Board developed by our inhouse
> hardware team...
>
> Thanks so much for the patch review and feedback, which sounds really
> plausible! Below you find the updated patch implementing the
> proposals.
>
Yes, that is much better. Couple of minor comments below.
> 2011/12/2 Guenter Roeck <guenter.roeck@xxxxxxxxxxxx>:
> > On Thu, Dec 01, 2011 at 10:37:47PM -0500, Guenter Roeck wrote:
> > Configuration register 0x01 and fan register 0x60 are both different. Both are used
> > and needed for fan configuration, so that is definitely a problem that will have
> > to be addressed.
> >
> Ok... so I'll have a look at the difference in the fan related
> registers in order to complete the f75387 implementation.
>
Yes, we'll need that. Fortunately you have your own board, so hopefully you should
be able to play with fan control even if the pins are not connected.
> --
> Bjoern Gerhart
>
>
> diff -uNr linux-2.6.39-orig/drivers/hwmon/f75375s.c
> linux-2.6.39/drivers/hwmon/f75375s.c
> --- linux-2.6.39-orig/drivers/hwmon/f75375s.c 2011-12-02
> 14:41:16.000000000 +0100
> +++ linux-2.6.39/drivers/hwmon/f75375s.c 2011-12-02 14:48:48.000000000 +0100
> @@ -1,6 +1,6 @@
> /*
> - * f75375s.c - driver for the Fintek F75375/SP and F75373
> - * hardware monitoring features
> + * f75375s.c - driver for the Fintek F75375/SP, F75373 and
> + * F75387SG/RG hardware monitoring features
> * Copyright (C) 2006-2007 Riku Voipio
> *
> * Datasheets available at:
> @@ -10,6 +10,9 @@
> *
> * f75373:
> * http://www.fintek.com.tw/files/productfiles/F75373_V025P.pdf
> + *
> + * f75387:
> + * http://www.fintek.com.tw/files/productfiles/F75387_V027P.pdf
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -40,7 +43,7 @@
> /* Addresses to scan */
> static const unsigned short normal_i2c[] = { 0x2d, 0x2e, I2C_CLIENT_END };
>
> -enum chips { f75373, f75375 };
> +enum chips { f75373, f75375, f75387 };
>
> /* Fintek F75375 registers */
> #define F75375_REG_CONFIG0 0x0
> @@ -59,6 +62,7 @@
> #define F75375_REG_VOLT_LOW(nr) (0x21 + (nr) * 2)
>
> #define F75375_REG_TEMP(nr) (0x14 + (nr))
> +#define F75387_REG_TEMP11_LSB(nr) (0x14 + (nr) + 6)
You really seem to like that ;). Still, I think it should be
#define F75387_REG_TEMP11_LSB(nr) (0x1a + (nr))
instead.
> #define F75375_REG_TEMP_HIGH(nr) (0x28 + (nr) * 2)
> #define F75375_REG_TEMP_HYST(nr) (0x29 + (nr) * 2)
>
> @@ -108,7 +112,11 @@
> u8 pwm[2];
> u8 pwm_mode[2];
> u8 pwm_enable[2];
> - s8 temp[2];
> + /* f75387: For remote temperature reading, it uses signed 11-bit
> + * values with LSB = 0.125 degree Celsius, left-justified in 16-bit
> + * registers. For original 8bit temp readings, the LSB just is 0.
> + */
Multi-line comment should start with
/*
* f75387: For remote temperature reading, it uses signed 11-bit
> + s16 temp11[2];
> s8 temp_high[2];
> s8 temp_max_hyst[2];
> };
> @@ -122,6 +130,7 @@
> static const struct i2c_device_id f75375_id[] = {
> { "f75373", f75373 },
> { "f75375", f75375 },
> + { "f75387", f75387 },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, f75375_id);
> @@ -205,8 +214,13 @@
> if (time_after(jiffies, data->last_updated + 2 * HZ)
> || !data->valid) {
> for (nr = 0; nr < 2; nr++) {
> - data->temp[nr] =
> - f75375_read8(client, F75375_REG_TEMP(nr));
> + /* assign MSB, therefore shift it by 8 bits */
> + data->temp11[nr] =
> + f75375_read8(client, F75375_REG_TEMP(nr)) << 8;
> + if (data->kind == f75387)
> + /* merge F75387's temperature LSB (11-bit) */
> + data->temp11[nr] |=
> + f75375_read8(client, F75387_REG_TEMP11_LSB(nr));
> data->fan[nr] =
> f75375_read16(client, F75375_REG_FAN(nr));
> }
> @@ -435,13 +449,14 @@
> }
> #define TEMP_FROM_REG(val) ((val) * 1000)
> #define TEMP_TO_REG(val) ((val) / 1000)
> +#define TEMP11_FROM_REG(reg) ((reg) / 32 * 125)
>
> -static ssize_t show_temp(struct device *dev, struct device_attribute *attr,
> +static ssize_t show_temp11(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> int nr = to_sensor_dev_attr(attr)->index;
> struct f75375_data *data = f75375_update_device(dev);
> - return sprintf(buf, "%d\n", TEMP_FROM_REG(data->temp[nr]));
> + return sprintf(buf, "%d\n", TEMP11_FROM_REG(data->temp11[nr]));
> }
>
> static ssize_t show_temp_max(struct device *dev, struct device_attribute *attr,
> @@ -525,12 +540,12 @@
> show_in_max, set_in_max, 3);
> static SENSOR_DEVICE_ATTR(in3_min, S_IRUGO|S_IWUSR,
> show_in_min, set_in_min, 3);
> -static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp, NULL, 0);
> +static SENSOR_DEVICE_ATTR(temp1_input, S_IRUGO, show_temp11, NULL, 0);
> static SENSOR_DEVICE_ATTR(temp1_max_hyst, S_IRUGO|S_IWUSR,
> show_temp_max_hyst, set_temp_max_hyst, 0);
> static SENSOR_DEVICE_ATTR(temp1_max, S_IRUGO|S_IWUSR,
> show_temp_max, set_temp_max, 0);
> -static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp, NULL, 1);
> +static SENSOR_DEVICE_ATTR(temp2_input, S_IRUGO, show_temp11, NULL, 1);
> static SENSOR_DEVICE_ATTR(temp2_max_hyst, S_IRUGO|S_IWUSR,
> show_temp_max_hyst, set_temp_max_hyst, 1);
> static SENSOR_DEVICE_ATTR(temp2_max, S_IRUGO|S_IWUSR,
> @@ -685,10 +700,15 @@
>
> vendid = f75375_read16(client, F75375_REG_VENDOR);
> chipid = f75375_read16(client, F75375_CHIP_ID);
> - if (chipid == 0x0306 && vendid == 0x1934)
> + if (vendid != 0x1934)
> + return -ENODEV;
> +
> + if (chipid == 0x0306)
> name = "f75375";
> - else if (chipid == 0x0204 && vendid == 0x1934)
> + else if (chipid == 0x0204)
> name = "f75373";
> + else if (chipid == 0x0410)
> + name = "f75387";
> else
> return -ENODEV;
>
> @@ -711,7 +731,7 @@
>
> MODULE_AUTHOR("Riku Voipio");
> MODULE_LICENSE("GPL");
> -MODULE_DESCRIPTION("F75373/F75375 hardware monitoring driver");
> +MODULE_DESCRIPTION("F75373/F75375/F75387 hardware monitoring driver");
>
> module_init(sensors_f75375_init);
> module_exit(sensors_f75375_exit);
> diff -uNr linux-2.6.39-orig/drivers/hwmon/Kconfig
> linux-2.6.39/drivers/hwmon/Kconfig
> --- linux-2.6.39-orig/drivers/hwmon/Kconfig 2011-12-02 14:41:28.000000000 +0100
> +++ linux-2.6.39/drivers/hwmon/Kconfig 2011-12-02 13:21:46.000000000 +0100
> @@ -335,11 +335,11 @@
> will be called f71882fg.
>
> config SENSORS_F75375S
> - tristate "Fintek F75375S/SP and F75373"
> + tristate "Fintek F75375S/SP, F75373 and F75387"
> depends on I2C
> help
> If you say yes here you get support for hardware monitoring
> - features of the Fintek F75375S/SP and F75373
> + features of the Fintek F75375S/SP, F75373 and F75387
>
> This driver can also be built as a module. If so, the module
> will be called f75375s.
_______________________________________________
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]
