[PATCH v3 0/3] pmbus: Expand fan support and add MAX31785 driver

[PATCH v3 0/3] pmbus: Expand fan support and add MAX31785 driver

[Andrew Jeffery]

Hello,

These three patches lay the ground work for fan control in the pmbus core and
introduce a driver for the MAX31785 Intelligent Fan Controller that makes use
of the new control features.

Since v2[1] I've addressed Rob's comments on the bindings, integrating the thermal
support and cleaning up some warts. One of the warts was regarding the
maxim,tmp-fans property which was switched to use a list of fan phandles rather
than indexes. A corresponding change is made in the driver. I also did a
comparison with the Aspeed PWM/Tach bindings but I'm not convinced there's much
to take away from it other than those bindings are somewhat broken.

In v3 I've dropped patch 4 from v2, which was a bunch of work-arounds for bad
behaviour I observed in testing. At this stage the bad behaviour appears to be
a product of the larger hardware design, not necessarily a problem with the
Maxim chip. Guenter has performed some quick testing of a cut-down v2 driver
against the MAX31785 evaluation board and couldn't reproduce the results I was
seeing[2][3], which increases the likelyhood that it's not the Maxim chip and
the work-around patch is inappropriate.

Please review!

Andrew

[1] https://lkml.org/lkml/2017/8/2/88
[2] https://lkml.org/lkml/2017/9/7/669
[3] https://lkml.org/lkml/2017/9/5/53

Andrew Jeffery (3):
  dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation
  hwmon: pmbus: Add fan control support
  pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller

 .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 +++++
 drivers/hwmon/pmbus/Kconfig                        |  10 +
 drivers/hwmon/pmbus/Makefile                       |   1 +
 drivers/hwmon/pmbus/max31785.c                     | 673 +++++++++++++++++++++
 drivers/hwmon/pmbus/pmbus.h                        |  29 +
 drivers/hwmon/pmbus/pmbus_core.c                   | 224 ++++++-
 6 files changed, 1080 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/max31785.txt
 create mode 100644 drivers/hwmon/pmbus/max31785.c

-- 
2.11.0

[PATCH v3 1/3] 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.11.0

[PATCH v3 2/3] hwmon: pmbus: Add fan control support

[Andrew Jeffery]

Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes.

Fans in a PMBus device are driven by the configuration of two registers:
FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan
and the tacho operate (if installed), while FAN_COMMAND_x sets the
desired fan rate. The unit of FAN_COMMAND_x is dependent on the
operational fan mode, RPM or PWM percent duty, as determined by the
corresponding FAN_CONFIG_x_y.

The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and
FAN_COMMAND_x is implemented with the addition of virtual registers and
generic implementations in the core:

1. PMBUS_VIRT_FAN_TARGET_x
2. PMBUS_VIRT_PWM_x
3. PMBUS_VIRT_PWM_ENABLE_x

The virtual registers facilitate the necessary side-effects of each
access. Examples of the read case, assuming m = 1, b = 0, R = 0:

             Read     |              With              || Gives
         -------------------------------------------------------
           Attribute  | FAN_CONFIG_x_y | FAN_COMMAND_y || Value
         ----------------------------------------------++-------
          fanX_target | ~PB_FAN_z_RPM  | 0x0001        || 1
          pwm1        | ~PB_FAN_z_RPM  | 0x0064        || 255
          pwmX_enable | ~PB_FAN_z_RPM  | 0x0001        || 1
          fanX_target |  PB_FAN_z_RPM  | 0x0001        || 1
          pwm1        |  PB_FAN_z_RPM  | 0x0064        || 0
          pwmX_enable |  PB_FAN_z_RPM  | 0x0001        || 1

And the write case:

             Write    | With  ||               Sets
         -------------+-------++----------------+---------------
           Attribute  | Value || FAN_CONFIG_x_y | FAN_COMMAND_x
         -------------+-------++----------------+---------------
          fanX_target | 1     ||  PB_FAN_z_RPM  | 0x0001
          pwmX        | 255   || ~PB_FAN_z_RPM  | 0x0064
          pwmX_enable | 1     || ~PB_FAN_z_RPM  | 0x0064

Also, the DIRECT mode scaling of some controllers is different between
RPM and PWM percent duty control modes, so PSC_PWM is introduced to
capture the necessary coefficients.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/pmbus/pmbus.h      |  29 +++++
 drivers/hwmon/pmbus/pmbus_core.c | 224 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 238 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13bae34b..a863b8fed16f 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -190,6 +190,28 @@ enum pmbus_regs {
 	PMBUS_VIRT_VMON_UV_FAULT_LIMIT,
 	PMBUS_VIRT_VMON_OV_FAULT_LIMIT,
 	PMBUS_VIRT_STATUS_VMON,
+
+	/*
+	 * RPM and PWM Fan control
+	 *
+	 * Drivers wanting to expose PWM control must define the behaviour of
+	 * PMBUS_VIRT_PWM_ENABLE_[1-4] in the {read,write}_word_data callback.
+	 *
+	 * pmbus core provides default implementations for
+	 * PMBUS_VIRT_FAN_TARGET_[1-4] and PMBUS_VIRT_PWM_[1-4].
+	 */
+	PMBUS_VIRT_FAN_TARGET_1,
+	PMBUS_VIRT_FAN_TARGET_2,
+	PMBUS_VIRT_FAN_TARGET_3,
+	PMBUS_VIRT_FAN_TARGET_4,
+	PMBUS_VIRT_PWM_1,
+	PMBUS_VIRT_PWM_2,
+	PMBUS_VIRT_PWM_3,
+	PMBUS_VIRT_PWM_4,
+	PMBUS_VIRT_PWM_ENABLE_1,
+	PMBUS_VIRT_PWM_ENABLE_2,
+	PMBUS_VIRT_PWM_ENABLE_3,
+	PMBUS_VIRT_PWM_ENABLE_4,
 };
 
 /*
@@ -223,6 +245,8 @@ enum pmbus_regs {
 #define PB_FAN_1_RPM			BIT(6)
 #define PB_FAN_1_INSTALLED		BIT(7)
 
+enum pmbus_fan_mode { percent = 0, rpm };
+
 /*
  * STATUS_BYTE, STATUS_WORD (lower)
  */
