hwmon: (ucd9000) Add gpio and debugfs interfaces

From: Christopher Bostic <[email protected]>

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 <[email protected]>

Signed-off-by Andrew Jeffery <[email protected]>

Signed-off-by Eddie James <[email protected]>

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

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

index b74dbec..e3a507f 100644
@@ -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,168 @@ 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,

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

+		return ret;
+	}
+
+	return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
+}
+

drivers/hwmon/pmbus/ucd9000.c +141 -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_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)
  {

@@ -263,6 +442,47 @@ 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.

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

+		return ret;
+	}
+
  	return pmbus_do_probe(client, mid, info);
  }
 

From: Christopher Bostic <[email protected]>

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 <[email protected]>

Signed-off-by Andrew Jeffery <[email protected]>

Signed-off-by Eddie James <[email protected]>

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

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

index e3a507f..297da0e 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/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;

@@ -328,6 +339,156 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
  					  val);
  }
 
+#if IS_ENABLED(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) {
+		dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+			ret);
+
+		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 int ucd9000_debugfs_show_mfr_status_word2(void *data, u64 *val)
+{
+	struct i2c_client *client = data;
+	__be16 buffer;
+	int ret;
+
+	ret = ucd9000_get_mfr_status(client, (u8 *)&buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+			ret);
+
+		return ret;
+	}
+
+	*val = be16_to_cpu(buffer);
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_word2,
+			 ucd9000_debugfs_show_mfr_status_word2, NULL,
+			 "%04llx\n");
+
+static int ucd9000_debugfs_show_mfr_status_word4(void *data, u64 *val)
+{
+	struct i2c_client *client = data;
+	__be32 buffer;
+	int ret;
+
+	ret = ucd9000_get_mfr_status(client, (u8 *)&buffer);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+			ret);
+
+		return ret;
+	}
+
+	*val = be32_to_cpu(buffer);
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_word4,
+			 ucd9000_debugfs_show_mfr_status_word4, NULL,
+			 "%08llx\n");
+
+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,

drivers/hwmon/pmbus/ucd9000.c +18 -1

+#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)
  {

@@ -483,7 +644,16 @@ static int ucd9000_probe(struct i2c_client *client,
  		return ret;
  	}
 
-	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 */

Quoted 203 lines · Eddie James

From: Christopher Bostic <[email protected]>

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 <[email protected]>

Signed-off-by Andrew Jeffery <[email protected]>

Signed-off-by Eddie James <[email protected]>

---
  drivers/hwmon/pmbus/ucd9000.c | 172
+++++++++++++++++++++++++++++++++++++++++-

  1 file changed, 171 insertions(+), 1 deletion(-)

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

index e3a507f..297da0e 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/of_device.h>

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

@@ -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

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

+#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;

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

+};
+

  static int ucd9000_get_fan_config(struct i2c_client *client, int fan) {
      int fan_config = 0;

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

@@ -328,6 +339,156 @@ static int ucd9000_gpio_direction_output(struct 
 gpio_chip *gc,
                         val);
   }
   +#if IS_ENABLED(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);

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

+}
+
+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) {
+        dev_err(&client->dev, "Failed to read mfr status. rc:%d\n", +            ret);

drivers/hwmon/pmbus/ucd9000.c +79 -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 int ucd9000_debugfs_show_mfr_status_word2(void *data, u64 *val)
+{
+    struct i2c_client *client = data;
+    __be16 buffer;
+    int ret;
+
+    ret = ucd9000_get_mfr_status(client, (u8 *)&buffer);
+    if (ret < 0) {
+        dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+            ret);
+
+        return ret;
+    }
+
+    *val = be16_to_cpu(buffer);
+
+    return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_word2,
+             ucd9000_debugfs_show_mfr_status_word2, NULL,
+             "%04llx\n");
+
+static int ucd9000_debugfs_show_mfr_status_word4(void *data, u64 *val)
+{
+    struct i2c_client *client = data;
+    __be32 buffer;
+    int ret;
+
+    ret = ucd9000_get_mfr_status(client, (u8 *)&buffer);
+    if (ret < 0) {
+        dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+            ret);
+
+        return ret;
+    }
+
+    *val = be32_to_cpu(buffer);
+
+    return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_word4,
+             ucd9000_debugfs_show_mfr_status_word4, NULL,
+             "%08llx\n");
+
+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;

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

