[PATCH v4 0/2] hwmon: (ucd9000) Add gpio and debugfs interfaces

[PATCH v4 0/2] hwmon: (ucd9000) Add gpio and debugfs interfaces

[Eddie James]

The ucd9000 series chips have gpio pins. Add a gpio chip interface to the ucd
device so that users can query and set the state of the gpio pins.

Add a debugfs interface using the existing pmbus debugfs directory to provide
MFR_STATUS and the status of the gpi faults to users.

Changes since v3:
 - remove setting of gpio_chip->owner
 - format the mfr_status data
 - switch to #ifdef rather than #if IS_ENABLED for debugfs

Changes since v2:
 - split the gpio registration into it's own function

Changes since v1:
 - dropped dev_err messages
 - made gpio chip registration conditional on having gpio pins
 - made mfr_status debugfs attribute more simple

Christopher Bostic (2):
  hwmon: (ucd9000) Add gpio chip interface
  hwmon: (ucd9000) Add debugfs attributes to provide mfr_status

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

-- 
1.8.3.1

[PATCH v4 1/2] hwmon: (ucd9000) Add gpio chip interface

[Eddie James]

From: Christopher Bostic <cbostic@linux.vnet.ibm.com>

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

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/hwmon/pmbus/ucd9000.c | 201 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)

diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index b74dbec..a34ffc4 100644
--- a/drivers/hwmon/pmbus/ucd9000.c
+++ b/drivers/hwmon/pmbus/ucd9000.c
@@ -27,6 +27,7 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/pmbus.h>
+#include <linux/gpio.h>
 #include "pmbus.h"
 
 enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
@@ -35,8 +36,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)
 
@@ -47,9 +58,15 @@
 
 #define UCD9000_NUM_FAN		4
 
+#define UCD9000_GPIO_NAME_LEN	16
+#define UCD9090_NUM_GPIOS	23
+#define UCD901XX_NUM_GPIOS	26
+#define UCD90910_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)
 
@@ -149,6 +166,188 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
 };
 MODULE_DEVICE_TABLE(of, ucd9000_of_match);
 
