linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/4] pmbus: Expand fan support and add MAX31785 driver
@ 2017-11-18  3:26 Andrew Jeffery
  2017-11-18  3:26 ` [PATCH v5 1/4] pmbus (core): Add fan control support Andrew Jeffery
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Andrew Jeffery @ 2017-11-18  3:26 UTC (permalink / raw)
  To: linux-hwmon
  Cc: Andrew Jeffery, linux, robh+dt, mark.rutland, jdelvare, corbet,
	devicetree, linux-kernel, linux-doc, joel, openbmc

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 fifth spin of the series. v4 can be found here:

    https://lkml.org/lkml/2017/11/3/15

The changes over v4 include a rework of the fan control support to provide a
more intuitive behaviour for fanX_target, pwmX. They now always read the value
last set (in v4 they returned 0 if they were not the active control mode, and
in even earlier spins they returned an error), which also allows implementation
of sane behaviour for pwmX_enable when switching control modes.

The default implementation for the PWM virtual registers is removed from pmbus
core in light of having no consumers (the max31785 driver implements them
itself), though whilst I was unsure about the generality of the scaling in
replies on v4, after some more thought I have reason to believe it should hold
in general. Regardless, it's gone for the moment, and I've added some
commentary about the scaling in the max31785 implementation.

Please review!

Andrew

Andrew Jeffery (4):
  pmbus (core): Add fan control support
  pmbus (max31785): Add fan control
  pmbus (core): Add virtual page config bit
  pmbus (max31785): Add dual tachometer support

 Documentation/hwmon/max31785     |  15 +-
 drivers/hwmon/pmbus/max31785.c   | 285 +++++++++++++++++++++++++++++++-
 drivers/hwmon/pmbus/pmbus.h      |  41 ++++-
 drivers/hwmon/pmbus/pmbus_core.c | 250 +++++++++++++++++++++++++---
 4 files changed, 566 insertions(+), 25 deletions(-)

base-commit: ded0eb83449e8fcba22fd2736826336e101ffbcb
-- 
git-series 0.9.1

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v5 1/4] pmbus (core): Add fan control support
  2017-11-18  3:26 [PATCH v5 0/4] pmbus: Expand fan support and add MAX31785 driver Andrew Jeffery
@ 2017-11-18  3:26 ` Andrew Jeffery
  2017-11-18  3:26 ` [PATCH v5 2/4] pmbus (max31785): Add fan control Andrew Jeffery
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Andrew Jeffery @ 2017-11-18  3:26 UTC (permalink / raw)
  To: linux-hwmon
  Cc: Andrew Jeffery, linux, robh+dt, mark.rutland, jdelvare, corbet,
	devicetree, linux-kernel, linux-doc, joel, openbmc

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 configuration in 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 to
facilitate the necessary side-effects of each access:

1. PMBUS_VIRT_FAN_TARGET_x
2. PMBUS_VIRT_PWM_x
3. PMBUS_VIRT_PWM_ENABLE_x

Some complexity arises with the fanX_target and pwmX attributes both mapping
onto FAN_COMMAND_x: There is no general mapping between PWM percent duty and
RPM, so we can't display values in either attribute in terms of the other
(which in my mind is the intuitive, if impossible, behaviour). This problem
also affects the pwmX_enable attribute which allows userspace to switch between
full speed, manual PWM and a number of automatic control modes, possibly
including a switch to RPM behaviour (e.g. automatically adjusting PWM duty to
reach a RPM target, the behaviour of fanX_target).

The next most intuitive behaviour is for fanX_target and pwmX to simply be
independent, to retain their most recently set value even if that value is not
active on the hardware (due to switching to the alternative control mode). This
property of retaining the value independent of the hardware state has useful
results for both userspace and the kernel: Userspace always sees a sensible
value in the attribute (the last thing it was set to, as opposed to 0 or
receiving an error on read), and the kernel can use the attributes as a value
cache. This latter point eases the implementation of pwmX_enable, which can
look up the associated pmbus_sensor object, take its cached value and apply it
to hardware on changing control mode. This ensures we will not arbitrarily set
a PWM value as an RPM value or vice versa, and we can assume that the RPM or
PWM value set was sensible at least at some point in the past.

Finally, the DIRECT mode coefficients of some controllers is different between
RPM and PWM percent duty control modes, so PSC_PWM is introduced to capture the
necessary coefficients. As pmbus core had no PWM support previously PSC_FAN
continues to be used to capture the RPM DIRECT coefficients, but in order to
avoid falsely applying RPM scaling to PWM values I have introduced the
PMBUS_HAVE_PWM12 and PMB_BUS_HAVE_PWM34 feature bits. These feature bits allow
drivers to explicitly declare PWM support in order to have the attributes
exposed.

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

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index fa613bd209e3..b54d7604d3ef 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -190,6 +190,33 @@ 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_[1-4] and PMBUS_VIRT_PWM_ENABLE_[1-4] in the
+	 * {read,write}_word_data callback.
+	 *
+	 * pmbus core provides a default implementation for
+	 * PMBUS_VIRT_FAN_TARGET_[1-4].
+	 *
+	 * TARGET, PWM and PWM_ENABLE members must be defined sequentially;
+	 * pmbus core uses the difference between the provided register and
+	 * it's _1 counterpart to calculate the FAN/PWM ID.
+	 */
+	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 +250,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 +342,7 @@ enum pmbus_sensor_classes {
 	PSC_POWER,
 	PSC_TEMPERATURE,
 	PSC_FAN,
+	PSC_PWM,
 	PSC_NUM_CLASSES		/* Number of power sensor classes */
 };
 
@@ -339,6 +369,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 };
@@ -421,5 +453,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 int pmbus_do_remove(struct i2c_client *client);
 const struct pmbus_driver_info *pmbus_get_driver_info(struct i2c_client
 						      *client);
-
+int pmbus_get_fan_rate_device(struct i2c_client *client, int page, int id,
+			      enum pmbus_fan_mode mode);
+int pmbus_get_fan_rate_cached(struct i2c_client *client, int page, int id,
+			      enum pmbus_fan_mode mode);
+int pmbus_update_fan(struct i2c_client *client, int page, int id,
+		     u8 config, u8 mask, u16 command);
 #endif /* PMBUS_H */
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 52a58b8b6e1b..631db88526b6 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);
@@ -197,6 +219,28 @@ int pmbus_write_word_data(struct i2c_client *client, int page, u8 reg,
 }
 EXPORT_SYMBOL_GPL(pmbus_write_word_data);
 
+
+static int pmbus_write_virt_reg(struct i2c_client *client, int page, int reg,
+				u16 word)
+{
+	int bit;
+	int id;
+	int rv;
+
+	switch (reg) {
+	case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:
+		id = reg - PMBUS_VIRT_FAN_TARGET_1;
+		bit = pmbus_fan_rpm_mask[id];
+		rv = pmbus_update_fan(client, page, id, bit, bit, word);
+		break;
+	default:
+		rv = -ENXIO;
+		break;
+	}
+
+	return rv;
+}
+
 /*
  * _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.
@@ -213,11 +257,38 @@ 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;
+		return pmbus_write_virt_reg(client, page, reg, word);
+
 	return pmbus_write_word_data(client, page, reg, word);
 }
 
+int pmbus_update_fan(struct i2c_client *client, int page, int id,
+	             u8 config, u8 mask, u16 command)
+{
+	int from;
+	int 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);
+
 int pmbus_read_word_data(struct i2c_client *client, int page, u8 reg)
 {
 	int rv;
@@ -230,6 +301,24 @@ int pmbus_read_word_data(struct i2c_client *client, int page, u8 reg)
 }
 EXPORT_SYMBOL_GPL(pmbus_read_word_data);
 
+static int pmbus_read_virt_reg(struct i2c_client *client, int page, int reg)
+{
+	int rv;
+	int id;
+
+	switch (reg) {
+	case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:
+		id = reg - PMBUS_VIRT_FAN_TARGET_1;
+		rv = pmbus_get_fan_rate_device(client, page, id, rpm);
+		break;
+	default:
+		rv = -ENXIO;
+		break;
+	}
+
+	return rv;
+}
+
 /*
  * _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.
@@ -245,8 +334,10 @@ 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;
+		return pmbus_read_virt_reg(client, page, reg);
+
 	return pmbus_read_word_data(client, page, reg);
 }
 
@@ -311,6 +402,66 @@ static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)
 	return pmbus_read_byte_data(client, page, reg);
 }
 
+static struct pmbus_sensor *pmbus_find_sensor(struct pmbus_data *data, int page,
+					      int reg)
+{
+	struct pmbus_sensor *sensor;
+
+	for (sensor = data->sensors; sensor; sensor = sensor->next) {
+		if (sensor->page == page && sensor->reg == reg)
+			return sensor;
+	}
+
+	return NULL;
+}
+
+static int pmbus_get_fan_rate(struct i2c_client *client, int page, int id,
+			      enum pmbus_fan_mode mode,
+			      bool from_cache)
+{
+	struct pmbus_data *data = i2c_get_clientdata(client);
+	bool want_rpm, have_rpm;
+	struct pmbus_sensor *s;
+	int config;
+	int reg;
+
+	want_rpm = (mode == rpm);
+
+	if (from_cache) {
+		reg = want_rpm ? PMBUS_VIRT_FAN_TARGET_1 : PMBUS_VIRT_PWM_1;
+		s = pmbus_find_sensor(data, page, reg + id);
+
+		return s->data;
+	}
+
+	config = pmbus_read_byte_data(client, page,
+				      pmbus_fan_config_registers[id]);
+	if (config < 0)
+		return config;
+
+	have_rpm = !!(config & pmbus_fan_rpm_mask[id]);
+	if (want_rpm == have_rpm)
+		return pmbus_read_word_data(client, page,
+					    pmbus_fan_command_registers[id]);
+
+	/* Can't sensibly map between RPM and PWM, just return zero */
+	return 0;
+}
+
+int pmbus_get_fan_rate_device(struct i2c_client *client, int page, int id,
+			      enum pmbus_fan_mode mode)
+{
+	return pmbus_get_fan_rate(client, page, id, mode, false);
+}
+EXPORT_SYMBOL_GPL(pmbus_get_fan_rate_device);
+
+int pmbus_get_fan_rate_cached(struct i2c_client *client, int page, int id,
+			      enum pmbus_fan_mode mode)
+{
+	return pmbus_get_fan_rate(client, page, id, mode, true);
+}
+EXPORT_SYMBOL_GPL(pmbus_get_fan_rate_cached);
+
 static void pmbus_clear_fault_page(struct i2c_client *client, int page)
 {
 	_pmbus_write_byte(client, page, PMBUS_CLEAR_FAULTS);
@@ -512,7 +663,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;
 	}
@@ -566,6 +717,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);
@@ -669,7 +823,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;
 	}
@@ -700,6 +854,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);
@@ -912,7 +1069,8 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
 					     const char *name, const char *type,
 					     int seq, int page, int reg,
 					     enum pmbus_sensor_classes class,
-					     bool update, bool readonly)
+					     bool update, bool readonly,
+					     bool convert)
 {
 	struct pmbus_sensor *sensor;
 	struct device_attribute *a;
@@ -922,12 +1080,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 = convert;
 	pmbus_dev_attr_init(a, sensor->name,
 			    readonly ? S_IRUGO : S_IRUGO | S_IWUSR,
 			    pmbus_show_sensor, pmbus_set_sensor);
@@ -1026,7 +1190,7 @@ static int pmbus_add_limit_attrs(struct i2c_client *client,
 			curr = pmbus_add_sensor(data, name, l->attr, index,
 						page, l->reg, attr->class,
 						attr->update || l->update,
-						false);
+						false, true);
 			if (!curr)
 				return -ENOMEM;
 			if (l->sbit && (info->func[page] & attr->sfunc)) {
@@ -1065,7 +1229,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
 			return ret;
 	}
 	base = pmbus_add_sensor(data, name, "input", index, page, attr->reg,
-				attr->class, true, true);
+				attr->class, true, true, true);
 	if (!base)
 		return -ENOMEM;
 	if (attr->sfunc) {
@@ -1589,13 +1753,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,
@@ -1618,6 +1775,46 @@ 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,
+				  false, false, true);
+
+	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,
+				  false, false, true);
+
+	if (!sensor)
+		return -ENOMEM;
+
+	sensor = pmbus_add_sensor(data, "pwm", "enable", index, page,
+				  PMBUS_VIRT_PWM_ENABLE_1 + id, PSC_PWM,
+				  true, false, false);
+
+	if (!sensor)
+		return -ENOMEM;
+
+	return 0;
+}
+
 static int pmbus_add_fan_attributes(struct i2c_client *client,
 				    struct pmbus_data *data)
 {
@@ -1652,9 +1849,18 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
 
 			if (pmbus_add_sensor(data, "fan", "input", index,
 					     page, pmbus_fan_registers[f],
-					     PSC_FAN, true, true) == NULL)
+					     PSC_FAN, true, 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.
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 2/4] pmbus (max31785): Add fan control
  2017-11-18  3:26 [PATCH v5 0/4] pmbus: Expand fan support and add MAX31785 driver Andrew Jeffery
  2017-11-18  3:26 ` [PATCH v5 1/4] pmbus (core): Add fan control support Andrew Jeffery
