[PATCH linux dev-4.13 0/5] max31785: Add devicetree support and work-arounds for transfer failures

[PATCH linux dev-4.13 0/5] max31785: Add devicetree support and work-arounds for transfer failures

[Andrew Jeffery]

Hello,

This is a clean-up and forward port of some patches that didn't entirely make
it into dev-4.10, but did make it into some release trees.

The devicetree support is required to work around manufacturing deficiencies
which leave the controllers unconfigured out of the factory for some POWER
systems.

The communication issues with the MAX31785 on some POWER systems remains
unresolved and the outlook is that they won't ever be root-caused.

The reworked patches have been lightly tested on a Witherspoon system and
appear to be functional.

Please review! If there's no significant feedback in the next few days I'll
include them in the dev-4.13 tree.

Andrew Jeffery (5):
  dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation
  pmbus (max31785): Add support for devicetree configuration
  pmbus (core): One-shot retries for failure to set page
  pmbus (core): Use driver callbacks in pmbus_get_fan_rate()
  pmbus (max31785): Wrap all I2C accessors in one-shot failure handlers

 .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 +++++++
 drivers/hwmon/pmbus/max31785.c                     | 497 +++++++++++++++++++--
 drivers/hwmon/pmbus/pmbus_core.c                   |  20 +-
 3 files changed, 641 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt

-- 
2.14.1

[PATCH linux dev-4.13 1/5] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 +++++++++++++++++++++
 1 file changed, 158 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
new file mode 100644
index 000000000000..af9578e7742c
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
@@ -0,0 +1,158 @@
+Bindings for the Maxim MAX31785 Intelligent Fan Controller
+==========================================================
+
+Reference:
+
+https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
+
+Required properties:
+- compatible     : One of "maxim,max31785" or "maxim,max31785a"
+- reg            : I2C address, one of 0x52, 0x53, 0x54, 0x55.
+- #address-cells : Must be 1
+- #size-cells    : Must be 0
+- #thermal-sensor-cells  : Should be 1. The device supports:
+                           - One internal sensor
+                           - Four external I2C digital sensors
+                           - Six external thermal diodes
+
+Optional properties:
+- use-stored-presence    : Do not treat the devicetree description as canon for
+                           fan presence (the 'installed' bit of FAN_CONFIG_*).
+                           Instead, rely on the on the default value store of
+                           the device to populate it.
+
+Capabilities are configured through subnodes of the controller's node.
+
+Fans
+----
+
+Only fans with subnodes present will be considered as installed. If
+use-stored-presence is present in the parent node, then only fans that are both
+defined in the devicetree and have their installed bit set are considered
+installed.
+
+Required subnode properties:
+- compatible             : Must be "pmbus-fan"
+- reg                    : The PMBus page the properties apply to.
+- #cooling-cells         : Should be 2. See the thermal bindings at [1].
+- maxim,fan-rotor-input  : The type of rotor measurement provided to the
+                           controller. Must be either "tach" for tachometer
+                           pulses or "lock" for a locked-rotor signal.
+- maxim,fan-lock-polarity: Required iff maxim,fan-rotor-input is "lock". Valid
+                           values are "low" for active low, "high" for active
+                           high.
+
+Optional subnode properties:
+- fan-mode               : "rpm" or "pwm". Default value is "pwm".
+- tach-pulses            : Tachometer pulses per revolution. Valid values are
+                           1, 2, 3 or 4. The default is 1.
+- cooling-min-level      : Smallest cooling state accepted. See [1].
+- cooling-max-level      : Largest cooling state accepted. See [1].
+- maxim,fan-no-fault-ramp: Do not ramp the fan to 100% PWM duty on detecting a
+                           fan fault
+- maxim,fan-startup      : The number of rotations required before taking
+                           emergency action for an unresponsive fan and driving
+                           it with 100% or 0% PWM duty, depending on the state
+                           of maxim,fan-no-fault-ramp. Valid values are 0
+                           (automatic spin-up disabled), 2, 4, or 8. Default
+                           value is 0.
+- maxim,fan-health       : Enable automated fan health check
+- maxim,fan-ramp         : Configures how fast the device ramps the PWM duty
+                           cycle from one value to another. Valid values are 0
+                           to 7 inclusive, with values 0 - 2 configuring a
+                           1000ms update rate and 1 - 3% duty respective duty
+                           increase, and 3 - 7 a 200ms update rate with a 1 -
+                           5% respective duty increase. Default value is 0.
+- maxim,fan-no-watchdog  : Do not ramp fan to 100% PWM duty on failure to
+                           update desired fan rate inside 10s. This implies
+                           maxim,tmp-no-fault-ramp
+- maxim,tmp-no-fault-ramp: Do not ramp fan to 100% PWM duty on temperature
+                           sensor fault detection. This implies
+                           maxim,fan-no-watchdog
+- maxim,tmp-hysteresis   : The temperature hysteresis used to determine
+                           transitions to lower fan speed bands in the
+                           temperature/fan rate lookup table. Valid values are
+                           2, 4, 6 or 8 (degrees celcius). Default value is 2.
+- maxim,fan-dual-tach    : Enable dual tachometer functionality
+- maxim,fan-pwm-freq     : The PWM frequency. Valid values are 30, 50, 100, 150
+                           and 25000 (Hz). Default value is 30Hz.
+- maxim,fan-lookup-table : A 16-element cell array of alternating temperature
+                           and rate values representing the look up table. The
+                           rate units are set through the fan-mode property.
+- maxim,fan-fault-pin-mon: Ramp fans to 100% PWM duty when the FAULT pin is
+                           asserted
+
+Temperature
+-----------
+
+Required subnode properties:
+- compatible    : Must be "pmbus-temperature"
+- reg           : The PMBus page the properties apply to.
+
+Optional subnode properties:
+- maxim,tmp-offset      : Valid values are 0 - 30 (degrees celcius) inclusive.
+                          Default value is 0.
+- maxim,tmp-fans        : An array of phandles to fans controlled by the
+                          current temperature sensor.
+
+[1] Documentation/devicetree/bindings/thermal/thermal.txt
+
+Example:
+	fan-max31785: max31785@52 {
+		reg = <0x52>;
+		compatible = "maxim,max31785";
+                #address-cells = <1>;
+                #size-cells = <0>;
+                #thermal-sensor-cells = <1>;
+
+                fan@0 {
+                        compatible = "pmbus-fan";
+                        reg = <0>;
+                        mode = "rpm";
+                        tach-pulses = <1>;
+
+                        #cooling-cells = <2>;
+                        cooling-min-level = <0>;
+                        cooling-max-level = <9>;
+
+                        maxim,fan-rotor-input = "tach";
+                        maxim,fan-dual-tach;
+                };
+
+                /*
+                 * Hardware controlled fan: Fan speed is controlled by a
+                 * temperature sensor feeding values into the lookup table. The
+                 * fan association is done in the temperature sensor node. One
+                 * sensor can drive multiple fans.
+                 */
+                cpu_fan: fan@1 {
+                        compatible = "pmbus-fan";
+                        reg = <1>;
+                        mode = "rpm";
+                        tach-pulses = <1>;
+
+                        #cooling-cells = <2>;
+
+                        maxim,fan-rotor-input = "tach";
+                        maxim,tmp-hysteresis = <2>;
+                        maxim,fan-lookup-table = <
+                        /*  Temperature    RPM  */
+                                 0        1000
+                                10        2000
+                                20        3000
+                                30        4000
+                                40        5000
+                                50        6000
+                                60        7000
+                                70        8000
+                        >;
+                };
+
+                cpu_temp: sensor@6 {
+                        compatible = "pmbus-temperature";
+                        reg = <6>;
+
+                        maxim,tmp-offset = <0>;
+                        maxim,tmp-fans = <&cpu_fan>;
+                };
+	};
-- 
2.14.1

