* [PATCH] hwmon: max31785: Use standard sysfs ABI for fast rotor inputs
@ 2017-06-08 6:12 Andrew Jeffery
2017-06-08 20:42 ` Matthew Barth
0 siblings, 1 reply; 3+ messages in thread
From: Andrew Jeffery @ 2017-06-08 6:12 UTC (permalink / raw)
To: joel; +Cc: Andrew Jeffery, msbarth, openbmc
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 <andrew@aj.id.au>
---
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).
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).
2. The fast-rotor attributes appear to correspond to the slow rotor values.
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
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].
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);
}
--
2.11.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hwmon: max31785: Use standard sysfs ABI for fast rotor inputs
2017-06-08 6:12 [PATCH] hwmon: max31785: Use standard sysfs ABI for fast rotor inputs Andrew Jeffery
@ 2017-06-08 20:42 ` Matthew Barth
2017-06-09 1:28 ` Andrew Jeffery
0 siblings, 1 reply; 3+ messages in thread
From: Matthew Barth @ 2017-06-08 20:42 UTC (permalink / raw)
To: Andrew Jeffery, joel; +Cc: openbmc
[-- Attachment #1: Type: text/plain, Size: 11470 bytes --]
On 06/08/17 1:12 AM, Andrew Jeffery wrote:
> 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 <andrew@aj.id.au>
> ---
> 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).
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).
> 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).
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.
> 2. The fast-rotor attributes appear to correspond to the slow rotor values.
What do you mean by attributes here? What were you seeing?
Just for info...the faster of tach feedbacks (fan/f/_input) should
always be in the 'upper' 2 bytes of the 0x90 reg and the slower
(fan/s/_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/x/_target, there will be differing speeds between the
fan/s/_input and fan/f/_input where these should be within a 15%
deviation of the given fan/x/_target.
>
> 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
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/, ie) fan1_input & fan2_input is the pair of feedbacks
associated with fan1_target/ is that suppose to be true here?
> 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].
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.
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?
>
> 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);
> }
Thanks,
Matt
[-- Attachment #2: Type: text/html, Size: 12274 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hwmon: max31785: Use standard sysfs ABI for fast rotor inputs
2017-06-08 20:42 ` Matthew Barth
@ 2017-06-09 1:28 ` Andrew Jeffery
0 siblings, 0 replies; 3+ messages in thread
From: Andrew Jeffery @ 2017-06-09 1:28 UTC (permalink / raw)
To: Matthew Barth, joel; +Cc: openbmc
[-- Attachment #1: Type: text/plain, Size: 10216 bytes --]
Hi Matt,
On Thu, 2017-06-08 at 15:42 -0500, Matthew Barth wrote:
>
>
> On 06/08/17 1:12 AM, Andrew Jeffery wrote:
> > 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].
Uh, so this range should be [NR_CHANNEL + 1, 2 * NR_CHANNEL], given
hwmon uses 1-based indexing.
Joel: I'll send a v2 to fix this and another issue.
> >
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > 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).
> 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.
Okay.
> 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.
Right - please keep me up to date with the state of play.
> (Along with this, he's again going to request an updated datasheet
> for rev 0x3040).
Great. Will it be a public release like the existing datasheet?
> > 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).
> 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.
Right - the behaviour I'm seeing is that instead of 0's I read the last
reported value, until the target is reached. I hadn't understood
exactly *when* the state changed, but my gut feeling is that the value
change aligned with reaching the target as you suggest.
> 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.
Okay, so it sounds like we're covered either way (zero, or the last
reported value).
> > 2.
> > The fast-rotor attributes appear to correspond to the slow rotor
> > values.
> What do you mean by attributes here?
hwmon sysfs attributes.
> What were you seeing?
The attributes I assigned to the fast rotor values were reporting lower
values than the attributes I assigned to the slow rotor.
> Just for info...the faster of tach feedbacks (fanf_input) should
> always be in the 'upper' 2 bytes of the 0x90 reg and the slower
> (fans_input) should always be the 'lower' 2 bytes according to Maxim
> (and what I verified last year with this 0x3040 rev update).
I'll address this below in the patch where I've implemented the logic
for reading the 4-byte values off the bus.
> So for the associated fanx_target, there will be differing speeds
> between the fans_input and fanf_input where these should be within a
> 15% deviation of the given fanx_target.
>
> > 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
> 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).
Yes, understood.
> So what did we decide on how these sysfs file names will be
> associated between _target and _input(s)? From our slack discussion,
> ie) fan1_input & fan2_input is the pair of feedbacks associated with
> fan1_target is that suppose to be true here?
No, I changed it as outlined in the commit message (modulo my off-by-
one error), as this made testing with the openbmc userspace easier.
> > fan7_input:6009
> > fan8_input:9351
> > fan9_input:7944
> >
> > The fanX_input values for X in [7, 10]
Again, due to some mental haziness, I was off-by-one here. That should
read [7, 11].
> > are consistently lower than X in [1, 4].
> 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.
> 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?
Is this a comment on how the driver is implemented or how I've
displayed the outputs for the analysis above?
> > 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;
This is where I interpret the top 16 bits as the fast fan value. For
reference, the implementation of max31785_smbus_read_long_data() is:
/* Cut down version of i2c_smbus_xfer_emulated(), reading 4 bytes */
static s64 max31785_smbus_read_long_data(struct i2c_client *client, u8 command)
{
unsigned char cmdbuf[1];
unsigned char rspbuf[4];
s64 rc;
struct i2c_msg msg[2] = {
{
.addr = client->addr,
.flags = 0,
.len = sizeof(cmdbuf),
.buf = cmdbuf,
},
{
.addr = client->addr,
.flags = I2C_M_RD,
.len = sizeof(rspbuf),
.buf = rspbuf,
},
};
cmdbuf[0] = command;
rc = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
if (rc < 0)
return rc;
rc = (rspbuf[0] << (0 * 8)) | (rspbuf[1] << (1 * 8)) |
(rspbuf[2] << (2 * 8)) | (rspbuf[3] << (3 * 8));
return rc;
}
Particularly,
rc = (rspbuf[0] << (0 * 8)) | (rspbuf[1] << (1 * 8)) |
(rspbuf[2] << (2 * 8)) | (rspbuf[3] << (3 * 8));
Is derived from the i2c_smbus_xfer_emulated() implementation, expanding
it out to 4 bytes. From the SMBus specification the low byte is sent
first, followed by the high byte. Your suggestion was the slow rotor is
still reported in the first two bytes for the 0x3040 firmware, and I
assume the fast rotor word conforms to the SMBus standard, therefore we
arrive at the implementation above for rspbuf bytes 2 and 3.
However, as I reported, I'm seeing *lower* RPM values for the 'fast'
bytes (2 and 3 in rspbuf, or bits 31-16 in the returned 32-bit
quantity). Can you please try the patch yourself?
Cheers,
Andrew
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-06-09 1:28 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-08 6:12 [PATCH] hwmon: max31785: Use standard sysfs ABI for fast rotor inputs Andrew Jeffery
2017-06-08 20:42 ` Matthew Barth
2017-06-09 1:28 ` Andrew Jeffery
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.