On Tue, Jul 18, 2017 at 1:06 PM, Andrew Jeffery <andrew@xxxxxxxx> wrote: > Expose fanX_target, pwmX and pwmX_enable hwmon sysfs attributes. Nice! I had a bit of a read. I'm not a pmbus expert, so most of the review is nitpicks about types, etc. I'll defer to Guenter for the real stuff. > > 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 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> > --- > > v1 -> v2: > * Convert to using virtual registers > * Drop struct pmbus_fan_ctrl > * Introduce PSC_PWM > * Drop struct pmbus_coeffs > * Drop additional callbacks > > drivers/hwmon/pmbus/pmbus.h | 19 ++++ > drivers/hwmon/pmbus/pmbus_core.c | 215 +++++++++++++++++++++++++++++++++++---- > 2 files changed, 217 insertions(+), 17 deletions(-) > > diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h > index bfcb13bae34b..226a37bd525f 100644 > --- a/drivers/hwmon/pmbus/pmbus.h > +++ b/drivers/hwmon/pmbus/pmbus.h > @@ -190,6 +190,20 @@ enum pmbus_regs { > PMBUS_VIRT_VMON_UV_FAULT_LIMIT, > PMBUS_VIRT_VMON_OV_FAULT_LIMIT, > PMBUS_VIRT_STATUS_VMON, > + > + /* Fan control */ > + 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 +237,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 +329,7 @@ enum pmbus_sensor_classes { > PSC_POWER, > PSC_TEMPERATURE, > PSC_FAN, > + PSC_PWM, > PSC_NUM_CLASSES /* Number of power sensor classes */ > }; > > @@ -413,6 +430,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, > + int config, int mask, int 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 ba59eaef2e07..712a8b6c4bd6 100644 > --- a/drivers/hwmon/pmbus/pmbus_core.c > +++ b/drivers/hwmon/pmbus/pmbus_core.c > @@ -63,6 +63,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 */ > }; > @@ -118,6 +119,27 @@ struct pmbus_data { > u8 currpage; > }; > > +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[] = { u8? > + PMBUS_FAN_CONFIG_12, > + PMBUS_FAN_CONFIG_12, > + PMBUS_FAN_CONFIG_34, > + PMBUS_FAN_CONFIG_34 > +}; > + > +static const int pmbus_fan_command_registers[] = { u8? > + 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); > @@ -188,6 +210,29 @@ 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, > + int config, int mask, int command) > +{ > + int from, to; > + int rv; > + > + from = pmbus_read_byte_data(client, page, > + pmbus_fan_config_registers[id]); > + if (from < 0) > + return from; > + > + to = (from & ~mask) | (config & mask); > + > + rv = pmbus_write_byte_data(client, page, > + pmbus_fan_config_registers[id], to); to is a u8. Perhaps define it as such? > + if (rv < 0) > + return rv; > + > + return pmbus_write_word_data(client, page, > + pmbus_fan_command_registers[id], command); Similar with command - it's a u16. This would help the definition of pmbus_update_fan match the others in the pmbus header, which mostly deal with explicitly sized types. mask and config could be u8 as well I think. > +} > +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. > @@ -197,15 +242,47 @@ static int _pmbus_write_word_data(struct i2c_client *client, int page, int reg, > { > struct pmbus_data *data = i2c_get_clientdata(client); > const struct pmbus_driver_info *info = data->info; > - int status; > + int status = -ENODATA; It looks like you modify this value in all of the code paths (except the one I suggest you remove below). > > if (info->write_word_data) { > status = info->write_word_data(client, page, reg, word); > if (status != -ENODATA) > return status; > } > - if (reg >= PMBUS_VIRT_BASE) > - return -ENXIO; > + if (status == -ENODATA && reg >= PMBUS_VIRT_BASE) { status will always be -ENODATA if we get down here. I don't think you need to make this change. > + 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: I haven't read the pmbus spec (feel free to point me to a page in the PDF if you think it will help). Can you educate me why we have 1..4? for all of these virtual registers? Why not 5, or 3? > + 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: > + { > + long command = word; long seems a bit... long. And we don't need the sign. Perhaps u32? > + > + id = reg - PMBUS_VIRT_PWM_1; > + bit = pmbus_fan_rpm_mask[id]; > + command *= 100; > + command /= 255; > + status = pmbus_update_fan(client, page, id, 0, bit, > + command); > + break; > + } > + default: > + status = -ENXIO; > + break; > + } > + return status; > + } > return pmbus_write_word_data(client, page, reg, word); > } > > @@ -221,6 +298,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. > @@ -229,15 +309,49 @@ static int _pmbus_read_word_data(struct i2c_client *client, int page, int reg) > { > struct pmbus_data *data = i2c_get_clientdata(client); > const struct pmbus_driver_info *info = data->info; > - int status; > + int status = -ENODATA; > > if (info->read_word_data) { > status = info->read_word_data(client, page, reg); > if (status != -ENODATA) > return status; > } > - if (reg >= PMBUS_VIRT_BASE) > - return -ENXIO; > + if (status == -ENODATA && reg >= PMBUS_VIRT_BASE) { Same comments as the write case. > + int id; > + > + switch (reg) { > + case PMBUS_VIRT_FAN_TARGET_1: > + case PMBUS_VIRT_FAN_TARGET_2: > + case PMBUS_VIRT_FAN_TARGET_3: > + case PMBUS_VIRT_FAN_TARGET_4: > + id = reg - PMBUS_VIRT_FAN_TARGET_1; > + status = pmbus_get_fan_command(client, page, id, rpm); > + break; > + case PMBUS_VIRT_PWM_1: > + case PMBUS_VIRT_PWM_2: > + case PMBUS_VIRT_PWM_3: > + case PMBUS_VIRT_PWM_4: > + { > + long rv; > + > + id = reg - PMBUS_VIRT_PWM_1; > + rv = pmbus_get_fan_command(client, page, id, percent); > + if (rv < 0) > + return rv; > + > + rv *= 255; > + rv /= 100; > + > + status = rv; > + break; > + } > + default: > + status = -ENXIO; > + break; > + } > + > + return status; > + } > return pmbus_read_word_data(client, page, reg); > } > > @@ -304,6 +418,23 @@ static int _pmbus_read_byte_data(struct i2c_client *client, int page, int reg) > return pmbus_read_byte_data(client, page, reg); > } > > +static int pmbus_get_fan_command(struct i2c_client *client, int page, int id, > + enum pmbus_fan_mode mode) > +{ > + int config; > + > + config = _pmbus_read_byte_data(client, page, > + pmbus_fan_config_registers[id]); > + if (config < 0) > + return config; > + > + if ((mode == rpm) != (!!(config & pmbus_fan_rpm_mask[id]))) What's (config & mask[id]) testing? Perhaps give it a variable so it's obvious. Avoids the ((())) too. This looks like it's testing for an error case. Should you be returning a negative value instead of zero? > + return 0; > + > + return _pmbus_read_word_data(client, page, > + pmbus_fan_command_registers[id]); > +} > + > static void pmbus_clear_fault_page(struct i2c_client *client, int page) > { > _pmbus_write_byte(client, page, PMBUS_CLEAR_FAULTS); > @@ -489,7 +620,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 */ Does this comment need updating? > - if (sensor->class != PSC_FAN) { > + if (!(sensor->class == PSC_FAN || sensor->class == PSC_PWM)) { > R += 3; > b *= 1000; > } > @@ -539,6 +670,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); > @@ -642,7 +776,7 @@ static u16 pmbus_data2reg_direct(struct pmbus_data *data, > } > > /* Calculate Y = (m * X + b) * 10^R */ > - if (sensor->class != PSC_FAN) { > + if (!(sensor->class == PSC_FAN || sensor->class == PSC_PWM)) { > R -= 3; /* Adjust R and b for data in milli-units */ > b *= 1000; > } > @@ -673,6 +807,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); > @@ -895,12 +1032,13 @@ 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); > + snprintf(sensor->name, sizeof(sensor->name), "%s%d%s%s", > + name, seq, type ? "_" : "", type ? type : ""); Perhaps something like this would be more readable: 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); > @@ -1558,13 +1696,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, > @@ -1587,6 +1718,47 @@ 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->m[PSC_PWM]) > + return 0; > + > + sensor = pmbus_add_sensor(data, "pwm", NULL, index, page, > + PMBUS_VIRT_PWM_1 + id, PSC_PWM, > + true, false); > + > + if (!sensor) > + return -ENOMEM; > + > + sensor = pmbus_add_sensor(data, "pwm", "enable", index, page, > + PMBUS_VIRT_PWM_ENABLE_1 + id, PSC_PWM, > + true, false); > + > + if (!sensor) > + return -ENOMEM; > + > + sensor->convert = false; > + > + return 0; > +} > + > static int pmbus_add_fan_attributes(struct i2c_client *client, > struct pmbus_data *data) > { > @@ -1624,6 +1796,15 @@ static int pmbus_add_fan_attributes(struct i2c_client *client, > PSC_FAN, true, true) == NULL) > return -ENOMEM; > > + /* Fan control */ > + if (pmbus_check_word_register(client, page, > + pmbus_fan_command_registers[f])) { > + ret = pmbus_add_fan_ctrl(client, data, index, > + page, f, regval); > + if (ret < 0) > + return ret; > + } > + > /* > * Each fan status register covers multiple fans, > * so we have to do some magic. > -- > 2.11.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html