<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><br>
</p>
<br>
<div class="moz-cite-prefix">On 06/08/17 1:12 AM, Andrew Jeffery
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20170608061249.29233-1-andrew@aj.id.au">
<pre wrap="">Non-standard attributes are an obstacle for userspace. Instead, use the
standard fanX_input attributes, and number the fast-rotor values in
[NR_CHANNEL, 2 * NR_CHANNEL - 1].
Signed-off-by: Andrew Jeffery <a class="moz-txt-link-rfc2396E" href="mailto:andrew@aj.id.au"><andrew@aj.id.au></a>
---
Matt,
I have a couple of observations testing this on a Witherspoon system that has
the necessary modifications:
1. The fast-rotor values are non-zero and vary despite the chassis power being
off (the slow rotor values become non-zero when the chassis is powered-on).</pre>
</blockquote>
I worked with Jordan on this issue and it was something pointed out
to Maxim to fix where we had verified the fix using a sampling of
updated max31785 chips in Oct 2016. He is working with them to find
out if this is a pre-mfg batch we have that's not 0'd out at
poweroff and what the current set of max31785 chips FW should do.
(Along with this, he's again going to request an updated datasheet
for rev 0x3040).<br>
<blockquote type="cite"
cite="mid:20170608061249.29233-1-andrew@aj.id.au">
<pre wrap="">
2. The fast-rotor values seem to be 'sticky' - sometimes they don't vary as
often as I would expect (e.g. in fan ramp- up or down).</pre>
</blockquote>
Not sure what you mean by 'sticky', but during Jordan and I
investigating, we found that the fast rotor 2bytes were reading 0's
during any speed change (up or down) and would only continue
providing the tach feedback once the target speed had been deemed
reached. This is another point Jordan is bringing up to Maxim,
however this should not preclude us from getting these fast rotor
feedbacks enabled as we have application code in place that does not
mark these as faulted due to this.<br>
<blockquote type="cite"
cite="mid:20170608061249.29233-1-andrew@aj.id.au">
<pre wrap="">
2. The fast-rotor attributes appear to correspond to the slow rotor values.</pre>
</blockquote>
What do you mean by attributes here? What were you seeing?<br>
Just for info...the faster of tach feedbacks (fan<i>f</i>_input)
should always be in the 'upper' 2 bytes of the 0x90 reg and the
slower (fan<i>s</i>_input) should always be the 'lower' 2 bytes
according to Maxim (and what I verified last year with this 0x3040
rev update). So for the associated fan<i>x</i>_target, there will be
differing speeds between the fan<i>s</i>_input and fan<i>f</i>_input
where these should be within a 15% deviation of the given fan<i>x</i>_target.<br>
<br>
<blockquote type="cite"
cite="mid:20170608061249.29233-1-andrew@aj.id.au">
<pre wrap="">
On point 1, here's what I see out of the box:
root@witherspoon:/sys/bus/i2c/drivers/max31785/3-0052/hwmon/hwmon1# grep '.*' fan*
fan10_input:10190
fan11_input:0
fan12_input:0
fan1_fault:0
fan1_input:10714
fan1_pulses:2
fan1_target:10500
fan2_fault:0
fan2_input:10416
fan2_pulses:2
fan2_target:10500
fan3_fault:0
fan3_input:10744
fan3_pulses:2
fan3_target:10500
fan4_fault:0
fan4_input:10806
fan4_pulses:2
fan4_target:10500
fan5_fault:0
fan5_input:0
fan5_pulses:1
fan5_target:1638
fan6_fault:0
fan6_input:0
fan6_pulses:1
fan6_target:1638
fan7_input:10135
fan8_input:9920
fan9_input:10162
Expanding on point 2, I issued `obmcutil chassison`, then proceeded to set
progressively increasing fan targets:
root@witherspoon:/sys/bus/i2c/drivers/max31785/3-0052/hwmon/hwmon1# grep '.*' fan*
fan10_input:7142
fan11_input:0
fan12_input:0
fan1_fault:0
fan1_input:6432
fan1_pulses:2
fan1_target:6500
fan2_fault:0
fan2_input:9816
fan2_pulses:2
fan2_target:9500
fan3_fault:0
fan3_input:8426
fan3_pulses:2
fan3_target:8500
fan4_fault:0
fan4_input:7515
fan4_pulses:2
fan4_target:7500
fan5_fault:0
fan5_input:0
fan5_pulses:1
fan5_target:1638
fan6_fault:0
fan6_input:0
fan6_pulses:1
fan6_target:1638</pre>
</blockquote>
For witherspoon, fan5_target & fan6_target are not wired up so
that 1638 target value is not valid and are never set (so it must be
some default within the max31785). So what did we decide on how
these sysfs file names will be associated between _target and
_input(s)? From our slack discussion<i>, ie) fan1_input &
fan2_input is the pair of feedbacks associated with fan1_target</i>
is that suppose to be true here?<br>
<blockquote type="cite"
cite="mid:20170608061249.29233-1-andrew@aj.id.au">
<pre wrap="">
fan7_input:6009
fan8_input:9351
fan9_input:7944
The fanX_input values for X in [6, 10] are consistently lower than X in [1, 4].</pre>
</blockquote>
There is a chance this is correct as until the thermal team examines
the air flow, its not unheard of to have back pressure on fans
causing them to actually spin slower, but regardless they should be
within the 15% target deviation.<br>
It would be more beneficial to group each _input with its
corresponding _target cuz it seems here, for example, that
fan7_input & fan1_input are associated with fan1_target?<br>
<blockquote type="cite"
cite="mid:20170608061249.29233-1-andrew@aj.id.au">
<pre wrap="">
drivers/hwmon/max31785.c | 103 +++++++++++++++++++++++++----------------------
1 file changed, 54 insertions(+), 49 deletions(-)
diff --git a/drivers/hwmon/max31785.c b/drivers/hwmon/max31785.c
index ed47d08a0ada..ca69d0974c00 100644
--- a/drivers/hwmon/max31785.c
+++ b/drivers/hwmon/max31785.c
@@ -88,10 +88,14 @@ struct max31785 {
u8 mfr_fan_config[NR_CHANNEL];
u8 fault_status[NR_CHANNEL];
u16 pwm[NR_CHANNEL];
- u16 tach_rpm[NR_CHANNEL];
- u16 tach_rpm_fast[NR_CHANNEL];
+ u16 tach_rpm[NR_CHANNEL * 2];
};
+static inline bool max31785_has_fast_rotor(struct max31785 *data)
+{
+ return !!(data->capabilities & MAX31785_CAP_FAST_ROTOR);
+}
+
static int max31785_set_page(struct i2c_client *client,
u8 page)
{
@@ -188,14 +192,14 @@ static int max31785_update_fan_speed(struct max31785 *data, u8 fan)
if (rc)
return rc;
- if (data->capabilities & MAX31785_CAP_FAST_ROTOR) {
+ if (max31785_has_fast_rotor(data)) {
rc = max31785_smbus_read_long_data(data->client,
MAX31785_REG_FAN_SPEED_1);
if (rc < 0)
return rc;
data->tach_rpm[fan] = rc & 0xffff;
- data->tach_rpm_fast[fan] = (rc >> 16) & 0xffff;
+ data->tach_rpm[NR_CHANNEL + fan] = (rc >> 16) & 0xffff;
return rc;
}
@@ -475,7 +479,7 @@ static int max31785_detect(struct i2c_client *client,
return 0;
}
-static const u32 max31785_fan_config[] = {
+static const u32 max31785_fan_config_0x3030[] = {
HWMON_F_INPUT | HWMON_F_PULSES | HWMON_F_TARGET | HWMON_F_FAULT,
HWMON_F_INPUT | HWMON_F_PULSES | HWMON_F_TARGET | HWMON_F_FAULT,
HWMON_F_INPUT | HWMON_F_PULSES | HWMON_F_TARGET | HWMON_F_FAULT,
@@ -485,9 +489,30 @@ static const u32 max31785_fan_config[] = {
0
};
-static const struct hwmon_channel_info max31785_fan = {
+static const struct hwmon_channel_info max31785_fan_0x3030 = {
.type = hwmon_fan,
- .config = max31785_fan_config,
+ .config = max31785_fan_config_0x3030,
+};
+
+static const u32 max31785_fan_config_0x3040[] = {
+ HWMON_F_INPUT | HWMON_F_PULSES | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_INPUT | HWMON_F_PULSES | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_INPUT | HWMON_F_PULSES | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_INPUT | HWMON_F_PULSES | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_INPUT | HWMON_F_PULSES | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_INPUT | HWMON_F_PULSES | HWMON_F_TARGET | HWMON_F_FAULT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ HWMON_F_INPUT,
+ 0
+};
+
+static const struct hwmon_channel_info max31785_fan_0x3040 = {
+ .type = hwmon_fan,
+ .config = max31785_fan_config_0x3040,
};
static const u32 max31785_pwm_config[] = {
@@ -505,8 +530,14 @@ static const struct hwmon_channel_info max31785_pwm = {
.config = max31785_pwm_config
};
-static const struct hwmon_channel_info *max31785_info[] = {
- &max31785_fan,
+static const struct hwmon_channel_info *max31785_info_0x3030[] = {
+ &max31785_fan_0x3030,
+ &max31785_pwm,
+ NULL,
+};
+
+static const struct hwmon_channel_info *max31785_info_0x3040[] = {
+ &max31785_fan_0x3040,
&max31785_pwm,
NULL,
};
@@ -562,15 +593,6 @@ static int max31785_read_fan(struct max31785 *data, u32 attr, int channel,
return rc;
}
-static int max31785_fan_get_fast(struct device *dev,
- struct device_attribute *attr, char *buf)
-{
- struct sensor_device_attribute_2 *attr2 = to_sensor_dev_attr_2(attr);
- struct max31785 *data = max31785_update_device(dev);
-
- return sprintf(buf, "%d\n", data->tach_rpm_fast[attr2->index]);
-}
-
static int max31785_read_pwm(struct max31785 *data, u32 attr, int channel,
long *val)
{
@@ -723,9 +745,14 @@ static const struct hwmon_ops max31785_hwmon_ops = {
.write = max31785_write,
};
-static const struct hwmon_chip_info max31785_chip_info = {
+static const struct hwmon_chip_info max31785_chip_info_0x3030 = {
.ops = &max31785_hwmon_ops,
- .info = max31785_info,
+ .info = max31785_info_0x3030,
+};
+
+static const struct hwmon_chip_info max31785_chip_info_0x3040 = {
+ .ops = &max31785_hwmon_ops,
+ .info = max31785_info_0x3040,
};
static ssize_t max31785_fault_resp_store(struct device *dev,
@@ -802,31 +829,7 @@ static ssize_t max31785_fault_resp_show(struct device *dev,
}
static DEVICE_ATTR(fault_resp, 0644, max31785_fault_resp_show,
- max31785_fault_resp_store);
-
-static SENSOR_DEVICE_ATTR(fan1_input_fast, 0444, max31785_fan_get_fast,
- NULL, 0);
-static SENSOR_DEVICE_ATTR(fan2_input_fast, 0444, max31785_fan_get_fast,
- NULL, 1);
-static SENSOR_DEVICE_ATTR(fan3_input_fast, 0444, max31785_fan_get_fast,
- NULL, 2);
-static SENSOR_DEVICE_ATTR(fan4_input_fast, 0444, max31785_fan_get_fast,
- NULL, 3);
-static SENSOR_DEVICE_ATTR(fan5_input_fast, 0444, max31785_fan_get_fast,
- NULL, 4);
-static SENSOR_DEVICE_ATTR(fan6_input_fast, 0444, max31785_fan_get_fast,
- NULL, 5);
-
-static struct attribute *max31785_attrs[] = {
- &sensor_dev_attr_fan1_input_fast.dev_attr.attr,
- &sensor_dev_attr_fan2_input_fast.dev_attr.attr,
- &sensor_dev_attr_fan3_input_fast.dev_attr.attr,
- &sensor_dev_attr_fan4_input_fast.dev_attr.attr,
- &sensor_dev_attr_fan5_input_fast.dev_attr.attr,
- &sensor_dev_attr_fan6_input_fast.dev_attr.attr,
- NULL,
-};
-ATTRIBUTE_GROUPS(max31785);
+ max31785_fault_resp_store);
static int max31785_get_capabilities(struct max31785 *data)
{
@@ -846,7 +849,7 @@ static int max31785_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
struct i2c_adapter *adapter = client->adapter;
- const struct attribute_group **extra_groups;
+ const struct hwmon_chip_info *chip;
struct device *dev = &client->dev;
struct device *hwmon_dev;
struct max31785 *data;
@@ -871,15 +874,17 @@ static int max31785_probe(struct i2c_client *client,
if (rc < 0)
return rc;
- if (data->capabilities & MAX31785_CAP_FAST_ROTOR)
- extra_groups = max31785_groups;
+ if (max31785_has_fast_rotor(data))
+ chip = &max31785_chip_info_0x3040;
+ else
+ chip = &max31785_chip_info_0x3030;
rc = device_create_file(dev, &dev_attr_fault_resp);
if (rc)
return rc;
hwmon_dev = devm_hwmon_device_register_with_info(dev,
- client->name, data, &max31785_chip_info, extra_groups);
+ client->name, data, chip, NULL);
return PTR_ERR_OR_ZERO(hwmon_dev);
}
</pre>
</blockquote>
Thanks,<br>
<br>
Matt<br>
</body>
</html>