[PATCH v4 0/6] pmbus: Expand fan support and add MAX31785 driver

[PATCH v4 0/6] pmbus: Expand fan support and add MAX31785 driver

[Andrew Jeffery]

Hello,

This series introduces support for the MAX31785 intelligent fan controller, a
PMBus device providing closed-loop fan control among a number of other
features. Along the way the series adds support to control fans and create
virtual pages to the PMBus core, the latter to support some of the more
annoying design decisions found in the 'A' variant of the chip.

This is the fourth spin of the series, v3 can be found here[1].

I've been running aground with the described devicetree bindings in the
previous iterations, so in order to get *some* support upstream I've gutted the
documentation and removed the corresponding support from the driver. I'll save
posting that for a later date once Guenter and I have some input from Rob about
what direction to take with respect to describing PMBus devices.

As mentioned, adding full support for the features of the MAX31785 requires
modifications to the PMBus core, so I've split the addition of features into
separate patches, in the hope that some can be incrementally applied while we
iterate on the details of any suboptimal parts.

Please review!

Andrew

[1] https://lkml.org/lkml/2017/9/8/4

Andrew Jeffery (6):
  dt-bindings: pmbus: Add Maxim MAX31785 documentation
  pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller
  pmbus: core: Add fan control support
  pmbus: max31785: Add fan control
  pmbus: core: Add virtual page config bit
  pmbus: max31785: Add dual tachometer support

 .../devicetree/bindings/hwmon/max31785.txt         |  22 ++
 Documentation/hwmon/max31785                       |  57 ++++
 drivers/hwmon/pmbus/Kconfig                        |  10 +
 drivers/hwmon/pmbus/Makefile                       |   1 +
 drivers/hwmon/pmbus/max31785.c                     | 375 +++++++++++++++++++++
 drivers/hwmon/pmbus/pmbus.h                        |  31 ++
 drivers/hwmon/pmbus/pmbus_core.c                   | 236 +++++++++++--
 7 files changed, 713 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/max31785.txt
 create mode 100644 Documentation/hwmon/max31785
 create mode 100644 drivers/hwmon/pmbus/max31785.c

-- 
2.11.0

[PATCH v4 1/6] dt-bindings: pmbus: Add Maxim MAX31785 documentation

[Andrew Jeffery]

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

diff --git a/Documentation/devicetree/bindings/hwmon/max31785.txt b/Documentation/devicetree/bindings/hwmon/max31785.txt
new file mode 100644
index 000000000000..106e08c56aaa
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/max31785.txt
@@ -0,0 +1,22 @@
+Bindings for the Maxim MAX31785 Intelligent Fan Controller
+==========================================================
+
+Reference:
+
+https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
+
+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.
+
+Required properties:
+- compatible     : One of "maxim,max31785" or "maxim,max31785a"
+- reg            : I2C address, one of 0x52, 0x53, 0x54, 0x55.
+
+Example:
+
+        fans@52 {
+                compatible = "maxim,max31785";
+                reg = <0x52>;
+        };
-- 
2.11.0

[PATCH v4 2/6] 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.

This patch presents a basic driver using only the existing features of the
PMBus subsystem.

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

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
new file mode 100644
index 000000000000..45fb6093dec2
--- /dev/null
+++ b/Documentation/hwmon/max31785
@@ -0,0 +1,51 @@
+Kernel driver max31785
+======================
+
+Supported chips:
+  * Maxim MAX31785, MAX31785A
+    Prefix: 'max31785' or 'max31785a'
+    Addresses scanned: -
+    Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
+
+Author: Andrew Jeffery <andrew@aj.id.au>
+
+Description
+-----------
+
+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.
+
+For dual rotor fan configuration, the MAX31785 exposes the slowest rotor of the
+two in the fan[1-4]_input attributes.
+
+Usage Notes
+-----------
+
+This driver does not probe for PMBus devices. You will have to instantiate
+devices explicitly.
+
+Sysfs attributes
+----------------
+
+fan[1-4]_alarm		Fan alarm.
+fan[1-4]_fault		Fan fault.
+fan[1-4]_input		Fan RPM.
+
+in[1-6]_crit		Critical maximum output voltage
+in[1-6]_crit_alarm	Output voltage critical high alarm
+in[1-6]_input		Measured output voltage
+in[1-6]_label		"vout[18-23]"
+in[1-6]_lcrit		Critical minimum output voltage
+in[1-6]_lcrit_alarm	Output voltage critical low alarm
+in[1-6]_max		Maximum output voltage
+in[1-6]_max_alarm	Output voltage high alarm
+in[1-6]_min		Minimum output voltage
+in[1-6]_min_alarm	Output voltage low alarm
+
+temp[1-11]_crit		Critical high temperature
+temp[1-11]_crit_alarm	Chip temperature critical high alarm
+temp[1-11]_input	Measured temperature
+temp[1-11]_max		Maximum temperature
+temp[1-11]_max_alarm	Chip temperature high alarm
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index 40019325b517..08479006c7f9 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -114,6 +114,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 459a6be3390e..a8bf0e490db9 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -12,6 +12,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..9313849d5160
--- /dev/null
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -0,0 +1,116 @@
+/*
+ * 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,
+};
+
+#define MAX31785_NR_PAGES		23
+
+#define MAX31785_FAN_FUNCS \
+	(PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12)
+
+#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,
+
+	/* RPM */
+	.format[PSC_FAN] = direct,
+	.m[PSC_FAN] = 1,
+	.b[PSC_FAN] = 0,
+	.R[PSC_FAN] = 0,
+	.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 pmbus_driver_info *info;
+	s64 ret;
+
+	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;
+
+	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

[PATCH v4 3/6] pmbus: core: 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 4efa2bd4f6d8..cdf3e288e626 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, vr13 };
@@ -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 302f0aef59de..55838b69e99a 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -64,6 +64,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 */
 };
@@ -128,6 +129,27 @@ struct pmbus_debugfs_entry {
 	u8 reg;
 };
 
+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);
@@ -198,6 +220,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.
@@ -214,8 +261,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);
 }
 
@@ -231,6 +310,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.
@@ -246,8 +328,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);
 }
 
@@ -314,6 +430,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);
@@ -515,7 +653,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;
 	}
@@ -569,6 +707,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);
@@ -672,7 +813,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;
 	}
@@ -703,6 +844,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);
@@ -925,12 +1069,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);
@@ -1592,13 +1742,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,
@@ -1621,6 +1764,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)
 {
@@ -1658,6 +1843,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 v4 4/6] pmbus: max31785: Add fan control

[Andrew Jeffery]

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

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/hwmon/max31785   |   4 ++
 drivers/hwmon/pmbus/max31785.c | 104 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 107 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
index 45fb6093dec2..e9edbf11948f 100644
--- a/Documentation/hwmon/max31785
+++ b/Documentation/hwmon/max31785
@@ -32,6 +32,7 @@ Sysfs attributes
 fan[1-4]_alarm		Fan alarm.
 fan[1-4]_fault		Fan fault.
 fan[1-4]_input		Fan RPM.
