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

From: Eddie James
Date: Tue Mar 13 2018 - 15:02:06 EST



On 03/10/2018 10:50 AM, Guenter Roeck wrote:
On 03/09/2018 11:19 AM, Eddie James wrote:
From: Christopher Bostic <cbostic@xxxxxxxxxxxxxxxxxx>

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@xxxxxxxxxxxxxxxxxx>
Signed-off-by: Andrew Jeffery <andrew@xxxxxxxx>
Signed-off-by: Eddie James <eajames@xxxxxxxxxxxxxxxxxx>
---
 drivers/hwmon/pmbus/ucd9000.c | 172 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 171 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index e3a507f..297da0e 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;
@@ -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,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &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;
+}

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


Thanks,
Guenter

+#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 */