[PATCH linux dev-4.10 v7 0/2] Add user space accessibility for ucd9000 stats

[PATCH linux dev-4.10 v7 0/2] Add user space accessibility for ucd9000 stats

[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.

Note also that no pinmux support is provided at this point.  The ucd9000 devs do
have mux capability so this should be added.

v6 patch 0001 which covers the addition of a 'clear logged faults' sysfs file is
now gone.   There are a few open questions in terms of requirements.
Specifically, why clear when no one reads the faults out?  This was pointed out
by Guenter Roeck.  Consequently v6 patch 0002 is now v7 patch 0001,  v6 patch
0003 is v7 patch 0002.

Christopher Bostic (2):
  hwmon: (ucd9000) Add gpio chip interface
  hwmon: (ucd9000) Add debugfs to list mfr_status info

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

-- 
1.8.2.2

[PATCH linux dev-4.10 v7 1/2] hwmon: (ucd9000) Add gpio chip interface

[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.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
v7 - Reorder this patch from #2 in series to #1
v6 - Break out sysfs clear logged faults into separate patch
   - Fix logic bug that was bitwise inverting instead of !
   - Add further comments about assumptions related to pin mux.
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 | 206 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 206 insertions(+)

diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index 3e3aa95..9a82e88 100644
--- a/drivers/hwmon/pmbus/ucd9000.c
+++ b/drivers/hwmon/pmbus/ucd9000.c
@@ -26,6 +26,7 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/gpio.h>
 #include "pmbus.h"
 
 enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
@@ -34,8 +35,18 @@
 #define UCD9000_NUM_PAGES		0xd6
 #define UCD9000_FAN_CONFIG_INDEX	0xe7
 #define UCD9000_FAN_CONFIG		0xe8
+#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 +57,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 +134,166 @@ 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 int ucd9000_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -227,6 +402,37 @@ static int ucd9000_probe(struct i2c_client *client,
 		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
 	}
 
+	/*
+	 * Note:
+	 *
+	 * Pinmux support has not been added to the new gpio_chip.
+	 * This support should be added when possible given the mux
+	 * behavior of these IO devices.
+	 */
+	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;
+	}
+
 	return pmbus_do_probe(client, mid, info);
 }
 
-- 
1.8.2.2

[PATCH linux dev-4.10 v7 2/2] hwmon: (ucd9000) Add debugfs to list mfr_status info

[Christopher Bostic]

Expose via files in debugfs the gpiN_fault fields as well as
the entire contents of the mfr_status register.  Allows user
space to pull this data for error logging etc...

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
v7 - Patch order changed from #3 to #2 and as a result needed
     to add the ucd9000_remove definition here.
v6 - Remove chip idx values on remove so that we can retain the
     same indexes regardless how many times we're bound/unbound.
   - Remove type 90160 check when adding debugfs.
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 | 166 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 165 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index 9a82e88..26870e6 100644
--- a/drivers/hwmon/pmbus/ucd9000.c
+++ b/drivers/hwmon/pmbus/ucd9000.c
@@ -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>
@@ -27,6 +28,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c/pmbus.h>
 #include <linux/gpio.h>
+#include <linux/idr.h>
 #include "pmbus.h"
 
 enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
@@ -35,6 +37,7 @@
 #define UCD9000_NUM_PAGES		0xd6
 #define UCD9000_FAN_CONFIG_INDEX	0xe7
 #define UCD9000_FAN_CONFIG		0xe8
+#define UCD9000_MFR_STATUS		0xf3
 #define UCD9000_GPIO_SELECT		0xfa
 #define UCD9000_GPIO_CONFIG		0xfb
 #define UCD9000_DEVICE_ID		0xfd
@@ -56,17 +59,29 @@
 #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;
+	int idx;
 };
 #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;