+fan[1-4]_target		Fan input target
 
 in[1-6]_crit		Critical maximum output voltage
 in[1-6]_crit_alarm	Output voltage critical high alarm
@@ -44,6 +45,9 @@ in[1-6]_max_alarm	Output voltage high alarm
 in[1-6]_min		Minimum output voltage
 in[1-6]_min_alarm	Output voltage low alarm
 
+pwm[1-4]		Fan target duty cycle (0..255)
+pwm[1-4]_enable		0: full-speed, 1: manual control, 2: automatic
+
 temp[1-11]_crit		Critical high temperature
 temp[1-11]_crit_alarm	Chip temperature critical high alarm
 temp[1-11]_input	Measured temperature
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index 9313849d5160..0d97ddf67079 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -20,8 +20,102 @@ enum max31785_regs {
 
 #define MAX31785_NR_PAGES		23
 
+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_VIRT_PWM_1:
+		rv = max31785_get_pwm(client, page);
+		if (rv < 0)
+			return rv;
+
+		rv *= 255;
+		rv /= 100;
+		break;
+	case PMBUS_VIRT_PWM_ENABLE_1:
+		rv = max31785_get_pwm_mode(client, page);
+		break;
+	default:
+		rv = -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)
+{
+	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;
+}
+
 #define MAX31785_FAN_FUNCS \
-	(PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12)
+	(PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | PMBUS_HAVE_PWM12)
 
 #define MAX31785_TEMP_FUNCS \
 	(PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP)
@@ -32,11 +126,19 @@ enum max31785_regs {
 static const struct pmbus_driver_info max31785_info = {
 	.pages = MAX31785_NR_PAGES,
 
+	.write_word_data = max31785_write_word_data,
+	.read_word_data = max31785_read_word_data,
+
 	/* 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,
-- 
2.11.0

[PATCH v4 5/6] pmbus: core: Add virtual page config bit

[Andrew Jeffery]

Some circumstances call for virtual pages to expose multiple values
packed into an extended PMBus register in a manner non-compliant with
the PMBus standard. We should not try to set virtual pages on the
device; add a flag so we can avoid doing so.

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

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index cdf3e288e626..0560a8dbcee0 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -367,6 +367,8 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_PWM12	BIT(20)
 #define PMBUS_HAVE_PWM34	BIT(21)
 
+#define PMBUS_PAGE_VIRTUAL	BIT(31)
+
 enum pmbus_data_format { linear = 0, direct, vid };
 enum vrm_version { vr11 = 0, vr12, vr13 };
 
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 55838b69e99a..af7362de405d 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -164,14 +164,18 @@ int pmbus_set_page(struct i2c_client *client, u8 page)
 	int rv = 0;
 	int newpage;
 
-	if (page != data->currpage) {
+	if (page == data->currpage)
+		return 0;
+
+	if (!(data->info->func[page] & PMBUS_PAGE_VIRTUAL)) {
 		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
 		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
 		if (newpage != page)
-			rv = -EIO;
-		else
-			data->currpage = page;
+			return -EIO;
 	}
+
+	data->currpage = page;
+
 	return rv;
 }
 EXPORT_SYMBOL_GPL(pmbus_set_page);
-- 
2.11.0

[PATCH v4 6/6] pmbus: max31785: Add dual tachometer support

[Andrew Jeffery]

The dual tachometer feature is implemented in hardware with a TACHSEL
input to indicate the rotor under measurement, and exposed on the device
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 the
values are exposed through virtual pages. We re-route accesses to
FAN_CONFIG_1_2 and READ_FAN_SPEED_1 on undefined (in hardware) pages
23-28 to the same registers on pages 0-5, and with the latter command we
extract the value from 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>
---
 Documentation/hwmon/max31785   |   8 ++-
 drivers/hwmon/pmbus/max31785.c | 159 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 163 insertions(+), 4 deletions(-)

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
index e9edbf11948f..8e75efc5e4b9 100644
--- a/Documentation/hwmon/max31785
+++ b/Documentation/hwmon/max31785
@@ -17,8 +17,9 @@ 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.
 
-For dual rotor fan configuration, the MAX31785 exposes the slowest rotor of the
-two in the fan[1-4]_input attributes.
+For dual-rotor configurations the MAX31785A exposes the second rotor tachometer
+readings in attributes fan[5-8]_input. By contrast the MAX31785 only exposes
+the slowest rotor measurement, and does so in the fan[1-4]_input attributes.
 
 Usage Notes
 -----------
@@ -31,7 +32,8 @@ Sysfs attributes
 
 fan[1-4]_alarm		Fan alarm.
 fan[1-4]_fault		Fan fault.
-fan[1-4]_input		Fan RPM.
+fan[1-8]_input		Fan RPM. On the MAX31785A, inputs 5-8 correspond to the
+			second rotor of fans 1-4
 fan[1-4]_target		Fan input target
 
 in[1-6]_crit		Critical maximum output voltage
diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
index 0d97ddf67079..2ca7febb2843 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -16,10 +16,82 @@
 
 enum max31785_regs {
 	MFR_REVISION		= 0x9b,
+	MFR_FAN_CONFIG		= 0xf1,
 };
 
+#define MAX31785			0x3030
+#define MAX31785A			0x3040
+
+#define MFR_FAN_CONFIG_DUAL_TACH	BIT(12)
+
 #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;
@@ -76,7 +148,25 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
 	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;
@@ -85,10 +175,13 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
 		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 = -ENODATA;
+		rv = (page >= MAX31785_NR_PAGES) ? -ENXIO : -ENODATA;
 		break;
 	}
 
@@ -100,6 +193,9 @@ 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))
@@ -127,7 +223,9 @@ 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,
@@ -174,6 +272,55 @@ static const struct pmbus_driver_info max31785_info = {
 	.func[22] = MAX31785_VOUT_FUNCS,
 };
 
+static int max31785_configure(struct i2c_client *client,
+			      const struct i2c_device_id *id,
+			      struct pmbus_driver_info *info)
+{
+	struct device *dev = &client->dev;
+	bool dual_tach = false;
+	int ret;
+	int i;
+
+	ret = i2c_smbus_read_word_data(client, MFR_REVISION);
+	if (ret < 0)
+		return ret;
+
+	if (!strcmp("max31785a", id->name)) {
+		if (ret == MAX31785A)
+			dual_tach = true;
+		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;
+	}
+
+	if (!dual_tach)
+		return 0;
+
+	for (i = 0; i <= 5; i++) {
+		ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
+		if (ret < 0)
+			return ret;
+
+		ret = i2c_smbus_read_word_data(client, MFR_FAN_CONFIG);
+		if (ret < 0)
+			return ret;
+
+		if (ret & MFR_FAN_CONFIG_DUAL_TACH) {
+			int virtual = MAX31785_NR_PAGES + i;
+
+			info->pages = max(info->pages, virtual + 1);
+			info->func[virtual] |= PMBUS_HAVE_FAN12;
+			info->func[virtual] |= PMBUS_PAGE_VIRTUAL;
+		}
+	}
+
+	return 0;
+}
+
 static int max31785_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
@@ -181,6 +328,12 @@ static int max31785_probe(struct i2c_client *client,
 	struct pmbus_driver_info *info;
 	s64 ret;
 
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA |
+				     I2C_FUNC_SMBUS_BLOCK_DATA))
+		return -ENODEV;
+
 	info = devm_kzalloc(dev, sizeof(struct pmbus_driver_info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
@@ -191,6 +344,10 @@ static int max31785_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	ret = max31785_configure(client, id, info);
+	if (ret < 0)
+		return ret;
+
 	return pmbus_do_probe(client, id, info);
 }
 
-- 
2.11.0

Re: [v4, 2/6] pmbus: Add driver for Maxim MAX31785 Intelligent Fan Controller

[Guenter Roeck]

On Fri, Nov 03, 2017 at 03:53:02PM +1100, Andrew Jeffery wrote:
> 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.
> 
I updated the above text a little to make it more obvious that "are provided"
refers to the chip, not (yet) to the driver.

> This patch presents a basic driver using only the existing features of the
> PMBus subsystem.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

Applied to hwmon-next.

> ---
>  Documentation/hwmon/max31785   |  51 ++++++++++++++++++
>  drivers/hwmon/pmbus/Kconfig    |  10 ++++
>  drivers/hwmon/pmbus/Makefile   |   1 +
>  drivers/hwmon/pmbus/max31785.c | 116 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 178 insertions(+)
>  create mode 100644 Documentation/hwmon/max31785
>  create mode 100644 drivers/hwmon/pmbus/max31785.c
> 
> diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
> new file mode 100644
> index 000000000000..45fb6093dec2
> --- /dev/null
> +++ b/Documentation/hwmon/max31785
> @@ -0,0 +1,51 @@
> +Kernel driver max31785
> +======================
> +
> +Supported chips:
> +  * Maxim MAX31785, MAX31785A
> +    Prefix: 'max31785' or 'max31785a'
> +    Addresses scanned: -
> +    Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
> +
> +Author: Andrew Jeffery <andrew@aj.id.au>
> +
> +Description
> +-----------
> +
> +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.
> +
> +For dual rotor fan configuration, the MAX31785 exposes the slowest rotor of the
> +two in the fan[1-4]_input attributes.
> +
> +Usage Notes
> +-----------
> +
> +This driver does not probe for PMBus devices. You will have to instantiate
> +devices explicitly.
> +
> +Sysfs attributes
> +----------------
> +
> +fan[1-4]_alarm		Fan alarm.
> +fan[1-4]_fault		Fan fault.
> +fan[1-4]_input		Fan RPM.
> +
> +in[1-6]_crit		Critical maximum output voltage
> +in[1-6]_crit_alarm	Output voltage critical high alarm
> +in[1-6]_input		Measured output voltage
> +in[1-6]_label		"vout[18-23]"
> +in[1-6]_lcrit		Critical minimum output voltage
> +in[1-6]_lcrit_alarm	Output voltage critical low alarm
> +in[1-6]_max		Maximum output voltage
> +in[1-6]_max_alarm	Output voltage high alarm
> +in[1-6]_min		Minimum output voltage
> +in[1-6]_min_alarm	Output voltage low alarm
> +
> +temp[1-11]_crit		Critical high temperature
> +temp[1-11]_crit_alarm	Chip temperature critical high alarm
> +temp[1-11]_input	Measured temperature
> +temp[1-11]_max		Maximum temperature
> +temp[1-11]_max_alarm	Chip temperature high alarm
> diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
> index 40019325b517..08479006c7f9 100644
> --- a/drivers/hwmon/pmbus/Kconfig
> +++ b/drivers/hwmon/pmbus/Kconfig
> @@ -114,6 +114,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 459a6be3390e..a8bf0e490db9 100644
> --- a/drivers/hwmon/pmbus/Makefile
> +++ b/drivers/hwmon/pmbus/Makefile
> @@ -12,6 +12,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..9313849d5160
> --- /dev/null
> +++ b/drivers/hwmon/pmbus/max31785.c
> @@ -0,0 +1,116 @@
> +/*
> + * 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,
> +};
> +
> +#define MAX31785_NR_PAGES		23
> +
> +#define MAX31785_FAN_FUNCS \
> +	(PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12)
> +
> +#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,
> +
> +	/* RPM */
> +	.format[PSC_FAN] = direct,
> +	.m[PSC_FAN] = 1,
> +	.b[PSC_FAN] = 0,
> +	.R[PSC_FAN] = 0,
> +	.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 pmbus_driver_info *info;
> +	s64 ret;
> +
> +	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;
> +
> +	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");

Re: [v4,3/6] pmbus: core: Add fan control support

[Guenter Roeck]

On Fri, Nov 03, 2017 at 03:53:03PM +1100, Andrew Jeffery wrote:
> 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 4efa2bd4f6d8..cdf3e288e626 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, vr13 };
> @@ -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 302f0aef59de..55838b69e99a 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -64,6 +64,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 */
>  };
> @@ -128,6 +129,27 @@ struct pmbus_debugfs_entry {
>  	u8 reg;
>  };
>  
> +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);
> @@ -198,6 +220,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)

