Re: [RFC PATCH 2/4] pmbus: Add fan configuration support
From: Guenter Roeck
Date: Tue Jul 11 2017 - 09:40:30 EST
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@xxxxxxxx>
---
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.