+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)
+		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)
+		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_dbg(&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_dbg(&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_dbg(&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)
+		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)
+		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)
+		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 void ucd9000_probe_gpio(struct i2c_client *client,
+			       const struct i2c_device_id *mid,
+			       struct ucd9000_data *data)
+{
+	int rc;
+
+	switch (mid->driver_data) {
+	case ucd9090:
+		data->gpio.ngpio = UCD9090_NUM_GPIOS;
+		break;
+	case ucd90120:
+	case ucd90124:
+	case ucd90160:
+		data->gpio.ngpio = UCD901XX_NUM_GPIOS;
+		break;
+	case ucd90910:
+		data->gpio.ngpio = UCD90910_NUM_GPIOS;
+		break;
+	default:
+		return; /* GPIO support is optional. */
+	}
+
+	/*
+	 * 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 = 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 = true;
+	data->gpio.base = -1;
+	data->gpio.parent = &client->dev;
+
+	rc = devm_gpiochip_add_data(&client->dev, &data->gpio, client);
+	if (rc)
+		dev_warn(&client->dev, "Could not add gpiochip: %d\n", rc);
+}
+
 static int ucd9000_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -263,6 +462,8 @@ static int ucd9000_probe(struct i2c_client *client,
 		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
 	}
 
+	ucd9000_probe_gpio(client, mid, data);
+
 	return pmbus_do_probe(client, mid, info);
 }
 
-- 
1.8.3.1

[PATCH v4 2/2] hwmon: (ucd9000) Add debugfs attributes to provide mfr_status

[Eddie James]

From: Christopher Bostic <cbostic@linux.vnet.ibm.com>

Expose the gpiN_fault fields of mfr_status as individual debugfs
attributes. This provides a way for users to be easily notified of gpi
faults. Also provide the whole mfr_status register in debugfs.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/hwmon/pmbus/ucd9000.c | 153 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 152 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index a34ffc4..f03c404 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/of_device.h>
@@ -36,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
@@ -63,13 +65,22 @@
 #define UCD901XX_NUM_GPIOS	26
 #define UCD90910_NUM_GPIOS	26
 
+#define UCD9000_DEBUGFS_NAME_LEN	24
+#define UCD9000_GPI_COUNT		8
+
 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;
@@ -306,6 +317,137 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
 					  val);
 }
 
+#ifdef CONFIG_DEBUG_FS
+static int ucd9000_get_mfr_status(struct i2c_client *client, u8 *buffer)
+{
+	int ret = pmbus_set_page(client, 0);
+
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * 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, buffer);
+}
+
+static int ucd9000_debugfs_show_mfr_status_bit(void *data, u64 *val)
+{
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	u8 buffer[4];
+	int ret;
+
+	/*
+	 * This attribute is only created for devices that return 4 bytes for
+	 * status_mfr, so it's safe to call with 4-byte buffer.
+	 */
+	ret = ucd9000_get_mfr_status(client, buffer);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Attribute only created for devices with gpi fault bits at bits
+	 * 16-23, which is the second byte of the response.
+	 */
+	*val = !!(buffer[1] & BIT(entry->index));
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_bit,
+			 ucd9000_debugfs_show_mfr_status_bit, NULL, "%1lld\n");
+
+static ssize_t ucd9000_debugfs_read_mfr_status(struct file *file,
+					       char __user *buf, size_t count,
+					       loff_t *ppos)
+{
+	struct i2c_client *client = file->private_data;
+	u8 buffer[5] = { 0 };	/* Need max 5 bytes for any ucd9000 chip. */
+	char str[12] = { 0 };	/* Two chars per byte plus \n and \0. */
+	int i, num_bytes, num_chars = 0, rc;
+
+	num_bytes = ucd9000_get_mfr_status(client, buffer);
+	if (num_bytes < 0)
+		return num_bytes;
+
+	for (i = 0; i < num_bytes; ++i) {
+		rc = snprintf(&str[num_chars], (sizeof(str) - 1) - num_chars,
+			      "%02x", buffer[i]);
+		if (rc <= 0)
+			break;
+
+		num_chars += rc;
+	}
+
+	str[num_chars] = '\n';
+
+	return simple_read_from_buffer(buf, count, ppos, str, num_chars + 2);
+}
+
+static const struct file_operations ucd9000_debugfs_show_mfr_status_fops = {
+	.llseek = noop_llseek,
+	.read = ucd9000_debugfs_read_mfr_status,
+	.open = simple_open,
+};
+
+static int ucd9000_init_debugfs(struct i2c_client *client,
+				const struct i2c_device_id *mid,
+				struct ucd9000_data *data)
+{
+	struct dentry *debugfs;
+	struct ucd9000_debugfs_entry *entries;
+	int i;
+	char name[UCD9000_DEBUGFS_NAME_LEN];
+
+	debugfs = pmbus_get_debugfs_dir(client);
+	if (!debugfs)
+		return -ENOENT;
+
+	data->debugfs = debugfs_create_dir(client->name, debugfs);
+	if (!data->debugfs)
+		return -ENOENT;
+
+	/*
+	 * Of the chips this driver supports, only the UCD9090, UCD90160,
+	 * and UCD90910 report GPI faults in their MFR_STATUS register, so only
+	 * create the GPI fault debugfs attributes for those chips.
+	 */
+	if (mid->driver_data == ucd9090 || mid->driver_data == ucd90160 ||
+	    mid->driver_data == ucd90910) {
+		entries = devm_kzalloc(&client->dev,
+				       sizeof(*entries) * UCD9000_GPI_COUNT,
+				       GFP_KERNEL);
+		if (!entries)
+			return -ENOMEM;
+
+		for (i = 0; i < UCD9000_GPI_COUNT; i++) {
+			entries[i].client = client;
+			entries[i].index = i;
+			scnprintf(name, UCD9000_DEBUGFS_NAME_LEN,
+				  "gpi%d_alarm", i + 1);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[i],
+					    &ucd9000_debugfs_mfr_status_bit);
+		}
+	}
+
+	scnprintf(name, UCD9000_DEBUGFS_NAME_LEN, "mfr_status");
+	debugfs_create_file(name, 0444, data->debugfs, client,
+			    &ucd9000_debugfs_show_mfr_status_fops);
+
+	return 0;
+}
+#else
+static int ucd9000_init_debugfs(struct i2c_client *client,
+				const struct i2c_device_id *mid,
+				struct ucd9000_data *data)
+{
+	return 0;
+}
+#endif /* CONFIG_DEBUG_FS */
+
 static void ucd9000_probe_gpio(struct i2c_client *client,
 			       const struct i2c_device_id *mid,
 			       struct ucd9000_data *data)
@@ -464,7 +606,16 @@ static int ucd9000_probe(struct i2c_client *client,
 
 	ucd9000_probe_gpio(client, mid, data);
 
-	return pmbus_do_probe(client, mid, info);
+	ret = pmbus_do_probe(client, mid, info);
+	if (ret)
+		return ret;
+
+	ret = ucd9000_init_debugfs(client, mid, data);
+	if (ret)
+		dev_warn(&client->dev, "Failed to register debugfs: %d\n",
+			 ret);
+
+	return 0;
 }
 
 /* This is the driver that will be inserted */
-- 
1.8.3.1

Re: [PATCH v4 2/2] hwmon: (ucd9000) Add debugfs attributes to provide mfr_status

[Guenter Roeck]