Please make sure continuation lines are aligned to '(' where possible.

> +{
> +	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.
> @@ -214,8 +261,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:

Maybe
		case PMBUS_VIRT_FAN_TARGET_1 ... 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;

Please move command up and drop the {.

Why u32 ? The value should be bound to 0..255 (if it isn't and a larger
value is accepted we have overflow issues).

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

Please move this code to a separate function.

> +	}
>  	return pmbus_write_word_data(client, page, reg, word);
>  }
>  
> @@ -231,6 +310,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.
> @@ -246,8 +328,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:

Since there is an implied assumption that those are sequential,
how about the following ?

		case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:

> +			id = reg - PMBUS_VIRT_FAN_TARGET_1;

This warrants a comment in the definition of PMBUS_VIRT_FAN_TARGET_X
and PMBUS_VIRT_PWM_X stating that the definitions must be in sequence.

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

Same as above.

> +		{
> +			int rv;

Please move the declaration up and drop the {.

> +
> +			id = reg - PMBUS_VIRT_PWM_1;
> +			rv = pmbus_get_fan_command(client, page, id, percent);
> +			if (rv < 0)
> +				return rv;
> +
Is this guaranteed to be <= 100 ?

> +			rv *= 255;
> +			rv /= 100;
> +
> +			status = rv;
> +			break;
> +		}
> +		default:
> +			status = -ENXIO;
> +			break;
> +		}

Please move this code to a separate function.

> +
> +		return status;
> +	}
>  	return pmbus_read_word_data(client, page, reg);
>  }
>  
> @@ -314,6 +430,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)

Maybe better pmbus_get_fan_get_speed_rpm to clarify that we are not
really interested in the command register value but but in speed or rpm ?

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

Please drop the unnecessary ().

I am not too happy about this - the user has no means to specify pwm or
fan target speed _before_ changing the mode. It would be better to report
(and accept) cached values in that situation, and update the actual value
as the mode is changed.

