Add user space accessibility for ucd9000 stats

PATCH Superseded Christopher Bostic 2 participants 7 messages Aug 29, 2017 – Aug 30, 2017 Hardware monitoring

lore

Share & tools

lore

Christopher Bostic

author

2017-08-29 20:08 UTC #0 lore raw

v5 cover Add user space accessibility for ucd9000 stats

This set provides means to allow user space to access various properties of the ucd9000 type chips.

patch 0001 Adds gpio_chip interface for user to access various GPIO pin states via the /sys/class/gpio interface.

Patch 0002 adds a debugfs for mfr_status information so user space can gain visibility of gpi type faults. Note that there are limitations on type of devices that debugfs supports. Presently only ucd90160 is supported. Any assumptions made as to formatting of data is commented inline in the code. Any expansion to other ucd9000 types will need to take this into consideration.

Christopher Bostic (2):
hwmon: (ucd9000) Add gpio chip interface and clear logged faults
hwmon: (ucd9000) Add debugfs for mfr_status info

 drivers/hwmon/pmbus/ucd9000.c | 404 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 402 insertions(+), 2 deletions(-)

Signature · 1 line

1.8.2.2

Christopher Bostic

author

2017-08-29 20:08 UTC #1 lore raw

PATCH v5 1/2 hwmon: (ucd9000) Add gpio chip interface and clear logged faults

Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.

Present requirements only call for retrieving current state of pin values and their direction. No requirement at this time to switch modes between output and input within the device driver.

Add capability to clear logged faults via sysfs.

Signed-off-by Christopher Bostic <[email protected]>

---
v5 - Clean up remaining branch return statements to !! non
branching method.
- Clean up white space issues.
- Add return codes to error messages.
- Add comments describing assumptions of ucd90160 type.
- Define gpio direction set methods.
- Add unique id for each ucd9000 in system for gpio chip.
v4 - Change status check from branch to a !! non branching method
- Remove usage comments on libgpiod for the struct gpio_chip methods.
- Clean up some text formatting.
v3 - Correct bug in gpio_chip get method. Wasn't retrieving
gpio config information correctly.
- Remove old debugfs flag from previous pmbus core changes.
- Remove all sysfs files for mfr_status command.
- Add comments on direct i2c_smbus calls to clarify that no page set is required.
v2 - Remove clear_faults file - redundant since all other sysfs
core accesses result in an automatic clear fault.
- Removed local status_word and status_vout register dumps and use the new pmbus core status facilities instead.
- Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
- Add full register dump for mfr_status.
- Add gpio chip to device structure and use gpio interfaces.

---
 drivers/hwmon/pmbus/ucd9000.c | 250 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 248 insertions(+), 2 deletions(-)

drivers/hwmon/pmbus/ucd9000.c +248 -2

index 3e3aa95..cfd7703 100644
@@ -26,6 +26,9 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/gpio.h>
 #include "pmbus.h"
 
 enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };

@@ -34,8 +37,19 @@
 #define UCD9000_NUM_PAGES		0xd6
 #define UCD9000_FAN_CONFIG_INDEX	0xe7
 #define UCD9000_FAN_CONFIG		0xe8
+#define UCD9000_LOGGED_FAULTS		0xea
+#define UCD9000_GPIO_SELECT		0xfa
+#define UCD9000_GPIO_CONFIG		0xfb
 #define UCD9000_DEVICE_ID		0xfd
 
+/* GPIO CONFIG bits */
+#define UCD9000_GPIO_CONFIG_ENABLE	BIT(0)
+#define UCD9000_GPIO_CONFIG_OUT_ENABLE	BIT(1)
+#define UCD9000_GPIO_CONFIG_OUT_VALUE	BIT(2)
+#define UCD9000_GPIO_CONFIG_STATUS	BIT(3)
+#define UCD9000_GPIO_INPUT		0
+#define UCD9000_GPIO_OUTPUT		1
+
 #define UCD9000_MON_TYPE(x)	(((x) >> 5) & 0x07)
 #define UCD9000_MON_PAGE(x)	((x) & 0x0f)

@@ -46,9 +60,13 @@
 
 #define UCD9000_NUM_FAN		4
 
+#define UCD9000_GPIO_NAME_LEN	16
+#define UCD90160_NUM_GPIOS	26
+
 struct ucd9000_data {
 	u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
 	struct pmbus_driver_info info;
+	struct gpio_chip gpio;
 };
 #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)

@@ -119,6 +137,197 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
 };
 MODULE_DEVICE_TABLE(i2c, ucd9000_id);
 