On Wed, Mar 14, 2018 at 05:32:14PM -0500, Eddie James wrote:
> From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> 
> Expose the gpiN_fault fields of mfr_status as individual debugfs
> attributes. This provides a way for users to be easily notified of gpi
> faults. Also provide the whole mfr_status register in debugfs.
> 
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---
>  drivers/hwmon/pmbus/ucd9000.c | 153 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 152 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> index a34ffc4..f03c404 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/of_device.h>
> @@ -36,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
> @@ -63,13 +65,22 @@
>  #define UCD901XX_NUM_GPIOS	26
>  #define UCD90910_NUM_GPIOS	26
>  
> +#define UCD9000_DEBUGFS_NAME_LEN	24
> +#define UCD9000_GPI_COUNT		8
> +
>  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;
> @@ -306,6 +317,137 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
>  					  val);
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +static int ucd9000_get_mfr_status(struct i2c_client *client, u8 *buffer)
> +{
> +	int ret = pmbus_set_page(client, 0);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * 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.
> +	 */

I am really uneasy about the limited buffer sizes.

The limited transfer size isn't guaranteed by the protocol. You are making 
assumptions about chip operation which adds risk. It would be much safer if
you would just use the maximum smbus block transfer size for all buffers.
It is not as if the few extra bytes on the stack would hurt, or am I missing
something ? As a bonus, all the comments about the transfers presumably being
safe would no longer be needed.

> +	return i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS, buffer);
> +}
> +
> +static int ucd9000_debugfs_show_mfr_status_bit(void *data, u64 *val)
> +{
> +	struct ucd9000_debugfs_entry *entry = data;
> +	struct i2c_client *client = entry->client;
> +	u8 buffer[4];
> +	int ret;
> +
> +	/*
> +	 * This attribute is only created for devices that return 4 bytes for
> +	 * status_mfr, so it's safe to call with 4-byte buffer.
> +	 */
> +	ret = ucd9000_get_mfr_status(client, buffer);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Attribute only created for devices with gpi fault bits at bits
> +	 * 16-23, which is the second byte of the response.
> +	 */
> +	*val = !!(buffer[1] & BIT(entry->index));
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_bit,
> +			 ucd9000_debugfs_show_mfr_status_bit, NULL, "%1lld\n");
> +
> +static ssize_t ucd9000_debugfs_read_mfr_status(struct file *file,
> +					       char __user *buf, size_t count,
> +					       loff_t *ppos)
> +{
> +	struct i2c_client *client = file->private_data;
> +	u8 buffer[5] = { 0 };	/* Need max 5 bytes for any ucd9000 chip. */
> +	char str[12] = { 0 };	/* Two chars per byte plus \n and \0. */
> +	int i, num_bytes, num_chars = 0, rc;
> +
> +	num_bytes = ucd9000_get_mfr_status(client, buffer);
> +	if (num_bytes < 0)
> +		return num_bytes;
> +
> +	for (i = 0; i < num_bytes; ++i) {
> +		rc = snprintf(&str[num_chars], (sizeof(str) - 1) - num_chars,
> +			      "%02x", buffer[i]);
> +		if (rc <= 0)
> +			break;
> +
> +		num_chars += rc;
> +	}
> +
> +	str[num_chars] = '\n';

Sorry for being annoying, but have you considered using hex_dump_to_buffer() ?

Thanks,
Guenter

> +
> +	return simple_read_from_buffer(buf, count, ppos, str, num_chars + 2);
> +}
> +
> +static const struct file_operations ucd9000_debugfs_show_mfr_status_fops = {
> +	.llseek = noop_llseek,
> +	.read = ucd9000_debugfs_read_mfr_status,
> +	.open = simple_open,
> +};
> +
> +static int ucd9000_init_debugfs(struct i2c_client *client,
> +				const struct i2c_device_id *mid,
> +				struct ucd9000_data *data)
> +{
> +	struct dentry *debugfs;
> +	struct ucd9000_debugfs_entry *entries;
> +	int i;
> +	char name[UCD9000_DEBUGFS_NAME_LEN];
> +
> +	debugfs = pmbus_get_debugfs_dir(client);
> +	if (!debugfs)
> +		return -ENOENT;
> +
> +	data->debugfs = debugfs_create_dir(client->name, debugfs);
> +	if (!data->debugfs)
> +		return -ENOENT;
> +
> +	/*
> +	 * Of the chips this driver supports, only the UCD9090, UCD90160,
> +	 * and UCD90910 report GPI faults in their MFR_STATUS register, so only
> +	 * create the GPI fault debugfs attributes for those chips.
> +	 */
> +	if (mid->driver_data == ucd9090 || mid->driver_data == ucd90160 ||
> +	    mid->driver_data == ucd90910) {
> +		entries = devm_kzalloc(&client->dev,
> +				       sizeof(*entries) * UCD9000_GPI_COUNT,
> +				       GFP_KERNEL);
> +		if (!entries)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < UCD9000_GPI_COUNT; i++) {
> +			entries[i].client = client;
> +			entries[i].index = i;
> +			scnprintf(name, UCD9000_DEBUGFS_NAME_LEN,
> +				  "gpi%d_alarm", i + 1);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[i],
> +					    &ucd9000_debugfs_mfr_status_bit);
> +		}
> +	}
> +
> +	scnprintf(name, UCD9000_DEBUGFS_NAME_LEN, "mfr_status");
> +	debugfs_create_file(name, 0444, data->debugfs, client,
> +			    &ucd9000_debugfs_show_mfr_status_fops);
> +
> +	return 0;
> +}
> +#else
> +static int ucd9000_init_debugfs(struct i2c_client *client,
> +				const struct i2c_device_id *mid,
> +				struct ucd9000_data *data)
> +{
> +	return 0;
> +}
> +#endif /* CONFIG_DEBUG_FS */
> +
>  static void ucd9000_probe_gpio(struct i2c_client *client,
>  			       const struct i2c_device_id *mid,
>  			       struct ucd9000_data *data)
> @@ -464,7 +606,16 @@ static int ucd9000_probe(struct i2c_client *client,
>  
>  	ucd9000_probe_gpio(client, mid, data);
>  
> -	return pmbus_do_probe(client, mid, info);
> +	ret = pmbus_do_probe(client, mid, info);
> +	if (ret)
> +		return ret;
> +
> +	ret = ucd9000_init_debugfs(client, mid, data);
> +	if (ret)
> +		dev_warn(&client->dev, "Failed to register debugfs: %d\n",
> +			 ret);
> +
> +	return 0;
>  }
>  
>  /* This is the driver that will be inserted */
> -- 
> 1.8.3.1
> 