> +
> +	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);
> @@ -515,7 +653,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)) {

Please use positive logic

	if (sensor->class != PSC_FAN && sensor->class != PSC_PWM)

It is (at least for me) much easier to read.

>  		R += 3;
>  		b *= 1000;
>  	}
> @@ -569,6 +707,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);
> @@ -672,7 +813,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)) {

Same as above.

>  		R -= 3;		/* Adjust R and b for data in milli-units */
>  		b *= 1000;
>  	}
> @@ -703,6 +844,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);
> @@ -925,12 +1069,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);
> @@ -1592,13 +1742,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,
> @@ -1621,6 +1764,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)))

why not just the following ?

	if (!(data->info->func[page] & (PMBUS_HAVE_PWM12 | PMBUS_HAVE_PWM34)))

Also, doesn't this add attributes for 1,2 even if only 3,4 are
supported, and 3,4 even if only 1,2 are supported ?

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

convert should be a new parameter to pmbus_add_sensor().

> +
> +	return 0;
> +}
> +
>  static int pmbus_add_fan_attributes(struct i2c_client *client,
>  				    struct pmbus_data *data)
>  {
> @@ -1658,6 +1843,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.

Re: [v4,6/6] pmbus: max31785: Add dual tachometer support

[Guenter Roeck]

On Fri, Nov 03, 2017 at 03:53:06PM +1100, Andrew Jeffery wrote:
> The dual tachometer feature is implemented in hardware with a TACHSEL
> input to indicate the rotor under measurement, and exposed on the device
> 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 the
> values are exposed through virtual pages. We re-route accesses to
> FAN_CONFIG_1_2 and READ_FAN_SPEED_1 on undefined (in hardware) pages
> 23-28 to the same registers on pages 0-5, and with the latter command we
> extract the value from 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>
> ---
>  Documentation/hwmon/max31785   |   8 ++-
>  drivers/hwmon/pmbus/max31785.c | 159 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 163 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
> index e9edbf11948f..8e75efc5e4b9 100644
> --- a/Documentation/hwmon/max31785
> +++ b/Documentation/hwmon/max31785
> @@ -17,8 +17,9 @@ 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.
>  
> -For dual rotor fan configuration, the MAX31785 exposes the slowest rotor of the
> -two in the fan[1-4]_input attributes.
> +For dual-rotor configurations the MAX31785A exposes the second rotor tachometer
> +readings in attributes fan[5-8]_input. By contrast the MAX31785 only exposes
> +the slowest rotor measurement, and does so in the fan[1-4]_input attributes.
>  
>  Usage Notes
>  -----------
> @@ -31,7 +32,8 @@ Sysfs attributes
>  
>  fan[1-4]_alarm		Fan alarm.
>  fan[1-4]_fault		Fan fault.
> -fan[1-4]_input		Fan RPM.
> +fan[1-8]_input		Fan RPM. On the MAX31785A, inputs 5-8 correspond to the
> +			second rotor of fans 1-4
>  fan[1-4]_target		Fan input target
>  
>  in[1-6]_crit		Critical maximum output voltage
> diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> index 0d97ddf67079..2ca7febb2843 100644
> --- a/drivers/hwmon/pmbus/max31785.c
> +++ b/drivers/hwmon/pmbus/max31785.c
> @@ -16,10 +16,82 @@
>  
>  enum max31785_regs {
>  	MFR_REVISION		= 0x9b,
> +	MFR_FAN_CONFIG		= 0xf1,
>  };
>  
> +#define MAX31785			0x3030
> +#define MAX31785A			0x3040
> +
> +#define MFR_FAN_CONFIG_DUAL_TACH	BIT(12)
> +
>  #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;
> @@ -76,7 +148,25 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
>  	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;
> @@ -85,10 +175,13 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
>  		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 = -ENODATA;
> +		rv = (page >= MAX31785_NR_PAGES) ? -ENXIO : -ENODATA;
>  		break;
>  	}
>  
> @@ -100,6 +193,9 @@ 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))
> @@ -127,7 +223,9 @@ 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,
> @@ -174,6 +272,55 @@ static const struct pmbus_driver_info max31785_info = {
>  	.func[22] = MAX31785_VOUT_FUNCS,
>  };
>  
> +static int max31785_configure(struct i2c_client *client,
> +			      const struct i2c_device_id *id,
> +			      struct pmbus_driver_info *info)
> +{
> +	struct device *dev = &client->dev;
> +	bool dual_tach = false;
> +	int ret;
> +	int i;
> +
> +	ret = i2c_smbus_read_word_data(client, MFR_REVISION);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!strcmp("max31785a", id->name)) {
> +		if (ret == MAX31785A)
> +			dual_tach = true;
> +		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;
> +	}

This is too restrictive. I would suggest to move the chip detection part
to the probe function. Also, since you have it, you should weed out any other
types (ret could be anything) and not just assume it is MAX31785 if it isn't
MAX31785A.

The "excpected" messages are ok, but don't unnecessaily suppress attributes.
If there is a need to do this for some reason, there should be a configuration
parameter: forcing the user to specify the wrong type on purpose to suppress
information is just wrong.

dual_tach should not depend on the type in id but on the detected type.