+static int ucd9000_gpio_read_config(struct i2c_client *client,
+				unsigned int offset)
+{
+	int ret;
+
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
+			ret);
+
+		return ret;
+	}
+
+	return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
+}
+
+static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct i2c_client *client  = gpiochip_get_data(gc);
+	int ret;
+
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
+		return ret;
+	}
+
+	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
+}
+
+static void ucd9000_gpio_set(struct gpio_chip *gc, unsigned int offset,
+			int value)
+{
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret;
+
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
+		return;
+	}
+
+	if (value) {
+		if (ret & UCD9000_GPIO_CONFIG_STATUS)
+			return;
+
+		ret |= UCD9000_GPIO_CONFIG_STATUS;
+	} else {
+		if (!(ret & UCD9000_GPIO_CONFIG_STATUS))
+			return;
+
+		ret &= ~UCD9000_GPIO_CONFIG_STATUS;
+	}
+
+	ret |= UCD9000_GPIO_CONFIG_ENABLE;
+
+	/* Page set not required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
+
+		return;
+	}
+
+	ret &= ~UCD9000_GPIO_CONFIG_ENABLE;
+
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
+	if (ret < 0)
+		dev_err(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
+}
+
+static int ucd9000_gpio_get_direction(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret;
+
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
+		return ret;
+	}
+
+	return ~(!!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE));
+}
+
+static int ucd9000_gpio_set_direction(struct gpio_chip *gc, unsigned int offset,
+					bool direction_out, int requested_out)
+{
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret, config, out_val;
+
+
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
+		return ret;
+	}
+
+	if (direction_out) {
+		out_val = requested_out ? UCD9000_GPIO_CONFIG_OUT_VALUE : 0;
+
+		if (ret & UCD9000_GPIO_CONFIG_OUT_ENABLE) {
+			if ((ret & UCD9000_GPIO_CONFIG_OUT_VALUE) == out_val)
+				return 0;
+		} else
+			ret |= UCD9000_GPIO_CONFIG_OUT_ENABLE;
+
+		if (out_val)
+			ret |= UCD9000_GPIO_CONFIG_OUT_VALUE;
+		else
+			ret &= ~UCD9000_GPIO_CONFIG_OUT_VALUE;
+
+	} else {
+		if (!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE))
+			return 0;
+
+		ret &= ~UCD9000_GPIO_CONFIG_OUT_ENABLE;
+	}
+
+	ret |= UCD9000_GPIO_CONFIG_ENABLE;
+	config = ret;
+
+	/* Page set not required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
+
+		return ret;
+	}
+
+	config &= ~UCD9000_GPIO_CONFIG_ENABLE;
+
+	return i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);
+}
+
+static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	return ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_INPUT, 0);
+}
+
+static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
+					unsigned int offset, int val)
+{
+	return ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT, val);
+}
+
+static ssize_t ucd9000_clear_logged_faults(struct device *dev,
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_LOGGED_FAULTS, 0);
+	if (ret) {
+		dev_err(&client->dev, "Failed to clear logged faults: %d\n",
+			ret);
+
+		return ret;
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
+		ucd9000_clear_logged_faults);
+
+static struct attribute *ucd9000_attributes[] = {
+	&dev_attr_clear_logged_faults.attr,
+	NULL
+};
+
+static const struct attribute_group ucd9000_attr_group = {
+	.attrs = ucd9000_attributes,
+};
+
 static int ucd9000_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {

@@ -227,7 +436,44 @@ static int ucd9000_probe(struct i2c_client *client,
 		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
 	}
 
-	return pmbus_do_probe(client, mid, info);
+	data->gpio.label = (const char *)&client->name;
+	data->gpio.get_direction = ucd9000_gpio_get_direction;
+	data->gpio.direction_input = ucd9000_gpio_direction_input;
+	data->gpio.direction_output = ucd9000_gpio_direction_output;
+	data->gpio.get = ucd9000_gpio_get;
+	data->gpio.set = ucd9000_gpio_set;
+	data->gpio.can_sleep = 1;
+	data->gpio.base = -1;
+
+	/*
+	 * TODO: set ngpio for ucd9000 devs that aren't 90160 type
+	 */
+	if (mid->driver_data == ucd90160)
+		data->gpio.ngpio = UCD90160_NUM_GPIOS;
+	data->gpio.parent = &client->dev;
+	data->gpio.owner = THIS_MODULE;
+
+	ret = devm_gpiochip_add_data(&client->dev, &data->gpio, client);
+	if (ret) {
+		data->gpio.parent = NULL;
+		dev_warn(&client->dev, "Could not add gpiochip: %d\n", ret);
+		return ret;
+	}
+
+	ret = sysfs_create_group(&client->dev.kobj, &ucd9000_attr_group);
+	if (ret < 0) {
+		dev_warn(&client->dev, "Failed to add sysfs files: %d\n", ret);
+
+		return ret;
+	}
+
+	return  pmbus_do_probe(client, mid, info);
+}
+
+static int ucd9000_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
+	return pmbus_do_remove(client);
 }
 
 /* This is the driver that will be inserted */

@@ -236,7 +482,7 @@ static int ucd9000_probe(struct i2c_client *client,
 		.name = "ucd9000",
 	},
 	.probe = ucd9000_probe,
-	.remove = pmbus_do_remove,
+	.remove = ucd9000_remove,
 	.id_table = ucd9000_id,
 };

Signature · 1 line

1.8.2.2

Andrew Jeffery

2017-08-30 03:44 UTC #2 lore raw

Re: [PATCH linux dev-4.10 v5 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults

Hi, Chris,

Quoted 181 lines · Christopher Bostic

Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.

Present requirements only call for retrieving current state of pin values and their direction.  No requirement at this time to switch modes between output and input within the device driver.

Add capability to clear logged faults via sysfs.
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>
---
v5 - Clean up remaining branch return statements to !! non branching method.
   - Clean up white space issues.
   - Add return codes to error messages.
   - Add comments describing assumptions of ucd90160 type.
   - Define gpio direction set methods.
   - Add unique id for each ucd9000 in system for gpio chip.
v4 - Change status check from branch to a !! non branching method
   - Remove usage comments on libgpiod for the struct gpio_chip methods.
   - Clean up some text formatting.
v3 - Correct bug in gpio_chip get method.  Wasn't retrieving
     gpio config information correctly.
   - Remove old debugfs flag from previous pmbus core changes.
   - Remove all sysfs files for mfr_status command.
   - Add comments on direct i2c_smbus calls to clarify that no page set is required.
v2 - Remove clear_faults file - redundant since all other sysfs core accesses result in an automatic clear fault.
   - Removed local status_word and status_vout register dumps and use the new pmbus core status facilities instead.
   - Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
   - Add full register dump for mfr_status.
   - Add gpio chip to device structure and use gpio interfaces.
---
drivers/hwmon/pmbus/ucd9000.c | 250 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 248 insertions(+), 2 deletions(-)
drivers/hwmon/pmbus/ucd9000.c +3 -0
index 3e3aa95..cfd7703 100644
@@ -26,6 +26,9 @@
  #include <linux/slab.h>
  #include <linux/i2c.h>
  #include <linux/i2c/pmbus.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/gpio.h>
  #include "pmbus.h"
enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -34,8 +37,19 @@
Quoted 7 lines
#define UCD9000_NUM_PAGES 0xd6
#define UCD9000_FAN_CONFIG_INDEX 0xe7
#define UCD9000_FAN_CONFIG 0xe8
drivers/hwmon/pmbus/ucd9000.c +3 -0
+#define UCD9000_LOGGED_FAULTS		0xea
+#define UCD9000_GPIO_SELECT		0xfa
+#define UCD9000_GPIO_CONFIG		0xfb
#define UCD9000_DEVICE_ID 0xfd
+/* GPIO CONFIG bits */
drivers/hwmon/pmbus/ucd9000.c line 51
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+#define UCD9000_GPIO_CONFIG_ENABLE	BIT(0)
+#define UCD9000_GPIO_CONFIG_OUT_ENABLE	BIT(1)
+#define UCD9000_GPIO_CONFIG_OUT_VALUE	BIT(2)
+#define UCD9000_GPIO_CONFIG_STATUS	BIT(3)
+#define UCD9000_GPIO_INPUT		0
+#define UCD9000_GPIO_OUTPUT		1
+
Quoted 2 lines
#define UCD9000_MON_TYPE(x) (((x) >> 5) & 0x07)
#define UCD9000_MON_PAGE(x) ((x) & 0x0f)
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -46,9 +60,13 @@
Quoted 1 line
#define UCD9000_NUM_FAN 4
drivers/hwmon/pmbus/ucd9000.c line 64
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+#define UCD9000_GPIO_NAME_LEN	16
+#define UCD90160_NUM_GPIOS	26
+
struct ucd9000_data {
drivers/hwmon/pmbus/ucd9000.c line 69
Quoted 3 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct pmbus_driver_info info;
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	struct gpio_chip gpio;
};
#define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -119,6 +137,197 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
  };
  MODULE_DEVICE_TABLE(i2c, ucd9000_id);
