[PATCH 1/4] hwmon: (pmbus) add support for output voltage registers

[PATCH 1/4] hwmon: (pmbus) add support for output voltage registers

[Ruslan Babayev]

Registers VOUT_COMMAND, VOUT_MARGIN_HIGH and VOUT_MARGIN_LOW are
described in PMBUS Spec Part II Rev 1.2. Exposing them in the PMBUS
core allows the drivers to turn them on with a corresponding
PMBUS_HAVE_VOUT_... flags.

Cc: xe-linux-external@cisco.com
Signed-off-by: Ruslan Babayev <ruslan@babayev.com>
---
 drivers/hwmon/pmbus/pmbus.h      |   7 ++
 drivers/hwmon/pmbus/pmbus_core.c | 183 +++++++++++++++++++++++++++++++
 2 files changed, 190 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 1d24397d36ec..723648b3da36 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -223,6 +223,9 @@ enum pmbus_regs {
  * OPERATION
  */
 #define PB_OPERATION_CONTROL_ON		BIT(7)
+#define PB_OPERATION_MARGIN_HIGH	BIT(5)
+#define PB_OPERATION_MARGIN_LOW		BIT(4)
+#define PB_OPERATION_ACT_ON_FAULT	BIT(3)
 
 /*
  * CAPABILITY
@@ -291,6 +294,7 @@ enum pmbus_fan_mode { percent = 0, rpm };
 /*
  * STATUS_VOUT, STATUS_INPUT
  */
+#define PB_VOLTAGE_MAX_WARNING		BIT(3)
 #define PB_VOLTAGE_UV_FAULT		BIT(4)
 #define PB_VOLTAGE_UV_WARNING		BIT(5)
 #define PB_VOLTAGE_OV_WARNING		BIT(6)
@@ -371,6 +375,9 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_STATUS_VMON	BIT(19)
 #define PMBUS_HAVE_PWM12	BIT(20)
 #define PMBUS_HAVE_PWM34	BIT(21)
+#define PMBUS_HAVE_VOUT_COMMAND		BIT(22)
+#define PMBUS_HAVE_VOUT_MARGIN_HIGH	BIT(23)
+#define PMBUS_HAVE_VOUT_MARGIN_LOW	BIT(24)
 
 #define PMBUS_PAGE_VIRTUAL	BIT(31)
 
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 2e2b5851139c..f35b239961e3 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -89,6 +89,14 @@ struct pmbus_label {
 #define to_pmbus_label(_attr) \
 	container_of(_attr, struct pmbus_label, attribute)
 
+struct pmbus_operation {
+	char name[PMBUS_NAME_SIZE];	/* sysfs label name */
+	struct sensor_device_attribute attribute;
+	u8 page;
+};
+#define to_pmbus_operation(_attr) \
+	container_of(_attr, struct pmbus_operation, attribute)
+
 struct pmbus_data {
 	struct device *dev;
 	struct device *hwmon_dev;
@@ -1004,6 +1012,65 @@ static ssize_t pmbus_show_label(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
 }
 
+static ssize_t pmbus_show_operation(struct device *dev,
+				    struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pmbus_operation *operation = to_pmbus_operation(attr);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+
+	ret = pmbus_read_byte_data(client, operation->page, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	if (ret & PB_OPERATION_CONTROL_ON) {
+		if (ret & PB_OPERATION_MARGIN_HIGH)
+			return snprintf(buf, PAGE_SIZE, "high\n");
+		else if (ret & PB_OPERATION_MARGIN_LOW)
+			return snprintf(buf, PAGE_SIZE, "low\n");
+		else
+			return snprintf(buf, PAGE_SIZE, "on\n");
+	} else {
+			return snprintf(buf, PAGE_SIZE, "off\n");
+	}
+}
+
+static ssize_t pmbus_set_operation(struct device *dev,
+				   struct device_attribute *devattr,
+				   const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pmbus_operation *operation = to_pmbus_operation(attr);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+	u8 val;
+
+	/*
+	 * sysfs_streq() doesn't need the \n's, but we add them so the strings
+	 * will be shared with pmbus_show_operation() above.
+	 */
+	if (sysfs_streq(buf, "on\n"))
+		val = PB_OPERATION_CONTROL_ON;
+	else if (sysfs_streq(buf, "off\n"))
+		val = 0;
+	else if (sysfs_streq(buf, "high\n"))
+		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
+		      PB_OPERATION_MARGIN_HIGH;
+	else if (sysfs_streq(buf, "low\n"))
+		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
+		      PB_OPERATION_MARGIN_LOW;
+	else
+		return -EINVAL;
+
+	ret = pmbus_write_byte_data(client, operation->page,
+				    PMBUS_OPERATION, val);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
 static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
 {
 	if (data->num_attributes >= data->max_attributes - 1) {
@@ -1143,6 +1210,29 @@ static int pmbus_add_label(struct pmbus_data *data,
 	return pmbus_add_attribute(data, &a->attr);
 }
 
+static int pmbus_add_operation(struct pmbus_data *data, const char *name,
+			       int seq, int page)
+{
+	struct pmbus_operation *operation;
+	struct sensor_device_attribute *a;
+
+	operation = devm_kzalloc(data->dev, sizeof(*operation), GFP_KERNEL);
+	if (!operation)
+		return -ENOMEM;
+
+	a = &operation->attribute;
+
+	snprintf(operation->name, sizeof(operation->name), "%s%d_operation",
+		 name, seq);
+
+	operation->page = page;
+
+	pmbus_attr_init(a, operation->name, S_IRUGO | S_IWUSR,
+			pmbus_show_operation, pmbus_set_operation, seq);
+
+	return pmbus_add_attribute(data, &a->dev_attr.attr);
+}
+
 /*
  * Search for attributes. Allocate sensors, booleans, and labels as needed.
  */
@@ -1901,6 +1991,93 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
 	return 0;
 }
 
+static const struct pmbus_limit_attr vout_cmd_limit_attrs[] = {
+	{
+		.reg = PMBUS_VOUT_MAX,
+		.attr = "max",
+		.alarm = "max_alarm",
+		.sbit = PB_VOLTAGE_MAX_WARNING,
+	}
+};
+
+static const struct pmbus_sensor_attr vout_attributes[] = {
+	{
+		.reg = PMBUS_VOUT_COMMAND,
+		.class = PSC_VOLTAGE_OUT,
+		.label = "command",
+		.paged = true,
+		.func = PMBUS_HAVE_VOUT_COMMAND,
+		.sfunc = PMBUS_HAVE_STATUS_VOUT,
+		.sbase = PB_STATUS_VOUT_BASE,
+		.gbit = PB_STATUS_VOUT_OV,
+		.limit = vout_cmd_limit_attrs,
+		.nlimit = ARRAY_SIZE(vout_cmd_limit_attrs),
+	}, {
+		.reg = PMBUS_VOUT_MARGIN_HIGH,
+		.class = PSC_VOLTAGE_OUT,
+		.label = "margin_high",
+		.paged = true,
+		.func = PMBUS_HAVE_VOUT_MARGIN_HIGH,
+	}, {
+		.reg = PMBUS_VOUT_MARGIN_LOW,
+		.class = PSC_VOLTAGE_OUT,
+		.label = "margin_low",
+		.paged = true,
+		.func = PMBUS_HAVE_VOUT_MARGIN_LOW,
+	}
+};
+
+static int pmbus_add_vout_attrs(struct i2c_client *client,
+				struct pmbus_data *data,
+				const char *name,
+				const struct pmbus_sensor_attr *attr,
+				int nattrs)
+{
+	const struct pmbus_driver_info *info = data->info;
+	struct pmbus_sensor *base;
+	int i, ret, index, page, pages;
+
+	index = 1;
+	for (page = 0; page < info->pages; page++) {
+		if (!pmbus_check_byte_register(client, page, PMBUS_OPERATION))
+			continue;
+		ret = pmbus_add_operation(data, name, index, page);
+		if (ret)
+			return ret;
+		index++;
+	}
+
+	for (i = 0; i < nattrs; i++) {
+		index = 1;
+		pages = attr->paged ? info->pages : 1;
+		for (page = 0; page < pages; page++) {
+			if (!(info->func[page] & attr->func))
+				continue;
+
+			if (!pmbus_check_word_register(client, page, attr->reg))
+				continue;
+
+			base = pmbus_add_sensor(data, name, attr->label, index,
+						page, attr->reg, attr->class,
+						true, false, true);
+			if (!base)
+				return -ENOMEM;
+
+			if (attr->sfunc) {
+				ret = pmbus_add_limit_attrs(client, data, info,
+							    name, index, page,
+							    base, attr);
+				if (ret < 0)
+					return ret;
+			}
+
+			index++;
+		}
+		attr++;
+	}
+	return 0;
+}
+
 static int pmbus_find_attributes(struct i2c_client *client,
 				 struct pmbus_data *data)
 {
@@ -1912,6 +2089,12 @@ static int pmbus_find_attributes(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	/* Output Voltage sensors */
+	ret = pmbus_add_vout_attrs(client, data, "out", vout_attributes,
+				   ARRAY_SIZE(vout_attributes));
+	if (ret)
+		return ret;
+
 	/* Current sensors */
 	ret = pmbus_add_sensor_attrs(client, data, "curr", current_attributes,
 				     ARRAY_SIZE(current_attributes));
-- 
2.17.1

Re: [PATCH 1/4] hwmon: (pmbus) add support for output voltage registers

[Guenter Roeck]

On Tue, Apr 16, 2019 at 11:36:16AM -0700, Ruslan Babayev wrote:
> Registers VOUT_COMMAND, VOUT_MARGIN_HIGH and VOUT_MARGIN_LOW are
> described in PMBUS Spec Part II Rev 1.2. Exposing them in the PMBUS
> core allows the drivers to turn them on with a corresponding
> PMBUS_HAVE_VOUT_... flags.
> 
> Cc: xe-linux-external@cisco.com
> Signed-off-by: Ruslan Babayev <ruslan@babayev.com>
> ---
>  drivers/hwmon/pmbus/pmbus.h      |   7 ++
>  drivers/hwmon/pmbus/pmbus_core.c | 183 +++++++++++++++++++++++++++++++
>  2 files changed, 190 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index 1d24397d36ec..723648b3da36 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -223,6 +223,9 @@ enum pmbus_regs {
>   * OPERATION
>   */
>  #define PB_OPERATION_CONTROL_ON		BIT(7)
> +#define PB_OPERATION_MARGIN_HIGH	BIT(5)
> +#define PB_OPERATION_MARGIN_LOW		BIT(4)
> +#define PB_OPERATION_ACT_ON_FAULT	BIT(3)
>  
>  /*
>   * CAPABILITY
> @@ -291,6 +294,7 @@ enum pmbus_fan_mode { percent = 0, rpm };
>  /*
>   * STATUS_VOUT, STATUS_INPUT
>   */
> +#define PB_VOLTAGE_MAX_WARNING		BIT(3)
>  #define PB_VOLTAGE_UV_FAULT		BIT(4)
>  #define PB_VOLTAGE_UV_WARNING		BIT(5)
>  #define PB_VOLTAGE_OV_WARNING		BIT(6)
> @@ -371,6 +375,9 @@ enum pmbus_sensor_classes {
>  #define PMBUS_HAVE_STATUS_VMON	BIT(19)
>  #define PMBUS_HAVE_PWM12	BIT(20)
>  #define PMBUS_HAVE_PWM34	BIT(21)
> +#define PMBUS_HAVE_VOUT_COMMAND		BIT(22)
> +#define PMBUS_HAVE_VOUT_MARGIN_HIGH	BIT(23)
> +#define PMBUS_HAVE_VOUT_MARGIN_LOW	BIT(24)
>  
>  #define PMBUS_PAGE_VIRTUAL	BIT(31)
>  
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 2e2b5851139c..f35b239961e3 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -89,6 +89,14 @@ struct pmbus_label {
>  #define to_pmbus_label(_attr) \
>  	container_of(_attr, struct pmbus_label, attribute)
>  
> +struct pmbus_operation {
> +	char name[PMBUS_NAME_SIZE];	/* sysfs label name */
> +	struct sensor_device_attribute attribute;
> +	u8 page;
> +};
> +#define to_pmbus_operation(_attr) \
> +	container_of(_attr, struct pmbus_operation, attribute)
> +
>  struct pmbus_data {
>  	struct device *dev;
>  	struct device *hwmon_dev;
> @@ -1004,6 +1012,65 @@ static ssize_t pmbus_show_label(struct device *dev,
>  	return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
>  }
>  
> +static ssize_t pmbus_show_operation(struct device *dev,
> +				    struct device_attribute *devattr, char *buf)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct pmbus_operation *operation = to_pmbus_operation(attr);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	int ret;
> +
> +	ret = pmbus_read_byte_data(client, operation->page, PMBUS_OPERATION);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret & PB_OPERATION_CONTROL_ON) {
> +		if (ret & PB_OPERATION_MARGIN_HIGH)
> +			return snprintf(buf, PAGE_SIZE, "high\n");
> +		else if (ret & PB_OPERATION_MARGIN_LOW)
> +			return snprintf(buf, PAGE_SIZE, "low\n");
> +		else
> +			return snprintf(buf, PAGE_SIZE, "on\n");
> +	} else {
> +			return snprintf(buf, PAGE_SIZE, "off\n");
> +	}
> +}
> +
> +static ssize_t pmbus_set_operation(struct device *dev,
> +				   struct device_attribute *devattr,
> +				   const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> +	struct pmbus_operation *operation = to_pmbus_operation(attr);
> +	struct i2c_client *client = to_i2c_client(dev->parent);
> +	int ret;
> +	u8 val;
> +
> +	/*
> +	 * sysfs_streq() doesn't need the \n's, but we add them so the strings
> +	 * will be shared with pmbus_show_operation() above.
> +	 */

Saving a few bytes of data in the driver is not worth such limitations. 
I would not want to have to deal with users sending "echo -n on" to the
driver and complain that it doesn't work.

> +	if (sysfs_streq(buf, "on\n"))
> +		val = PB_OPERATION_CONTROL_ON;
> +	else if (sysfs_streq(buf, "off\n"))
> +		val = 0;
> +	else if (sysfs_streq(buf, "high\n"))
> +		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
> +		      PB_OPERATION_MARGIN_HIGH;
> +	else if (sysfs_streq(buf, "low\n"))
> +		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
> +		      PB_OPERATION_MARGIN_LOW;
> +	else
> +		return -EINVAL;
> +
> +	ret = pmbus_write_byte_data(client, operation->page,
> +				    PMBUS_OPERATION, val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return count;
> +}
> +
I am not convinced that it is a good idea to implement this as sysfs
attributes. It is notmally only used in manufacturing, and there the
registers can be acccessed directly.

Either case, we can not implement this in the pmbus driver as generic code.
Operation commands are notoriously implemented with slight differences across
different devices. It would also probably be more appropriate to implement
as regulator, since it affects setting voltages, not limits.

>  static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
>  {
>  	if (data->num_attributes >= data->max_attributes - 1) {
> @@ -1143,6 +1210,29 @@ static int pmbus_add_label(struct pmbus_data *data,
>  	return pmbus_add_attribute(data, &a->attr);
>  }
>  
> +static int pmbus_add_operation(struct pmbus_data *data, const char *name,
> +			       int seq, int page)
> +{
> +	struct pmbus_operation *operation;
> +	struct sensor_device_attribute *a;
> +
> +	operation = devm_kzalloc(data->dev, sizeof(*operation), GFP_KERNEL);
> +	if (!operation)
> +		return -ENOMEM;
> +
> +	a = &operation->attribute;
> +
> +	snprintf(operation->name, sizeof(operation->name), "%s%d_operation",
> +		 name, seq);
> +
> +	operation->page = page;
> +
> +	pmbus_attr_init(a, operation->name, S_IRUGO | S_IWUSR,
> +			pmbus_show_operation, pmbus_set_operation, seq);
> +
> +	return pmbus_add_attribute(data, &a->dev_attr.attr);
> +}
> +
>  /*
>   * Search for attributes. Allocate sensors, booleans, and labels as needed.
>   */
> @@ -1901,6 +1991,93 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static const struct pmbus_limit_attr vout_cmd_limit_attrs[] = {
> +	{
> +		.reg = PMBUS_VOUT_MAX,
> +		.attr = "max",
> +		.alarm = "max_alarm",
> +		.sbit = PB_VOLTAGE_MAX_WARNING,
> +	}
> +};
> +
> +static const struct pmbus_sensor_attr vout_attributes[] = {
> +	{
> +		.reg = PMBUS_VOUT_COMMAND,
> +		.class = PSC_VOLTAGE_OUT,
> +		.label = "command",

Bad name for a label.

> +		.paged = true,
> +		.func = PMBUS_HAVE_VOUT_COMMAND,
> +		.sfunc = PMBUS_HAVE_STATUS_VOUT,
> +		.sbase = PB_STATUS_VOUT_BASE,
> +		.gbit = PB_STATUS_VOUT_OV,
> +		.limit = vout_cmd_limit_attrs,
> +		.nlimit = ARRAY_SIZE(vout_cmd_limit_attrs),
> +	}, {
> +		.reg = PMBUS_VOUT_MARGIN_HIGH,
> +		.class = PSC_VOLTAGE_OUT,
> +		.label = "margin_high",
> +		.paged = true,
> +		.func = PMBUS_HAVE_VOUT_MARGIN_HIGH,
> +	}, {
> +		.reg = PMBUS_VOUT_MARGIN_LOW,
> +		.class = PSC_VOLTAGE_OUT,
> +		.label = "margin_low",
> +		.paged = true,
> +		.func = PMBUS_HAVE_VOUT_MARGIN_LOW,
> +	}
> +};
> +
> +static int pmbus_add_vout_attrs(struct i2c_client *client,
> +				struct pmbus_data *data,
> +				const char *name,
> +				const struct pmbus_sensor_attr *attr,
> +				int nattrs)
> +{
> +	const struct pmbus_driver_info *info = data->info;
> +	struct pmbus_sensor *base;
> +	int i, ret, index, page, pages;
> +
> +	index = 1;
> +	for (page = 0; page < info->pages; page++) {
> +		if (!pmbus_check_byte_register(client, page, PMBUS_OPERATION))
> +			continue;
> +		ret = pmbus_add_operation(data, name, index, page);
> +		if (ret)
> +			return ret;
> +		index++;
> +	}
> +
> +	for (i = 0; i < nattrs; i++) {
> +		index = 1;
> +		pages = attr->paged ? info->pages : 1;
> +		for (page = 0; page < pages; page++) {
> +			if (!(info->func[page] & attr->func))
> +				continue;
> +
> +			if (!pmbus_check_word_register(client, page, attr->reg))
> +				continue;
> +
> +			base = pmbus_add_sensor(data, name, attr->label, index,
> +						page, attr->reg, attr->class,
> +						true, false, true);
> +			if (!base)
> +				return -ENOMEM;
> +
> +			if (attr->sfunc) {
> +				ret = pmbus_add_limit_attrs(client, data, info,
> +							    name, index, page,
> +							    base, attr);
> +				if (ret < 0)
> +					return ret;
> +			}
> +
> +			index++;
> +		}
> +		attr++;
> +	}
> +	return 0;
> +}
> +
>  static int pmbus_find_attributes(struct i2c_client *client,
>  				 struct pmbus_data *data)
>  {
> @@ -1912,6 +2089,12 @@ static int pmbus_find_attributes(struct i2c_client *client,
>  	if (ret)
>  		return ret;
>  
> +	/* Output Voltage sensors */
> +	ret = pmbus_add_vout_attrs(client, data, "out", vout_attributes,
> +				   ARRAY_SIZE(vout_attributes));
> +	if (ret)
> +		return ret;
> +

The sensors you are adding are not output voltage sensors. They are not
sensors in the first place, and should probably not be reported as sensor
values. And setting voltages as limits is really not appropriate.

>  	/* Current sensors */
>  	ret = pmbus_add_sensor_attrs(client, data, "curr", current_attributes,
>  				     ARRAY_SIZE(current_attributes));
> -- 
> 2.17.1
> 

Re: [PATCH 1/4] hwmon: (pmbus) add support for output voltage registers

[Ruslan Babayev]

Hi Guenter,

Thanks for taking the time to review these patches. Please see my
comments inline.

Guenter Roeck writes:

> On Tue, Apr 16, 2019 at 11:36:16AM -0700, Ruslan Babayev wrote:
>> +static ssize_t pmbus_show_operation(struct device *dev,
>> +				    struct device_attribute *devattr, char *buf)
>> +{
>> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +	struct pmbus_operation *operation = to_pmbus_operation(attr);
>> +	struct i2c_client *client = to_i2c_client(dev->parent);
>> +	int ret;
>> +
>> +	ret = pmbus_read_byte_data(client, operation->page, PMBUS_OPERATION);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ret & PB_OPERATION_CONTROL_ON) {
>> +		if (ret & PB_OPERATION_MARGIN_HIGH)
>> +			return snprintf(buf, PAGE_SIZE, "high\n");
>> +		else if (ret & PB_OPERATION_MARGIN_LOW)
>> +			return snprintf(buf, PAGE_SIZE, "low\n");
>> +		else
>> +			return snprintf(buf, PAGE_SIZE, "on\n");
>> +	} else {
>> +			return snprintf(buf, PAGE_SIZE, "off\n");
>> +	}
>> +}
>> +
>> +static ssize_t pmbus_set_operation(struct device *dev,
>> +				   struct device_attribute *devattr,
>> +				   const char *buf, size_t count)
>> +{
>> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +	struct pmbus_operation *operation = to_pmbus_operation(attr);
>> +	struct i2c_client *client = to_i2c_client(dev->parent);
>> +	int ret;
>> +	u8 val;
>> +
>> +	/*
>> +	 * sysfs_streq() doesn't need the \n's, but we add them so the strings
>> +	 * will be shared with pmbus_show_operation() above.
>> +	 */
>
> Saving a few bytes of data in the driver is not worth such limitations.
> I would not want to have to deal with users sending "echo -n on" to the
> driver and complain that it doesn't work.
>

Sending "echo -n on" works just fine. sysfs_streq ignores \n's as
mentioned in the comment. This was actually shamelessly stolen from
drivers/regulator/virtual.c set_mode().

>> +	if (sysfs_streq(buf, "on\n"))
>> +		val = PB_OPERATION_CONTROL_ON;
>> +	else if (sysfs_streq(buf, "off\n"))
>> +		val = 0;
>> +	else if (sysfs_streq(buf, "high\n"))
>> +		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
>> +		      PB_OPERATION_MARGIN_HIGH;
>> +	else if (sysfs_streq(buf, "low\n"))
>> +		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
>> +		      PB_OPERATION_MARGIN_LOW;
>> +	else
>> +		return -EINVAL;
>> +
>> +	ret = pmbus_write_byte_data(client, operation->page,
>> +				    PMBUS_OPERATION, val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
> I am not convinced that it is a good idea to implement this as sysfs
> attributes. It is notmally only used in manufacturing, and there the
> registers can be acccessed directly.
>
> Either case, we can not implement this in the pmbus driver as generic code.
> Operation commands are notoriously implemented with slight differences across
> different devices. It would also probably be more appropriate to implement
> as regulator, since it affects setting voltages, not limits.
>

The OPERATION command as well as VOUT_COMMAND and VOUT_MARGIN_{HIGH,LOW}
are standard registers specified in PMBus Spec Part II, Rev 1.2 Section
13. "Output Voltage Related Commands". The devices I have seen so far
(the ones that support voltage margining) all seem to be compliant with
the standard as far as the OPERATION register is concerned.

Some devices like TPS40422 use manufacturer specific registers instead of
VOUT_MARGIN_{HIGH,LOW}. Those can be handled by overriding
{read,write}_word_data to make it look standard to pmbus_core (as I have
done in the PATCH 4/4)

>> +
>> +static const struct pmbus_sensor_attr vout_attributes[] = {
>> +	{
>> +		.reg = PMBUS_VOUT_COMMAND,
>> +		.class = PSC_VOLTAGE_OUT,
>> +		.label = "command",
>
> Bad name for a label.

Could you please suggest a better name?

>>  static int pmbus_find_attributes(struct i2c_client *client,
>>  				 struct pmbus_data *data)
>>  {
>> @@ -1912,6 +2089,12 @@ static int pmbus_find_attributes(struct i2c_client *client,
>>  	if (ret)
>>  		return ret;
>>
>> +	/* Output Voltage sensors */
>> +	ret = pmbus_add_vout_attrs(client, data, "out", vout_attributes,
>> +				   ARRAY_SIZE(vout_attributes));
>> +	if (ret)
>> +		return ret;
>> +
>
> The sensors you are adding are not output voltage sensors. They are not
> sensors in the first place, and should probably not be reported as sensor
> values. And setting voltages as limits is really not appropriate.
>

I understand your concerns about exposing these registers as sysfs
entries in hwmon. These devices _are_ voltage regulators, and these
registers are not exactly "sensors", although a lot of the existing
pmbus_core code can be reused by pretending they are. Like VOUT_MODE
exponent handling, linear16 format etc.

I have looked at the regulator framework and into extending the
pmbus_regulator_ops to support voltage margining with standard PMBUS
VOUT_MARGIN registers. My predicament is that regulator framework

1) Does NOT have the concept of "margining".

2) Is not expected to be used at ALL on ACPI based systems. With ACPI
   kernel assumes power control is done with Power Resource Objects
   (ACPI section 7.1).

The latter is a bigger issue for us - our system is Intel based. It
seems like sysfs attributes is our only option.

How would you feel if these registers (namely OPERATION and
VOUT_MARGIN_{HIGH,LOW}) were exposed only in the drivers without making
any changes to pmbus_core.c? Would these drivers have a chance of
getting upstreamed? I hate the idea of maintaining out-of-tree drivers.

Best regards,
Ruslan

Re: [PATCH 1/4] hwmon: (pmbus) add support for output voltage registers

[Ruslan Babayev]

Hi Guenter,

Thanks for taking the time to review these patches. Please see my
comments inline.

Guenter Roeck writes:

> On Tue, Apr 16, 2019 at 11:36:16AM -0700, Ruslan Babayev wrote:
>> +static ssize_t pmbus_show_operation(struct device *dev,
>> +				    struct device_attribute *devattr, char *buf)
>> +{
>> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +	struct pmbus_operation *operation = to_pmbus_operation(attr);
>> +	struct i2c_client *client = to_i2c_client(dev->parent);
>> +	int ret;
>> +
>> +	ret = pmbus_read_byte_data(client, operation->page, PMBUS_OPERATION);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (ret & PB_OPERATION_CONTROL_ON) {
>> +		if (ret & PB_OPERATION_MARGIN_HIGH)
>> +			return snprintf(buf, PAGE_SIZE, "high\n");
>> +		else if (ret & PB_OPERATION_MARGIN_LOW)
>> +			return snprintf(buf, PAGE_SIZE, "low\n");
>> +		else
>> +			return snprintf(buf, PAGE_SIZE, "on\n");
>> +	} else {
>> +			return snprintf(buf, PAGE_SIZE, "off\n");
>> +	}
>> +}
>> +
>> +static ssize_t pmbus_set_operation(struct device *dev,
>> +				   struct device_attribute *devattr,
>> +				   const char *buf, size_t count)
>> +{
>> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> +	struct pmbus_operation *operation = to_pmbus_operation(attr);
>> +	struct i2c_client *client = to_i2c_client(dev->parent);
>> +	int ret;
>> +	u8 val;
>> +
>> +	/*
>> +	 * sysfs_streq() doesn't need the \n's, but we add them so the strings
>> +	 * will be shared with pmbus_show_operation() above.
>> +	 */
>
> Saving a few bytes of data in the driver is not worth such limitations.
> I would not want to have to deal with users sending "echo -n on" to the
> driver and complain that it doesn't work.
>

Sending "echo -n on" works just fine. sysfs_streq ignores \n's as
mentioned in the comment. This was actually shamelessly stolen from
drivers/regulator/virtual.c set_mode().

>> +	if (sysfs_streq(buf, "on\n"))
>> +		val = PB_OPERATION_CONTROL_ON;
>> +	else if (sysfs_streq(buf, "off\n"))
>> +		val = 0;
>> +	else if (sysfs_streq(buf, "high\n"))
>> +		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
>> +		      PB_OPERATION_MARGIN_HIGH;
>> +	else if (sysfs_streq(buf, "low\n"))
>> +		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
>> +		      PB_OPERATION_MARGIN_LOW;
>> +	else
>> +		return -EINVAL;
>> +
>> +	ret = pmbus_write_byte_data(client, operation->page,
>> +				    PMBUS_OPERATION, val);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +
> I am not convinced that it is a good idea to implement this as sysfs
> attributes. It is notmally only used in manufacturing, and there the
> registers can be acccessed directly.
>
> Either case, we can not implement this in the pmbus driver as generic code.
> Operation commands are notoriously implemented with slight differences across
> different devices. It would also probably be more appropriate to implement
> as regulator, since it affects setting voltages, not limits.
>

The OPERATION command as well as VOUT_COMMAND and VOUT_MARGIN_{HIGH,LOW}
are standard registers specified in PMBus Spec Part II, Rev 1.2 Section
13. "Output Voltage Related Commands". The devices I have seen so far
(the ones that support voltage margining) all seem to be compliant with
the standard as far as the OPERATION register is concerned.

Some devices like TPS40422 use manufacturer specific registers instead of
VOUT_MARGIN_{HIGH,LOW}. Those can be handled by overriding
{read,write}_word_data to make it look standard to pmbus_core (as I have
done in the PATCH 4/4)

>> +
>> +static const struct pmbus_sensor_attr vout_attributes[] = {
>> +	{
>> +		.reg = PMBUS_VOUT_COMMAND,
>> +		.class = PSC_VOLTAGE_OUT,
>> +		.label = "command",
>
> Bad name for a label.

Could you please suggest a better name?

>>  static int pmbus_find_attributes(struct i2c_client *client,
>>  				 struct pmbus_data *data)
>>  {
>> @@ -1912,6 +2089,12 @@ static int pmbus_find_attributes(struct i2c_client *client,
>>  	if (ret)
>>  		return ret;
>>
>> +	/* Output Voltage sensors */
>> +	ret = pmbus_add_vout_attrs(client, data, "out", vout_attributes,
>> +				   ARRAY_SIZE(vout_attributes));
>> +	if (ret)
>> +		return ret;
>> +
>
> The sensors you are adding are not output voltage sensors. They are not
> sensors in the first place, and should probably not be reported as sensor
> values. And setting voltages as limits is really not appropriate.
>

I understand your concerns about exposing these registers as sysfs
entries in hwmon. These devices _are_ voltage regulators, and these
registers are not exactly "sensors", although a lot of the existing
pmbus_core code can be reused by pretending they are. Like VOUT_MODE
exponent handling, linear16 format etc.

I have looked at the regulator framework and into extending the
pmbus_regulator_ops to support voltage margining with standard PMBUS
VOUT_MARGIN registers. My predicament is that regulator framework

1) Does NOT have the concept of "margining".

2) Is not expected to be used at ALL on ACPI based systems. With ACPI
   kernel assumes power control is done with Power Resource Objects
   (ACPI section 7.1).

The latter is a bigger issue for us - our system is Intel based. It
seems like sysfs attributes is our only option.

How would you feel if these registers (namely OPERATION and
VOUT_MARGIN_{HIGH,LOW}) were exposed only in the drivers without making
any changes to pmbus_core.c? Would these drivers have a chance of
getting upstreamed? I hate the idea of maintaining out-of-tree drivers.

Best regards,
Ruslan

Re: [PATCH 1/4] hwmon: (pmbus) add support for output voltage registers

[Guenter Roeck]

On Tue, Apr 16, 2019 at 06:57:18PM -0700, Ruslan Babayev wrote:
> Hi Guenter,
> 
> Thanks for taking the time to review these patches. Please see my
> comments inline.
> 
> Guenter Roeck writes:
> 
> > On Tue, Apr 16, 2019 at 11:36:16AM -0700, Ruslan Babayev wrote:
> >> +static ssize_t pmbus_show_operation(struct device *dev,
> >> +				    struct device_attribute *devattr, char *buf)
> >> +{
> >> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> >> +	struct pmbus_operation *operation = to_pmbus_operation(attr);
> >> +	struct i2c_client *client = to_i2c_client(dev->parent);
> >> +	int ret;
> >> +
> >> +	ret = pmbus_read_byte_data(client, operation->page, PMBUS_OPERATION);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	if (ret & PB_OPERATION_CONTROL_ON) {
> >> +		if (ret & PB_OPERATION_MARGIN_HIGH)
> >> +			return snprintf(buf, PAGE_SIZE, "high\n");
> >> +		else if (ret & PB_OPERATION_MARGIN_LOW)
> >> +			return snprintf(buf, PAGE_SIZE, "low\n");
> >> +		else
> >> +			return snprintf(buf, PAGE_SIZE, "on\n");
> >> +	} else {
> >> +			return snprintf(buf, PAGE_SIZE, "off\n");
> >> +	}
> >> +}
> >> +
> >> +static ssize_t pmbus_set_operation(struct device *dev,
> >> +				   struct device_attribute *devattr,
> >> +				   const char *buf, size_t count)
> >> +{
> >> +	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> >> +	struct pmbus_operation *operation = to_pmbus_operation(attr);
> >> +	struct i2c_client *client = to_i2c_client(dev->parent);
> >> +	int ret;
> >> +	u8 val;
> >> +
> >> +	/*
> >> +	 * sysfs_streq() doesn't need the \n's, but we add them so the strings
> >> +	 * will be shared with pmbus_show_operation() above.
> >> +	 */
> >
> > Saving a few bytes of data in the driver is not worth such limitations.
> > I would not want to have to deal with users sending "echo -n on" to the
> > driver and complain that it doesn't work.
> >
> 
> Sending "echo -n on" works just fine. sysfs_streq ignores \n's as
> mentioned in the comment. This was actually shamelessly stolen from
> drivers/regulator/virtual.c set_mode().
> 

Still not a reason to do this.

> >> +	if (sysfs_streq(buf, "on\n"))
> >> +		val = PB_OPERATION_CONTROL_ON;
> >> +	else if (sysfs_streq(buf, "off\n"))
> >> +		val = 0;
> >> +	else if (sysfs_streq(buf, "high\n"))
> >> +		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
> >> +		      PB_OPERATION_MARGIN_HIGH;
> >> +	else if (sysfs_streq(buf, "low\n"))
> >> +		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
> >> +		      PB_OPERATION_MARGIN_LOW;
> >> +	else
> >> +		return -EINVAL;
> >> +
> >> +	ret = pmbus_write_byte_data(client, operation->page,
> >> +				    PMBUS_OPERATION, val);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	return count;
> >> +}
> >> +
> > I am not convinced that it is a good idea to implement this as sysfs
> > attributes. It is notmally only used in manufacturing, and there the
> > registers can be acccessed directly.
> >
> > Either case, we can not implement this in the pmbus driver as generic code.
> > Operation commands are notoriously implemented with slight differences across
> > different devices. It would also probably be more appropriate to implement
> > as regulator, since it affects setting voltages, not limits.
> >
> 
> The OPERATION command as well as VOUT_COMMAND and VOUT_MARGIN_{HIGH,LOW}
> are standard registers specified in PMBus Spec Part II, Rev 1.2 Section
> 13. "Output Voltage Related Commands". The devices I have seen so far
> (the ones that support voltage margining) all seem to be compliant with
> the standard as far as the OPERATION register is concerned.
> 
> Some devices like TPS40422 use manufacturer specific registers instead of
> VOUT_MARGIN_{HIGH,LOW}. Those can be handled by overriding
> {read,write}_word_data to make it look standard to pmbus_core (as I have
> done in the PATCH 4/4)
> 
The more I think about it, the more I am convinced that it is a bad idea to
expose those values. It is not just about the hwmon subsystem, which again
is for monitoring, not for setting or enabling voltages. Much worse is that
your suggested code actually lets people _set_ arbitrary voltages. Write the
wrong value, and your board goes up in smoke (or fire). This goes way beyond
what the regulator subsystem permits, where voltages are (at least typically)
only adjustable within given limits.

On top of that, I would really argue that such attributes have no place
in a shipping system, ie should not be visible to users. The expected use case
for margin values is to be able to enable them during production tests.
But that does not (or should not) happen in the field.

Many if not most PMBus chips actually have such critical settings marked as
read-only, sometimes even password protected. Any shipping system should have
that protection enabled to prevent accidental writes into the chip registers.
At one of a previous employers, such write protection was not enabled. This
caused and i2cdump on the chip to reset it to manufacturing state, turning off
power to itself and thus turning the board into a brick. That is really not
something we would want to have exposed with sysfs attributes.

> >> +
> >> +static const struct pmbus_sensor_attr vout_attributes[] = {
> >> +	{
> >> +		.reg = PMBUS_VOUT_COMMAND,
> >> +		.class = PSC_VOLTAGE_OUT,
> >> +		.label = "command",
> >
> > Bad name for a label.
> 
> Could you please suggest a better name?
> 
"target" or "vout_target" maybe, for target voltage.

> >>  static int pmbus_find_attributes(struct i2c_client *client,
> >>  				 struct pmbus_data *data)
> >>  {
> >> @@ -1912,6 +2089,12 @@ static int pmbus_find_attributes(struct i2c_client *client,
> >>  	if (ret)
> >>  		return ret;
> >>
> >> +	/* Output Voltage sensors */
> >> +	ret = pmbus_add_vout_attrs(client, data, "out", vout_attributes,
> >> +				   ARRAY_SIZE(vout_attributes));
> >> +	if (ret)
> >> +		return ret;
> >> +
> >
> > The sensors you are adding are not output voltage sensors. They are not
> > sensors in the first place, and should probably not be reported as sensor
> > values. And setting voltages as limits is really not appropriate.
> >
> 
> I understand your concerns about exposing these registers as sysfs
> entries in hwmon. These devices _are_ voltage regulators, and these
> registers are not exactly "sensors", although a lot of the existing
> pmbus_core code can be reused by pretending they are. Like VOUT_MODE
> exponent handling, linear16 format etc.
> 

I understand, but code reusability isn't really a good argument here,
much less for exposing the values as hwmon sysfs attributes.

> I have looked at the regulator framework and into extending the
> pmbus_regulator_ops to support voltage margining with standard PMBUS
> VOUT_MARGIN registers. My predicament is that regulator framework
> 
> 1) Does NOT have the concept of "margining".
> 
> 2) Is not expected to be used at ALL on ACPI based systems. With ACPI
>    kernel assumes power control is done with Power Resource Objects
>    (ACPI section 7.1).
> 
> The latter is a bigger issue for us - our system is Intel based. It
> seems like sysfs attributes is our only option.
> 
> How would you feel if these registers (namely OPERATION and
> VOUT_MARGIN_{HIGH,LOW}) were exposed only in the drivers without making
> any changes to pmbus_core.c? Would these drivers have a chance of
> getting upstreamed? I hate the idea of maintaining out-of-tree drivers.
> 
Same thing. Again, I don't think the attributes should be exposed
to userspace in the first place. Please have a look at your use case.
The only use case I can imagine is manufacturing tests, and those can
use i2cset -f directly (potentially in conjunction with the necessary
commands to enable/disable write protection for the respective commands,
if the chip supports it).

Thanks,
Guenter

[PATCH 1/4] hwmon: (pmbus) add support for output voltage registers

[Ruslan Babayev]

From: Ruslan Babayev <ruslan@babayev.com>

Registers VOUT_COMMAND, VOUT_MARGIN_HIGH and VOUT_MARGIN_LOW are
described in PMBUS Spec Part II Rev 1.2. Exposing them in the PMBUS
core allows the drivers to turn them on with a corresponding
PMBUS_HAVE_VOUT_... flags.

Cc: xe-linux-external@cisco.com
Signed-off-by: Ruslan Babayev <ruslan@babayev.com>
---
 drivers/hwmon/pmbus/pmbus.h      |   7 ++
 drivers/hwmon/pmbus/pmbus_core.c | 183 +++++++++++++++++++++++++++++++
 2 files changed, 190 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index 1d24397d36ec..723648b3da36 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -223,6 +223,9 @@ enum pmbus_regs {
  * OPERATION
  */
 #define PB_OPERATION_CONTROL_ON		BIT(7)
+#define PB_OPERATION_MARGIN_HIGH	BIT(5)
+#define PB_OPERATION_MARGIN_LOW		BIT(4)
+#define PB_OPERATION_ACT_ON_FAULT	BIT(3)
 
 /*
  * CAPABILITY
@@ -291,6 +294,7 @@ enum pmbus_fan_mode { percent = 0, rpm };
 /*
  * STATUS_VOUT, STATUS_INPUT
  */
+#define PB_VOLTAGE_MAX_WARNING		BIT(3)
 #define PB_VOLTAGE_UV_FAULT		BIT(4)
 #define PB_VOLTAGE_UV_WARNING		BIT(5)
 #define PB_VOLTAGE_OV_WARNING		BIT(6)
@@ -371,6 +375,9 @@ enum pmbus_sensor_classes {
 #define PMBUS_HAVE_STATUS_VMON	BIT(19)
 #define PMBUS_HAVE_PWM12	BIT(20)
 #define PMBUS_HAVE_PWM34	BIT(21)
+#define PMBUS_HAVE_VOUT_COMMAND		BIT(22)
+#define PMBUS_HAVE_VOUT_MARGIN_HIGH	BIT(23)
+#define PMBUS_HAVE_VOUT_MARGIN_LOW	BIT(24)
 
 #define PMBUS_PAGE_VIRTUAL	BIT(31)
 
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 2e2b5851139c..f35b239961e3 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -89,6 +89,14 @@ struct pmbus_label {
 #define to_pmbus_label(_attr) \
 	container_of(_attr, struct pmbus_label, attribute)
 
+struct pmbus_operation {
+	char name[PMBUS_NAME_SIZE];	/* sysfs label name */
+	struct sensor_device_attribute attribute;
+	u8 page;
+};
+#define to_pmbus_operation(_attr) \
+	container_of(_attr, struct pmbus_operation, attribute)
+
 struct pmbus_data {
 	struct device *dev;
 	struct device *hwmon_dev;
@@ -1004,6 +1012,65 @@ static ssize_t pmbus_show_label(struct device *dev,
 	return snprintf(buf, PAGE_SIZE, "%s\n", label->label);
 }
 
+static ssize_t pmbus_show_operation(struct device *dev,
+				    struct device_attribute *devattr, char *buf)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pmbus_operation *operation = to_pmbus_operation(attr);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+
+	ret = pmbus_read_byte_data(client, operation->page, PMBUS_OPERATION);
+	if (ret < 0)
+		return ret;
+
+	if (ret & PB_OPERATION_CONTROL_ON) {
+		if (ret & PB_OPERATION_MARGIN_HIGH)
+			return snprintf(buf, PAGE_SIZE, "high\n");
+		else if (ret & PB_OPERATION_MARGIN_LOW)
+			return snprintf(buf, PAGE_SIZE, "low\n");
+		else
+			return snprintf(buf, PAGE_SIZE, "on\n");
+	} else {
+			return snprintf(buf, PAGE_SIZE, "off\n");
+	}
+}
+
+static ssize_t pmbus_set_operation(struct device *dev,
+				   struct device_attribute *devattr,
+				   const char *buf, size_t count)
+{
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
+	struct pmbus_operation *operation = to_pmbus_operation(attr);
+	struct i2c_client *client = to_i2c_client(dev->parent);
+	int ret;
+	u8 val;
+
+	/*
+	 * sysfs_streq() doesn't need the \n's, but we add them so the strings
+	 * will be shared with pmbus_show_operation() above.
+	 */
+	if (sysfs_streq(buf, "on\n"))
+		val = PB_OPERATION_CONTROL_ON;
+	else if (sysfs_streq(buf, "off\n"))
+		val = 0;
+	else if (sysfs_streq(buf, "high\n"))
+		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
+		      PB_OPERATION_MARGIN_HIGH;
+	else if (sysfs_streq(buf, "low\n"))
+		val = PB_OPERATION_CONTROL_ON | PB_OPERATION_ACT_ON_FAULT |
+		      PB_OPERATION_MARGIN_LOW;
+	else
+		return -EINVAL;
+
+	ret = pmbus_write_byte_data(client, operation->page,
+				    PMBUS_OPERATION, val);
+	if (ret < 0)
+		return ret;
+
+	return count;
+}
+
 static int pmbus_add_attribute(struct pmbus_data *data, struct attribute *attr)
 {
 	if (data->num_attributes >= data->max_attributes - 1) {
@@ -1143,6 +1210,29 @@ static int pmbus_add_label(struct pmbus_data *data,
 	return pmbus_add_attribute(data, &a->attr);
 }
 
+static int pmbus_add_operation(struct pmbus_data *data, const char *name,
+			       int seq, int page)
+{
+	struct pmbus_operation *operation;
+	struct sensor_device_attribute *a;
+
+	operation = devm_kzalloc(data->dev, sizeof(*operation), GFP_KERNEL);
+	if (!operation)
+		return -ENOMEM;
+
+	a = &operation->attribute;
+
+	snprintf(operation->name, sizeof(operation->name), "%s%d_operation",
+		 name, seq);
+
+	operation->page = page;
+
+	pmbus_attr_init(a, operation->name, S_IRUGO | S_IWUSR,
+			pmbus_show_operation, pmbus_set_operation, seq);
+
+	return pmbus_add_attribute(data, &a->dev_attr.attr);
+}
+
 /*
  * Search for attributes. Allocate sensors, booleans, and labels as needed.
  */
@@ -1901,6 +1991,93 @@ static int pmbus_add_fan_attributes(struct i2c_client *client,
 	return 0;
 }
 
+static const struct pmbus_limit_attr vout_cmd_limit_attrs[] = {
+	{
+		.reg = PMBUS_VOUT_MAX,
+		.attr = "max",
+		.alarm = "max_alarm",
+		.sbit = PB_VOLTAGE_MAX_WARNING,
+	}
+};
+
+static const struct pmbus_sensor_attr vout_attributes[] = {
+	{
+		.reg = PMBUS_VOUT_COMMAND,
+		.class = PSC_VOLTAGE_OUT,
+		.label = "command",
+		.paged = true,
+		.func = PMBUS_HAVE_VOUT_COMMAND,
+		.sfunc = PMBUS_HAVE_STATUS_VOUT,
+		.sbase = PB_STATUS_VOUT_BASE,
+		.gbit = PB_STATUS_VOUT_OV,
+		.limit = vout_cmd_limit_attrs,
+		.nlimit = ARRAY_SIZE(vout_cmd_limit_attrs),
+	}, {
+		.reg = PMBUS_VOUT_MARGIN_HIGH,
+		.class = PSC_VOLTAGE_OUT,
+		.label = "margin_high",
+		.paged = true,
+		.func = PMBUS_HAVE_VOUT_MARGIN_HIGH,
+	}, {
+		.reg = PMBUS_VOUT_MARGIN_LOW,
+		.class = PSC_VOLTAGE_OUT,
+		.label = "margin_low",
+		.paged = true,
+		.func = PMBUS_HAVE_VOUT_MARGIN_LOW,
+	}
+};
+
+static int pmbus_add_vout_attrs(struct i2c_client *client,
+				struct pmbus_data *data,
+				const char *name,
+				const struct pmbus_sensor_attr *attr,
+				int nattrs)
+{
+	const struct pmbus_driver_info *info = data->info;
+	struct pmbus_sensor *base;
+	int i, ret, index, page, pages;
+
+	index = 1;
+	for (page = 0; page < info->pages; page++) {
+		if (!pmbus_check_byte_register(client, page, PMBUS_OPERATION))
+			continue;
+		ret = pmbus_add_operation(data, name, index, page);
+		if (ret)
+			return ret;
+		index++;
+	}
+
+	for (i = 0; i < nattrs; i++) {
+		index = 1;
+		pages = attr->paged ? info->pages : 1;
+		for (page = 0; page < pages; page++) {
+			if (!(info->func[page] & attr->func))
+				continue;
+
+			if (!pmbus_check_word_register(client, page, attr->reg))
+				continue;
+
+			base = pmbus_add_sensor(data, name, attr->label, index,
+						page, attr->reg, attr->class,
+						true, false, true);
+			if (!base)
+				return -ENOMEM;
+
+			if (attr->sfunc) {
+				ret = pmbus_add_limit_attrs(client, data, info,
+							    name, index, page,
+							    base, attr);
+				if (ret < 0)
+					return ret;
+			}
+
+			index++;
+		}
+		attr++;
+	}
+	return 0;
+}
+
 static int pmbus_find_attributes(struct i2c_client *client,
 				 struct pmbus_data *data)
 {
@@ -1912,6 +2089,12 @@ static int pmbus_find_attributes(struct i2c_client *client,
 	if (ret)
 		return ret;
 
+	/* Output Voltage sensors */
+	ret = pmbus_add_vout_attrs(client, data, "out", vout_attributes,
+				   ARRAY_SIZE(vout_attributes));
+	if (ret)
+		return ret;
+
 	/* Current sensors */
 	ret = pmbus_add_sensor_attrs(client, data, "curr", current_attributes,
 				     ARRAY_SIZE(current_attributes));
-- 
2.17.1