[PATCH 0/4] drivers/hwmon/pmbus: Core extensions for STATUS_WORD support and debugfs

[PATCH 0/4] drivers/hwmon/pmbus: Core extensions for STATUS_WORD support and debugfs

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

This series adds some functionality to the pmbus core.

The first two patches provide support for the STATUS_WORD register. This allows
more default alarm attributes to be used, as the upper byte of the status
register is available. The third patch then uses the STATUS_INPUT bit of the
status register to setup boolean attributes for input voltage and input power
attributes.

The fourth patch provides support for raw reads of pmbus status registers
through the debugfs interface. These can be very useful for hardware
diagnostics, especially on multi-page pmbus devices, as user-space access of
the i2c space could corrupt the pmbus page accounting.

Since RFC series:
 * Just use u16 instead of complicated u8 method for STATUS_WORD.
 * Re-ordered the changes.
 * Added conditional for creating bool attr for higher byte STATUS_WORD bits.

Edward A. James (4):
  drivers/hwmon/pmbus: Switch status registers to 16 bit
  drivers/hwmon/pmbus: Access word data for STATUS_WORD
  drivers/hwmon/pmbus: Add generic alarm bit for iin and pin
  drivers/hwmon/pmbus: Add debugfs for status registers

 drivers/hwmon/pmbus/pmbus.c      |  24 +++-
 drivers/hwmon/pmbus/pmbus.h      |  11 ++
 drivers/hwmon/pmbus/pmbus_core.c | 251 +++++++++++++++++++++++++++++++++++----
 3 files changed, 263 insertions(+), 23 deletions(-)

-- 
1.8.3.1