+static int ucd9000_gpio_read_config(struct i2c_client *client,
Review
Quoted 1 line
(quoted diff) +1 -0
+				unsigned int offset)
+{
Review
Quoted 1 line
(quoted diff) +1 -0
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 149
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
+			ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
Review
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client  = gpiochip_get_data(gc);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 165
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static void ucd9000_gpio_set(struct gpio_chip *gc, unsigned int offset,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 174
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+			int value)
+{
Review
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 165
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 189
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+	if (value) {
+		if (ret & UCD9000_GPIO_CONFIG_STATUS)
+			return;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 194
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+		ret |= UCD9000_GPIO_CONFIG_STATUS;
+	} else {
+		if (!(ret & UCD9000_GPIO_CONFIG_STATUS))
+			return;
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		ret &= ~UCD9000_GPIO_CONFIG_STATUS;
+	}
+
Review
Quoted 1 line
(quoted diff) +1 -0
+	ret |= UCD9000_GPIO_CONFIG_ENABLE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 205
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	/* Page set not required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 210
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	ret &= ~UCD9000_GPIO_CONFIG_ENABLE;
+
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
+	if (ret < 0)
+		dev_err(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_get_direction(struct gpio_chip *gc,
Review
Quoted 1 line
(quoted diff) +1 -0
+					unsigned int offset)
+{
Review
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 165
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+

Quoted 4 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 232
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return ~(!!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE));

Why are we doing bit-wise negation after boolean double negation? That looks wrong. I would expect to only do the boolean double negation.

The documentation says a return value of 0 is out, 1 is in, and negative is an error. With the bit-wise negation this would give values of either -1 or -2, both of which are errors.

	2 13:07:24 andrew@keelia:~$ cat /tmp/bits.c
	#include <stdio.h>

	int main(void) {
		printf("%d, %d\n", ~((int) 0), ~((int) 1));

		return 0;
	}
	2 13:07:29 andrew@keelia:~$ make /tmp/bits
	cc     /tmp/bits.c   -o /tmp/bits
	2 13:07:31 andrew@keelia:~$ /tmp/bits
	-1, -2
	2 13:07:36 andrew@keelia:~$

Quoted 47 lines

drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_set_direction(struct gpio_chip *gc, unsigned int offset,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 236
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+					bool direction_out, int requested_out)
+{
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret, config, out_val;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 165
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 251
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	if (direction_out) {
+		out_val = requested_out ? UCD9000_GPIO_CONFIG_OUT_VALUE : 0;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 257
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+		if (ret & UCD9000_GPIO_CONFIG_OUT_ENABLE) {
+			if ((ret & UCD9000_GPIO_CONFIG_OUT_VALUE) == out_val)
+				return 0;
+		} else
+			ret |= UCD9000_GPIO_CONFIG_OUT_ENABLE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 262
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+		if (out_val)
+			ret |= UCD9000_GPIO_CONFIG_OUT_VALUE;
+		else
+			ret &= ~UCD9000_GPIO_CONFIG_OUT_VALUE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 266
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+	} else {
+		if (!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE))
+			return 0;
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		ret &= ~UCD9000_GPIO_CONFIG_OUT_ENABLE;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 272
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	ret |= UCD9000_GPIO_CONFIG_ENABLE;
+	config = ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 278
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	/* Page set not required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
+

Quoted 7 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 283
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	config &= ~UCD9000_GPIO_CONFIG_ENABLE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 285
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);
+}

Thanks for implementing the direction switch. With the early exits it is a bit more involved than I was anticipating, but certainly we want to avoid the double-write if we can.

Quoted 41 lines

drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
Review
Quoted 1 line
(quoted diff) +1 -0
+					unsigned int offset)
+{
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_INPUT, 0);
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 295
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+					unsigned int offset, int val)
+{
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT, val);
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static ssize_t ucd9000_clear_logged_faults(struct device *dev,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 302
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 305
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 311
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_LOGGED_FAULTS, 0);
+	if (ret) {
+		dev_err(&client->dev, "Failed to clear logged faults: %d\n",
+			ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return count;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+		ucd9000_clear_logged_faults);
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static struct attribute *ucd9000_attributes[] = {
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	&dev_attr_clear_logged_faults.attr,
+	NULL
drivers/hwmon/pmbus/ucd9000.c +3 -0
+};
+
+static const struct attribute_group ucd9000_attr_group = {

Quoted 2 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 328
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	.attrs = ucd9000_attributes,
+};

We discussed splitting ucd9000_clear_logged_faults() and associated bits out into a separate patch so it could be sent upstream for review. Can you talk about why it's still included?

Quoted 58 lines

+
static int ucd9000_probe(struct i2c_client *client,
Quoted 1 line
const struct i2c_device_id *id)
{
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -227,7 +436,44 @@ static int ucd9000_probe(struct i2c_client *client,
 >  		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
 >  	}
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 446
Quoted 9 lines
drivers/hwmon/pmbus/ucd9000.c +8 -1
-	return pmbus_do_probe(client, mid, info);
+	data->gpio.label = (const char *)&client->name;
+	data->gpio.get_direction = ucd9000_gpio_get_direction;
+	data->gpio.direction_input = ucd9000_gpio_direction_input;
+	data->gpio.direction_output = ucd9000_gpio_direction_output;
+	data->gpio.get = ucd9000_gpio_get;
+	data->gpio.set = ucd9000_gpio_set;
+	data->gpio.can_sleep = 1;
+	data->gpio.base = -1;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 454
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c +7 -0
+	/*
+	 * TODO: set ngpio for ucd9000 devs that aren't 90160 type
+	 */
+	if (mid->driver_data == ucd90160)
+		data->gpio.ngpio = UCD90160_NUM_GPIOS;
+	data->gpio.parent = &client->dev;
+	data->gpio.owner = THIS_MODULE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 461
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ret = devm_gpiochip_add_data(&client->dev, &data->gpio, client);
+	if (ret) {
+		data->gpio.parent = NULL;
+		dev_warn(&client->dev, "Could not add gpiochip: %d\n", ret);
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 465
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+	ret = sysfs_create_group(&client->dev.kobj, &ucd9000_attr_group);
+	if (ret < 0) {
+		dev_warn(&client->dev, "Failed to add sysfs files: %d\n", ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return  pmbus_do_probe(client, mid, info);
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_remove(struct i2c_client *client)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 476
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
+	return pmbus_do_remove(client);
}

/* This is the driver that will be inserted */
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -236,7 +482,7 @@ static int ucd9000_probe(struct i2c_client *client,
 >  		.name = "ucd9000",
 >  	},
 >  	.probe = ucd9000_probe,
 > -	.remove = pmbus_do_remove,
 > +	.remove = ucd9000_remove,
 >  	.id_table = ucd9000_id,
  };

Quoted 1 line

There's no comment about pinmux, which is another one of the assumptions we're making with this patch. In general I was trying to make the point that we should document anything we're assuming. The mux state is another such thing.

Cheers,

Andrew

Christopher Bostic

author

2017-08-30 19:29 UTC #3 lore raw

Re: [PATCH linux dev-4.10 v5 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults

Quoted 195 lines · Andrew Jeffery

Hi, Chris,
Quoted 181 lines · Christopher Bostic
Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.
Present requirements only call for retrieving current state of pin values and their direction. No requirement at this time to switch modes between output and input within the device driver.
Add capability to clear logged faults via sysfs.
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>
---
v5 - Clean up remaining branch return statements to !! non
branching method.
- Clean up white space issues.
- Add return codes to error messages.
- Add comments describing assumptions of ucd90160 type.
- Define gpio direction set methods.
- Add unique id for each ucd9000 in system for gpio chip.
v4 - Change status check from branch to a !! non branching method
- Remove usage comments on libgpiod for the struct gpio_chip methods.
- Clean up some text formatting.
v3 - Correct bug in gpio_chip get method. Wasn't retrieving
gpio config information correctly.
- Remove old debugfs flag from previous pmbus core changes.
- Remove all sysfs files for mfr_status command.
- Add comments on direct i2c_smbus calls to clarify that no page set is required.
v2 - Remove clear_faults file - redundant since all other sysfs
core accesses result in an automatic clear fault.
- Removed local status_word and status_vout register dumps and use the new pmbus core status facilities instead.
- Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
- Add full register dump for mfr_status.
- Add gpio chip to device structure and use gpio interfaces.
---
  drivers/hwmon/pmbus/ucd9000.c | 250 +++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 248 insertions(+), 2 deletions(-)
drivers/hwmon/pmbus/ucd9000.c +3 -0
index 3e3aa95..cfd7703 100644
@@ -26,6 +26,9 @@
  #include <linux/slab.h>
  #include <linux/i2c.h>
  #include <linux/i2c/pmbus.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/gpio.h>
  #include "pmbus.h"
 
  enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
@@ -34,8 +37,19 @@
drivers/hwmon/pmbus/ucd9000.c line 43
Quoted 7 lines
  #define UCD9000_NUM_PAGES		0xd6
  #define UCD9000_FAN_CONFIG_INDEX	0xe7
  #define UCD9000_FAN_CONFIG		0xe8
drivers/hwmon/pmbus/ucd9000.c +3 -0
+#define UCD9000_LOGGED_FAULTS		0xea
+#define UCD9000_GPIO_SELECT		0xfa
+#define UCD9000_GPIO_CONFIG		0xfb
  #define UCD9000_DEVICE_ID		0xfd
+/* GPIO CONFIG bits */
drivers/hwmon/pmbus/ucd9000.c line 51
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+#define UCD9000_GPIO_CONFIG_ENABLE	BIT(0)
+#define UCD9000_GPIO_CONFIG_OUT_ENABLE	BIT(1)
+#define UCD9000_GPIO_CONFIG_OUT_VALUE	BIT(2)
+#define UCD9000_GPIO_CONFIG_STATUS	BIT(3)
+#define UCD9000_GPIO_INPUT		0
+#define UCD9000_GPIO_OUTPUT		1
+
Quoted 2 lines
  #define UCD9000_MON_TYPE(x)	(((x) >> 5) & 0x07)
  #define UCD9000_MON_PAGE(x)	((x) & 0x0f)
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -46,9 +60,13 @@
 
drivers/hwmon/pmbus/ucd9000.c line 64
Quoted 4 lines
  #define UCD9000_NUM_FAN		4
drivers/hwmon/pmbus/ucd9000.c +2 -0
+#define UCD9000_GPIO_NAME_LEN	16
+#define UCD90160_NUM_GPIOS	26
+
struct ucd9000_data {
drivers/hwmon/pmbus/ucd9000.c line 69
Quoted 3 lines
  	u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
  	struct pmbus_driver_info info;
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	struct gpio_chip gpio;
};
#define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
drivers/hwmon/pmbus/ucd9000.c +3 -0
@@ -119,6 +137,197 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
  };
  MODULE_DEVICE_TABLE(i2c, ucd9000_id);
 
+static int ucd9000_gpio_read_config(struct i2c_client *client,
 > +				unsigned int offset)
+{
 > +	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 149
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
+			ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
Review
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client  = gpiochip_get_data(gc);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 165
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static void ucd9000_gpio_set(struct gpio_chip *gc, unsigned int offset,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 174
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+			int value)
+{
Review
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 165
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 189
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+	if (value) {
+		if (ret & UCD9000_GPIO_CONFIG_STATUS)
+			return;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 194
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+		ret |= UCD9000_GPIO_CONFIG_STATUS;
+	} else {
+		if (!(ret & UCD9000_GPIO_CONFIG_STATUS))
+			return;
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		ret &= ~UCD9000_GPIO_CONFIG_STATUS;
+	}
+
Review
Quoted 1 line
(quoted diff) +1 -0
+	ret |= UCD9000_GPIO_CONFIG_ENABLE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 205
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	/* Page set not required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 210
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	ret &= ~UCD9000_GPIO_CONFIG_ENABLE;
+
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
+	if (ret < 0)
+		dev_err(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_get_direction(struct gpio_chip *gc,
Review
Quoted 1 line
(quoted diff) +1 -0
+					unsigned int offset)
+{
Review
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 165
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 232
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return ~(!!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE));
Why are we doing bit-wise negation after boolean double negation? That looks wrong. I would expect to only do the boolean double negation.
The documentation says a return value of 0 is out, 1 is in, and negative is an error. With the bit-wise negation this would give values of either -1 or -2, both of which are errors.

Quoted 14 lines

	2 13:07:24 andrew@keelia:~$ cat /tmp/bits.c
	#include <stdio.h>

	int main(void) {
		printf("%d, %d\n", ~((int) 0), ~((int) 1));

		return 0;
	}
	2 13:07:29 andrew@keelia:~$ make /tmp/bits
	cc     /tmp/bits.c   -o /tmp/bits
	2 13:07:31 andrew@keelia:~$ /tmp/bits
	-1, -2
	2 13:07:36 andrew@keelia:~$

Hi Andrew,

Yeah this is a problem. My tests passed but clearly this is wrong. Will fix.

Quoted 99 lines

Quoted 47 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_set_direction(struct gpio_chip *gc, unsigned int offset,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 236
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+					bool direction_out, int requested_out)
+{
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret, config, out_val;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 165
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 251
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	if (direction_out) {
+		out_val = requested_out ? UCD9000_GPIO_CONFIG_OUT_VALUE : 0;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 257
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+		if (ret & UCD9000_GPIO_CONFIG_OUT_ENABLE) {
+			if ((ret & UCD9000_GPIO_CONFIG_OUT_VALUE) == out_val)
+				return 0;
+		} else
+			ret |= UCD9000_GPIO_CONFIG_OUT_ENABLE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 262
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+		if (out_val)
+			ret |= UCD9000_GPIO_CONFIG_OUT_VALUE;
+		else
+			ret &= ~UCD9000_GPIO_CONFIG_OUT_VALUE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 266
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+	} else {
+		if (!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE))
+			return 0;
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		ret &= ~UCD9000_GPIO_CONFIG_OUT_ENABLE;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 272
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	ret |= UCD9000_GPIO_CONFIG_ENABLE;
+	config = ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 278
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	/* Page set not required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
+
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 283
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	config &= ~UCD9000_GPIO_CONFIG_ENABLE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 285
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);
+}
Thanks for implementing the direction switch. With the early exits it is a bit more involved than I was anticipating, but certainly we want to avoid the double-write if we can.
Quoted 41 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
Review
Quoted 1 line
(quoted diff) +1 -0
+					unsigned int offset)
+{
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_INPUT, 0);
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 295
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+					unsigned int offset, int val)
+{
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT, val);
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static ssize_t ucd9000_clear_logged_faults(struct device *dev,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 302
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 305
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 311
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_LOGGED_FAULTS, 0);
+	if (ret) {
+		dev_err(&client->dev, "Failed to clear logged faults: %d\n",
+			ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return count;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+		ucd9000_clear_logged_faults);
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static struct attribute *ucd9000_attributes[] = {
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	&dev_attr_clear_logged_faults.attr,
+	NULL
drivers/hwmon/pmbus/ucd9000.c +3 -0
+};
+
+static const struct attribute_group ucd9000_attr_group = {

Quoted 5 lines

Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 328
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	.attrs = ucd9000_attributes,
+};
We discussed splitting ucd9000_clear_logged_faults() and associated bits out into a separate patch so it could be sent upstream for review. Can you talk about why it's still included?

I broke out the ucd9000_clear_logged_faults( ) in the community patch but not for this set. I will do so for next version.

Quoted 59 lines

Quoted 52 lines
+
static int ucd9000_probe(struct i2c_client *client,
Quoted 1 line
const struct i2c_device_id *id)
{
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -227,7 +436,44 @@ static int ucd9000_probe(struct i2c_client *client,
 >   		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
 >   	}
 
 > -	return pmbus_do_probe(client, mid, info);
 > +	data->gpio.label = (const char *)&client->name;
 > +	data->gpio.get_direction = ucd9000_gpio_get_direction;
 > +	data->gpio.direction_input = ucd9000_gpio_direction_input;
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 446
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	data->gpio.direction_output = ucd9000_gpio_direction_output;
+	data->gpio.get = ucd9000_gpio_get;
+	data->gpio.set = ucd9000_gpio_set;
+	data->gpio.can_sleep = 1;
+	data->gpio.base = -1;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 454
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c +7 -0
+	/*
+	 * TODO: set ngpio for ucd9000 devs that aren't 90160 type
+	 */
+	if (mid->driver_data == ucd90160)
+		data->gpio.ngpio = UCD90160_NUM_GPIOS;
+	data->gpio.parent = &client->dev;
+	data->gpio.owner = THIS_MODULE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 461
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ret = devm_gpiochip_add_data(&client->dev, &data->gpio, client);
+	if (ret) {
+		data->gpio.parent = NULL;
+		dev_warn(&client->dev, "Could not add gpiochip: %d\n", ret);
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 465
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+	ret = sysfs_create_group(&client->dev.kobj, &ucd9000_attr_group);
+	if (ret < 0) {
+		dev_warn(&client->dev, "Failed to add sysfs files: %d\n", ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return  pmbus_do_probe(client, mid, info);
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_remove(struct i2c_client *client)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 476
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
+	return pmbus_do_remove(client);
}
/* This is the driver that will be inserted */
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 488
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -236,7 +482,7 @@ static int ucd9000_probe(struct i2c_client *client,
 >   		.name = "ucd9000",
 >   	},
 >   	.probe = ucd9000_probe,
 > -	.remove = pmbus_do_remove,
 > +	.remove = ucd9000_remove,
 >   	.id_table = ucd9000_id,
  };

Quoted 3 lines

There's no comment about pinmux, which is another one of the assumptions we're making with this patch. In general I was trying to make the point that we should document anything we're assuming. The mux state is another such thing.

Will add that to the documented assumptions.

Thanks,
Chris

Quoted 4 lines

Cheers,
Andrew

Christopher Bostic

author

2017-08-29 20:08 UTC #4 lore raw

PATCH v5 2/2 hwmon: (ucd9000) Add debugfs for mfr_status info

In reply to Christopher Bostic

Add debugfs to list mfr_status register and its gpiN_fault fields.

Signed-off-by Christopher Bostic <[email protected]>

---
v5 - Add unique debugfs dir ID for each ucd9000 sysfs device.
- Change branching return to a !! method.
- Clean up line breaks, other white space.
- Add comments on assumptions made regarding ucd90160 versus other types.
v2 - Remove mfr_status directory in debugfs and place the files
up in the parent ucd9000 directory.

---
 drivers/hwmon/pmbus/ucd9000.c | 154 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 154 insertions(+)

drivers/hwmon/pmbus/ucd9000.c +154 -0

index cfd7703..edb026f 100644
@@ -19,6 +19,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>

@@ -29,6 +30,7 @@
 #include <linux/sysfs.h>
 #include <linux/hwmon-sysfs.h>
 #include <linux/gpio.h>
+#include <linux/idr.h>
 #include "pmbus.h"
 
 enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };

@@ -38,6 +40,7 @@
 #define UCD9000_FAN_CONFIG_INDEX	0xe7
 #define UCD9000_FAN_CONFIG		0xe8
 #define UCD9000_LOGGED_FAULTS		0xea
+#define UCD9000_MFR_STATUS		0xf3
 #define UCD9000_GPIO_SELECT		0xfa
 #define UCD9000_GPIO_CONFIG		0xfb
 #define UCD9000_DEVICE_ID		0xfd

@@ -59,17 +62,28 @@
 #define UCD9000_MON_VOLTAGE_HW	4
 
 #define UCD9000_NUM_FAN		4
+#define UCD9000_NAME_SIZE	24
 
 #define UCD9000_GPIO_NAME_LEN	16
 #define UCD90160_NUM_GPIOS	26
+#define UCD90160_GPI_COUNT	8
+#define UCD90160_GPI_FAULT_BASE	16
+
+static DEFINE_IDA(ucd9000_ida);
 
 struct ucd9000_data {
 	u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
 	struct pmbus_driver_info info;
 	struct gpio_chip gpio;
+	struct dentry *debugfs;
 };
 #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
 
+struct ucd9000_debugfs_entry {
+	struct i2c_client *client;
+	u8 index;
+};
+
 static int ucd9000_get_fan_config(struct i2c_client *client, int fan)
 {
 	int fan_config = 0;

@@ -297,6 +311,138 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
 	return ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT, val);
 }
 
+static struct dentry *ucd9000_debugfs_dir;
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static int ucd9000_get_mfr_status(struct i2c_client *client, u32 *buffer)
+{
+	int ret;
+
+	ret = pmbus_set_page(client, 0);
+	if (ret < 0) {
+		dev_err(&client->dev, "pmbus_set_page failed. rc:%d\n", ret);
+
+		return ret;
+	}
+
+	/*
+	 * Warning:
+	 *
+	 * Though not currently supported this will cause stack corruption for
+	 * ucd90240!  Command reference, page 81:
+	 *
+	 *    With the ucd90120 and ucd90124 devices, this command [MFR_STATUS]
+	 *    is 2 bytes long (bits 0-15).  With the ucd90240 this command is 5
+	 *    bytes long.  With all other devices, it is 4 bytes long.
+	 */
+	return i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS,
+					(u8 *)buffer);
+}
+
+static int ucd9000_debugfs_get_mfr_status_bit(void *data, u64 *val)
+{
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	int nr = entry->index;
+	u32 buffer;
+	int ret;
+
+	ret = ucd9000_get_mfr_status(client, &buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+			ret);
+
+		return ret;
+	}
+
+	*val = !!(ret & BIT(nr));
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_bit,
+		ucd9000_debugfs_get_mfr_status_bit, NULL, "%1lld\n");
+
+static int ucd9000_debugfs_get_mfr_status_word(void *data, u64 *val)
+{
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	u32 buffer;
+	int ret;
+
+	ret = ucd9000_get_mfr_status(client, &buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+			ret);
+
+		return ret;
+	}
+
+	*val = ret;
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_word,
+		ucd9000_debugfs_get_mfr_status_word, NULL, "%08llx\n");
+
+static int ucd9000_init_debugfs(struct i2c_client *client,
+				struct ucd9000_data *data)
+{
+	struct ucd9000_debugfs_entry *entries;
+	char name[UCD9000_NAME_SIZE];
+	int i;
+
+	i = ida_simple_get(&ucd9000_ida, 0, INT_MAX, GFP_KERNEL);
+	scnprintf(name, UCD9000_NAME_SIZE, "ucd9000.%d", i);
+	ucd9000_debugfs_dir = debugfs_create_dir(name, NULL);
+	if (IS_ERR(ucd9000_debugfs_dir)) {
+		dev_warn(&client->dev, "Failed to create debugfs dir: %p\n",
+			ucd9000_debugfs_dir);
+		ucd9000_debugfs_dir = NULL;
+
+		return 0;
+	}
+
+	/*
+	 * Warning:
+	 *
+	 * Makes assumption we're on a ucd90160 type! entries will be different
+	 * sizes for other types.
+	 */
+	entries = devm_kzalloc(&client->dev, sizeof(*entries) *
+				(UCD90160_GPI_COUNT + 1) * 10, GFP_KERNEL);
+	if (!entries)
+		return -ENOMEM;
+
+	/*
+	 * Warning:
+	 *
+	 * This makes the assumption we're probing a ucd90160 type and how the
+	 * GPI information is organized.  Needs to account for all other
+	 * ucd9000 varieties.
+	 */
+	for (i = 0; i < UCD90160_GPI_COUNT; i++) {
+		entries[i].client = client;
+		entries[i].index = UCD90160_GPI_FAULT_BASE + i;
+		scnprintf(name, UCD9000_NAME_SIZE, "gpi%d_alarm", i+1);
+		debugfs_create_file(name, 0444, ucd9000_debugfs_dir,
+				&entries[i],
+				&ucd9000_debugfs_ops_mfr_status_bit);
+	}
+	entries[i].client = client;
+	scnprintf(name, UCD9000_NAME_SIZE, "mfr_status");
+	debugfs_create_file(name, 0444, ucd9000_debugfs_dir, &entries[i],
+			&ucd9000_debugfs_ops_mfr_status_word);
+
+	return 0;
+}
+#else
+static int ucd9000_init_debugfs(struct i2c_client *client,
+				struct ucd9000_data *data)
+{
+	return 0;
+}
+#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
+
 static ssize_t ucd9000_clear_logged_faults(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)

@@ -467,12 +613,20 @@ static int ucd9000_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	if (mid->driver_data == ucd90160) {
+		ret = ucd9000_init_debugfs(client, data);
+		if (ret < 0)
+			dev_warn(&client->dev,
+				"Failed to register debugfs: %d\n", ret);
+	}
+
 	return  pmbus_do_probe(client, mid, info);
 }
 
 static int ucd9000_remove(struct i2c_client *client)
 {
 	sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
+	debugfs_remove_recursive(ucd9000_debugfs_dir);
 	return pmbus_do_remove(client);
 }

