[PATCH] hwmon: max31785: Use standard sysfs ABI for fast rotor inputs
Matthew Barth
msbarth at linux.vnet.ibm.com
Fri Jun 9 06:42:39 AEST 2017
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 at 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 at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170608/b054c837/attachment-0001.html>
More information about the openbmc
mailing list