Re: [PATCH v4 1/2] hwmon: (ucd9000) Add gpio chip interface

[Guenter Roeck]

On Wed, Mar 14, 2018 at 05:32:13PM -0500, Eddie James wrote:
> From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> 
> Add a struct gpio_chip and define some methods so that this device's
> I/O can be accessed via /sys/class/gpio.
> 
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>

This patch looks good to me. Any further comments ? Otherwise I'll
go ahead and apply to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/ucd9000.c | 201 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 201 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> index b74dbec..a34ffc4 100644
> --- a/drivers/hwmon/pmbus/ucd9000.c
> +++ b/drivers/hwmon/pmbus/ucd9000.c
> @@ -27,6 +27,7 @@
>  #include <linux/slab.h>
>  #include <linux/i2c.h>
>  #include <linux/pmbus.h>
> +#include <linux/gpio.h>
>  #include "pmbus.h"
>  
>  enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
> @@ -35,8 +36,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)
>  
> @@ -47,9 +58,15 @@
>  
>  #define UCD9000_NUM_FAN		4
>  
> +#define UCD9000_GPIO_NAME_LEN	16
> +#define UCD9090_NUM_GPIOS	23
> +#define UCD901XX_NUM_GPIOS	26
> +#define UCD90910_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)
>  
> @@ -149,6 +166,188 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
>  };
>  MODULE_DEVICE_TABLE(of, ucd9000_of_match);
>  
> +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)
> +		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)
> +		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_dbg(&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_dbg(&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_dbg(&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)
> +		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)
> +		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)
> +		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 void ucd9000_probe_gpio(struct i2c_client *client,
> +			       const struct i2c_device_id *mid,
> +			       struct ucd9000_data *data)
> +{
> +	int rc;
> +
> +	switch (mid->driver_data) {
> +	case ucd9090:
> +		data->gpio.ngpio = UCD9090_NUM_GPIOS;
> +		break;
> +	case ucd90120:
> +	case ucd90124:
> +	case ucd90160:
> +		data->gpio.ngpio = UCD901XX_NUM_GPIOS;
> +		break;
> +	case ucd90910:
> +		data->gpio.ngpio = UCD90910_NUM_GPIOS;
> +		break;
> +	default:
> +		return; /* GPIO support is optional. */
> +	}
> +
> +	/*
> +	 * 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 = 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 = true;
> +	data->gpio.base = -1;
> +	data->gpio.parent = &client->dev;
> +
> +	rc = devm_gpiochip_add_data(&client->dev, &data->gpio, client);
> +	if (rc)
> +		dev_warn(&client->dev, "Could not add gpiochip: %d\n", rc);
> +}
> +
>  static int ucd9000_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -263,6 +462,8 @@ static int ucd9000_probe(struct i2c_client *client,
>  		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
>  	}
>  
> +	ucd9000_probe_gpio(client, mid, data);
> +
>  	return pmbus_do_probe(client, mid, info);
>  }
>  
> -- 
> 1.8.3.1
>