Signature · 1 line

1.8.2.2

Andrew Jeffery

2017-08-30 03:59 UTC #5 lore raw

Re: [PATCH linux dev-4.10 v5 2/2] hwmon: (ucd9000) Add debugfs for mfr_status info

Quoted 99 lines · Christopher Bostic

Add debugfs to list mfr_status register and its gpiN_fault fields.
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>
---
v5 - Add unique debugfs dir ID for each ucd9000 sysfs device.
   - Change branching return to a !! method.
   - Clean up line breaks, other white space.
   - Add comments on assumptions made regarding ucd90160 versus other types.
v2 - Remove mfr_status directory in debugfs and place the files
     up in the parent ucd9000 directory.
---
drivers/hwmon/pmbus/ucd9000.c | 154 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 154 insertions(+)
drivers/hwmon/pmbus/ucd9000.c +0 -0
index cfd7703..edb026f 100644
@@ -19,6 +19,7 @@
   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
   */
+#include <linux/debugfs.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
drivers/hwmon/pmbus/ucd9000.c +1 -0
@@ -29,6 +30,7 @@
  #include <linux/sysfs.h>
  #include <linux/hwmon-sysfs.h>
  #include <linux/gpio.h>
+#include <linux/idr.h>
  #include "pmbus.h"
enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -38,6 +40,7 @@
Quoted 7 lines
#define UCD9000_FAN_CONFIG_INDEX 0xe7
#define UCD9000_FAN_CONFIG 0xe8
#define UCD9000_LOGGED_FAULTS 0xea
+#define UCD9000_MFR_STATUS 0xf3
#define UCD9000_GPIO_SELECT 0xfa
#define UCD9000_GPIO_CONFIG 0xfb
#define UCD9000_DEVICE_ID 0xfd
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -59,17 +62,28 @@
Quoted 1 line
#define UCD9000_MON_VOLTAGE_HW 4
Quoted 2 lines
#define UCD9000_NUM_FAN 4
+#define UCD9000_NAME_SIZE 24
Quoted 4 lines
#define UCD9000_GPIO_NAME_LEN 16
#define UCD90160_NUM_GPIOS 26
drivers/hwmon/pmbus/ucd9000.c +2 -0
+#define UCD90160_GPI_COUNT	8
+#define UCD90160_GPI_FAULT_BASE	16
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static DEFINE_IDA(ucd9000_ida);
struct ucd9000_data {
drivers/hwmon/pmbus/ucd9000.c line 78
Quoted 4 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct pmbus_driver_info info;
struct gpio_chip gpio;
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	struct dentry *debugfs;
};
#define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
+struct ucd9000_debugfs_entry {
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	struct i2c_client *client;
+	u8 index;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+};
+
static int ucd9000_get_fan_config(struct i2c_client *client, int fan) {
Quoted 1 line
int fan_config = 0;
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -297,6 +311,138 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
 >  	return ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT, val);
  }