> +
> +	if (!dual_tach)
> +		return 0;
> +
> +	for (i = 0; i <= 5; i++) {
> +		ret = i2c_smbus_write_byte_data(client, PMBUS_PAGE, i);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = i2c_smbus_read_word_data(client, MFR_FAN_CONFIG);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (ret & MFR_FAN_CONFIG_DUAL_TACH) {
> +			int virtual = MAX31785_NR_PAGES + i;
> +
> +			info->pages = max(info->pages, virtual + 1);
> +			info->func[virtual] |= PMBUS_HAVE_FAN12;
> +			info->func[virtual] |= PMBUS_PAGE_VIRTUAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int max31785_probe(struct i2c_client *client,
>  			  const struct i2c_device_id *id)
>  {
> @@ -181,6 +328,12 @@ static int max31785_probe(struct i2c_client *client,
>  	struct pmbus_driver_info *info;
>  	s64 ret;
>  
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WORD_DATA |
> +				     I2C_FUNC_SMBUS_BLOCK_DATA))
> +		return -ENODEV;
> +
>  	info = devm_kzalloc(dev, sizeof(struct pmbus_driver_info), GFP_KERNEL);
>  	if (!info)
>  		return -ENOMEM;
> @@ -191,6 +344,10 @@ static int max31785_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = max31785_configure(client, id, info);
> +	if (ret < 0)
> +		return ret;
> +
>  	return pmbus_do_probe(client, id, info);
>  }
>  

Re: [v4,4/6] pmbus: max31785: Add fan control

[Guenter Roeck]

On Fri, Nov 03, 2017 at 03:53:04PM +1100, Andrew Jeffery wrote:
> The 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.
> 
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/hwmon/max31785   |   4 ++
>  drivers/hwmon/pmbus/max31785.c | 104 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 107 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
> index 45fb6093dec2..e9edbf11948f 100644
> --- a/Documentation/hwmon/max31785
> +++ b/Documentation/hwmon/max31785
> @@ -32,6 +32,7 @@ Sysfs attributes
>  fan[1-4]_alarm		Fan alarm.
>  fan[1-4]_fault		Fan fault.
>  fan[1-4]_input		Fan RPM.
> +fan[1-4]_target		Fan input target
>  
>  in[1-6]_crit		Critical maximum output voltage
>  in[1-6]_crit_alarm	Output voltage critical high alarm
> @@ -44,6 +45,9 @@ in[1-6]_max_alarm	Output voltage high alarm
>  in[1-6]_min		Minimum output voltage
>  in[1-6]_min_alarm	Output voltage low alarm
>  
> +pwm[1-4]		Fan target duty cycle (0..255)
> +pwm[1-4]_enable		0: full-speed, 1: manual control, 2: automatic
> +
>  temp[1-11]_crit		Critical high temperature
>  temp[1-11]_crit_alarm	Chip temperature critical high alarm
>  temp[1-11]_input	Measured temperature
> diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> index 9313849d5160..0d97ddf67079 100644
> --- a/drivers/hwmon/pmbus/max31785.c
> +++ b/drivers/hwmon/pmbus/max31785.c
> @@ -20,8 +20,102 @@ enum max31785_regs {
>  
>  #define MAX31785_NR_PAGES		23
>  
> +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_VIRT_PWM_1:
> +		rv = max31785_get_pwm(client, page);
> +		if (rv < 0)
> +			return rv;
> +
> +		rv *= 255;
> +		rv /= 100;
> +		break;
> +	case PMBUS_VIRT_PWM_ENABLE_1:
> +		rv = max31785_get_pwm_mode(client, page);
> +		break;

I do wonder ... does it even make sense to specify generic code
for the new virtual attributes in the pmbus core code, or would
it be better to have it all in this driver, at least for now ?

> +	default:
> +		rv = -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)
> +{
> +	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;
> +}
> +
>  #define MAX31785_FAN_FUNCS \
> -	(PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12)
> +	(PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12 | PMBUS_HAVE_PWM12)
>  
>  #define MAX31785_TEMP_FUNCS \
>  	(PMBUS_HAVE_TEMP | PMBUS_HAVE_STATUS_TEMP)
> @@ -32,11 +126,19 @@ enum max31785_regs {
>  static const struct pmbus_driver_info max31785_info = {
>  	.pages = MAX31785_NR_PAGES,
>  
> +	.write_word_data = max31785_write_word_data,
> +	.read_word_data = max31785_read_word_data,
> +
>  	/* 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,

Re: [PATCH v4 1/6] dt-bindings: pmbus: Add Maxim MAX31785 documentation

[Rob Herring]

On Fri, Nov 03, 2017 at 03:53:01PM +1100, Andrew Jeffery wrote:
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  .../devicetree/bindings/hwmon/max31785.txt         | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/max31785.txt

Acked-by: Rob Herring <robh@kernel.org>

Re: [v4,1/6] dt-bindings: pmbus: Add Maxim MAX31785 documentation

[Guenter Roeck]

On Fri, Nov 03, 2017 at 03:53:01PM +1100, Andrew Jeffery wrote:
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Acked-by: Rob Herring <robh@kernel.org>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  .../devicetree/bindings/hwmon/max31785.txt         | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/max31785.txt
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/max31785.txt b/Documentation/devicetree/bindings/hwmon/max31785.txt
> new file mode 100644
> index 000000000000..106e08c56aaa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/max31785.txt
> @@ -0,0 +1,22 @@
> +Bindings for the Maxim MAX31785 Intelligent Fan Controller
> +==========================================================
> +
> +Reference:
> +
> +https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf
> +
> +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.
> +
> +Required properties:
> +- compatible     : One of "maxim,max31785" or "maxim,max31785a"
> +- reg            : I2C address, one of 0x52, 0x53, 0x54, 0x55.
> +
> +Example:
> +
> +        fans@52 {
> +                compatible = "maxim,max31785";
> +                reg = <0x52>;
> +        };

Re: [v4,3/6] pmbus: core: Add fan control support

[Andrew Jeffery]

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

On Sun, 2017-11-05 at 06:39 -0800, Guenter Roeck wrote:
> On Fri, Nov 03, 2017 at 03:53:03PM +1100, Andrew Jeffery wrote:
> > 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 4efa2bd4f6d8..cdf3e288e626 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, vr13 };
> > @@ -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 302f0aef59de..55838b69e99a 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -64,6 +64,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 */
> >  };
> > @@ -128,6 +129,27 @@ struct pmbus_debugfs_entry {
> >  	u8 reg;
> >  };
> >  
> > +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);
> > @@ -198,6 +220,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)
> 
> Please make sure continuation lines are aligned to '(' where possible.

Yep, sorry about that.

> 
> > +{
> > +	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.
> > @@ -214,8 +261,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:
> 
> Maybe
> 		case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:

Sure. I'll fix all instances of this.

> 
> > +			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;
> 
> Please move command up and drop the {.

My default is to scope variables to where they are needed, but will
fix.

> 
> Why u32 ? The value should be bound to 0..255 (if it isn't and a larger
> value is accepted we have overflow issues).

It gets scaled below, though we still only need u16 if the max value is
255. I'll fix it.

> 
> > +
> > +			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;
> 
> Please move this code to a separate function.

Will do.

> 
> > +	}
> >  	return pmbus_write_word_data(client, page, reg, word);
> >  }
> >  
> > @@ -231,6 +310,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.
> > @@ -246,8 +328,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:
> 
> Since there is an implied assumption that those are sequential,
> how about the following ?
> 
> 		case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:

Will fix all instances of this, as noted above.

> 
> > +			id = reg - PMBUS_VIRT_FAN_TARGET_1;
> 
> This warrants a comment in the definition of PMBUS_VIRT_FAN_TARGET_X
> and PMBUS_VIRT_PWM_X stating that the definitions must be in sequence.

Right, will add a comment to that effect.

> 
> > +			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:
> 
> Same as above.

Ack.

> 
> > +		{
> > +			int rv;
> 
> Please move the declaration up and drop the {.

Ack.

> 
> > +
> > +			id = reg - PMBUS_VIRT_PWM_1;
> > +			rv = pmbus_get_fan_command(client, page, id, percent);
> > +			if (rv < 0)
> > +				return rv;
> > +
> 
> Is this guaranteed to be <= 100 ?

PMBus spec rev 1.2 part II says in 14.10 and 14.11:

The second part of the configuration tells the device whether the fan
speed commands are in RPM or PWM duty cycle (in percent).

However, the MAX31785 accepts the ranges 0-0x2710, so allows fractions
of a percent.

Not sure what my thoughts were here, it's probably best just to drop
this default PWM implementation as well.

> 
> > +			rv *= 255;
> > +			rv /= 100;
> > +
> > +			status = rv;
> > +			break;
> > +		}
> > +		default:
> > +			status = -ENXIO;
> > +			break;
> > +		}
> 
> Please move this code to a separate function.

Will do, though given I plan to gut the PWM implementation is it still
so disruptive?