@ 2017-11-18  3:26 ` Andrew Jeffery
  2017-11-18  3:26 ` [PATCH v5 3/4] pmbus (core): Add virtual page config bit Andrew Jeffery
  2017-11-18  3:26 ` [PATCH v5 4/4] pmbus (max31785): Add dual tachometer support Andrew Jeffery
  3 siblings, 0 replies; 7+ messages in thread
From: Andrew Jeffery @ 2017-11-18  3:26 UTC (permalink / raw)
  To: linux-hwmon
  Cc: Andrew Jeffery, linux, robh+dt, mark.rutland, jdelvare, corbet,
	devicetree, linux-kernel, linux-doc, joel, openbmc

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   |   7 ++-
 drivers/hwmon/pmbus/max31785.c | 138 +++++++++++++++++++++++++++++++++-
 2 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
index 45fb6093dec2..7b0a0a8cdb6b 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,12 @@ 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 PWM control
+			2: Automatic PWM (tach-feedback RPM fan-control)
+			3: Automatic closed-loop (temp-feedback fan-control)
+
 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..2699134dfbe7 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -20,8 +20,136 @@ enum max31785_regs {
 
 #define MAX31785_NR_PAGES		23
 
+static int max31785_get_pwm(struct i2c_client *client, int page)
+{
+	int rv;
+
+	rv = pmbus_get_fan_rate_device(client, page, 0, percent);
+	if (rv < 0)
+		return rv;
+	else if (rv >= 0x8000)
+		return 0;
+	else if (rv >= 0x2711)
+		return 0x2710;
+
+	return rv;
+}
+
+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)
+		return (command >= 0x8000) ? 3 : 2;
+
+	if (command >= 0x8000)
+		return 3;
+	else if (command >= 0x2711)
+		return 0;
+
+	return 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);
+		break;
+	case PMBUS_VIRT_PWM_ENABLE_1:
+		rv = max31785_get_pwm_mode(client, page);
+		break;
+	default:
+		rv = -ENODATA;
+		break;
+	}
+
+	return rv;
+}
+
+static inline u32 max31785_scale_pwm(u32 sensor_val)
+{
+	/*
+	 * The datasheet describes the accepted value range for manual PWM as
+	 * [0, 0x2710], while the hwmon pwmX sysfs interface accepts values in
+	 * [0, 255]. The MAX31785 uses DIRECT mode to scale the FAN_COMMAND
+	 * registers and in PWM mode the coefficients are m=1, b=0, R=2. The
+	 * important observation here is that 0x2710 == 10000 == 100 * 100.
+	 *
+	 * R=2 (== 10^2 == 100) accounts for scaling the value provided at the
+	 * sysfs interface into the required hardware resolution, but it does
+	 * not yet yield a value that we can write to the device (this initial
+	 * scaling is handled by pmbus_data2reg()). Multiplying by 100 below
+	 * translates the parameter value into the percentage units required by
+	 * PMBus, and then we scale back by 255 as required by the hwmon pwmX
+	 * interface to yield the percentage value at the appropriate
+	 * resolution for hardware.
+	 */
+	return (sensor_val * 100) / 255;
+}
+
+static int max31785_pwm_enable(struct i2c_client *client, int page,
+				    u16 word)
+{
+	int config = 0;
+	int rate;
+
+	switch (word) {
+	case 0:
+		rate = 0x7fff;
+		break;
+	case 1:
+		rate = pmbus_get_fan_rate_cached(client, page, 0, percent);
+		rate = max31785_scale_pwm(rate);
+		if (rate < 0)
+			return rate;
+		break;
+	case 2:
+		config = PB_FAN_1_RPM;
+		rate = pmbus_get_fan_rate_cached(client, page, 0, rpm);
+		if (rate < 0)
+			return rate;
+		break;
+	case 3:
+		rate = 0xffff;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return pmbus_update_fan(client, page, 0, config, PB_FAN_1_RPM, rate);
+}
+
+static int max31785_write_word_data(struct i2c_client *client, int page,
+				    int reg, u16 word)
+{
+	switch (reg) {
+	case PMBUS_VIRT_PWM_1:
+		return pmbus_update_fan(client, page, 0, 0, PB_FAN_1_RPM,
+					max31785_scale_pwm(word));
+	case PMBUS_VIRT_PWM_ENABLE_1:
+		return max31785_pwm_enable(client, page, 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 +160,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,
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 3/4] pmbus (core): Add virtual page config bit
  2017-11-18  3:26 [PATCH v5 0/4] pmbus: Expand fan support and add MAX31785 driver Andrew Jeffery
  2017-11-18  3:26 ` [PATCH v5 1/4] pmbus (core): Add fan control support Andrew Jeffery
  2017-11-18  3:26 ` [PATCH v5 2/4] pmbus (max31785): Add fan control Andrew Jeffery
@ 2017-11-18  3:26 ` Andrew Jeffery
  2017-11-19 16:49   ` Guenter Roeck
  2017-11-18  3:26 ` [PATCH v5 4/4] pmbus (max31785): Add dual tachometer support Andrew Jeffery
  3 siblings, 1 reply; 7+ messages in thread