[PATCH linux dev-4.13 2/5] pmbus (max31785): Add support for devicetree configuration

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/pmbus/max31785.c | 318 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 318 insertions(+)

diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index bffab449be39..9a7e745b6b31 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -16,13 +16,23 @@
 
 enum max31785_regs {
 	MFR_REVISION		= 0x9b,
+	MFR_FAULT_RESPONSE	= 0xd9,
+	MFR_TEMP_SENSOR_CONFIG	= 0xf0,
 	MFR_FAN_CONFIG		= 0xf1,
+	MFR_FAN_FAULT_LIMIT	= 0xf5,
 };
 
 #define MAX31785			0x3030
 #define MAX31785A			0x3040
 
 #define MFR_FAN_CONFIG_DUAL_TACH	BIT(12)
+#define MFR_FAN_CONFIG_TSFO		BIT(9)
+#define MFR_FAN_CONFIG_TACHO		BIT(8)
+#define MFR_FAN_CONFIG_HEALTH		BIT(4)
+#define MFR_FAN_CONFIG_ROTOR_HI_LO	BIT(3)
+#define MFR_FAN_CONFIG_ROTOR		BIT(2)
+
+#define MFR_FAULT_RESPONSE_MONITOR	BIT(0)
 
 #define MAX31785_NR_PAGES		23
 #define MAX31785_NR_FAN_PAGES		6
@@ -239,6 +249,271 @@ static int max31785_write_word_data(struct i2c_client *client, int page,
 	return -ENODATA;
 }
 