> 
> > +
> > +		return status;
> > +	}
> >  	return pmbus_read_word_data(client, page, reg);
> >  }
> >  
> > @@ -314,6 +430,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)
> 
> Maybe better pmbus_get_fan_get_speed_rpm to clarify that we are not
> really interested in the command register value but but in speed or rpm ?

Sounds reasonable on the surface. I'll look into it.

> 
> > +{
> > +	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;
> 
> Please drop the unnecessary ().

Sure.

> 
> I am not too happy about this - the user has no means to specify pwm or
> fan target speed _before_ changing the mode. It would be better to report
> (and accept) cached values in that situation, and update the actual value
> as the mode is changed.

I'm with you on the caching idea for switching modes, but do we want to
report values that don't obviously reflect the fan rate to userspace?
In previous revisions I was returning an error here but you pointed out
this would break sensors(1) and suggested I switch to 0 in the conflict
case.

> 
> > +
> > +	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);
> > @@ -515,7 +653,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)) {
> 
> Please use positive logic
> 
> 	if (sensor->class != PSC_FAN && sensor->class != PSC_PWM)
> 
> It is (at least for me) much easier to read.

Okay.

> 
> >  		R += 3;
> >  		b *= 1000;
> >  	}
> > @@ -569,6 +707,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);
> > @@ -672,7 +813,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)) {
> 
> Same as above.
> 

Ack.

> >  		R -= 3;		/* Adjust R and b for data in milli-units */
> >  		b *= 1000;
> >  	}
> > @@ -703,6 +844,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);
> > @@ -925,12 +1069,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);
> > @@ -1592,13 +1742,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,
> > @@ -1621,6 +1764,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)))
> 
> why not just the following ?
> 
> 	if (!(data->info->func[page] & (PMBUS_HAVE_PWM12 | PMBUS_HAVE_PWM34)))

Sure, that's much neater.

> 
> Also, doesn't this add attributes for 1,2 even if only 3,4 are
> supported, and 3,4 even if only 1,2 are supported ?

It shouldn't, but that's due to the way I've called
pmbus_add_fan_ctrl() in pmbus_add_fan_attributes(). 