@@ -313,6 +337,7 @@ enum pmbus_sensor_classes {
 	PSC_POWER,
 	PSC_TEMPERATURE,
 	PSC_FAN,
+	PSC_PWM,
 	PSC_NUM_CLASSES		/* Number of power sensor classes */
 };
 
@@ -339,6 +364,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_STATUS_FAN34	BIT(17)
 #define PMBUS_HAVE_VMON		BIT(18)
 #define PMBUS_HAVE_STATUS_VMON	BIT(19)
+#define PMBUS_HAVE_PWM12	BIT(20)
+#define PMBUS_HAVE_PWM34	BIT(21)
 
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12 };
@@ -413,6 +440,8 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg,
 			  u8 value);
 int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg,
 			   u8 mask, u8 value);
+int pmbus_update_fan(struct i2c_client *client, int page, int id,
+		     u8 config, u8 mask, u16 command);
 void pmbus_clear_faults(struct i2c_client *client);
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg);
 bool pmbus_check_word_register(struct i2c_client *client, int page, int reg);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index f1eff6b6c798..5ed9cbf1daf9 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -63,6 +63,7 @@ struct pmbus_sensor {
 	u16 reg;		/* register */
 	enum pmbus_sensor_classes class;	/* sensor class */
 	bool update;		/* runtime sensor update needed */
+	bool convert;		/* Whether or not to apply linear/vid/direct */
 	int data;		/* Sensor data.
 				   Negative if there was a read error */
 };
@@ -118,6 +119,27 @@ struct pmbus_data {
 	u8 currpage;
 };
 
+static const int pmbus_fan_rpm_mask[] = {
+	PB_FAN_1_RPM,
+	PB_FAN_2_RPM,
+	PB_FAN_1_RPM,
+	PB_FAN_2_RPM,
+};
+
+static const int pmbus_fan_config_registers[] = {
+	PMBUS_FAN_CONFIG_12,
+	PMBUS_FAN_CONFIG_12,
+	PMBUS_FAN_CONFIG_34,
+	PMBUS_FAN_CONFIG_34
+};
+
+static const int pmbus_fan_command_registers[] = {
+	PMBUS_FAN_COMMAND_1,
+	PMBUS_FAN_COMMAND_2,
+	PMBUS_FAN_COMMAND_3,
+	PMBUS_FAN_COMMAND_4,
+};
+
 void pmbus_clear_cache(struct i2c_client *client)
 {
 	struct pmbus_data *data = i2c_get_clientdata(client);
@@ -188,6 +210,31 @@ int pmbus_write_word_data(struct i2c_client *client, u8 page, u8 reg, u16 word)
 }
 EXPORT_SYMBOL_GPL(pmbus_write_word_data);
 