[PATCH 1/4] drivers/hwmon/pmbus: Switch status registers to 16 bit

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Switch the storage of status registers to 16 bit values. This allows us
to store all the bits of STATUS_WORD.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index f1eff6b..4ec7586 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -112,7 +112,7 @@ struct pmbus_data {
 	 * A single status register covers multiple attributes,
 	 * so we keep them all together.
 	 */
-	u8 status[PB_NUM_STATUS_REG];
+	u16 status[PB_NUM_STATUS_REG];
 	u8 status_register;
 
 	u8 currpage;
@@ -716,10 +716,10 @@ static int pmbus_get_boolean(struct pmbus_data *data, struct pmbus_boolean *b,
 {
 	struct pmbus_sensor *s1 = b->s1;
 	struct pmbus_sensor *s2 = b->s2;
-	u16 reg = (index >> 8) & 0xffff;
-	u8 mask = index & 0xff;
+	u16 reg = (index >> 16) & 0xffff;
+	u16 mask = index & 0xffff;
 	int ret, status;
-	u8 regval;
+	u16 regval;
 
 	status = data->status[reg];
 	if (status < 0)
@@ -860,7 +860,7 @@ static int pmbus_add_boolean(struct pmbus_data *data,
 			     const char *name, const char *type, int seq,
 			     struct pmbus_sensor *s1,
 			     struct pmbus_sensor *s2,
-			     u16 reg, u8 mask)
+			     u16 reg, u16 mask)
 {
 	struct pmbus_boolean *boolean;
 	struct sensor_device_attribute *a;
@@ -876,7 +876,7 @@ static int pmbus_add_boolean(struct pmbus_data *data,
 	boolean->s1 = s1;
 	boolean->s2 = s2;
 	pmbus_attr_init(a, boolean->name, S_IRUGO, pmbus_show_boolean, NULL,
-			(reg << 8) | mask);
+			(reg << 16) | mask);
 
 	return pmbus_add_attribute(data, &a->dev_attr.attr);
 }
@@ -962,7 +962,7 @@ struct pmbus_limit_attr {
  */
 struct pmbus_sensor_attr {
 	u16 reg;			/* sensor register */
-	u8 gbit;			/* generic status bit */
+	u16 gbit;			/* generic status bit */
 	u8 nlimit;			/* # of limit registers */
 	enum pmbus_sensor_classes class;/* sensor class */
 	const char *label;		/* sensor label */
-- 
1.8.3.1

[PATCH 2/4] drivers/hwmon/pmbus: Access word data for STATUS_WORD

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Pmbus always reads byte data from the status register, even if
configured to use STATUS_WORD. Use a function pointer to read the
correct amount of data from the registers.
Also switch to try STATUS_WORD first before STATUS_BYTE on init.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 54 +++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 4ec7586..277d170 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -113,7 +113,8 @@ struct pmbus_data {
 	 * so we keep them all together.
 	 */
 	u16 status[PB_NUM_STATUS_REG];
-	u8 status_register;
+
+	int (*read_status)(struct i2c_client *client, int page);
 
 	u8 currpage;
 };
@@ -324,7 +325,7 @@ static int pmbus_check_status_cml(struct i2c_client *client)
 	struct pmbus_data *data = i2c_get_clientdata(client);
 	int status, status2;
 
-	status = _pmbus_read_byte_data(client, -1, data->status_register);
+	status = data->read_status(client, -1);
 	if (status < 0 || (status & PB_STATUS_CML)) {
 		status2 = _pmbus_read_byte_data(client, -1, PMBUS_STATUS_CML);
 		if (status2 < 0 || (status2 & PB_CML_FAULT_INVALID_COMMAND))
@@ -348,6 +349,23 @@ static bool pmbus_check_register(struct i2c_client *client,
 	return rv >= 0;
 }
 
+static bool pmbus_check_status_register(struct i2c_client *client, int page)
+{
+	int status;
+	struct pmbus_data *data = i2c_get_clientdata(client);
+
+	status = data->read_status(client, page);
+	if (status >= 0 && !(data->flags & PMBUS_SKIP_STATUS_CHECK) &&
+	    (status & PB_STATUS_CML)) {
+		status = _pmbus_read_byte_data(client, -1, PMBUS_STATUS_CML);
+		if (status < 0 || (status & PB_CML_FAULT_INVALID_COMMAND))
+			status = -EIO;
+	}
+
+	pmbus_clear_fault_page(client, -1);
+	return status >= 0;
+}
+
 bool pmbus_check_byte_register(struct i2c_client *client, int page, int reg)
 {
 	return pmbus_check_register(client, _pmbus_read_byte_data, page, reg);
@@ -394,8 +412,7 @@ static struct pmbus_data *pmbus_update_device(struct device *dev)
 
 		for (i = 0; i < info->pages; i++) {
 			data->status[PB_STATUS_BASE + i]
-			    = _pmbus_read_byte_data(client, i,
-						    data->status_register);
+			    = data->read_status(client, i);
 			for (j = 0; j < ARRAY_SIZE(pmbus_status); j++) {
 				struct _pmbus_status *s = &pmbus_status[j];
 
@@ -1051,8 +1068,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
 		 * the generic status register for this page is accessible.
 		 */
 		if (!ret && attr->gbit &&
-		    pmbus_check_byte_register(client, page,
-					      data->status_register)) {
+		    pmbus_check_status_register(client, page)) {
 			ret = pmbus_add_boolean(data, name, "alarm", index,
 						NULL, NULL,
 						PB_STATUS_BASE + page,
@@ -1729,6 +1745,16 @@ static int pmbus_identify_common(struct i2c_client *client,
 	return 0;
 }
 
+static int pmbus_read_status_byte(struct i2c_client *client, int page)
+{
+	return _pmbus_read_byte_data(client, page, PMBUS_STATUS_BYTE);
+}
+
+static int pmbus_read_status_word(struct i2c_client *client, int page)
+{
+	return _pmbus_read_word_data(client, page, PMBUS_STATUS_WORD);
+}
+
 static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 			     struct pmbus_driver_info *info)
 {
@@ -1736,16 +1762,16 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 	int page, ret;
 
 	/*
-	 * Some PMBus chips don't support PMBUS_STATUS_BYTE, so try
-	 * to use PMBUS_STATUS_WORD instead if that is the case.
+	 * Some PMBus chips don't support PMBUS_STATUS_WORD, so try
+	 * to use PMBUS_STATUS_BYTE instead if that is the case.
 	 * Bail out if both registers are not supported.
 	 */
-	data->status_register = PMBUS_STATUS_BYTE;
-	ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
-	if (ret < 0 || ret == 0xff) {
-		data->status_register = PMBUS_STATUS_WORD;
-		ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
-		if (ret < 0 || ret == 0xffff) {
+	data->read_status = pmbus_read_status_word;
+	ret = i2c_smbus_read_word_data(client, PMBUS_STATUS_WORD);
+	if (ret < 0 || ret == 0xffff) {
+		data->read_status = pmbus_read_status_byte;
+		ret = i2c_smbus_read_byte_data(client, PMBUS_STATUS_BYTE);
+		if (ret < 0 || ret == 0xff) {
 			dev_err(dev, "PMBus status register not found\n");
 			return -ENODEV;
 		}
-- 
1.8.3.1

[PATCH 3/4] drivers/hwmon/pmbus: Add generic alarm bit for iin and pin

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Add PB_STATUS_INPUT as the generic alarm bit for iin and pin. We also
need to redo the status register checking before setting up the boolean
attribute, since it won't necessarily check STATUS_WORD if the device
doesn't support it, which we need for this bit.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/pmbus/pmbus_core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 277d170..1a51f8f 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -1045,6 +1045,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
 				      const struct pmbus_sensor_attr *attr)
 {
 	struct pmbus_sensor *base;
+	bool upper = !!(attr->gbit & 0xff00);	/* need to check STATUS_WORD */
 	int ret;
 
 	if (attr->label) {
@@ -1065,10 +1066,13 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
 		/*
 		 * Add generic alarm attribute only if there are no individual
 		 * alarm attributes, if there is a global alarm bit, and if
-		 * the generic status register for this page is accessible.
+		 * the generic status register (word or byte, depending on
+		 * which global bit is set) for this page is accessible.
 		 */
 		if (!ret && attr->gbit &&
-		    pmbus_check_status_register(client, page)) {
+		    ((upper && pmbus_check_word_register(client, page,
+							 PMBUS_STATUS_WORD)) ||
+		     (!upper && pmbus_check_status_register(client, page)))) {
 			ret = pmbus_add_boolean(data, name, "alarm", index,
 						NULL, NULL,
 						PB_STATUS_BASE + page,
@@ -1324,6 +1328,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
 		.func = PMBUS_HAVE_IIN,
 		.sfunc = PMBUS_HAVE_STATUS_INPUT,
 		.sbase = PB_STATUS_INPUT_BASE,
+		.gbit = PB_STATUS_INPUT,
 		.limit = iin_limit_attrs,
 		.nlimit = ARRAY_SIZE(iin_limit_attrs),
 	}, {
@@ -1408,6 +1413,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
 		.func = PMBUS_HAVE_PIN,
 		.sfunc = PMBUS_HAVE_STATUS_INPUT,
 		.sbase = PB_STATUS_INPUT_BASE,
+		.gbit = PB_STATUS_INPUT,
 		.limit = pin_limit_attrs,
 		.nlimit = ARRAY_SIZE(pin_limit_attrs),
 	}, {
-- 
1.8.3.1

[PATCH 4/4] drivers/hwmon/pmbus: Add debugfs for status registers

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Export all the available status registers through debugfs, if the client
driver wants them.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/pmbus/pmbus.c      |  24 +++++-
 drivers/hwmon/pmbus/pmbus.h      |  11 +++
 drivers/hwmon/pmbus/pmbus_core.c | 175 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 209 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
index 7718e58..fc27417 100644
--- a/drivers/hwmon/pmbus/pmbus.c
+++ b/drivers/hwmon/pmbus/pmbus.c
@@ -18,6 +18,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>
@@ -230,7 +231,28 @@ static int pmbus_probe(struct i2c_client *client,
 	.id_table = pmbus_id,
 };
 
-module_i2c_driver(pmbus_driver);
+struct dentry *pmbus_debugfs_dir;
+EXPORT_SYMBOL_GPL(pmbus_debugfs_dir);
+
+static int __init pmbus_init(void)
+{
+	pmbus_debugfs_dir = debugfs_create_dir(pmbus_driver.driver.name, NULL);
+	if (IS_ERR(pmbus_debugfs_dir))
+		/* Don't fail out if we don't have debugfs support. */
+		pmbus_debugfs_dir = NULL;
+
+	return i2c_add_driver(&pmbus_driver);
+}
+
+static void __exit pmbus_exit(void)
+{
+	debugfs_remove_recursive(pmbus_debugfs_dir);
+
+	i2c_del_driver(&pmbus_driver);
+}
+
+module_init(pmbus_init);
+module_exit(pmbus_exit);
 
 MODULE_AUTHOR("Guenter Roeck");
 MODULE_DESCRIPTION("Generic PMBus driver");
diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13b..c772b83 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -25,6 +25,8 @@
 #include <linux/bitops.h>
 #include <linux/regulator/driver.h>
 
+struct dentry;
+
 /*
  * Registers
  */
@@ -383,6 +385,12 @@ struct pmbus_driver_info {
 	/* Regulator functionality, if supported by this chip driver. */
 	int num_regulators;
 	const struct regulator_desc *reg_desc;
+
+	/*
+	 * Controls whether or not to create debugfs entries for this driver's
+	 * devices.
+	 */
+	bool debugfs;
 };
 
 /* Regulator ops */
@@ -401,6 +409,9 @@ struct pmbus_driver_info {
 		.owner = THIS_MODULE,				\
 	}
 
+/* Handle for debugfs directory */
+extern struct dentry *pmbus_debugfs_dir;
+
 /* Function declarations */
 
 void pmbus_clear_cache(struct i2c_client *client);
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 1a51f8f..afbd364 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.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>
@@ -49,6 +50,9 @@
 #define PB_STATUS_FAN34_BASE	(PB_STATUS_FAN_BASE + PMBUS_PAGES)
 #define PB_STATUS_TEMP_BASE	(PB_STATUS_FAN34_BASE + PMBUS_PAGES)
 #define PB_STATUS_INPUT_BASE	(PB_STATUS_TEMP_BASE + PMBUS_PAGES)
+#define PB_STATUS_CML_BASE	(PB_STATUS_INPUT_BASE + PMBUS_PAGES)
+#define PB_STATUS_OTHER_BASE	(PB_STATUS_CML_BASE + PMBUS_PAGES)
+#define PB_STATUS_MFR_BASE	(PB_STATUS_OTHER_BASE + PMBUS_PAGES)
 #define PB_STATUS_VMON_BASE	(PB_STATUS_INPUT_BASE + 1)
 
 #define PB_NUM_STATUS_REG	(PB_STATUS_VMON_BASE + 1)
@@ -101,6 +105,7 @@ struct pmbus_data {
 	int num_attributes;
 	struct attribute_group group;
 	const struct attribute_group *groups[2];
+	struct dentry *debugfs;		/* debugfs device directory */
 
 	struct pmbus_sensor *sensors;
 
@@ -119,6 +124,11 @@ struct pmbus_data {
 	u8 currpage;
 };
 
+struct pmbus_debugfs_entry {
+	struct device *dev;
+	int index;
+};
+
 void pmbus_clear_cache(struct i2c_client *client)
 {
 	struct pmbus_data *data = i2c_get_clientdata(client);
@@ -422,6 +432,19 @@ static struct pmbus_data *pmbus_update_device(struct device *dev)
 					= _pmbus_read_byte_data(client, i,
 								s->reg);
 			}
+
+			if (!data->debugfs)
+				continue;
+
+			data->status[PB_STATUS_CML_BASE + i]
+				= _pmbus_read_byte_data(client, i,
+							PMBUS_STATUS_CML);
+			data->status[PB_STATUS_OTHER_BASE + i]
+				= _pmbus_read_byte_data(client, i,
+							PMBUS_STATUS_OTHER);
+			data->status[PB_STATUS_MFR_BASE + i]
+			    = _pmbus_read_byte_data(client, i,
+						    PMBUS_STATUS_MFR_SPECIFIC);
 		}
 
 		if (info->func[0] & PMBUS_HAVE_STATUS_INPUT)
@@ -1891,6 +1914,151 @@ static int pmbus_regulator_register(struct pmbus_data *data)
 }
 #endif
 
+static int pmbus_debugfs_get(void *data, u64 *val)
+{
+	struct pmbus_debugfs_entry *entry = data;
+	struct pmbus_data *pdata = pmbus_update_device(entry->dev);
+
+	*val = pdata->status[entry->index];
+
+	return 0;
+}
+
+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops8, pmbus_debugfs_get, NULL,
+			 "0x%02llx\n");
+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops16, pmbus_debugfs_get, NULL,
+			 "0x%04llx\n");
+
+static int pmbus_init_debugfs(struct i2c_client *client,
+			      struct pmbus_data *data)
+{
+	int i, idx = 0;
+	struct dentry *dbg;
+	char name[PMBUS_NAME_SIZE];
+	struct pmbus_debugfs_entry *entries;
+
+	/*
+	 * Either user didn't request debugfs or debugfs is not enabled in
+	 * kernel. Exit but don't throw an error in these cases.
+	 */
+	if (!data->info->debugfs || !pmbus_debugfs_dir)
+		return 0;
+
+	/*
+	 * Create the debugfs directory for this device. Use the hwmon device
+	 * name to avoid conflicts (hwmon numbers are globally unique).
+	 */
+	data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
+					   pmbus_debugfs_dir);
+	if (IS_ERR_OR_NULL(data->debugfs)) {
+		data->debugfs = NULL;
+		return -ENODEV;
+	}
+
+	/* Allocate the max possible entries we need. */
+	entries = devm_kzalloc(data->dev,
+			       sizeof(*entries) * (data->info->pages * 10),
+			       GFP_KERNEL);
+	if (!entries)
+		return -ENOMEM;
+
+	for (i = 0; i < data->info->pages; ++i) {
+		/* check accessibility of status register if it's not page 0 */
+		if (!i || pmbus_check_status_register(client, i)) {
+			entries[idx].dev = data->hwmon_dev;
+			entries[idx].index = PB_STATUS_BASE + i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
+			dbg = debugfs_create_file(name, 0444, data->debugfs,
+						  &entries[idx++],
+						  &pmbus_debugfs_ops16);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
+			entries[idx].dev = data->hwmon_dev;
+			entries[idx].index = PB_STATUS_VOUT_BASE + i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
+			dbg = debugfs_create_file(name, 0444, data->debugfs,
+						  &entries[idx++],
+						  &pmbus_debugfs_ops8);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
+			entries[idx].dev = data->hwmon_dev;
+			entries[idx].index = PB_STATUS_IOUT_BASE + i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
+			dbg = debugfs_create_file(name, 0444, data->debugfs,
+						  &entries[idx++],
+						  &pmbus_debugfs_ops8);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
+			entries[idx].dev = data->hwmon_dev;
+			entries[idx].index = PB_STATUS_INPUT_BASE + i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
+			dbg = debugfs_create_file(name, 0444, data->debugfs,
+						  &entries[idx++],
+						  &pmbus_debugfs_ops8);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
+			entries[idx].dev = data->hwmon_dev;
+			entries[idx].index = PB_STATUS_TEMP_BASE + i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
+			dbg = debugfs_create_file(name, 0444, data->debugfs,
+						  &entries[idx++],
+						  &pmbus_debugfs_ops8);
+		}
+
+		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
+			entries[idx].dev = data->hwmon_dev;
+			entries[idx].index = PB_STATUS_CML_BASE + i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
+			dbg = debugfs_create_file(name, 0444, data->debugfs,
+						  &entries[idx++],
+						  &pmbus_debugfs_ops8);
+		}
+
+		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
+			entries[idx].dev = data->hwmon_dev;
+			entries[idx].index = PB_STATUS_OTHER_BASE + i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
+			dbg = debugfs_create_file(name, 0444, data->debugfs,
+						  &entries[idx++],
+						  &pmbus_debugfs_ops8);
+		}
+
+		if (pmbus_check_byte_register(client, i,
+					      PMBUS_STATUS_MFR_SPECIFIC)) {
+			entries[idx].dev = data->hwmon_dev;
+			entries[idx].index = PB_STATUS_MFR_BASE + i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
+			dbg = debugfs_create_file(name, 0444, data->debugfs,
+						  &entries[idx++],
+						  &pmbus_debugfs_ops8);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
+			entries[idx].dev = data->hwmon_dev;
+			entries[idx].index = PB_STATUS_FAN_BASE + i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
+			dbg = debugfs_create_file(name, 0444, data->debugfs,
+						  &entries[idx++],
+						  &pmbus_debugfs_ops8);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
+			entries[idx].dev = data->hwmon_dev;
+			entries[idx].index = PB_STATUS_FAN34_BASE + i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
+			dbg = debugfs_create_file(name, 0444, data->debugfs,
+						  &entries[idx++],
+						  &pmbus_debugfs_ops8);
+		}
+	}
+
+	return 0;
+}
+
 int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		   struct pmbus_driver_info *info)
 {
@@ -1950,6 +2118,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 	if (ret)
 		goto out_unregister;
 
+	ret = pmbus_init_debugfs(client, data);
+	if (ret)
+		dev_warn(dev, "Failed to register debugfs\n");
+
 	return 0;
 
 out_unregister:
@@ -1963,6 +2135,9 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 int pmbus_do_remove(struct i2c_client *client)
 {
 	struct pmbus_data *data = i2c_get_clientdata(client);
+
+	debugfs_remove_recursive(data->debugfs);
+
 	hwmon_device_unregister(data->hwmon_dev);
 	kfree(data->group.attrs);
 	return 0;
-- 
1.8.3.1

Re: [PATCH 4/4] drivers/hwmon/pmbus: Add debugfs for status registers

[Guenter Roeck]

On Wed, Aug 09, 2017 at 05:19:17PM -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Export all the available status registers through debugfs, if the client
> driver wants them.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/hwmon/pmbus/pmbus.c      |  24 +++++-
>  drivers/hwmon/pmbus/pmbus.h      |  11 +++
>  drivers/hwmon/pmbus/pmbus_core.c | 175 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 209 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
> index 7718e58..fc27417 100644
> --- a/drivers/hwmon/pmbus/pmbus.c
> +++ b/drivers/hwmon/pmbus/pmbus.c

That won't work: SENSORS_PMBUS is independent of PMBUS and does not have
to be enabled even if PMBUS is enabled. This will have to be in pmbus_core.c.

I would suggest to do the same as done with other drivers. Move
pmbus_debugfs_dir into pmbus_core.c and create the directory on probe
if it has not already been created.

> @@ -18,6 +18,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>
> @@ -230,7 +231,28 @@ static int pmbus_probe(struct i2c_client *client,
>  	.id_table = pmbus_id,
>  };
>  
> -module_i2c_driver(pmbus_driver);
> +struct dentry *pmbus_debugfs_dir;
> +EXPORT_SYMBOL_GPL(pmbus_debugfs_dir);
> +
> +static int __init pmbus_init(void)
> +{
> +	pmbus_debugfs_dir = debugfs_create_dir(pmbus_driver.driver.name, NULL);
> +	if (IS_ERR(pmbus_debugfs_dir))
> +		/* Don't fail out if we don't have debugfs support. */
> +		pmbus_debugfs_dir = NULL;
> +
> +	return i2c_add_driver(&pmbus_driver);
> +}
> +
> +static void __exit pmbus_exit(void)
> +{
> +	debugfs_remove_recursive(pmbus_debugfs_dir);
> +
> +	i2c_del_driver(&pmbus_driver);
> +}
> +
> +module_init(pmbus_init);
> +module_exit(pmbus_exit);
>  
>  MODULE_AUTHOR("Guenter Roeck");
>  MODULE_DESCRIPTION("Generic PMBus driver");
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index bfcb13b..c772b83 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -25,6 +25,8 @@
>  #include <linux/bitops.h>
>  #include <linux/regulator/driver.h>
>  
> +struct dentry;
> +
>  /*
>   * Registers
>   */
> @@ -383,6 +385,12 @@ struct pmbus_driver_info {
>  	/* Regulator functionality, if supported by this chip driver. */
>  	int num_regulators;
>  	const struct regulator_desc *reg_desc;
> +
> +	/*
> +	 * Controls whether or not to create debugfs entries for this driver's
> +	 * devices.
> +	 */
> +	bool debugfs;
>  };
>  
>  /* Regulator ops */
> @@ -401,6 +409,9 @@ struct pmbus_driver_info {
>  		.owner = THIS_MODULE,				\
>  	}
>  
> +/* Handle for debugfs directory */
> +extern struct dentry *pmbus_debugfs_dir;
> +
>  /* Function declarations */
>  
>  void pmbus_clear_cache(struct i2c_client *client);
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 1a51f8f..afbd364 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.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>
> @@ -49,6 +50,9 @@
>  #define PB_STATUS_FAN34_BASE	(PB_STATUS_FAN_BASE + PMBUS_PAGES)
>  #define PB_STATUS_TEMP_BASE	(PB_STATUS_FAN34_BASE + PMBUS_PAGES)
>  #define PB_STATUS_INPUT_BASE	(PB_STATUS_TEMP_BASE + PMBUS_PAGES)
> +#define PB_STATUS_CML_BASE	(PB_STATUS_INPUT_BASE + PMBUS_PAGES)
> +#define PB_STATUS_OTHER_BASE	(PB_STATUS_CML_BASE + PMBUS_PAGES)
> +#define PB_STATUS_MFR_BASE	(PB_STATUS_OTHER_BASE + PMBUS_PAGES)

We are not actively using those status registers, are we ?
I am a bit concerned that we'll continuously read those status registers
but don't do anything with it most of the time. Ultimately I'd prefer
to get rid of all caching, not more, since it gets quite expensive
on chips with many pages.

Maybe just display uncached status register values in debugfs ?

>  #define PB_STATUS_VMON_BASE	(PB_STATUS_INPUT_BASE + 1)
>  
>  #define PB_NUM_STATUS_REG	(PB_STATUS_VMON_BASE + 1)
> @@ -101,6 +105,7 @@ struct pmbus_data {
>  	int num_attributes;
>  	struct attribute_group group;
>  	const struct attribute_group *groups[2];
> +	struct dentry *debugfs;		/* debugfs device directory */
>  
>  	struct pmbus_sensor *sensors;
>  
> @@ -119,6 +124,11 @@ struct pmbus_data {
>  	u8 currpage;
>  };
>  
> +struct pmbus_debugfs_entry {
> +	struct device *dev;
> +	int index;
> +};
> +
>  void pmbus_clear_cache(struct i2c_client *client)
>  {
>  	struct pmbus_data *data = i2c_get_clientdata(client);
> @@ -422,6 +432,19 @@ static struct pmbus_data *pmbus_update_device(struct device *dev)
>  					= _pmbus_read_byte_data(client, i,
>  								s->reg);
>  			}
> +
> +			if (!data->debugfs)
> +				continue;
> +
> +			data->status[PB_STATUS_CML_BASE + i]
> +				= _pmbus_read_byte_data(client, i,
> +							PMBUS_STATUS_CML);
> +			data->status[PB_STATUS_OTHER_BASE + i]
> +				= _pmbus_read_byte_data(client, i,
> +							PMBUS_STATUS_OTHER);
> +			data->status[PB_STATUS_MFR_BASE + i]
> +			    = _pmbus_read_byte_data(client, i,
> +						    PMBUS_STATUS_MFR_SPECIFIC);
>  		}
>  
>  		if (info->func[0] & PMBUS_HAVE_STATUS_INPUT)
> @@ -1891,6 +1914,151 @@ static int pmbus_regulator_register(struct pmbus_data *data)
>  }
>  #endif
>  
> +static int pmbus_debugfs_get(void *data, u64 *val)
> +{
> +	struct pmbus_debugfs_entry *entry = data;
> +	struct pmbus_data *pdata = pmbus_update_device(entry->dev);
> +
> +	*val = pdata->status[entry->index];
> +
> +	return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops8, pmbus_debugfs_get, NULL,
> +			 "0x%02llx\n");
> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops16, pmbus_debugfs_get, NULL,
> +			 "0x%04llx\n");
> +
> +static int pmbus_init_debugfs(struct i2c_client *client,
> +			      struct pmbus_data *data)
> +{
> +	int i, idx = 0;
> +	struct dentry *dbg;
> +	char name[PMBUS_NAME_SIZE];
> +	struct pmbus_debugfs_entry *entries;
> +
> +	/*
> +	 * Either user didn't request debugfs or debugfs is not enabled in
> +	 * kernel. Exit but don't throw an error in these cases.
> +	 */

Here might be the place to initialize pmbus_debugfs_dir
if it is not yet initialized.

> +	if (!data->info->debugfs || !pmbus_debugfs_dir)
> +		return 0;
> +
> +	/*
> +	 * Create the debugfs directory for this device. Use the hwmon device
> +	 * name to avoid conflicts (hwmon numbers are globally unique).
> +	 */
> +	data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
> +					   pmbus_debugfs_dir);
> +	if (IS_ERR_OR_NULL(data->debugfs)) {
> +		data->debugfs = NULL;
> +		return -ENODEV;
> +	}
> +
> +	/* Allocate the max possible entries we need. */
> +	entries = devm_kzalloc(data->dev,
> +			       sizeof(*entries) * (data->info->pages * 10),
> +			       GFP_KERNEL);
> +	if (!entries)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < data->info->pages; ++i) {
> +		/* check accessibility of status register if it's not page 0 */
> +		if (!i || pmbus_check_status_register(client, i)) {
> +			entries[idx].dev = data->hwmon_dev;
> +			entries[idx].index = PB_STATUS_BASE + i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
> +						  &entries[idx++],
> +						  &pmbus_debugfs_ops16);

What is the point of the dbg variable ?

> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
> +			entries[idx].dev = data->hwmon_dev;
> +			entries[idx].index = PB_STATUS_VOUT_BASE + i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
> +						  &entries[idx++],
> +						  &pmbus_debugfs_ops8);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
> +			entries[idx].dev = data->hwmon_dev;
> +			entries[idx].index = PB_STATUS_IOUT_BASE + i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
> +						  &entries[idx++],
> +						  &pmbus_debugfs_ops8);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
> +			entries[idx].dev = data->hwmon_dev;
> +			entries[idx].index = PB_STATUS_INPUT_BASE + i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
> +						  &entries[idx++],
> +						  &pmbus_debugfs_ops8);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
> +			entries[idx].dev = data->hwmon_dev;
> +			entries[idx].index = PB_STATUS_TEMP_BASE + i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
> +						  &entries[idx++],
> +						  &pmbus_debugfs_ops8);
> +		}
> +
> +		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
> +			entries[idx].dev = data->hwmon_dev;
> +			entries[idx].index = PB_STATUS_CML_BASE + i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
> +						  &entries[idx++],
> +						  &pmbus_debugfs_ops8);
> +		}
> +
> +		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
> +			entries[idx].dev = data->hwmon_dev;
> +			entries[idx].index = PB_STATUS_OTHER_BASE + i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
> +						  &entries[idx++],
> +						  &pmbus_debugfs_ops8);
> +		}
> +
> +		if (pmbus_check_byte_register(client, i,
> +					      PMBUS_STATUS_MFR_SPECIFIC)) {
> +			entries[idx].dev = data->hwmon_dev;
> +			entries[idx].index = PB_STATUS_MFR_BASE + i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
> +						  &entries[idx++],
> +						  &pmbus_debugfs_ops8);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
> +			entries[idx].dev = data->hwmon_dev;
> +			entries[idx].index = PB_STATUS_FAN_BASE + i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
> +						  &entries[idx++],
> +						  &pmbus_debugfs_ops8);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
> +			entries[idx].dev = data->hwmon_dev;
> +			entries[idx].index = PB_STATUS_FAN34_BASE + i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
> +						  &entries[idx++],
> +						  &pmbus_debugfs_ops8);
> +		}
> +	}
> +
> +	return 0;
> +}
> +