+/*
+ * Returns negative error codes if an unrecoverable problem is detected, 0 if a
+ * recoverable problem is detected, or a positive value on success.
+ */
+static int max31785_of_fan_config(struct i2c_client *client,
+				  struct pmbus_driver_info *info,
+				  struct device_node *child)
+{
+	int mfr_cfg = 0, mfr_fault_resp = 0, pb_cfg;
+	struct device *dev = &client->dev;
+	char *lock_polarity = NULL;
+	const char *sval;
+	u32 page;
+	u32 uval;
+	int ret;
+
+	if (!of_device_is_compatible(child, "pmbus-fan"))
+		return 0;
+
+	ret = of_property_read_u32(child, "reg", &page);
+	if (ret < 0) {
+		dev_err(&client->dev, "Missing valid reg property\n");
+		return ret;
+	}
+
+	if (!(info->func[page] & PMBUS_HAVE_FAN12)) {
+		dev_err(dev, "Page %d does not have fan capabilities\n", page);
+		return -ENXIO;
+	}
+
+	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
+	if (ret < 0)
+		return ret;
+
+	pb_cfg = i2c_smbus_read_byte_data(client, PMBUS_FAN_CONFIG_12);
+	if (pb_cfg < 0)
+		return pb_cfg;
+
+	if (of_property_read_bool(child->parent, "use-stored-presence")) {
+		if (!(pb_cfg & PB_FAN_1_INSTALLED))
+			dev_info(dev, "Fan %d is configured but not installed\n",
+				 page);
+	} else {
+		pb_cfg |= PB_FAN_1_INSTALLED;
+	}
+
+	ret = of_property_read_string(child, "maxim,fan-rotor-input", &sval);
+	if (ret < 0) {
+		dev_err(dev, "Missing valid maxim,fan-rotor-input property for fan %d\n",
+				page);
+		return ret;
+	}
+
+	if (strcmp("tach", sval) && strcmp("lock", sval)) {
+		dev_err(dev, "maxim,fan-rotor-input has invalid value for fan %d: %s\n",
+				page, sval);
+		return -EINVAL;
+	} else if (!strcmp("lock", sval)) {
+		mfr_cfg |= MFR_FAN_CONFIG_ROTOR;
+
+		ret = i2c_smbus_write_word_data(client, MFR_FAN_FAULT_LIMIT, 1);
+		if (ret < 0)
+			return ret;
+
+		ret = of_property_read_string(child, "maxim,fan-lock-polarity",
+					      &sval);
+		if (ret < 0) {
+			dev_err(dev, "Missing valid maxim,fan-lock-polarity property for fan %d\n",
+					page);
+			return ret;
+		}
+
+		if (strcmp("low", sval) && strcmp("high", sval)) {
+			dev_err(dev, "maxim,fan-lock-polarity has invalid value for fan %d: %s\n",
+					page, lock_polarity);
+			return -EINVAL;
+		} else if (!strcmp("high", sval))
+			mfr_cfg |= MFR_FAN_CONFIG_ROTOR_HI_LO;
+	}
+
+	if (!of_property_read_string(child, "fan-mode", &sval)) {
+		if (!strcmp("rpm", sval))
+			pb_cfg |= PB_FAN_1_RPM;
+		else if (!strcmp("pwm", sval))
+			pb_cfg &= ~PB_FAN_1_RPM;
+		else {
+			dev_err(dev, "fan-mode has invalid value for fan %d: %s\n",
+					page, sval);
+			return -EINVAL;
+		}
+	}
+
+	ret = of_property_read_u32(child, "tach-pulses", &uval);
+	if (ret < 0) {
+		pb_cfg &= ~PB_FAN_1_PULSE_MASK;
+	} else if (uval && (uval - 1) < 4) {
+		pb_cfg = ((pb_cfg & ~PB_FAN_1_PULSE_MASK) | ((uval - 1) << 4));
+	} else {
+		dev_err(dev, "tach-pulses has invalid value for fan %d: %u\n",
+				page, uval);
+		return -EINVAL;
+	}
+
+	if (of_property_read_bool(child, "maxim,fan-health"))
+		mfr_cfg |= MFR_FAN_CONFIG_HEALTH;
+
+	if (of_property_read_bool(child, "maxim,fan-no-watchdog") ||
+		of_property_read_bool(child, "maxim,tmp-no-fault-ramp"))
+		mfr_cfg |= MFR_FAN_CONFIG_TSFO;
+
+	if (of_property_read_bool(child, "maxim,fan-dual-tach"))
+		mfr_cfg |= MFR_FAN_CONFIG_DUAL_TACH;
+
+	if (of_property_read_bool(child, "maxim,fan-no-fault-ramp"))
+		mfr_cfg |= MFR_FAN_CONFIG_TACHO;
+
+	if (!of_property_read_u32(child, "maxim,fan-startup", &uval)) {
+		uval /= 2;
+		if (uval < 5) {
+			mfr_cfg |= uval;
+		} else {
+			dev_err(dev, "maxim,fan-startup has invalid value for fan %d: %u\n",
+					page, uval);
+			return -EINVAL;
+		}
+	}
+
+	if (!of_property_read_u32(child, "maxim,fan-ramp", &uval)) {
+		if (uval < 8) {
+			mfr_cfg |= uval << 5;
+		} else {
+			dev_err(dev, "maxim,fan-ramp has invalid value for fan %d: %u\n",
+					page, uval);
+			return -EINVAL;
+		}
+	}
+
+	if (!of_property_read_u32(child, "maxim,tmp-hysteresis", &uval)) {
+		uval /= 2;
+		uval -= 1;
+		if (uval < 4) {
+			mfr_cfg |= uval << 10;
+		} else {
+			dev_err(dev, "maxim,tmp-hysteresis has invalid value for fan %d, %u\n",
+					page, uval);
+			return -EINVAL;
+		}
+	}
+
+	if (!of_property_read_u32(child, "maxim,fan-pwm-freq", &uval)) {
+		u16 val;
+
+		if (uval == 30) {
+			val = 0;
+		} else if (uval == 50) {
+			val = 1;
+		} else if (uval == 100) {
+			val = 2;
+		} else if (uval == 150) {
+			val = 3;
+		} else if (uval == 25000) {
+			val = 7;
+		} else {
+			dev_err(dev, "maxim,fan-pwm-freq has invalid value for fan %d: %u\n",
+					page, uval);
+			return -EINVAL;
+		}
+
+		mfr_cfg |= val << 13;
+	}
+
+	if (of_property_read_bool(child, "maxim,fan-fault-pin-mon"))
+		mfr_fault_resp |= MFR_FAULT_RESPONSE_MONITOR;
+
+	ret = i2c_smbus_write_byte_data(client, PMBUS_FAN_CONFIG_12,
+					pb_cfg & ~PB_FAN_1_INSTALLED);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_word_data(client, MFR_FAN_CONFIG, mfr_cfg);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, MFR_FAULT_RESPONSE,
+					mfr_fault_resp);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(client, PMBUS_FAN_CONFIG_12, pb_cfg);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Fans are on pages 0 - 5. If the page property of a fan node is
+	 * greater than 5 we will have errored in checks above out above.
+	 * Therefore we don't need to cope with values up to 31, and the int
+	 * return type is enough.
+	 *
+	 * The bit mask return value is used to populate a bitfield of fans
+	 * who are both configured in the devicetree _and_ reported as
+	 * installed by the hardware. Any fans that are not configured in the
+	 * devicetree but are reported as installed by the hardware will have
+	 * their hardware configuration updated to unset the installed bit.
+	 */
+	return BIT(page);
+}
+
+static int max31785_of_tmp_config(struct i2c_client *client,
+				  struct pmbus_driver_info *info,
+				  struct device_node *child)
+{
+	struct device *dev = &client->dev;
+	struct device_node *np;
+	u16 mfr_tmp_cfg = 0;
+	u32 page;
+	u32 uval;
+	int ret;
+	int i;
+
+	if (!of_device_is_compatible(child, "pmbus-temperature"))
+		return 0;
+
+	ret = of_property_read_u32(child, "reg", &page);
+	if (ret < 0) {
+		dev_err(&client->dev, "Missing valid reg property\n");
+		return ret;
+	}
+
+	if (!(info->func[page] & PMBUS_HAVE_TEMP)) {
+		dev_err(dev, "Page %d does not have temp capabilities\n", page);
+		return -ENXIO;
+	}
+
+	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
+	if (ret < 0)
+		return ret;
+
+	if (!of_property_read_u32(child, "maxim,tmp-offset", &uval)) {
+		if (uval < 32)
+			mfr_tmp_cfg |= uval << 10;
+	}
+
+	i = 0;
+	while ((np = of_parse_phandle(child, "maxim,tmp-fans", i))) {
+		if (of_property_read_u32(np, "reg", &uval)) {
+			dev_err(&client->dev, "Failed to read fan reg property for phandle index %d\n",
+					i);
+		} else {
+			if (uval < 6)
+				mfr_tmp_cfg |= BIT(uval);
+			else
+				dev_warn(&client->dev, "Invalid fan page: %d\n",
+						uval);
+		}
+		i++;
+	}
+
+	ret = i2c_smbus_write_word_data(client, MFR_TEMP_SENSOR_CONFIG,
+					mfr_tmp_cfg);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
 #define MAX31785_FAN_FUNCS \
 	(PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | PMBUS_HAVE_PWM12)
 
@@ -334,9 +609,12 @@ static int max31785_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
+	struct device_node *child;
 	struct pmbus_driver_info *info;
 	bool dual_tach = false;
+	u32 fans;
 	s64 ret;
+	int i;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_BYTE_DATA |
@@ -366,6 +644,46 @@ static int max31785_probe(struct i2c_client *client,
 		return -ENODEV;
 	}
 
+	fans = 0;
+	for_each_child_of_node(dev->of_node, child) {
+		ret = max31785_of_fan_config(client, info, child);
+		if (ret < 0) {
+			of_node_put(child);
+			return ret;
+		}
+
+		if (ret)
+			fans |= ret;
+
+		ret = max31785_of_tmp_config(client, info, child);
+		if (ret < 0) {
+			of_node_put(child);
+			return ret;
+		}
+	}
+
+	for (i = 0; i < MAX31785_NR_PAGES; i++) {
+		bool have_fan = !!(info->func[i] & PMBUS_HAVE_FAN12);
+		bool fan_configured = !!(fans & BIT(i));
+
+		if (!have_fan || fan_configured)
+			continue;
+
+		ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
+		if (ret < 0)
+			return ret;
+
+		ret = i2c_smbus_read_byte_data(client, PMBUS_FAN_CONFIG_12);
+		if (ret < 0)
+			return ret;
+
+		ret &= ~PB_FAN_1_INSTALLED;
+		ret = i2c_smbus_write_word_data(client, PMBUS_FAN_CONFIG_12,
+						ret);
+		if (ret < 0)
+			return ret;
+	}
+
 	if (dual_tach) {
 		ret = max31785_configure_dual_tach(client, info);
 		if (ret < 0)
-- 
2.14.1

[PATCH linux dev-4.13 3/5] pmbus (core): One-shot retries for failure to set page

[Andrew Jeffery]

Work around the shonky behaviour seen with the MAX31785 where we fail
to set the page register in some circumstances.

There's no real elegant way to do this. We can propagate the error up,
but that forces us to retry the operation way up the call tree in any
number of places. It also forces callers to split out pmbus_set_page()
from the pmbus_{read,write}_{byte,word}_data() functions in order to
differentiate between a failure to set the page and a failure to read a
register (that might not exist, in which case an error is anticiptated).

Cc: Eddie James <eajames@linux.vnet.ibm.com>
Cc: Matt Spinler <mspinler@linux.vnet.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus_core.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ee7e0684c29f..ee1bf11c7fc6 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -167,9 +167,17 @@ int pmbus_set_page(struct i2c_client *client, int page)
 		return 0;
 
 	if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
+		dev_dbg(&client->dev, "Want page %u, %u cached\n", page,
+			data->currpage);
+
 		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
-		if (rv < 0)
-			return rv;
+		if (rv) {
+			rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE,
+						       page);
+			dev_dbg(&client->dev,
+				"Failed to set page %u, performed one-shot retry %s: %d\n",
+				page, rv ? "and failed" : "with success", rv);
+		}
 
 		rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
 		if (rv < 0)
-- 
2.14.1

[PATCH linux dev-4.13 4/5] pmbus (core): Use driver callbacks in pmbus_get_fan_rate()

[Andrew Jeffery]

The driver may have overridden the pmbus_read_byte_data() callback, so
make sure we use that to achieve expected behaviour.

This helps in the MAX31785 case where we may need to perform a one-shot
retry of transfers in the face of a failure.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus_core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ee1bf11c7fc6..d0ffa4b3fcbd 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -453,15 +453,15 @@ static int pmbus_get_fan_rate(struct i2c_client *client, int page, int id,
 		return s->data;
 	}
 
-	config = pmbus_read_byte_data(client, page,
-				      pmbus_fan_config_registers[id]);
+	config = _pmbus_read_byte_data(client, page,
+				       pmbus_fan_config_registers[id]);
 	if (config < 0)
 		return config;
 
 	have_rpm = !!(config & pmbus_fan_rpm_mask[id]);
 	if (want_rpm == have_rpm)
-		return pmbus_read_word_data(client, page,
-					    pmbus_fan_command_registers[id]);
+		return _pmbus_read_word_data(client, page,
+					     pmbus_fan_command_registers[id]);
 
 	/* Can't sensibly map between RPM and PWM, just return zero */
 	return 0;
-- 
2.14.1

[PATCH linux dev-4.13 5/5] pmbus (max31785): Wrap all I2C accessors in one-shot failure handlers

[Andrew Jeffery]

The MAX31785(A) has shown erratic behaviour across multiple system
designs, unexpectedly clock stretching and NAKing transactions. Perform
a one-shot retry if necessary for all access attempts.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/pmbus/max31785.c | 207 ++++++++++++++++++++++++++++++++---------
 1 file changed, 165 insertions(+), 42 deletions(-)

diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index 9a7e745b6b31..3fb111c6d528 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -37,29 +37,105 @@ enum max31785_regs {
 #define MAX31785_NR_PAGES		23
 #define MAX31785_NR_FAN_PAGES		6
 
+/*
+ * MAX31785 dragons ahead
+ *
+ * We see weird issues where some transfers fail. There doesn't appear to be
+ * any pattern to the problem, so below we wrap all the read/write calls with a
+ * retry. The device provides no indication of this besides NACK'ing master
+ * Txs; no bits are set in STATUS_BYTE to suggest anything has gone wrong.
+ */
+
+#define max31785_retry(_func, ...) ({					\
+	/* All relevant functions return int, sue me */			\
+	int _ret = _func(__VA_ARGS__);					\
+	if (_ret == -EIO)						\
+		_ret = _func(__VA_ARGS__);				\
+	_ret;								\
+})
+
+static int max31785_i2c_smbus_read_byte_data(struct i2c_client *client,
+					      int command)
+{
+	return max31785_retry(i2c_smbus_read_byte_data, client, command);
+}
+
+
+static int max31785_i2c_smbus_write_byte_data(struct i2c_client *client,
+					      int command, u16 data)
+{
+	return max31785_retry(i2c_smbus_write_byte_data, client, command, data);
+}
+
+static int max31785_i2c_smbus_read_word_data(struct i2c_client *client,
+					     int command)
+{
+	return max31785_retry(i2c_smbus_read_word_data, client, command);
+}
+
+static int max31785_i2c_smbus_write_word_data(struct i2c_client *client,
+					      int command, u16 data)
+{
+	return max31785_retry(i2c_smbus_write_word_data, client, command, data);
+}
+
+static int max31785_pmbus_write_byte(struct i2c_client *client, int page,
+				     u8 value)
+{
+	return max31785_retry(pmbus_write_byte, client, page, value);
+}
+
+static int max31785_pmbus_read_byte_data(struct i2c_client *client, int page,
+					  int command)
+{
+	return max31785_retry(pmbus_read_byte_data, client, page, command);
+}
+
+static int max31785_pmbus_write_byte_data(struct i2c_client *client, int page,
+					  int command, u16 data)
+{
+	return max31785_retry(pmbus_write_byte_data, client, page, command,
+			      data);
+}
+
+static int max31785_pmbus_read_word_data(struct i2c_client *client, int page,
+					  int command)
+{
+	return max31785_retry(pmbus_read_word_data, client, page, command);
+}
+
+static int max31785_pmbus_write_word_data(struct i2c_client *client, int page,
+					  int command, u16 data)
+{
+	return max31785_retry(pmbus_write_word_data, client, page, command,
+			      data);
+}
+
 static int max31785_read_byte_data(struct i2c_client *client, int page,
 				   int reg)
 {
-	if (page < MAX31785_NR_PAGES)
-		return -ENODATA;
-
 	switch (reg) {
 	case PMBUS_VOUT_MODE:
-		return -ENOTSUPP;
+		if (page >= MAX31785_NR_PAGES)
+			return -ENOTSUPP;
+		break;
 	case PMBUS_FAN_CONFIG_12:
-		return pmbus_read_byte_data(client, page - MAX31785_NR_PAGES,
-					    reg);
+		if (page >= MAX31785_NR_PAGES)
+			return max31785_pmbus_read_byte_data(client,
+					page - MAX31785_NR_PAGES,
+					reg);
+		break;
 	}
 
-	return -ENODATA;
+	return max31785_pmbus_read_byte_data(client, page, reg);
 }
 
 static int max31785_write_byte(struct i2c_client *client, int page, u8 value)
 {
-	if (page < MAX31785_NR_PAGES)
-		return -ENODATA;
+	if (page >= MAX31785_NR_PAGES)
+		return -ENOTSUPP;
 
-	return -ENOTSUPP;
+	return max31785_pmbus_write_byte(client, page, value);
 }
 
 static int max31785_read_long_data(struct i2c_client *client, int page,
@@ -120,11 +196,13 @@ static int max31785_get_pwm_mode(struct i2c_client *client, int page)
 	int config;
 	int command;
 
-	config = pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12);
+	config = max31785_pmbus_read_byte_data(client, page,
+					       PMBUS_FAN_CONFIG_12);
 	if (config < 0)
 		return config;
 