+int pmbus_update_fan(struct i2c_client *client, int page, int id,
+			       u8 config, u8 mask, u16 command)
+{
+	int from, rv;
+	u8 to;
+
+	from = pmbus_read_byte_data(client, page,
+				    pmbus_fan_config_registers[id]);
+	if (from < 0)
+		return from;
+
+	to = (from & ~mask) | (config & mask);
+
+	if (to != from) {
+		rv = pmbus_write_byte_data(client, page,
+					   pmbus_fan_config_registers[id], to);
+		if (rv < 0)
+			return rv;
+	}
+
+	return pmbus_write_word_data(client, page,
+				     pmbus_fan_command_registers[id], command);
+}
+EXPORT_SYMBOL_GPL(pmbus_update_fan);
+
 /*
  * _pmbus_write_word_data() is similar to pmbus_write_word_data(), but checks if
  * a device specific mapping function exists and calls it if necessary.
@@ -204,8 +251,40 @@ static int _pmbus_write_word_data(struct i2c_client *client, int page, int reg,
 		if (status != -ENODATA)
 			return status;
 	}
-	if (reg >= PMBUS_VIRT_BASE)
-		return -ENXIO;
+	if (reg >= PMBUS_VIRT_BASE) {
+		int id, bit;
+
+		switch (reg) {
+		case PMBUS_VIRT_FAN_TARGET_1:
+		case PMBUS_VIRT_FAN_TARGET_2:
+		case PMBUS_VIRT_FAN_TARGET_3:
+		case PMBUS_VIRT_FAN_TARGET_4:
+			id = reg - PMBUS_VIRT_FAN_TARGET_1;
+			bit = pmbus_fan_rpm_mask[id];
+			status = pmbus_update_fan(client, page, id, bit, bit,
+						  word);
+			break;
+		case PMBUS_VIRT_PWM_1:
+		case PMBUS_VIRT_PWM_2:
+		case PMBUS_VIRT_PWM_3:
+		case PMBUS_VIRT_PWM_4:
+		{
+			u32 command = word;
+
+			id = reg - PMBUS_VIRT_PWM_1;
+			bit = pmbus_fan_rpm_mask[id];
+			command *= 100;
+			command /= 255;
+			status = pmbus_update_fan(client, page, id, 0, bit,
+						  command);
+			break;
+		}
+		default:
+			status = -ENXIO;
+			break;
+		}
+		return status;
+	}
 	return pmbus_write_word_data(client, page, reg, word);
 }
 
@@ -221,6 +300,9 @@ int pmbus_read_word_data(struct i2c_client *client, u8 page, u8 reg)
 }
 EXPORT_SYMBOL_GPL(pmbus_read_word_data);
 
+static int pmbus_get_fan_command(struct i2c_client *client, int page, int id,
+				 enum pmbus_fan_mode mode);
+
 /*
  * _pmbus_read_word_data() is similar to pmbus_read_word_data(), but checks if
  * a device specific mapping function exists and calls it if necessary.
@@ -236,8 +318,42 @@ static int _pmbus_read_word_data(struct i2c_client *client, int page, int reg)
 		if (status != -ENODATA)
 			return status;
 	}
-	if (reg >= PMBUS_VIRT_BASE)
-		return -ENXIO;
+	if (reg >= PMBUS_VIRT_BASE) {
+		int id;
+
+		switch (reg) {
+		case PMBUS_VIRT_FAN_TARGET_1:
+		case PMBUS_VIRT_FAN_TARGET_2:
+		case PMBUS_VIRT_FAN_TARGET_3:
+		case PMBUS_VIRT_FAN_TARGET_4:
+			id = reg - PMBUS_VIRT_FAN_TARGET_1;
+			status = pmbus_get_fan_command(client, page, id, rpm);
+			break;
+		case PMBUS_VIRT_PWM_1:
+		case PMBUS_VIRT_PWM_2:
+		case PMBUS_VIRT_PWM_3:
+		case PMBUS_VIRT_PWM_4:
+		{
+			int rv;
+
+			id = reg - PMBUS_VIRT_PWM_1;
+			rv = pmbus_get_fan_command(client, page, id, percent);
+			if (rv < 0)
+				return rv;
+
+			rv *= 255;
+			rv /= 100;
+
+			status = rv;
+			break;
+		}
+		default:
+			status = -ENXIO;
+			break;
+		}
+
+		return status;
+	}
 	return pmbus_read_word_data(client, page, reg);
 }
 
@@ -304,6 +420,28 @@ static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
 	return pmbus_read_byte_data(client, page, reg);
 }
 
+static int pmbus_get_fan_command(struct i2c_client *client, int page, int id,
+				 enum pmbus_fan_mode mode)
+{
+	int config;
+
+	config = _pmbus_read_byte_data(client, page,
+				       pmbus_fan_config_registers[id]);
+	if (config < 0)
+		return config;
+
+	/*
+	 * We can't meaningfully translate between PWM and RPM, so if the
+	 * attribute mode (fan[1-*]_target is RPM, pwm[1-*] and pwm[1-*]_enable
+	 * are PWM) doesn't match the hardware mode, then report 0 instead.
+	 */
+	if ((mode == rpm) != (!!(config & pmbus_fan_rpm_mask[id])))
+		return 0;
+
+	return _pmbus_read_word_data(client, page,
+				     pmbus_fan_command_registers[id]);
+}
+
 static void pmbus_clear_fault_page(struct i2c_client *client, int page)
 {
 	_pmbus_write_byte(client, page, PMBUS_CLEAR_FAULTS);
@@ -489,7 +627,7 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
 	/* X = 1/m * (Y * 10^-R - b) */
 	R = -R;
 	/* scale result to milli-units for everything but fans */
-	if (sensor->class != PSC_FAN) {
+	if (!(sensor->class == PSC_FAN || sensor->class == PSC_PWM)) {
 		R += 3;
 		b *= 1000;
 	}
@@ -539,6 +677,9 @@ static long pmbus_reg2data(struct pmbus_data *data, struct pmbus_sensor *sensor)
 {
 	long val;
 
+	if (!sensor->convert)
+		return sensor->data;
+
 	switch (data->info->format[sensor->class]) {
 	case direct:
 		val = pmbus_reg2data_direct(data, sensor);
@@ -642,7 +783,7 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
 	}
 
 	/* Calculate Y = (m * X + b) * 10^R */
-	if (sensor->class != PSC_FAN) {
+	if (!(sensor->class == PSC_FAN || sensor->class == PSC_PWM)) {
 		R -= 3;		/* Adjust R and b for data in milli-units */
 		b *= 1000;
 	}
@@ -673,6 +814,9 @@ static u16 pmbus_data2reg(struct pmbus_data *data,
 {
 	u16 regval;
 
+	if (!sensor->convert)
+		return val;
+
 	switch (data->info->format[sensor->class]) {
 	case direct:
 		regval = pmbus_data2reg_direct(data, sensor, val);
@@ -895,12 +1039,18 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
 		return NULL;
 	a = &sensor->attribute;
 
-	snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
-		 name, seq, type);
+	if (type)
+		snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
+			 name, seq, type);
+	else
+		snprintf(sensor->name, sizeof(sensor->name), "%s%d",
+			 name, seq);
+
 	sensor->page = page;
 	sensor->reg = reg;
 	sensor->class = class;
 	sensor->update = update;
+	sensor->convert = true;
 	pmbus_dev_attr_init(a, sensor->name,
 			    readonly ? S_IRUGO : S_IRUGO | S_IWUSR,
 			    pmbus_show_sensor, pmbus_set_sensor);
@@ -1558,13 +1708,6 @@ static const int pmbus_fan_registers[] = {
 	PMBUS_READ_FAN_SPEED_4
 };
 
-static const int pmbus_fan_config_registers[] = {
-	PMBUS_FAN_CONFIG_12,
-	PMBUS_FAN_CONFIG_12,
-	PMBUS_FAN_CONFIG_34,
-	PMBUS_FAN_CONFIG_34
-};
-
 static const int pmbus_fan_status_registers[] = {
 	PMBUS_STATUS_FAN_12,
 	PMBUS_STATUS_FAN_12,
@@ -1587,6 +1730,48 @@ static const u32 pmbus_fan_status_flags[] = {
 };
 
 /* Fans */
+static int pmbus_add_fan_ctrl(struct i2c_client *client,
+		struct pmbus_data *data, int index, int page, int id,
+		u8 config)
+{
+	struct pmbus_sensor *sensor;
+	int rv;
+
+	rv = _pmbus_read_word_data(client, page,
+				   pmbus_fan_command_registers[id]);
+	if (rv < 0)
+		return rv;
+
+	sensor = pmbus_add_sensor(data, "fan", "target", index, page,
+				  PMBUS_VIRT_FAN_TARGET_1 + id, PSC_FAN,
+				  true, false);
+
+	if (!sensor)
+		return -ENOMEM;
+
+	if (!((data->info->func[page] & PMBUS_HAVE_PWM12) ||
+			(data->info->func[page] & PMBUS_HAVE_PWM34)))
+		return 0;
+
+	sensor = pmbus_add_sensor(data, "pwm", NULL, index, page,
+				  PMBUS_VIRT_PWM_1 + id, PSC_PWM,
+				  true, false);
+
+	if (!sensor)
+		return -ENOMEM;
+
+	sensor = pmbus_add_sensor(data, "pwm", "enable", index, page,
+				  PMBUS_VIRT_PWM_ENABLE_1 + id, PSC_PWM,
+				  true, false);
+
+	if (!sensor)
+		return -ENOMEM;
+
+	sensor->convert = false;
+
+	return 0;
+}
+
 static int pmbus_add_fan_attributes(struct i2c_client *client,
 				    struct pmbus_data *data)
 {
@@ -1624,6 +1809,15 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
 					     PSC_FAN, true, true) == NULL)
 				return -ENOMEM;
 
+			/* Fan control */
+			if (pmbus_check_word_register(client, page,
+					pmbus_fan_command_registers[f])) {
+				ret = pmbus_add_fan_ctrl(client, data, index,
+							 page, f, regval);
+				if (ret < 0)
+					return ret;
+			}
+
 			/*
 			 * Each fan status register covers multiple fans,
 			 * so we have to do some magic.
-- 
2.11.0

[PATCH v3 3/3] pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller

[Andrew Jeffery]

The Maxim MAX31785 is a PMBus device providing closed-loop,
multi-channel fan management with temperature and remote voltage
sensing. Various fan control features are provided, including PWM
frequency control, temperature hysteresis, dual tachometer measurements,
and fan health monitoring.

The driver implementation makes use of the new fan control virtual
registers exposed by the pmbus core. It mixes use of the default
implementations with some overrides via the read/write handlers to
handle FAN_COMMAND_1 on the MAX31785, whose definition breaks the value
range into various control bands dependent on RPM or PWM mode.

The dual tachometer feature is implemented in hardware with a TACHSEL
input to indicate the rotor under measurement, and exposed on the bus by
extending the READ_FAN_SPEED_1 word with two extra bytes*.
The need to read the non-standard four-byte response leads to a cut-down
implementation of i2c_smbus_xfer_emulated() included in the driver.
Further, to expose the second rotor tachometer value to userspace,
virtual fans are implemented by re-routing the FAN_CONFIG_1_2 register
from the undefined (in hardware) pages 23-28 to the same register on
pages 0-5, and similarly re-routing READ_FAN_SPEED_1 requests on 23-28
to pages 0-5 but extracting the second word of the four-byte response.

* The documentation recommends the slower rotor be associated with
TACHSEL=0, which provides the first word of the response. The TACHSEL=0
measurement is used by the controller's closed-loop fan management.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/hwmon/pmbus/Kconfig    |  10 +
 drivers/hwmon/pmbus/Makefile   |   1 +
 drivers/hwmon/pmbus/max31785.c | 673 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 684 insertions(+)
 create mode 100644 drivers/hwmon/pmbus/max31785.c

diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 68d717a3fd59..04f6baa98a14 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -105,6 +105,16 @@ config SENSORS_MAX20751
 	  This driver can also be built as a module. If so, the module will
 	  be called max20751.
 
+config SENSORS_MAX31785
+	tristate "Maxim MAX31785 and compatibles"
+	default n
+	help
+	  If you say yes here you get hardware monitoring support for Maxim
+	  MAX31785.
+
+	  This driver can also be built as a module. If so, the module will
+	  be called max31785.
+
 config SENSORS_MAX34440
 	tristate "Maxim MAX34440 and compatibles"
 	default n
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 75bb7ca619d9..663801c57a82 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_SENSORS_LTC2978)	+= ltc2978.o
 obj-$(CONFIG_SENSORS_LTC3815)	+= ltc3815.o
 obj-$(CONFIG_SENSORS_MAX16064)	+= max16064.o
 obj-$(CONFIG_SENSORS_MAX20751)	+= max20751.o
+obj-$(CONFIG_SENSORS_MAX31785)	+= max31785.o
 obj-$(CONFIG_SENSORS_MAX34440)	+= max34440.o
 obj-$(CONFIG_SENSORS_MAX8688)	+= max8688.o
 obj-$(CONFIG_SENSORS_TPS40422)	+= tps40422.o
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
new file mode 100644
index 000000000000..a83f59ca31c7
--- /dev/null
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -0,0 +1,673 @@
+/*
+ * Copyright (C) 2017 IBM Corp.
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include "pmbus.h"
+
+enum max31785_regs {
+	MFR_REVISION		= 0x9b,
+	MFR_FAULT_RESPONSE	= 0xd9,
+	MFR_TEMP_SENSOR_CONFIG	= 0xf0,
+	MFR_FAN_CONFIG		= 0xf1,
+	MFR_READ_FAN_PWM	= 0xf3,
+	MFR_FAN_FAULT_LIMIT	= 0xf5,
+	MFR_FAN_WARN_LIMIT	= 0xf6,
+	MFR_FAN_PWM_AVG		= 0xf8,
+};
+
+#define MAX31785			0x3030
+#define MAX31785A			0x3040
+
+#define MFR_TEMP_SENSOR_CONFIG_ENABLE	BIT(15)
+#define MFR_TEMP_SENSOR_CONFIG_OFFSET	GENMASK(14, 10)
+
+#define MFR_FAN_CONFIG_FREQ		GENMASK(15, 13)
+#define MFR_FAN_CONFIG_DUAL_TACH	BIT(12)
+#define MFR_FAN_CONFIG_HYS		GENMASK(11, 10)
+#define MFR_FAN_CONFIG_TSFO		BIT(9)
+#define MFR_FAN_CONFIG_TACHO		BIT(8)
+#define MFR_FAN_CONFIG_RAMP		GENMASK(7, 5)
+#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_FAN_CONFIG_SPIN		GENMASK(1, 0)
+
+#define MFR_FAULT_RESPONSE_MONITOR	BIT(0)
+
+#define MAX31785_CAP_READ_DUAL_TACH	BIT(0)
+
+#define MAX31785_NR_PAGES		23
+
+static int max31785_read_byte_data(struct i2c_client *client, int page,
+				   int reg)
+{
+	switch (reg) {
+	case PMBUS_VOUT_MODE:
+		if (page < MAX31785_NR_PAGES)
+			return -ENODATA;
+
+		return -ENOTSUPP;
+	case PMBUS_FAN_CONFIG_12:
+		if (page < MAX31785_NR_PAGES)
+			return -ENODATA;
+
+		return pmbus_read_byte_data(client, page - MAX31785_NR_PAGES,
+					    reg);
+	}
+
+	return -ENODATA;
+}
+
+static int max31785_write_byte(struct i2c_client *client, int page, u8 value)
+{
+	if (page < MAX31785_NR_PAGES)
+		return -ENODATA;
+
+	return -ENOTSUPP;
+}
+
+static int max31785_read_long_data(struct i2c_client *client, int page,
+				   int reg, u32 *data)
+{
+	unsigned char cmdbuf[1];
+	unsigned char rspbuf[4];
+	int 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] = reg;
+
+	rc = pmbus_set_page(client, page);
+	if (rc < 0)
+		return rc;
+
+	rc = i2c_transfer(client->adapter, msg, ARRAY_SIZE(msg));
+	if (rc < 0)
+		return rc;
+
+	*data = (rspbuf[0] << (0 * 8)) | (rspbuf[1] << (1 * 8)) |
+		(rspbuf[2] << (2 * 8)) | (rspbuf[3] << (3 * 8));
+
+	return rc;
+}
+
+static int max31785_get_pwm(struct i2c_client *client, int page)
+{
+	int config;
+	int command;
+
+	config = 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);
+	if (command < 0)
+		return command;
+
+	if (!(config & PB_FAN_1_RPM)) {
+		if (command >= 0x8000)
+			return 0;
+		else if (command >= 0x2711)
+			return 0x2710;
+
+		return command;
+	}
+
+	return 0;
+}
+
+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);
+	if (config < 0)
+		return config;
+
+	command = pmbus_read_word_data(client, page, PMBUS_FAN_COMMAND_1);
+	if (command < 0)
+		return command;
+
+	if (!(config & PB_FAN_1_RPM)) {
+		if (command >= 0x8000)
+			return 2;
+		else if (command >= 0x2711)
+			return 0;
+
+		return 1;
+	}
+
+	return (command >= 0x8000) ? 2 : 1;
+}
+
+static int max31785_read_word_data(struct i2c_client *client, int page,
+				   int reg)
+{
+	int rv;
+
+	switch (reg) {
+	case PMBUS_READ_FAN_SPEED_1:
+	{
+		u32 val;
+
+		if (page < MAX31785_NR_PAGES)
+			return -ENODATA;
+
+		rv = max31785_read_long_data(client, page - MAX31785_NR_PAGES,
+					     reg, &val);
+		if (rv < 0)
+			return rv;
+
+		rv = (val >> 16) & 0xffff;
+		break;
+	}
+	case PMBUS_VIRT_PWM_1:
+		if (page >= MAX31785_NR_PAGES)
+			return -ENOTSUPP;
+
+		rv = max31785_get_pwm(client, page);
+		if (rv < 0)
+			return rv;
+
+		rv *= 255;
+		rv /= 100;
+		break;
+	case PMBUS_VIRT_PWM_ENABLE_1:
+		if (page >= MAX31785_NR_PAGES)
+			return -ENOTSUPP;
+
+		rv = max31785_get_pwm_mode(client, page);
+		break;
+	default:
+		rv = (page >= MAX31785_NR_PAGES) ? -ENXIO : -ENODATA;
+		break;
+	}
+
+	return rv;
+}
+
+static const int max31785_pwm_modes[] = { 0x7fff, 0x2710, 0xffff };
+
+static int max31785_write_word_data(struct i2c_client *client, int page,
+				    int reg, u16 word)
+{
+	if (page >= MAX31785_NR_PAGES)
+		return -ENXIO;
+
+	switch (reg) {
+	case PMBUS_VIRT_PWM_ENABLE_1:
+		if (word >= ARRAY_SIZE(max31785_pwm_modes))
+			return -EINVAL;
+
+		return pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM,
+					max31785_pwm_modes[word]);
+	default:
+		break;
+	}
+
+	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,
+				  u32 capabilities, 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 (capabilities & MAX31785_CAP_READ_DUAL_TACH) {
+			int virtual = MAX31785_NR_PAGES + page;
+
+			info->pages = max(info->pages, virtual + 1);
+			info->func[virtual] |= PMBUS_HAVE_FAN12;
+		}
+	}
+
+	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)
+
+#define MAX31785_TEMP_FUNCS \
+	(PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP)
+
+#define MAX31785_VOUT_FUNCS \
+	(PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT)
+
+static const struct pmbus_driver_info max31785_info = {
+	.pages = MAX31785_NR_PAGES,
+
+	.write_word_data = max31785_write_word_data,
+	.read_byte_data = max31785_read_byte_data,
+	.read_word_data = max31785_read_word_data,
+	.write_byte = max31785_write_byte,
+
+	/* RPM */
+	.format[PSC_FAN] = direct,
+	.m[PSC_FAN] = 1,
+	.b[PSC_FAN] = 0,
+	.R[PSC_FAN] = 0,
+	/* PWM */
+	.format[PSC_PWM] = direct,
+	.m[PSC_PWM] = 1,
+	.b[PSC_PWM] = 0,
+	.R[PSC_PWM] = 2,
+	.func[0] = MAX31785_FAN_FUNCS,
+	.func[1] = MAX31785_FAN_FUNCS,
+	.func[2] = MAX31785_FAN_FUNCS,
+	.func[3] = MAX31785_FAN_FUNCS,
+	.func[4] = MAX31785_FAN_FUNCS,
+	.func[5] = MAX31785_FAN_FUNCS,
+
+	.format[PSC_TEMPERATURE] = direct,
+	.m[PSC_TEMPERATURE] = 1,
+	.b[PSC_TEMPERATURE] = 0,
+	.R[PSC_TEMPERATURE] = 2,
+	.func[6]  = MAX31785_TEMP_FUNCS,
+	.func[7]  = MAX31785_TEMP_FUNCS,
+	.func[8]  = MAX31785_TEMP_FUNCS,
+	.func[9]  = MAX31785_TEMP_FUNCS,
+	.func[10] = MAX31785_TEMP_FUNCS,
+	.func[11] = MAX31785_TEMP_FUNCS,
+	.func[12] = MAX31785_TEMP_FUNCS,
+	.func[13] = MAX31785_TEMP_FUNCS,
+	.func[14] = MAX31785_TEMP_FUNCS,
+	.func[15] = MAX31785_TEMP_FUNCS,
+	.func[16] = MAX31785_TEMP_FUNCS,
+
+	.format[PSC_VOLTAGE_OUT] = direct,
+	.m[PSC_VOLTAGE_OUT] = 1,
+	.b[PSC_VOLTAGE_OUT] = 0,
+	.R[PSC_VOLTAGE_OUT] = 0,
+	.func[17] = MAX31785_VOUT_FUNCS,
+	.func[18] = MAX31785_VOUT_FUNCS,
+	.func[19] = MAX31785_VOUT_FUNCS,
+	.func[20] = MAX31785_VOUT_FUNCS,
+	.func[21] = MAX31785_VOUT_FUNCS,
+	.func[22] = MAX31785_VOUT_FUNCS,
+};
+
+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;
+	u32 fans;
+	u32 caps;
+	s64 ret;
+	int i;
+
+	info = devm_kzalloc(dev, sizeof(struct pmbus_driver_info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	*info = max31785_info;
+
+	ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, 255);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_read_word_data(client, MFR_REVISION);
+	if (ret < 0)
+		return ret;
+
+	caps = 0;
+	if (!strcmp("max31785a", id->name)) {
+		if (ret == MAX31785A)
+			caps |= MAX31785_CAP_READ_DUAL_TACH;
+		else
+			dev_warn(dev, "Expected max3175a, found max31785: cannot provide secondary tachometer readings\n");
+	} else if (!strcmp("max31785", id->name)) {
+		if (ret == MAX31785A)
+			dev_info(dev, "Expected max31785, found max3175a: suppressing secondary tachometer attributes\n");
+	} else {
+		return -EINVAL;
+	}
+
+
+	fans = 0;
+	for_each_child_of_node(dev->of_node, child) {
+		ret = max31785_of_fan_config(client, info, caps, 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;
+	}
+
+	return pmbus_do_probe(client, id, info);
+}
+
+static const struct i2c_device_id max31785_id[] = {
+	{ "max31785", 0 },
+	{ "max31785a", 0 },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(i2c, max31785_id);
+
+static struct i2c_driver max31785_driver = {
+	.driver = {
+		.name = "max31785",
+	},
+	.probe = max31785_probe,
+	.remove = pmbus_do_remove,
+	.id_table = max31785_id,
+};
+
+module_i2c_driver(max31785_driver);
+
+MODULE_AUTHOR("Andrew Jeffery <andrew@aj.id.au>");
+MODULE_DESCRIPTION("PMBus driver for the Maxim MAX31785");
+MODULE_LICENSE("GPL");
-- 
2.11.0

Re: [PATCH v3 1/3] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Rob Herring]

On Fri, Sep 08, 2017 at 02:39:17PM +1000, Andrew Jeffery wrote:
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 +++++++++++++++++++++

I think this needs to be located by what it does (fan control), not what 
interface it has (pmbus).

I'm not all that happy with hwmon either because things here seem to 
just be based on being Linux hwmon devices which is sometimes arbitrary. 

>  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

You should define the IDs here, not just how many of each type.

> +
> +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.

Boolean? Please be explicit.

Needs a vendor prefix.

> +
> +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"

"pmbus" is a property of the controller, not the fan, right? We should 
have compatibles along the lines of the type of fan like 4-wire, 3-wire, 
etc. 

> +- 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

In general, I think a good portion of these should be either implied by 
a fan compatible string or be part of a common fan binding.

> +
> +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.

Is this a feature of the Maxim chip or just a mapping of temperature 
sensors to fans. The latter should be a common property.

Rob

Re: [PATCH v3 1/3] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Guenter Roeck]

On Mon, Sep 18, 2017 at 02:26:38PM -0500, Rob Herring wrote:
> On Fri, Sep 08, 2017 at 02:39:17PM +1000, Andrew Jeffery wrote:
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 +++++++++++++++++++++
> 
> I think this needs to be located by what it does (fan control), not what 
> interface it has (pmbus).
> 
> I'm not all that happy with hwmon either because things here seem to 
> just be based on being Linux hwmon devices which is sometimes arbitrary. 
> 
The chip also measures temperatures. Other PMBus chips may do fan control,
measure temperatures, measure and/or control voltages, current, power ...
Strictly speaking pretty much all PMBus chips are multi-function devices.
I personally don't really care if the documentation is spread across
several directories, but even here this is already challenging.

Only solution I can think of would be to create separate documents for each
functionality, ie here one for the device itself, one for fan control,
and one for temperature control (if that needs separate bindings). That
would be similar to mfd. But then we would still have to sort out where
to store the various bindings. Like iio, in subdirectories ? Like mfd,
in the matching subsystems ? If so, what to do if there is no matching
subsystem ?

Lots of questions. I'll be happy to spend some time sorting it out,
but I would need some directions.

Thanks,
Guenter

> >  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
> 
> You should define the IDs here, not just how many of each type.
> 
> > +
> > +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.
> 
> Boolean? Please be explicit.
> 
> Needs a vendor prefix.
> 
> > +
> > +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"
> 
> "pmbus" is a property of the controller, not the fan, right? We should 
> have compatibles along the lines of the type of fan like 4-wire, 3-wire, 
> etc. 
> 
> > +- 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
> 
> In general, I think a good portion of these should be either implied by 
> a fan compatible string or be part of a common fan binding.
> 
> > +
> > +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.
> 
> Is this a feature of the Maxim chip or just a mapping of temperature 
> sensors to fans. The latter should be a common property.
> 
> Rob

Re: [PATCH v3 1/3] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 10550 bytes --]

On Mon, 2017-09-18 at 14:26 -0500, Rob Herring wrote:
> On Fri, Sep 08, 2017 at 02:39:17PM +1000, Andrew Jeffery wrote:
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 +++++++++++++++++++++
> 
> I think this needs to be located by what it does (fan control), not what 
> interface it has (pmbus).
> 
> I'm not all that happy with hwmon either because things here seem to 
> just be based on being Linux hwmon devices which is sometimes arbitrary.

I'll follow-up on this on Guenter's reply if necessary.

>  
> 
> >  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
> 
> You should define the IDs here, not just how many of each type.

Okay.

> 
> > +
> > +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.
> 
> Boolean? Please be explicit.

Okay.

> 
> Needs a vendor prefix.

We've discussed this previously. It's describing how to interpret part
of the PMBus spec, not something Maxim have done, so I don't think it
should have a vendor prefix.

But maybe I'm representing it wrong?

> 
> > +
> > +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"
> 
> "pmbus" is a property of the controller, not the fan, right? We should 
> have compatibles along the lines of the type of fan like 4-wire, 3-wire, 
> etc. 

Yes, PMBus is a property of the controller. But a controller that
implements PMBus fan control and monitoring abstracts away most of the
details of the fan hardware, so I don't see it fit to describe e.g. 4-
wire or 3-wire properties here.

Perhaps this is resolved by having a "pmbus-fan-control" compatible
node, and nesting a fan node under that? My concern would be that the
only element of the fan subnode would be the "tach-pulses" property
(though maybe also dual-tach if we generalise the maxim,fan-dual-tach
property below).

Regardless, what I'm trying to describe here with the non-vendor-
prefixed properties are configurable elements of the PMBus interface
for fan control. Adherence to PMBus by a device dictates the command
interface, and is therefore a property of the controller hardware at
the end of the day. In this instance (PMBus) I don't see how we can
draw the distinction between "what it does" and "what interface it has"
- adherence to relevant parts of the PMBus spec defines the minimum of
what the controller does (PMBus allows for vendor extensions).
Unfortunately the PMBus spec is fairly loose in terms of what it
*requires* you to implement, but I don't think that's relevant here.

> 
> > +- 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
> 
> In general, I think a good portion of these should be either implied by 
> a fan compatible string or be part of a common fan binding.

Above you made the distinction between fan and fan controller, but I
feel like following your suggestion here is mixing things up in the
opposite direction to what you desired above. In my mind most of these
properties describe a fan controller's capability rather than a
property of a fan, save say "tach-pulses", which is a property of the
fan's tacho, and maybe "maxim,fan-dual-tach" which applies to any dual-
rotor fan and should probably be generalised.

> 
> > +
> > +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.
> 
> Is this a feature of the Maxim chip or just a mapping of temperature 
> sensors to fans. The latter should be a common property.

It's a manufacturer-specific feature of the Maxim chip, which can do
the full closed-loop fan control using a lookup table mapping
temperatures to fan speeds. The lookup table can be described with the
maxim,fan-lookup-table property specified above, and maxim,tmp-fans
maps the temperature sensor to a set of fans to control using its
temperature readings with respect to each fan's lookup table.

Cheers,

Andrew

> 
> Rob

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 1/3] dt-bindings: hwmon: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 2331 bytes --]

On Mon, 2017-09-18 at 13:15 -0700, Guenter Roeck wrote:
> On Mon, Sep 18, 2017 at 02:26:38PM -0500, Rob Herring wrote:
> > On Fri, Sep 08, 2017 at 02:39:17PM +1000, Andrew Jeffery wrote:
> > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > >  .../devicetree/bindings/hwmon/pmbus/max31785.txt   | 158 +++++++++++++++++++++
> > 
> > I think this needs to be located by what it does (fan control), not what 
> > interface it has (pmbus).
> > 
> > I'm not all that happy with hwmon either because things here seem to 
> > just be based on being Linux hwmon devices which is sometimes arbitrary. 
> > 
> 
> The chip also measures temperatures. Other PMBus chips may do fan control,
> measure temperatures, measure and/or control voltages, current, power ...
> Strictly speaking pretty much all PMBus chips are multi-function devices.
> I personally don't really care if the documentation is spread across
> several directories, but even here this is already challenging.
> 
> Only solution I can think of would be to create separate documents for each
> functionality, ie here one for the device itself, one for fan control,
> and one for temperature control (if that needs separate bindings). That
> would be similar to mfd. But then we would still have to sort out where
> to store the various bindings. Like iio, in subdirectories ? Like mfd,
> in the matching subsystems ? If so, what to do if there is no matching
> subsystem ?
> 
> Lots of questions. I'll be happy to spend some time sorting it out,
> but I would need some directions.
> 

Likewise - I'm keen to discuss and iterate on this so we get something
satisfactory.

At least I could split out the PMBus-specific bindings from the Maxim-
specific bindings in the current document, but there's still the
question of how to arrange it as Guenter has queried above, and also
how much of PMBus to define bindings for. I'm hesitant to take a stab
at describing bindings across the whole spec if I don't have useful
driver implementations to test against. I know that the bindings
describe the hardware and not the driver, but there are probably more
or less clumsy ways to describe devices that could be ironed out with a
corresponding implementation (e.g. the Aspeed PWM/Tacho...).

Thoughts?

Andrew

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]