From: Andrew Jeffery @ 2017-11-18  3:26 UTC (permalink / raw)
  To: linux-hwmon
  Cc: Andrew Jeffery, linux, robh+dt, mark.rutland, jdelvare, corbet,
	devicetree, linux-kernel, linux-doc, joel, openbmc

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. An example of this is the Maxim MAX31785 controller, which
extends the READ_FAN_SPEED_1 PMBus register from two to four bytes to support
tach readings for both rotors of a dual rotor fan. This extended register
contains two word-sized values, one reporting the rate of the fastest rotor,
the other the rate of the slowest. The concept of virtual pages aids this
situation by mapping the page number onto the value to be selected from the
vectored result.

We should not try to set virtual pages on the device as such a page explicitly
doesn't exist; 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 | 10 +++++++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index b54d7604d3ef..d39d506aa63e 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -372,6 +372,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 631db88526b6..ba97230fcde7 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, int page)
 	int rv = 0;
 	int newpage;
 
-	if (page >= 0 && page != data->currpage) {
+	if (page < 0 || 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;
 	}
+
+	data->currpage = page;
+
 	return rv;
 }
 EXPORT_SYMBOL_GPL(pmbus_set_page);
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 4/4] pmbus (max31785): Add dual tachometer support
  2017-11-18  3:26 [PATCH v5 0/4] pmbus: Expand fan support and add MAX31785 driver Andrew Jeffery
                   ` (2 preceding siblings ...)
  2017-11-18  3:26 ` [PATCH v5 3/4] pmbus (core): Add virtual page config bit Andrew Jeffery