-	command = pmbus_read_word_data(client, page, PMBUS_FAN_COMMAND_1);
+	command = max31785_pmbus_read_word_data(client, page,
+						PMBUS_FAN_COMMAND_1);
 	if (command < 0)
 		return command;
 
@@ -148,15 +226,14 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
 	switch (reg) {
 	case PMBUS_READ_FAN_SPEED_1:
 		if (page < MAX31785_NR_PAGES)
-			return -ENODATA;
+			return max31785_pmbus_read_word_data(client, page, reg);
 
 		rv = max31785_read_long_data(client, page - MAX31785_NR_PAGES,
 					     reg, &val);
 		if (rv < 0)
 			return rv;
 
-		rv = (val >> 16) & 0xffff;
-		break;
+		return (val >> 16) & 0xffff;
 	case PMBUS_FAN_COMMAND_1:
 		/*
 		 * PMBUS_FAN_COMMAND_x is probed to judge whether or not to
@@ -164,20 +241,28 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
 		 *
 		 * Don't expose fan_target attribute for virtual pages.
 		 */
-		rv = (page >= MAX31785_NR_PAGES) ? -ENOTSUPP : -ENODATA;
+		if (page >= MAX31785_NR_PAGES)
+			return -ENOTSUPP;
 		break;
+	case PMBUS_VIRT_FAN_TARGET_1:
+		if (page >= MAX31785_NR_PAGES)
+			return -ENOTSUPP;
+
+		return -ENODATA;
 	case PMBUS_VIRT_PWM_1:
-		rv = max31785_get_pwm(client, page);
-		break;
+		return max31785_get_pwm(client, page);
 	case PMBUS_VIRT_PWM_ENABLE_1:
-		rv = max31785_get_pwm_mode(client, page);
-		break;
+		return max31785_get_pwm_mode(client, page);
 	default:
-		rv = -ENODATA;
+		if (page >= MAX31785_NR_PAGES)
+			return -ENXIO;
 		break;
 	}
 