+
+        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_mfr_status_word4);
+    } else if (mid->driver_data == ucd90120 ||
+           mid->driver_data == ucd90124) {
+        scnprintf(name, UCD9000_DEBUGFS_NAME_LEN, "mfr_status");
+        debugfs_create_file(name, 0444, data->debugfs, client,
+                    &ucd9000_debugfs_mfr_status_word2);
+    }

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 384

Quoted 3 lines

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

+
+    return 0;
+}

Is all that complexity really worth it ? Why not just read the manufacturing status as byte string into a buffer and use hexdump to pront it, no matter how many bytes are actually returned ? This would also be less error prone, and automatically support future chips.

Thanks,
Guenter

Quoted 28 lines

drivers/hwmon/pmbus/ucd9000.c +8 -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) {

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

@@ -483,7 +644,16 @@ static int ucd9000_probe(struct i2c_client *client,
           return ret;
       }
   -    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 */
 

Quoted 210 lines · Guenter Roeck

Quoted 198 lines · Eddie James

From: Christopher Bostic <[email protected]>

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 <[email protected]>

Signed-off-by Andrew Jeffery <[email protected]>

Signed-off-by Eddie James <[email protected]>

---
  drivers/hwmon/pmbus/ucd9000.c | 172
+++++++++++++++++++++++++++++++++++++++++-

  1 file changed, 171 insertions(+), 1 deletion(-)

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

index e3a507f..297da0e 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/of_device.h>

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

@@ -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

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

+#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;

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

+};
+

  static int ucd9000_get_fan_config(struct i2c_client *client, int fan) {
      int fan_config = 0;

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

@@ -328,6 +339,156 @@ static int ucd9000_gpio_direction_output(struct
 gpio_chip *gc,
                         val);
   }
   +#if IS_ENABLED(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);

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

+}
+
+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) {
+        dev_err(&client->dev, "Failed to read mfr status. rc:%d\n", +            ret);

drivers/hwmon/pmbus/ucd9000.c +79 -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 int ucd9000_debugfs_show_mfr_status_word2(void *data, u64 *val)
+{
+    struct i2c_client *client = data;
+    __be16 buffer;
+    int ret;
+
+    ret = ucd9000_get_mfr_status(client, (u8 *)&buffer);
+    if (ret < 0) {
+        dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+            ret);
+
+        return ret;
+    }
+
+    *val = be16_to_cpu(buffer);
+
+    return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_word2,
+             ucd9000_debugfs_show_mfr_status_word2, NULL,
+             "%04llx\n");
+
+static int ucd9000_debugfs_show_mfr_status_word4(void *data, u64 *val)
+{
+    struct i2c_client *client = data;
+    __be32 buffer;
+    int ret;
+
+    ret = ucd9000_get_mfr_status(client, (u8 *)&buffer);
+    if (ret < 0) {
+        dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+            ret);
+
+        return ret;
+    }
+
+    *val = be32_to_cpu(buffer);
+
+    return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_word4,
+             ucd9000_debugfs_show_mfr_status_word4, NULL,
+             "%08llx\n");
+
+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;

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

+
+        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,

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 483

Quoted 10 lines

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

+                    &ucd9000_debugfs_mfr_status_word4);
+    } else if (mid->driver_data == ucd90120 ||
+           mid->driver_data == ucd90124) {
+        scnprintf(name, UCD9000_DEBUGFS_NAME_LEN, "mfr_status");
+        debugfs_create_file(name, 0444, data->debugfs, client,
+                    &ucd9000_debugfs_mfr_status_word2);
+    }
+
+    return 0;
+}

Quoted 7 lines

Is all that complexity really worth it ? Why not just read the manufacturing
status as byte string into a buffer and use hexdump to pront it, no matter how
many bytes are actually returned ? This would also be less error prone, and
automatically support future chips.

Hm, well then we have the additional complexity of setting up custom debugfs file operations to show the binary data instead of just using DEFINE_DEBUGFS_ATTRIBUTE. Plus, at some point someone has to interpret it as either a word, half-word, or 5 bytes chunk. Where better to do it than the driver, as this is hw-dependent?

I could just use one function and do a byte-swap based on data length in a loop within #ifdef LITTLE_ENDIAN, but that's a little messy. It will handle all the cases though. What do you think?

Thanks,
Eddie

Quoted 33 lines

Thanks,
Guenter

Quoted 28 lines