@ 2017-11-18  3:26 ` Andrew Jeffery
  3 siblings, 0 replies; 7+ messages in thread
From: Andrew Jeffery @ 2017-11-18  3:26 UTC (permalink / raw)
  To: linux-hwmon
  Cc: Andrew Jeffery, linux, robh+dt, mark.rutland, jdelvare, corbet,
	devicetree, linux-kernel, linux-doc, joel, openbmc

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 pages 23-28 (not defined by the
hardware) 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 corresponds to the first word of the response. The TACHSEL=0
measurement is used by the controller's closed-loop fan management to judge
target fan rate.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/hwmon/max31785   |   8 +-
 drivers/hwmon/pmbus/max31785.c | 147 ++++++++++++++++++++++++++++++++++-
 2 files changed, 152 insertions(+), 3 deletions(-)

diff --git a/Documentation/hwmon/max31785 b/Documentation/hwmon/max31785
index 7b0a0a8cdb6b..270c5f865261 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 2699134dfbe7..b5389070ae06 100644
--- a/drivers/hwmon/pmbus/max31785.c
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -16,9 +16,79 @@
 
 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
+#define MAX31785_NR_FAN_PAGES		6
+
+static int max31785_read_byte_data(struct i2c_client *client, int page,
+				   int reg)
+{
+	if (page < MAX31785_NR_PAGES)
+		return -ENODATA;
+
+	switch (reg) {
+	case PMBUS_VOUT_MODE:
+		return -ENOTSUPP;
+	case PMBUS_FAN_CONFIG_12:
+		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)
 {
@@ -62,9 +132,30 @@ static int max31785_get_pwm_mode(struct i2c_client *client, int page)
 static int max31785_read_word_data(struct i2c_client *client, int page,
 				   int reg)
 {
+	u32 val;
 	int rv;
 
 	switch (reg) {
+	case PMBUS_READ_FAN_SPEED_1:
+		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_FAN_COMMAND_1:
+		/*
+		 * PMBUS_FAN_COMMAND_x is probed to judge whether or not to
+		 * expose fan control registers.
+		 *
+		 * Don't expose fan_target attribute for virtual pages.
+		 */
+		rv = (page >= MAX31785_NR_PAGES) ? -ENOTSUPP : -ENODATA;
+		break;
 	case PMBUS_VIRT_PWM_1:
 		rv = max31785_get_pwm(client, page);
 		break;
@@ -157,11 +248,15 @@ static int max31785_write_word_data(struct i2c_client *client, int page,
 #define MAX31785_VOUT_FUNCS \
 	(PMBUS_HAVE_VOUT | PMBUS_HAVE_STATUS_VOUT)
 
+#define MAX37185_NUM_FAN_PAGES 6
+
 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,
@@ -208,13 +303,46 @@ static const struct pmbus_driver_info max31785_info = {
 	.func[22] = MAX31785_VOUT_FUNCS,
 };
 
+static int max31785_configure_dual_tach(struct i2c_client *client,
+					struct pmbus_driver_info *info)
+{
+	int ret;
+	int i;
+
+	for (i = 0; i < MAX31785_NR_FAN_PAGES; 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 = 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)
 {
 	struct device *dev = &client->dev;
 	struct pmbus_driver_info *info;
+	bool dual_tach = false;
 	s64 ret;
 
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WORD_DATA))
+		return -ENODEV;
+
 	info = devm_kzalloc(dev, sizeof(struct pmbus_driver_info), GFP_KERNEL);
 	if (!info)
 		return -ENOMEM;
@@ -225,6 +353,25 @@ static int max31785_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	ret = i2c_smbus_read_word_data(client, MFR_REVISION);
+	if (ret < 0)
+		return ret;
+
+	if (ret == MAX31785A) {
+		dual_tach = true;
+	} else if (ret == MAX31785) {
+		if (!strcmp("max31785a", id->name))
+			dev_warn(dev, "Expected max3175a, found max31785: cannot provide secondary tachometer readings\n");
+	} else {
+		return -ENODEV;
+	}
+
+	if (dual_tach) {
+		ret = max31785_configure_dual_tach(client, info);
+		if (ret < 0)
+			return ret;
+	}
+
 	return pmbus_do_probe(client, id, info);
 }
 
-- 
git-series 0.9.1

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 3/4] pmbus (core): Add virtual page config bit
  2017-11-18  3:26 ` [PATCH v5 3/4] pmbus (core): Add virtual page config bit Andrew Jeffery