Typically debugfs code is conditional. What happens if DEBUGFS
is not enabled ? See below.

>  int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>  		   struct pmbus_driver_info *info)
>  {
> @@ -1950,6 +2118,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>  	if (ret)
>  		goto out_unregister;
>  
> +	ret = pmbus_init_debugfs(client, data);
> +	if (ret)
> +		dev_warn(dev, "Failed to register debugfs\n");

I think this warning will be generated if debugfs is disabled.
We should not do that. Consider putting the debugfs code into
#fidef and have a dummy pmbus_init_debugfs() returning 0 if
it is disabled.

> +
>  	return 0;
>  
>  out_unregister:
> @@ -1963,6 +2135,9 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>  int pmbus_do_remove(struct i2c_client *client)
>  {
>  	struct pmbus_data *data = i2c_get_clientdata(client);
> +
> +	debugfs_remove_recursive(data->debugfs);
> +
>  	hwmon_device_unregister(data->hwmon_dev);
>  	kfree(data->group.attrs);
>  	return 0;
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] drivers/hwmon/pmbus: Add debugfs for status registers

[Eddie James]

On 08/09/2017 08:15 PM, Guenter Roeck wrote:
> On Wed, Aug 09, 2017 at 05:19:17PM -0500, Eddie James wrote:
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> Export all the available status registers through debugfs, if the client
>> driver wants them.
>>
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> ---
>>   drivers/hwmon/pmbus/pmbus.c      |  24 +++++-
>>   drivers/hwmon/pmbus/pmbus.h      |  11 +++
>>   drivers/hwmon/pmbus/pmbus_core.c | 175 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 209 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
>> index 7718e58..fc27417 100644
>> --- a/drivers/hwmon/pmbus/pmbus.c
>> +++ b/drivers/hwmon/pmbus/pmbus.c
> That won't work: SENSORS_PMBUS is independent of PMBUS and does not have
> to be enabled even if PMBUS is enabled. This will have to be in pmbus_core.c.
>
> I would suggest to do the same as done with other drivers. Move
> pmbus_debugfs_dir into pmbus_core.c and create the directory on probe
> if it has not already been created.

Hmm, I'm having trouble with this. Don't I have to call 
debugfs_recursive_remove() on the pmbus_debugfs_dir when the pmbus 
driver is unloaded? That can't be done from pmbus_core.c, as that is 
just device probe/remove. I agree it makes sense to only create it if a 
device is going to be using it (and is enabled in the kernel) but I 
think I need to declare it here so I can remove it on unload.

>
>> @@ -18,6 +18,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>
>> @@ -230,7 +231,28 @@ static int pmbus_probe(struct i2c_client *client,
>>   	.id_table = pmbus_id,
>>   };
>>   
>> -module_i2c_driver(pmbus_driver);
>> +struct dentry *pmbus_debugfs_dir;
>> +EXPORT_SYMBOL_GPL(pmbus_debugfs_dir);
>> +
>> +static int __init pmbus_init(void)
>> +{
>> +	pmbus_debugfs_dir = debugfs_create_dir(pmbus_driver.driver.name, NULL);
>> +	if (IS_ERR(pmbus_debugfs_dir))
>> +		/* Don't fail out if we don't have debugfs support. */
>> +		pmbus_debugfs_dir = NULL;
>> +
>> +	return i2c_add_driver(&pmbus_driver);
>> +}
>> +
>> +static void __exit pmbus_exit(void)
>> +{
>> +	debugfs_remove_recursive(pmbus_debugfs_dir);
>> +
>> +	i2c_del_driver(&pmbus_driver);
>> +}
>> +
>> +module_init(pmbus_init);
>> +module_exit(pmbus_exit);
>>   
>>   MODULE_AUTHOR("Guenter Roeck");
>>   MODULE_DESCRIPTION("Generic PMBus driver");
>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
>> index bfcb13b..c772b83 100644
>> --- a/drivers/hwmon/pmbus/pmbus.h
>> +++ b/drivers/hwmon/pmbus/pmbus.h
>> @@ -25,6 +25,8 @@
>>   #include <linux/bitops.h>
>>   #include <linux/regulator/driver.h>
>>   
>> +struct dentry;
>> +
>>   /*
>>    * Registers
>>    */
>> @@ -383,6 +385,12 @@ struct pmbus_driver_info {
>>   	/* Regulator functionality, if supported by this chip driver. */
>>   	int num_regulators;
>>   	const struct regulator_desc *reg_desc;
>> +
>> +	/*
>> +	 * Controls whether or not to create debugfs entries for this driver's
>> +	 * devices.
>> +	 */
>> +	bool debugfs;
>>   };
>>   
>>   /* Regulator ops */
>> @@ -401,6 +409,9 @@ struct pmbus_driver_info {
>>   		.owner = THIS_MODULE,				\
>>   	}
>>   
>> +/* Handle for debugfs directory */
>> +extern struct dentry *pmbus_debugfs_dir;
>> +
>>   /* Function declarations */
>>   
>>   void pmbus_clear_cache(struct i2c_client *client);
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
>> index 1a51f8f..afbd364 100644
>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>> +++ b/drivers/hwmon/pmbus/pmbus_core.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>
>> @@ -49,6 +50,9 @@
>>   #define PB_STATUS_FAN34_BASE	(PB_STATUS_FAN_BASE + PMBUS_PAGES)
>>   #define PB_STATUS_TEMP_BASE	(PB_STATUS_FAN34_BASE + PMBUS_PAGES)
>>   #define PB_STATUS_INPUT_BASE	(PB_STATUS_TEMP_BASE + PMBUS_PAGES)
>> +#define PB_STATUS_CML_BASE	(PB_STATUS_INPUT_BASE + PMBUS_PAGES)
>> +#define PB_STATUS_OTHER_BASE	(PB_STATUS_CML_BASE + PMBUS_PAGES)
>> +#define PB_STATUS_MFR_BASE	(PB_STATUS_OTHER_BASE + PMBUS_PAGES)
> We are not actively using those status registers, are we ?
> I am a bit concerned that we'll continuously read those status registers
> but don't do anything with it most of the time. Ultimately I'd prefer
> to get rid of all caching, not more, since it gets quite expensive
> on chips with many pages.
>
> Maybe just display uncached status register values in debugfs ?

Sounds good.

>
>>   #define PB_STATUS_VMON_BASE	(PB_STATUS_INPUT_BASE + 1)
>>   
>>   #define PB_NUM_STATUS_REG	(PB_STATUS_VMON_BASE + 1)
>> @@ -101,6 +105,7 @@ struct pmbus_data {
>>   	int num_attributes;
>>   	struct attribute_group group;
>>   	const struct attribute_group *groups[2];
>> +	struct dentry *debugfs;		/* debugfs device directory */
>>   
>>   	struct pmbus_sensor *sensors;
>>   
>> @@ -119,6 +124,11 @@ struct pmbus_data {
>>   	u8 currpage;
>>   };
>>   
>> +struct pmbus_debugfs_entry {
>> +	struct device *dev;
>> +	int index;
>> +};
>> +
>>   void pmbus_clear_cache(struct i2c_client *client)
>>   {
>>   	struct pmbus_data *data = i2c_get_clientdata(client);
>> @@ -422,6 +432,19 @@ static struct pmbus_data *pmbus_update_device(struct device *dev)
>>   					= _pmbus_read_byte_data(client, i,
>>   								s->reg);
>>   			}
>> +
>> +			if (!data->debugfs)
>> +				continue;
>> +
>> +			data->status[PB_STATUS_CML_BASE + i]
>> +				= _pmbus_read_byte_data(client, i,
>> +							PMBUS_STATUS_CML);
>> +			data->status[PB_STATUS_OTHER_BASE + i]
>> +				= _pmbus_read_byte_data(client, i,
>> +							PMBUS_STATUS_OTHER);
>> +			data->status[PB_STATUS_MFR_BASE + i]
>> +			    = _pmbus_read_byte_data(client, i,
>> +						    PMBUS_STATUS_MFR_SPECIFIC);
>>   		}
>>   
>>   		if (info->func[0] & PMBUS_HAVE_STATUS_INPUT)
>> @@ -1891,6 +1914,151 @@ static int pmbus_regulator_register(struct pmbus_data *data)
>>   }
>>   #endif
>>   
>> +static int pmbus_debugfs_get(void *data, u64 *val)
>> +{
>> +	struct pmbus_debugfs_entry *entry = data;
>> +	struct pmbus_data *pdata = pmbus_update_device(entry->dev);
>> +
>> +	*val = pdata->status[entry->index];
>> +
>> +	return 0;
>> +}
>> +
>> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops8, pmbus_debugfs_get, NULL,
>> +			 "0x%02llx\n");
>> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops16, pmbus_debugfs_get, NULL,
>> +			 "0x%04llx\n");
>> +
>> +static int pmbus_init_debugfs(struct i2c_client *client,
>> +			      struct pmbus_data *data)
>> +{
>> +	int i, idx = 0;
>> +	struct dentry *dbg;
>> +	char name[PMBUS_NAME_SIZE];
>> +	struct pmbus_debugfs_entry *entries;
>> +
>> +	/*
>> +	 * Either user didn't request debugfs or debugfs is not enabled in
>> +	 * kernel. Exit but don't throw an error in these cases.
>> +	 */
> Here might be the place to initialize pmbus_debugfs_dir
> if it is not yet initialized.
>
>> +	if (!data->info->debugfs || !pmbus_debugfs_dir)
>> +		return 0;
>> +
>> +	/*
>> +	 * Create the debugfs directory for this device. Use the hwmon device
>> +	 * name to avoid conflicts (hwmon numbers are globally unique).
>> +	 */
>> +	data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
>> +					   pmbus_debugfs_dir);
>> +	if (IS_ERR_OR_NULL(data->debugfs)) {
>> +		data->debugfs = NULL;
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* Allocate the max possible entries we need. */
>> +	entries = devm_kzalloc(data->dev,
>> +			       sizeof(*entries) * (data->info->pages * 10),
>> +			       GFP_KERNEL);
>> +	if (!entries)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < data->info->pages; ++i) {
>> +		/* check accessibility of status register if it's not page 0 */
>> +		if (!i || pmbus_check_status_register(client, i)) {
>> +			entries[idx].dev = data->hwmon_dev;
>> +			entries[idx].index = PB_STATUS_BASE + i;
>> +			snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
>> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
>> +						  &entries[idx++],
>> +						  &pmbus_debugfs_ops16);
> What is the point of the dbg variable ?
>
>> +		}
>> +
>> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
>> +			entries[idx].dev = data->hwmon_dev;
>> +			entries[idx].index = PB_STATUS_VOUT_BASE + i;
>> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
>> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
>> +						  &entries[idx++],
>> +						  &pmbus_debugfs_ops8);
>> +		}
>> +
>> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
>> +			entries[idx].dev = data->hwmon_dev;
>> +			entries[idx].index = PB_STATUS_IOUT_BASE + i;
>> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
>> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
>> +						  &entries[idx++],
>> +						  &pmbus_debugfs_ops8);
>> +		}
>> +
>> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
>> +			entries[idx].dev = data->hwmon_dev;
>> +			entries[idx].index = PB_STATUS_INPUT_BASE + i;
>> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
>> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
>> +						  &entries[idx++],
>> +						  &pmbus_debugfs_ops8);
>> +		}
>> +
>> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
>> +			entries[idx].dev = data->hwmon_dev;
>> +			entries[idx].index = PB_STATUS_TEMP_BASE + i;
>> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
>> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
>> +						  &entries[idx++],
>> +						  &pmbus_debugfs_ops8);
>> +		}
>> +
>> +		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
>> +			entries[idx].dev = data->hwmon_dev;
>> +			entries[idx].index = PB_STATUS_CML_BASE + i;
>> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
>> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
>> +						  &entries[idx++],
>> +						  &pmbus_debugfs_ops8);
>> +		}
>> +
>> +		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
>> +			entries[idx].dev = data->hwmon_dev;
>> +			entries[idx].index = PB_STATUS_OTHER_BASE + i;
>> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
>> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
>> +						  &entries[idx++],
>> +						  &pmbus_debugfs_ops8);
>> +		}
>> +
>> +		if (pmbus_check_byte_register(client, i,
>> +					      PMBUS_STATUS_MFR_SPECIFIC)) {
>> +			entries[idx].dev = data->hwmon_dev;
>> +			entries[idx].index = PB_STATUS_MFR_BASE + i;
>> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
>> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
>> +						  &entries[idx++],
>> +						  &pmbus_debugfs_ops8);
>> +		}
>> +
>> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
>> +			entries[idx].dev = data->hwmon_dev;
>> +			entries[idx].index = PB_STATUS_FAN_BASE + i;
>> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
>> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
>> +						  &entries[idx++],
>> +						  &pmbus_debugfs_ops8);
>> +		}
>> +
>> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
>> +			entries[idx].dev = data->hwmon_dev;
>> +			entries[idx].index = PB_STATUS_FAN34_BASE + i;
>> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
>> +			dbg = debugfs_create_file(name, 0444, data->debugfs,
>> +						  &entries[idx++],
>> +						  &pmbus_debugfs_ops8);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> Typically debugfs code is conditional. What happens if DEBUGFS
> is not enabled ? See below.
>
>>   int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>>   		   struct pmbus_driver_info *info)
>>   {
>> @@ -1950,6 +2118,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>>   	if (ret)
>>   		goto out_unregister;
>>   
>> +	ret = pmbus_init_debugfs(client, data);
>> +	if (ret)
>> +		dev_warn(dev, "Failed to register debugfs\n");
> I think this warning will be generated if debugfs is disabled.
> We should not do that. Consider putting the debugfs code into
> #fidef and have a dummy pmbus_init_debugfs() returning 0 if
> it is disabled.

Yes, good idea.

>
>> +
>>   	return 0;
>>   
>>   out_unregister:
>> @@ -1963,6 +2135,9 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>>   int pmbus_do_remove(struct i2c_client *client)
>>   {
>>   	struct pmbus_data *data = i2c_get_clientdata(client);
>> +
>> +	debugfs_remove_recursive(data->debugfs);
>> +
>>   	hwmon_device_unregister(data->hwmon_dev);
>>   	kfree(data->group.attrs);
>>   	return 0;
>> -- 
>> 1.8.3.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] drivers/hwmon/pmbus: Add debugfs for status registers

[Eddie James]

On 08/10/2017 03:15 PM, Eddie James wrote:
>
>
> On 08/09/2017 08:15 PM, Guenter Roeck wrote:
>> On Wed, Aug 09, 2017 at 05:19:17PM -0500, Eddie James wrote:
>>> From: "Edward A. James" <eajames@us.ibm.com>
>>>
>>> Export all the available status registers through debugfs, if the 
>>> client
>>> driver wants them.
>>>
>>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>>> ---
>>>   drivers/hwmon/pmbus/pmbus.c      |  24 +++++-
>>>   drivers/hwmon/pmbus/pmbus.h      |  11 +++
>>>   drivers/hwmon/pmbus/pmbus_core.c | 175 
>>> +++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 209 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
>>> index 7718e58..fc27417 100644
>>> --- a/drivers/hwmon/pmbus/pmbus.c
>>> +++ b/drivers/hwmon/pmbus/pmbus.c
>> That won't work: SENSORS_PMBUS is independent of PMBUS and does not have
>> to be enabled even if PMBUS is enabled. This will have to be in 
>> pmbus_core.c.
>>
>> I would suggest to do the same as done with other drivers. Move
>> pmbus_debugfs_dir into pmbus_core.c and create the directory on probe
>> if it has not already been created.
>
> Hmm, I'm having trouble with this. Don't I have to call 
> debugfs_recursive_remove() on the pmbus_debugfs_dir when the pmbus 
> driver is unloaded? That can't be done from pmbus_core.c, as that is 
> just device probe/remove. I agree it makes sense to only create it if 
> a device is going to be using it (and is enabled in the kernel) but I 
> think I need to declare it here so I can remove it on unload.

Sorry was a bit confused about which file was compiled with what CONFIG. 
I should create pmbus_debugfs_dir in pmbus_core.c, agreed. But I should 
still do the remove in pmbus.c I think.

>
>>
>>> @@ -18,6 +18,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>
>>> @@ -230,7 +231,28 @@ static int pmbus_probe(struct i2c_client *client,
>>>       .id_table = pmbus_id,
>>>   };
>>>   -module_i2c_driver(pmbus_driver);
>>> +struct dentry *pmbus_debugfs_dir;
>>> +EXPORT_SYMBOL_GPL(pmbus_debugfs_dir);
>>> +
>>> +static int __init pmbus_init(void)
>>> +{
>>> +    pmbus_debugfs_dir = 
>>> debugfs_create_dir(pmbus_driver.driver.name, NULL);
>>> +    if (IS_ERR(pmbus_debugfs_dir))
>>> +        /* Don't fail out if we don't have debugfs support. */
>>> +        pmbus_debugfs_dir = NULL;
>>> +
>>> +    return i2c_add_driver(&pmbus_driver);
>>> +}
>>> +
>>> +static void __exit pmbus_exit(void)
>>> +{
>>> +    debugfs_remove_recursive(pmbus_debugfs_dir);
>>> +
>>> +    i2c_del_driver(&pmbus_driver);
>>> +}
>>> +
>>> +module_init(pmbus_init);
>>> +module_exit(pmbus_exit);
>>>     MODULE_AUTHOR("Guenter Roeck");
>>>   MODULE_DESCRIPTION("Generic PMBus driver");
>>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
>>> index bfcb13b..c772b83 100644
>>> --- a/drivers/hwmon/pmbus/pmbus.h
>>> +++ b/drivers/hwmon/pmbus/pmbus.h
>>> @@ -25,6 +25,8 @@
>>>   #include <linux/bitops.h>
>>>   #include <linux/regulator/driver.h>
>>>   +struct dentry;
>>> +
>>>   /*
>>>    * Registers
>>>    */
>>> @@ -383,6 +385,12 @@ struct pmbus_driver_info {
>>>       /* Regulator functionality, if supported by this chip driver. */
>>>       int num_regulators;
>>>       const struct regulator_desc *reg_desc;
>>> +
>>> +    /*
>>> +     * Controls whether or not to create debugfs entries for this 
>>> driver's
>>> +     * devices.
>>> +     */
>>> +    bool debugfs;
>>>   };
>>>     /* Regulator ops */
>>> @@ -401,6 +409,9 @@ struct pmbus_driver_info {
>>>           .owner = THIS_MODULE,                \
>>>       }
>>>   +/* Handle for debugfs directory */
>>> +extern struct dentry *pmbus_debugfs_dir;
>>> +
>>>   /* Function declarations */
>>>     void pmbus_clear_cache(struct i2c_client *client);
>>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c 
>>> b/drivers/hwmon/pmbus/pmbus_core.c
>>> index 1a51f8f..afbd364 100644
>>> --- a/drivers/hwmon/pmbus/pmbus_core.c
>>> +++ b/drivers/hwmon/pmbus/pmbus_core.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>
>>> @@ -49,6 +50,9 @@
>>>   #define PB_STATUS_FAN34_BASE    (PB_STATUS_FAN_BASE + PMBUS_PAGES)
>>>   #define PB_STATUS_TEMP_BASE    (PB_STATUS_FAN34_BASE + PMBUS_PAGES)
>>>   #define PB_STATUS_INPUT_BASE    (PB_STATUS_TEMP_BASE + PMBUS_PAGES)
>>> +#define PB_STATUS_CML_BASE    (PB_STATUS_INPUT_BASE + PMBUS_PAGES)
>>> +#define PB_STATUS_OTHER_BASE    (PB_STATUS_CML_BASE + PMBUS_PAGES)
>>> +#define PB_STATUS_MFR_BASE    (PB_STATUS_OTHER_BASE + PMBUS_PAGES)
>> We are not actively using those status registers, are we ?
>> I am a bit concerned that we'll continuously read those status registers
>> but don't do anything with it most of the time. Ultimately I'd prefer
>> to get rid of all caching, not more, since it gets quite expensive
>> on chips with many pages.
>>
>> Maybe just display uncached status register values in debugfs ?
>
> Sounds good.
>
>>
>>>   #define PB_STATUS_VMON_BASE (PB_STATUS_INPUT_BASE + 1)
>>>     #define PB_NUM_STATUS_REG    (PB_STATUS_VMON_BASE + 1)
>>> @@ -101,6 +105,7 @@ struct pmbus_data {
>>>       int num_attributes;
>>>       struct attribute_group group;
>>>       const struct attribute_group *groups[2];
>>> +    struct dentry *debugfs;        /* debugfs device directory */
>>>         struct pmbus_sensor *sensors;
>>>   @@ -119,6 +124,11 @@ struct pmbus_data {
>>>       u8 currpage;
>>>   };
>>>   +struct pmbus_debugfs_entry {
>>> +    struct device *dev;
>>> +    int index;
>>> +};
>>> +
>>>   void pmbus_clear_cache(struct i2c_client *client)
>>>   {
>>>       struct pmbus_data *data = i2c_get_clientdata(client);
>>> @@ -422,6 +432,19 @@ static struct pmbus_data 
>>> *pmbus_update_device(struct device *dev)
>>>                       = _pmbus_read_byte_data(client, i,
>>>                                   s->reg);
>>>               }
>>> +
>>> +            if (!data->debugfs)
>>> +                continue;
>>> +
>>> +            data->status[PB_STATUS_CML_BASE + i]
>>> +                = _pmbus_read_byte_data(client, i,
>>> +                            PMBUS_STATUS_CML);
>>> +            data->status[PB_STATUS_OTHER_BASE + i]
>>> +                = _pmbus_read_byte_data(client, i,
>>> +                            PMBUS_STATUS_OTHER);
>>> +            data->status[PB_STATUS_MFR_BASE + i]
>>> +                = _pmbus_read_byte_data(client, i,
>>> +                            PMBUS_STATUS_MFR_SPECIFIC);
>>>           }
>>>             if (info->func[0] & PMBUS_HAVE_STATUS_INPUT)
>>> @@ -1891,6 +1914,151 @@ static int pmbus_regulator_register(struct 
>>> pmbus_data *data)
>>>   }
>>>   #endif
>>>   +static int pmbus_debugfs_get(void *data, u64 *val)
>>> +{
>>> +    struct pmbus_debugfs_entry *entry = data;
>>> +    struct pmbus_data *pdata = pmbus_update_device(entry->dev);
>>> +
>>> +    *val = pdata->status[entry->index];
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops8, pmbus_debugfs_get, NULL,
>>> +             "0x%02llx\n");
>>> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops16, pmbus_debugfs_get, NULL,
>>> +             "0x%04llx\n");
>>> +
>>> +static int pmbus_init_debugfs(struct i2c_client *client,
>>> +                  struct pmbus_data *data)
>>> +{
>>> +    int i, idx = 0;
>>> +    struct dentry *dbg;
>>> +    char name[PMBUS_NAME_SIZE];
>>> +    struct pmbus_debugfs_entry *entries;
>>> +
>>> +    /*
>>> +     * Either user didn't request debugfs or debugfs is not enabled in
>>> +     * kernel. Exit but don't throw an error in these cases.
>>> +     */
>> Here might be the place to initialize pmbus_debugfs_dir
>> if it is not yet initialized.
>>
>>> +    if (!data->info->debugfs || !pmbus_debugfs_dir)
>>> +        return 0;
>>> +
>>> +    /*
>>> +     * Create the debugfs directory for this device. Use the hwmon 
>>> device
>>> +     * name to avoid conflicts (hwmon numbers are globally unique).
>>> +     */
>>> +    data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
>>> +                       pmbus_debugfs_dir);
>>> +    if (IS_ERR_OR_NULL(data->debugfs)) {
>>> +        data->debugfs = NULL;
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    /* Allocate the max possible entries we need. */
>>> +    entries = devm_kzalloc(data->dev,
>>> +                   sizeof(*entries) * (data->info->pages * 10),
>>> +                   GFP_KERNEL);
>>> +    if (!entries)
>>> +        return -ENOMEM;
>>> +
>>> +    for (i = 0; i < data->info->pages; ++i) {
>>> +        /* check accessibility of status register if it's not page 
>>> 0 */
>>> +        if (!i || pmbus_check_status_register(client, i)) {
>>> +            entries[idx].dev = data->hwmon_dev;
>>> +            entries[idx].index = PB_STATUS_BASE + i;
>>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
>>> +            dbg = debugfs_create_file(name, 0444, data->debugfs,
>>> +                          &entries[idx++],
>>> +                          &pmbus_debugfs_ops16);
>> What is the point of the dbg variable ?
>>
>>> +        }
>>> +
>>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
>>> +            entries[idx].dev = data->hwmon_dev;
>>> +            entries[idx].index = PB_STATUS_VOUT_BASE + i;
>>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
>>> +            dbg = debugfs_create_file(name, 0444, data->debugfs,
>>> +                          &entries[idx++],
>>> +                          &pmbus_debugfs_ops8);
>>> +        }
>>> +
>>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
>>> +            entries[idx].dev = data->hwmon_dev;
>>> +            entries[idx].index = PB_STATUS_IOUT_BASE + i;
>>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
>>> +            dbg = debugfs_create_file(name, 0444, data->debugfs,
>>> +                          &entries[idx++],
>>> +                          &pmbus_debugfs_ops8);
>>> +        }
>>> +
>>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
>>> +            entries[idx].dev = data->hwmon_dev;
>>> +            entries[idx].index = PB_STATUS_INPUT_BASE + i;
>>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
>>> +            dbg = debugfs_create_file(name, 0444, data->debugfs,
>>> +                          &entries[idx++],
>>> +                          &pmbus_debugfs_ops8);
>>> +        }
>>> +
>>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
>>> +            entries[idx].dev = data->hwmon_dev;
>>> +            entries[idx].index = PB_STATUS_TEMP_BASE + i;
>>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
>>> +            dbg = debugfs_create_file(name, 0444, data->debugfs,
>>> +                          &entries[idx++],
>>> +                          &pmbus_debugfs_ops8);
>>> +        }
>>> +
>>> +        if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
>>> +            entries[idx].dev = data->hwmon_dev;
>>> +            entries[idx].index = PB_STATUS_CML_BASE + i;
>>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
>>> +            dbg = debugfs_create_file(name, 0444, data->debugfs,
>>> +                          &entries[idx++],
>>> +                          &pmbus_debugfs_ops8);
>>> +        }
>>> +
>>> +        if (pmbus_check_byte_register(client, i, 
>>> PMBUS_STATUS_OTHER)) {
>>> +            entries[idx].dev = data->hwmon_dev;
>>> +            entries[idx].index = PB_STATUS_OTHER_BASE + i;
>>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
>>> +            dbg = debugfs_create_file(name, 0444, data->debugfs,
>>> +                          &entries[idx++],
>>> +                          &pmbus_debugfs_ops8);
>>> +        }
>>> +
>>> +        if (pmbus_check_byte_register(client, i,
>>> +                          PMBUS_STATUS_MFR_SPECIFIC)) {
>>> +            entries[idx].dev = data->hwmon_dev;
>>> +            entries[idx].index = PB_STATUS_MFR_BASE + i;
>>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
>>> +            dbg = debugfs_create_file(name, 0444, data->debugfs,
>>> +                          &entries[idx++],
>>> +                          &pmbus_debugfs_ops8);
>>> +        }
>>> +
>>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
>>> +            entries[idx].dev = data->hwmon_dev;
>>> +            entries[idx].index = PB_STATUS_FAN_BASE + i;
>>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
>>> +            dbg = debugfs_create_file(name, 0444, data->debugfs,
>>> +                          &entries[idx++],
>>> +                          &pmbus_debugfs_ops8);
>>> +        }
>>> +
>>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
>>> +            entries[idx].dev = data->hwmon_dev;
>>> +            entries[idx].index = PB_STATUS_FAN34_BASE + i;
>>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
>>> +            dbg = debugfs_create_file(name, 0444, data->debugfs,
>>> +                          &entries[idx++],
>>> +                          &pmbus_debugfs_ops8);
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>> Typically debugfs code is conditional. What happens if DEBUGFS
>> is not enabled ? See below.
>>
>>>   int pmbus_do_probe(struct i2c_client *client, const struct 
>>> i2c_device_id *id,
>>>              struct pmbus_driver_info *info)
>>>   {
>>> @@ -1950,6 +2118,10 @@ int pmbus_do_probe(struct i2c_client *client, 
>>> const struct i2c_device_id *id,
>>>       if (ret)
>>>           goto out_unregister;
>>>   +    ret = pmbus_init_debugfs(client, data);
>>> +    if (ret)
>>> +        dev_warn(dev, "Failed to register debugfs\n");
>> I think this warning will be generated if debugfs is disabled.
>> We should not do that. Consider putting the debugfs code into
>> #fidef and have a dummy pmbus_init_debugfs() returning 0 if
>> it is disabled.
>
> Yes, good idea.
>
>>
>>> +
>>>       return 0;
>>>     out_unregister:
>>> @@ -1963,6 +2135,9 @@ int pmbus_do_probe(struct i2c_client *client, 
>>> const struct i2c_device_id *id,
>>>   int pmbus_do_remove(struct i2c_client *client)
>>>   {
>>>       struct pmbus_data *data = i2c_get_clientdata(client);
>>> +
>>> +    debugfs_remove_recursive(data->debugfs);
>>> +
>>>       hwmon_device_unregister(data->hwmon_dev);
>>>       kfree(data->group.attrs);
>>>       return 0;
>>> -- 
>>> 1.8.3.1
>>>
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe 
>>> linux-hwmon" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 4/4] drivers/hwmon/pmbus: Add debugfs for status registers

[Guenter Roeck]

On Thu, Aug 10, 2017 at 03:23:11PM -0500, Eddie James wrote:
> 
> 
> On 08/10/2017 03:15 PM, Eddie James wrote:
> >
> >
> >On 08/09/2017 08:15 PM, Guenter Roeck wrote:
> >>On Wed, Aug 09, 2017 at 05:19:17PM -0500, Eddie James wrote:
> >>>From: "Edward A. James" <eajames@us.ibm.com>
> >>>
> >>>Export all the available status registers through debugfs, if the
> >>>client
> >>>driver wants them.
> >>>
> >>>Signed-off-by: Edward A. James <eajames@us.ibm.com>
> >>>---
> >>>  drivers/hwmon/pmbus/pmbus.c      |  24 +++++-
> >>>  drivers/hwmon/pmbus/pmbus.h      |  11 +++
> >>>  drivers/hwmon/pmbus/pmbus_core.c | 175
> >>>+++++++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 209 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
> >>>index 7718e58..fc27417 100644
> >>>--- a/drivers/hwmon/pmbus/pmbus.c
> >>>+++ b/drivers/hwmon/pmbus/pmbus.c
> >>That won't work: SENSORS_PMBUS is independent of PMBUS and does not have
> >>to be enabled even if PMBUS is enabled. This will have to be in
> >>pmbus_core.c.
> >>
> >>I would suggest to do the same as done with other drivers. Move
> >>pmbus_debugfs_dir into pmbus_core.c and create the directory on probe
> >>if it has not already been created.
> >
> >Hmm, I'm having trouble with this. Don't I have to call
> >debugfs_recursive_remove() on the pmbus_debugfs_dir when the pmbus driver
> >is unloaded? That can't be done from pmbus_core.c, as that is just device
> >probe/remove. I agree it makes sense to only create it if a device is
> >going to be using it (and is enabled in the kernel) but I think I need to
> >declare it here so I can remove it on unload.
> 
> Sorry was a bit confused about which file was compiled with what CONFIG. I
> should create pmbus_debugfs_dir in pmbus_core.c, agreed. But I should still
> do the remove in pmbus.c I think.
> 

nack. pmbus.c is logically completely different to pmbus_core.c, and doesn't
even have to be enabled.

Guenter

> >
> >>
> >>>@@ -18,6 +18,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>
> >>>@@ -230,7 +231,28 @@ static int pmbus_probe(struct i2c_client *client,
> >>>      .id_table = pmbus_id,
> >>>  };
> >>>  -module_i2c_driver(pmbus_driver);
> >>>+struct dentry *pmbus_debugfs_dir;
> >>>+EXPORT_SYMBOL_GPL(pmbus_debugfs_dir);
> >>>+
> >>>+static int __init pmbus_init(void)
> >>>+{
> >>>+    pmbus_debugfs_dir = debugfs_create_dir(pmbus_driver.driver.name,
> >>>NULL);
> >>>+    if (IS_ERR(pmbus_debugfs_dir))
> >>>+        /* Don't fail out if we don't have debugfs support. */
> >>>+        pmbus_debugfs_dir = NULL;
> >>>+
> >>>+    return i2c_add_driver(&pmbus_driver);
> >>>+}
> >>>+
> >>>+static void __exit pmbus_exit(void)
> >>>+{
> >>>+    debugfs_remove_recursive(pmbus_debugfs_dir);
> >>>+
> >>>+    i2c_del_driver(&pmbus_driver);
> >>>+}
> >>>+
> >>>+module_init(pmbus_init);
> >>>+module_exit(pmbus_exit);
> >>>    MODULE_AUTHOR("Guenter Roeck");
> >>>  MODULE_DESCRIPTION("Generic PMBus driver");
> >>>diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> >>>index bfcb13b..c772b83 100644
> >>>--- a/drivers/hwmon/pmbus/pmbus.h
> >>>+++ b/drivers/hwmon/pmbus/pmbus.h
> >>>@@ -25,6 +25,8 @@
> >>>  #include <linux/bitops.h>
> >>>  #include <linux/regulator/driver.h>
> >>>  +struct dentry;
> >>>+
> >>>  /*
> >>>   * Registers
> >>>   */
> >>>@@ -383,6 +385,12 @@ struct pmbus_driver_info {
> >>>      /* Regulator functionality, if supported by this chip driver. */
> >>>      int num_regulators;
> >>>      const struct regulator_desc *reg_desc;
> >>>+
> >>>+    /*
> >>>+     * Controls whether or not to create debugfs entries for this
> >>>driver's
> >>>+     * devices.
> >>>+     */
> >>>+    bool debugfs;
> >>>  };
> >>>    /* Regulator ops */
> >>>@@ -401,6 +409,9 @@ struct pmbus_driver_info {
> >>>          .owner = THIS_MODULE,                \
> >>>      }
> >>>  +/* Handle for debugfs directory */
> >>>+extern struct dentry *pmbus_debugfs_dir;
> >>>+
> >>>  /* Function declarations */
> >>>    void pmbus_clear_cache(struct i2c_client *client);
> >>>diff --git a/drivers/hwmon/pmbus/pmbus_core.c
> >>>b/drivers/hwmon/pmbus/pmbus_core.c
> >>>index 1a51f8f..afbd364 100644
> >>>--- a/drivers/hwmon/pmbus/pmbus_core.c
> >>>+++ b/drivers/hwmon/pmbus/pmbus_core.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>
> >>>@@ -49,6 +50,9 @@
> >>>  #define PB_STATUS_FAN34_BASE    (PB_STATUS_FAN_BASE + PMBUS_PAGES)
> >>>  #define PB_STATUS_TEMP_BASE    (PB_STATUS_FAN34_BASE + PMBUS_PAGES)
> >>>  #define PB_STATUS_INPUT_BASE    (PB_STATUS_TEMP_BASE + PMBUS_PAGES)
> >>>+#define PB_STATUS_CML_BASE    (PB_STATUS_INPUT_BASE + PMBUS_PAGES)
> >>>+#define PB_STATUS_OTHER_BASE    (PB_STATUS_CML_BASE + PMBUS_PAGES)
> >>>+#define PB_STATUS_MFR_BASE    (PB_STATUS_OTHER_BASE + PMBUS_PAGES)
> >>We are not actively using those status registers, are we ?
> >>I am a bit concerned that we'll continuously read those status registers
> >>but don't do anything with it most of the time. Ultimately I'd prefer
> >>to get rid of all caching, not more, since it gets quite expensive
> >>on chips with many pages.
> >>
> >>Maybe just display uncached status register values in debugfs ?
> >
> >Sounds good.
> >
> >>
> >>>  #define PB_STATUS_VMON_BASE (PB_STATUS_INPUT_BASE + 1)
> >>>    #define PB_NUM_STATUS_REG    (PB_STATUS_VMON_BASE + 1)
> >>>@@ -101,6 +105,7 @@ struct pmbus_data {
> >>>      int num_attributes;
> >>>      struct attribute_group group;
> >>>      const struct attribute_group *groups[2];
> >>>+    struct dentry *debugfs;        /* debugfs device directory */
> >>>        struct pmbus_sensor *sensors;
> >>>  @@ -119,6 +124,11 @@ struct pmbus_data {
> >>>      u8 currpage;
> >>>  };
> >>>  +struct pmbus_debugfs_entry {
> >>>+    struct device *dev;
> >>>+    int index;
> >>>+};
> >>>+
> >>>  void pmbus_clear_cache(struct i2c_client *client)
> >>>  {
> >>>      struct pmbus_data *data = i2c_get_clientdata(client);
> >>>@@ -422,6 +432,19 @@ static struct pmbus_data
> >>>*pmbus_update_device(struct device *dev)
> >>>                      = _pmbus_read_byte_data(client, i,
> >>>                                  s->reg);
> >>>              }
> >>>+
> >>>+            if (!data->debugfs)
> >>>+                continue;
> >>>+
> >>>+            data->status[PB_STATUS_CML_BASE + i]
> >>>+                = _pmbus_read_byte_data(client, i,
> >>>+                            PMBUS_STATUS_CML);
> >>>+            data->status[PB_STATUS_OTHER_BASE + i]
> >>>+                = _pmbus_read_byte_data(client, i,
> >>>+                            PMBUS_STATUS_OTHER);
> >>>+            data->status[PB_STATUS_MFR_BASE + i]
> >>>+                = _pmbus_read_byte_data(client, i,
> >>>+                            PMBUS_STATUS_MFR_SPECIFIC);
> >>>          }
> >>>            if (info->func[0] & PMBUS_HAVE_STATUS_INPUT)
> >>>@@ -1891,6 +1914,151 @@ static int pmbus_regulator_register(struct
> >>>pmbus_data *data)
> >>>  }
> >>>  #endif
> >>>  +static int pmbus_debugfs_get(void *data, u64 *val)
> >>>+{
> >>>+    struct pmbus_debugfs_entry *entry = data;
> >>>+    struct pmbus_data *pdata = pmbus_update_device(entry->dev);
> >>>+
> >>>+    *val = pdata->status[entry->index];
> >>>+
> >>>+    return 0;
> >>>+}
> >>>+
> >>>+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops8, pmbus_debugfs_get, NULL,
> >>>+             "0x%02llx\n");
> >>>+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops16, pmbus_debugfs_get, NULL,
> >>>+             "0x%04llx\n");
> >>>+
> >>>+static int pmbus_init_debugfs(struct i2c_client *client,
> >>>+                  struct pmbus_data *data)
> >>>+{
> >>>+    int i, idx = 0;
> >>>+    struct dentry *dbg;
> >>>+    char name[PMBUS_NAME_SIZE];
> >>>+    struct pmbus_debugfs_entry *entries;
> >>>+
> >>>+    /*
> >>>+     * Either user didn't request debugfs or debugfs is not enabled in
> >>>+     * kernel. Exit but don't throw an error in these cases.
> >>>+     */
> >>Here might be the place to initialize pmbus_debugfs_dir
> >>if it is not yet initialized.
> >>
> >>>+    if (!data->info->debugfs || !pmbus_debugfs_dir)
> >>>+        return 0;
> >>>+
> >>>+    /*
> >>>+     * Create the debugfs directory for this device. Use the hwmon
> >>>device
> >>>+     * name to avoid conflicts (hwmon numbers are globally unique).
> >>>+     */
> >>>+    data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
> >>>+                       pmbus_debugfs_dir);
> >>>+    if (IS_ERR_OR_NULL(data->debugfs)) {
> >>>+        data->debugfs = NULL;
> >>>+        return -ENODEV;
> >>>+    }
> >>>+
> >>>+    /* Allocate the max possible entries we need. */
> >>>+    entries = devm_kzalloc(data->dev,
> >>>+                   sizeof(*entries) * (data->info->pages * 10),
> >>>+                   GFP_KERNEL);
> >>>+    if (!entries)
> >>>+        return -ENOMEM;
> >>>+
> >>>+    for (i = 0; i < data->info->pages; ++i) {
> >>>+        /* check accessibility of status register if it's not page 0
> >>>*/
> >>>+        if (!i || pmbus_check_status_register(client, i)) {
> >>>+            entries[idx].dev = data->hwmon_dev;
> >>>+            entries[idx].index = PB_STATUS_BASE + i;
> >>>+            snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
> >>>+            dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>>+                          &entries[idx++],
> >>>+                          &pmbus_debugfs_ops16);
> >>What is the point of the dbg variable ?
> >>
> >>>+        }
> >>>+
> >>>+        if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
> >>>+            entries[idx].dev = data->hwmon_dev;
> >>>+            entries[idx].index = PB_STATUS_VOUT_BASE + i;
> >>>+            snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
> >>>+            dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>>+                          &entries[idx++],
> >>>+                          &pmbus_debugfs_ops8);
> >>>+        }
> >>>+
> >>>+        if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
> >>>+            entries[idx].dev = data->hwmon_dev;
> >>>+            entries[idx].index = PB_STATUS_IOUT_BASE + i;
> >>>+            snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
> >>>+            dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>>+                          &entries[idx++],
> >>>+                          &pmbus_debugfs_ops8);
> >>>+        }
> >>>+
> >>>+        if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
> >>>+            entries[idx].dev = data->hwmon_dev;
> >>>+            entries[idx].index = PB_STATUS_INPUT_BASE + i;
> >>>+            snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
> >>>+            dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>>+                          &entries[idx++],
> >>>+                          &pmbus_debugfs_ops8);
> >>>+        }
> >>>+
> >>>+        if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
> >>>+            entries[idx].dev = data->hwmon_dev;
> >>>+            entries[idx].index = PB_STATUS_TEMP_BASE + i;
> >>>+            snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
> >>>+            dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>>+                          &entries[idx++],
> >>>+                          &pmbus_debugfs_ops8);
> >>>+        }
> >>>+
> >>>+        if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
> >>>+            entries[idx].dev = data->hwmon_dev;
> >>>+            entries[idx].index = PB_STATUS_CML_BASE + i;
> >>>+            snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
> >>>+            dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>>+                          &entries[idx++],
> >>>+                          &pmbus_debugfs_ops8);
> >>>+        }
> >>>+
> >>>+        if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER))
> >>>{
> >>>+            entries[idx].dev = data->hwmon_dev;
> >>>+            entries[idx].index = PB_STATUS_OTHER_BASE + i;
> >>>+            snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
> >>>+            dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>>+                          &entries[idx++],
> >>>+                          &pmbus_debugfs_ops8);
> >>>+        }
> >>>+
> >>>+        if (pmbus_check_byte_register(client, i,
> >>>+                          PMBUS_STATUS_MFR_SPECIFIC)) {
> >>>+            entries[idx].dev = data->hwmon_dev;
> >>>+            entries[idx].index = PB_STATUS_MFR_BASE + i;
> >>>+            snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
> >>>+            dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>>+                          &entries[idx++],
> >>>+                          &pmbus_debugfs_ops8);
> >>>+        }
> >>>+
> >>>+        if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
> >>>+            entries[idx].dev = data->hwmon_dev;
> >>>+            entries[idx].index = PB_STATUS_FAN_BASE + i;
> >>>+            snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
> >>>+            dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>>+                          &entries[idx++],
> >>>+                          &pmbus_debugfs_ops8);
> >>>+        }
> >>>+
> >>>+        if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
> >>>+            entries[idx].dev = data->hwmon_dev;
> >>>+            entries[idx].index = PB_STATUS_FAN34_BASE + i;
> >>>+            snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
> >>>+            dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>>+                          &entries[idx++],
> >>>+                          &pmbus_debugfs_ops8);
> >>>+        }
> >>>+    }
> >>>+
> >>>+    return 0;
> >>>+}
> >>>+
> >>Typically debugfs code is conditional. What happens if DEBUGFS
> >>is not enabled ? See below.
> >>
> >>>  int pmbus_do_probe(struct i2c_client *client, const struct
> >>>i2c_device_id *id,
> >>>             struct pmbus_driver_info *info)
> >>>  {
> >>>@@ -1950,6 +2118,10 @@ int pmbus_do_probe(struct i2c_client *client,
> >>>const struct i2c_device_id *id,
> >>>      if (ret)
> >>>          goto out_unregister;
> >>>  +    ret = pmbus_init_debugfs(client, data);
> >>>+    if (ret)
> >>>+        dev_warn(dev, "Failed to register debugfs\n");
> >>I think this warning will be generated if debugfs is disabled.
> >>We should not do that. Consider putting the debugfs code into
> >>#fidef and have a dummy pmbus_init_debugfs() returning 0 if
> >>it is disabled.
> >
> >Yes, good idea.
> >
> >>
> >>>+
> >>>      return 0;
> >>>    out_unregister:
> >>>@@ -1963,6 +2135,9 @@ int pmbus_do_probe(struct i2c_client *client,
> >>>const struct i2c_device_id *id,
> >>>  int pmbus_do_remove(struct i2c_client *client)
> >>>  {
> >>>      struct pmbus_data *data = i2c_get_clientdata(client);
> >>>+
> >>>+    debugfs_remove_recursive(data->debugfs);
> >>>+
> >>>      hwmon_device_unregister(data->hwmon_dev);
> >>>      kfree(data->group.attrs);
> >>>      return 0;
> >>>-- 
> >>>1.8.3.1
> >>>
> >>>-- 
> >>>To unsubscribe from this list: send the line "unsubscribe linux-hwmon"
> >>>in
> >>>the body of a message to majordomo@vger.kernel.org
> >>>More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
> 

Re: [PATCH 4/4] drivers/hwmon/pmbus: Add debugfs for status registers

[Guenter Roeck]

On Thu, Aug 10, 2017 at 03:15:57PM -0500, Eddie James wrote:
> 
> 
> On 08/09/2017 08:15 PM, Guenter Roeck wrote:
> >On Wed, Aug 09, 2017 at 05:19:17PM -0500, Eddie James wrote:
> >>From: "Edward A. James" <eajames@us.ibm.com>
> >>
> >>Export all the available status registers through debugfs, if the client
> >>driver wants them.
> >>
> >>Signed-off-by: Edward A. James <eajames@us.ibm.com>
> >>---
> >>  drivers/hwmon/pmbus/pmbus.c      |  24 +++++-
> >>  drivers/hwmon/pmbus/pmbus.h      |  11 +++
> >>  drivers/hwmon/pmbus/pmbus_core.c | 175 +++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 209 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/hwmon/pmbus/pmbus.c b/drivers/hwmon/pmbus/pmbus.c
> >>index 7718e58..fc27417 100644
> >>--- a/drivers/hwmon/pmbus/pmbus.c
> >>+++ b/drivers/hwmon/pmbus/pmbus.c
> >That won't work: SENSORS_PMBUS is independent of PMBUS and does not have
> >to be enabled even if PMBUS is enabled. This will have to be in pmbus_core.c.
> >
> >I would suggest to do the same as done with other drivers. Move
> >pmbus_debugfs_dir into pmbus_core.c and create the directory on probe
> >if it has not already been created.
> 
> Hmm, I'm having trouble with this. Don't I have to call
> debugfs_recursive_remove() on the pmbus_debugfs_dir when the pmbus driver is
> unloaded? That can't be done from pmbus_core.c, as that is just device
> probe/remove. I agree it makes sense to only create it if a device is going
> to be using it (and is enabled in the kernel) but I think I need to declare
> it here so I can remove it on unload.
> 
So if CONFIG_SENSORS_PMBUS is not enabled, you won't have debugfs.
actually, even worse, this effectively mandates that CONFIG_SENSORS_PMBUS
must be enabled since it derclares a variable used by the core.
That doesn't make sense. What makes even less sense is that it defines the
variable in a module, thus _mandating_ that PMBUS_CORE is also built as module
if this driver is built as module. Which you can't even express in Kconfig
right now without adding a lot of complexity.

Guenter

> >
> >>@@ -18,6 +18,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>
> >>@@ -230,7 +231,28 @@ static int pmbus_probe(struct i2c_client *client,
> >>  	.id_table = pmbus_id,
> >>  };
> >>-module_i2c_driver(pmbus_driver);
> >>+struct dentry *pmbus_debugfs_dir;
> >>+EXPORT_SYMBOL_GPL(pmbus_debugfs_dir);
> >>+
> >>+static int __init pmbus_init(void)
> >>+{
> >>+	pmbus_debugfs_dir = debugfs_create_dir(pmbus_driver.driver.name, NULL);
> >>+	if (IS_ERR(pmbus_debugfs_dir))
> >>+		/* Don't fail out if we don't have debugfs support. */
> >>+		pmbus_debugfs_dir = NULL;
> >>+
> >>+	return i2c_add_driver(&pmbus_driver);
> >>+}
> >>+
> >>+static void __exit pmbus_exit(void)
> >>+{
> >>+	debugfs_remove_recursive(pmbus_debugfs_dir);
> >>+
> >>+	i2c_del_driver(&pmbus_driver);
> >>+}
> >>+
> >>+module_init(pmbus_init);
> >>+module_exit(pmbus_exit);
> >>  MODULE_AUTHOR("Guenter Roeck");
> >>  MODULE_DESCRIPTION("Generic PMBus driver");
> >>diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> >>index bfcb13b..c772b83 100644
> >>--- a/drivers/hwmon/pmbus/pmbus.h
> >>+++ b/drivers/hwmon/pmbus/pmbus.h
> >>@@ -25,6 +25,8 @@
> >>  #include <linux/bitops.h>
> >>  #include <linux/regulator/driver.h>
> >>+struct dentry;
> >>+
> >>  /*
> >>   * Registers
> >>   */
> >>@@ -383,6 +385,12 @@ struct pmbus_driver_info {
> >>  	/* Regulator functionality, if supported by this chip driver. */
> >>  	int num_regulators;
> >>  	const struct regulator_desc *reg_desc;
> >>+
> >>+	/*
> >>+	 * Controls whether or not to create debugfs entries for this driver's
> >>+	 * devices.
> >>+	 */
> >>+	bool debugfs;
> >>  };
> >>  /* Regulator ops */
> >>@@ -401,6 +409,9 @@ struct pmbus_driver_info {
> >>  		.owner = THIS_MODULE,				\
> >>  	}
> >>+/* Handle for debugfs directory */
> >>+extern struct dentry *pmbus_debugfs_dir;
> >>+
> >>  /* Function declarations */
> >>  void pmbus_clear_cache(struct i2c_client *client);
> >>diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> >>index 1a51f8f..afbd364 100644
> >>--- a/drivers/hwmon/pmbus/pmbus_core.c
> >>+++ b/drivers/hwmon/pmbus/pmbus_core.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>
> >>@@ -49,6 +50,9 @@
> >>  #define PB_STATUS_FAN34_BASE	(PB_STATUS_FAN_BASE + PMBUS_PAGES)
> >>  #define PB_STATUS_TEMP_BASE	(PB_STATUS_FAN34_BASE + PMBUS_PAGES)
> >>  #define PB_STATUS_INPUT_BASE	(PB_STATUS_TEMP_BASE + PMBUS_PAGES)
> >>+#define PB_STATUS_CML_BASE	(PB_STATUS_INPUT_BASE + PMBUS_PAGES)
> >>+#define PB_STATUS_OTHER_BASE	(PB_STATUS_CML_BASE + PMBUS_PAGES)
> >>+#define PB_STATUS_MFR_BASE	(PB_STATUS_OTHER_BASE + PMBUS_PAGES)
> >We are not actively using those status registers, are we ?
> >I am a bit concerned that we'll continuously read those status registers
> >but don't do anything with it most of the time. Ultimately I'd prefer
> >to get rid of all caching, not more, since it gets quite expensive
> >on chips with many pages.
> >
> >Maybe just display uncached status register values in debugfs ?
> 
> Sounds good.
> 
> >
> >>  #define PB_STATUS_VMON_BASE	(PB_STATUS_INPUT_BASE + 1)
> >>  #define PB_NUM_STATUS_REG	(PB_STATUS_VMON_BASE + 1)
> >>@@ -101,6 +105,7 @@ struct pmbus_data {
> >>  	int num_attributes;
> >>  	struct attribute_group group;
> >>  	const struct attribute_group *groups[2];
> >>+	struct dentry *debugfs;		/* debugfs device directory */
> >>  	struct pmbus_sensor *sensors;
> >>@@ -119,6 +124,11 @@ struct pmbus_data {
> >>  	u8 currpage;
> >>  };
> >>+struct pmbus_debugfs_entry {
> >>+	struct device *dev;
> >>+	int index;
> >>+};
> >>+
> >>  void pmbus_clear_cache(struct i2c_client *client)
> >>  {
> >>  	struct pmbus_data *data = i2c_get_clientdata(client);
> >>@@ -422,6 +432,19 @@ static struct pmbus_data *pmbus_update_device(struct device *dev)
> >>  					= _pmbus_read_byte_data(client, i,
> >>  								s->reg);
> >>  			}
> >>+
> >>+			if (!data->debugfs)
> >>+				continue;
> >>+
> >>+			data->status[PB_STATUS_CML_BASE + i]
> >>+				= _pmbus_read_byte_data(client, i,
> >>+							PMBUS_STATUS_CML);
> >>+			data->status[PB_STATUS_OTHER_BASE + i]
> >>+				= _pmbus_read_byte_data(client, i,
> >>+							PMBUS_STATUS_OTHER);
> >>+			data->status[PB_STATUS_MFR_BASE + i]
> >>+			    = _pmbus_read_byte_data(client, i,
> >>+						    PMBUS_STATUS_MFR_SPECIFIC);
> >>  		}
> >>  		if (info->func[0] & PMBUS_HAVE_STATUS_INPUT)
> >>@@ -1891,6 +1914,151 @@ static int pmbus_regulator_register(struct pmbus_data *data)
> >>  }
> >>  #endif
> >>+static int pmbus_debugfs_get(void *data, u64 *val)
> >>+{
> >>+	struct pmbus_debugfs_entry *entry = data;
> >>+	struct pmbus_data *pdata = pmbus_update_device(entry->dev);
> >>+
> >>+	*val = pdata->status[entry->index];
> >>+
> >>+	return 0;
> >>+}
> >>+
> >>+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops8, pmbus_debugfs_get, NULL,
> >>+			 "0x%02llx\n");
> >>+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops16, pmbus_debugfs_get, NULL,
> >>+			 "0x%04llx\n");
> >>+
> >>+static int pmbus_init_debugfs(struct i2c_client *client,
> >>+			      struct pmbus_data *data)
> >>+{
> >>+	int i, idx = 0;
> >>+	struct dentry *dbg;
> >>+	char name[PMBUS_NAME_SIZE];
> >>+	struct pmbus_debugfs_entry *entries;
> >>+
> >>+	/*
> >>+	 * Either user didn't request debugfs or debugfs is not enabled in
> >>+	 * kernel. Exit but don't throw an error in these cases.
> >>+	 */
> >Here might be the place to initialize pmbus_debugfs_dir
> >if it is not yet initialized.
> >
> >>+	if (!data->info->debugfs || !pmbus_debugfs_dir)
> >>+		return 0;
> >>+
> >>+	/*
> >>+	 * Create the debugfs directory for this device. Use the hwmon device
> >>+	 * name to avoid conflicts (hwmon numbers are globally unique).
> >>+	 */
> >>+	data->debugfs = debugfs_create_dir(dev_name(data->hwmon_dev),
> >>+					   pmbus_debugfs_dir);
> >>+	if (IS_ERR_OR_NULL(data->debugfs)) {
> >>+		data->debugfs = NULL;
> >>+		return -ENODEV;
> >>+	}
> >>+
> >>+	/* Allocate the max possible entries we need. */
> >>+	entries = devm_kzalloc(data->dev,
> >>+			       sizeof(*entries) * (data->info->pages * 10),
> >>+			       GFP_KERNEL);
> >>+	if (!entries)
> >>+		return -ENOMEM;
> >>+
> >>+	for (i = 0; i < data->info->pages; ++i) {
> >>+		/* check accessibility of status register if it's not page 0 */
> >>+		if (!i || pmbus_check_status_register(client, i)) {
> >>+			entries[idx].dev = data->hwmon_dev;
> >>+			entries[idx].index = PB_STATUS_BASE + i;
> >>+			snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
> >>+			dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>+						  &entries[idx++],
> >>+						  &pmbus_debugfs_ops16);
> >What is the point of the dbg variable ?
> >
> >>+		}
> >>+
> >>+		if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
> >>+			entries[idx].dev = data->hwmon_dev;
> >>+			entries[idx].index = PB_STATUS_VOUT_BASE + i;
> >>+			snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
> >>+			dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>+						  &entries[idx++],
> >>+						  &pmbus_debugfs_ops8);
> >>+		}
> >>+
> >>+		if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
> >>+			entries[idx].dev = data->hwmon_dev;
> >>+			entries[idx].index = PB_STATUS_IOUT_BASE + i;
> >>+			snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
> >>+			dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>+						  &entries[idx++],
> >>+						  &pmbus_debugfs_ops8);
> >>+		}
> >>+
> >>+		if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
> >>+			entries[idx].dev = data->hwmon_dev;
> >>+			entries[idx].index = PB_STATUS_INPUT_BASE + i;
> >>+			snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
> >>+			dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>+						  &entries[idx++],
> >>+						  &pmbus_debugfs_ops8);
> >>+		}
> >>+
> >>+		if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
> >>+			entries[idx].dev = data->hwmon_dev;
> >>+			entries[idx].index = PB_STATUS_TEMP_BASE + i;
> >>+			snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
> >>+			dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>+						  &entries[idx++],
> >>+						  &pmbus_debugfs_ops8);
> >>+		}
> >>+
> >>+		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
> >>+			entries[idx].dev = data->hwmon_dev;
> >>+			entries[idx].index = PB_STATUS_CML_BASE + i;
> >>+			snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
> >>+			dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>+						  &entries[idx++],
> >>+						  &pmbus_debugfs_ops8);
> >>+		}
> >>+
> >>+		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
> >>+			entries[idx].dev = data->hwmon_dev;
> >>+			entries[idx].index = PB_STATUS_OTHER_BASE + i;
> >>+			snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
> >>+			dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>+						  &entries[idx++],
> >>+						  &pmbus_debugfs_ops8);
> >>+		}
> >>+
> >>+		if (pmbus_check_byte_register(client, i,
> >>+					      PMBUS_STATUS_MFR_SPECIFIC)) {
> >>+			entries[idx].dev = data->hwmon_dev;
> >>+			entries[idx].index = PB_STATUS_MFR_BASE + i;
> >>+			snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
> >>+			dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>+						  &entries[idx++],
> >>+						  &pmbus_debugfs_ops8);
> >>+		}
> >>+
> >>+		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
> >>+			entries[idx].dev = data->hwmon_dev;
> >>+			entries[idx].index = PB_STATUS_FAN_BASE + i;
> >>+			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
> >>+			dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>+						  &entries[idx++],
> >>+						  &pmbus_debugfs_ops8);
> >>+		}
> >>+
> >>+		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
> >>+			entries[idx].dev = data->hwmon_dev;
> >>+			entries[idx].index = PB_STATUS_FAN34_BASE + i;
> >>+			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
> >>+			dbg = debugfs_create_file(name, 0444, data->debugfs,
> >>+						  &entries[idx++],
> >>+						  &pmbus_debugfs_ops8);
> >>+		}
> >>+	}
> >>+
> >>+	return 0;
> >>+}
> >>+
> >Typically debugfs code is conditional. What happens if DEBUGFS
> >is not enabled ? See below.
> >
> >>  int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> >>  		   struct pmbus_driver_info *info)
> >>  {
> >>@@ -1950,6 +2118,10 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> >>  	if (ret)
> >>  		goto out_unregister;
> >>+	ret = pmbus_init_debugfs(client, data);
> >>+	if (ret)
> >>+		dev_warn(dev, "Failed to register debugfs\n");
> >I think this warning will be generated if debugfs is disabled.
> >We should not do that. Consider putting the debugfs code into
> >#fidef and have a dummy pmbus_init_debugfs() returning 0 if
> >it is disabled.
> 
> Yes, good idea.
> 
> >
> >>+
> >>  	return 0;
> >>  out_unregister:
> >>@@ -1963,6 +2135,9 @@ int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
> >>  int pmbus_do_remove(struct i2c_client *client)
> >>  {
> >>  	struct pmbus_data *data = i2c_get_clientdata(client);
> >>+
> >>+	debugfs_remove_recursive(data->debugfs);
> >>+
> >>  	hwmon_device_unregister(data->hwmon_dev);
> >>  	kfree(data->group.attrs);
> >>  	return 0;
> >>-- 
> >>1.8.3.1
> >>
> >>--
> >>To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> >>the body of a message to majordomo@vger.kernel.org
> >>More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] drivers/hwmon/pmbus: Add generic alarm bit for iin and pin

[Guenter Roeck]

On Wed, Aug 09, 2017 at 05:19:16PM -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Add PB_STATUS_INPUT as the generic alarm bit for iin and pin. We also
> need to redo the status register checking before setting up the boolean
> attribute, since it won't necessarily check STATUS_WORD if the device
> doesn't support it, which we need for this bit.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

For the entire series: subject should start with "hwmon: (pmbus)".

> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 277d170..1a51f8f 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -1045,6 +1045,7 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
>  				      const struct pmbus_sensor_attr *attr)
>  {
>  	struct pmbus_sensor *base;
> +	bool upper = !!(attr->gbit & 0xff00);	/* need to check STATUS_WORD */
>  	int ret;
>  
>  	if (attr->label) {
> @@ -1065,10 +1066,13 @@ static int pmbus_add_sensor_attrs_one(struct i2c_client *client,
>  		/*
>  		 * Add generic alarm attribute only if there are no individual
>  		 * alarm attributes, if there is a global alarm bit, and if
> -		 * the generic status register for this page is accessible.
> +		 * the generic status register (word or byte, depending on
> +		 * which global bit is set) for this page is accessible.
>  		 */
>  		if (!ret && attr->gbit &&
> -		    pmbus_check_status_register(client, page)) {
> +		    ((upper && pmbus_check_word_register(client, page,
> +							 PMBUS_STATUS_WORD)) ||
> +		     (!upper && pmbus_check_status_register(client, page)))) {

We already know if PMBUS_STATUS_WORD is supported or not, since
pmbus_check_status_register() will use it. Just store that knowledge
in a flag when discovered and make this something like

		if (!ret && attr->gbit &&
		    (!upper || (upper && data->has_word_status)) &&
		    pmbus_check_status_register(client, page)) {

>  			ret = pmbus_add_boolean(data, name, "alarm", index,
>  						NULL, NULL,
>  						PB_STATUS_BASE + page,
> @@ -1324,6 +1328,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
>  		.func = PMBUS_HAVE_IIN,
>  		.sfunc = PMBUS_HAVE_STATUS_INPUT,
>  		.sbase = PB_STATUS_INPUT_BASE,
> +		.gbit = PB_STATUS_INPUT,
>  		.limit = iin_limit_attrs,
>  		.nlimit = ARRAY_SIZE(iin_limit_attrs),
>  	}, {
> @@ -1408,6 +1413,7 @@ static int pmbus_add_sensor_attrs(struct i2c_client *client,
>  		.func = PMBUS_HAVE_PIN,
>  		.sfunc = PMBUS_HAVE_STATUS_INPUT,
>  		.sbase = PB_STATUS_INPUT_BASE,
> +		.gbit = PB_STATUS_INPUT,
>  		.limit = pin_limit_attrs,
>  		.nlimit = ARRAY_SIZE(pin_limit_attrs),
>  	}, {
> -- 
> 1.8.3.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html