drivers/hwmon/pmbus/ucd9000.c +8 -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) {

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

@@ -483,7 +644,16 @@ static int ucd9000_probe(struct i2c_client *client,
           return ret;
       }
   -    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 */
 

Quoted 218 lines · Eddie James

Quoted 208 lines · Guenter Roeck

Quoted 207 lines · Eddie James

From: Christopher Bostic <[email protected]>

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 <[email protected]>

Signed-off-by Andrew Jeffery <[email protected]>

Signed-off-by Eddie James <[email protected]>

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

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

index e3a507f..297da0e 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/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;

@@ -328,6 +339,156 @@ static int ucd9000_gpio_direction_output(struct
 gpio_chip *gc,
                         val);
   }
   +#if IS_ENABLED(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);

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

+}
+
+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) {
+        dev_err(&client->dev, "Failed to read mfr status. rc:%d\n", +            ret);

drivers/hwmon/pmbus/ucd9000.c +79 -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 int ucd9000_debugfs_show_mfr_status_word2(void *data, u64 *val)
+{
+    struct i2c_client *client = data;
+    __be16 buffer;
+    int ret;
+
+    ret = ucd9000_get_mfr_status(client, (u8 *)&buffer);
+    if (ret < 0) {
+        dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+            ret);
+
+        return ret;
+    }
+
+    *val = be16_to_cpu(buffer);
+
+    return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_word2,
+             ucd9000_debugfs_show_mfr_status_word2, NULL,
+             "%04llx\n");
+
+static int ucd9000_debugfs_show_mfr_status_word4(void *data, u64 *val)
+{
+    struct i2c_client *client = data;
+    __be32 buffer;
+    int ret;
+
+    ret = ucd9000_get_mfr_status(client, (u8 *)&buffer);
+    if (ret < 0) {
+        dev_err(&client->dev, "Failed to read mfr status. rc:%d\n",
+            ret);
+
+        return ret;
+    }
+
+    *val = be32_to_cpu(buffer);
+
+    return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_word4,
+             ucd9000_debugfs_show_mfr_status_word4, NULL,
+             "%08llx\n");
+
+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;

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

+
+        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_mfr_status_word4);
+    } else if (mid->driver_data == ucd90120 ||
+           mid->driver_data == ucd90124) {
+        scnprintf(name, UCD9000_DEBUGFS_NAME_LEN, "mfr_status");
+        debugfs_create_file(name, 0444, data->debugfs, client,
+                    &ucd9000_debugfs_mfr_status_word2);
+    }
+
+    return 0;

Quoted 8 lines

Quoted 1 line

+}

Is all that complexity really worth it ? Why not just read the manufacturing
status as byte string into a buffer and use hexdump to pront it, no matter how
many bytes are actually returned ? This would also be less error prone, and
automatically support future chips.

Quoted 6 lines

Hm, well then we have the additional complexity of setting up custom debugfs file operations to show the binary data instead of just using DEFINE_DEBUGFS_ATTRIBUTE. Plus, at some point someone has to interpret it as either a word, half-word, or 5 bytes chunk. Where better to do it than the driver, as this is hw-dependent?

I can't exactly follow your logic. You mean it is acceptable for the user to have to look into the datasheet to find out what the 1/2/4 byte hex value means, but it is unacceptable to expect the user to have to use the datasheet to identify what a 1..5 byte hex string, displayed in the order received from the chip, means ? I am having difficulties understanding the difference. How is 12345678 different from, say, 12 34 56 78 (which you could display as 12345678 as well) ?

The macro generates the file operations as part of the define, so I don't see having to define as valid argument. One could instead add a generic debugfs macro to display a string if that is of interest.

Quoted 4 lines

I could just use one function and do a byte-swap based on data length in a loop within #ifdef LITTLE_ENDIAN, but that's a little messy. It will handle all the cases though. What do you think?

Personally I don't see a problem displaying data as received. Either case, there are functions/macros to convert from big/little endian to host byte order, so related ifdefs in the code should never be necessary.

Guenter

Quoted 34 lines

Thanks,
Eddie

Quoted 31 lines

Thanks,
Guenter

Quoted 28 lines

drivers/hwmon/pmbus/ucd9000.c +18 -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)
   {

@@ -483,7 +644,16 @@ static int ucd9000_probe(struct i2c_client *client,
           return ret;
       }
   -    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 */