@ 2017-11-19 16:49   ` Guenter Roeck
  2017-11-20  0:40     ` Andrew Jeffery
  0 siblings, 1 reply; 7+ messages in thread
From: Guenter Roeck @ 2017-11-19 16:49 UTC (permalink / raw)
  To: Andrew Jeffery, linux-hwmon
  Cc: robh+dt, mark.rutland, jdelvare, corbet, devicetree,
	linux-kernel, linux-doc, joel, openbmc

On 11/17/2017 07:26 PM, Andrew Jeffery wrote:
> 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. An example of this is the Maxim MAX31785 controller, which
> extends the READ_FAN_SPEED_1 PMBus register from two to four bytes to support
> tach readings for both rotors of a dual rotor fan. This extended register
> contains two word-sized values, one reporting the rate of the fastest rotor,
> the other the rate of the slowest. The concept of virtual pages aids this
> situation by mapping the page number onto the value to be selected from the
> vectored result.
> 
> We should not try to set virtual pages on the device as such a page explicitly
> doesn't exist; 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 | 10 +++++++---
>   2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index b54d7604d3ef..d39d506aa63e 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -372,6 +372,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 631db88526b6..ba97230fcde7 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, int page)
>   	int rv = 0;
>   	int newpage;
>   
> -	if (page >= 0 && page != data->currpage) {
> +	if (page < 0 || 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;

This should probably be something like
		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
		if (rv < 0)
			return rv;
		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
		if (newpage < 0)
			return newpage;
		if (newpage != page)
			return -EIO;

We can actually drop 'newpage' and just use 'rv'.

> -		else
> -			data->currpage = page;
>   	}
> +
> +	data->currpage = page;
> +

... otherwise currpage is set even on error.

>   	return rv;

this can then be
	return 0;

>   }
>   EXPORT_SYMBOL_GPL(pmbus_set_page);
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 3/4] pmbus (core): Add virtual page config bit
  2017-11-19 16:49   ` Guenter Roeck
@ 2017-11-20  0:40     ` Andrew Jeffery
  0 siblings, 0 replies; 7+ messages in thread