drivers/hwmon/pmbus/ucd9000.c +5 -0
+static struct dentry *ucd9000_debugfs_dir;
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static int ucd9000_get_mfr_status(struct i2c_client *client, u32 *buffer)
+{
Review
Quoted 1 line
(quoted diff) +1 -0
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 323
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+	ret = pmbus_set_page(client, 0);
+	if (ret < 0) {
+		dev_err(&client->dev, "pmbus_set_page failed. rc:%d\n", ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
Quoted 9 lines
drivers/hwmon/pmbus/ucd9000.c +9 -0
+	/*
+	 * Warning:
+	 *
+	 * Though not currently supported this will cause stack corruption for
+	 * ucd90240!  Command reference, page 81:
+	 *
+	 *    With the ucd90120 and ucd90124 devices, this command [MFR_STATUS]
+	 *    is 2 bytes long (bits 0-15).  With the ucd90240 this command is 5
+	 *    bytes long.  With all other devices, it is 4 bytes long.

drivers/hwmon/pmbus/ucd9000.c

Quoted 1 line

drivers/hwmon/pmbus/ucd9000.c +1 -0
+	 */

Hopefully it won't be missed :)

Quoted 52 lines

drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+					(u8 *)buffer);
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_debugfs_get_mfr_status_bit(void *data, u64 *val)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 348
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	int nr = entry->index;
+	u32 buffer;
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 353
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_get_mfr_status(client, &buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+			ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 358
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	*val = !!(ret & BIT(nr));
+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_bit,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+		ucd9000_debugfs_get_mfr_status_bit, NULL, "%1lld\n");
drivers/hwmon/pmbus/ucd9000.c +3 -0
+
+static int ucd9000_debugfs_get_mfr_status_word(void *data, u64 *val)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 348
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	u32 buffer;
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 353
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_get_mfr_status(client, &buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+			ret);
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 380
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	*val = ret;
+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_word,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+		ucd9000_debugfs_get_mfr_status_word, NULL, "%08llx\n");
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static int ucd9000_init_debugfs(struct i2c_client *client,
Review
Quoted 1 line
(quoted diff) +1 -0
+				struct ucd9000_data *data)
+{

Quoted 6 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 392
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+	struct ucd9000_debugfs_entry *entries;
+	char name[UCD9000_NAME_SIZE];
+	int i;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 395
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	i = ida_simple_get(&ucd9000_ida, 0, INT_MAX, GFP_KERNEL);
+	scnprintf(name, UCD9000_NAME_SIZE, "ucd9000.%d", i);

Given it's debugfs it doesn't matter for the moment, but I do wonder whether we should use the actual device name rather than the driver name here. Some thing to fix later.

Quoted 69 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 400
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	ucd9000_debugfs_dir = debugfs_create_dir(name, NULL);
+	if (IS_ERR(ucd9000_debugfs_dir)) {
+		dev_warn(&client->dev, "Failed to create debugfs dir: %p\n",
+			ucd9000_debugfs_dir);
+		ucd9000_debugfs_dir = NULL;
+
Review
Quoted 2 lines
(quoted diff) +2 -0
+		return 0;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 414
Quoted 10 lines
drivers/hwmon/pmbus/ucd9000.c +10 -0
+	/*
+	 * Warning:
+	 *
+	 * Makes assumption we're on a ucd90160 type! entries will be different
+	 * sizes for other types.
+	 */
+	entries = devm_kzalloc(&client->dev, sizeof(*entries) *
+				(UCD90160_GPI_COUNT + 1) * 10, GFP_KERNEL);
+	if (!entries)
+		return -ENOMEM;
+
Quoted 9 lines
drivers/hwmon/pmbus/ucd9000.c +9 -0
+	/*
+	 * Warning:
+	 *
+	 * This makes the assumption we're probing a ucd90160 type and how the
+	 * GPI information is organized.  Needs to account for all other
+	 * ucd9000 varieties.
+	 */
+	for (i = 0; i < UCD90160_GPI_COUNT; i++) {
+		entries[i].client = client;
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 434
Quoted 10 lines
drivers/hwmon/pmbus/ucd9000.c +10 -0
+		entries[i].index = UCD90160_GPI_FAULT_BASE + i;
+		scnprintf(name, UCD9000_NAME_SIZE, "gpi%d_alarm", i+1);
+		debugfs_create_file(name, 0444, ucd9000_debugfs_dir,
+				&entries[i],
+				&ucd9000_debugfs_ops_mfr_status_bit);
+	}
+	entries[i].client = client;
+	scnprintf(name, UCD9000_NAME_SIZE, "mfr_status");
+	debugfs_create_file(name, 0444, ucd9000_debugfs_dir, &entries[i],
+			&ucd9000_debugfs_ops_mfr_status_word);
+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+#else
+static int ucd9000_init_debugfs(struct i2c_client *client,
Review
Quoted 1 line
(quoted diff) +1 -0
+				struct ucd9000_data *data)
+{
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
+
static ssize_t ucd9000_clear_logged_faults(struct device *dev,
Quoted 2 lines
struct device_attribute *attr, const char *buf, size_t count)
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -467,12 +613,20 @@ static int ucd9000_probe(struct i2c_client *client,
 >  		return ret;
 >  	}
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 621
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	if (mid->driver_data == ucd90160) {
+		ret = ucd9000_init_debugfs(client, data);
+		if (ret < 0)
+			dev_warn(&client->dev,
+				"Failed to register debugfs: %d\n", ret);
+	}
+
Quoted 1 line
return pmbus_do_probe(client, mid, info);
}

static int ucd9000_remove(struct i2c_client *client) {

Quoted 5 lines

Quoted 3 lines
sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	debugfs_remove_recursive(ucd9000_debugfs_dir);
return pmbus_do_remove(client);
}

Looks good.

Thanks,

Andrew

Andrew Jeffery

2017-08-30 03:00 UTC #6 lore raw

In reply to Christopher Bostic

Quoted 11 lines · Christopher Bostic

This set provides means to allow user space to access various properties of the ucd9000 type chips.

patch 0001 Adds gpio_chip interface for user to access various GPIO pin states via the /sys/class/gpio interface.

Patch 0002 adds a debugfs for mfr_status information so user space can gain visibility of gpi type faults. Note that there are limitations on type of devices that debugfs supports. Presently only ucd90160 is supported. Any assumptions made as to formatting of data is commented inline in the code. Any expansion to other ucd9000 types will need to take this into consideration.

Thanks for improving the cover letter!

Quoted 8 lines

Christopher Bostic (2):
hwmon: (ucd9000) Add gpio chip interface and clear logged faults
hwmon: (ucd9000) Add debugfs for mfr_status info

drivers/hwmon/pmbus/ucd9000.c | 404 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 402 insertions(+), 2 deletions(-)