-	return rv;
+	if (reg >= PMBUS_VIRT_BASE)
+		return -ENXIO;
+
+	return max31785_pmbus_read_word_data(client, page, reg);
 }
 
 static inline u32 max31785_scale_pwm(u32 sensor_val)
@@ -201,6 +286,31 @@ static inline u32 max31785_scale_pwm(u32 sensor_val)
 	return (sensor_val * 100) / 255;
 }
 
+static int max31785_update_fan(struct i2c_client *client, int page,
+			       u8 config, u8 mask, u16 command)
+{
+	int from, rv;
+	u8 to;
+
+	from = max31785_pmbus_read_byte_data(client, page, PMBUS_FAN_CONFIG_12);
+	if (from < 0)
+		return from;
+
+	to = (from & ~mask) | (config & mask);
+
+	if (to != from) {
+		rv = max31785_pmbus_write_byte_data(client, page,
+						    PMBUS_FAN_CONFIG_12, to);
+		if (rv < 0)
+			return rv;
+	}
+
+	rv = max31785_pmbus_write_word_data(client, page, PMBUS_FAN_COMMAND_1,
+					    command);
+
+	return rv;
+}
+
 static int max31785_pwm_enable(struct i2c_client *client, int page,
 				    u16 word)
 {
@@ -230,15 +340,18 @@ static int max31785_pwm_enable(struct i2c_client *client, int page,
 		return -EINVAL;
 	}
 
-	return pmbus_update_fan(client, page, 0, config, PB_FAN_1_RPM, rate);
+	return max31785_update_fan(client, page, config, PB_FAN_1_RPM, rate);
 }
 
 static int max31785_write_word_data(struct i2c_client *client, int page,
 				    int reg, u16 word)
 {
 	switch (reg) {
+	case PMBUS_VIRT_FAN_TARGET_1:
+		return max31785_update_fan(client, page, PB_FAN_1_RPM,
+					   PB_FAN_1_RPM, word);
 	case PMBUS_VIRT_PWM_1:
-		return pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM,
+		return max31785_update_fan(client, page, 0, PB_FAN_1_RPM,
 					max31785_scale_pwm(word));
 	case PMBUS_VIRT_PWM_ENABLE_1:
 		return max31785_pwm_enable(client, page, word);
@@ -246,7 +359,10 @@ static int max31785_write_word_data(struct i2c_client *client, int page,
 		break;
 	}
 
-	return -ENODATA;
+	if (reg < PMBUS_VIRT_BASE)
+		return max31785_pmbus_write_word_data(client, page, reg, word);
+
+	return -ENXIO;
 }
 
 /*
@@ -279,11 +395,11 @@ static int max31785_of_fan_config(struct i2c_client *client,
 		return -ENXIO;
 	}
 
-	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
+	ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
 	if (ret < 0)
 		return ret;
 
-	pb_cfg = i2c_smbus_read_byte_data(client, PMBUS_FAN_CONFIG_12);
+	pb_cfg = max31785_i2c_smbus_read_byte_data(client, PMBUS_FAN_CONFIG_12);
 	if (pb_cfg < 0)
 		return pb_cfg;
 
@@ -309,7 +425,9 @@ static int max31785_of_fan_config(struct i2c_client *client,
 	} else if (!strcmp("lock", sval)) {
 		mfr_cfg |= MFR_FAN_CONFIG_ROTOR;
 
-		ret = i2c_smbus_write_word_data(client, MFR_FAN_FAULT_LIMIT, 1);
+		ret = max31785_i2c_smbus_write_word_data(client,
+							 MFR_FAN_FAULT_LIMIT,
+							 1);
 		if (ret < 0)
 			return ret;
 
@@ -423,21 +541,23 @@ static int max31785_of_fan_config(struct i2c_client *client,
 	if (of_property_read_bool(child, "maxim,fan-fault-pin-mon"))
 		mfr_fault_resp |= MFR_FAULT_RESPONSE_MONITOR;
 
-	ret = i2c_smbus_write_byte_data(client, PMBUS_FAN_CONFIG_12,
+	ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_FAN_CONFIG_12,
 					pb_cfg & ~PB_FAN_1_INSTALLED);
 	if (ret < 0)
 		return ret;
 
-	ret = i2c_smbus_write_word_data(client, MFR_FAN_CONFIG, mfr_cfg);
+	ret = max31785_i2c_smbus_write_word_data(client, MFR_FAN_CONFIG,
+						 mfr_cfg);
 	if (ret < 0)
 		return ret;
 
-	ret = i2c_smbus_write_byte_data(client, MFR_FAULT_RESPONSE,
-					mfr_fault_resp);
+	ret = max31785_i2c_smbus_write_byte_data(client, MFR_FAULT_RESPONSE,
+						 mfr_fault_resp);
 	if (ret < 0)
 		return ret;
 
-	ret = i2c_smbus_write_byte_data(client, PMBUS_FAN_CONFIG_12, pb_cfg);
+	ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_FAN_CONFIG_12,
+						 pb_cfg);
 	if (ret < 0)
 		return ret;
 
@@ -482,7 +602,7 @@ static int max31785_of_tmp_config(struct i2c_client *client,
 		return -ENXIO;
 	}
 
-	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
+	ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
 	if (ret < 0)
 		return ret;
 
@@ -506,7 +626,7 @@ static int max31785_of_tmp_config(struct i2c_client *client,
 		i++;
 	}
 
-	ret = i2c_smbus_write_word_data(client, MFR_TEMP_SENSOR_CONFIG,
+	ret = max31785_i2c_smbus_write_word_data(client, MFR_TEMP_SENSOR_CONFIG,
 					mfr_tmp_cfg);
 	if (ret < 0)
 		return ret;
@@ -585,11 +705,11 @@ static int max31785_configure_dual_tach(struct i2c_client *client,
 	int i;
 
 	for (i = 0; i < MAX31785_NR_FAN_PAGES; i++) {
-		ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
+		ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
 		if (ret < 0)
 			return ret;
 
-		ret = i2c_smbus_read_word_data(client, MFR_FAN_CONFIG);
+		ret = max31785_i2c_smbus_read_word_data(client, MFR_FAN_CONFIG);
 		if (ret < 0)
 			return ret;
 
@@ -627,7 +747,7 @@ static int max31785_probe(struct i2c_client *client,
 
 	*info = max31785_info;
 
-	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255);
+	ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255);
 	if (ret < 0)
 		return ret;
 
@@ -669,17 +789,20 @@ static int max31785_probe(struct i2c_client *client,
 		if (!have_fan || fan_configured)
 			continue;
 
-		ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
+		ret = max31785_i2c_smbus_write_byte_data(client, PMBUS_PAGE,
+							 i);
 		if (ret < 0)
 			return ret;
 
-		ret = i2c_smbus_read_byte_data(client, PMBUS_FAN_CONFIG_12);
+		ret = max31785_i2c_smbus_read_byte_data(client,
+							PMBUS_FAN_CONFIG_12);
 		if (ret < 0)
 			return ret;
 
 		ret &= ~PB_FAN_1_INSTALLED;
-		ret = i2c_smbus_write_word_data(client, PMBUS_FAN_CONFIG_12,
-						ret);
+		ret = max31785_i2c_smbus_write_word_data(client,
+							 PMBUS_FAN_CONFIG_12,
+							 ret);
 		if (ret < 0)
 			return ret;
 	}
-- 
2.14.1

Re: [PATCH linux dev-4.13 3/5] pmbus (core): One-shot retries for failure to set page

[Eddie James]

On 04/03/2018 09:26 AM, Andrew Jeffery wrote:
> Work around the shonky behaviour seen with the MAX31785 where we fail
> to set the page register in some circumstances.
>
> There's no real elegant way to do this. We can propagate the error up,
> but that forces us to retry the operation way up the call tree in any
> number of places. It also forces callers to split out pmbus_set_page()
> from the pmbus_{read,write}_{byte,word}_data() functions in order to
> differentiate between a failure to set the page and a failure to read a
> register (that might not exist, in which case an error is anticiptated).

Reviewed-by: Eddie James <eajames@linux.vnet.ibm.com>

>
> Cc: Eddie James <eajames@linux.vnet.ibm.com>
> Cc: Matt Spinler <mspinler@linux.vnet.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index ee7e0684c29f..ee1bf11c7fc6 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -167,9 +167,17 @@ int pmbus_set_page(struct i2c_client *client, int page)
>   		return 0;
>
>   	if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
> +		dev_dbg(&client->dev, "Want page %u, %u cached\n", page,
> +			data->currpage);
> +
>   		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> -		if (rv < 0)
> -			return rv;
> +		if (rv) {
> +			rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE,
> +						       page);
> +			dev_dbg(&client->dev,
> +				"Failed to set page %u, performed one-shot retry %s: %d\n",
> +				page, rv ? "and failed" : "with success", rv);
> +		}
>
>   		rv = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
>   		if (rv < 0)

Re: [PATCH linux dev-4.13 4/5] pmbus (core): Use driver callbacks in pmbus_get_fan_rate()

[Eddie James]

On 04/03/2018 09:26 AM, Andrew Jeffery wrote:
> The driver may have overridden the pmbus_read_byte_data() callback, so
> make sure we use that to achieve expected behaviour.
>
> This helps in the MAX31785 case where we may need to perform a one-shot
> retry of transfers in the face of a failure.

Reviewed-by: Eddie James <eajames@linux.vnet.ibm.com>

>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/hwmon/pmbus/pmbus_core.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index ee1bf11c7fc6..d0ffa4b3fcbd 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -453,15 +453,15 @@ static int pmbus_get_fan_rate(struct i2c_client *client, int page, int id,
>   		return s->data;
>   	}
>
> -	config = pmbus_read_byte_data(client, page,
> -				      pmbus_fan_config_registers[id]);
> +	config = _pmbus_read_byte_data(client, page,
> +				       pmbus_fan_config_registers[id]);
>   	if (config < 0)
>   		return config;
>
>   	have_rpm = !!(config & pmbus_fan_rpm_mask[id]);
>   	if (want_rpm == have_rpm)
> -		return pmbus_read_word_data(client, page,
> -					    pmbus_fan_command_registers[id]);
> +		return _pmbus_read_word_data(client, page,
> +					     pmbus_fan_command_registers[id]);
>
>   	/* Can't sensibly map between RPM and PWM, just return zero */
>   	return 0;