[RFC PATCH 2/4] pmbus: Add fan configuration support
Guenter Roeck
linux at roeck-us.net
Tue Jul 11 23:40:04 AEST 2017
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 at 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.
>
More information about the openbmc
mailing list