* [RFC PATCH 0/4] pmbus: Expand fan support and add MAX31785 driver
@ 2017-07-10 13:56 Andrew Jeffery
2017-07-10 13:56 ` [RFC PATCH 1/4] hwmon: pmbus: Disable build of non-max31785 drivers Andrew Jeffery
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-10 13:56 UTC (permalink / raw)
To: linux, linux-hwmon
Cc: Andrew Jeffery, jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
Hello,
I recently sent a patch adding support for the MAX31785 chip[1], but
implemented in terms of hwmon rather than pmbus. The hwmon-based implementation
was driven by a lack of support for the features we needed from the pmbus
subsystem but ideally that shouldn't be a roadblock to a pmbus implementation.
[1] https://lkml.org/lkml/2017/6/6/71
So on that front, I've had a go at modifying the pmbus core to explore the
problem space and provide what we needed for the MAX31785 chip. Namely,
exposing fan interfaces for both RPM and PWM control:
* fan[1-*]_target
* pwm[1-*]
* pwm[1-*]_enable
The interfaces exposed conform to the hwmon sysfs definitions as much as
possible, though there are some warts due to the nature of PMBus. Specifically,
the following details need some discussion:
1. The PMBus spec defines FAN_COMMAND_[1-4] as the fan rate control register
2. Fan rate control can be done either in terms of PWM or RPM
3. Conversion between PWM and RPM is configuration dependent and therefore
cannot be generic or even necessarily provided by driver callbacks.
4. Attributes for RPM and PWM control need to be exposed to userspace, but only
one set can be operational at a given time (constrained by the one command
register).
5. Therefore it is not generally possible to support reading both
fan[1-*]_target and pwm[1-*] without changing the control method as
appropriate
To address this last point the implementation currently returns -ENOTSUPP for
reads of fan[1-*]_target and pwm[1-*] if they are not associated with the
current control mode.
Further, pwm[1-*]_enable is effectively invalid whilst in RPM mode. Maybe it
too should return -ENOTSUPP when read in the RPM context, but currently it
provides a value.
Further I've implemented the change of control mode as a last-write-wins
strategy, where a write to fan[1-*]_target configures the fan for RPM mode,
whilst a write to pwm[1-*] or pwm[1-*]_target configures PWM mode. It's not
clear to me that there's any other good strategy whilst being confined to the
existing hwmon sysfs interface.
Given the fan control implementation is driven by wanting to support the
MAX31785 device, the patches add some new callbacks to struct pmbus_driver_info:
int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
u16 *fan_command);
These callbacks allow the driver to use the fan configuration and command
values to interpret or return the correct value for pwm[1-*]_enable. The
MAX31785 chip defines several value ranges for FAN_COMMAND_[1-4] that put the
chip in manual, automatic or full-speed modes.
A third callback was also added:
const struct pmbus_coeffs *(*get_fan_coeffs)(
const struct pmbus_driver_info *info, int index,
enum pmbus_fan_mode mode, int command);
This was a strategy to cope with the MAX31785 chip changing its fan
coefficients when moving between RPM and PWM control modes: The current
coefficients array mechanism isn't capable of describing this behaviour.
Further, different subsets of fan-related registers are affected by the control
mode change. Thus, the simplest solution with the least impact on existing
drivers appeared to be to provide an optional callback member. To go back on
that a little, I've changed the array structure anyway to make the callback
prototype and pmbus_core implementation a little easier. This justifies the
first patch in the RFC, which simply removes the ability to build any of the
other pmbus drivers. Clearly it's a junk patch that won't exist in a non-RFC
series, as if the idea is sensible then all existing drivers will be converted.
I'm hoping this is a reasonable start to getting the support we need for the
MAX31785 chip into the PMBus subsystem, and that we can evolve it into
something acceptable.
Still on the TODO list is adding support for variants of the MAX31785 that
can perform dual-rotor measurements. I haven't yet had a good think about how
to do it given the hardware behaviour, but given the design of pmbus core it
could be a bit intrusive. Having sent this series out I'll have a think about
the problem, and will look to send a follow-up RFC hashing out an
implementation.
Cheers,
Andrew
Andrew Jeffery (4):
hwmon: pmbus: Disable build of non-max31785 drivers
pmbus: Add fan configuration support
pmbus: Allow dynamic fan coefficient values
pmbus: Add MAX31785 driver
drivers/hwmon/pmbus/Kconfig | 10 +
drivers/hwmon/pmbus/Makefile | 25 +--
drivers/hwmon/pmbus/max31785.c | 201 ++++++++++++++++++
drivers/hwmon/pmbus/pmbus.h | 25 ++-
drivers/hwmon/pmbus/pmbus_core.c | 439 +++++++++++++++++++++++++++++++++++++--
5 files changed, 670 insertions(+), 30 deletions(-)
create mode 100644 drivers/hwmon/pmbus/max31785.c
--
2.11.0
^ permalink raw reply [flat|nested] 16+ messages in thread
* [RFC PATCH 1/4] hwmon: pmbus: Disable build of non-max31785 drivers
2017-07-10 13:56 [RFC PATCH 0/4] pmbus: Expand fan support and add MAX31785 driver Andrew Jeffery
@ 2017-07-10 13:56 ` Andrew Jeffery
2017-07-10 13:56 ` [RFC PATCH 2/4] pmbus: Add fan configuration support Andrew Jeffery
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-10 13:56 UTC (permalink / raw)
To: linux, linux-hwmon
Cc: Andrew Jeffery, jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
Later patches make incompatible changes to struct pmbus_driver_info, so this
serves to keep the following patches buildable under otherwise incompatible
configurations. It is a junk patch that won't be present in a non-RFC series.
drivers/hwmon/pmbus/Makefile | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/hwmon/pmbus/Makefile b/drivers/hwmon/pmbus/Makefile
index 562132054aaf..24ff7ee7f8bb 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -4,15 +4,15 @@
obj-$(CONFIG_PMBUS) += pmbus_core.o
obj-$(CONFIG_SENSORS_PMBUS) += pmbus.o
-obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
-obj-$(CONFIG_SENSORS_LM25066) += lm25066.o
-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_MAX34440) += max34440.o
-obj-$(CONFIG_SENSORS_MAX8688) += max8688.o
-obj-$(CONFIG_SENSORS_TPS40422) += tps40422.o
-obj-$(CONFIG_SENSORS_UCD9000) += ucd9000.o
-obj-$(CONFIG_SENSORS_UCD9200) += ucd9200.o
-obj-$(CONFIG_SENSORS_ZL6100) += zl6100.o
+# obj-$(CONFIG_SENSORS_ADM1275) += adm1275.o
+# obj-$(CONFIG_SENSORS_LM25066) += lm25066.o
+# 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_MAX34440) += max34440.o
+# obj-$(CONFIG_SENSORS_MAX8688) += max8688.o
+# obj-$(CONFIG_SENSORS_TPS40422) += tps40422.o
+# obj-$(CONFIG_SENSORS_UCD9000) += ucd9000.o
+# obj-$(CONFIG_SENSORS_UCD9200) += ucd9200.o
+# obj-$(CONFIG_SENSORS_ZL6100) += zl6100.o
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH 2/4] pmbus: Add fan configuration support
2017-07-10 13:56 [RFC PATCH 0/4] pmbus: Expand fan support and add MAX31785 driver Andrew Jeffery
2017-07-10 13:56 ` [RFC PATCH 1/4] hwmon: pmbus: Disable build of non-max31785 drivers Andrew Jeffery
@ 2017-07-10 13:56 ` Andrew Jeffery
2017-07-11 13:40 ` Guenter Roeck
2017-07-10 13:56 ` [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values Andrew Jeffery
2017-07-10 13:56 ` [RFC PATCH 4/4] pmbus: Add MAX31785 driver Andrew Jeffery
3 siblings, 1 reply; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-10 13:56 UTC (permalink / raw)
To: linux, linux-hwmon
Cc: Andrew Jeffery, jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
Augment PMBus support to include control of fans via the
FAN_COMMAND_[1-4] registers, both in RPM and PWM modes. The behaviour
of FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4] are tightly coupled, and
their interactions do not fit the existing use of struct pmbus_sensor.
The patch introduces struct pmbus_fan_ctrl to distinguish from the
simple sensor case, along with associated sysfs show/set implementations.
Further, the interpreting the value of FAN_COMMAND_[1-4] depends on both
the current fan mode (RPM or PWM, as configured in
FAN_CONFIG_{1_2,3_4}), and the device-specific behaviour for the
register. For example, the MAX31785 chip defines the following:
PWM (m = 1, b = 0, R = 2):
0x0000 - 0x2710: 0 - 100% fan PWM duty cycle
0x2711 - 0x7fff: 100% fan PWM duty cycle
0x8000 - 0xffff: Ignore FAN_COMMAND_[1-4], use automatic fan control
RPM (m = 1, b = 0, R = 0):
0x0000 - 0x7FFF: 0 - 32,767 RPM
0x8000 - 0xFFFF: Ignore FAN_COMMAND_[1-4], use automatic fan control
To handle the device-specific interpretation of the FAN_COMMAND_[1-4],
add an optional callbacks to the info struct to get/set the 'mode'
value required for the pwm[1-n]_enable sysfs attribute. A fallback
calculation exists if the callbacks are not populated; the fallback
ignores device-specific ranges and tries to determine a reasonable value
from FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4].
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
drivers/hwmon/pmbus/pmbus.h | 7 +
drivers/hwmon/pmbus/pmbus_core.c | 335 +++++++++++++++++++++++++++++++++++++++
2 files changed, 342 insertions(+)
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13bae34b..927eabc1b273 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -223,6 +223,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)
*/
@@ -380,6 +382,11 @@ struct pmbus_driver_info {
int (*identify)(struct i2c_client *client,
struct pmbus_driver_info *info);
+ /* Allow the driver to interpret the fan command value */
+ int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
+ int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
+ u16 *fan_command);
+
/* Regulator functionality, if supported by this chip driver. */
int num_regulators;
const struct regulator_desc *reg_desc;
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ba59eaef2e07..3b0a55bbbd2c 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -69,6 +69,38 @@ struct pmbus_sensor {
#define to_pmbus_sensor(_attr) \
container_of(_attr, struct pmbus_sensor, attribute)
+#define PB_FAN_CONFIG_RPM PB_FAN_2_RPM
+#define PB_FAN_CONFIG_INSTALLED PB_FAN_2_INSTALLED
+#define PB_FAN_CONFIG_MASK(i) (0xff << (4 * !((i) & 1)))
+#define PB_FAN_CONFIG_GET(i, n) (((n) >> (4 * !((i) & 1))) & 0xff)
+#define PB_FAN_CONFIG_PUT(i, n) (((n) & 0xff) << (4 * !((i) & 1)))
+
+struct pmbus_fan_ctrl_attr {
+ struct device_attribute attribute;
+ char name[PMBUS_NAME_SIZE];
+};
+
+struct pmbus_fan_ctrl {
+ struct pmbus_fan_ctrl_attr fan_target;
+ struct pmbus_fan_ctrl_attr pwm;
+ struct pmbus_fan_ctrl_attr pwm_enable;
+ int index;
+ u8 page;
+ u8 id;
+ u8 config;
+ u16 command;
+};
+#define to_pmbus_fan_ctrl_attr(_attr) \
+ container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
+#define fan_target_to_pmbus_fan_ctrl(_attr) \
+ container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
+ fan_target)
+#define pwm_to_pmbus_fan_ctrl(_attr) \
+ container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, pwm)
+#define pwm_enable_to_pmbus_fan_ctrl(_attr) \
+ container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
+ pwm_enable)
+
struct pmbus_boolean {
char name[PMBUS_NAME_SIZE]; /* sysfs boolean name */
struct sensor_device_attribute attribute;
@@ -806,6 +838,219 @@ static ssize_t pmbus_show_label(struct device *dev,
return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
}
+static ssize_t pmbus_show_fan_command(struct device *dev,
+ enum pmbus_fan_mode mode,
+ struct pmbus_fan_ctrl *fan, char *buf)
+{
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct pmbus_data *data = i2c_get_clientdata(client);
+ struct pmbus_sensor sensor;
+ long val;
+
+ mutex_lock(&data->update_lock);
+
+ if ((mode == percent && (fan->config & PB_FAN_CONFIG_RPM)) ||
+ (mode == rpm && !(fan->config & PB_FAN_CONFIG_RPM))) {
+ mutex_unlock(&data->update_lock);
+ return -ENOTSUPP; /* XXX: This seems dodgy, but what to do? */
+ }
+
+ sensor.class = PSC_FAN;
+ if (mode == percent)
+ sensor.data = fan->command * 255 / 100;
+ else
+ sensor.data = fan->command;
+
+ val = pmbus_reg2data(data, &sensor);
+
+ mutex_unlock(&data->update_lock);
+
+ return snprintf(buf, PAGE_SIZE, "%ld\n", val);
+}
+
+static ssize_t pmbus_show_fan_target(struct device *dev,
+ struct device_attribute *da, char *buf)
+{
+ return pmbus_show_fan_command(dev, rpm,
+ fan_target_to_pmbus_fan_ctrl(da), buf);
+}
+
+static ssize_t pmbus_show_pwm(struct device *dev,
+ struct device_attribute *da, char *buf)
+{
+ return pmbus_show_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
+ buf);
+}
+
+static ssize_t pmbus_set_fan_command(struct device *dev,
+ enum pmbus_fan_mode mode,
+ struct pmbus_fan_ctrl *fan,
+ const char *buf, ssize_t count)
+{
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct pmbus_data *data = i2c_get_clientdata(client);
+ int config_addr, command_addr;
+ struct pmbus_sensor sensor;
+ ssize_t rv;
+ long val;
+
+ if (kstrtol(buf, 10, &val) < 0)
+ return -EINVAL;
+
+ mutex_lock(&data->update_lock);
+
+ sensor.class = PSC_FAN;
+
+ val = pmbus_data2reg(data, &sensor, val);
+
+ if (mode == percent)
+ val = val * 100 / 255;
+
+ config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
+ command_addr = config_addr + 1 + (fan->id & 1);
+
+ if (mode == rpm)
+ fan->config |= PB_FAN_CONFIG_RPM;
+ else
+ fan->config &= ~PB_FAN_CONFIG_RPM;
+
+ rv = pmbus_update_byte_data(client, fan->page, config_addr,
+ PB_FAN_CONFIG_PUT(fan->id, fan->config),
+ PB_FAN_CONFIG_MASK(fan->id));
+ if (rv < 0)
+ goto done;
+
+ fan->command = val;
+ rv = pmbus_write_word_data(client, fan->page, command_addr,
+ fan->command);
+
+done:
+ mutex_unlock(&data->update_lock);
+
+ if (rv < 0)
+ return rv;
+
+ return count;
+}
+
+static ssize_t pmbus_set_fan_target(struct device *dev,
+ struct device_attribute *da,
+ const char *buf, size_t count)
+{
+ return pmbus_set_fan_command(dev, rpm,
+ fan_target_to_pmbus_fan_ctrl(da), buf,
+ count);
+}
+
+static ssize_t pmbus_set_pwm(struct device *dev, struct device_attribute *da,
+ const char *buf, size_t count)
+{
+ return pmbus_set_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
+ buf, count);
+}
+
+static ssize_t pmbus_show_pwm_enable(struct device *dev,
+ struct device_attribute *da, char *buf)
+{
+ struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct pmbus_data *data = i2c_get_clientdata(client);
+ long mode;
+
+ mutex_lock(&data->update_lock);
+
+
+ if (data->info->get_pwm_mode) {
+ u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
+
+ mode = data->info->get_pwm_mode(fan->id, config, fan->command);
+ } else {
+ struct pmbus_sensor sensor = {
+ .class = PSC_FAN,
+ .data = fan->command,
+ };
+ long command;
+
+ command = pmbus_reg2data(data, &sensor);
+
+ /* XXX: Need to do something sensible */
+ if (fan->config & PB_FAN_CONFIG_RPM)
+ mode = 2;
+ else
+ mode = (command >= 0 && command < 100);
+ }
+
+ mutex_unlock(&data->update_lock);
+
+ return snprintf(buf, PAGE_SIZE, "%ld\n", mode);
+}
+
+static ssize_t pmbus_set_pwm_enable(struct device *dev,
+ struct device_attribute *da,
+ const char *buf, size_t count)
+{
+ struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
+ struct i2c_client *client = to_i2c_client(dev->parent);
+ struct pmbus_data *data = i2c_get_clientdata(client);
+ int config_addr, command_addr;
+ struct pmbus_sensor sensor;
+ ssize_t rv = count;
+ long mode;
+
+ if (kstrtol(buf, 10, &mode) < 0)
+ return -EINVAL;
+
+ mutex_lock(&data->update_lock);
+
+ sensor.class = PSC_FAN;
+
+ config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
+ command_addr = config_addr + 1 + (fan->id & 1);
+
+ if (data->info->set_pwm_mode) {
+ u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
+ u16 command = fan->command;
+
+ rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
+ if (rv < 0)
+ goto done;
+
+ fan->config = PB_FAN_CONFIG_GET(fan->id, config);
+ fan->command = command;
+ } else {
+ fan->config &= ~PB_FAN_CONFIG_RPM;
+ switch (mode) {
+ case 0:
+ case 1:
+ /* XXX: Safe at least? */
+ fan->command = pmbus_data2reg(data, &sensor, 100);
+ break;
+ case 2:
+ default:
+ /* XXX: Safe at least? */
+ fan->command = 0xffff;
+ break;
+ }
+ }
+
+ rv = pmbus_update_byte_data(client, fan->page, config_addr,
+ PB_FAN_CONFIG_PUT(fan->id, fan->config),
+ PB_FAN_CONFIG_MASK(fan->id));
+ if (rv < 0)
+ goto done;
+
+ rv = pmbus_write_word_data(client, fan->page, command_addr,
+ fan->command);
+
+done:
+ mutex_unlock(&data->update_lock);
+
+ if (rv < 0)
+ return rv;
+
+ return count;
+}
+
static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
{
if (data->num_attributes >= data->max_attributes - 1) {
@@ -1094,6 +1339,51 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
return 0;
}
+static int pmbus_add_fan_ctrl_attr(struct pmbus_data *data,
+ struct pmbus_fan_ctrl_attr *fa,
+ const char *name_fmt,
+ ssize_t (*show)(struct device *dev,
+ struct device_attribute *attr,
+ char *buf),
+ ssize_t (*store)(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count),
+ int idx)
+{
+ struct device_attribute *da;
+
+ da = &fa->attribute;
+
+ snprintf(fa->name, sizeof(fa->name), name_fmt, idx);
+ pmbus_dev_attr_init(da, fa->name, 0644, show, store);
+
+ return pmbus_add_attribute(data, &da->attr);
+}
+
+static inline int pmbus_add_fan_target_attr(struct pmbus_data *data,
+ struct pmbus_fan_ctrl *fan)
+{
+ return pmbus_add_fan_ctrl_attr(data, &fan->fan_target, "fan%d_target",
+ pmbus_show_fan_target,
+ pmbus_set_fan_target, fan->index);
+}
+
+static inline int pmbus_add_pwm_attr(struct pmbus_data *data,
+ struct pmbus_fan_ctrl *fan)
+{
+
+ return pmbus_add_fan_ctrl_attr(data, &fan->pwm, "pwm%d", pmbus_show_pwm,
+ pmbus_set_pwm, fan->index);
+}
+
+static inline int pmbus_add_pwm_enable_attr(struct pmbus_data *data,
+ struct pmbus_fan_ctrl *fan)
+{
+ return pmbus_add_fan_ctrl_attr(data, &fan->pwm_enable, "pwm%d_enable",
+ pmbus_show_pwm_enable,
+ pmbus_set_pwm_enable, fan->index);
+}
+
static const struct pmbus_limit_attr vin_limit_attrs[] = {
{
.reg = PMBUS_VIN_UV_WARN_LIMIT,
@@ -1565,6 +1855,13 @@ static const int pmbus_fan_config_registers[] = {
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,
+};
+
static const int pmbus_fan_status_registers[] = {
PMBUS_STATUS_FAN_12,
PMBUS_STATUS_FAN_12,
@@ -1587,6 +1884,39 @@ 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_fan_ctrl *fan;
+ int rv;
+
+ fan = devm_kzalloc(data->dev, sizeof(*fan), GFP_KERNEL);
+ if (!fan)
+ return -ENOMEM;
+
+ fan->index = index;
+ fan->page = page;
+ fan->id = id;
+ fan->config = config;
+
+ rv = _pmbus_read_word_data(client, page,
+ pmbus_fan_command_registers[id]);
+ if (rv < 0)
+ return rv;
+ fan->command = rv;
+
+ rv = pmbus_add_fan_target_attr(data, fan);
+ if (rv < 0)
+ return rv;
+
+ rv = pmbus_add_pwm_attr(data, fan);
+ if (rv < 0)
+ return rv;
+
+ return pmbus_add_pwm_enable_attr(data, fan);
+}
+
static int pmbus_add_fan_attributes(struct i2c_client *client,
struct pmbus_data *data)
{
@@ -1624,6 +1954,11 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
PSC_FAN, true, true) == NULL)
return -ENOMEM;
+ 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values
2017-07-10 13:56 [RFC PATCH 0/4] pmbus: Expand fan support and add MAX31785 driver Andrew Jeffery
2017-07-10 13:56 ` [RFC PATCH 1/4] hwmon: pmbus: Disable build of non-max31785 drivers Andrew Jeffery
2017-07-10 13:56 ` [RFC PATCH 2/4] pmbus: Add fan configuration support Andrew Jeffery
@ 2017-07-10 13:56 ` Andrew Jeffery
2017-07-11 13:31 ` Guenter Roeck
2017-07-10 13:56 ` [RFC PATCH 4/4] pmbus: Add MAX31785 driver Andrew Jeffery
3 siblings, 1 reply; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-10 13:56 UTC (permalink / raw)
To: linux, linux-hwmon
Cc: Andrew Jeffery, jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
Some PMBus chips, such as the MAX31785, use different coefficients for
FAN_COMMAND_[1-4] depending on whether the fan is in PWM (percent duty)
or RPM mode. Add a callback to allow the driver to provide the
applicable coefficients to avoid imposing on devices that don't have
this requirement.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
drivers/hwmon/pmbus/pmbus.h | 18 +++++--
drivers/hwmon/pmbus/pmbus_core.c | 112 ++++++++++++++++++++++++++++++++-------
2 files changed, 108 insertions(+), 22 deletions(-)
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 927eabc1b273..338ecc8b25a4 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -345,6 +345,12 @@ enum pmbus_sensor_classes {
enum pmbus_data_format { linear = 0, direct, vid };
enum vrm_version { vr11 = 0, vr12 };
+struct pmbus_coeffs {
+ int m; /* mantissa for direct data format */
+ int b; /* offset */
+ int R; /* exponent */
+};
+
struct pmbus_driver_info {
int pages; /* Total number of pages */
enum pmbus_data_format format[PSC_NUM_CLASSES];
@@ -353,9 +359,7 @@ struct pmbus_driver_info {
* Support one set of coefficients for each sensor type
* Used for chips providing data in direct mode.
*/
- int m[PSC_NUM_CLASSES]; /* mantissa for direct data format */
- int b[PSC_NUM_CLASSES]; /* offset */
- int R[PSC_NUM_CLASSES]; /* exponent */
+ struct pmbus_coeffs coeffs[PSC_NUM_CLASSES];
u32 func[PMBUS_PAGES]; /* Functionality, per page */
/*
@@ -382,6 +386,14 @@ struct pmbus_driver_info {
int (*identify)(struct i2c_client *client,
struct pmbus_driver_info *info);
+ /*
+ * If a fan's coefficents change over time (e.g. between RPM and PWM
+ * mode), then the driver can provide a function for retrieving the
+ * currently applicable coefficients.
+ */
+ const struct pmbus_coeffs *(*get_fan_coeffs)(
+ const struct pmbus_driver_info *info, int index,
+ enum pmbus_fan_mode mode, int command);
/* Allow the driver to interpret the fan command value */
int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 3b0a55bbbd2c..4ff6a1fd5cce 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -58,10 +58,11 @@
struct pmbus_sensor {
struct pmbus_sensor *next;
char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */
- struct device_attribute attribute;
+ struct sensor_device_attribute attribute;
u8 page; /* page number */
u16 reg; /* register */
enum pmbus_sensor_classes class; /* sensor class */
+ const struct pmbus_coeffs *coeffs;
bool update; /* runtime sensor update needed */
int data; /* Sensor data.
Negative if there was a read error */
@@ -89,6 +90,7 @@ struct pmbus_fan_ctrl {
u8 id;
u8 config;
u16 command;
+ const struct pmbus_coeffs *coeffs;
};
#define to_pmbus_fan_ctrl_attr(_attr) \
container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
@@ -511,9 +513,15 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
long val = (s16) sensor->data;
long m, b, R;
- m = data->info->m[sensor->class];
- b = data->info->b[sensor->class];
- R = data->info->R[sensor->class];
+ if (sensor->coeffs) {
+ m = sensor->coeffs->m;
+ b = sensor->coeffs->b;
+ R = sensor->coeffs->R;
+ } else {
+ m = data->info->coeffs[sensor->class].m;
+ b = data->info->coeffs[sensor->class].b;
+ R = data->info->coeffs[sensor->class].R;
+ }
if (m == 0)
return 0;
@@ -663,9 +671,15 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
{
long m, b, R;
- m = data->info->m[sensor->class];
- b = data->info->b[sensor->class];
- R = data->info->R[sensor->class];
+ if (sensor->coeffs) {
+ m = sensor->coeffs->m;
+ b = sensor->coeffs->b;
+ R = sensor->coeffs->R;
+ } else {
+ m = data->info->coeffs[sensor->class].m;
+ b = data->info->coeffs[sensor->class].b;
+ R = data->info->coeffs[sensor->class].R;
+ }
/* Power is in uW. Adjust R and b. */
if (sensor->class == PSC_POWER) {
@@ -796,7 +810,9 @@ static ssize_t pmbus_show_sensor(struct device *dev,
struct device_attribute *devattr, char *buf)
{
struct pmbus_data *data = pmbus_update_device(dev);
- struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
+ struct pmbus_sensor *sensor;
+
+ sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
if (sensor->data < 0)
return sensor->data;
@@ -810,12 +826,14 @@ static ssize_t pmbus_set_sensor(struct device *dev,
{
struct i2c_client *client = to_i2c_client(dev->parent);
struct pmbus_data *data = i2c_get_clientdata(client);
- struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
+ struct pmbus_sensor *sensor;
ssize_t rv = count;
long val = 0;
int ret;
u16 regval;
+ sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
+
if (kstrtol(buf, 10, &val) < 0)
return -EINVAL;
@@ -856,6 +874,7 @@ static ssize_t pmbus_show_fan_command(struct device *dev,
}
sensor.class = PSC_FAN;
+ sensor.coeffs = fan->coeffs;
if (mode == percent)
sensor.data = fan->command * 255 / 100;
else
@@ -882,6 +901,29 @@ static ssize_t pmbus_show_pwm(struct device *dev,
buf);
}
+static int pmbus_update_fan_coeffs(struct pmbus_data *data,
+ struct pmbus_fan_ctrl *fan,
+ enum pmbus_fan_mode mode)
+{
+ const struct pmbus_driver_info *info = data->info;
+ struct pmbus_sensor *curr = data->sensors;
+
+ fan->coeffs = info->get_fan_coeffs(info, fan->index, mode,
+ PMBUS_FAN_COMMAND_1 + fan->id);
+
+ while (curr) {
+ if (curr->class == PSC_FAN &&
+ curr->attribute.index == fan->index) {
+ curr->coeffs = info->get_fan_coeffs(info, fan->index,
+ mode, curr->reg);
+ }
+
+ curr = curr->next;
+ }
+
+ return 0;
+}
+
static ssize_t pmbus_set_fan_command(struct device *dev,
enum pmbus_fan_mode mode,
struct pmbus_fan_ctrl *fan,
@@ -889,6 +931,7 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
{
struct i2c_client *client = to_i2c_client(dev->parent);
struct pmbus_data *data = i2c_get_clientdata(client);
+ const struct pmbus_driver_info *info = data->info;
int config_addr, command_addr;
struct pmbus_sensor sensor;
ssize_t rv;
@@ -899,7 +942,13 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
mutex_lock(&data->update_lock);
+ if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
+ pmbus_update_fan_coeffs(data, fan, mode);
+ sensor.coeffs = fan->coeffs;
+ }
+
sensor.class = PSC_FAN;
+ sensor.attribute.index = fan->index;
val = pmbus_data2reg(data, &sensor, val);
@@ -968,6 +1017,8 @@ static ssize_t pmbus_show_pwm_enable(struct device *dev,
struct pmbus_sensor sensor = {
.class = PSC_FAN,
.data = fan->command,
+ .attribute.index = fan->index,
+ .coeffs = fan->coeffs,
};
long command;
@@ -992,6 +1043,7 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
struct i2c_client *client = to_i2c_client(dev->parent);
struct pmbus_data *data = i2c_get_clientdata(client);
+ const struct pmbus_driver_info *info = data->info;
int config_addr, command_addr;
struct pmbus_sensor sensor;
ssize_t rv = count;
@@ -1003,15 +1055,21 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
mutex_lock(&data->update_lock);
sensor.class = PSC_FAN;
+ sensor.attribute.index = fan->index;
+
+ if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
+ pmbus_update_fan_coeffs(data, fan, percent);
+ sensor.coeffs = fan->coeffs;
+ }
config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
command_addr = config_addr + 1 + (fan->id & 1);
- if (data->info->set_pwm_mode) {
+ if (info->set_pwm_mode) {
u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
u16 command = fan->command;
- rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
+ rv = info->set_pwm_mode(fan->id, mode, &config, &command);
if (rv < 0)
goto done;
@@ -1138,7 +1196,7 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
sensor = devm_kzalloc(data->dev, sizeof(*sensor), GFP_KERNEL);
if (!sensor)
return NULL;
- a = &sensor->attribute;
+ a = &sensor->attribute.dev_attr;
snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
name, seq, type);
@@ -1146,9 +1204,9 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
sensor->reg = reg;
sensor->class = class;
sensor->update = update;
- pmbus_dev_attr_init(a, sensor->name,
+ pmbus_attr_init(&sensor->attribute, sensor->name,
readonly ? S_IRUGO : S_IRUGO | S_IWUSR,
- pmbus_show_sensor, pmbus_set_sensor);
+ pmbus_show_sensor, pmbus_set_sensor, seq);
if (pmbus_add_attribute(data, &a->attr))
return NULL;
@@ -1886,7 +1944,7 @@ 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)
+ u8 config, const struct pmbus_coeffs *coeffs)
{
struct pmbus_fan_ctrl *fan;
int rv;
@@ -1898,6 +1956,7 @@ static int pmbus_add_fan_ctrl(struct i2c_client *client,
fan->index = index;
fan->page = page;
fan->id = id;
+ fan->coeffs = coeffs;
fan->config = config;
rv = _pmbus_read_word_data(client, page,
@@ -1921,6 +1980,8 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
struct pmbus_data *data)
{
const struct pmbus_driver_info *info = data->info;
+ const struct pmbus_coeffs *coeffs = NULL;
+ enum pmbus_fan_mode mode;
int index = 1;
int page;
int ret;
@@ -1929,6 +1990,7 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
int f;
for (f = 0; f < ARRAY_SIZE(pmbus_fan_registers); f++) {
+ struct pmbus_sensor *sensor;
int regval;
if (!(info->func[page] & pmbus_fan_flags[f]))
@@ -1949,13 +2011,25 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
(!(regval & (PB_FAN_1_INSTALLED >> ((f & 1) * 4)))))
continue;
- if (pmbus_add_sensor(data, "fan", "input", index,
- page, pmbus_fan_registers[f],
- PSC_FAN, true, true) == NULL)
+ sensor = pmbus_add_sensor(data, "fan", "input", index,
+ page, pmbus_fan_registers[f],
+ PSC_FAN, true, true);
+ if (!sensor)
return -ENOMEM;
+ /* Add coeffs here as we have access to the fan mode */
+ if (info->format[PSC_FAN] == direct &&
+ info->get_fan_coeffs) {
+ const u16 mask = PB_FAN_1_RPM >> ((f & 1) * 4);
+
+ mode = (regval & mask) ? rpm : percent;
+ coeffs = info->get_fan_coeffs(info, index, mode,
+ pmbus_fan_registers[f]);
+ sensor->coeffs = coeffs;
+ }
+
ret = pmbus_add_fan_ctrl(client, data, index, page, f,
- regval);
+ regval, coeffs);
if (ret < 0)
return ret;
--
2.11.0
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [RFC PATCH 4/4] pmbus: Add MAX31785 driver
2017-07-10 13:56 [RFC PATCH 0/4] pmbus: Expand fan support and add MAX31785 driver Andrew Jeffery
` (2 preceding siblings ...)
2017-07-10 13:56 ` [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values Andrew Jeffery
@ 2017-07-10 13:56 ` Andrew Jeffery
3 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-10 13:56 UTC (permalink / raw)
To: linux, linux-hwmon
Cc: Andrew Jeffery, jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
drivers/hwmon/pmbus/Kconfig | 10 ++
drivers/hwmon/pmbus/Makefile | 1 +
drivers/hwmon/pmbus/max31785.c | 201 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 212 insertions(+)
create mode 100644 drivers/hwmon/pmbus/max31785.c
diff --git a/drivers/hwmon/pmbus/Kconfig b/drivers/hwmon/pmbus/Kconfig
index cad1229b7e17..5f2f3c6c7499 100644
--- a/drivers/hwmon/pmbus/Kconfig
+++ b/drivers/hwmon/pmbus/Kconfig
@@ -95,6 +95,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 24ff7ee7f8bb..acba1be0d9ad 100644
--- a/drivers/hwmon/pmbus/Makefile
+++ b/drivers/hwmon/pmbus/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_SENSORS_PMBUS) += pmbus.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..8432ed5a0ad6
--- /dev/null
+++ b/drivers/hwmon/pmbus/max31785.c
@@ -0,0 +1,201 @@
+/*
+ * 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 {
+ PMBUS_MFR_FAN_CONFIG = 0xF1,
+ PMBUS_MFR_READ_FAN_PWM = 0xF3,
+ PMBUS_MFR_FAN_FAULT_LIMIT = 0xF5,
+ PMBUS_MFR_FAN_WARN_LIMIT = 0xF6,
+ PMBUS_MFR_FAN_PWM_AVG = 0xF8,
+};
+
+static const struct pmbus_coeffs fan_coeffs[] = {
+ [percent] = { .m = 1, .b = 0, .R = 2 },
+ [rpm] = { .m = 1, .b = 0, .R = 0 },
+};
+
+static const struct pmbus_coeffs *max31785_get_fan_coeffs(
+ const struct pmbus_driver_info *info, int index,
+ enum pmbus_fan_mode mode, int command)
+{
+ switch (command) {
+ case PMBUS_FAN_COMMAND_1:
+ case PMBUS_MFR_FAN_FAULT_LIMIT:
+ case PMBUS_MFR_FAN_WARN_LIMIT:
+ return &fan_coeffs[mode];
+ case PMBUS_READ_FAN_SPEED_1:
+ return &fan_coeffs[rpm];
+ case PMBUS_MFR_FAN_PWM_AVG:
+ return &fan_coeffs[percent];
+ default:
+ break;
+ }
+
+ return NULL;
+}
+
+static int max31785_get_pwm_mode(int id, u8 fan_config, u16 fan_command)
+{
+ enum pmbus_fan_mode mode;
+
+ /* MAX31785 only supports fan 1 on the fan pages */
+ if (WARN_ON(id > 0))
+ return -ENODEV;
+
+ mode = (fan_config & PB_FAN_1_RPM) ? rpm : percent;
+
+ switch (mode) {
+ case percent:
+ if (fan_command >= 0x8000)
+ return 2;
+ else if (fan_command >= 0x2711)
+ return 0;
+ else
+ return 1;
+ case rpm:
+ if (fan_command >= 0x8000)
+ return 2;
+ else
+ return 1;
+ break;
+ }
+
+ return 0;
+}
+
+static const int max31785_pwm_modes[] = { 0x7fff, 0x2710, 0xffff };
+
+int max31785_set_pwm_mode(int id, long mode, u8 *fan_config, u16 *fan_command)
+{
+ /* MAX31785 only supports fan 1 on the fan pages */
+ if (WARN_ON(id > 0))
+ return -ENODEV;
+
+ *fan_config &= ~PB_FAN_1_RPM;
+
+ if (mode >= ARRAY_SIZE(max31785_pwm_modes))
+ return -ENOTSUPP;
+
+ *fan_command = max31785_pwm_modes[mode];
+
+ return 0;
+}
+
+static struct pmbus_driver_info max31785_info = {
+ .pages = 23,
+
+ .get_fan_coeffs = max31785_get_fan_coeffs,
+ .get_pwm_mode = max31785_get_pwm_mode,
+ .set_pwm_mode = max31785_set_pwm_mode,
+
+ .format[PSC_FAN] = direct,
+ .func[0] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
+ .func[1] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
+ .func[2] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
+ .func[3] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
+ .func[4] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
+ .func[5] = PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12,
+
+ .format[PSC_TEMPERATURE] = direct,
+ .coeffs[PSC_TEMPERATURE].m = 1,
+ .coeffs[PSC_TEMPERATURE].b = 0,
+ .coeffs[PSC_TEMPERATURE].R = 2,
+ .func[6] = PMBUS_HAVE_STATUS_TEMP,
+ .func[7] = PMBUS_HAVE_STATUS_TEMP,
+ .func[8] = PMBUS_HAVE_STATUS_TEMP,
+ .func[9] = PMBUS_HAVE_STATUS_TEMP,
+ .func[10] = PMBUS_HAVE_STATUS_TEMP,
+ .func[11] = PMBUS_HAVE_STATUS_TEMP,
+ .func[12] = PMBUS_HAVE_STATUS_TEMP,
+ .func[13] = PMBUS_HAVE_STATUS_TEMP,
+ .func[14] = PMBUS_HAVE_STATUS_TEMP,
+ .func[15] = PMBUS_HAVE_STATUS_TEMP,
+ .func[16] = PMBUS_HAVE_STATUS_TEMP,
+
+ .format[PSC_VOLTAGE_OUT] = direct,
+ .coeffs[PSC_VOLTAGE_OUT].m = 1,
+ .coeffs[PSC_VOLTAGE_OUT].b = 0,
+ .coeffs[PSC_VOLTAGE_OUT].R = 0,
+ .func[17] = PMBUS_HAVE_STATUS_VOUT,
+ .func[18] = PMBUS_HAVE_STATUS_VOUT,
+ .func[19] = PMBUS_HAVE_STATUS_VOUT,
+ .func[20] = PMBUS_HAVE_STATUS_VOUT,
+ .func[21] = PMBUS_HAVE_STATUS_VOUT,
+ .func[22] = PMBUS_HAVE_STATUS_VOUT,
+};
+
+#define MFR_FAN_CONFIG_TSFO BIT(9)
+#define MFR_FAN_CONFIG_TACHO BIT(8)
+
+static int max31785_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ int rv;
+ int i;
+
+ rv = pmbus_do_probe(client, id, &max31785_info);
+ if (rv < 0)
+ return rv;
+
+ for (i = 0; i < max31785_info.pages; i++) {
+ int reg;
+
+ if (!(max31785_info.func[i] & (PMBUS_HAVE_FAN12)))
+ continue;
+
+ reg = pmbus_read_word_data(client, i, PMBUS_MFR_FAN_CONFIG);
+ if (reg < 0)
+ continue;
+
+ /*
+ * XXX: Purely for RFC/testing purposes, don't ramp fans on fan
+ * or temperature sensor fault, or a failure to write
+ * FAN_COMMAND_1 inside a 10s window (watchdog mode).
+ *
+ * The TSFO bit controls both ramping on temp sensor failure
+ * AND whether FAN_COMMAND_1 is in watchdog mode.
+ */
+ reg = (reg | MFR_FAN_CONFIG_TSFO | MFR_FAN_CONFIG_TACHO);
+ reg &= 0xffff;
+
+ rv = pmbus_write_word_data(client, i, PMBUS_MFR_FAN_CONFIG,
+ reg);
+ }
+
+ return 0;
+}
+
+static const struct i2c_device_id max31785_id[] = {
+ { "max31785", 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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values
2017-07-10 13:56 ` [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values Andrew Jeffery
@ 2017-07-11 13:31 ` Guenter Roeck
2017-07-12 1:20 ` Andrew Jeffery
0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2017-07-11 13:31 UTC (permalink / raw)
To: Andrew Jeffery, linux-hwmon
Cc: jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> Some PMBus chips, such as the MAX31785, use different coefficients for
> FAN_COMMAND_[1-4] depending on whether the fan is in PWM (percent duty)
> or RPM mode. Add a callback to allow the driver to provide the
> applicable coefficients to avoid imposing on devices that don't have
> this requirement.
>
Why not just introduce another class, such as PSC_PWM ?
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> drivers/hwmon/pmbus/pmbus.h | 18 +++++--
> drivers/hwmon/pmbus/pmbus_core.c | 112 ++++++++++++++++++++++++++++++++-------
> 2 files changed, 108 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 927eabc1b273..338ecc8b25a4 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -345,6 +345,12 @@ enum pmbus_sensor_classes {
> enum pmbus_data_format { linear = 0, direct, vid };
> enum vrm_version { vr11 = 0, vr12 };
>
> +struct pmbus_coeffs {
> + int m; /* mantissa for direct data format */
> + int b; /* offset */
> + int R; /* exponent */
> +};
> +
> struct pmbus_driver_info {
> int pages; /* Total number of pages */
> enum pmbus_data_format format[PSC_NUM_CLASSES];
> @@ -353,9 +359,7 @@ struct pmbus_driver_info {
> * Support one set of coefficients for each sensor type
> * Used for chips providing data in direct mode.
> */
> - int m[PSC_NUM_CLASSES]; /* mantissa for direct data format */
> - int b[PSC_NUM_CLASSES]; /* offset */
> - int R[PSC_NUM_CLASSES]; /* exponent */
> + struct pmbus_coeffs coeffs[PSC_NUM_CLASSES];
>
> u32 func[PMBUS_PAGES]; /* Functionality, per page */
> /*
> @@ -382,6 +386,14 @@ struct pmbus_driver_info {
> int (*identify)(struct i2c_client *client,
> struct pmbus_driver_info *info);
>
> + /*
> + * If a fan's coefficents change over time (e.g. between RPM and PWM
> + * mode), then the driver can provide a function for retrieving the
> + * currently applicable coefficients.
> + */
> + const struct pmbus_coeffs *(*get_fan_coeffs)(
> + const struct pmbus_driver_info *info, int index,
> + enum pmbus_fan_mode mode, int command);
> /* Allow the driver to interpret the fan command value */
> int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 3b0a55bbbd2c..4ff6a1fd5cce 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -58,10 +58,11 @@
> struct pmbus_sensor {
> struct pmbus_sensor *next;
> char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */
> - struct device_attribute attribute;
> + struct sensor_device_attribute attribute;
> u8 page; /* page number */
> u16 reg; /* register */
> enum pmbus_sensor_classes class; /* sensor class */
> + const struct pmbus_coeffs *coeffs;
> bool update; /* runtime sensor update needed */
> int data; /* Sensor data.
> Negative if there was a read error */
> @@ -89,6 +90,7 @@ struct pmbus_fan_ctrl {
> u8 id;
> u8 config;
> u16 command;
> + const struct pmbus_coeffs *coeffs;
> };
> #define to_pmbus_fan_ctrl_attr(_attr) \
> container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
> @@ -511,9 +513,15 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
> long val = (s16) sensor->data;
> long m, b, R;
>
> - m = data->info->m[sensor->class];
> - b = data->info->b[sensor->class];
> - R = data->info->R[sensor->class];
> + if (sensor->coeffs) {
> + m = sensor->coeffs->m;
> + b = sensor->coeffs->b;
> + R = sensor->coeffs->R;
> + } else {
> + m = data->info->coeffs[sensor->class].m;
> + b = data->info->coeffs[sensor->class].b;
> + R = data->info->coeffs[sensor->class].R;
> + }
>
> if (m == 0)
> return 0;
> @@ -663,9 +671,15 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
> {
> long m, b, R;
>
> - m = data->info->m[sensor->class];
> - b = data->info->b[sensor->class];
> - R = data->info->R[sensor->class];
> + if (sensor->coeffs) {
> + m = sensor->coeffs->m;
> + b = sensor->coeffs->b;
> + R = sensor->coeffs->R;
> + } else {
> + m = data->info->coeffs[sensor->class].m;
> + b = data->info->coeffs[sensor->class].b;
> + R = data->info->coeffs[sensor->class].R;
> + }
>
> /* Power is in uW. Adjust R and b. */
> if (sensor->class == PSC_POWER) {
> @@ -796,7 +810,9 @@ static ssize_t pmbus_show_sensor(struct device *dev,
> struct device_attribute *devattr, char *buf)
> {
> struct pmbus_data *data = pmbus_update_device(dev);
> - struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> + struct pmbus_sensor *sensor;
> +
> + sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
>
> if (sensor->data < 0)
> return sensor->data;
> @@ -810,12 +826,14 @@ static ssize_t pmbus_set_sensor(struct device *dev,
> {
> struct i2c_client *client = to_i2c_client(dev->parent);
> struct pmbus_data *data = i2c_get_clientdata(client);
> - struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> + struct pmbus_sensor *sensor;
> ssize_t rv = count;
> long val = 0;
> int ret;
> u16 regval;
>
> + sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
> +
> if (kstrtol(buf, 10, &val) < 0)
> return -EINVAL;
>
> @@ -856,6 +874,7 @@ static ssize_t pmbus_show_fan_command(struct device *dev,
> }
>
> sensor.class = PSC_FAN;
> + sensor.coeffs = fan->coeffs;
> if (mode == percent)
> sensor.data = fan->command * 255 / 100;
> else
> @@ -882,6 +901,29 @@ static ssize_t pmbus_show_pwm(struct device *dev,
> buf);
> }
>
> +static int pmbus_update_fan_coeffs(struct pmbus_data *data,
> + struct pmbus_fan_ctrl *fan,
> + enum pmbus_fan_mode mode)
> +{
> + const struct pmbus_driver_info *info = data->info;
> + struct pmbus_sensor *curr = data->sensors;
> +
> + fan->coeffs = info->get_fan_coeffs(info, fan->index, mode,
> + PMBUS_FAN_COMMAND_1 + fan->id);
> +
> + while (curr) {
> + if (curr->class == PSC_FAN &&
> + curr->attribute.index == fan->index) {
> + curr->coeffs = info->get_fan_coeffs(info, fan->index,
> + mode, curr->reg);
> + }
> +
> + curr = curr->next;
> + }
> +
> + return 0;
> +}
> +
> static ssize_t pmbus_set_fan_command(struct device *dev,
> enum pmbus_fan_mode mode,
> struct pmbus_fan_ctrl *fan,
> @@ -889,6 +931,7 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
> {
> struct i2c_client *client = to_i2c_client(dev->parent);
> struct pmbus_data *data = i2c_get_clientdata(client);
> + const struct pmbus_driver_info *info = data->info;
> int config_addr, command_addr;
> struct pmbus_sensor sensor;
> ssize_t rv;
> @@ -899,7 +942,13 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
>
> mutex_lock(&data->update_lock);
>
> + if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
> + pmbus_update_fan_coeffs(data, fan, mode);
> + sensor.coeffs = fan->coeffs;
> + }
> +
> sensor.class = PSC_FAN;
> + sensor.attribute.index = fan->index;
>
> val = pmbus_data2reg(data, &sensor, val);
>
> @@ -968,6 +1017,8 @@ static ssize_t pmbus_show_pwm_enable(struct device *dev,
> struct pmbus_sensor sensor = {
> .class = PSC_FAN,
> .data = fan->command,
> + .attribute.index = fan->index,
> + .coeffs = fan->coeffs,
> };
> long command;
>
> @@ -992,6 +1043,7 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
> struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> struct i2c_client *client = to_i2c_client(dev->parent);
> struct pmbus_data *data = i2c_get_clientdata(client);
> + const struct pmbus_driver_info *info = data->info;
> int config_addr, command_addr;
> struct pmbus_sensor sensor;
> ssize_t rv = count;
> @@ -1003,15 +1055,21 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
> mutex_lock(&data->update_lock);
>
> sensor.class = PSC_FAN;
> + sensor.attribute.index = fan->index;
> +
> + if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
> + pmbus_update_fan_coeffs(data, fan, percent);
> + sensor.coeffs = fan->coeffs;
> + }
>
> config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> command_addr = config_addr + 1 + (fan->id & 1);
>
> - if (data->info->set_pwm_mode) {
> + if (info->set_pwm_mode) {
> u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> u16 command = fan->command;
>
> - rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> + rv = info->set_pwm_mode(fan->id, mode, &config, &command);
> if (rv < 0)
> goto done;
>
> @@ -1138,7 +1196,7 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> sensor = devm_kzalloc(data->dev, sizeof(*sensor), GFP_KERNEL);
> if (!sensor)
> return NULL;
> - a = &sensor->attribute;
> + a = &sensor->attribute.dev_attr;
>
> snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
> name, seq, type);
> @@ -1146,9 +1204,9 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> sensor->reg = reg;
> sensor->class = class;
> sensor->update = update;
> - pmbus_dev_attr_init(a, sensor->name,
> + pmbus_attr_init(&sensor->attribute, sensor->name,
> readonly ? S_IRUGO : S_IRUGO | S_IWUSR,
> - pmbus_show_sensor, pmbus_set_sensor);
> + pmbus_show_sensor, pmbus_set_sensor, seq);
>
> if (pmbus_add_attribute(data, &a->attr))
> return NULL;
> @@ -1886,7 +1944,7 @@ 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)
> + u8 config, const struct pmbus_coeffs *coeffs)
> {
> struct pmbus_fan_ctrl *fan;
> int rv;
> @@ -1898,6 +1956,7 @@ static int pmbus_add_fan_ctrl(struct i2c_client *client,
> fan->index = index;
> fan->page = page;
> fan->id = id;
> + fan->coeffs = coeffs;
> fan->config = config;
>
> rv = _pmbus_read_word_data(client, page,
> @@ -1921,6 +1980,8 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> struct pmbus_data *data)
> {
> const struct pmbus_driver_info *info = data->info;
> + const struct pmbus_coeffs *coeffs = NULL;
> + enum pmbus_fan_mode mode;
> int index = 1;
> int page;
> int ret;
> @@ -1929,6 +1990,7 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> int f;
>
> for (f = 0; f < ARRAY_SIZE(pmbus_fan_registers); f++) {
> + struct pmbus_sensor *sensor;
> int regval;
>
> if (!(info->func[page] & pmbus_fan_flags[f]))
> @@ -1949,13 +2011,25 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> (!(regval & (PB_FAN_1_INSTALLED >> ((f & 1) * 4)))))
> continue;
>
> - if (pmbus_add_sensor(data, "fan", "input", index,
> - page, pmbus_fan_registers[f],
> - PSC_FAN, true, true) == NULL)
> + sensor = pmbus_add_sensor(data, "fan", "input", index,
> + page, pmbus_fan_registers[f],
> + PSC_FAN, true, true);
> + if (!sensor)
> return -ENOMEM;
>
> + /* Add coeffs here as we have access to the fan mode */
> + if (info->format[PSC_FAN] == direct &&
> + info->get_fan_coeffs) {
> + const u16 mask = PB_FAN_1_RPM >> ((f & 1) * 4);
> +
> + mode = (regval & mask) ? rpm : percent;
> + coeffs = info->get_fan_coeffs(info, index, mode,
> + pmbus_fan_registers[f]);
> + sensor->coeffs = coeffs;
> + }
> +
> ret = pmbus_add_fan_ctrl(client, data, index, page, f,
> - regval);
> + regval, coeffs);
> if (ret < 0)
> return ret;
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
2017-07-10 13:56 ` [RFC PATCH 2/4] pmbus: Add fan configuration support Andrew Jeffery
@ 2017-07-11 13:40 ` Guenter Roeck
2017-07-12 0:39 ` Andrew Jeffery
0 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2017-07-11 13:40 UTC (permalink / raw)
To: Andrew Jeffery, linux-hwmon
Cc: jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> Augment PMBus support to include control of fans via the
> FAN_COMMAND_[1-4] registers, both in RPM and PWM modes. The behaviour
> of FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4] are tightly coupled, and
> their interactions do not fit the existing use of struct pmbus_sensor.
> The patch introduces struct pmbus_fan_ctrl to distinguish from the
> simple sensor case, along with associated sysfs show/set implementations.
>
> Further, the interpreting the value of FAN_COMMAND_[1-4] depends on both
> the current fan mode (RPM or PWM, as configured in
> FAN_CONFIG_{1_2,3_4}), and the device-specific behaviour for the
> register. For example, the MAX31785 chip defines the following:
>
> PWM (m = 1, b = 0, R = 2):
> 0x0000 - 0x2710: 0 - 100% fan PWM duty cycle
> 0x2711 - 0x7fff: 100% fan PWM duty cycle
> 0x8000 - 0xffff: Ignore FAN_COMMAND_[1-4], use automatic fan control
>
> RPM (m = 1, b = 0, R = 0):
> 0x0000 - 0x7FFF: 0 - 32,767 RPM
> 0x8000 - 0xFFFF: Ignore FAN_COMMAND_[1-4], use automatic fan control
>
> To handle the device-specific interpretation of the FAN_COMMAND_[1-4],
> add an optional callbacks to the info struct to get/set the 'mode'
> value required for the pwm[1-n]_enable sysfs attribute. A fallback
> calculation exists if the callbacks are not populated; the fallback
> ignores device-specific ranges and tries to determine a reasonable value
> from FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4].
>
This seems overly complex, but unfortunately I don't have time for a detailed
analysis right now. Couple of comments below.
Guenter
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
> drivers/hwmon/pmbus/pmbus.h | 7 +
> drivers/hwmon/pmbus/pmbus_core.c | 335 +++++++++++++++++++++++++++++++++++++++
> 2 files changed, 342 insertions(+)
>
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index bfcb13bae34b..927eabc1b273 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -223,6 +223,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)
> */
> @@ -380,6 +382,11 @@ struct pmbus_driver_info {
> int (*identify)(struct i2c_client *client,
> struct pmbus_driver_info *info);
>
> + /* Allow the driver to interpret the fan command value */
> + int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> + int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> + u16 *fan_command);
> +
It is not entirely obvious to me why this would require new callback functions.
Can you overload PMBUS_FAN_CONFIG_12 / PMBUS_FAN_CONFIG_34 or, if that does not
work for some reason, introduce a virtual register, such as PMBUS_VIRT_PWM_MODE ?
> /* Regulator functionality, if supported by this chip driver. */
> int num_regulators;
> const struct regulator_desc *reg_desc;
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index ba59eaef2e07..3b0a55bbbd2c 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -69,6 +69,38 @@ struct pmbus_sensor {
> #define to_pmbus_sensor(_attr) \
> container_of(_attr, struct pmbus_sensor, attribute)
>
> +#define PB_FAN_CONFIG_RPM PB_FAN_2_RPM
> +#define PB_FAN_CONFIG_INSTALLED PB_FAN_2_INSTALLEDBUS_VIRT_
Something seems odd here. PB_FAN_2_INSTALLEDBUS_VIRT_ ?
> +#define PB_FAN_CONFIG_MASK(i) (0xff << (4 * !((i) & 1)))
> +#define PB_FAN_CONFIG_GET(i, n) (((n) >> (4 * !((i) & 1))) & 0xff)
> +#define PB_FAN_CONFIG_PUT(i, n) (((n) & 0xff) << (4 * !((i) & 1)))
> +
Aren't there standard bit manipulation macros for that ? Either case, this is just to avoid
having to use the existing defines. Ok, but then I think it would make more sense to
make it generic, ie change the core to not use PB_FAN_2_RPM / PB_FAN_1_RPM etc.
but PB_FAN_RPM(index) everywhere.
> +struct pmbus_fan_ctrl_attr {
> + struct device_attribute attribute;
> + char name[PMBUS_NAME_SIZE];
> +};
> +
> +struct pmbus_fan_ctrl {
> + struct pmbus_fan_ctrl_attr fan_target;
> + struct pmbus_fan_ctrl_attr pwm;
> + struct pmbus_fan_ctrl_attr pwm_enable;
> + int index;
> + u8 page;
> + u8 id;
> + u8 config;
> + u16 command;
> +};
> +#define to_pmbus_fan_ctrl_attr(_attr) \
> + container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
> +#define fan_target_to_pmbus_fan_ctrl(_attr) \
> + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
> + fan_target)
> +#define pwm_to_pmbus_fan_ctrl(_attr) \
> + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, pwm)
> +#define pwm_enable_to_pmbus_fan_ctrl(_attr) \
> + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
> + pwm_enable)
> +
> struct pmbus_boolean {
> char name[PMBUS_NAME_SIZE]; /* sysfs boolean name */
> struct sensor_device_attribute attribute;
> @@ -806,6 +838,219 @@ static ssize_t pmbus_show_label(struct device *dev,
> return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
> }
>
> +static ssize_t pmbus_show_fan_command(struct device *dev,
> + enum pmbus_fan_mode mode,
> + struct pmbus_fan_ctrl *fan, char *buf)
> +{
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct pmbus_data *data = i2c_get_clientdata(client);
> + struct pmbus_sensor sensor;
> + long val;
> +
> + mutex_lock(&data->update_lock);
> +
> + if ((mode == percent && (fan->config & PB_FAN_CONFIG_RPM)) ||
> + (mode == rpm && !(fan->config & PB_FAN_CONFIG_RPM))) {
> + mutex_unlock(&data->update_lock);
> + return -ENOTSUPP; /* XXX: This seems dodgy, but what to do? */
Not create the attribute in question in the first place, or return 0. The above
messes up the 'sensors' command.
> + }
> +
> + sensor.class = PSC_FAN;
> + if (mode == percent)
> + sensor.data = fan->command * 255 / 100;
> + else
> + sensor.data = fan->command;
> +
> + val = pmbus_reg2data(data, &sensor);
> +
> + mutex_unlock(&data->update_lock);
> +
> + return snprintf(buf, PAGE_SIZE, "%ld\n", val);
> +}
> +
> +static ssize_t pmbus_show_fan_target(struct device *dev,
> + struct device_attribute *da, char *buf)
> +{
> + return pmbus_show_fan_command(dev, rpm,
> + fan_target_to_pmbus_fan_ctrl(da), buf);
> +}
> +
> +static ssize_t pmbus_show_pwm(struct device *dev,
> + struct device_attribute *da, char *buf)
> +{
> + return pmbus_show_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> + buf);
> +}
> +
> +static ssize_t pmbus_set_fan_command(struct device *dev,
> + enum pmbus_fan_mode mode,
> + struct pmbus_fan_ctrl *fan,
> + const char *buf, ssize_t count)
> +{
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct pmbus_data *data = i2c_get_clientdata(client);
> + int config_addr, command_addr;
> + struct pmbus_sensor sensor;
> + ssize_t rv;
> + long val;
> +
> + if (kstrtol(buf, 10, &val) < 0)
> + return -EINVAL;
> +
> + mutex_lock(&data->update_lock);
> +
> + sensor.class = PSC_FAN;
> +
> + val = pmbus_data2reg(data, &sensor, val);
> +
> + if (mode == percent)
> + val = val * 100 / 255;
> +
> + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> + command_addr = config_addr + 1 + (fan->id & 1);
> +
> + if (mode == rpm)
> + fan->config |= PB_FAN_CONFIG_RPM;
> + else
> + fan->config &= ~PB_FAN_CONFIG_RPM;
> +
> + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> + PB_FAN_CONFIG_PUT(fan->id, fan->config),
> + PB_FAN_CONFIG_MASK(fan->id));
> + if (rv < 0)
> + goto done;
> +
> + fan->command = val;
> + rv = pmbus_write_word_data(client, fan->page, command_addr,
> + fan->command);
> +
> +done:
> + mutex_unlock(&data->update_lock);
> +
> + if (rv < 0)
> + return rv;
> +
> + return count;
> +}
> +
> +static ssize_t pmbus_set_fan_target(struct device *dev,
> + struct device_attribute *da,
> + const char *buf, size_t count)
> +{
> + return pmbus_set_fan_command(dev, rpm,
> + fan_target_to_pmbus_fan_ctrl(da), buf,
> + count);
> +}
> +
> +static ssize_t pmbus_set_pwm(struct device *dev, struct device_attribute *da,
> + const char *buf, size_t count)
> +{
> + return pmbus_set_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> + buf, count);
> +}
> +
> +static ssize_t pmbus_show_pwm_enable(struct device *dev,
> + struct device_attribute *da, char *buf)
> +{
> + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct pmbus_data *data = i2c_get_clientdata(client);
> + long mode;
> +
> + mutex_lock(&data->update_lock);
> +
> +
> + if (data->info->get_pwm_mode) {
> + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> +
> + mode = data->info->get_pwm_mode(fan->id, config, fan->command);
> + } else {
> + struct pmbus_sensor sensor = {
> + .class = PSC_FAN,
> + .data = fan->command,
> + };
> + long command;
> +
> + command = pmbus_reg2data(data, &sensor);
> +
> + /* XXX: Need to do something sensible */
> + if (fan->config & PB_FAN_CONFIG_RPM)
> + mode = 2;
> + else
> + mode = (command >= 0 && command < 100);
> + }
> +
> + mutex_unlock(&data->update_lock);
> +
> + return snprintf(buf, PAGE_SIZE, "%ld\n", mode);
> +}
> +
> +static ssize_t pmbus_set_pwm_enable(struct device *dev,
> + struct device_attribute *da,
> + const char *buf, size_t count)
> +{
> + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> + struct i2c_client *client = to_i2c_client(dev->parent);
> + struct pmbus_data *data = i2c_get_clientdata(client);
> + int config_addr, command_addr;
> + struct pmbus_sensor sensor;
> + ssize_t rv = count;
> + long mode;
> +
> + if (kstrtol(buf, 10, &mode) < 0)
> + return -EINVAL;
> +
> + mutex_lock(&data->update_lock);
> +
> + sensor.class = PSC_FAN;
> +
> + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> + command_addr = config_addr + 1 + (fan->id & 1);
> +
> + if (data->info->set_pwm_mode) {
> + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> + u16 command = fan->command;
> +
> + rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> + if (rv < 0)
> + goto done;
> +
> + fan->config = PB_FAN_CONFIG_GET(fan->id, config);
> + fan->command = command;
> + } else {
> + fan->config &= ~PB_FAN_CONFIG_RPM;
> + switch (mode) {
> + case 0:
> + case 1:
> + /* XXX: Safe at least? */
> + fan->command = pmbus_data2reg(data, &sensor, 100);
> + break;
> + case 2:
> + default:
> + /* XXX: Safe at least? */
> + fan->command = 0xffff;
> + break;
> + }
> + }
> +
> + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> + PB_FAN_CONFIG_PUT(fan->id, fan->config),
> + PB_FAN_CONFIG_MASK(fan->id));
> + if (rv < 0)
> + goto done;
> +
> + rv = pmbus_write_word_data(client, fan->page, command_addr,
> + fan->command);
> +
> +done:
> + mutex_unlock(&data->update_lock);
> +
> + if (rv < 0)
> + return rv;
> +
> + return count;
> +}
> +
> static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
> {
> if (data->num_attributes >= data->max_attributes - 1) {
> @@ -1094,6 +1339,51 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> return 0;
> }
>
> +static int pmbus_add_fan_ctrl_attr(struct pmbus_data *data,
> + struct pmbus_fan_ctrl_attr *fa,
> + const char *name_fmt,
> + ssize_t (*show)(struct device *dev,
> + struct device_attribute *attr,
> + char *buf),
> + ssize_t (*store)(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count),
> + int idx)
> +{
> + struct device_attribute *da;
> +
> + da = &fa->attribute;
> +
> + snprintf(fa->name, sizeof(fa->name), name_fmt, idx);
> + pmbus_dev_attr_init(da, fa->name, 0644, show, store);
> +
> + return pmbus_add_attribute(data, &da->attr);
> +}
> +
> +static inline int pmbus_add_fan_target_attr(struct pmbus_data *data,
> + struct pmbus_fan_ctrl *fan)
> +{
> + return pmbus_add_fan_ctrl_attr(data, &fan->fan_target, "fan%d_target",
> + pmbus_show_fan_target,
> + pmbus_set_fan_target, fan->index);
> +}
> +
> +static inline int pmbus_add_pwm_attr(struct pmbus_data *data,
> + struct pmbus_fan_ctrl *fan)
> +{
> +
> + return pmbus_add_fan_ctrl_attr(data, &fan->pwm, "pwm%d", pmbus_show_pwm,
> + pmbus_set_pwm, fan->index);
> +}
> +
> +static inline int pmbus_add_pwm_enable_attr(struct pmbus_data *data,
> + struct pmbus_fan_ctrl *fan)
> +{
> + return pmbus_add_fan_ctrl_attr(data, &fan->pwm_enable, "pwm%d_enable",
> + pmbus_show_pwm_enable,
> + pmbus_set_pwm_enable, fan->index);
> +}
> +
> static const struct pmbus_limit_attr vin_limit_attrs[] = {
> {
> .reg = PMBUS_VIN_UV_WARN_LIMIT,
> @@ -1565,6 +1855,13 @@ static const int pmbus_fan_config_registers[] = {
> 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,
> +};
> +
> static const int pmbus_fan_status_registers[] = {
> PMBUS_STATUS_FAN_12,
> PMBUS_STATUS_FAN_12,
> @@ -1587,6 +1884,39 @@ 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_fan_ctrl *fan;
> + int rv;
> +
> + fan = devm_kzalloc(data->dev, sizeof(*fan), GFP_KERNEL);
> + if (!fan)
> + return -ENOMEM;
> +
> + fan->index = index;
> + fan->page = page;
> + fan->id = id;
> + fan->config = config;
> +
> + rv = _pmbus_read_word_data(client, page,
> + pmbus_fan_command_registers[id]);
> + if (rv < 0)
> + return rv;
> + fan->command = rv;
> +
> + rv = pmbus_add_fan_target_attr(data, fan);
> + if (rv < 0)
> + return rv;
> +
> + rv = pmbus_add_pwm_attr(data, fan);
> + if (rv < 0)
> + return rv;
> +
> + return pmbus_add_pwm_enable_attr(data, fan);
> +}
> +
> static int pmbus_add_fan_attributes(struct i2c_client *client,
> struct pmbus_data *data)
> {
> @@ -1624,6 +1954,11 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> PSC_FAN, true, true) == NULL)
> return -ENOMEM;
>
> + 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.
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
2017-07-11 13:40 ` Guenter Roeck
@ 2017-07-12 0:39 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-12 0:39 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon
Cc: jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
[-- Attachment #1: Type: text/plain, Size: 19101 bytes --]
On Tue, 2017-07-11 at 06:40 -0700, Guenter Roeck wrote:
> On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> > Augment PMBus support to include control of fans via the
> > FAN_COMMAND_[1-4] registers, both in RPM and PWM modes. The behaviour
> > of FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4] are tightly coupled, and
> > their interactions do not fit the existing use of struct pmbus_sensor.
> > The patch introduces struct pmbus_fan_ctrl to distinguish from the
> > simple sensor case, along with associated sysfs show/set implementations.
> >
> > Further, the interpreting the value of FAN_COMMAND_[1-4] depends on both
> > the current fan mode (RPM or PWM, as configured in
> > FAN_CONFIG_{1_2,3_4}), and the device-specific behaviour for the
> > register. For example, the MAX31785 chip defines the following:
> >
> > PWM (m = 1, b = 0, R = 2):
> > 0x0000 - 0x2710: 0 - 100% fan PWM duty cycle
> > 0x2711 - 0x7fff: 100% fan PWM duty cycle
> > 0x8000 - 0xffff: Ignore FAN_COMMAND_[1-4], use automatic fan control
> >
> > RPM (m = 1, b = 0, R = 0):
> > 0x0000 - 0x7FFF: 0 - 32,767 RPM
> > 0x8000 - 0xFFFF: Ignore FAN_COMMAND_[1-4], use automatic fan control
> >
> > To handle the device-specific interpretation of the FAN_COMMAND_[1-4],
> > add an optional callbacks to the info struct to get/set the 'mode'
> > value required for the pwm[1-n]_enable sysfs attribute. A fallback
> > calculation exists if the callbacks are not populated; the fallback
> > ignores device-specific ranges and tries to determine a reasonable value
> > from FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4].
> >
>
> This seems overly complex, but unfortunately I don't have time for a detailed
> analysis right now.
No worries. It turned out more complex than I was hoping as well, and I
am keen to hear any insights to trim it down.
> Couple of comments below.
Yep, thanks for taking a look.
>
> Guenter
>
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > drivers/hwmon/pmbus/pmbus.h | 7 +
> > drivers/hwmon/pmbus/pmbus_core.c | 335 +++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 342 insertions(+)
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index bfcb13bae34b..927eabc1b273 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -223,6 +223,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)
> > */
> > @@ -380,6 +382,11 @@ struct pmbus_driver_info {
> > > > int (*identify)(struct i2c_client *client,
> > > > struct pmbus_driver_info *info);
> >
> > > > + /* Allow the driver to interpret the fan command value */
> > > > + int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> > > > + int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> > > > + u16 *fan_command);
> > +
>
> It is not entirely obvious to me why this would require new callback functions.
> Can you overload PMBUS_FAN_CONFIG_12 / PMBUS_FAN_CONFIG_34 or, if that does not
> work for some reason, introduce a virtual register, such as PMBUS_VIRT_PWM_MODE ?
Can you expand on the thought of overloading PMBUS_FAN_CONFIG_{12,34}?
Regarding virtual registers, I saw references to them whilst I was
working my way through the core code but didn't stop to investigate.
I'll take a deeper look.
However, the addition of the callbacks was driven by the behaviour of
the MAX31785, where some values written to PMBUS_FAN_COMMAND_1 trigger
automated control, while others retain manual control. Patch 4/4 should
provide a bit more context, though I've also outlined the behaviour in
the commit message for this patch. I don't have a lot of experience
with PMBus devices so I don't have a good idea if there's a better way
to capture the behaviour that isn't so unconstrained in its approach.
>
> > > > /* Regulator functionality, if supported by this chip driver. */
> > > > int num_regulators;
> > > > const struct regulator_desc *reg_desc;
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index ba59eaef2e07..3b0a55bbbd2c 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -69,6 +69,38 @@ struct pmbus_sensor {
> > #define to_pmbus_sensor(_attr) \
> > > > container_of(_attr, struct pmbus_sensor, attribute)
> >
> > > > +#define PB_FAN_CONFIG_RPM PB_FAN_2_RPM
> > +#define PB_FAN_CONFIG_INSTALLED PB_FAN_2_INSTALLEDBUS_VIRT_
>
> Something seems odd here. PB_FAN_2_INSTALLEDBUS_VIRT_ ?
Yes, that's busted. Not sure what went wrong, but I'll clean it up.
>
> > > > +#define PB_FAN_CONFIG_MASK(i) (0xff << (4 * !((i) & 1)))
> > > > +#define PB_FAN_CONFIG_GET(i, n) (((n) >> (4 * !((i) & 1))) & 0xff)
> > > > +#define PB_FAN_CONFIG_PUT(i, n) (((n) & 0xff) << (4 * !((i) & 1)))
> > +
>
> Aren't there standard bit manipulation macros for that ? Either case, this is just to avoid
> having to use the existing defines.
As I store the configuration for each fan in a struct pmbus_fan_ctrl
dedicated to the fan, I reasoned that intermediate code should not have
to deal with the details of which nibble to access with respect to the
fan's (per-page) ID. Rather, code reading or writing
PMBUS_FAN_COMMAND_[1-4] should deal with ensuring the correct values
are provided.
> Ok, but then I think it would make more sense to
> make it generic, ie change the core to not use PB_FAN_2_RPM / PB_FAN_1_RPM etc.
> but PB_FAN_RPM(index) everywhere.
I'll make the change throughout pmbus core.
>
> > +struct pmbus_fan_ctrl_attr {
> > > > + struct device_attribute attribute;
> > > > + char name[PMBUS_NAME_SIZE];
> > +};
> > +
> > +struct pmbus_fan_ctrl {
> > > > + struct pmbus_fan_ctrl_attr fan_target;
> > > > + struct pmbus_fan_ctrl_attr pwm;
> > > > + struct pmbus_fan_ctrl_attr pwm_enable;
> > > > + int index;
> > > > + u8 page;
> > > > + u8 id;
> > > > + u8 config;
> > > > + u16 command;
> > +};
> > +#define to_pmbus_fan_ctrl_attr(_attr) \
> > > > + container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
> > +#define fan_target_to_pmbus_fan_ctrl(_attr) \
> > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
> > > > + fan_target)
> > +#define pwm_to_pmbus_fan_ctrl(_attr) \
> > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, pwm)
> > +#define pwm_enable_to_pmbus_fan_ctrl(_attr) \
> > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
> > > > + pwm_enable)
> > +
> > struct pmbus_boolean {
> > > > > > char name[PMBUS_NAME_SIZE]; /* sysfs boolean name */
> > > > struct sensor_device_attribute attribute;
> > @@ -806,6 +838,219 @@ static ssize_t pmbus_show_label(struct device *dev,
> > > > return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
> > }
> >
> > +static ssize_t pmbus_show_fan_command(struct device *dev,
> > > > + enum pmbus_fan_mode mode,
> > > > + struct pmbus_fan_ctrl *fan, char *buf)
> > +{
> > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + struct pmbus_sensor sensor;
> > > > + long val;
> > +
> > > > + mutex_lock(&data->update_lock);
> > +
> > > > + if ((mode == percent && (fan->config & PB_FAN_CONFIG_RPM)) ||
> > > > + (mode == rpm && !(fan->config & PB_FAN_CONFIG_RPM))) {
> > > > + mutex_unlock(&data->update_lock);
> > + return -ENOTSUPP; /* XXX: This seems dodgy, but what to do? */
>
> Not create the attribute in question in the first place, or return 0. The above
> messes up the 'sensors' command.
I think returning 0 is the only valid option of the two, given that we
can dynamically switch between RPM and PWM modes.
Thanks for the feedback.
Andrew
>
> > > > + }
> > +
> > > > + sensor.class = PSC_FAN;
> > > > + if (mode == percent)
> > > > + sensor.data = fan->command * 255 / 100;
> > > > + else
> > > > + sensor.data = fan->command;
> > +
> > > > + val = pmbus_reg2data(data, &sensor);
> > +
> > > > + mutex_unlock(&data->update_lock);
> > +
> > > > + return snprintf(buf, PAGE_SIZE, "%ld\n", val);
> > +}
> > +
> > +static ssize_t pmbus_show_fan_target(struct device *dev,
> > > > + struct device_attribute *da, char *buf)
> > +{
> > > > + return pmbus_show_fan_command(dev, rpm,
> > > > + fan_target_to_pmbus_fan_ctrl(da), buf);
> > +}
> > +
> > +static ssize_t pmbus_show_pwm(struct device *dev,
> > > > + struct device_attribute *da, char *buf)
> > +{
> > > > + return pmbus_show_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> > > > + buf);
> > +}
> > +
> > +static ssize_t pmbus_set_fan_command(struct device *dev,
> > > > + enum pmbus_fan_mode mode,
> > > > + struct pmbus_fan_ctrl *fan,
> > > > + const char *buf, ssize_t count)
> > +{
> > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + int config_addr, command_addr;
> > > > + struct pmbus_sensor sensor;
> > > > + ssize_t rv;
> > > > + long val;
> > +
> > > > + if (kstrtol(buf, 10, &val) < 0)
> > > > + return -EINVAL;
> > +
> > > > + mutex_lock(&data->update_lock);
> > +
> > > > + sensor.class = PSC_FAN;
> > +
> > > > + val = pmbus_data2reg(data, &sensor, val);
> > +
> > > > + if (mode == percent)
> > > > + val = val * 100 / 255;
> > +
> > > > + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > + command_addr = config_addr + 1 + (fan->id & 1);
> > +
> > > > + if (mode == rpm)
> > > > + fan->config |= PB_FAN_CONFIG_RPM;
> > > > + else
> > > > + fan->config &= ~PB_FAN_CONFIG_RPM;
> > +
> > > > + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> > > > + PB_FAN_CONFIG_PUT(fan->id, fan->config),
> > > > + PB_FAN_CONFIG_MASK(fan->id));
> > > > + if (rv < 0)
> > > > + goto done;
> > +
> > > > + fan->command = val;
> > > > + rv = pmbus_write_word_data(client, fan->page, command_addr,
> > > > + fan->command);
> > +
> > +done:
> > > > + mutex_unlock(&data->update_lock);
> > +
> > > > + if (rv < 0)
> > > > + return rv;
> > +
> > > > + return count;
> > +}
> > +
> > +static ssize_t pmbus_set_fan_target(struct device *dev,
> > > > + struct device_attribute *da,
> > > > + const char *buf, size_t count)
> > +{
> > > > + return pmbus_set_fan_command(dev, rpm,
> > > > + fan_target_to_pmbus_fan_ctrl(da), buf,
> > > > + count);
> > +}
> > +
> > +static ssize_t pmbus_set_pwm(struct device *dev, struct device_attribute *da,
> > > > + const char *buf, size_t count)
> > +{
> > > > + return pmbus_set_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> > > > + buf, count);
> > +}
> > +
> > +static ssize_t pmbus_show_pwm_enable(struct device *dev,
> > > > + struct device_attribute *da, char *buf)
> > +{
> > > > + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + long mode;
> > +
> > > > + mutex_lock(&data->update_lock);
> > +
> > +
> > > > + if (data->info->get_pwm_mode) {
> > > > + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > +
> > > > + mode = data->info->get_pwm_mode(fan->id, config, fan->command);
> > > > + } else {
> > > > + struct pmbus_sensor sensor = {
> > > > + .class = PSC_FAN,
> > > > + .data = fan->command,
> > > > + };
> > > > + long command;
> > +
> > > > + command = pmbus_reg2data(data, &sensor);
> > +
> > > > + /* XXX: Need to do something sensible */
> > > > + if (fan->config & PB_FAN_CONFIG_RPM)
> > > > + mode = 2;
> > > > + else
> > > > + mode = (command >= 0 && command < 100);
> > > > + }
> > +
> > > > + mutex_unlock(&data->update_lock);
> > +
> > > > + return snprintf(buf, PAGE_SIZE, "%ld\n", mode);
> > +}
> > +
> > +static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > + struct device_attribute *da,
> > > > + const char *buf, size_t count)
> > +{
> > > > + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + int config_addr, command_addr;
> > > > + struct pmbus_sensor sensor;
> > > > + ssize_t rv = count;
> > > > + long mode;
> > +
> > > > + if (kstrtol(buf, 10, &mode) < 0)
> > > > + return -EINVAL;
> > +
> > > > + mutex_lock(&data->update_lock);
> > +
> > > > + sensor.class = PSC_FAN;
> > +
> > > > + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > + command_addr = config_addr + 1 + (fan->id & 1);
> > +
> > > > + if (data->info->set_pwm_mode) {
> > > > + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > > + u16 command = fan->command;
> > +
> > > > + rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > + if (rv < 0)
> > > > + goto done;
> > +
> > > > + fan->config = PB_FAN_CONFIG_GET(fan->id, config);
> > > > + fan->command = command;
> > > > + } else {
> > > > + fan->config &= ~PB_FAN_CONFIG_RPM;
> > > > + switch (mode) {
> > > > + case 0:
> > > > + case 1:
> > > > + /* XXX: Safe at least? */
> > > > + fan->command = pmbus_data2reg(data, &sensor, 100);
> > > > + break;
> > > > + case 2:
> > > > + default:
> > > > + /* XXX: Safe at least? */
> > > > + fan->command = 0xffff;
> > > > + break;
> > > > + }
> > > > + }
> > +
> > > > + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> > > > + PB_FAN_CONFIG_PUT(fan->id, fan->config),
> > > > + PB_FAN_CONFIG_MASK(fan->id));
> > > > + if (rv < 0)
> > > > + goto done;
> > +
> > > > + rv = pmbus_write_word_data(client, fan->page, command_addr,
> > > > + fan->command);
> > +
> > +done:
> > > > + mutex_unlock(&data->update_lock);
> > +
> > > > + if (rv < 0)
> > > > + return rv;
> > +
> > > > + return count;
> > +}
> > +
> > static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
> > {
> > > > if (data->num_attributes >= data->max_attributes - 1) {
> > @@ -1094,6 +1339,51 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> > > > return 0;
> > }
> >
> > +static int pmbus_add_fan_ctrl_attr(struct pmbus_data *data,
> > > > + struct pmbus_fan_ctrl_attr *fa,
> > > > + const char *name_fmt,
> > > > + ssize_t (*show)(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + char *buf),
> > > > + ssize_t (*store)(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + const char *buf, size_t count),
> > > > + int idx)
> > +{
> > > > + struct device_attribute *da;
> > +
> > > > + da = &fa->attribute;
> > +
> > > > + snprintf(fa->name, sizeof(fa->name), name_fmt, idx);
> > > > + pmbus_dev_attr_init(da, fa->name, 0644, show, store);
> > +
> > > > + return pmbus_add_attribute(data, &da->attr);
> > +}
> > +
> > +static inline int pmbus_add_fan_target_attr(struct pmbus_data *data,
> > > > + struct pmbus_fan_ctrl *fan)
> > +{
> > > > + return pmbus_add_fan_ctrl_attr(data, &fan->fan_target, "fan%d_target",
> > > > + pmbus_show_fan_target,
> > > > + pmbus_set_fan_target, fan->index);
> > +}
> > +
> > +static inline int pmbus_add_pwm_attr(struct pmbus_data *data,
> > > > + struct pmbus_fan_ctrl *fan)
> > +{
> > +
> > > > + return pmbus_add_fan_ctrl_attr(data, &fan->pwm, "pwm%d", pmbus_show_pwm,
> > > > + pmbus_set_pwm, fan->index);
> > +}
> > +
> > +static inline int pmbus_add_pwm_enable_attr(struct pmbus_data *data,
> > > > + struct pmbus_fan_ctrl *fan)
> > +{
> > > > + return pmbus_add_fan_ctrl_attr(data, &fan->pwm_enable, "pwm%d_enable",
> > > > + pmbus_show_pwm_enable,
> > > > + pmbus_set_pwm_enable, fan->index);
> > +}
> > +
> > static const struct pmbus_limit_attr vin_limit_attrs[] = {
> > > > {
> > > > .reg = PMBUS_VIN_UV_WARN_LIMIT,
> > @@ -1565,6 +1855,13 @@ static const int pmbus_fan_config_registers[] = {
> > > > 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,
> > +};
> > +
> > static const int pmbus_fan_status_registers[] = {
> > > > PMBUS_STATUS_FAN_12,
> > > > PMBUS_STATUS_FAN_12,
> > @@ -1587,6 +1884,39 @@ 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_fan_ctrl *fan;
> > > > + int rv;
> > +
> > > > + fan = devm_kzalloc(data->dev, sizeof(*fan), GFP_KERNEL);
> > > > + if (!fan)
> > > > + return -ENOMEM;
> > +
> > > > + fan->index = index;
> > > > + fan->page = page;
> > > > + fan->id = id;
> > > > + fan->config = config;
> > +
> > > > + rv = _pmbus_read_word_data(client, page,
> > > > + pmbus_fan_command_registers[id]);
> > > > + if (rv < 0)
> > > > + return rv;
> > > > + fan->command = rv;
> > +
> > > > + rv = pmbus_add_fan_target_attr(data, fan);
> > > > + if (rv < 0)
> > > > + return rv;
> > +
> > > > + rv = pmbus_add_pwm_attr(data, fan);
> > > > + if (rv < 0)
> > > > + return rv;
> > +
> > > > + return pmbus_add_pwm_enable_attr(data, fan);
> > +}
> > +
> > static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > struct pmbus_data *data)
> > {
> > @@ -1624,6 +1954,11 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > PSC_FAN, true, true) == NULL)
> > > > return -ENOMEM;
> >
> > > > + 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
@ 2017-07-12 0:39 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-12 0:39 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon
Cc: jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
[-- Attachment #1: Type: text/plain, Size: 19101 bytes --]
On Tue, 2017-07-11 at 06:40 -0700, Guenter Roeck wrote:
> On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> > Augment PMBus support to include control of fans via the
> > FAN_COMMAND_[1-4] registers, both in RPM and PWM modes. The behaviour
> > of FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4] are tightly coupled, and
> > their interactions do not fit the existing use of struct pmbus_sensor.
> > The patch introduces struct pmbus_fan_ctrl to distinguish from the
> > simple sensor case, along with associated sysfs show/set implementations.
> >
> > Further, the interpreting the value of FAN_COMMAND_[1-4] depends on both
> > the current fan mode (RPM or PWM, as configured in
> > FAN_CONFIG_{1_2,3_4}), and the device-specific behaviour for the
> > register. For example, the MAX31785 chip defines the following:
> >
> > PWM (m = 1, b = 0, R = 2):
> > 0x0000 - 0x2710: 0 - 100% fan PWM duty cycle
> > 0x2711 - 0x7fff: 100% fan PWM duty cycle
> > 0x8000 - 0xffff: Ignore FAN_COMMAND_[1-4], use automatic fan control
> >
> > RPM (m = 1, b = 0, R = 0):
> > 0x0000 - 0x7FFF: 0 - 32,767 RPM
> > 0x8000 - 0xFFFF: Ignore FAN_COMMAND_[1-4], use automatic fan control
> >
> > To handle the device-specific interpretation of the FAN_COMMAND_[1-4],
> > add an optional callbacks to the info struct to get/set the 'mode'
> > value required for the pwm[1-n]_enable sysfs attribute. A fallback
> > calculation exists if the callbacks are not populated; the fallback
> > ignores device-specific ranges and tries to determine a reasonable value
> > from FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4].
> >
>
> This seems overly complex, but unfortunately I don't have time for a detailed
> analysis right now.
No worries. It turned out more complex than I was hoping as well, and I
am keen to hear any insights to trim it down.
> Couple of comments below.
Yep, thanks for taking a look.
>
> Guenter
>
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > drivers/hwmon/pmbus/pmbus.h | 7 +
> > drivers/hwmon/pmbus/pmbus_core.c | 335 +++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 342 insertions(+)
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index bfcb13bae34b..927eabc1b273 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -223,6 +223,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)
> > */
> > @@ -380,6 +382,11 @@ struct pmbus_driver_info {
> > > > int (*identify)(struct i2c_client *client,
> > > > struct pmbus_driver_info *info);
> >
> > > > + /* Allow the driver to interpret the fan command value */
> > > > + int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> > > > + int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> > > > + u16 *fan_command);
> > +
>
> It is not entirely obvious to me why this would require new callback functions.
> Can you overload PMBUS_FAN_CONFIG_12 / PMBUS_FAN_CONFIG_34 or, if that does not
> work for some reason, introduce a virtual register, such as PMBUS_VIRT_PWM_MODE ?
Can you expand on the thought of overloading PMBUS_FAN_CONFIG_{12,34}?
Regarding virtual registers, I saw references to them whilst I was
working my way through the core code but didn't stop to investigate.
I'll take a deeper look.
However, the addition of the callbacks was driven by the behaviour of
the MAX31785, where some values written to PMBUS_FAN_COMMAND_1 trigger
automated control, while others retain manual control. Patch 4/4 should
provide a bit more context, though I've also outlined the behaviour in
the commit message for this patch. I don't have a lot of experience
with PMBus devices so I don't have a good idea if there's a better way
to capture the behaviour that isn't so unconstrained in its approach.
>
> > > > /* Regulator functionality, if supported by this chip driver. */
> > > > int num_regulators;
> > > > const struct regulator_desc *reg_desc;
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index ba59eaef2e07..3b0a55bbbd2c 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -69,6 +69,38 @@ struct pmbus_sensor {
> > #define to_pmbus_sensor(_attr) \
> > > > container_of(_attr, struct pmbus_sensor, attribute)
> >
> > > > +#define PB_FAN_CONFIG_RPM PB_FAN_2_RPM
> > +#define PB_FAN_CONFIG_INSTALLED PB_FAN_2_INSTALLEDBUS_VIRT_
>
> Something seems odd here. PB_FAN_2_INSTALLEDBUS_VIRT_ ?
Yes, that's busted. Not sure what went wrong, but I'll clean it up.
>
> > > > +#define PB_FAN_CONFIG_MASK(i) (0xff << (4 * !((i) & 1)))
> > > > +#define PB_FAN_CONFIG_GET(i, n) (((n) >> (4 * !((i) & 1))) & 0xff)
> > > > +#define PB_FAN_CONFIG_PUT(i, n) (((n) & 0xff) << (4 * !((i) & 1)))
> > +
>
> Aren't there standard bit manipulation macros for that ? Either case, this is just to avoid
> having to use the existing defines.
As I store the configuration for each fan in a struct pmbus_fan_ctrl
dedicated to the fan, I reasoned that intermediate code should not have
to deal with the details of which nibble to access with respect to the
fan's (per-page) ID. Rather, code reading or writing
PMBUS_FAN_COMMAND_[1-4] should deal with ensuring the correct values
are provided.
> Ok, but then I think it would make more sense to
> make it generic, ie change the core to not use PB_FAN_2_RPM / PB_FAN_1_RPM etc.
> but PB_FAN_RPM(index) everywhere.
I'll make the change throughout pmbus core.
>
> > +struct pmbus_fan_ctrl_attr {
> > > > + struct device_attribute attribute;
> > > > + char name[PMBUS_NAME_SIZE];
> > +};
> > +
> > +struct pmbus_fan_ctrl {
> > > > + struct pmbus_fan_ctrl_attr fan_target;
> > > > + struct pmbus_fan_ctrl_attr pwm;
> > > > + struct pmbus_fan_ctrl_attr pwm_enable;
> > > > + int index;
> > > > + u8 page;
> > > > + u8 id;
> > > > + u8 config;
> > > > + u16 command;
> > +};
> > +#define to_pmbus_fan_ctrl_attr(_attr) \
> > > > + container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
> > +#define fan_target_to_pmbus_fan_ctrl(_attr) \
> > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
> > > > + fan_target)
> > +#define pwm_to_pmbus_fan_ctrl(_attr) \
> > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, pwm)
> > +#define pwm_enable_to_pmbus_fan_ctrl(_attr) \
> > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
> > > > + pwm_enable)
> > +
> > struct pmbus_boolean {
> > > > > > char name[PMBUS_NAME_SIZE]; /* sysfs boolean name */
> > > > struct sensor_device_attribute attribute;
> > @@ -806,6 +838,219 @@ static ssize_t pmbus_show_label(struct device *dev,
> > > > return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
> > }
> >
> > +static ssize_t pmbus_show_fan_command(struct device *dev,
> > > > + enum pmbus_fan_mode mode,
> > > > + struct pmbus_fan_ctrl *fan, char *buf)
> > +{
> > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + struct pmbus_sensor sensor;
> > > > + long val;
> > +
> > > > + mutex_lock(&data->update_lock);
> > +
> > > > + if ((mode == percent && (fan->config & PB_FAN_CONFIG_RPM)) ||
> > > > + (mode == rpm && !(fan->config & PB_FAN_CONFIG_RPM))) {
> > > > + mutex_unlock(&data->update_lock);
> > + return -ENOTSUPP; /* XXX: This seems dodgy, but what to do? */
>
> Not create the attribute in question in the first place, or return 0. The above
> messes up the 'sensors' command.
I think returning 0 is the only valid option of the two, given that we
can dynamically switch between RPM and PWM modes.
Thanks for the feedback.
Andrew
>
> > > > + }
> > +
> > > > + sensor.class = PSC_FAN;
> > > > + if (mode == percent)
> > > > + sensor.data = fan->command * 255 / 100;
> > > > + else
> > > > + sensor.data = fan->command;
> > +
> > > > + val = pmbus_reg2data(data, &sensor);
> > +
> > > > + mutex_unlock(&data->update_lock);
> > +
> > > > + return snprintf(buf, PAGE_SIZE, "%ld\n", val);
> > +}
> > +
> > +static ssize_t pmbus_show_fan_target(struct device *dev,
> > > > + struct device_attribute *da, char *buf)
> > +{
> > > > + return pmbus_show_fan_command(dev, rpm,
> > > > + fan_target_to_pmbus_fan_ctrl(da), buf);
> > +}
> > +
> > +static ssize_t pmbus_show_pwm(struct device *dev,
> > > > + struct device_attribute *da, char *buf)
> > +{
> > > > + return pmbus_show_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> > > > + buf);
> > +}
> > +
> > +static ssize_t pmbus_set_fan_command(struct device *dev,
> > > > + enum pmbus_fan_mode mode,
> > > > + struct pmbus_fan_ctrl *fan,
> > > > + const char *buf, ssize_t count)
> > +{
> > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + int config_addr, command_addr;
> > > > + struct pmbus_sensor sensor;
> > > > + ssize_t rv;
> > > > + long val;
> > +
> > > > + if (kstrtol(buf, 10, &val) < 0)
> > > > + return -EINVAL;
> > +
> > > > + mutex_lock(&data->update_lock);
> > +
> > > > + sensor.class = PSC_FAN;
> > +
> > > > + val = pmbus_data2reg(data, &sensor, val);
> > +
> > > > + if (mode == percent)
> > > > + val = val * 100 / 255;
> > +
> > > > + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > + command_addr = config_addr + 1 + (fan->id & 1);
> > +
> > > > + if (mode == rpm)
> > > > + fan->config |= PB_FAN_CONFIG_RPM;
> > > > + else
> > > > + fan->config &= ~PB_FAN_CONFIG_RPM;
> > +
> > > > + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> > > > + PB_FAN_CONFIG_PUT(fan->id, fan->config),
> > > > + PB_FAN_CONFIG_MASK(fan->id));
> > > > + if (rv < 0)
> > > > + goto done;
> > +
> > > > + fan->command = val;
> > > > + rv = pmbus_write_word_data(client, fan->page, command_addr,
> > > > + fan->command);
> > +
> > +done:
> > > > + mutex_unlock(&data->update_lock);
> > +
> > > > + if (rv < 0)
> > > > + return rv;
> > +
> > > > + return count;
> > +}
> > +
> > +static ssize_t pmbus_set_fan_target(struct device *dev,
> > > > + struct device_attribute *da,
> > > > + const char *buf, size_t count)
> > +{
> > > > + return pmbus_set_fan_command(dev, rpm,
> > > > + fan_target_to_pmbus_fan_ctrl(da), buf,
> > > > + count);
> > +}
> > +
> > +static ssize_t pmbus_set_pwm(struct device *dev, struct device_attribute *da,
> > > > + const char *buf, size_t count)
> > +{
> > > > + return pmbus_set_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> > > > + buf, count);
> > +}
> > +
> > +static ssize_t pmbus_show_pwm_enable(struct device *dev,
> > > > + struct device_attribute *da, char *buf)
> > +{
> > > > + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + long mode;
> > +
> > > > + mutex_lock(&data->update_lock);
> > +
> > +
> > > > + if (data->info->get_pwm_mode) {
> > > > + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > +
> > > > + mode = data->info->get_pwm_mode(fan->id, config, fan->command);
> > > > + } else {
> > > > + struct pmbus_sensor sensor = {
> > > > + .class = PSC_FAN,
> > > > + .data = fan->command,
> > > > + };
> > > > + long command;
> > +
> > > > + command = pmbus_reg2data(data, &sensor);
> > +
> > > > + /* XXX: Need to do something sensible */
> > > > + if (fan->config & PB_FAN_CONFIG_RPM)
> > > > + mode = 2;
> > > > + else
> > > > + mode = (command >= 0 && command < 100);
> > > > + }
> > +
> > > > + mutex_unlock(&data->update_lock);
> > +
> > > > + return snprintf(buf, PAGE_SIZE, "%ld\n", mode);
> > +}
> > +
> > +static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > + struct device_attribute *da,
> > > > + const char *buf, size_t count)
> > +{
> > > > + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + int config_addr, command_addr;
> > > > + struct pmbus_sensor sensor;
> > > > + ssize_t rv = count;
> > > > + long mode;
> > +
> > > > + if (kstrtol(buf, 10, &mode) < 0)
> > > > + return -EINVAL;
> > +
> > > > + mutex_lock(&data->update_lock);
> > +
> > > > + sensor.class = PSC_FAN;
> > +
> > > > + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > + command_addr = config_addr + 1 + (fan->id & 1);
> > +
> > > > + if (data->info->set_pwm_mode) {
> > > > + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > > + u16 command = fan->command;
> > +
> > > > + rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > + if (rv < 0)
> > > > + goto done;
> > +
> > > > + fan->config = PB_FAN_CONFIG_GET(fan->id, config);
> > > > + fan->command = command;
> > > > + } else {
> > > > + fan->config &= ~PB_FAN_CONFIG_RPM;
> > > > + switch (mode) {
> > > > + case 0:
> > > > + case 1:
> > > > + /* XXX: Safe at least? */
> > > > + fan->command = pmbus_data2reg(data, &sensor, 100);
> > > > + break;
> > > > + case 2:
> > > > + default:
> > > > + /* XXX: Safe at least? */
> > > > + fan->command = 0xffff;
> > > > + break;
> > > > + }
> > > > + }
> > +
> > > > + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> > > > + PB_FAN_CONFIG_PUT(fan->id, fan->config),
> > > > + PB_FAN_CONFIG_MASK(fan->id));
> > > > + if (rv < 0)
> > > > + goto done;
> > +
> > > > + rv = pmbus_write_word_data(client, fan->page, command_addr,
> > > > + fan->command);
> > +
> > +done:
> > > > + mutex_unlock(&data->update_lock);
> > +
> > > > + if (rv < 0)
> > > > + return rv;
> > +
> > > > + return count;
> > +}
> > +
> > static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
> > {
> > > > if (data->num_attributes >= data->max_attributes - 1) {
> > @@ -1094,6 +1339,51 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> > > > return 0;
> > }
> >
> > +static int pmbus_add_fan_ctrl_attr(struct pmbus_data *data,
> > > > + struct pmbus_fan_ctrl_attr *fa,
> > > > + const char *name_fmt,
> > > > + ssize_t (*show)(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + char *buf),
> > > > + ssize_t (*store)(struct device *dev,
> > > > + struct device_attribute *attr,
> > > > + const char *buf, size_t count),
> > > > + int idx)
> > +{
> > > > + struct device_attribute *da;
> > +
> > > > + da = &fa->attribute;
> > +
> > > > + snprintf(fa->name, sizeof(fa->name), name_fmt, idx);
> > > > + pmbus_dev_attr_init(da, fa->name, 0644, show, store);
> > +
> > > > + return pmbus_add_attribute(data, &da->attr);
> > +}
> > +
> > +static inline int pmbus_add_fan_target_attr(struct pmbus_data *data,
> > > > + struct pmbus_fan_ctrl *fan)
> > +{
> > > > + return pmbus_add_fan_ctrl_attr(data, &fan->fan_target, "fan%d_target",
> > > > + pmbus_show_fan_target,
> > > > + pmbus_set_fan_target, fan->index);
> > +}
> > +
> > +static inline int pmbus_add_pwm_attr(struct pmbus_data *data,
> > > > + struct pmbus_fan_ctrl *fan)
> > +{
> > +
> > > > + return pmbus_add_fan_ctrl_attr(data, &fan->pwm, "pwm%d", pmbus_show_pwm,
> > > > + pmbus_set_pwm, fan->index);
> > +}
> > +
> > +static inline int pmbus_add_pwm_enable_attr(struct pmbus_data *data,
> > > > + struct pmbus_fan_ctrl *fan)
> > +{
> > > > + return pmbus_add_fan_ctrl_attr(data, &fan->pwm_enable, "pwm%d_enable",
> > > > + pmbus_show_pwm_enable,
> > > > + pmbus_set_pwm_enable, fan->index);
> > +}
> > +
> > static const struct pmbus_limit_attr vin_limit_attrs[] = {
> > > > {
> > > > .reg = PMBUS_VIN_UV_WARN_LIMIT,
> > @@ -1565,6 +1855,13 @@ static const int pmbus_fan_config_registers[] = {
> > > > 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,
> > +};
> > +
> > static const int pmbus_fan_status_registers[] = {
> > > > PMBUS_STATUS_FAN_12,
> > > > PMBUS_STATUS_FAN_12,
> > @@ -1587,6 +1884,39 @@ 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_fan_ctrl *fan;
> > > > + int rv;
> > +
> > > > + fan = devm_kzalloc(data->dev, sizeof(*fan), GFP_KERNEL);
> > > > + if (!fan)
> > > > + return -ENOMEM;
> > +
> > > > + fan->index = index;
> > > > + fan->page = page;
> > > > + fan->id = id;
> > > > + fan->config = config;
> > +
> > > > + rv = _pmbus_read_word_data(client, page,
> > > > + pmbus_fan_command_registers[id]);
> > > > + if (rv < 0)
> > > > + return rv;
> > > > + fan->command = rv;
> > +
> > > > + rv = pmbus_add_fan_target_attr(data, fan);
> > > > + if (rv < 0)
> > > > + return rv;
> > +
> > > > + rv = pmbus_add_pwm_attr(data, fan);
> > > > + if (rv < 0)
> > > > + return rv;
> > +
> > > > + return pmbus_add_pwm_enable_attr(data, fan);
> > +}
> > +
> > static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > struct pmbus_data *data)
> > {
> > @@ -1624,6 +1954,11 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > PSC_FAN, true, true) == NULL)
> > > > return -ENOMEM;
> >
> > > > + 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values
2017-07-11 13:31 ` Guenter Roeck
@ 2017-07-12 1:20 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-12 1:20 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon
Cc: jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
[-- Attachment #1: Type: text/plain, Size: 14877 bytes --]
On Tue, 2017-07-11 at 06:31 -0700, Guenter Roeck wrote:
> On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> > Some PMBus chips, such as the MAX31785, use different coefficients for
> > FAN_COMMAND_[1-4] depending on whether the fan is in PWM (percent duty)
> > or RPM mode. Add a callback to allow the driver to provide the
> > applicable coefficients to avoid imposing on devices that don't have
> > this requirement.
> >
>
> Why not just introduce another class, such as PSC_PWM ?
I considered this up front however I wasn't sure where the PMBus sensor
classes were modelled from. The PMBus spec generally doesn't discuss
PMW beyond the concept of fans, and given PSC_FAN already existed I had
a vague feeling that introducing PSC_PWM might not be the way to go. It
also means that PSC_FAN is implicitly RPM in some circumstances, or
both RPM and PWM in others, and wasn't previously contrasted against
PWM as PWM-specific configuration wasn't an option.
However if it's reasonable it should lead to a more straight forward
patch. I'll rework and resend if it falls out nicely.
Thanks,
Andrew
>
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > drivers/hwmon/pmbus/pmbus.h | 18 +++++--
> > drivers/hwmon/pmbus/pmbus_core.c | 112 ++++++++++++++++++++++++++++++++-------
> > 2 files changed, 108 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index 927eabc1b273..338ecc8b25a4 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -345,6 +345,12 @@ enum pmbus_sensor_classes {
> > enum pmbus_data_format { linear = 0, direct, vid };
> > enum vrm_version { vr11 = 0, vr12 };
> >
> > +struct pmbus_coeffs {
> > > > + int m; /* mantissa for direct data format */
> > > > + int b; /* offset */
> > > > + int R; /* exponent */
> > +};
> > +
> > struct pmbus_driver_info {
> > > > > > int pages; /* Total number of pages */
> > > > enum pmbus_data_format format[PSC_NUM_CLASSES];
> > @@ -353,9 +359,7 @@ struct pmbus_driver_info {
> > > > * Support one set of coefficients for each sensor type
> > > > * Used for chips providing data in direct mode.
> > > > */
> > > > > > - int m[PSC_NUM_CLASSES]; /* mantissa for direct data format */
> > > > > > - int b[PSC_NUM_CLASSES]; /* offset */
> > > > > > - int R[PSC_NUM_CLASSES]; /* exponent */
> > > > + struct pmbus_coeffs coeffs[PSC_NUM_CLASSES];
> >
> > > > > > u32 func[PMBUS_PAGES]; /* Functionality, per page */
> > > > /*
> > @@ -382,6 +386,14 @@ struct pmbus_driver_info {
> > > > int (*identify)(struct i2c_client *client,
> > > > struct pmbus_driver_info *info);
> >
> > > > + /*
> > > > + * If a fan's coefficents change over time (e.g. between RPM and PWM
> > > > + * mode), then the driver can provide a function for retrieving the
> > > > + * currently applicable coefficients.
> > > > + */
> > > > + const struct pmbus_coeffs *(*get_fan_coeffs)(
> > > > + const struct pmbus_driver_info *info, int index,
> > > > + enum pmbus_fan_mode mode, int command);
> > > > /* Allow the driver to interpret the fan command value */
> > > > int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> > > > int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index 3b0a55bbbd2c..4ff6a1fd5cce 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -58,10 +58,11 @@
> > struct pmbus_sensor {
> > > > struct pmbus_sensor *next;
> > > > > > char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */
> > > > - struct device_attribute attribute;
> > > > + struct sensor_device_attribute attribute;
> > > > > > u8 page; /* page number */
> > > > > > u16 reg; /* register */
> > > > > > enum pmbus_sensor_classes class; /* sensor class */
> > > > + const struct pmbus_coeffs *coeffs;
> > > > > > bool update; /* runtime sensor update needed */
> > > > > > int data; /* Sensor data.
> > > > Negative if there was a read error */
> > @@ -89,6 +90,7 @@ struct pmbus_fan_ctrl {
> > > > u8 id;
> > > > u8 config;
> > > > u16 command;
> > > > + const struct pmbus_coeffs *coeffs;
> > };
> > #define to_pmbus_fan_ctrl_attr(_attr) \
> > > > container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
> > @@ -511,9 +513,15 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
> > > > long val = (s16) sensor->data;
> > > > long m, b, R;
> >
> > > > - m = data->info->m[sensor->class];
> > > > - b = data->info->b[sensor->class];
> > > > - R = data->info->R[sensor->class];
> > > > + if (sensor->coeffs) {
> > > > + m = sensor->coeffs->m;
> > > > + b = sensor->coeffs->b;
> > > > + R = sensor->coeffs->R;
> > > > + } else {
> > > > + m = data->info->coeffs[sensor->class].m;
> > > > + b = data->info->coeffs[sensor->class].b;
> > > > + R = data->info->coeffs[sensor->class].R;
> > > > + }
> >
> > > > if (m == 0)
> > > > return 0;
> > @@ -663,9 +671,15 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
> > {
> > > > long m, b, R;
> >
> > > > - m = data->info->m[sensor->class];
> > > > - b = data->info->b[sensor->class];
> > > > - R = data->info->R[sensor->class];
> > > > + if (sensor->coeffs) {
> > > > + m = sensor->coeffs->m;
> > > > + b = sensor->coeffs->b;
> > > > + R = sensor->coeffs->R;
> > > > + } else {
> > > > + m = data->info->coeffs[sensor->class].m;
> > > > + b = data->info->coeffs[sensor->class].b;
> > > > + R = data->info->coeffs[sensor->class].R;
> > > > + }
> >
> > > > /* Power is in uW. Adjust R and b. */
> > > > if (sensor->class == PSC_POWER) {
> > @@ -796,7 +810,9 @@ static ssize_t pmbus_show_sensor(struct device *dev,
> > > > struct device_attribute *devattr, char *buf)
> > {
> > > > struct pmbus_data *data = pmbus_update_device(dev);
> > > > - struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> > > > + struct pmbus_sensor *sensor;
> > +
> > > > + sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
> >
> > > > if (sensor->data < 0)
> > > > return sensor->data;
> > @@ -810,12 +826,14 @@ static ssize_t pmbus_set_sensor(struct device *dev,
> > {
> > > > struct i2c_client *client = to_i2c_client(dev->parent);
> > > > struct pmbus_data *data = i2c_get_clientdata(client);
> > > > - struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> > > > + struct pmbus_sensor *sensor;
> > > > ssize_t rv = count;
> > > > long val = 0;
> > > > int ret;
> > > > u16 regval;
> >
> > > > + sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
> > +
> > > > if (kstrtol(buf, 10, &val) < 0)
> > > > return -EINVAL;
> >
> > @@ -856,6 +874,7 @@ static ssize_t pmbus_show_fan_command(struct device *dev,
> > > > }
> >
> > > > sensor.class = PSC_FAN;
> > > > + sensor.coeffs = fan->coeffs;
> > > > if (mode == percent)
> > > > sensor.data = fan->command * 255 / 100;
> > > > else
> > @@ -882,6 +901,29 @@ static ssize_t pmbus_show_pwm(struct device *dev,
> > > > buf);
> > }
> >
> > +static int pmbus_update_fan_coeffs(struct pmbus_data *data,
> > > > + struct pmbus_fan_ctrl *fan,
> > > > + enum pmbus_fan_mode mode)
> > +{
> > > > + const struct pmbus_driver_info *info = data->info;
> > > > + struct pmbus_sensor *curr = data->sensors;
> > +
> > > > + fan->coeffs = info->get_fan_coeffs(info, fan->index, mode,
> > > > + PMBUS_FAN_COMMAND_1 + fan->id);
> > +
> > > > + while (curr) {
> > > > + if (curr->class == PSC_FAN &&
> > > > + curr->attribute.index == fan->index) {
> > > > + curr->coeffs = info->get_fan_coeffs(info, fan->index,
> > > > + mode, curr->reg);
> > > > + }
> > +
> > > > + curr = curr->next;
> > > > + }
> > +
> > > > + return 0;
> > +}
> > +
> > static ssize_t pmbus_set_fan_command(struct device *dev,
> > > > enum pmbus_fan_mode mode,
> > > > struct pmbus_fan_ctrl *fan,
> > @@ -889,6 +931,7 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
> > {
> > > > struct i2c_client *client = to_i2c_client(dev->parent);
> > > > struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + const struct pmbus_driver_info *info = data->info;
> > > > int config_addr, command_addr;
> > > > struct pmbus_sensor sensor;
> > > > ssize_t rv;
> > @@ -899,7 +942,13 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
> >
> > > > mutex_lock(&data->update_lock);
> >
> > > > + if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
> > > > + pmbus_update_fan_coeffs(data, fan, mode);
> > > > + sensor.coeffs = fan->coeffs;
> > > > + }
> > +
> > > > sensor.class = PSC_FAN;
> > > > + sensor.attribute.index = fan->index;
> >
> > > > val = pmbus_data2reg(data, &sensor, val);
> >
> > @@ -968,6 +1017,8 @@ static ssize_t pmbus_show_pwm_enable(struct device *dev,
> > > > struct pmbus_sensor sensor = {
> > > > .class = PSC_FAN,
> > > > .data = fan->command,
> > > > + .attribute.index = fan->index,
> > > > + .coeffs = fan->coeffs,
> > > > };
> > > > long command;
> >
> > @@ -992,6 +1043,7 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > struct i2c_client *client = to_i2c_client(dev->parent);
> > > > struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + const struct pmbus_driver_info *info = data->info;
> > > > int config_addr, command_addr;
> > > > struct pmbus_sensor sensor;
> > > > ssize_t rv = count;
> > @@ -1003,15 +1055,21 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > mutex_lock(&data->update_lock);
> >
> > > > sensor.class = PSC_FAN;
> > > > + sensor.attribute.index = fan->index;
> > +
> > > > + if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
> > > > + pmbus_update_fan_coeffs(data, fan, percent);
> > > > + sensor.coeffs = fan->coeffs;
> > > > + }
> >
> > > > config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > command_addr = config_addr + 1 + (fan->id & 1);
> >
> > > > - if (data->info->set_pwm_mode) {
> > > > + if (info->set_pwm_mode) {
> > > > u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > > u16 command = fan->command;
> >
> > > > - rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > + rv = info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > if (rv < 0)
> > > > goto done;
> >
> > @@ -1138,7 +1196,7 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> > > > sensor = devm_kzalloc(data->dev, sizeof(*sensor), GFP_KERNEL);
> > > > if (!sensor)
> > > > return NULL;
> > > > - a = &sensor->attribute;
> > > > + a = &sensor->attribute.dev_attr;
> >
> > > > snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
> > > > name, seq, type);
> > @@ -1146,9 +1204,9 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> > > > sensor->reg = reg;
> > > > sensor->class = class;
> > > > sensor->update = update;
> > > > - pmbus_dev_attr_init(a, sensor->name,
> > > > + pmbus_attr_init(&sensor->attribute, sensor->name,
> > > > readonly ? S_IRUGO : S_IRUGO | S_IWUSR,
> > > > - pmbus_show_sensor, pmbus_set_sensor);
> > > > + pmbus_show_sensor, pmbus_set_sensor, seq);
> >
> > > > if (pmbus_add_attribute(data, &a->attr))
> > > > return NULL;
> > @@ -1886,7 +1944,7 @@ 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)
> > > > + u8 config, const struct pmbus_coeffs *coeffs)
> > {
> > > > struct pmbus_fan_ctrl *fan;
> > > > int rv;
> > @@ -1898,6 +1956,7 @@ static int pmbus_add_fan_ctrl(struct i2c_client *client,
> > > > fan->index = index;
> > > > fan->page = page;
> > > > fan->id = id;
> > > > + fan->coeffs = coeffs;
> > > > fan->config = config;
> >
> > > > rv = _pmbus_read_word_data(client, page,
> > @@ -1921,6 +1980,8 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > struct pmbus_data *data)
> > {
> > > > const struct pmbus_driver_info *info = data->info;
> > > > + const struct pmbus_coeffs *coeffs = NULL;
> > > > + enum pmbus_fan_mode mode;
> > > > int index = 1;
> > > > int page;
> > > > int ret;
> > @@ -1929,6 +1990,7 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > int f;
> >
> > > > for (f = 0; f < ARRAY_SIZE(pmbus_fan_registers); f++) {
> > > > + struct pmbus_sensor *sensor;
> > > > int regval;
> >
> > > > if (!(info->func[page] & pmbus_fan_flags[f]))
> > @@ -1949,13 +2011,25 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > (!(regval & (PB_FAN_1_INSTALLED >> ((f & 1) * 4)))))
> > > > continue;
> >
> > > > - if (pmbus_add_sensor(data, "fan", "input", index,
> > > > - page, pmbus_fan_registers[f],
> > > > - PSC_FAN, true, true) == NULL)
> > > > + sensor = pmbus_add_sensor(data, "fan", "input", index,
> > > > + page, pmbus_fan_registers[f],
> > > > + PSC_FAN, true, true);
> > > > + if (!sensor)
> > > > return -ENOMEM;
> >
> > > > + /* Add coeffs here as we have access to the fan mode */
> > > > + if (info->format[PSC_FAN] == direct &&
> > > > + info->get_fan_coeffs) {
> > > > + const u16 mask = PB_FAN_1_RPM >> ((f & 1) * 4);
> > +
> > > > + mode = (regval & mask) ? rpm : percent;
> > > > + coeffs = info->get_fan_coeffs(info, index, mode,
> > > > + pmbus_fan_registers[f]);
> > > > + sensor->coeffs = coeffs;
> > > > + }
> > +
> > > > ret = pmbus_add_fan_ctrl(client, data, index, page, f,
> > > > - regval);
> > > > + regval, coeffs);
> > > > if (ret < 0)
> > > > return ret;
> >
> >
>
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values
@ 2017-07-12 1:20 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-12 1:20 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon
Cc: jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
[-- Attachment #1: Type: text/plain, Size: 14877 bytes --]
On Tue, 2017-07-11 at 06:31 -0700, Guenter Roeck wrote:
> On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> > Some PMBus chips, such as the MAX31785, use different coefficients for
> > FAN_COMMAND_[1-4] depending on whether the fan is in PWM (percent duty)
> > or RPM mode. Add a callback to allow the driver to provide the
> > applicable coefficients to avoid imposing on devices that don't have
> > this requirement.
> >
>
> Why not just introduce another class, such as PSC_PWM ?
I considered this up front however I wasn't sure where the PMBus sensor
classes were modelled from. The PMBus spec generally doesn't discuss
PMW beyond the concept of fans, and given PSC_FAN already existed I had
a vague feeling that introducing PSC_PWM might not be the way to go. It
also means that PSC_FAN is implicitly RPM in some circumstances, or
both RPM and PWM in others, and wasn't previously contrasted against
PWM as PWM-specific configuration wasn't an option.
However if it's reasonable it should lead to a more straight forward
patch. I'll rework and resend if it falls out nicely.
Thanks,
Andrew
>
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> > drivers/hwmon/pmbus/pmbus.h | 18 +++++--
> > drivers/hwmon/pmbus/pmbus_core.c | 112 ++++++++++++++++++++++++++++++++-------
> > 2 files changed, 108 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > index 927eabc1b273..338ecc8b25a4 100644
> > --- a/drivers/hwmon/pmbus/pmbus.h
> > +++ b/drivers/hwmon/pmbus/pmbus.h
> > @@ -345,6 +345,12 @@ enum pmbus_sensor_classes {
> > enum pmbus_data_format { linear = 0, direct, vid };
> > enum vrm_version { vr11 = 0, vr12 };
> >
> > +struct pmbus_coeffs {
> > > > + int m; /* mantissa for direct data format */
> > > > + int b; /* offset */
> > > > + int R; /* exponent */
> > +};
> > +
> > struct pmbus_driver_info {
> > > > > > int pages; /* Total number of pages */
> > > > enum pmbus_data_format format[PSC_NUM_CLASSES];
> > @@ -353,9 +359,7 @@ struct pmbus_driver_info {
> > > > * Support one set of coefficients for each sensor type
> > > > * Used for chips providing data in direct mode.
> > > > */
> > > > > > - int m[PSC_NUM_CLASSES]; /* mantissa for direct data format */
> > > > > > - int b[PSC_NUM_CLASSES]; /* offset */
> > > > > > - int R[PSC_NUM_CLASSES]; /* exponent */
> > > > + struct pmbus_coeffs coeffs[PSC_NUM_CLASSES];
> >
> > > > > > u32 func[PMBUS_PAGES]; /* Functionality, per page */
> > > > /*
> > @@ -382,6 +386,14 @@ struct pmbus_driver_info {
> > > > int (*identify)(struct i2c_client *client,
> > > > struct pmbus_driver_info *info);
> >
> > > > + /*
> > > > + * If a fan's coefficents change over time (e.g. between RPM and PWM
> > > > + * mode), then the driver can provide a function for retrieving the
> > > > + * currently applicable coefficients.
> > > > + */
> > > > + const struct pmbus_coeffs *(*get_fan_coeffs)(
> > > > + const struct pmbus_driver_info *info, int index,
> > > > + enum pmbus_fan_mode mode, int command);
> > > > /* Allow the driver to interpret the fan command value */
> > > > int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> > > > int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > index 3b0a55bbbd2c..4ff6a1fd5cce 100644
> > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > @@ -58,10 +58,11 @@
> > struct pmbus_sensor {
> > > > struct pmbus_sensor *next;
> > > > > > char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */
> > > > - struct device_attribute attribute;
> > > > + struct sensor_device_attribute attribute;
> > > > > > u8 page; /* page number */
> > > > > > u16 reg; /* register */
> > > > > > enum pmbus_sensor_classes class; /* sensor class */
> > > > + const struct pmbus_coeffs *coeffs;
> > > > > > bool update; /* runtime sensor update needed */
> > > > > > int data; /* Sensor data.
> > > > Negative if there was a read error */
> > @@ -89,6 +90,7 @@ struct pmbus_fan_ctrl {
> > > > u8 id;
> > > > u8 config;
> > > > u16 command;
> > > > + const struct pmbus_coeffs *coeffs;
> > };
> > #define to_pmbus_fan_ctrl_attr(_attr) \
> > > > container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
> > @@ -511,9 +513,15 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
> > > > long val = (s16) sensor->data;
> > > > long m, b, R;
> >
> > > > - m = data->info->m[sensor->class];
> > > > - b = data->info->b[sensor->class];
> > > > - R = data->info->R[sensor->class];
> > > > + if (sensor->coeffs) {
> > > > + m = sensor->coeffs->m;
> > > > + b = sensor->coeffs->b;
> > > > + R = sensor->coeffs->R;
> > > > + } else {
> > > > + m = data->info->coeffs[sensor->class].m;
> > > > + b = data->info->coeffs[sensor->class].b;
> > > > + R = data->info->coeffs[sensor->class].R;
> > > > + }
> >
> > > > if (m == 0)
> > > > return 0;
> > @@ -663,9 +671,15 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
> > {
> > > > long m, b, R;
> >
> > > > - m = data->info->m[sensor->class];
> > > > - b = data->info->b[sensor->class];
> > > > - R = data->info->R[sensor->class];
> > > > + if (sensor->coeffs) {
> > > > + m = sensor->coeffs->m;
> > > > + b = sensor->coeffs->b;
> > > > + R = sensor->coeffs->R;
> > > > + } else {
> > > > + m = data->info->coeffs[sensor->class].m;
> > > > + b = data->info->coeffs[sensor->class].b;
> > > > + R = data->info->coeffs[sensor->class].R;
> > > > + }
> >
> > > > /* Power is in uW. Adjust R and b. */
> > > > if (sensor->class == PSC_POWER) {
> > @@ -796,7 +810,9 @@ static ssize_t pmbus_show_sensor(struct device *dev,
> > > > struct device_attribute *devattr, char *buf)
> > {
> > > > struct pmbus_data *data = pmbus_update_device(dev);
> > > > - struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> > > > + struct pmbus_sensor *sensor;
> > +
> > > > + sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
> >
> > > > if (sensor->data < 0)
> > > > return sensor->data;
> > @@ -810,12 +826,14 @@ static ssize_t pmbus_set_sensor(struct device *dev,
> > {
> > > > struct i2c_client *client = to_i2c_client(dev->parent);
> > > > struct pmbus_data *data = i2c_get_clientdata(client);
> > > > - struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
> > > > + struct pmbus_sensor *sensor;
> > > > ssize_t rv = count;
> > > > long val = 0;
> > > > int ret;
> > > > u16 regval;
> >
> > > > + sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
> > +
> > > > if (kstrtol(buf, 10, &val) < 0)
> > > > return -EINVAL;
> >
> > @@ -856,6 +874,7 @@ static ssize_t pmbus_show_fan_command(struct device *dev,
> > > > }
> >
> > > > sensor.class = PSC_FAN;
> > > > + sensor.coeffs = fan->coeffs;
> > > > if (mode == percent)
> > > > sensor.data = fan->command * 255 / 100;
> > > > else
> > @@ -882,6 +901,29 @@ static ssize_t pmbus_show_pwm(struct device *dev,
> > > > buf);
> > }
> >
> > +static int pmbus_update_fan_coeffs(struct pmbus_data *data,
> > > > + struct pmbus_fan_ctrl *fan,
> > > > + enum pmbus_fan_mode mode)
> > +{
> > > > + const struct pmbus_driver_info *info = data->info;
> > > > + struct pmbus_sensor *curr = data->sensors;
> > +
> > > > + fan->coeffs = info->get_fan_coeffs(info, fan->index, mode,
> > > > + PMBUS_FAN_COMMAND_1 + fan->id);
> > +
> > > > + while (curr) {
> > > > + if (curr->class == PSC_FAN &&
> > > > + curr->attribute.index == fan->index) {
> > > > + curr->coeffs = info->get_fan_coeffs(info, fan->index,
> > > > + mode, curr->reg);
> > > > + }
> > +
> > > > + curr = curr->next;
> > > > + }
> > +
> > > > + return 0;
> > +}
> > +
> > static ssize_t pmbus_set_fan_command(struct device *dev,
> > > > enum pmbus_fan_mode mode,
> > > > struct pmbus_fan_ctrl *fan,
> > @@ -889,6 +931,7 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
> > {
> > > > struct i2c_client *client = to_i2c_client(dev->parent);
> > > > struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + const struct pmbus_driver_info *info = data->info;
> > > > int config_addr, command_addr;
> > > > struct pmbus_sensor sensor;
> > > > ssize_t rv;
> > @@ -899,7 +942,13 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
> >
> > > > mutex_lock(&data->update_lock);
> >
> > > > + if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
> > > > + pmbus_update_fan_coeffs(data, fan, mode);
> > > > + sensor.coeffs = fan->coeffs;
> > > > + }
> > +
> > > > sensor.class = PSC_FAN;
> > > > + sensor.attribute.index = fan->index;
> >
> > > > val = pmbus_data2reg(data, &sensor, val);
> >
> > @@ -968,6 +1017,8 @@ static ssize_t pmbus_show_pwm_enable(struct device *dev,
> > > > struct pmbus_sensor sensor = {
> > > > .class = PSC_FAN,
> > > > .data = fan->command,
> > > > + .attribute.index = fan->index,
> > > > + .coeffs = fan->coeffs,
> > > > };
> > > > long command;
> >
> > @@ -992,6 +1043,7 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > struct i2c_client *client = to_i2c_client(dev->parent);
> > > > struct pmbus_data *data = i2c_get_clientdata(client);
> > > > + const struct pmbus_driver_info *info = data->info;
> > > > int config_addr, command_addr;
> > > > struct pmbus_sensor sensor;
> > > > ssize_t rv = count;
> > @@ -1003,15 +1055,21 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > mutex_lock(&data->update_lock);
> >
> > > > sensor.class = PSC_FAN;
> > > > + sensor.attribute.index = fan->index;
> > +
> > > > + if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
> > > > + pmbus_update_fan_coeffs(data, fan, percent);
> > > > + sensor.coeffs = fan->coeffs;
> > > > + }
> >
> > > > config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > command_addr = config_addr + 1 + (fan->id & 1);
> >
> > > > - if (data->info->set_pwm_mode) {
> > > > + if (info->set_pwm_mode) {
> > > > u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > > u16 command = fan->command;
> >
> > > > - rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > + rv = info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > if (rv < 0)
> > > > goto done;
> >
> > @@ -1138,7 +1196,7 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> > > > sensor = devm_kzalloc(data->dev, sizeof(*sensor), GFP_KERNEL);
> > > > if (!sensor)
> > > > return NULL;
> > > > - a = &sensor->attribute;
> > > > + a = &sensor->attribute.dev_attr;
> >
> > > > snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
> > > > name, seq, type);
> > @@ -1146,9 +1204,9 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
> > > > sensor->reg = reg;
> > > > sensor->class = class;
> > > > sensor->update = update;
> > > > - pmbus_dev_attr_init(a, sensor->name,
> > > > + pmbus_attr_init(&sensor->attribute, sensor->name,
> > > > readonly ? S_IRUGO : S_IRUGO | S_IWUSR,
> > > > - pmbus_show_sensor, pmbus_set_sensor);
> > > > + pmbus_show_sensor, pmbus_set_sensor, seq);
> >
> > > > if (pmbus_add_attribute(data, &a->attr))
> > > > return NULL;
> > @@ -1886,7 +1944,7 @@ 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)
> > > > + u8 config, const struct pmbus_coeffs *coeffs)
> > {
> > > > struct pmbus_fan_ctrl *fan;
> > > > int rv;
> > @@ -1898,6 +1956,7 @@ static int pmbus_add_fan_ctrl(struct i2c_client *client,
> > > > fan->index = index;
> > > > fan->page = page;
> > > > fan->id = id;
> > > > + fan->coeffs = coeffs;
> > > > fan->config = config;
> >
> > > > rv = _pmbus_read_word_data(client, page,
> > @@ -1921,6 +1980,8 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > struct pmbus_data *data)
> > {
> > > > const struct pmbus_driver_info *info = data->info;
> > > > + const struct pmbus_coeffs *coeffs = NULL;
> > > > + enum pmbus_fan_mode mode;
> > > > int index = 1;
> > > > int page;
> > > > int ret;
> > @@ -1929,6 +1990,7 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > int f;
> >
> > > > for (f = 0; f < ARRAY_SIZE(pmbus_fan_registers); f++) {
> > > > + struct pmbus_sensor *sensor;
> > > > int regval;
> >
> > > > if (!(info->func[page] & pmbus_fan_flags[f]))
> > @@ -1949,13 +2011,25 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > (!(regval & (PB_FAN_1_INSTALLED >> ((f & 1) * 4)))))
> > > > continue;
> >
> > > > - if (pmbus_add_sensor(data, "fan", "input", index,
> > > > - page, pmbus_fan_registers[f],
> > > > - PSC_FAN, true, true) == NULL)
> > > > + sensor = pmbus_add_sensor(data, "fan", "input", index,
> > > > + page, pmbus_fan_registers[f],
> > > > + PSC_FAN, true, true);
> > > > + if (!sensor)
> > > > return -ENOMEM;
> >
> > > > + /* Add coeffs here as we have access to the fan mode */
> > > > + if (info->format[PSC_FAN] == direct &&
> > > > + info->get_fan_coeffs) {
> > > > + const u16 mask = PB_FAN_1_RPM >> ((f & 1) * 4);
> > +
> > > > + mode = (regval & mask) ? rpm : percent;
> > > > + coeffs = info->get_fan_coeffs(info, index, mode,
> > > > + pmbus_fan_registers[f]);
> > > > + sensor->coeffs = coeffs;
> > > > + }
> > +
> > > > ret = pmbus_add_fan_ctrl(client, data, index, page, f,
> > > > - regval);
> > > > + regval, coeffs);
> > > > if (ret < 0)
> > > > return ret;
> >
> >
>
>
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values
2017-07-12 1:20 ` Andrew Jeffery
(?)
@ 2017-07-12 3:20 ` Guenter Roeck
-1 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2017-07-12 3:20 UTC (permalink / raw)
To: Andrew Jeffery, linux-hwmon
Cc: jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
On 07/11/2017 06:20 PM, Andrew Jeffery wrote:
> On Tue, 2017-07-11 at 06:31 -0700, Guenter Roeck wrote:
>> On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
>>> Some PMBus chips, such as the MAX31785, use different coefficients for
>>> FAN_COMMAND_[1-4] depending on whether the fan is in PWM (percent duty)
>>> or RPM mode. Add a callback to allow the driver to provide the
>>> applicable coefficients to avoid imposing on devices that don't have
>>> this requirement.
>>>
>>
>> Why not just introduce another class, such as PSC_PWM ?
>
> I considered this up front however I wasn't sure where the PMBus sensor
> classes were modelled from. The PMBus spec generally doesn't discuss
The classes are modeled from my brain, so we can do whatever we want with them.
> PMW beyond the concept of fans, and given PSC_FAN already existed I had
> a vague feeling that introducing PSC_PWM might not be the way to go. It
> also means that PSC_FAN is implicitly RPM in some circumstances, or
> both RPM and PWM in others, and wasn't previously contrasted against
> PWM as PWM-specific configuration wasn't an option.
>
> However if it's reasonable it should lead to a more straight forward
> patch. I'll rework and resend if it falls out nicely.
>
Please do.
Thanks,
Guenter
> Thanks,
>
> Andrew
>
>>
>>>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>> ---
>>> drivers/hwmon/pmbus/pmbus.h | 18 +++++--
>>> drivers/hwmon/pmbus/pmbus_core.c | 112 ++++++++++++++++++++++++++++++++-------
>>> 2 files changed, 108 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
>>> index 927eabc1b273..338ecc8b25a4 100644
>>> --- a/drivers/hwmon/pmbus/pmbus.h
>>> +++ b/drivers/hwmon/pmbus/pmbus.h
>>> @@ -345,6 +345,12 @@ enum pmbus_sensor_classes {
>>> enum pmbus_data_format { linear = 0, direct, vid };
>>> enum vrm_version { vr11 = 0, vr12 };
>>>
>>> +struct pmbus_coeffs {
>>>>> + int m; /* mantissa for direct data format */
>>>>> + int b; /* offset */
>>>>> + int R; /* exponent */
>>> +};
>>> +
>>> struct pmbus_driver_info {
>>>>>>> int pages; /* Total number of pages */
>>>>> enum pmbus_data_format format[PSC_NUM_CLASSES];
>>> @@ -353,9 +359,7 @@ struct pmbus_driver_info {
>>>>> * Support one set of coefficients for each sensor type
>>>>> * Used for chips providing data in direct mode.
>>>>> */
>>>>>>> - int m[PSC_NUM_CLASSES]; /* mantissa for direct data format */
>>>>>>> - int b[PSC_NUM_CLASSES]; /* offset */
>>>>>>> - int R[PSC_NUM_CLASSES]; /* exponent */
>>>>> + struct pmbus_coeffs coeffs[PSC_NUM_CLASSES];
>>>
>>>>>>> u32 func[PMBUS_PAGES]; /* Functionality, per page */
>>>>> /*
>>> @@ -382,6 +386,14 @@ struct pmbus_driver_info {
>>>>> int (*identify)(struct i2c_client *client,
>>>>> struct pmbus_driver_info *info);
>>>
>>>>> + /*
>>>>> + * If a fan's coefficents change over time (e.g. between RPM and PWM
>>>>> + * mode), then the driver can provide a function for retrieving the
>>>>> + * currently applicable coefficients.
>>>>> + */
>>>>> + const struct pmbus_coeffs *(*get_fan_coeffs)(
>>>>> + const struct pmbus_driver_info *info, int index,
>>>>> + enum pmbus_fan_mode mode, int command);
>>>>> /* Allow the driver to interpret the fan command value */
>>>>> int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
>>>>> int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
>>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>>> index 3b0a55bbbd2c..4ff6a1fd5cce 100644
>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>>> @@ -58,10 +58,11 @@
>>> struct pmbus_sensor {
>>>>> struct pmbus_sensor *next;
>>>>>>> char name[PMBUS_NAME_SIZE]; /* sysfs sensor name */
>>>>> - struct device_attribute attribute;
>>>>> + struct sensor_device_attribute attribute;
>>>>>>> u8 page; /* page number */
>>>>>>> u16 reg; /* register */
>>>>>>> enum pmbus_sensor_classes class; /* sensor class */
>>>>> + const struct pmbus_coeffs *coeffs;
>>>>>>> bool update; /* runtime sensor update needed */
>>>>>>> int data; /* Sensor data.
>>>>> Negative if there was a read error */
>>> @@ -89,6 +90,7 @@ struct pmbus_fan_ctrl {
>>>>> u8 id;
>>>>> u8 config;
>>>>> u16 command;
>>>>> + const struct pmbus_coeffs *coeffs;
>>> };
>>> #define to_pmbus_fan_ctrl_attr(_attr) \
>>>>> container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
>>> @@ -511,9 +513,15 @@ static long pmbus_reg2data_direct(struct pmbus_data *data,
>>>>> long val = (s16) sensor->data;
>>>>> long m, b, R;
>>>
>>>>> - m = data->info->m[sensor->class];
>>>>> - b = data->info->b[sensor->class];
>>>>> - R = data->info->R[sensor->class];
>>>>> + if (sensor->coeffs) {
>>>>> + m = sensor->coeffs->m;
>>>>> + b = sensor->coeffs->b;
>>>>> + R = sensor->coeffs->R;
>>>>> + } else {
>>>>> + m = data->info->coeffs[sensor->class].m;
>>>>> + b = data->info->coeffs[sensor->class].b;
>>>>> + R = data->info->coeffs[sensor->class].R;
>>>>> + }
>>>
>>>>> if (m == 0)
>>>>> return 0;
>>> @@ -663,9 +671,15 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data,
>>> {
>>>>> long m, b, R;
>>>
>>>>> - m = data->info->m[sensor->class];
>>>>> - b = data->info->b[sensor->class];
>>>>> - R = data->info->R[sensor->class];
>>>>> + if (sensor->coeffs) {
>>>>> + m = sensor->coeffs->m;
>>>>> + b = sensor->coeffs->b;
>>>>> + R = sensor->coeffs->R;
>>>>> + } else {
>>>>> + m = data->info->coeffs[sensor->class].m;
>>>>> + b = data->info->coeffs[sensor->class].b;
>>>>> + R = data->info->coeffs[sensor->class].R;
>>>>> + }
>>>
>>>>> /* Power is in uW. Adjust R and b. */
>>>>> if (sensor->class == PSC_POWER) {
>>> @@ -796,7 +810,9 @@ static ssize_t pmbus_show_sensor(struct device *dev,
>>>>> struct device_attribute *devattr, char *buf)
>>> {
>>>>> struct pmbus_data *data = pmbus_update_device(dev);
>>>>> - struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
>>>>> + struct pmbus_sensor *sensor;
>>> +
>>>>> + sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
>>>
>>>>> if (sensor->data < 0)
>>>>> return sensor->data;
>>> @@ -810,12 +826,14 @@ static ssize_t pmbus_set_sensor(struct device *dev,
>>> {
>>>>> struct i2c_client *client = to_i2c_client(dev->parent);
>>>>> struct pmbus_data *data = i2c_get_clientdata(client);
>>>>> - struct pmbus_sensor *sensor = to_pmbus_sensor(devattr);
>>>>> + struct pmbus_sensor *sensor;
>>>>> ssize_t rv = count;
>>>>> long val = 0;
>>>>> int ret;
>>>>> u16 regval;
>>>
>>>>> + sensor = to_pmbus_sensor(to_sensor_dev_attr(devattr));
>>> +
>>>>> if (kstrtol(buf, 10, &val) < 0)
>>>>> return -EINVAL;
>>>
>>> @@ -856,6 +874,7 @@ static ssize_t pmbus_show_fan_command(struct device *dev,
>>>>> }
>>>
>>>>> sensor.class = PSC_FAN;
>>>>> + sensor.coeffs = fan->coeffs;
>>>>> if (mode == percent)
>>>>> sensor.data = fan->command * 255 / 100;
>>>>> else
>>> @@ -882,6 +901,29 @@ static ssize_t pmbus_show_pwm(struct device *dev,
>>>>> buf);
>>> }
>>>
>>> +static int pmbus_update_fan_coeffs(struct pmbus_data *data,
>>>>> + struct pmbus_fan_ctrl *fan,
>>>>> + enum pmbus_fan_mode mode)
>>> +{
>>>>> + const struct pmbus_driver_info *info = data->info;
>>>>> + struct pmbus_sensor *curr = data->sensors;
>>> +
>>>>> + fan->coeffs = info->get_fan_coeffs(info, fan->index, mode,
>>>>> + PMBUS_FAN_COMMAND_1 + fan->id);
>>> +
>>>>> + while (curr) {
>>>>> + if (curr->class == PSC_FAN &&
>>>>> + curr->attribute.index == fan->index) {
>>>>> + curr->coeffs = info->get_fan_coeffs(info, fan->index,
>>>>> + mode, curr->reg);
>>>>> + }
>>> +
>>>>> + curr = curr->next;
>>>>> + }
>>> +
>>>>> + return 0;
>>> +}
>>> +
>>> static ssize_t pmbus_set_fan_command(struct device *dev,
>>>>> enum pmbus_fan_mode mode,
>>>>> struct pmbus_fan_ctrl *fan,
>>> @@ -889,6 +931,7 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
>>> {
>>>>> struct i2c_client *client = to_i2c_client(dev->parent);
>>>>> struct pmbus_data *data = i2c_get_clientdata(client);
>>>>> + const struct pmbus_driver_info *info = data->info;
>>>>> int config_addr, command_addr;
>>>>> struct pmbus_sensor sensor;
>>>>> ssize_t rv;
>>> @@ -899,7 +942,13 @@ static ssize_t pmbus_set_fan_command(struct device *dev,
>>>
>>>>> mutex_lock(&data->update_lock);
>>>
>>>>> + if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
>>>>> + pmbus_update_fan_coeffs(data, fan, mode);
>>>>> + sensor.coeffs = fan->coeffs;
>>>>> + }
>>> +
>>>>> sensor.class = PSC_FAN;
>>>>> + sensor.attribute.index = fan->index;
>>>
>>>>> val = pmbus_data2reg(data, &sensor, val);
>>>
>>> @@ -968,6 +1017,8 @@ static ssize_t pmbus_show_pwm_enable(struct device *dev,
>>>>> struct pmbus_sensor sensor = {
>>>>> .class = PSC_FAN,
>>>>> .data = fan->command,
>>>>> + .attribute.index = fan->index,
>>>>> + .coeffs = fan->coeffs,
>>>>> };
>>>>> long command;
>>>
>>> @@ -992,6 +1043,7 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
>>>>> struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
>>>>> struct i2c_client *client = to_i2c_client(dev->parent);
>>>>> struct pmbus_data *data = i2c_get_clientdata(client);
>>>>> + const struct pmbus_driver_info *info = data->info;
>>>>> int config_addr, command_addr;
>>>>> struct pmbus_sensor sensor;
>>>>> ssize_t rv = count;
>>> @@ -1003,15 +1055,21 @@ static ssize_t pmbus_set_pwm_enable(struct device *dev,
>>>>> mutex_lock(&data->update_lock);
>>>
>>>>> sensor.class = PSC_FAN;
>>>>> + sensor.attribute.index = fan->index;
>>> +
>>>>> + if (info->format[PSC_FAN] == direct && info->get_fan_coeffs) {
>>>>> + pmbus_update_fan_coeffs(data, fan, percent);
>>>>> + sensor.coeffs = fan->coeffs;
>>>>> + }
>>>
>>>>> config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
>>>>> command_addr = config_addr + 1 + (fan->id & 1);
>>>
>>>>> - if (data->info->set_pwm_mode) {
>>>>> + if (info->set_pwm_mode) {
>>>>> u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
>>>>> u16 command = fan->command;
>>>
>>>>> - rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
>>>>> + rv = info->set_pwm_mode(fan->id, mode, &config, &command);
>>>>> if (rv < 0)
>>>>> goto done;
>>>
>>> @@ -1138,7 +1196,7 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
>>>>> sensor = devm_kzalloc(data->dev, sizeof(*sensor), GFP_KERNEL);
>>>>> if (!sensor)
>>>>> return NULL;
>>>>> - a = &sensor->attribute;
>>>>> + a = &sensor->attribute.dev_attr;
>>>
>>>>> snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",
>>>>> name, seq, type);
>>> @@ -1146,9 +1204,9 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data,
>>>>> sensor->reg = reg;
>>>>> sensor->class = class;
>>>>> sensor->update = update;
>>>>> - pmbus_dev_attr_init(a, sensor->name,
>>>>> + pmbus_attr_init(&sensor->attribute, sensor->name,
>>>>> readonly ? S_IRUGO : S_IRUGO | S_IWUSR,
>>>>> - pmbus_show_sensor, pmbus_set_sensor);
>>>>> + pmbus_show_sensor, pmbus_set_sensor, seq);
>>>
>>>>> if (pmbus_add_attribute(data, &a->attr))
>>>>> return NULL;
>>> @@ -1886,7 +1944,7 @@ 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)
>>>>> + u8 config, const struct pmbus_coeffs *coeffs)
>>> {
>>>>> struct pmbus_fan_ctrl *fan;
>>>>> int rv;
>>> @@ -1898,6 +1956,7 @@ static int pmbus_add_fan_ctrl(struct i2c_client *client,
>>>>> fan->index = index;
>>>>> fan->page = page;
>>>>> fan->id = id;
>>>>> + fan->coeffs = coeffs;
>>>>> fan->config = config;
>>>
>>>>> rv = _pmbus_read_word_data(client, page,
>>> @@ -1921,6 +1980,8 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
>>>>> struct pmbus_data *data)
>>> {
>>>>> const struct pmbus_driver_info *info = data->info;
>>>>> + const struct pmbus_coeffs *coeffs = NULL;
>>>>> + enum pmbus_fan_mode mode;
>>>>> int index = 1;
>>>>> int page;
>>>>> int ret;
>>> @@ -1929,6 +1990,7 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
>>>>> int f;
>>>
>>>>> for (f = 0; f < ARRAY_SIZE(pmbus_fan_registers); f++) {
>>>>> + struct pmbus_sensor *sensor;
>>>>> int regval;
>>>
>>>>> if (!(info->func[page] & pmbus_fan_flags[f]))
>>> @@ -1949,13 +2011,25 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
>>>>> (!(regval & (PB_FAN_1_INSTALLED >> ((f & 1) * 4)))))
>>>>> continue;
>>>
>>>>> - if (pmbus_add_sensor(data, "fan", "input", index,
>>>>> - page, pmbus_fan_registers[f],
>>>>> - PSC_FAN, true, true) == NULL)
>>>>> + sensor = pmbus_add_sensor(data, "fan", "input", index,
>>>>> + page, pmbus_fan_registers[f],
>>>>> + PSC_FAN, true, true);
>>>>> + if (!sensor)
>>>>> return -ENOMEM;
>>>
>>>>> + /* Add coeffs here as we have access to the fan mode */
>>>>> + if (info->format[PSC_FAN] == direct &&
>>>>> + info->get_fan_coeffs) {
>>>>> + const u16 mask = PB_FAN_1_RPM >> ((f & 1) * 4);
>>> +
>>>>> + mode = (regval & mask) ? rpm : percent;
>>>>> + coeffs = info->get_fan_coeffs(info, index, mode,
>>>>> + pmbus_fan_registers[f]);
>>>>> + sensor->coeffs = coeffs;
>>>>> + }
>>> +
>>>>> ret = pmbus_add_fan_ctrl(client, data, index, page, f,
>>>>> - regval);
>>>>> + regval, coeffs);
>>>>> if (ret < 0)
>>>>> return ret;
>>>
>>>
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
2017-07-12 0:39 ` Andrew Jeffery
(?)
@ 2017-07-12 3:43 ` Guenter Roeck
2017-07-12 7:01 ` Andrew Jeffery
-1 siblings, 1 reply; 16+ messages in thread
From: Guenter Roeck @ 2017-07-12 3:43 UTC (permalink / raw)
To: Andrew Jeffery, linux-hwmon
Cc: jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
On 07/11/2017 05:39 PM, Andrew Jeffery wrote:
> On Tue, 2017-07-11 at 06:40 -0700, Guenter Roeck wrote:
>> On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
>>> Augment PMBus support to include control of fans via the
>>> FAN_COMMAND_[1-4] registers, both in RPM and PWM modes. The behaviour
>>> of FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4] are tightly coupled, and
>>> their interactions do not fit the existing use of struct pmbus_sensor.
>>> The patch introduces struct pmbus_fan_ctrl to distinguish from the
>>> simple sensor case, along with associated sysfs show/set implementations.
>>>
>>> Further, the interpreting the value of FAN_COMMAND_[1-4] depends on both
>>> the current fan mode (RPM or PWM, as configured in
>>> FAN_CONFIG_{1_2,3_4}), and the device-specific behaviour for the
>>> register. For example, the MAX31785 chip defines the following:
>>>
>>> PWM (m = 1, b = 0, R = 2):
>>> 0x0000 - 0x2710: 0 - 100% fan PWM duty cycle
>>> 0x2711 - 0x7fff: 100% fan PWM duty cycle
>>> 0x8000 - 0xffff: Ignore FAN_COMMAND_[1-4], use automatic fan control
>>>
>>> RPM (m = 1, b = 0, R = 0):
>>> 0x0000 - 0x7FFF: 0 - 32,767 RPM
>>> 0x8000 - 0xFFFF: Ignore FAN_COMMAND_[1-4], use automatic fan control
>>>
>>> To handle the device-specific interpretation of the FAN_COMMAND_[1-4],
>>> add an optional callbacks to the info struct to get/set the 'mode'
>>> value required for the pwm[1-n]_enable sysfs attribute. A fallback
>>> calculation exists if the callbacks are not populated; the fallback
>>> ignores device-specific ranges and tries to determine a reasonable value
>>> from FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4].
>>>
>>
>> This seems overly complex, but unfortunately I don't have time for a detailed
>> analysis right now.
>
> No worries. It turned out more complex than I was hoping as well, and I
> am keen to hear any insights to trim it down.
>
>> Couple of comments below.
>
> Yep, thanks for taking a look.
>
>>
>> Guenter
>>
>>>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>> ---
>>> drivers/hwmon/pmbus/pmbus.h | 7 +
>>> drivers/hwmon/pmbus/pmbus_core.c | 335 +++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 342 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
>>> index bfcb13bae34b..927eabc1b273 100644
>>> --- a/drivers/hwmon/pmbus/pmbus.h
>>> +++ b/drivers/hwmon/pmbus/pmbus.h
>>> @@ -223,6 +223,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)
>>> */
>>> @@ -380,6 +382,11 @@ struct pmbus_driver_info {
>>>>> int (*identify)(struct i2c_client *client,
>>>>> struct pmbus_driver_info *info);
>>>
>>>>> + /* Allow the driver to interpret the fan command value */
>>>>> + int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
>>>>> + int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
>>>>> + u16 *fan_command);
>>> +
>>
>> It is not entirely obvious to me why this would require new callback functions.
>> Can you overload PMBUS_FAN_CONFIG_12 / PMBUS_FAN_CONFIG_34 or, if that does not
>> work for some reason, introduce a virtual register, such as PMBUS_VIRT_PWM_MODE ?
>
> Can you expand on the thought of overloading PMBUS_FAN_CONFIG_{12,34}?
>
Every register/command can be implemented in the front end driver in its read/write
functions. For example, see max34440_read_byte_data(), which replaces some of the
status registers. ucd9000.c actually overrides PMBUS_FAN_CONFIG_12 and
PMBUS_FAN_CONFIG_34.
> Regarding virtual registers, I saw references to them whilst I was
> working my way through the core code but didn't stop to investigate.
> I'll take a deeper look.
>
Virtual registers/commands are meant to "standardize" non-standard PMBus commands.
For example, PMBus provides no means to read the average/minimum/maximum temperature.
Modeling the respective attributes using PMBUS_VIRT_READ_TEMP_AVG, PMBUS_VIRT_READ_TEMP_MIN,
and PMBUS_VIRT_READ_TEMP_MAX, and providing driver-specific read/write functions
which map the virtual commands to real chip registers makes the code much simpler
than per-attribute callbacks. With virtual commands, the core only needs entries such as
}, {
.reg = PMBUS_VIRT_READ_TEMP_MIN,
.attr = "lowest",
}, {
.reg = PMBUS_VIRT_READ_TEMP_AVG,
.attr = "average",
}, {
.reg = PMBUS_VIRT_READ_TEMP_MAX,
.attr = "highest",
to support such non-standard attributes. Imagine how that would look like
if each of the supported virtual commands would be implemented as callback.
> However, the addition of the callbacks was driven by the behaviour of
> the MAX31785, where some values written to PMBUS_FAN_COMMAND_1 trigger
> automated control, while others retain manual control. Patch 4/4 should
> provide a bit more context, though I've also outlined the behaviour in
> the commit message for this patch. I don't have a lot of experience
> with PMBus devices so I don't have a good idea if there's a better way
> to capture the behaviour that isn't so unconstrained in its approach.
>
Many pmbus commands have side effects. I don't see how an explicit callback
would be different to overloading a standard register or to providing a virtual
register/command, whichever is more convenient.
>>
>>>>> /* Regulator functionality, if supported by this chip driver. */
>>>>> int num_regulators;
>>>>> const struct regulator_desc *reg_desc;
>>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>>> index ba59eaef2e07..3b0a55bbbd2c 100644
>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>> +++ b/drivers/hwmon/pmbus/pmbus_core.c
>>> @@ -69,6 +69,38 @@ struct pmbus_sensor {
>>> #define to_pmbus_sensor(_attr) \
>>>>> container_of(_attr, struct pmbus_sensor, attribute)
>>>
>>>>> +#define PB_FAN_CONFIG_RPM PB_FAN_2_RPM
>>> +#define PB_FAN_CONFIG_INSTALLED PB_FAN_2_INSTALLEDBUS_VIRT_
>>
>> Something seems odd here. PB_FAN_2_INSTALLEDBUS_VIRT_ ?
>
> Yes, that's busted. Not sure what went wrong, but I'll clean it up.
>
>>
>>>>> +#define PB_FAN_CONFIG_MASK(i) (0xff << (4 * !((i) & 1)))
>>>>> +#define PB_FAN_CONFIG_GET(i, n) (((n) >> (4 * !((i) & 1))) & 0xff)
>>>>> +#define PB_FAN_CONFIG_PUT(i, n) (((n) & 0xff) << (4 * !((i) & 1)))
>>> +
>>
>> Aren't there standard bit manipulation macros for that ? Either case, this is just to avoid
>> having to use the existing defines.
>
> As I store the configuration for each fan in a struct pmbus_fan_ctrl
> dedicated to the fan, I reasoned that intermediate code should not have
I rather wonder if pmbus_fan_ctrl is needed in the first place. The notion of
local 'struct pmbus_sensor' variables seems really messy. I think I'll really
have to spend some time on this to see if and how it can be simplified;
it just should not be that complex.
Thanks,
Guenter
> to deal with the details of which nibble to access with respect to the
> fan's (per-page) ID. Rather, code reading or writing
> PMBUS_FAN_COMMAND_[1-4] should deal with ensuring the correct values
> are provided.
>
>> Ok, but then I think it would make more sense to
>> make it generic, ie change the core to not use PB_FAN_2_RPM / PB_FAN_1_RPM etc.
>> but PB_FAN_RPM(index) everywhere.
>
> I'll make the change throughout pmbus core.
>
>>
>>> +struct pmbus_fan_ctrl_attr {
>>>>> + struct device_attribute attribute;
>>>>> + char name[PMBUS_NAME_SIZE];
>>> +};
>>> +
>>> +struct pmbus_fan_ctrl {
>>>>> + struct pmbus_fan_ctrl_attr fan_target;
>>>>> + struct pmbus_fan_ctrl_attr pwm;
>>>>> + struct pmbus_fan_ctrl_attr pwm_enable;
>>>>> + int index;
>>>>> + u8 page;
>>>>> + u8 id;
>>>>> + u8 config;
>>>>> + u16 command;
>>> +};
>>> +#define to_pmbus_fan_ctrl_attr(_attr) \
>>>>> + container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
>>> +#define fan_target_to_pmbus_fan_ctrl(_attr) \
>>>>> + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
>>>>> + fan_target)
>>> +#define pwm_to_pmbus_fan_ctrl(_attr) \
>>>>> + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, pwm)
>>> +#define pwm_enable_to_pmbus_fan_ctrl(_attr) \
>>>>> + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
>>>>> + pwm_enable)
>>> +
>>> struct pmbus_boolean {
>>>>>>> char name[PMBUS_NAME_SIZE]; /* sysfs boolean name */
>>>>> struct sensor_device_attribute attribute;
>>> @@ -806,6 +838,219 @@ static ssize_t pmbus_show_label(struct device *dev,
>>>>> return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
>>> }
>>>
>>> +static ssize_t pmbus_show_fan_command(struct device *dev,
>>>>> + enum pmbus_fan_mode mode,
>>>>> + struct pmbus_fan_ctrl *fan, char *buf)
>>> +{
>>>>> + struct i2c_client *client = to_i2c_client(dev->parent);
>>>>> + struct pmbus_data *data = i2c_get_clientdata(client);
>>>>> + struct pmbus_sensor sensor;
>>>>> + long val;
>>> +
>>>>> + mutex_lock(&data->update_lock);
>>> +
>>>>> + if ((mode == percent && (fan->config & PB_FAN_CONFIG_RPM)) ||
>>>>> + (mode == rpm && !(fan->config & PB_FAN_CONFIG_RPM))) {
>>>>> + mutex_unlock(&data->update_lock);
>>> + return -ENOTSUPP; /* XXX: This seems dodgy, but what to do? */
>>
>> Not create the attribute in question in the first place, or return 0. The above
>> messes up the 'sensors' command.
>
> I think returning 0 is the only valid option of the two, given that we
> can dynamically switch between RPM and PWM modes.
>
> Thanks for the feedback.
>
> Andrew
>
>>
>>>>> + }
>>> +
>>>>> + sensor.class = PSC_FAN;
>>>>> + if (mode == percent)
>>>>> + sensor.data = fan->command * 255 / 100;
>>>>> + else
>>>>> + sensor.data = fan->command;
>>> +
>>>>> + val = pmbus_reg2data(data, &sensor);
>>> +
>>>>> + mutex_unlock(&data->update_lock);
>>> +
>>>>> + return snprintf(buf, PAGE_SIZE, "%ld\n", val);
>>> +}
>>> +
>>> +static ssize_t pmbus_show_fan_target(struct device *dev,
>>>>> + struct device_attribute *da, char *buf)
>>> +{
>>>>> + return pmbus_show_fan_command(dev, rpm,
>>>>> + fan_target_to_pmbus_fan_ctrl(da), buf);
>>> +}
>>> +
>>> +static ssize_t pmbus_show_pwm(struct device *dev,
>>>>> + struct device_attribute *da, char *buf)
>>> +{
>>>>> + return pmbus_show_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
>>>>> + buf);
>>> +}
>>> +
>>> +static ssize_t pmbus_set_fan_command(struct device *dev,
>>>>> + enum pmbus_fan_mode mode,
>>>>> + struct pmbus_fan_ctrl *fan,
>>>>> + const char *buf, ssize_t count)
>>> +{
>>>>> + struct i2c_client *client = to_i2c_client(dev->parent);
>>>>> + struct pmbus_data *data = i2c_get_clientdata(client);
>>>>> + int config_addr, command_addr;
>>>>> + struct pmbus_sensor sensor;
>>>>> + ssize_t rv;
>>>>> + long val;
>>> +
>>>>> + if (kstrtol(buf, 10, &val) < 0)
>>>>> + return -EINVAL;
>>> +
>>>>> + mutex_lock(&data->update_lock);
>>> +
>>>>> + sensor.class = PSC_FAN;
>>> +
>>>>> + val = pmbus_data2reg(data, &sensor, val);
>>> +
>>>>> + if (mode == percent)
>>>>> + val = val * 100 / 255;
>>> +
>>>>> + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
>>>>> + command_addr = config_addr + 1 + (fan->id & 1);
>>> +
>>>>> + if (mode == rpm)
>>>>> + fan->config |= PB_FAN_CONFIG_RPM;
>>>>> + else
>>>>> + fan->config &= ~PB_FAN_CONFIG_RPM;
>>> +
>>>>> + rv = pmbus_update_byte_data(client, fan->page, config_addr,
>>>>> + PB_FAN_CONFIG_PUT(fan->id, fan->config),
>>>>> + PB_FAN_CONFIG_MASK(fan->id));
>>>>> + if (rv < 0)
>>>>> + goto done;
>>> +
>>>>> + fan->command = val;
>>>>> + rv = pmbus_write_word_data(client, fan->page, command_addr,
>>>>> + fan->command);
>>> +
>>> +done:
>>>>> + mutex_unlock(&data->update_lock);
>>> +
>>>>> + if (rv < 0)
>>>>> + return rv;
>>> +
>>>>> + return count;
>>> +}
>>> +
>>> +static ssize_t pmbus_set_fan_target(struct device *dev,
>>>>> + struct device_attribute *da,
>>>>> + const char *buf, size_t count)
>>> +{
>>>>> + return pmbus_set_fan_command(dev, rpm,
>>>>> + fan_target_to_pmbus_fan_ctrl(da), buf,
>>>>> + count);
>>> +}
>>> +
>>> +static ssize_t pmbus_set_pwm(struct device *dev, struct device_attribute *da,
>>>>> + const char *buf, size_t count)
>>> +{
>>>>> + return pmbus_set_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
>>>>> + buf, count);
>>> +}
>>> +
>>> +static ssize_t pmbus_show_pwm_enable(struct device *dev,
>>>>> + struct device_attribute *da, char *buf)
>>> +{
>>>>> + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
>>>>> + struct i2c_client *client = to_i2c_client(dev->parent);
>>>>> + struct pmbus_data *data = i2c_get_clientdata(client);
>>>>> + long mode;
>>> +
>>>>> + mutex_lock(&data->update_lock);
>>> +
>>> +
>>>>> + if (data->info->get_pwm_mode) {
>>>>> + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
>>> +
>>>>> + mode = data->info->get_pwm_mode(fan->id, config, fan->command);
>>>>> + } else {
>>>>> + struct pmbus_sensor sensor = {
>>>>> + .class = PSC_FAN,
>>>>> + .data = fan->command,
>>>>> + };
>>>>> + long command;
>>> +
>>>>> + command = pmbus_reg2data(data, &sensor);
>>> +
>>>>> + /* XXX: Need to do something sensible */
>>>>> + if (fan->config & PB_FAN_CONFIG_RPM)
>>>>> + mode = 2;
>>>>> + else
>>>>> + mode = (command >= 0 && command < 100);
>>>>> + }
>>> +
>>>>> + mutex_unlock(&data->update_lock);
>>> +
>>>>> + return snprintf(buf, PAGE_SIZE, "%ld\n", mode);
>>> +}
>>> +
>>> +static ssize_t pmbus_set_pwm_enable(struct device *dev,
>>>>> + struct device_attribute *da,
>>>>> + const char *buf, size_t count)
>>> +{
>>>>> + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
>>>>> + struct i2c_client *client = to_i2c_client(dev->parent);
>>>>> + struct pmbus_data *data = i2c_get_clientdata(client);
>>>>> + int config_addr, command_addr;
>>>>> + struct pmbus_sensor sensor;
>>>>> + ssize_t rv = count;
>>>>> + long mode;
>>> +
>>>>> + if (kstrtol(buf, 10, &mode) < 0)
>>>>> + return -EINVAL;
>>> +
>>>>> + mutex_lock(&data->update_lock);
>>> +
>>>>> + sensor.class = PSC_FAN;
>>> +
>>>>> + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
>>>>> + command_addr = config_addr + 1 + (fan->id & 1);
>>> +
>>>>> + if (data->info->set_pwm_mode) {
>>>>> + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
>>>>> + u16 command = fan->command;
>>> +
>>>>> + rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
>>>>> + if (rv < 0)
>>>>> + goto done;
>>> +
>>>>> + fan->config = PB_FAN_CONFIG_GET(fan->id, config);
>>>>> + fan->command = command;
>>>>> + } else {
>>>>> + fan->config &= ~PB_FAN_CONFIG_RPM;
>>>>> + switch (mode) {
>>>>> + case 0:
>>>>> + case 1:
>>>>> + /* XXX: Safe at least? */
>>>>> + fan->command = pmbus_data2reg(data, &sensor, 100);
>>>>> + break;
>>>>> + case 2:
>>>>> + default:
>>>>> + /* XXX: Safe at least? */
>>>>> + fan->command = 0xffff;
>>>>> + break;
>>>>> + }
>>>>> + }
>>> +
>>>>> + rv = pmbus_update_byte_data(client, fan->page, config_addr,
>>>>> + PB_FAN_CONFIG_PUT(fan->id, fan->config),
>>>>> + PB_FAN_CONFIG_MASK(fan->id));
>>>>> + if (rv < 0)
>>>>> + goto done;
>>> +
>>>>> + rv = pmbus_write_word_data(client, fan->page, command_addr,
>>>>> + fan->command);
>>> +
>>> +done:
>>>>> + mutex_unlock(&data->update_lock);
>>> +
>>>>> + if (rv < 0)
>>>>> + return rv;
>>> +
>>>>> + return count;
>>> +}
>>> +
>>> static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
>>> {
>>>>> if (data->num_attributes >= data->max_attributes - 1) {
>>> @@ -1094,6 +1339,51 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
>>>>> return 0;
>>> }
>>>
>>> +static int pmbus_add_fan_ctrl_attr(struct pmbus_data *data,
>>>>> + struct pmbus_fan_ctrl_attr *fa,
>>>>> + const char *name_fmt,
>>>>> + ssize_t (*show)(struct device *dev,
>>>>> + struct device_attribute *attr,
>>>>> + char *buf),
>>>>> + ssize_t (*store)(struct device *dev,
>>>>> + struct device_attribute *attr,
>>>>> + const char *buf, size_t count),
>>>>> + int idx)
>>> +{
>>>>> + struct device_attribute *da;
>>> +
>>>>> + da = &fa->attribute;
>>> +
>>>>> + snprintf(fa->name, sizeof(fa->name), name_fmt, idx);
>>>>> + pmbus_dev_attr_init(da, fa->name, 0644, show, store);
>>> +
>>>>> + return pmbus_add_attribute(data, &da->attr);
>>> +}
>>> +
>>> +static inline int pmbus_add_fan_target_attr(struct pmbus_data *data,
>>>>> + struct pmbus_fan_ctrl *fan)
>>> +{
>>>>> + return pmbus_add_fan_ctrl_attr(data, &fan->fan_target, "fan%d_target",
>>>>> + pmbus_show_fan_target,
>>>>> + pmbus_set_fan_target, fan->index);
>>> +}
>>> +
>>> +static inline int pmbus_add_pwm_attr(struct pmbus_data *data,
>>>>> + struct pmbus_fan_ctrl *fan)
>>> +{
>>> +
>>>>> + return pmbus_add_fan_ctrl_attr(data, &fan->pwm, "pwm%d", pmbus_show_pwm,
>>>>> + pmbus_set_pwm, fan->index);
>>> +}
>>> +
>>> +static inline int pmbus_add_pwm_enable_attr(struct pmbus_data *data,
>>>>> + struct pmbus_fan_ctrl *fan)
>>> +{
>>>>> + return pmbus_add_fan_ctrl_attr(data, &fan->pwm_enable, "pwm%d_enable",
>>>>> + pmbus_show_pwm_enable,
>>>>> + pmbus_set_pwm_enable, fan->index);
>>> +}
>>> +
>>> static const struct pmbus_limit_attr vin_limit_attrs[] = {
>>>>> {
>>>>> .reg = PMBUS_VIN_UV_WARN_LIMIT,
>>> @@ -1565,6 +1855,13 @@ static const int pmbus_fan_config_registers[] = {
>>>>> 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,
>>> +};
>>> +
>>> static const int pmbus_fan_status_registers[] = {
>>>>> PMBUS_STATUS_FAN_12,
>>>>> PMBUS_STATUS_FAN_12,
>>> @@ -1587,6 +1884,39 @@ 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_fan_ctrl *fan;
>>>>> + int rv;
>>> +
>>>>> + fan = devm_kzalloc(data->dev, sizeof(*fan), GFP_KERNEL);
>>>>> + if (!fan)
>>>>> + return -ENOMEM;
>>> +
>>>>> + fan->index = index;
>>>>> + fan->page = page;
>>>>> + fan->id = id;
>>>>> + fan->config = config;
>>> +
>>>>> + rv = _pmbus_read_word_data(client, page,
>>>>> + pmbus_fan_command_registers[id]);
>>>>> + if (rv < 0)
>>>>> + return rv;
>>>>> + fan->command = rv;
>>> +
>>>>> + rv = pmbus_add_fan_target_attr(data, fan);
>>>>> + if (rv < 0)
>>>>> + return rv;
>>> +
>>>>> + rv = pmbus_add_pwm_attr(data, fan);
>>>>> + if (rv < 0)
>>>>> + return rv;
>>> +
>>>>> + return pmbus_add_pwm_enable_attr(data, fan);
>>> +}
>>> +
>>> static int pmbus_add_fan_attributes(struct i2c_client *client,
>>>>> struct pmbus_data *data)
>>> {
>>> @@ -1624,6 +1954,11 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
>>>>> PSC_FAN, true, true) == NULL)
>>>>> return -ENOMEM;
>>>
>>>>> + 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.
>>>
>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
2017-07-12 3:43 ` Guenter Roeck
@ 2017-07-12 7:01 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-12 7:01 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon
Cc: jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
[-- Attachment #1: Type: text/plain, Size: 27152 bytes --]
On Tue, 2017-07-11 at 20:43 -0700, Guenter Roeck wrote:
> On 07/11/2017 05:39 PM, Andrew Jeffery wrote:
> > On Tue, 2017-07-11 at 06:40 -0700, Guenter Roeck wrote:
> > > On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> > > > Augment PMBus support to include control of fans via the
> > > > FAN_COMMAND_[1-4] registers, both in RPM and PWM modes. The behaviour
> > > > of FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4] are tightly coupled, and
> > > > their interactions do not fit the existing use of struct pmbus_sensor.
> > > > The patch introduces struct pmbus_fan_ctrl to distinguish from the
> > > > simple sensor case, along with associated sysfs show/set implementations.
> > > >
> > > > Further, the interpreting the value of FAN_COMMAND_[1-4] depends on both
> > > > the current fan mode (RPM or PWM, as configured in
> > > > FAN_CONFIG_{1_2,3_4}), and the device-specific behaviour for the
> > > > register. For example, the MAX31785 chip defines the following:
> > > >
> > > > PWM (m = 1, b = 0, R = 2):
> > > > 0x0000 - 0x2710: 0 - 100% fan PWM duty cycle
> > > > 0x2711 - 0x7fff: 100% fan PWM duty cycle
> > > > 0x8000 - 0xffff: Ignore FAN_COMMAND_[1-4], use automatic fan control
> > > >
> > > > RPM (m = 1, b = 0, R = 0):
> > > > 0x0000 - 0x7FFF: 0 - 32,767 RPM
> > > > 0x8000 - 0xFFFF: Ignore FAN_COMMAND_[1-4], use automatic fan control
> > > >
> > > > To handle the device-specific interpretation of the FAN_COMMAND_[1-4],
> > > > add an optional callbacks to the info struct to get/set the 'mode'
> > > > value required for the pwm[1-n]_enable sysfs attribute. A fallback
> > > > calculation exists if the callbacks are not populated; the fallback
> > > > ignores device-specific ranges and tries to determine a reasonable value
> > > > from FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4].
> > > >
> > >
> > > This seems overly complex, but unfortunately I don't have time for a detailed
> > > analysis right now.
> >
> > No worries. It turned out more complex than I was hoping as well, and I
> > am keen to hear any insights to trim it down.
> >
> > > Couple of comments below.
> >
> > Yep, thanks for taking a look.
> >
> > >
> > > Guenter
> > >
> > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > >
> > > > ---
> > > > drivers/hwmon/pmbus/pmbus.h | 7 +
> > > > drivers/hwmon/pmbus/pmbus_core.c | 335 +++++++++++++++++++++++++++++++++++++++
> > > > 2 files changed, 342 insertions(+)
> > > >
> > > > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > > > index bfcb13bae34b..927eabc1b273 100644
> > > > --- a/drivers/hwmon/pmbus/pmbus.h
> > > > +++ b/drivers/hwmon/pmbus/pmbus.h
> > > > @@ -223,6 +223,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)
> > > > */
> > > > @@ -380,6 +382,11 @@ struct pmbus_driver_info {
> > > > > > > > > > > > int (*identify)(struct i2c_client *client,
> > > > > > struct pmbus_driver_info *info);
> > > >
> > > >
> > > > > > > > > > > > + /* Allow the driver to interpret the fan command value */
> > > > > > > > > > > > + int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> > > > > > > > > > > > + int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> > > > > > + u16 *fan_command);
> > > >
> > > > +
> > >
> > > It is not entirely obvious to me why this would require new callback functions.
> > > Can you overload PMBUS_FAN_CONFIG_12 / PMBUS_FAN_CONFIG_34 or, if that does not
> > > work for some reason, introduce a virtual register, such as PMBUS_VIRT_PWM_MODE ?
> >
> > Can you expand on the thought of overloading PMBUS_FAN_CONFIG_{12,34}?
> >
>
> Every register/command can be implemented in the front end driver in its read/write
> functions. For example, see max34440_read_byte_data(), which replaces some of the
> status registers. ucd9000.c actually overrides PMBUS_FAN_CONFIG_12 and
> PMBUS_FAN_CONFIG_34.
Right; In the cover letter I mentioned I hadn't thought about how to
implement the dual tach feature of the MAX31785 at the time. After
sending the RFC series I had go at that as well, and ended up
implementing support purely in terms of the read/write callbacks with
no modifications to the core, so I've become familiar with them.
>
> > Regarding virtual registers, I saw references to them whilst I was
> > working my way through the core code but didn't stop to investigate.
> > I'll take a deeper look.
> >
>
> Virtual registers/commands are meant to "standardize" non-standard PMBus commands.
>
> For example, PMBus provides no means to read the average/minimum/maximum temperature.
> Modeling the respective attributes using PMBUS_VIRT_READ_TEMP_AVG, PMBUS_VIRT_READ_TEMP_MIN,
> and PMBUS_VIRT_READ_TEMP_MAX, and providing driver-specific read/write functions
> which map the virtual commands to real chip registers makes the code much simpler
> than per-attribute callbacks. With virtual commands, the core only needs entries such as
>
> }, {
> .reg = PMBUS_VIRT_READ_TEMP_MIN,
> .attr = "lowest",
> }, {
> .reg = PMBUS_VIRT_READ_TEMP_AVG,
> .attr = "average",
> }, {
> .reg = PMBUS_VIRT_READ_TEMP_MAX,
> .attr = "highest",
>
> to support such non-standard attributes. Imagine how that would look like
> if each of the supported virtual commands would be implemented as callback.
Indeed. Hence RFC in case I had overlooked something :) Clearly I have.
>
> > However, the addition of the callbacks was driven by the behaviour of
> > the MAX31785, where some values written to PMBUS_FAN_COMMAND_1 trigger
> > automated control, while others retain manual control. Patch 4/4 should
> > provide a bit more context, though I've also outlined the behaviour in
> > the commit message for this patch. I don't have a lot of experience
> > with PMBus devices so I don't have a good idea if there's a better way
> > to capture the behaviour that isn't so unconstrained in its approach.
> >
>
> Many pmbus commands have side effects. I don't see how an explicit callback
> would be different to overloading a standard register or to providing a virtual
> register/command, whichever is more convenient.
I'm going to experiment with the virtual registers. From your
description above and looking at the comments in pmbus.h I think I can
make something work (and drop the callbacks).
>
> > >
> > > > > > > > > > > > /* Regulator functionality, if supported by this chip driver. */
> > > > > > > > > > > > int num_regulators;
> > > > > > const struct regulator_desc *reg_desc;
> > > >
> > > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > > > index ba59eaef2e07..3b0a55bbbd2c 100644
> > > > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > > > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > > > @@ -69,6 +69,38 @@ struct pmbus_sensor {
> > > > #define to_pmbus_sensor(_attr) \
> > > > > > container_of(_attr, struct pmbus_sensor, attribute)
> > > >
> > > >
> > > > > > +#define PB_FAN_CONFIG_RPM PB_FAN_2_RPM
> > > >
> > > > +#define PB_FAN_CONFIG_INSTALLED PB_FAN_2_INSTALLEDBUS_VIRT_
> > >
> > > Something seems odd here. PB_FAN_2_INSTALLEDBUS_VIRT_ ?
> >
> > Yes, that's busted. Not sure what went wrong, but I'll clean it up.
> >
> > >
> > > > > > > > > > > > +#define PB_FAN_CONFIG_MASK(i) (0xff << (4 * !((i) & 1)))
> > > > > > > > > > > > +#define PB_FAN_CONFIG_GET(i, n) (((n) >> (4 * !((i) & 1))) & 0xff)
> > > > > > +#define PB_FAN_CONFIG_PUT(i, n) (((n) & 0xff) << (4 * !((i) & 1)))
> > > >
> > > > +
> > >
> > > Aren't there standard bit manipulation macros for that ? Either case, this is just to avoid
> > > having to use the existing defines.
> >
> > As I store the configuration for each fan in a struct pmbus_fan_ctrl
> > dedicated to the fan, I reasoned that intermediate code should not have
>
> I rather wonder if pmbus_fan_ctrl is needed in the first place. The notion of
> local 'struct pmbus_sensor' variables seems really messy. I think I'll really
> have to spend some time on this to see if and how it can be simplified;
> it just should not be that complex.
Sure. FWIW I plan on sending a follow-up RFC based on the feedback
you've given here, and I'll look to chop out pmbus_fan_ctrl. I was
suspicious of needing it as well, but was after your input on the
general approach and figured sending the patches was better than
guessing at your potential feedback.
If a follow-up isn't of interest and you'd definitely rather take on
the work up yourself, let me know.
Thanks again for taking a look.
Andrew
>
> Thanks,
> Guenter
>
> > to deal with the details of which nibble to access with respect to the
> > fan's (per-page) ID. Rather, code reading or writing
> > PMBUS_FAN_COMMAND_[1-4] should deal with ensuring the correct values
> > are provided.
> >
> > > Ok, but then I think it would make more sense to
> > > make it generic, ie change the core to not use PB_FAN_2_RPM / PB_FAN_1_RPM etc.
> > > but PB_FAN_RPM(index) everywhere.
> >
> > I'll make the change throughout pmbus core.
> >
> > >
> > > > +struct pmbus_fan_ctrl_attr {
> > > > > > > > > > > > + struct device_attribute attribute;
> > > > > > + char name[PMBUS_NAME_SIZE];
> > > >
> > > > +};
> > > > +
> > > > +struct pmbus_fan_ctrl {
> > > > > > > > > > > > + struct pmbus_fan_ctrl_attr fan_target;
> > > > > > > > > > > > + struct pmbus_fan_ctrl_attr pwm;
> > > > > > > > > > > > + struct pmbus_fan_ctrl_attr pwm_enable;
> > > > > > > > > > > > + int index;
> > > > > > > > > > > > + u8 page;
> > > > > > > > > > > > + u8 id;
> > > > > > > > > > > > + u8 config;
> > > > > > + u16 command;
> > > >
> > > > +};
> > > > +#define to_pmbus_fan_ctrl_attr(_attr) \
> > > > > > + container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
> > > >
> > > > +#define fan_target_to_pmbus_fan_ctrl(_attr) \
> > > > > > > > > > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
> > > > > > + fan_target)
> > > >
> > > > +#define pwm_to_pmbus_fan_ctrl(_attr) \
> > > > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, pwm)
> > > >
> > > > +#define pwm_enable_to_pmbus_fan_ctrl(_attr) \
> > > > > > > > > > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
> > > > > > + pwm_enable)
> > > >
> > > > +
> > > > struct pmbus_boolean {
> > > > > > > > char name[PMBUS_NAME_SIZE]; /* sysfs boolean name */
> > > > > >
> > > > > > struct sensor_device_attribute attribute;
> > > >
> > > > @@ -806,6 +838,219 @@ static ssize_t pmbus_show_label(struct device *dev,
> > > > > > return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
> > > >
> > > > }
> > > >
> > > > +static ssize_t pmbus_show_fan_command(struct device *dev,
> > > > > > > > > > > > + enum pmbus_fan_mode mode,
> > > > > > + struct pmbus_fan_ctrl *fan, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > > > > > > > + struct pmbus_sensor sensor;
> > > > > > + long val;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > > > > > > > > > + if ((mode == percent && (fan->config & PB_FAN_CONFIG_RPM)) ||
> > > > > > > > > > > > + (mode == rpm && !(fan->config & PB_FAN_CONFIG_RPM))) {
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > + return -ENOTSUPP; /* XXX: This seems dodgy, but what to do? */
> > >
> > > Not create the attribute in question in the first place, or return 0. The above
> > > messes up the 'sensors' command.
> >
> > I think returning 0 is the only valid option of the two, given that we
> > can dynamically switch between RPM and PWM modes.
> >
> > Thanks for the feedback.
> >
> > Andrew
> >
> > >
> > > > > > + }
> > > >
> > > > +
> > > > > > > > > > > > + sensor.class = PSC_FAN;
> > > > > > > > > > > > + if (mode == percent)
> > > > > > > > > > > > + sensor.data = fan->command * 255 / 100;
> > > > > > > > > > > > + else
> > > > > > + sensor.data = fan->command;
> > > >
> > > > +
> > > > > > + val = pmbus_reg2data(data, &sensor);
> > > >
> > > > +
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > + return snprintf(buf, PAGE_SIZE, "%ld\n", val);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_show_fan_target(struct device *dev,
> > > > > > + struct device_attribute *da, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_show_fan_command(dev, rpm,
> > > > > > + fan_target_to_pmbus_fan_ctrl(da), buf);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_show_pwm(struct device *dev,
> > > > > > + struct device_attribute *da, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_show_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> > > > > > + buf);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_fan_command(struct device *dev,
> > > > > > > > > > > > + enum pmbus_fan_mode mode,
> > > > > > > > > > > > + struct pmbus_fan_ctrl *fan,
> > > > > > + const char *buf, ssize_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > > > > > > > + int config_addr, command_addr;
> > > > > > > > > > > > + struct pmbus_sensor sensor;
> > > > > > > > > > > > + ssize_t rv;
> > > > > > + long val;
> > > >
> > > > +
> > > > > > > > > > > > + if (kstrtol(buf, 10, &val) < 0)
> > > > > > + return -EINVAL;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > > > + sensor.class = PSC_FAN;
> > > >
> > > > +
> > > > > > + val = pmbus_data2reg(data, &sensor, val);
> > > >
> > > > +
> > > > > > > > > > > > + if (mode == percent)
> > > > > > + val = val * 100 / 255;
> > > >
> > > > +
> > > > > > > > > > > > + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > > > + command_addr = config_addr + 1 + (fan->id & 1);
> > > >
> > > > +
> > > > > > > > > > > > + if (mode == rpm)
> > > > > > > > > > > > + fan->config |= PB_FAN_CONFIG_RPM;
> > > > > > > > > > > > + else
> > > > > > + fan->config &= ~PB_FAN_CONFIG_RPM;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> > > > > > > > > > > > + PB_FAN_CONFIG_PUT(fan->id, fan->config),
> > > > > > > > > > > > + PB_FAN_CONFIG_MASK(fan->id));
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + goto done;
> > > >
> > > > +
> > > > > > > > > > > > + fan->command = val;
> > > > > > > > > > > > + rv = pmbus_write_word_data(client, fan->page, command_addr,
> > > > > > + fan->command);
> > > >
> > > > +
> > > > +done:
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > + return count;
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_fan_target(struct device *dev,
> > > > > > > > > > > > + struct device_attribute *da,
> > > > > > + const char *buf, size_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_set_fan_command(dev, rpm,
> > > > > > > > > > > > + fan_target_to_pmbus_fan_ctrl(da), buf,
> > > > > > + count);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_pwm(struct device *dev, struct device_attribute *da,
> > > > > > + const char *buf, size_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_set_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> > > > > > + buf, count);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_show_pwm_enable(struct device *dev,
> > > > > > + struct device_attribute *da, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > + long mode;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > +
> > > > > > > > > > > > + if (data->info->get_pwm_mode) {
> > > > > > + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > >
> > > > +
> > > > > > > > > > > > + mode = data->info->get_pwm_mode(fan->id, config, fan->command);
> > > > > > > > > > > > + } else {
> > > > > > > > > > > > + struct pmbus_sensor sensor = {
> > > > > > > > > > > > + .class = PSC_FAN,
> > > > > > > > > > > > + .data = fan->command,
> > > > > > > > > > > > + };
> > > > > > + long command;
> > > >
> > > > +
> > > > > > + command = pmbus_reg2data(data, &sensor);
> > > >
> > > > +
> > > > > > > > > > > > + /* XXX: Need to do something sensible */
> > > > > > > > > > > > + if (fan->config & PB_FAN_CONFIG_RPM)
> > > > > > > > > > > > + mode = 2;
> > > > > > > > > > > > + else
> > > > > > > > > > > > + mode = (command >= 0 && command < 100);
> > > > > > + }
> > > >
> > > > +
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > + return snprintf(buf, PAGE_SIZE, "%ld\n", mode);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > > > > > > > > > + struct device_attribute *da,
> > > > > > + const char *buf, size_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > > > > > > > + int config_addr, command_addr;
> > > > > > > > > > > > + struct pmbus_sensor sensor;
> > > > > > > > > > > > + ssize_t rv = count;
> > > > > > + long mode;
> > > >
> > > > +
> > > > > > > > > > > > + if (kstrtol(buf, 10, &mode) < 0)
> > > > > > + return -EINVAL;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > > > + sensor.class = PSC_FAN;
> > > >
> > > > +
> > > > > > > > > > > > + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > > > + command_addr = config_addr + 1 + (fan->id & 1);
> > > >
> > > > +
> > > > > > > > > > > > + if (data->info->set_pwm_mode) {
> > > > > > > > > > > > + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > > > > + u16 command = fan->command;
> > > >
> > > > +
> > > > > > > > > > > > + rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + goto done;
> > > >
> > > > +
> > > > > > > > > > > > + fan->config = PB_FAN_CONFIG_GET(fan->id, config);
> > > > > > > > > > > > + fan->command = command;
> > > > > > > > > > > > + } else {
> > > > > > > > > > > > + fan->config &= ~PB_FAN_CONFIG_RPM;
> > > > > > > > > > > > + switch (mode) {
> > > > > > > > > > > > + case 0:
> > > > > > > > > > > > + case 1:
> > > > > > > > > > > > + /* XXX: Safe at least? */
> > > > > > > > > > > > + fan->command = pmbus_data2reg(data, &sensor, 100);
> > > > > > > > > > > > + break;
> > > > > > > > > > > > + case 2:
> > > > > > > > > > > > + default:
> > > > > > > > > > > > + /* XXX: Safe at least? */
> > > > > > > > > > > > + fan->command = 0xffff;
> > > > > > > > > > > > + break;
> > > > > > > > > > > > + }
> > > > > > + }
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> > > > > > > > > > > > + PB_FAN_CONFIG_PUT(fan->id, fan->config),
> > > > > > > > > > > > + PB_FAN_CONFIG_MASK(fan->id));
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + goto done;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_write_word_data(client, fan->page, command_addr,
> > > > > > + fan->command);
> > > >
> > > > +
> > > > +done:
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > + return count;
> > > >
> > > > +}
> > > > +
> > > > static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
> > > > {
> > > > > > if (data->num_attributes >= data->max_attributes - 1) {
> > > >
> > > > @@ -1094,6 +1339,51 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> > > > > > return 0;
> > > >
> > > > }
> > > >
> > > > +static int pmbus_add_fan_ctrl_attr(struct pmbus_data *data,
> > > > > > > > > > > > + struct pmbus_fan_ctrl_attr *fa,
> > > > > > > > > > > > + const char *name_fmt,
> > > > > > > > > > > > + ssize_t (*show)(struct device *dev,
> > > > > > > > > > > > + struct device_attribute *attr,
> > > > > > > > > > > > + char *buf),
> > > > > > > > > > > > + ssize_t (*store)(struct device *dev,
> > > > > > > > > > > > + struct device_attribute *attr,
> > > > > > > > > > > > + const char *buf, size_t count),
> > > > > > + int idx)
> > > >
> > > > +{
> > > > > > + struct device_attribute *da;
> > > >
> > > > +
> > > > > > + da = &fa->attribute;
> > > >
> > > > +
> > > > > > > > > > > > + snprintf(fa->name, sizeof(fa->name), name_fmt, idx);
> > > > > > + pmbus_dev_attr_init(da, fa->name, 0644, show, store);
> > > >
> > > > +
> > > > > > + return pmbus_add_attribute(data, &da->attr);
> > > >
> > > > +}
> > > > +
> > > > +static inline int pmbus_add_fan_target_attr(struct pmbus_data *data,
> > > > > > + struct pmbus_fan_ctrl *fan)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_add_fan_ctrl_attr(data, &fan->fan_target, "fan%d_target",
> > > > > > > > > > > > + pmbus_show_fan_target,
> > > > > > + pmbus_set_fan_target, fan->index);
> > > >
> > > > +}
> > > > +
> > > > +static inline int pmbus_add_pwm_attr(struct pmbus_data *data,
> > > > > > + struct pmbus_fan_ctrl *fan)
> > > >
> > > > +{
> > > > +
> > > > > > > > > > > > + return pmbus_add_fan_ctrl_attr(data, &fan->pwm, "pwm%d", pmbus_show_pwm,
> > > > > > + pmbus_set_pwm, fan->index);
> > > >
> > > > +}
> > > > +
> > > > +static inline int pmbus_add_pwm_enable_attr(struct pmbus_data *data,
> > > > > > + struct pmbus_fan_ctrl *fan)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_add_fan_ctrl_attr(data, &fan->pwm_enable, "pwm%d_enable",
> > > > > > > > > > > > + pmbus_show_pwm_enable,
> > > > > > + pmbus_set_pwm_enable, fan->index);
> > > >
> > > > +}
> > > > +
> > > > static const struct pmbus_limit_attr vin_limit_attrs[] = {
> > > > > > > > > > > > {
> > > > > > .reg = PMBUS_VIN_UV_WARN_LIMIT,
> > > >
> > > > @@ -1565,6 +1855,13 @@ static const int pmbus_fan_config_registers[] = {
> > > > > > 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,
> > > >
> > > > +};
> > > > +
> > > > static const int pmbus_fan_status_registers[] = {
> > > > > > > > > > > > PMBUS_STATUS_FAN_12,
> > > > > > PMBUS_STATUS_FAN_12,
> > > >
> > > > @@ -1587,6 +1884,39 @@ 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_fan_ctrl *fan;
> > > > > > + int rv;
> > > >
> > > > +
> > > > > > > > > > > > + fan = devm_kzalloc(data->dev, sizeof(*fan), GFP_KERNEL);
> > > > > > > > > > > > + if (!fan)
> > > > > > + return -ENOMEM;
> > > >
> > > > +
> > > > > > > > > > > > + fan->index = index;
> > > > > > > > > > > > + fan->page = page;
> > > > > > > > > > > > + fan->id = id;
> > > > > > + fan->config = config;
> > > >
> > > > +
> > > > > > > > > > > > + rv = _pmbus_read_word_data(client, page,
> > > > > > > > > > > > + pmbus_fan_command_registers[id]);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > > > > > > > + return rv;
> > > > > > + fan->command = rv;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_add_fan_target_attr(data, fan);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_add_pwm_attr(data, fan);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > + return pmbus_add_pwm_enable_attr(data, fan);
> > > >
> > > > +}
> > > > +
> > > > static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > > > struct pmbus_data *data)
> > > >
> > > > {
> > > > @@ -1624,6 +1954,11 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > > > > > > > > > PSC_FAN, true, true) == NULL)
> > > > > > return -ENOMEM;
> > > >
> > > >
> > > > > > > > > > > > + 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
@ 2017-07-12 7:01 ` Andrew Jeffery
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2017-07-12 7:01 UTC (permalink / raw)
To: Guenter Roeck, linux-hwmon
Cc: jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
[-- Attachment #1: Type: text/plain, Size: 27152 bytes --]
On Tue, 2017-07-11 at 20:43 -0700, Guenter Roeck wrote:
> On 07/11/2017 05:39 PM, Andrew Jeffery wrote:
> > On Tue, 2017-07-11 at 06:40 -0700, Guenter Roeck wrote:
> > > On 07/10/2017 06:56 AM, Andrew Jeffery wrote:
> > > > Augment PMBus support to include control of fans via the
> > > > FAN_COMMAND_[1-4] registers, both in RPM and PWM modes. The behaviour
> > > > of FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4] are tightly coupled, and
> > > > their interactions do not fit the existing use of struct pmbus_sensor.
> > > > The patch introduces struct pmbus_fan_ctrl to distinguish from the
> > > > simple sensor case, along with associated sysfs show/set implementations.
> > > >
> > > > Further, the interpreting the value of FAN_COMMAND_[1-4] depends on both
> > > > the current fan mode (RPM or PWM, as configured in
> > > > FAN_CONFIG_{1_2,3_4}), and the device-specific behaviour for the
> > > > register. For example, the MAX31785 chip defines the following:
> > > >
> > > > PWM (m = 1, b = 0, R = 2):
> > > > 0x0000 - 0x2710: 0 - 100% fan PWM duty cycle
> > > > 0x2711 - 0x7fff: 100% fan PWM duty cycle
> > > > 0x8000 - 0xffff: Ignore FAN_COMMAND_[1-4], use automatic fan control
> > > >
> > > > RPM (m = 1, b = 0, R = 0):
> > > > 0x0000 - 0x7FFF: 0 - 32,767 RPM
> > > > 0x8000 - 0xFFFF: Ignore FAN_COMMAND_[1-4], use automatic fan control
> > > >
> > > > To handle the device-specific interpretation of the FAN_COMMAND_[1-4],
> > > > add an optional callbacks to the info struct to get/set the 'mode'
> > > > value required for the pwm[1-n]_enable sysfs attribute. A fallback
> > > > calculation exists if the callbacks are not populated; the fallback
> > > > ignores device-specific ranges and tries to determine a reasonable value
> > > > from FAN_CONFIG_{1_2,3_4} and FAN_COMMAND_[1-4].
> > > >
> > >
> > > This seems overly complex, but unfortunately I don't have time for a detailed
> > > analysis right now.
> >
> > No worries. It turned out more complex than I was hoping as well, and I
> > am keen to hear any insights to trim it down.
> >
> > > Couple of comments below.
> >
> > Yep, thanks for taking a look.
> >
> > >
> > > Guenter
> > >
> > > > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > >
> > > > ---
> > > > drivers/hwmon/pmbus/pmbus.h | 7 +
> > > > drivers/hwmon/pmbus/pmbus_core.c | 335 +++++++++++++++++++++++++++++++++++++++
> > > > 2 files changed, 342 insertions(+)
> > > >
> > > > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> > > > index bfcb13bae34b..927eabc1b273 100644
> > > > --- a/drivers/hwmon/pmbus/pmbus.h
> > > > +++ b/drivers/hwmon/pmbus/pmbus.h
> > > > @@ -223,6 +223,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)
> > > > */
> > > > @@ -380,6 +382,11 @@ struct pmbus_driver_info {
> > > > > > > > > > > > int (*identify)(struct i2c_client *client,
> > > > > > struct pmbus_driver_info *info);
> > > >
> > > >
> > > > > > > > > > > > + /* Allow the driver to interpret the fan command value */
> > > > > > > > > > > > + int (*get_pwm_mode)(int id, u8 fan_config, u16 fan_command);
> > > > > > > > > > > > + int (*set_pwm_mode)(int id, long mode, u8 *fan_config,
> > > > > > + u16 *fan_command);
> > > >
> > > > +
> > >
> > > It is not entirely obvious to me why this would require new callback functions.
> > > Can you overload PMBUS_FAN_CONFIG_12 / PMBUS_FAN_CONFIG_34 or, if that does not
> > > work for some reason, introduce a virtual register, such as PMBUS_VIRT_PWM_MODE ?
> >
> > Can you expand on the thought of overloading PMBUS_FAN_CONFIG_{12,34}?
> >
>
> Every register/command can be implemented in the front end driver in its read/write
> functions. For example, see max34440_read_byte_data(), which replaces some of the
> status registers. ucd9000.c actually overrides PMBUS_FAN_CONFIG_12 and
> PMBUS_FAN_CONFIG_34.
Right; In the cover letter I mentioned I hadn't thought about how to
implement the dual tach feature of the MAX31785 at the time. After
sending the RFC series I had go at that as well, and ended up
implementing support purely in terms of the read/write callbacks with
no modifications to the core, so I've become familiar with them.
>
> > Regarding virtual registers, I saw references to them whilst I was
> > working my way through the core code but didn't stop to investigate.
> > I'll take a deeper look.
> >
>
> Virtual registers/commands are meant to "standardize" non-standard PMBus commands.
>
> For example, PMBus provides no means to read the average/minimum/maximum temperature.
> Modeling the respective attributes using PMBUS_VIRT_READ_TEMP_AVG, PMBUS_VIRT_READ_TEMP_MIN,
> and PMBUS_VIRT_READ_TEMP_MAX, and providing driver-specific read/write functions
> which map the virtual commands to real chip registers makes the code much simpler
> than per-attribute callbacks. With virtual commands, the core only needs entries such as
>
> }, {
> .reg = PMBUS_VIRT_READ_TEMP_MIN,
> .attr = "lowest",
> }, {
> .reg = PMBUS_VIRT_READ_TEMP_AVG,
> .attr = "average",
> }, {
> .reg = PMBUS_VIRT_READ_TEMP_MAX,
> .attr = "highest",
>
> to support such non-standard attributes. Imagine how that would look like
> if each of the supported virtual commands would be implemented as callback.
Indeed. Hence RFC in case I had overlooked something :) Clearly I have.
>
> > However, the addition of the callbacks was driven by the behaviour of
> > the MAX31785, where some values written to PMBUS_FAN_COMMAND_1 trigger
> > automated control, while others retain manual control. Patch 4/4 should
> > provide a bit more context, though I've also outlined the behaviour in
> > the commit message for this patch. I don't have a lot of experience
> > with PMBus devices so I don't have a good idea if there's a better way
> > to capture the behaviour that isn't so unconstrained in its approach.
> >
>
> Many pmbus commands have side effects. I don't see how an explicit callback
> would be different to overloading a standard register or to providing a virtual
> register/command, whichever is more convenient.
I'm going to experiment with the virtual registers. From your
description above and looking at the comments in pmbus.h I think I can
make something work (and drop the callbacks).
>
> > >
> > > > > > > > > > > > /* Regulator functionality, if supported by this chip driver. */
> > > > > > > > > > > > int num_regulators;
> > > > > > const struct regulator_desc *reg_desc;
> > > >
> > > > diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> > > > index ba59eaef2e07..3b0a55bbbd2c 100644
> > > > --- a/drivers/hwmon/pmbus/pmbus_core.c
> > > > +++ b/drivers/hwmon/pmbus/pmbus_core.c
> > > > @@ -69,6 +69,38 @@ struct pmbus_sensor {
> > > > #define to_pmbus_sensor(_attr) \
> > > > > > container_of(_attr, struct pmbus_sensor, attribute)
> > > >
> > > >
> > > > > > +#define PB_FAN_CONFIG_RPM PB_FAN_2_RPM
> > > >
> > > > +#define PB_FAN_CONFIG_INSTALLED PB_FAN_2_INSTALLEDBUS_VIRT_
> > >
> > > Something seems odd here. PB_FAN_2_INSTALLEDBUS_VIRT_ ?
> >
> > Yes, that's busted. Not sure what went wrong, but I'll clean it up.
> >
> > >
> > > > > > > > > > > > +#define PB_FAN_CONFIG_MASK(i) (0xff << (4 * !((i) & 1)))
> > > > > > > > > > > > +#define PB_FAN_CONFIG_GET(i, n) (((n) >> (4 * !((i) & 1))) & 0xff)
> > > > > > +#define PB_FAN_CONFIG_PUT(i, n) (((n) & 0xff) << (4 * !((i) & 1)))
> > > >
> > > > +
> > >
> > > Aren't there standard bit manipulation macros for that ? Either case, this is just to avoid
> > > having to use the existing defines.
> >
> > As I store the configuration for each fan in a struct pmbus_fan_ctrl
> > dedicated to the fan, I reasoned that intermediate code should not have
>
> I rather wonder if pmbus_fan_ctrl is needed in the first place. The notion of
> local 'struct pmbus_sensor' variables seems really messy. I think I'll really
> have to spend some time on this to see if and how it can be simplified;
> it just should not be that complex.
Sure. FWIW I plan on sending a follow-up RFC based on the feedback
you've given here, and I'll look to chop out pmbus_fan_ctrl. I was
suspicious of needing it as well, but was after your input on the
general approach and figured sending the patches was better than
guessing at your potential feedback.
If a follow-up isn't of interest and you'd definitely rather take on
the work up yourself, let me know.
Thanks again for taking a look.
Andrew
>
> Thanks,
> Guenter
>
> > to deal with the details of which nibble to access with respect to the
> > fan's (per-page) ID. Rather, code reading or writing
> > PMBUS_FAN_COMMAND_[1-4] should deal with ensuring the correct values
> > are provided.
> >
> > > Ok, but then I think it would make more sense to
> > > make it generic, ie change the core to not use PB_FAN_2_RPM / PB_FAN_1_RPM etc.
> > > but PB_FAN_RPM(index) everywhere.
> >
> > I'll make the change throughout pmbus core.
> >
> > >
> > > > +struct pmbus_fan_ctrl_attr {
> > > > > > > > > > > > + struct device_attribute attribute;
> > > > > > + char name[PMBUS_NAME_SIZE];
> > > >
> > > > +};
> > > > +
> > > > +struct pmbus_fan_ctrl {
> > > > > > > > > > > > + struct pmbus_fan_ctrl_attr fan_target;
> > > > > > > > > > > > + struct pmbus_fan_ctrl_attr pwm;
> > > > > > > > > > > > + struct pmbus_fan_ctrl_attr pwm_enable;
> > > > > > > > > > > > + int index;
> > > > > > > > > > > > + u8 page;
> > > > > > > > > > > > + u8 id;
> > > > > > > > > > > > + u8 config;
> > > > > > + u16 command;
> > > >
> > > > +};
> > > > +#define to_pmbus_fan_ctrl_attr(_attr) \
> > > > > > + container_of(_attr, struct pmbus_fan_ctrl_attr, attribute)
> > > >
> > > > +#define fan_target_to_pmbus_fan_ctrl(_attr) \
> > > > > > > > > > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
> > > > > > + fan_target)
> > > >
> > > > +#define pwm_to_pmbus_fan_ctrl(_attr) \
> > > > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, pwm)
> > > >
> > > > +#define pwm_enable_to_pmbus_fan_ctrl(_attr) \
> > > > > > > > > > > > + container_of(to_pmbus_fan_ctrl_attr(_attr), struct pmbus_fan_ctrl, \
> > > > > > + pwm_enable)
> > > >
> > > > +
> > > > struct pmbus_boolean {
> > > > > > > > char name[PMBUS_NAME_SIZE]; /* sysfs boolean name */
> > > > > >
> > > > > > struct sensor_device_attribute attribute;
> > > >
> > > > @@ -806,6 +838,219 @@ static ssize_t pmbus_show_label(struct device *dev,
> > > > > > return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
> > > >
> > > > }
> > > >
> > > > +static ssize_t pmbus_show_fan_command(struct device *dev,
> > > > > > > > > > > > + enum pmbus_fan_mode mode,
> > > > > > + struct pmbus_fan_ctrl *fan, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > > > > > > > + struct pmbus_sensor sensor;
> > > > > > + long val;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > > > > > > > > > + if ((mode == percent && (fan->config & PB_FAN_CONFIG_RPM)) ||
> > > > > > > > > > > > + (mode == rpm && !(fan->config & PB_FAN_CONFIG_RPM))) {
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > + return -ENOTSUPP; /* XXX: This seems dodgy, but what to do? */
> > >
> > > Not create the attribute in question in the first place, or return 0. The above
> > > messes up the 'sensors' command.
> >
> > I think returning 0 is the only valid option of the two, given that we
> > can dynamically switch between RPM and PWM modes.
> >
> > Thanks for the feedback.
> >
> > Andrew
> >
> > >
> > > > > > + }
> > > >
> > > > +
> > > > > > > > > > > > + sensor.class = PSC_FAN;
> > > > > > > > > > > > + if (mode == percent)
> > > > > > > > > > > > + sensor.data = fan->command * 255 / 100;
> > > > > > > > > > > > + else
> > > > > > + sensor.data = fan->command;
> > > >
> > > > +
> > > > > > + val = pmbus_reg2data(data, &sensor);
> > > >
> > > > +
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > + return snprintf(buf, PAGE_SIZE, "%ld\n", val);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_show_fan_target(struct device *dev,
> > > > > > + struct device_attribute *da, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_show_fan_command(dev, rpm,
> > > > > > + fan_target_to_pmbus_fan_ctrl(da), buf);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_show_pwm(struct device *dev,
> > > > > > + struct device_attribute *da, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_show_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> > > > > > + buf);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_fan_command(struct device *dev,
> > > > > > > > > > > > + enum pmbus_fan_mode mode,
> > > > > > > > > > > > + struct pmbus_fan_ctrl *fan,
> > > > > > + const char *buf, ssize_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > > > > > > > + int config_addr, command_addr;
> > > > > > > > > > > > + struct pmbus_sensor sensor;
> > > > > > > > > > > > + ssize_t rv;
> > > > > > + long val;
> > > >
> > > > +
> > > > > > > > > > > > + if (kstrtol(buf, 10, &val) < 0)
> > > > > > + return -EINVAL;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > > > + sensor.class = PSC_FAN;
> > > >
> > > > +
> > > > > > + val = pmbus_data2reg(data, &sensor, val);
> > > >
> > > > +
> > > > > > > > > > > > + if (mode == percent)
> > > > > > + val = val * 100 / 255;
> > > >
> > > > +
> > > > > > > > > > > > + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > > > + command_addr = config_addr + 1 + (fan->id & 1);
> > > >
> > > > +
> > > > > > > > > > > > + if (mode == rpm)
> > > > > > > > > > > > + fan->config |= PB_FAN_CONFIG_RPM;
> > > > > > > > > > > > + else
> > > > > > + fan->config &= ~PB_FAN_CONFIG_RPM;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> > > > > > > > > > > > + PB_FAN_CONFIG_PUT(fan->id, fan->config),
> > > > > > > > > > > > + PB_FAN_CONFIG_MASK(fan->id));
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + goto done;
> > > >
> > > > +
> > > > > > > > > > > > + fan->command = val;
> > > > > > > > > > > > + rv = pmbus_write_word_data(client, fan->page, command_addr,
> > > > > > + fan->command);
> > > >
> > > > +
> > > > +done:
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > + return count;
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_fan_target(struct device *dev,
> > > > > > > > > > > > + struct device_attribute *da,
> > > > > > + const char *buf, size_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_set_fan_command(dev, rpm,
> > > > > > > > > > > > + fan_target_to_pmbus_fan_ctrl(da), buf,
> > > > > > + count);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_pwm(struct device *dev, struct device_attribute *da,
> > > > > > + const char *buf, size_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_set_fan_command(dev, percent, pwm_to_pmbus_fan_ctrl(da),
> > > > > > + buf, count);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_show_pwm_enable(struct device *dev,
> > > > > > + struct device_attribute *da, char *buf)
> > > >
> > > > +{
> > > > > > > > > > > > + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > + long mode;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > +
> > > > > > > > > > > > + if (data->info->get_pwm_mode) {
> > > > > > + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > >
> > > > +
> > > > > > > > > > > > + mode = data->info->get_pwm_mode(fan->id, config, fan->command);
> > > > > > > > > > > > + } else {
> > > > > > > > > > > > + struct pmbus_sensor sensor = {
> > > > > > > > > > > > + .class = PSC_FAN,
> > > > > > > > > > > > + .data = fan->command,
> > > > > > > > > > > > + };
> > > > > > + long command;
> > > >
> > > > +
> > > > > > + command = pmbus_reg2data(data, &sensor);
> > > >
> > > > +
> > > > > > > > > > > > + /* XXX: Need to do something sensible */
> > > > > > > > > > > > + if (fan->config & PB_FAN_CONFIG_RPM)
> > > > > > > > > > > > + mode = 2;
> > > > > > > > > > > > + else
> > > > > > > > > > > > + mode = (command >= 0 && command < 100);
> > > > > > + }
> > > >
> > > > +
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > + return snprintf(buf, PAGE_SIZE, "%ld\n", mode);
> > > >
> > > > +}
> > > > +
> > > > +static ssize_t pmbus_set_pwm_enable(struct device *dev,
> > > > > > > > > > > > + struct device_attribute *da,
> > > > > > + const char *buf, size_t count)
> > > >
> > > > +{
> > > > > > > > > > > > + struct pmbus_fan_ctrl *fan = pwm_enable_to_pmbus_fan_ctrl(da);
> > > > > > > > > > > > + struct i2c_client *client = to_i2c_client(dev->parent);
> > > > > > > > > > > > + struct pmbus_data *data = i2c_get_clientdata(client);
> > > > > > > > > > > > + int config_addr, command_addr;
> > > > > > > > > > > > + struct pmbus_sensor sensor;
> > > > > > > > > > > > + ssize_t rv = count;
> > > > > > + long mode;
> > > >
> > > > +
> > > > > > > > > > > > + if (kstrtol(buf, 10, &mode) < 0)
> > > > > > + return -EINVAL;
> > > >
> > > > +
> > > > > > + mutex_lock(&data->update_lock);
> > > >
> > > > +
> > > > > > + sensor.class = PSC_FAN;
> > > >
> > > > +
> > > > > > > > > > > > + config_addr = (fan->id < 2) ? PMBUS_FAN_CONFIG_12 : PMBUS_FAN_CONFIG_34;
> > > > > > + command_addr = config_addr + 1 + (fan->id & 1);
> > > >
> > > > +
> > > > > > > > > > > > + if (data->info->set_pwm_mode) {
> > > > > > > > > > > > + u8 config = PB_FAN_CONFIG_PUT(fan->id, fan->config);
> > > > > > + u16 command = fan->command;
> > > >
> > > > +
> > > > > > > > > > > > + rv = data->info->set_pwm_mode(fan->id, mode, &config, &command);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + goto done;
> > > >
> > > > +
> > > > > > > > > > > > + fan->config = PB_FAN_CONFIG_GET(fan->id, config);
> > > > > > > > > > > > + fan->command = command;
> > > > > > > > > > > > + } else {
> > > > > > > > > > > > + fan->config &= ~PB_FAN_CONFIG_RPM;
> > > > > > > > > > > > + switch (mode) {
> > > > > > > > > > > > + case 0:
> > > > > > > > > > > > + case 1:
> > > > > > > > > > > > + /* XXX: Safe at least? */
> > > > > > > > > > > > + fan->command = pmbus_data2reg(data, &sensor, 100);
> > > > > > > > > > > > + break;
> > > > > > > > > > > > + case 2:
> > > > > > > > > > > > + default:
> > > > > > > > > > > > + /* XXX: Safe at least? */
> > > > > > > > > > > > + fan->command = 0xffff;
> > > > > > > > > > > > + break;
> > > > > > > > > > > > + }
> > > > > > + }
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_update_byte_data(client, fan->page, config_addr,
> > > > > > > > > > > > + PB_FAN_CONFIG_PUT(fan->id, fan->config),
> > > > > > > > > > > > + PB_FAN_CONFIG_MASK(fan->id));
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + goto done;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_write_word_data(client, fan->page, command_addr,
> > > > > > + fan->command);
> > > >
> > > > +
> > > > +done:
> > > > > > + mutex_unlock(&data->update_lock);
> > > >
> > > > +
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > + return count;
> > > >
> > > > +}
> > > > +
> > > > static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
> > > > {
> > > > > > if (data->num_attributes >= data->max_attributes - 1) {
> > > >
> > > > @@ -1094,6 +1339,51 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
> > > > > > return 0;
> > > >
> > > > }
> > > >
> > > > +static int pmbus_add_fan_ctrl_attr(struct pmbus_data *data,
> > > > > > > > > > > > + struct pmbus_fan_ctrl_attr *fa,
> > > > > > > > > > > > + const char *name_fmt,
> > > > > > > > > > > > + ssize_t (*show)(struct device *dev,
> > > > > > > > > > > > + struct device_attribute *attr,
> > > > > > > > > > > > + char *buf),
> > > > > > > > > > > > + ssize_t (*store)(struct device *dev,
> > > > > > > > > > > > + struct device_attribute *attr,
> > > > > > > > > > > > + const char *buf, size_t count),
> > > > > > + int idx)
> > > >
> > > > +{
> > > > > > + struct device_attribute *da;
> > > >
> > > > +
> > > > > > + da = &fa->attribute;
> > > >
> > > > +
> > > > > > > > > > > > + snprintf(fa->name, sizeof(fa->name), name_fmt, idx);
> > > > > > + pmbus_dev_attr_init(da, fa->name, 0644, show, store);
> > > >
> > > > +
> > > > > > + return pmbus_add_attribute(data, &da->attr);
> > > >
> > > > +}
> > > > +
> > > > +static inline int pmbus_add_fan_target_attr(struct pmbus_data *data,
> > > > > > + struct pmbus_fan_ctrl *fan)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_add_fan_ctrl_attr(data, &fan->fan_target, "fan%d_target",
> > > > > > > > > > > > + pmbus_show_fan_target,
> > > > > > + pmbus_set_fan_target, fan->index);
> > > >
> > > > +}
> > > > +
> > > > +static inline int pmbus_add_pwm_attr(struct pmbus_data *data,
> > > > > > + struct pmbus_fan_ctrl *fan)
> > > >
> > > > +{
> > > > +
> > > > > > > > > > > > + return pmbus_add_fan_ctrl_attr(data, &fan->pwm, "pwm%d", pmbus_show_pwm,
> > > > > > + pmbus_set_pwm, fan->index);
> > > >
> > > > +}
> > > > +
> > > > +static inline int pmbus_add_pwm_enable_attr(struct pmbus_data *data,
> > > > > > + struct pmbus_fan_ctrl *fan)
> > > >
> > > > +{
> > > > > > > > > > > > + return pmbus_add_fan_ctrl_attr(data, &fan->pwm_enable, "pwm%d_enable",
> > > > > > > > > > > > + pmbus_show_pwm_enable,
> > > > > > + pmbus_set_pwm_enable, fan->index);
> > > >
> > > > +}
> > > > +
> > > > static const struct pmbus_limit_attr vin_limit_attrs[] = {
> > > > > > > > > > > > {
> > > > > > .reg = PMBUS_VIN_UV_WARN_LIMIT,
> > > >
> > > > @@ -1565,6 +1855,13 @@ static const int pmbus_fan_config_registers[] = {
> > > > > > 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,
> > > >
> > > > +};
> > > > +
> > > > static const int pmbus_fan_status_registers[] = {
> > > > > > > > > > > > PMBUS_STATUS_FAN_12,
> > > > > > PMBUS_STATUS_FAN_12,
> > > >
> > > > @@ -1587,6 +1884,39 @@ 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_fan_ctrl *fan;
> > > > > > + int rv;
> > > >
> > > > +
> > > > > > > > > > > > + fan = devm_kzalloc(data->dev, sizeof(*fan), GFP_KERNEL);
> > > > > > > > > > > > + if (!fan)
> > > > > > + return -ENOMEM;
> > > >
> > > > +
> > > > > > > > > > > > + fan->index = index;
> > > > > > > > > > > > + fan->page = page;
> > > > > > > > > > > > + fan->id = id;
> > > > > > + fan->config = config;
> > > >
> > > > +
> > > > > > > > > > > > + rv = _pmbus_read_word_data(client, page,
> > > > > > > > > > > > + pmbus_fan_command_registers[id]);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > > > > > > > + return rv;
> > > > > > + fan->command = rv;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_add_fan_target_attr(data, fan);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > > > > > > > + rv = pmbus_add_pwm_attr(data, fan);
> > > > > > > > > > > > + if (rv < 0)
> > > > > > + return rv;
> > > >
> > > > +
> > > > > > + return pmbus_add_pwm_enable_attr(data, fan);
> > > >
> > > > +}
> > > > +
> > > > static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > > > struct pmbus_data *data)
> > > >
> > > > {
> > > > @@ -1624,6 +1954,11 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
> > > > > > > > > > > > PSC_FAN, true, true) == NULL)
> > > > > > return -ENOMEM;
> > > >
> > > >
> > > > > > > > > > > > + 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 --]
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
2017-07-12 7:01 ` Andrew Jeffery
(?)
@ 2017-07-12 15:10 ` Guenter Roeck
-1 siblings, 0 replies; 16+ messages in thread
From: Guenter Roeck @ 2017-07-12 15:10 UTC (permalink / raw)
To: Andrew Jeffery
Cc: linux-hwmon, jdelvare, linux-kernel, joel, openbmc, msbarth, mspinler
On Wed, Jul 12, 2017 at 04:31:09PM +0930, Andrew Jeffery wrote:
>
> Indeed. Hence RFC in case I had overlooked something :) Clearly I have.
>
Not surprising. It isn't exceptionally well documented :-)
> >
> > > However, the addition of the callbacks was driven by the behaviour of
> > > the MAX31785, where some values written to PMBUS_FAN_COMMAND_1 trigger
> > > automated control, while others retain manual control. Patch 4/4 should
> > > provide a bit more context, though I've also outlined the behaviour in
> > > the commit message for this patch. I don't have a lot of experience
> > > with PMBus devices so I don't have a good idea if there's a better way
> > > to capture the behaviour that isn't so unconstrained in its approach.
> > >
> >
> > Many pmbus commands have side effects. I don't see how an explicit callback
> > would be different to overloading a standard register or to providing a virtual
> > register/command, whichever is more convenient.
>
> I'm going to experiment with the virtual registers. From your
> description above and looking at the comments in pmbus.h I think I can
> make something work (and drop the callbacks).
>
Excellent.
>
> Sure. FWIW I plan on sending a follow-up RFC based on the feedback
> you've given here, and I'll look to chop out pmbus_fan_ctrl. I was
> suspicious of needing it as well, but was after your input on the
> general approach and figured sending the patches was better than
> guessing at your potential feedback.
>
> If a follow-up isn't of interest and you'd definitely rather take on
> the work up yourself, let me know.
>
By all means, please go ahead. I got way too much on my plate already.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-07-12 15:10 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-10 13:56 [RFC PATCH 0/4] pmbus: Expand fan support and add MAX31785 driver Andrew Jeffery
2017-07-10 13:56 ` [RFC PATCH 1/4] hwmon: pmbus: Disable build of non-max31785 drivers Andrew Jeffery
2017-07-10 13:56 ` [RFC PATCH 2/4] pmbus: Add fan configuration support Andrew Jeffery
2017-07-11 13:40 ` Guenter Roeck
2017-07-12 0:39 ` Andrew Jeffery
2017-07-12 0:39 ` Andrew Jeffery
2017-07-12 3:43 ` Guenter Roeck
2017-07-12 7:01 ` Andrew Jeffery
2017-07-12 7:01 ` Andrew Jeffery
2017-07-12 15:10 ` Guenter Roeck
2017-07-10 13:56 ` [RFC PATCH 3/4] pmbus: Allow dynamic fan coefficient values Andrew Jeffery
2017-07-11 13:31 ` Guenter Roeck
2017-07-12 1:20 ` Andrew Jeffery
2017-07-12 1:20 ` Andrew Jeffery
2017-07-12 3:20 ` Guenter Roeck
2017-07-10 13:56 ` [RFC PATCH 4/4] pmbus: Add MAX31785 driver Andrew Jeffery
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.