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@xxxxxxxx> > > --- > > 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:
signature.asc
Description: This is a digitally signed message part