@@ -294,6 +309,141 @@ 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;
+
+	data->idx = ida_simple_get(&ucd9000_ida, 0, INT_MAX, GFP_KERNEL);
+	scnprintf(name, UCD9000_NAME_SIZE, "ucd9000.%d", data->idx);
+	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;
+		ida_simple_remove(&ucd9000_ida, data->idx);
+		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) {
+		ida_simple_remove(&ucd9000_ida, data->idx);
+		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 int ucd9000_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -433,16 +583,30 @@ static int ucd9000_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	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)
+{
+	struct ucd9000_data *data
+		= to_ucd9000_data(pmbus_get_driver_info(client));
+
+	ida_simple_remove(&ucd9000_ida, data->idx);
+	debugfs_remove_recursive(ucd9000_debugfs_dir);
+	return pmbus_do_remove(client);
+}
+
 /* This is the driver that will be inserted */
 static struct i2c_driver ucd9000_driver = {
 	.driver = {
 		.name = "ucd9000",
 	},
 	.probe = ucd9000_probe,
-	.remove = pmbus_do_remove,
+	.remove = ucd9000_remove,
 	.id_table = ucd9000_id,
 };
 
-- 
1.8.2.2

Re: [PATCH linux dev-4.10 v7 1/2] hwmon: (ucd9000) Add gpio chip interface

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 9732 bytes --]

On Wed, 2017-09-06 at 16:00 -0500, Christopher Bostic wrote:
> 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.
> 
> > Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
> v7 - Reorder this patch from #2 in series to #1
> v6 - Break out sysfs clear logged faults into separate patch
>    - Fix logic bug that was bitwise inverting instead of !
>    - Add further comments about assumptions related to pin mux.
> 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 | 206 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 206 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> index 3e3aa95..9a82e88 100644
> --- a/drivers/hwmon/pmbus/ucd9000.c
> +++ b/drivers/hwmon/pmbus/ucd9000.c
> @@ -26,6 +26,7 @@
>  #include <linux/slab.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c/pmbus.h>
> +#include <linux/gpio.h>
>  #include "pmbus.h"
>  
>  enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
> @@ -34,8 +35,18 @@
>  #define UCD9000_NUM_PAGES		0xd6
>  #define UCD9000_FAN_CONFIG_INDEX	0xe7
>  #define UCD9000_FAN_CONFIG		0xe8
> +#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 +57,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 +134,166 @@ 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));

Why are we triple negating? One is enough. This will work, but it's unnecessary
and excessive.

> +}
> +
> +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 int ucd9000_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -227,6 +402,37 @@ static int ucd9000_probe(struct i2c_client *client,
>  		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
>  	}
>  
> +	/*
> +	 * Note:
> +	 *
> +	 * Pinmux support has not been added to the new gpio_chip.
> +	 * This support should be added when possible given the mux
> +	 * behavior of these IO devices.
> +	 */
> +	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;
> +	}
> +
>  	return pmbus_do_probe(client, mid, info);
>  }
>  

In the interest of expediting this:

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v7 2/2] hwmon: (ucd9000) Add debugfs to list mfr_status info

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 8669 bytes --]

On Wed, 2017-09-06 at 16:00 -0500, Christopher Bostic wrote:
> Expose via files in debugfs the gpiN_fault fields as well as
> the entire contents of the mfr_status register.  Allows user
> space to pull this data for error logging etc...
> 
> > Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
> v7 - Patch order changed from #3 to #2 and as a result needed
>      to add the ucd9000_remove definition here.
> v6 - Remove chip idx values on remove so that we can retain the
>      same indexes regardless how many times we're bound/unbound.
>    - Remove type 90160 check when adding debugfs.
> 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 | 166 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 165 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> index 9a82e88..26870e6 100644
> --- a/drivers/hwmon/pmbus/ucd9000.c
> +++ b/drivers/hwmon/pmbus/ucd9000.c
> @@ -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>
> @@ -27,6 +28,7 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c/pmbus.h>
>  #include <linux/gpio.h>
> +#include <linux/idr.h>
>  #include "pmbus.h"
>  
>  enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
> @@ -35,6 +37,7 @@
>  #define UCD9000_NUM_PAGES		0xd6
>  #define UCD9000_FAN_CONFIG_INDEX	0xe7
>  #define UCD9000_FAN_CONFIG		0xe8
> +#define UCD9000_MFR_STATUS		0xf3
>  #define UCD9000_GPIO_SELECT		0xfa
>  #define UCD9000_GPIO_CONFIG		0xfb
>  #define UCD9000_DEVICE_ID		0xfd
> @@ -56,17 +59,29 @@
>  #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;
> +	int idx;
>  };
>  #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;
> @@ -294,6 +309,141 @@ 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;

I thought we were fixing this up?