From: Andrew Jeffery @ 2017-11-20  0:40 UTC (permalink / raw)
  To: Guenter Roeck, linux-hwmon
  Cc: robh+dt, mark.rutland, jdelvare, corbet, devicetree,
	linux-kernel, linux-doc, joel, openbmc

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

On Sun, 2017-11-19 at 08:49 -0800, Guenter Roeck wrote:
> On 11/17/2017 07:26 PM, Andrew Jeffery wrote:
> > 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. An example of this is the Maxim MAX31785 controller, which
> > extends the READ_FAN_SPEED_1 PMBus register from two to four bytes to support
> > tach readings for both rotors of a dual rotor fan. This extended register
> > contains two word-sized values, one reporting the rate of the fastest rotor,
> > the other the rate of the slowest. The concept of virtual pages aids this
> > situation by mapping the page number onto the value to be selected from the
> > vectored result.
> > 
> > We should not try to set virtual pages on the device as such a page explicitly
> > doesn't exist; 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 | 10 +++++++---
> >   2 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index b54d7604d3ef..d39d506aa63e 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -372,6 +372,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 631db88526b6..ba97230fcde7 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, int page)
> >   	int rv = 0;
> >   	int newpage;
> >   
> > -	if (page >= 0 && page != data->currpage) {
> > +	if (page < 0 || 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;
> 
> This should probably be something like
> 		rv = i2c_smbus_write_byte_data(client, PMBUS_PAGE, page);
> 		if (rv < 0)
> 			return rv;
> 		newpage = i2c_smbus_read_byte_data(client, PMBUS_PAGE);
> 		if (newpage < 0)
> 			return newpage;
> 		if (newpage != page)
> 			return -EIO;
> 
> We can actually drop 'newpage' and just use 'rv'.
> 
> > -		else
> > -			data->currpage = page;
> >   	}
> > +
> > +	data->currpage = page;
> > +
> 
> ... otherwise currpage is set even on error.

Thanks for catching that, clearly I needed to pay more attention
resolving the conflicts when rebasing on hwmon-next. I'll address all
your points.

Cheers,

Andrew

> 
> >   	return rv;
> 
> this can then be
> 	return 0;
> 
> >   }
> >   EXPORT_SYMBOL_GPL(pmbus_set_page);
> > 
> 
> 

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2017-11-20  0:41 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-18  3:26 [PATCH v5 0/4] pmbus: Expand fan support and add MAX31785 driver Andrew Jeffery
2017-11-18  3:26 ` [PATCH v5 1/4] pmbus (core): Add fan control support Andrew Jeffery
2017-11-18  3:26 ` [PATCH v5 2/4] pmbus (max31785): Add fan control Andrew Jeffery
2017-11-18  3:26 ` [PATCH v5 3/4] pmbus (core): Add virtual page config bit Andrew Jeffery
2017-11-19 16:49   ` Guenter Roeck
2017-11-20  0:40     ` Andrew Jeffery
2017-11-18  3:26 ` [PATCH v5 4/4] pmbus (max31785): Add dual tachometer support Andrew Jeffery

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).