Re: [PATCH 2/2] hwmon: (ucd9000) Add debugfs attributes to provide mfr_status
From: Eddie James
Date: Tue Mar 13 2018 - 16:00:27 EST
On 03/13/2018 02:29 PM, Guenter Roeck wrote:
On Tue, Mar 13, 2018 at 02:01:51PM -0500, Eddie James wrote:
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 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) ?
Yea, it's not different at all. Sorry, I wasn't very clear, when I said
"interpret," I meant "figure out the endian swapping," so my proposal
would display 78563412, so the user can directly use the value. I
typically expect the kernel to provide data through interfaces in host
endian, but displaying it as-received is fine as well. User can figure
it out.
V2 incoming.
Thanks,
Eddie
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.
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
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 */