The call is guarded with: 

    /* 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;
    }

So 'f', or the formal parameter 'id', is only ever provided as
appropriate, and together the two tests should ensure that only the
appropriate attributes are created.

If the fan is marked as "not installed" then the addition of the
attribute is skipped. This code was already in place to handle the
fanX_input attribute.

So to the caller test, regardless of RPM or PWM mode, we need the
FAN_COMMAND register to configure the values. As it stands the change
makes fan control available for any PMBus device which has the
FAN_COMMANDxy register and whose driver configures a page with
PMBUS_HAVE_FANxy, exposing the fanX_target attribute. The test in
question above deals only with PMBUS_HAVE_PWMxy which was introduced in
this change.

There are some constraints: PMBUS_HAVE_FANxy is required if
PMBUS_HAVE_PWMxy is to be specified for a given page. This should
probably be documented in a comment alongside the #defines.

> 
> > +		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;
> 
> convert should be a new parameter to pmbus_add_sensor().

Will fix.

Cheers for the detailed feedback.

Andrew

> 
> > +
> > +	return 0;
> > +}
> > +
> >  static int pmbus_add_fan_attributes(struct i2c_client *client,
> >  				    struct pmbus_data *data)
> >  {
> > @@ -1658,6 +1843,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.

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

Re: [v4,4/6] pmbus: max31785: Add fan control

[Andrew Jeffery]

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

On Sun, 2017-11-05 at 07:04 -0800, Guenter Roeck wrote:
> On Fri, Nov 03, 2017 at 03:53:04PM +1100, Andrew Jeffery wrote:
> > The 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.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  Documentation/hwmon/max31785   |   4 ++
> >  drivers/hwmon/pmbus/max31785.c | 104 ++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 107 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
> > index 45fb6093dec2..e9edbf11948f 100644
> > --- a/Documentation/hwmon/max31785
> > +++ b/Documentation/hwmon/max31785
> > @@ -32,6 +32,7 @@ Sysfs attributes
> >  fan[1-4]_alarm		Fan alarm.
> >  fan[1-4]_fault		Fan fault.
> >  fan[1-4]_input		Fan RPM.
> > +fan[1-4]_target		Fan input target
> >  
> >  in[1-6]_crit		Critical maximum output voltage
> >  in[1-6]_crit_alarm	Output voltage critical high alarm
> > @@ -44,6 +45,9 @@ in[1-6]_max_alarm	Output voltage high alarm
> >  in[1-6]_min		Minimum output voltage
> >  in[1-6]_min_alarm	Output voltage low alarm
> >  
> > +pwm[1-4]		Fan target duty cycle (0..255)
> > +pwm[1-4]_enable		0: full-speed, 1: manual control, 2: automatic
> > +
> >  temp[1-11]_crit		Critical high temperature
> >  temp[1-11]_crit_alarm	Chip temperature critical high alarm
> >  temp[1-11]_input	Measured temperature
> > diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> > index 9313849d5160..0d97ddf67079 100644
> > --- a/drivers/hwmon/pmbus/max31785.c
> > +++ b/drivers/hwmon/pmbus/max31785.c
> > @@ -20,8 +20,102 @@ enum max31785_regs {
> >  
> >  #define MAX31785_NR_PAGES		23
> >  
> > +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_VIRT_PWM_1:
> > +		rv = max31785_get_pwm(client, page);
> > +		if (rv < 0)
> > +			return rv;
> > +
> > +		rv *= 255;
> > +		rv /= 100;
> > +		break;
> > +	case PMBUS_VIRT_PWM_ENABLE_1:
> > +		rv = max31785_get_pwm_mode(client, page);
> > +		break;
> 
> I do wonder ... does it even make sense to specify generic code
> for the new virtual attributes in the pmbus core code, or would
> it be better to have it all in this driver, at least for now ?

I think I'll pull the generic implementations out in light of my
response on 3/6. At best the generic implementation for the PWM virtual
regs is a guess. We can always put it back if others come to need it.

Cheers,

Andrew

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

Re: [v4,6/6] pmbus: max31785: Add dual tachometer support

[Andrew Jeffery]

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

On Sun, 2017-11-05 at 06:58 -0800, Guenter Roeck wrote:
> On Fri, Nov 03, 2017 at 03:53:06PM +1100, Andrew Jeffery wrote:
> > The dual tachometer feature is implemented in hardware with a TACHSEL
> > input to indicate the rotor under measurement, and exposed on the device
> > 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 the
> > values are exposed through virtual pages. We re-route accesses to
> > FAN_CONFIG_1_2 and READ_FAN_SPEED_1 on undefined (in hardware) pages
> > 23-28 to the same registers on pages 0-5, and with the latter command we
> > extract the value from 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>
> > ---
> >  Documentation/hwmon/max31785   |   8 ++-
> >  drivers/hwmon/pmbus/max31785.c | 159 ++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 163 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
> > index e9edbf11948f..8e75efc5e4b9 100644
> > --- a/Documentation/hwmon/max31785
> > +++ b/Documentation/hwmon/max31785
> > @@ -17,8 +17,9 @@ 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.
> >  
> > -For dual rotor fan configuration, the MAX31785 exposes the slowest rotor of the
> > -two in the fan[1-4]_input attributes.
> > +For dual-rotor configurations the MAX31785A exposes the second rotor tachometer
> > +readings in attributes fan[5-8]_input. By contrast the MAX31785 only exposes
> > +the slowest rotor measurement, and does so in the fan[1-4]_input attributes.
> >  
> >  Usage Notes
> >  -----------
> > @@ -31,7 +32,8 @@ Sysfs attributes
> >  
> >  fan[1-4]_alarm		Fan alarm.
> >  fan[1-4]_fault		Fan fault.
> > -fan[1-4]_input		Fan RPM.
> > +fan[1-8]_input		Fan RPM. On the MAX31785A, inputs 5-8 correspond to the
> > +			second rotor of fans 1-4
> >  fan[1-4]_target		Fan input target
> >  
> >  in[1-6]_crit		Critical maximum output voltage
> > diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
> > index 0d97ddf67079..2ca7febb2843 100644
> > --- a/drivers/hwmon/pmbus/max31785.c
> > +++ b/drivers/hwmon/pmbus/max31785.c
> > @@ -16,10 +16,82 @@
> >  
> >  enum max31785_regs {
> >  	MFR_REVISION		= 0x9b,
> > +	MFR_FAN_CONFIG		= 0xf1,
> >  };
> >  
> > +#define MAX31785			0x3030
> > +#define MAX31785A			0x3040
> > +
> > +#define MFR_FAN_CONFIG_DUAL_TACH	BIT(12)
> > +
> >  #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;
> > @@ -76,7 +148,25 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
> >  	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;
> > @@ -85,10 +175,13 @@ static int max31785_read_word_data(struct i2c_client *client, int page,
> >  		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 = -ENODATA;
> > +		rv = (page >= MAX31785_NR_PAGES) ? -ENXIO : -ENODATA;
> >  		break;
> >  	}
> >  
> > @@ -100,6 +193,9 @@ 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))
> > @@ -127,7 +223,9 @@ 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,
> > @@ -174,6 +272,55 @@ static const struct pmbus_driver_info max31785_info = {
> >  	.func[22] = MAX31785_VOUT_FUNCS,
> >  };
> >  
> > +static int max31785_configure(struct i2c_client *client,
> > +			      const struct i2c_device_id *id,
> > +			      struct pmbus_driver_info *info)
> > +{
> > +	struct device *dev = &client->dev;
> > +	bool dual_tach = false;
> > +	int ret;
> > +	int i;
> > +
> > +	ret = i2c_smbus_read_word_data(client, MFR_REVISION);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (!strcmp("max31785a", id->name)) {
> > +		if (ret == MAX31785A)
> > +			dual_tach = true;
> > +		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;
> > +	}
> 
> This is too restrictive. I would suggest to move the chip detection part
> to the probe function. Also, since you have it, you should weed out any other
> types (ret could be anything) and not just assume it is MAX31785 if it isn't
> MAX31785A.
> 
> The "excpected" messages are ok, but don't unnecessaily suppress attributes.
> If there is a need to do this for some reason, there should be a configuration
> parameter: forcing the user to specify the wrong type on purpose to suppress
> information is just wrong.
> 
> dual_tach should not depend on the type in id but on the detected type.

No worries, will fix all of the above.

Thanks for the feedback.

Andrew

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

Re: [v4,4/6] pmbus: max31785: Add fan control

[Guenter Roeck]

On 11/09/2017 07:10 PM, Andrew Jeffery wrote:
> On Sun, 2017-11-05 at 07:04 -0800, Guenter Roeck wrote:
>> On Fri, Nov 03, 2017 at 03:53:04PM +1100, Andrew Jeffery wrote:
>>> The 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.
>>>
>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>> ---
>>>   Documentation/hwmon/max31785   |   4 ++
>>>   drivers/hwmon/pmbus/max31785.c | 104 ++++++++++++++++++++++++++++++++++++++++-
>>>   2 files changed, 107 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
>>> index 45fb6093dec2..e9edbf11948f 100644
>>> --- a/Documentation/hwmon/max31785
>>> +++ b/Documentation/hwmon/max31785
>>> @@ -32,6 +32,7 @@ Sysfs attributes
>>>   fan[1-4]_alarm		Fan alarm.
>>>   fan[1-4]_fault		Fan fault.
>>>   fan[1-4]_input		Fan RPM.
>>> +fan[1-4]_target		Fan input target
>>>   
>>>   in[1-6]_crit		Critical maximum output voltage
>>>   in[1-6]_crit_alarm	Output voltage critical high alarm
>>> @@ -44,6 +45,9 @@ in[1-6]_max_alarm	Output voltage high alarm
>>>   in[1-6]_min		Minimum output voltage
>>>   in[1-6]_min_alarm	Output voltage low alarm
>>>   
>>> +pwm[1-4]		Fan target duty cycle (0..255)
>>> +pwm[1-4]_enable		0: full-speed, 1: manual control, 2: automatic
>>> +
>>>   temp[1-11]_crit		Critical high temperature
>>>   temp[1-11]_crit_alarm	Chip temperature critical high alarm
>>>   temp[1-11]_input	Measured temperature
>>> diff --git a/drivers/hwmon/pmbus/max31785.c b/drivers/hwmon/pmbus/max31785.c
>>> index 9313849d5160..0d97ddf67079 100644
>>> --- a/drivers/hwmon/pmbus/max31785.c
>>> +++ b/drivers/hwmon/pmbus/max31785.c
>>> @@ -20,8 +20,102 @@ enum max31785_regs {
>>>   
>>>   #define MAX31785_NR_PAGES		23
>>>   
>>> +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_VIRT_PWM_1:
>>> +		rv = max31785_get_pwm(client, page);
>>> +		if (rv < 0)
>>> +			return rv;
>>> +
>>> +		rv *= 255;
>>> +		rv /= 100;
>>> +		break;
>>> +	case PMBUS_VIRT_PWM_ENABLE_1:
>>> +		rv = max31785_get_pwm_mode(client, page);
>>> +		break;
>>
>> I do wonder ... does it even make sense to specify generic code
>> for the new virtual attributes in the pmbus core code, or would
>> it be better to have it all in this driver, at least for now ?
> 
> I think I'll pull the generic implementations out in light of my
> response on 3/6. At best the generic implementation for the PWM virtual
> regs is a guess. We can always put it back if others come to need it.
> 

SGTM.

Thanks,
Guenter

Re: [v4,3/6] pmbus: core: Add fan control support

[Guenter Roeck]

On 11/09/2017 07:03 PM, Andrew Jeffery wrote:
> On Sun, 2017-11-05 at 06:39 -0800, Guenter Roeck wrote:
>> On Fri, Nov 03, 2017 at 03:53:03PM +1100, Andrew Jeffery wrote:
>>> 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 4efa2bd4f6d8..cdf3e288e626 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, vr13 };
>>> @@ -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 302f0aef59de..55838b69e99a 100644
>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>>> @@ -64,6 +64,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 */
>>>   };
>>> @@ -128,6 +129,27 @@ struct pmbus_debugfs_entry {
>>>   	u8 reg;
>>>   };
>>>   
>>> +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);
>>> @@ -198,6 +220,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)
>>
>> Please make sure continuation lines are aligned to '(' where possible.
> 
> Yep, sorry about that.
> 
>>
>>> +{
>>> +	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.
>>> @@ -214,8 +261,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:
>>
>> Maybe
>> 		case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:
> 
> Sure. I'll fix all instances of this.
> 
>>
>>> +			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;
>>
>> Please move command up and drop the {.
> 
> My default is to scope variables to where they are needed, but will
> fix.
> 
I am fine with that and do it as well as long as it is in loops or if statements.
In case statements it becomes awkward. Of course, that is all POV.

>>
>> Why u32 ? The value should be bound to 0..255 (if it isn't and a larger
>> value is accepted we have overflow issues).
> 
> It gets scaled below, though we still only need u16 if the max value is
> 255. I'll fix it.
> 
Good point. u32 is better than u16 - it results in less code on many architectures.

>>
>>> +
>>> +			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;
>>
>> Please move this code to a separate function.
> 
> Will do.
> 
>>
>>> +	}
>>>   	return pmbus_write_word_data(client, page, reg, word);
>>>   }
>>>   
>>> @@ -231,6 +310,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.
>>> @@ -246,8 +328,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:
>>
>> Since there is an implied assumption that those are sequential,
>> how about the following ?
>>
>> 		case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:
> 
> Will fix all instances of this, as noted above.
> 
>>
>>> +			id = reg - PMBUS_VIRT_FAN_TARGET_1;
>>
>> This warrants a comment in the definition of PMBUS_VIRT_FAN_TARGET_X
>> and PMBUS_VIRT_PWM_X stating that the definitions must be in sequence.
> 
> Right, will add a comment to that effect.
> 
>>
>>> +			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:
>>
>> Same as above.
> 
> Ack.
> 
>>
>>> +		{
>>> +			int rv;
>>
>> Please move the declaration up and drop the {.
> 
> Ack.
> 
>>
>>> +
>>> +			id = reg - PMBUS_VIRT_PWM_1;
>>> +			rv = pmbus_get_fan_command(client, page, id, percent);
>>> +			if (rv < 0)
>>> +				return rv;
>>> +
>>
>> Is this guaranteed to be <= 100 ?
> 
> PMBus spec rev 1.2 part II says in 14.10 and 14.11:
> 
> The second part of the configuration tells the device whether the fan
> speed commands are in RPM or PWM duty cycle (in percent).
> 

Hmm, yes, sure, but can you trust the chip to follow the specification ?

> However, the MAX31785 accepts the ranges 0-0x2710, so allows fractions
> of a percent.
> 
Answering my own question ... apparently not.

> Not sure what my thoughts were here, it's probably best just to drop
> this default PWM implementation as well.
> 

I agree.

>>
>>> +			rv *= 255;
>>> +			rv /= 100;
>>> +
>>> +			status = rv;
>>> +			break;
>>> +		}
>>> +		default:
>>> +			status = -ENXIO;
>>> +			break;
>>> +		}
>>
>> Please move this code to a separate function.
> 
> Will do, though given I plan to gut the PWM implementation is it still
> so disruptive?
> 
Let's see how it looks like. If it does more than just call another function
it should be a separate function.