> +
> +#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;
> +
> +	data->idx = ida_simple_get(&ucd9000_ida, 0, INT_MAX, GFP_KERNEL);
> +	scnprintf(name, UCD9000_NAME_SIZE, "ucd9000.%d", data->idx);
> +	ucd9000_debugfs_dir = debugfs_create_dir(name, NULL);

We should be storing the dentry in the context struct. We're now uniquely
naming the directory, but we're still storing the dentry in static storage. The
code as it stands will still stomp over the top of existing devices on probe,
and it cannot be cleaned up properly. In fact, we'll cleanup the wrong entry
depending on the order of probe/remove across the devices. It's not just fixed
by uniquely naming the directory.

Andrew

> +	if (IS_ERR(ucd9000_debugfs_dir)) {
> +		dev_warn(&client->dev, "Failed to create debugfs dir: %p\n",
> +			ucd9000_debugfs_dir);
> +		ucd9000_debugfs_dir = NULL;
> +		ida_simple_remove(&ucd9000_ida, data->idx);
> +		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) {
> +		ida_simple_remove(&ucd9000_ida, data->idx);
> +		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 int ucd9000_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -433,16 +583,30 @@ static int ucd9000_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	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)
> +{
> +	struct ucd9000_data *data
> +		= to_ucd9000_data(pmbus_get_driver_info(client));
> +
> +	ida_simple_remove(&ucd9000_ida, data->idx);
> +	debugfs_remove_recursive(ucd9000_debugfs_dir);
> +	return pmbus_do_remove(client);
> +}
> +
>  /* This is the driver that will be inserted */
>  static struct i2c_driver ucd9000_driver = {
>  	.driver = {
>  		.name = "ucd9000",
>  	},
>  	.probe = ucd9000_probe,
> -	.remove = pmbus_do_remove,
> +	.remove = ucd9000_remove,
>  	.id_table = ucd9000_id,
>  };
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH linux dev-4.10 v7 1/2] hwmon: (ucd9000) Add gpio chip interface

[Christopher Bostic]

On 9/6/17 7:27 PM, Andrew Jeffery wrote:
> On Wed, 2017-09-06 at 16:00 -0500, Christopher Bostic wrote:
>> 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.
>>   
>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> ---
>> v7 - Reorder this patch from #2 in series to #1
>> v6 - Break out sysfs clear logged faults into separate patch
>>     - Fix logic bug that was bitwise inverting instead of !
>>     - Add further comments about assumptions related to pin mux.
>> 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 | 206 ++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 206 insertions(+)
>>   
>> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
>> index 3e3aa95..9a82e88 100644
>> --- a/drivers/hwmon/pmbus/ucd9000.c
>> +++ b/drivers/hwmon/pmbus/ucd9000.c
>> @@ -26,6 +26,7 @@
>>   #include <linux/slab.h>
>>   #include <linux/i2c.h>
>>   #include <linux/i2c/pmbus.h>
>> +#include <linux/gpio.h>
>>   #include "pmbus.h"
>>   
>>   enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
>> @@ -34,8 +35,18 @@
>>   #define UCD9000_NUM_PAGES		0xd6
>>   #define UCD9000_FAN_CONFIG_INDEX	0xe7
>>   #define UCD9000_FAN_CONFIG		0xe8
>> +#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 +57,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 +134,166 @@ 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));
> Why are we triple negating? One is enough. This will work, but it's unnecessary
> and excessive.
>

Will remove the unnecessary negating since debugfs updates in patch 0002 
of this series needs updating anyway.

Thanks
Chris

>> +}
>> +
>> +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 int ucd9000_probe(struct i2c_client *client,
>>   			 const struct i2c_device_id *id)
>>   {
>> @@ -227,6 +402,37 @@ static int ucd9000_probe(struct i2c_client *client,
>>   		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
>>   	}
>>   
>> +	/*
>> +	 * Note:
>> +	 *
>> +	 * Pinmux support has not been added to the new gpio_chip.
>> +	 * This support should be added when possible given the mux
>> +	 * behavior of these IO devices.
>> +	 */
>> +	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;
>> +	}
>> +
>>   	return pmbus_do_probe(client, mid, info);
>>   }
>>   
> In the interest of expediting this:
>
> Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

Re: [PATCH linux dev-4.10 v7 2/2] hwmon: (ucd9000) Add debugfs to list mfr_status info

