On 11/09/2017 07:03 PM, Andrew Jeffery wrote:
On Sun, 2017-11-05 at 06:39 -0800, Guenter Roeck wrote:On Fri, Nov 03, 2017 at 03:53:03PM +1100, Andrew Jeffery wrote:Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes. Fans in a PMBus device are driven by the configuration of two registers: FAN_CONFIG_x_y and FAN_COMMAND_x: FAN_CONFIG_x_y dictates how the fan and the tacho operate (if installed), while FAN_COMMAND_x sets the desired fan rate. The unit of FAN_COMMAND_x is dependent on the operational fan mode, RPM or PWM percent duty, as determined by the corresponding FAN_CONFIG_x_y. The mapping of fanX_target, pwmX and pwmX_enable onto FAN_CONFIG_x_y and FAN_COMMAND_x is implemented with the addition of virtual registers and generic implementations in the core: 1. PMBUS_VIRT_FAN_TARGET_x 2. PMBUS_VIRT_PWM_x 3. PMBUS_VIRT_PWM_ENABLE_x The virtual registers facilitate the necessary side-effects of each access. Examples of the read case, assuming m = 1, b = 0, R = 0: Read | With || Gives ------------------------------------------------------- Attribute | FAN_CONFIG_x_y | FAN_COMMAND_y || Value ----------------------------------------------++------- fanX_target | ~PB_FAN_z_RPM | 0x0001 || 1 pwm1 | ~PB_FAN_z_RPM | 0x0064 || 255 pwmX_enable | ~PB_FAN_z_RPM | 0x0001 || 1 fanX_target | PB_FAN_z_RPM | 0x0001 || 1 pwm1 | PB_FAN_z_RPM | 0x0064 || 0 pwmX_enable | PB_FAN_z_RPM | 0x0001 || 1 And the write case: Write | With || Sets -------------+-------++----------------+--------------- Attribute | Value || FAN_CONFIG_x_y | FAN_COMMAND_x -------------+-------++----------------+--------------- fanX_target | 1 || PB_FAN_z_RPM | 0x0001 pwmX | 255 || ~PB_FAN_z_RPM | 0x0064 pwmX_enable | 1 || ~PB_FAN_z_RPM | 0x0064 Also, the DIRECT mode scaling of some controllers is different between RPM and PWM percent duty control modes, so PSC_PWM is introduced to capture the necessary coefficients. Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx> --- drivers/hwmon/pmbus/pmbus.h | 29 +++++ drivers/hwmon/pmbus/pmbus_core.c | 224 ++++++++++++++++++++++++++++++++++++--- 2 files changed, 238 insertions(+), 15 deletions(-) diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h index 4efa2bd4f6d8..cdf3e288e626 100644 --- a/drivers/hwmon/pmbus/pmbus.h +++ b/drivers/hwmon/pmbus/pmbus.h @@ -190,6 +190,28 @@ enum pmbus_regs { PMBUS_VIRT_VMON_UV_FAULT_LIMIT, PMBUS_VIRT_VMON_OV_FAULT_LIMIT, PMBUS_VIRT_STATUS_VMON, + + /* + * RPM and PWM Fan control + * + * Drivers wanting to expose PWM control must define the behaviour of + * PMBUS_VIRT_PWM_ENABLE_[1-4] in the {read,write}_word_data callback. + * + * pmbus core provides default implementations for + * PMBUS_VIRT_FAN_TARGET_[1-4] and PMBUS_VIRT_PWM_[1-4]. + */ + PMBUS_VIRT_FAN_TARGET_1, + PMBUS_VIRT_FAN_TARGET_2, + PMBUS_VIRT_FAN_TARGET_3, + PMBUS_VIRT_FAN_TARGET_4, + PMBUS_VIRT_PWM_1, + PMBUS_VIRT_PWM_2, + PMBUS_VIRT_PWM_3, + PMBUS_VIRT_PWM_4, + PMBUS_VIRT_PWM_ENABLE_1, + PMBUS_VIRT_PWM_ENABLE_2, + PMBUS_VIRT_PWM_ENABLE_3, + PMBUS_VIRT_PWM_ENABLE_4, };/*@@ -223,6 +245,8 @@ enum pmbus_regs { #define PB_FAN_1_RPM BIT(6) #define PB_FAN_1_INSTALLED BIT(7)+enum pmbus_fan_mode { percent = 0, rpm };+ /* * STATUS_BYTE, STATUS_WORD (lower) */ @@ -313,6 +337,7 @@ enum pmbus_sensor_classes { PSC_POWER, PSC_TEMPERATURE, PSC_FAN, + PSC_PWM, PSC_NUM_CLASSES /* Number of power sensor classes */ };@@ -339,6 +364,8 @@ enum pmbus_sensor_classes {#define PMBUS_HAVE_STATUS_FAN34 BIT(17) #define PMBUS_HAVE_VMON BIT(18) #define PMBUS_HAVE_STATUS_VMON BIT(19) +#define PMBUS_HAVE_PWM12 BIT(20) +#define PMBUS_HAVE_PWM34 BIT(21)enum pmbus_data_format { linear = 0, direct, vid };enum vrm_version { vr11 = 0, vr12, vr13 }; @@ -413,6 +440,8 @@ int pmbus_write_byte_data(struct i2c_client *client, int page, u8 reg, u8 value); int pmbus_update_byte_data(struct i2c_client *client, int page, u8 reg, u8 mask, u8 value); +int pmbus_update_fan(struct i2c_client *client, int page, int id, + u8 config, u8 mask, u16 command); void pmbus_clear_faults(struct i2c_client *client); bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg); bool pmbus_check_word_register(struct i2c_client *client, int page, int reg); diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c index 302f0aef59de..55838b69e99a 100644 --- a/drivers/hwmon/pmbus/pmbus_core.c +++ b/drivers/hwmon/pmbus/pmbus_core.c @@ -64,6 +64,7 @@ struct pmbus_sensor { u16 reg; /* register */ enum pmbus_sensor_classes class; /* sensor class */ bool update; /* runtime sensor update needed */ + bool convert; /* Whether or not to apply linear/vid/direct */ int data; /* Sensor data. Negative if there was a read error */ }; @@ -128,6 +129,27 @@ struct pmbus_debugfs_entry { u8 reg; };+static const int pmbus_fan_rpm_mask[] = {+ PB_FAN_1_RPM, + PB_FAN_2_RPM, + PB_FAN_1_RPM, + PB_FAN_2_RPM, +}; + +static const int pmbus_fan_config_registers[] = { + PMBUS_FAN_CONFIG_12, + PMBUS_FAN_CONFIG_12, + PMBUS_FAN_CONFIG_34, + PMBUS_FAN_CONFIG_34 +}; + +static const int pmbus_fan_command_registers[] = { + PMBUS_FAN_COMMAND_1, + PMBUS_FAN_COMMAND_2, + PMBUS_FAN_COMMAND_3, + PMBUS_FAN_COMMAND_4, +}; + void pmbus_clear_cache(struct i2c_client *client) { struct pmbus_data *data = i2c_get_clientdata(client); @@ -198,6 +220,31 @@ int pmbus_write_word_data(struct i2c_client *client, u8 page, u8 reg, u16 word) } EXPORT_SYMBOL_GPL(pmbus_write_word_data);+int pmbus_update_fan(struct i2c_client *client, int page, int id,+ u8 config, u8 mask, u16 command)Please make sure continuation lines are aligned to '(' where possible.Yep, sorry about that.+{ + int from, rv; + u8 to; + + from = pmbus_read_byte_data(client, page, + pmbus_fan_config_registers[id]); + if (from < 0) + return from; + + to = (from & ~mask) | (config & mask); + + if (to != from) { + rv = pmbus_write_byte_data(client, page, + pmbus_fan_config_registers[id], to); + if (rv < 0) + return rv; + } + + return pmbus_write_word_data(client, page, + pmbus_fan_command_registers[id], command); +} +EXPORT_SYMBOL_GPL(pmbus_update_fan); + /* * _pmbus_write_word_data() is similar to pmbus_write_word_data(), but checks if * a device specific mapping function exists and calls it if necessary. @@ -214,8 +261,40 @@ static int _pmbus_write_word_data(struct i2c_client *client, int page, int reg, if (status != -ENODATA) return status; } - if (reg >= PMBUS_VIRT_BASE) - return -ENXIO; + if (reg >= PMBUS_VIRT_BASE) { + int id, bit; + + switch (reg) { + case PMBUS_VIRT_FAN_TARGET_1: + case PMBUS_VIRT_FAN_TARGET_2: + case PMBUS_VIRT_FAN_TARGET_3: + case PMBUS_VIRT_FAN_TARGET_4:Maybe case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:Sure. I'll fix all instances of this.+ id = reg - PMBUS_VIRT_FAN_TARGET_1; + bit = pmbus_fan_rpm_mask[id]; + status = pmbus_update_fan(client, page, id, bit, bit, + word); + break; + case PMBUS_VIRT_PWM_1: + case PMBUS_VIRT_PWM_2: + case PMBUS_VIRT_PWM_3: + case PMBUS_VIRT_PWM_4: + { + u32 command = word;Please move command up and drop the {.My default is to scope variables to where they are needed, but will fix.
I am fine with that and do it as well as long as it is in loops or if statements. In case statements it becomes awkward. Of course, that is all POV.
Why u32 ? The value should be bound to 0..255 (if it isn't and a larger value is accepted we have overflow issues).It gets scaled below, though we still only need u16 if the max value is 255. I'll fix it.
Good point. u32 is better than u16 - it results in less code on many architectures.
+ + id = reg - PMBUS_VIRT_PWM_1; + bit = pmbus_fan_rpm_mask[id]; + command *= 100; + command /= 255; + status = pmbus_update_fan(client, page, id, 0, bit, + command); + break; + } + default: + status = -ENXIO; + break; + } + return status;Please move this code to a separate function.Will do.+ } return pmbus_write_word_data(client, page, reg, word); }@@ -231,6 +310,9 @@ int pmbus_read_word_data(struct i2c_client *client, u8 page, u8 reg)} EXPORT_SYMBOL_GPL(pmbus_read_word_data);+static int pmbus_get_fan_command(struct i2c_client *client, int page, int id,+ enum pmbus_fan_mode mode); + /* * _pmbus_read_word_data() is similar to pmbus_read_word_data(), but checks if * a device specific mapping function exists and calls it if necessary. @@ -246,8 +328,42 @@ static int _pmbus_read_word_data(struct i2c_client *client, int page, int reg) if (status != -ENODATA) return status; } - if (reg >= PMBUS_VIRT_BASE) - return -ENXIO; + if (reg >= PMBUS_VIRT_BASE) { + int id; + + switch (reg) { + case PMBUS_VIRT_FAN_TARGET_1: + case PMBUS_VIRT_FAN_TARGET_2: + case PMBUS_VIRT_FAN_TARGET_3: + case PMBUS_VIRT_FAN_TARGET_4:Since there is an implied assumption that those are sequential, how about the following ? case PMBUS_VIRT_FAN_TARGET_1 ... PMBUS_VIRT_FAN_TARGET_4:Will fix all instances of this, as noted above.+ id = reg - PMBUS_VIRT_FAN_TARGET_1;This warrants a comment in the definition of PMBUS_VIRT_FAN_TARGET_X and PMBUS_VIRT_PWM_X stating that the definitions must be in sequence.Right, will add a comment to that effect.+ status = pmbus_get_fan_command(client, page, id, rpm); + break; + case PMBUS_VIRT_PWM_1: + case PMBUS_VIRT_PWM_2: + case PMBUS_VIRT_PWM_3: + case PMBUS_VIRT_PWM_4:Same as above.Ack.+ { + int rv;Please move the declaration up and drop the {.Ack.+ + id = reg - PMBUS_VIRT_PWM_1; + rv = pmbus_get_fan_command(client, page, id, percent); + if (rv < 0) + return rv; +Is this guaranteed to be <= 100 ?PMBus spec rev 1.2 part II says in 14.10 and 14.11: The second part of the configuration tells the device whether the fan speed commands are in RPM or PWM duty cycle (in percent).
Hmm, yes, sure, but can you trust the chip to follow the specification ?
However, the MAX31785 accepts the ranges 0-0x2710, so allows fractions of a percent.
Answering my own question ... apparently not.
Not sure what my thoughts were here, it's probably best just to drop this default PWM implementation as well.
I agree.
+ rv *= 255; + rv /= 100; + + status = rv; + break; + } + default: + status = -ENXIO; + break; + }Please move this code to a separate function.Will do, though given I plan to gut the PWM implementation is it still so disruptive?
Let's see how it looks like. If it does more than just call another function it should be a separate function.
+ + return status; + } return pmbus_read_word_data(client, page, reg); }@@ -314,6 +430,28 @@ static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg)return pmbus_read_byte_data(client, page, reg); }+static int pmbus_get_fan_command(struct i2c_client *client, int page, int id,+ enum pmbus_fan_mode mode)Maybe better pmbus_get_fan_get_speed_rpm to clarify that we are not really interested in the command register value but but in speed or rpm ?Sounds reasonable on the surface. I'll look into it.+{ + int config; + + config = _pmbus_read_byte_data(client, page, + pmbus_fan_config_registers[id]); + if (config < 0) + return config; + + /* + * We can't meaningfully translate between PWM and RPM, so if the + * attribute mode (fan[1-*]_target is RPM, pwm[1-*] and pwm[1-*]_enable + * are PWM) doesn't match the hardware mode, then report 0 instead. + */ + if ((mode == rpm) != (!!(config & pmbus_fan_rpm_mask[id]))) + return 0;Please drop the unnecessary ().Sure.I am not too happy about this - the user has no means to specify pwm or fan target speed _before_ changing the mode. It would be better to report (and accept) cached values in that situation, and update the actual value as the mode is changed.I'm with you on the caching idea for switching modes, but do we want to report values that don't obviously reflect the fan rate to userspace?
0 doesn't reflect the rate either. Bad choices either way.
In previous revisions I was returning an error here but you pointed out this would break sensors(1) and suggested I switch to 0 in the conflict case.+ + return _pmbus_read_word_data(client, page, + pmbus_fan_command_registers[id]); +} + static void pmbus_clear_fault_page(struct i2c_client *client, int page) { _pmbus_write_byte(client, page, PMBUS_CLEAR_FAULTS); @@ -515,7 +653,7 @@ static long pmbus_reg2data_direct(struct pmbus_data *data, /* X = 1/m * (Y * 10^-R - b) */ R = -R; /* scale result to milli-units for everything but fans */ - if (sensor->class != PSC_FAN) { + if (!(sensor->class == PSC_FAN || sensor->class == PSC_PWM)) {Please use positive logic if (sensor->class != PSC_FAN && sensor->class != PSC_PWM) It is (at least for me) much easier to read.Okay.R += 3; b *= 1000; } @@ -569,6 +707,9 @@ static long pmbus_reg2data(struct pmbus_data *data, struct pmbus_sensor *sensor) { long val;+ if (!sensor->convert)+ return sensor->data; + switch (data->info->format[sensor->class]) { case direct: val = pmbus_reg2data_direct(data, sensor); @@ -672,7 +813,7 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data, }/* Calculate Y = (m * X + b) * 10^R */- if (sensor->class != PSC_FAN) { + if (!(sensor->class == PSC_FAN || sensor->class == PSC_PWM)) {Same as above.Ack.R -= 3; /* Adjust R and b for data in milli-units */ b *= 1000; } @@ -703,6 +844,9 @@ static u16 pmbus_data2reg(struct pmbus_data *data, { u16 regval;+ if (!sensor->convert)+ return val; + switch (data->info->format[sensor->class]) { case direct: regval = pmbus_data2reg_direct(data, sensor, val); @@ -925,12 +1069,18 @@ static struct pmbus_sensor *pmbus_add_sensor(struct pmbus_data *data, return NULL; a = &sensor->attribute;- snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s",- name, seq, type); + if (type) + snprintf(sensor->name, sizeof(sensor->name), "%s%d_%s", + name, seq, type); + else + snprintf(sensor->name, sizeof(sensor->name), "%s%d", + name, seq); + sensor->page = page; sensor->reg = reg; sensor->class = class; sensor->update = update; + sensor->convert = true; pmbus_dev_attr_init(a, sensor->name, readonly ? S_IRUGO : S_IRUGO | S_IWUSR, pmbus_show_sensor, pmbus_set_sensor); @@ -1592,13 +1742,6 @@ static const int pmbus_fan_registers[] = { PMBUS_READ_FAN_SPEED_4 };-static const int pmbus_fan_config_registers[] = {- PMBUS_FAN_CONFIG_12, - PMBUS_FAN_CONFIG_12, - PMBUS_FAN_CONFIG_34, - PMBUS_FAN_CONFIG_34 -}; - static const int pmbus_fan_status_registers[] = { PMBUS_STATUS_FAN_12, PMBUS_STATUS_FAN_12, @@ -1621,6 +1764,48 @@ static const u32 pmbus_fan_status_flags[] = { };/* Fans */+static int pmbus_add_fan_ctrl(struct i2c_client *client, + struct pmbus_data *data, int index, int page, int id, + u8 config) +{ + struct pmbus_sensor *sensor; + int rv; + + rv = _pmbus_read_word_data(client, page, + pmbus_fan_command_registers[id]); + if (rv < 0) + return rv; + + sensor = pmbus_add_sensor(data, "fan", "target", index, page, + PMBUS_VIRT_FAN_TARGET_1 + id, PSC_FAN, + true, false); + + if (!sensor) + return -ENOMEM; + + if (!((data->info->func[page] & PMBUS_HAVE_PWM12) || + (data->info->func[page] & PMBUS_HAVE_PWM34)))why not just the following ? if (!(data->info->func[page] & (PMBUS_HAVE_PWM12 | PMBUS_HAVE_PWM34)))Sure, that's much neater.Also, doesn't this add attributes for 1,2 even if only 3,4 are supported, and 3,4 even if only 1,2 are supported ?It shouldn't, but that's due to the way I've called pmbus_add_fan_ctrl() in pmbus_add_fan_attributes(). The call is guarded with: /* Fan control */ if (pmbus_check_word_register(client, page, pmbus_fan_command_registers[f])) { ret = pmbus_add_fan_ctrl(client, data, index, page, f, regval); if (ret < 0) return ret; } So 'f', or the formal parameter 'id', is only ever provided as appropriate, and together the two tests should ensure that only the appropriate attributes are created. If the fan is marked as "not installed" then the addition of the attribute is skipped. This code was already in place to handle the fanX_input attribute. So to the caller test, regardless of RPM or PWM mode, we need the FAN_COMMAND register to configure the values. As it stands the change makes fan control available for any PMBus device which has the FAN_COMMANDxy register and whose driver configures a page with PMBUS_HAVE_FANxy, exposing the fanX_target attribute. The test in question above deals only with PMBUS_HAVE_PWMxy which was introduced in this change. There are some constraints: PMBUS_HAVE_FANxy is required if PMBUS_HAVE_PWMxy is to be specified for a given page. This should probably be documented in a comment alongside the #defines.+ return 0; + + sensor = pmbus_add_sensor(data, "pwm", NULL, index, page, + PMBUS_VIRT_PWM_1 + id, PSC_PWM, + true, false); + + if (!sensor) + return -ENOMEM; + + sensor = pmbus_add_sensor(data, "pwm", "enable", index, page, + PMBUS_VIRT_PWM_ENABLE_1 + id, PSC_PWM, + true, false); + + if (!sensor) + return -ENOMEM; + + sensor->convert = false;convert should be a new parameter to pmbus_add_sensor().Will fix. Cheers for the detailed feedback. Andrew+ + return 0; +} + static int pmbus_add_fan_attributes(struct i2c_client *client, struct pmbus_data *data) { @@ -1658,6 +1843,15 @@ static int pmbus_add_fan_attributes(struct i2c_client *client, PSC_FAN, true, true) == NULL) return -ENOMEM;+ /* Fan control */+ if (pmbus_check_word_register(client, page, + pmbus_fan_command_registers[f])) { + ret = pmbus_add_fan_ctrl(client, data, index, + page, f, regval); + if (ret < 0) + return ret; + } + /* * Each fan status register covers multiple fans, * so we have to do some magic.
-- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