>>
>>> +
>>> +		return status;
>>> +	}
>>>   	return pmbus_read_word_data(client, page, reg);
>>>   }
>>>   
>>> @@ -314,6 +430,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)
>>
>> Maybe better pmbus_get_fan_get_speed_rpm to clarify that we are not
>> really interested in the command register value but but in speed or rpm ?
> 
> Sounds reasonable on the surface. I'll look into it.
> 
>>
>>> +{
>>> +	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;
>>
>> Please drop the unnecessary ().
> 
> Sure.
> 
>>
>> I am not too happy about this - the user has no means to specify pwm or
>> fan target speed _before_ changing the mode. It would be better to report
>> (and accept) cached values in that situation, and update the actual value
>> as the mode is changed.
> 
> I'm with you on the caching idea for switching modes, but do we want to
> report values that don't obviously reflect the fan rate to userspace?

0 doesn't reflect the rate either. Bad choices either way.

> In previous revisions I was returning an error here but you pointed out
> this would break sensors(1) and suggested I switch to 0 in the conflict
> case.
> 
>>
>>> +
>>> +	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);
>>> @@ -515,7 +653,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)) {
>>
>> Please use positive logic
>>
>> 	if (sensor->class != PSC_FAN && sensor->class != PSC_PWM)
>>
>> It is (at least for me) much easier to read.
> 
> Okay.
> 
>>
>>>   		R += 3;
>>>   		b *= 1000;
>>>   	}
>>> @@ -569,6 +707,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);
>>> @@ -672,7 +813,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)) {
>>
>> Same as above.
>>
> 
> Ack.
> 
>>>   		R -= 3;		/* Adjust R and b for data in milli-units */
>>>   		b *= 1000;
>>>   	}
>>> @@ -703,6 +844,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);
>>> @@ -925,12 +1069,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);
>>> @@ -1592,13 +1742,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,
>>> @@ -1621,6 +1764,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)))
>>
>> why not just the following ?
>>
>> 	if (!(data->info->func[page] & (PMBUS_HAVE_PWM12 | PMBUS_HAVE_PWM34)))
> 
> Sure, that's much neater.
> 
>>
>> Also, doesn't this add attributes for 1,2 even if only 3,4 are
>> supported, and 3,4 even if only 1,2 are supported ?
> 
> It shouldn't, but that's due to the way I've called
> pmbus_add_fan_ctrl() in pmbus_add_fan_attributes().
> 
> The call is guarded with:
> 
>      /* 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;
>      }
> 
> So 'f', or the formal parameter 'id', is only ever provided as
> appropriate, and together the two tests should ensure that only the
> appropriate attributes are created.
> 
> If the fan is marked as "not installed" then the addition of the
> attribute is skipped. This code was already in place to handle the
> fanX_input attribute.
> 
> So to the caller test, regardless of RPM or PWM mode, we need the
> FAN_COMMAND register to configure the values. As it stands the change
> makes fan control available for any PMBus device which has the
> FAN_COMMANDxy register and whose driver configures a page with
> PMBUS_HAVE_FANxy, exposing the fanX_target attribute. The test in
> question above deals only with PMBUS_HAVE_PWMxy which was introduced in
> this change.
> 
> There are some constraints: PMBUS_HAVE_FANxy is required if
> PMBUS_HAVE_PWMxy is to be specified for a given page. This should
> probably be documented in a comment alongside the #defines.
> 
>>
>>> +		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;
>>
>> convert should be a new parameter to pmbus_add_sensor().
> 
> Will fix.
> 
> Cheers for the detailed feedback.
> 
> Andrew
> 
>>
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   static int pmbus_add_fan_attributes(struct i2c_client *client,
>>>   				    struct pmbus_data *data)
>>>   {
>>> @@ -1658,6 +1843,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.