[Christopher Bostic]

On 9/6/17 8:17 PM, Andrew Jeffery wrote:
> On Wed, 2017-09-06 at 16:00 -0500, Christopher Bostic wrote:
>> Expose via files in debugfs the gpiN_fault fields as well as
>> the entire contents of the mfr_status register.  Allows user
>> space to pull this data for error logging etc...
>>   
>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> ---
>> v7 - Patch order changed from #3 to #2 and as a result needed
>>       to add the ucd9000_remove definition here.
>> v6 - Remove chip idx values on remove so that we can retain the
>>       same indexes regardless how many times we're bound/unbound.
>>     - Remove type 90160 check when adding debugfs.
>> 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 | 166 +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 165 insertions(+), 1 deletion(-)
>>   
>> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
>> index 9a82e88..26870e6 100644
>> --- a/drivers/hwmon/pmbus/ucd9000.c
>> +++ b/drivers/hwmon/pmbus/ucd9000.c
>> @@ -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>
>> @@ -27,6 +28,7 @@
>>   #include <linux/i2c.h>
>>   #include <linux/i2c/pmbus.h>
>>   #include <linux/gpio.h>
>> +#include <linux/idr.h>
>>   #include "pmbus.h"
>>   
>>   enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
>> @@ -35,6 +37,7 @@
>>   #define UCD9000_NUM_PAGES		0xd6
>>   #define UCD9000_FAN_CONFIG_INDEX	0xe7
>>   #define UCD9000_FAN_CONFIG		0xe8
>> +#define UCD9000_MFR_STATUS		0xf3
>>   #define UCD9000_GPIO_SELECT		0xfa
>>   #define UCD9000_GPIO_CONFIG		0xfb
>>   #define UCD9000_DEVICE_ID		0xfd
>> @@ -56,17 +59,29 @@
>>   #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;
>> +	int idx;
>>   };
>>   #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;
>> @@ -294,6 +309,141 @@ 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;
> I thought we were fixing this up?
>
>> +
>> +#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;
>> +
>> +	data->idx = ida_simple_get(&ucd9000_ida, 0, INT_MAX, GFP_KERNEL);
>> +	scnprintf(name, UCD9000_NAME_SIZE, "ucd9000.%d", data->idx);
>> +	ucd9000_debugfs_dir = debugfs_create_dir(name, NULL);
> We should be storing the dentry in the context struct. We're now uniquely
> naming the directory, but we're still storing the dentry in static storage. The
> code as it stands will still stomp over the top of existing devices on probe,
> and it cannot be cleaned up properly. In fact, we'll cleanup the wrong entry
> depending on the order of probe/remove across the devices. It's not just fixed
> by uniquely naming the directory.

OK, will correct for next set.

Thanks,
Chris

>
> Andrew
>
>> +	if (IS_ERR(ucd9000_debugfs_dir)) {
>> +		dev_warn(&client->dev, "Failed to create debugfs dir: %p\n",
>> +			ucd9000_debugfs_dir);
>> +		ucd9000_debugfs_dir = NULL;
>> +		ida_simple_remove(&ucd9000_ida, data->idx);
>> +		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) {
>> +		ida_simple_remove(&ucd9000_ida, data->idx);
>> +		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 int ucd9000_probe(struct i2c_client *client,
>>   			 const struct i2c_device_id *id)
>>   {
>> @@ -433,16 +583,30 @@ static int ucd9000_probe(struct i2c_client *client,
>>   		return ret;
>>   	}
>>   
>> +	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)
>> +{
>> +	struct ucd9000_data *data
>> +		= to_ucd9000_data(pmbus_get_driver_info(client));
>> +
>> +	ida_simple_remove(&ucd9000_ida, data->idx);
>> +	debugfs_remove_recursive(ucd9000_debugfs_dir);
>> +	return pmbus_do_remove(client);
>> +}
>> +
>>   /* This is the driver that will be inserted */
>>   static struct i2c_driver ucd9000_driver = {
>>   	.driver = {
>>   		.name = "ucd9000",
>>   	},
>>   	.probe = ucd9000_probe,
>> -	.remove = pmbus_do_remove,
>> +	.remove = ucd9000_remove,
>>   	.id_table = ucd9000_id,
>>   };
>>