[PATCH v2 0/4] hwmon: (pmbus): Core extension for STATUS_WORD and debugfs

[PATCH v2 0/4] hwmon: (pmbus): Core extension for STATUS_WORD 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 v1:
 * Pull all debugfs stuff into pmbus_core.c to prevent problems when running
   without SENSORS_PMBUS.
 * Better boolean attr conditional.
 * Don't cache additional attributes, and display uncached registers for
   debugfs.
 * #ifdef around debugfs stuff.

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):
  hwmon: (pmbus): Switch status registers to 16 bit
  hwmon: (pmbus): Access word data for STATUS_WORD
  hwmon: (pmbus): Add generic alarm bit for iin and pin
  hwmon: (pmbus): Add debugfs for status registers

 drivers/hwmon/pmbus/pmbus.h      |   6 +
 drivers/hwmon/pmbus/pmbus_core.c | 279 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 263 insertions(+), 22 deletions(-)

-- 
1.8.3.1

[PATCH v2 1/4] 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 v2 2/4] 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 v2 3/4] 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, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index 277d170..ef135d8 100644
--- a/drivers/hwmon/pmbus/pmbus_core.c
+++ b/drivers/hwmon/pmbus/pmbus_core.c
@@ -114,6 +114,7 @@ struct pmbus_data {
 	 */
 	u16 status[PB_NUM_STATUS_REG];
 
+	bool has_status_word;		/* device uses STATUS_WORD register */
 	int (*read_status)(struct i2c_client *client, int page);
 
 	u8 currpage;
@@ -1045,6 +1046,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,9 +1067,11 @@ 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 &&
+		    (!upper || (upper && data->has_status_word)) &&
 		    pmbus_check_status_register(client, page)) {
 			ret = pmbus_add_boolean(data, name, "alarm", index,
 						NULL, NULL,
@@ -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),
 	}, {
@@ -1775,6 +1781,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
 			dev_err(dev, "PMBus status register not found\n");
 			return -ENODEV;
 		}
+	} else {
+		data->has_status_word = true;
 	}
 
 	/* Enable PEC if the controller supports it */
-- 
1.8.3.1

[PATCH v2 4/4] 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.h      |   6 ++
 drivers/hwmon/pmbus/pmbus_core.c | 201 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 207 insertions(+)

diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
index bfcb13b..5f91a19 100644
--- a/drivers/hwmon/pmbus/pmbus.h
+++ b/drivers/hwmon/pmbus/pmbus.h
@@ -383,6 +383,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 */
diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
index ef135d8..b11ebec 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>
@@ -101,6 +102,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;
 
@@ -120,6 +122,12 @@ struct pmbus_data {
 	u8 currpage;
 };
 
+struct pmbus_debugfs_entry {
+	struct i2c_client *client;
+	u8 page;
+	u8 reg;
+};
+
 void pmbus_clear_cache(struct i2c_client *client)
 {
 	struct pmbus_data *data = i2c_get_clientdata(client);
@@ -1893,6 +1901,184 @@ static int pmbus_regulator_register(struct pmbus_data *data)
 }
 #endif
 
+static struct dentry *pmbus_debugfs_dir;	/* pmbus debugfs directory */
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static int pmbus_debugfs_get(void *data, u64 *val)
+{
+	struct pmbus_debugfs_entry *entry = data;
+
+	*val = _pmbus_read_byte_data(entry->client, entry->page, entry->reg);
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops, pmbus_debugfs_get, NULL,
+			 "0x%02llx\n");
+
+static int pmbus_debugfs_get_status(void *data, u64 *val)
+{
+	struct pmbus_debugfs_entry *entry = data;
+	struct pmbus_data *pdata = i2c_get_clientdata(entry->client);
+
+	*val = pdata->read_status(entry->client, entry->page);
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops_status, pmbus_debugfs_get_status,
+			 NULL, "0x%04llx\n");
+
+static int pmbus_init_debugfs(struct i2c_client *client,
+			      struct pmbus_data *data)
+{
+	int i, idx = 0;
+	char name[PMBUS_NAME_SIZE];
+	struct pmbus_debugfs_entry *entries;
+
+	/* Check if client driver requested debugfs support. */
+	if (!data->info->debugfs)
+		return 0;
+
+	/* Create the root pmbus debugfs directory if it's not created yet. */
+	if (!pmbus_debugfs_dir) {
+		pmbus_debugfs_dir = debugfs_create_dir("pmbus", NULL);
+		if (IS_ERR_OR_NULL(pmbus_debugfs_dir)) {
+			pmbus_debugfs_dir = NULL;
+			return -ENODEV;
+		}
+	}
+
+	/*
+	 * 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)) {
+			/* No need to set reg as we have special read op. */
+			entries[idx].client = client;
+			entries[idx].page = i;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[idx++],
+					    &pmbus_debugfs_ops_status);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
+			entries[idx].client = client;
+			entries[idx].page = i;
+			entries[idx].reg = PMBUS_STATUS_VOUT;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[idx++],
+					    &pmbus_debugfs_ops);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
+			entries[idx].client = client;
+			entries[idx].page = i;
+			entries[idx].reg = PMBUS_STATUS_IOUT;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[idx++],
+					    &pmbus_debugfs_ops);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
+			entries[idx].client = client;
+			entries[idx].page = i;
+			entries[idx].reg = PMBUS_STATUS_INPUT;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[idx++],
+					    &pmbus_debugfs_ops);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
+			entries[idx].client = client;
+			entries[idx].page = i;
+			entries[idx].reg = PMBUS_STATUS_TEMPERATURE;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[idx++],
+					    &pmbus_debugfs_ops);
+		}
+
+		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
+			entries[idx].client = client;
+			entries[idx].page = i;
+			entries[idx].reg = PMBUS_STATUS_CML;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[idx++],
+					    &pmbus_debugfs_ops);
+		}
+
+		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
+			entries[idx].client = client;
+			entries[idx].page = i;
+			entries[idx].reg = PMBUS_STATUS_OTHER;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[idx++],
+					    &pmbus_debugfs_ops);
+		}
+
+		if (pmbus_check_byte_register(client, i,
+					      PMBUS_STATUS_MFR_SPECIFIC)) {
+			entries[idx].client = client;
+			entries[idx].page = i;
+			entries[idx].reg = PMBUS_STATUS_MFR_SPECIFIC;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[idx++],
+					    &pmbus_debugfs_ops);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
+			entries[idx].client = client;
+			entries[idx].page = i;
+			entries[idx].reg = PMBUS_STATUS_FAN_12;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[idx++],
+					    &pmbus_debugfs_ops);
+		}
+
+		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
+			entries[idx].client = client;
+			entries[idx].page = i;
+			entries[idx].reg = PMBUS_STATUS_FAN_34;
+			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[idx++],
+					    &pmbus_debugfs_ops);
+		}
+	}
+
+	return 0;
+}
+#else
+static int pmbus_init_debugfs(struct i2c_client *client,
+			      struct pmbus_data *data)
+{
+	return 0;
+}
+#endif	/* IS_ENABLED(CONFIG_DEBUG_FS) */
+
 int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
 		   struct pmbus_driver_info *info)
 {
@@ -1952,6 +2138,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:
@@ -1965,6 +2155,17 @@ 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);
+	if (pmbus_debugfs_dir && simple_empty(pmbus_debugfs_dir)) {
+		/*
+		 * Remove the root pmbus debugfs directory if we've removed
+		 * all the device directories underneath already.
+		 */
+		debugfs_remove(pmbus_debugfs_dir);
+		pmbus_debugfs_dir = NULL;
+	}
+
 	hwmon_device_unregister(data->hwmon_dev);
 	kfree(data->group.attrs);
 	return 0;
-- 
1.8.3.1

Re: [PATCH v2 4/4] hwmon: (pmbus): Add debugfs for status registers

[Guenter Roeck]

On 08/10/2017 02:57 PM, 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.h      |   6 ++
>   drivers/hwmon/pmbus/pmbus_core.c | 201 +++++++++++++++++++++++++++++++++++++++
>   2 files changed, 207 insertions(+)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
> index bfcb13b..5f91a19 100644
> --- a/drivers/hwmon/pmbus/pmbus.h
> +++ b/drivers/hwmon/pmbus/pmbus.h
> @@ -383,6 +383,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 */
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index ef135d8..b11ebec 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>
> @@ -101,6 +102,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;
>   
> @@ -120,6 +122,12 @@ struct pmbus_data {
>   	u8 currpage;
>   };
>   
> +struct pmbus_debugfs_entry {
> +	struct i2c_client *client;
> +	u8 page;
> +	u8 reg;
> +};
> +
>   void pmbus_clear_cache(struct i2c_client *client)
>   {
>   	struct pmbus_data *data = i2c_get_clientdata(client);
> @@ -1893,6 +1901,184 @@ static int pmbus_regulator_register(struct pmbus_data *data)
>   }
>   #endif
>   
> +static struct dentry *pmbus_debugfs_dir;	/* pmbus debugfs directory */
> +
> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> +static int pmbus_debugfs_get(void *data, u64 *val)
> +{
> +	struct pmbus_debugfs_entry *entry = data;
> +
> +	*val = _pmbus_read_byte_data(entry->client, entry->page, entry->reg);
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops, pmbus_debugfs_get, NULL,
> +			 "0x%02llx\n");
> +
> +static int pmbus_debugfs_get_status(void *data, u64 *val)
> +{
> +	struct pmbus_debugfs_entry *entry = data;
> +	struct pmbus_data *pdata = i2c_get_clientdata(entry->client);
> +
> +	*val = pdata->read_status(entry->client, entry->page);
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops_status, pmbus_debugfs_get_status,
> +			 NULL, "0x%04llx\n");
> +
> +static int pmbus_init_debugfs(struct i2c_client *client,
> +			      struct pmbus_data *data)
> +{
> +	int i, idx = 0;
> +	char name[PMBUS_NAME_SIZE];
> +	struct pmbus_debugfs_entry *entries;
> +
> +	/* Check if client driver requested debugfs support. */
> +	if (!data->info->debugfs)
> +		return 0;
> +

Let's just make this unconditional and drop the flag. I don't see the value
in enabling it per driver. The code is there anyway, might as well support it.

> +	/* Create the root pmbus debugfs directory if it's not created yet. */
> +	if (!pmbus_debugfs_dir) {
> +		pmbus_debugfs_dir = debugfs_create_dir("pmbus", NULL);
> +		if (IS_ERR_OR_NULL(pmbus_debugfs_dir)) {
> +			pmbus_debugfs_dir = NULL;
> +			return -ENODEV;
> +		}
> +	}
> +
> +	/*
> +	 * 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)) {
> +			/* No need to set reg as we have special read op. */
> +			entries[idx].client = client;
> +			entries[idx].page = i;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d", i);

scnprintf might be better.

> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[idx++],
> +					    &pmbus_debugfs_ops_status);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
> +			entries[idx].client = client;
> +			entries[idx].page = i;
> +			entries[idx].reg = PMBUS_STATUS_VOUT;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[idx++],
> +					    &pmbus_debugfs_ops);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
> +			entries[idx].client = client;
> +			entries[idx].page = i;
> +			entries[idx].reg = PMBUS_STATUS_IOUT;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[idx++],
> +					    &pmbus_debugfs_ops);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
> +			entries[idx].client = client;
> +			entries[idx].page = i;
> +			entries[idx].reg = PMBUS_STATUS_INPUT;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[idx++],
> +					    &pmbus_debugfs_ops);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
> +			entries[idx].client = client;
> +			entries[idx].page = i;
> +			entries[idx].reg = PMBUS_STATUS_TEMPERATURE;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[idx++],
> +					    &pmbus_debugfs_ops);
> +		}
> +
> +		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
> +			entries[idx].client = client;
> +			entries[idx].page = i;
> +			entries[idx].reg = PMBUS_STATUS_CML;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[idx++],
> +					    &pmbus_debugfs_ops);
> +		}
> +
> +		if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
> +			entries[idx].client = client;
> +			entries[idx].page = i;
> +			entries[idx].reg = PMBUS_STATUS_OTHER;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[idx++],
> +					    &pmbus_debugfs_ops);
> +		}
> +
> +		if (pmbus_check_byte_register(client, i,
> +					      PMBUS_STATUS_MFR_SPECIFIC)) {
> +			entries[idx].client = client;
> +			entries[idx].page = i;
> +			entries[idx].reg = PMBUS_STATUS_MFR_SPECIFIC;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[idx++],
> +					    &pmbus_debugfs_ops);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
> +			entries[idx].client = client;
> +			entries[idx].page = i;
> +			entries[idx].reg = PMBUS_STATUS_FAN_12;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[idx++],
> +					    &pmbus_debugfs_ops);
> +		}
> +
> +		if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
> +			entries[idx].client = client;
> +			entries[idx].page = i;
> +			entries[idx].reg = PMBUS_STATUS_FAN_34;
> +			snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[idx++],
> +					    &pmbus_debugfs_ops);
> +		}
> +	}
> +
> +	return 0;
> +}
> +#else
> +static int pmbus_init_debugfs(struct i2c_client *client,
> +			      struct pmbus_data *data)
> +{
> +	return 0;
> +}
> +#endif	/* IS_ENABLED(CONFIG_DEBUG_FS) */
> +
>   int pmbus_do_probe(struct i2c_client *client, const struct i2c_device_id *id,
>   		   struct pmbus_driver_info *info)
>   {
> @@ -1952,6 +2138,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:
> @@ -1965,6 +2155,17 @@ 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);
> +	if (pmbus_debugfs_dir && simple_empty(pmbus_debugfs_dir)) {

Almost. Problem is that it is racy against insertions.
We would need a mutex to protect write accesses to pmbus_debugfs_dir.
Wonder if we can just add module_init/module_exit to pmbus_core.c
and initialize/remove the directory from there.

Guenter

> +		/*
> +		 * Remove the root pmbus debugfs directory if we've removed
> +		 * all the device directories underneath already.
> +		 */
> +		debugfs_remove(pmbus_debugfs_dir);
> +		pmbus_debugfs_dir = NULL;
> +	}
> +
>   	hwmon_device_unregister(data->hwmon_dev);
>   	kfree(data->group.attrs);
>   	return 0;
> 

Re: [v2,1/4] hwmon: (pmbus): Switch status registers to 16 bit

[Guenter Roeck]

On Thu, Aug 10, 2017 at 04:57:47PM -0500, eajames@linux.vnet.ibm.com wrote:
> 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>

Applied to hwmon-next.

Thanks,
Guenter

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

Re: [v2,2/4] hwmon: (pmbus): Access word data for STATUS_WORD

[Guenter Roeck]

On Thu, Aug 10, 2017 at 04:57:48PM -0500, eajames@linux.vnet.ibm.com wrote:
> 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>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  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;
>  		}

Re: [v2,3/4] hwmon: (pmbus): Add generic alarm bit for iin and pin

[Guenter Roeck]

On Thu, Aug 10, 2017 at 04:57:49PM -0500, eajames@linux.vnet.ibm.com 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>

Applied to hwmon-next.

Thanks,
Guenter

> ---
>  drivers/hwmon/pmbus/pmbus_core.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/pmbus_core.c b/drivers/hwmon/pmbus/pmbus_core.c
> index 277d170..ef135d8 100644
> --- a/drivers/hwmon/pmbus/pmbus_core.c
> +++ b/drivers/hwmon/pmbus/pmbus_core.c
> @@ -114,6 +114,7 @@ struct pmbus_data {
>  	 */
>  	u16 status[PB_NUM_STATUS_REG];
>  
> +	bool has_status_word;		/* device uses STATUS_WORD register */
>  	int (*read_status)(struct i2c_client *client, int page);
>  
>  	u8 currpage;
> @@ -1045,6 +1046,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,9 +1067,11 @@ 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 &&
> +		    (!upper || (upper && data->has_status_word)) &&
>  		    pmbus_check_status_register(client, page)) {
>  			ret = pmbus_add_boolean(data, name, "alarm", index,
>  						NULL, NULL,
> @@ -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),
>  	}, {
> @@ -1775,6 +1781,8 @@ static int pmbus_init_common(struct i2c_client *client, struct pmbus_data *data,
>  			dev_err(dev, "PMBus status register not found\n");
>  			return -ENODEV;
>  		}
> +	} else {
> +		data->has_status_word = true;
>  	}
>  
>  	/* Enable PEC if the controller supports it */

Re: [PATCH v2 4/4] hwmon: (pmbus): Add debugfs for status registers

[Eddie James]

On 08/11/2017 08:52 AM, Guenter Roeck wrote:
> On 08/10/2017 02:57 PM, 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.h      |   6 ++
>>   drivers/hwmon/pmbus/pmbus_core.c | 201 
>> +++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 207 insertions(+)
>>
>> diff --git a/drivers/hwmon/pmbus/pmbus.h b/drivers/hwmon/pmbus/pmbus.h
>> index bfcb13b..5f91a19 100644
>> --- a/drivers/hwmon/pmbus/pmbus.h
>> +++ b/drivers/hwmon/pmbus/pmbus.h
>> @@ -383,6 +383,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 */
>> diff --git a/drivers/hwmon/pmbus/pmbus_core.c 
>> b/drivers/hwmon/pmbus/pmbus_core.c
>> index ef135d8..b11ebec 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>
>> @@ -101,6 +102,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;
>>   @@ -120,6 +122,12 @@ struct pmbus_data {
>>       u8 currpage;
>>   };
>>   +struct pmbus_debugfs_entry {
>> +    struct i2c_client *client;
>> +    u8 page;
>> +    u8 reg;
>> +};
>> +
>>   void pmbus_clear_cache(struct i2c_client *client)
>>   {
>>       struct pmbus_data *data = i2c_get_clientdata(client);
>> @@ -1893,6 +1901,184 @@ static int pmbus_regulator_register(struct 
>> pmbus_data *data)
>>   }
>>   #endif
>>   +static struct dentry *pmbus_debugfs_dir;    /* pmbus debugfs 
>> directory */
>> +
>> +#if IS_ENABLED(CONFIG_DEBUG_FS)
>> +static int pmbus_debugfs_get(void *data, u64 *val)
>> +{
>> +    struct pmbus_debugfs_entry *entry = data;
>> +
>> +    *val = _pmbus_read_byte_data(entry->client, entry->page, 
>> entry->reg);
>> +
>> +    return 0;
>> +}
>> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops, pmbus_debugfs_get, NULL,
>> +             "0x%02llx\n");
>> +
>> +static int pmbus_debugfs_get_status(void *data, u64 *val)
>> +{
>> +    struct pmbus_debugfs_entry *entry = data;
>> +    struct pmbus_data *pdata = i2c_get_clientdata(entry->client);
>> +
>> +    *val = pdata->read_status(entry->client, entry->page);
>> +
>> +    return 0;
>> +}
>> +DEFINE_DEBUGFS_ATTRIBUTE(pmbus_debugfs_ops_status, 
>> pmbus_debugfs_get_status,
>> +             NULL, "0x%04llx\n");
>> +
>> +static int pmbus_init_debugfs(struct i2c_client *client,
>> +                  struct pmbus_data *data)
>> +{
>> +    int i, idx = 0;
>> +    char name[PMBUS_NAME_SIZE];
>> +    struct pmbus_debugfs_entry *entries;
>> +
>> +    /* Check if client driver requested debugfs support. */
>> +    if (!data->info->debugfs)
>> +        return 0;
>> +
>
> Let's just make this unconditional and drop the flag. I don't see the 
> value
> in enabling it per driver. The code is there anyway, might as well 
> support it.
>
>> +    /* Create the root pmbus debugfs directory if it's not created 
>> yet. */
>> +    if (!pmbus_debugfs_dir) {
>> +        pmbus_debugfs_dir = debugfs_create_dir("pmbus", NULL);
>> +        if (IS_ERR_OR_NULL(pmbus_debugfs_dir)) {
>> +            pmbus_debugfs_dir = NULL;
>> +            return -ENODEV;
>> +        }
>> +    }
>> +
>> +    /*
>> +     * 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)) {
>> +            /* No need to set reg as we have special read op. */
>> +            entries[idx].client = client;
>> +            entries[idx].page = i;
>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d", i);
>
> scnprintf might be better.
>
>> +            debugfs_create_file(name, 0444, data->debugfs,
>> +                        &entries[idx++],
>> +                        &pmbus_debugfs_ops_status);
>> +        }
>> +
>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_VOUT) {
>> +            entries[idx].client = client;
>> +            entries[idx].page = i;
>> +            entries[idx].reg = PMBUS_STATUS_VOUT;
>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_vout", i);
>> +            debugfs_create_file(name, 0444, data->debugfs,
>> +                        &entries[idx++],
>> +                        &pmbus_debugfs_ops);
>> +        }
>> +
>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_IOUT) {
>> +            entries[idx].client = client;
>> +            entries[idx].page = i;
>> +            entries[idx].reg = PMBUS_STATUS_IOUT;
>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_iout", i);
>> +            debugfs_create_file(name, 0444, data->debugfs,
>> +                        &entries[idx++],
>> +                        &pmbus_debugfs_ops);
>> +        }
>> +
>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_INPUT) {
>> +            entries[idx].client = client;
>> +            entries[idx].page = i;
>> +            entries[idx].reg = PMBUS_STATUS_INPUT;
>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_input", i);
>> +            debugfs_create_file(name, 0444, data->debugfs,
>> +                        &entries[idx++],
>> +                        &pmbus_debugfs_ops);
>> +        }
>> +
>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_TEMP) {
>> +            entries[idx].client = client;
>> +            entries[idx].page = i;
>> +            entries[idx].reg = PMBUS_STATUS_TEMPERATURE;
>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_temp", i);
>> +            debugfs_create_file(name, 0444, data->debugfs,
>> +                        &entries[idx++],
>> +                        &pmbus_debugfs_ops);
>> +        }
>> +
>> +        if (pmbus_check_byte_register(client, i, PMBUS_STATUS_CML)) {
>> +            entries[idx].client = client;
>> +            entries[idx].page = i;
>> +            entries[idx].reg = PMBUS_STATUS_CML;
>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_cml", i);
>> +            debugfs_create_file(name, 0444, data->debugfs,
>> +                        &entries[idx++],
>> +                        &pmbus_debugfs_ops);
>> +        }
>> +
>> +        if (pmbus_check_byte_register(client, i, PMBUS_STATUS_OTHER)) {
>> +            entries[idx].client = client;
>> +            entries[idx].page = i;
>> +            entries[idx].reg = PMBUS_STATUS_OTHER;
>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_other", i);
>> +            debugfs_create_file(name, 0444, data->debugfs,
>> +                        &entries[idx++],
>> +                        &pmbus_debugfs_ops);
>> +        }
>> +
>> +        if (pmbus_check_byte_register(client, i,
>> +                          PMBUS_STATUS_MFR_SPECIFIC)) {
>> +            entries[idx].client = client;
>> +            entries[idx].page = i;
>> +            entries[idx].reg = PMBUS_STATUS_MFR_SPECIFIC;
>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_mfr", i);
>> +            debugfs_create_file(name, 0444, data->debugfs,
>> +                        &entries[idx++],
>> +                        &pmbus_debugfs_ops);
>> +        }
>> +
>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN12) {
>> +            entries[idx].client = client;
>> +            entries[idx].page = i;
>> +            entries[idx].reg = PMBUS_STATUS_FAN_12;
>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_fan12", i);
>> +            debugfs_create_file(name, 0444, data->debugfs,
>> +                        &entries[idx++],
>> +                        &pmbus_debugfs_ops);
>> +        }
>> +
>> +        if (data->info->func[i] & PMBUS_HAVE_STATUS_FAN34) {
>> +            entries[idx].client = client;
>> +            entries[idx].page = i;
>> +            entries[idx].reg = PMBUS_STATUS_FAN_34;
>> +            snprintf(name, PMBUS_NAME_SIZE, "status%d_fan34", i);
>> +            debugfs_create_file(name, 0444, data->debugfs,
>> +                        &entries[idx++],
>> +                        &pmbus_debugfs_ops);
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +#else
>> +static int pmbus_init_debugfs(struct i2c_client *client,
>> +                  struct pmbus_data *data)
>> +{
>> +    return 0;
>> +}
>> +#endif    /* IS_ENABLED(CONFIG_DEBUG_FS) */
>> +
>>   int pmbus_do_probe(struct i2c_client *client, const struct 
>> i2c_device_id *id,
>>              struct pmbus_driver_info *info)
>>   {
>> @@ -1952,6 +2138,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:
>> @@ -1965,6 +2155,17 @@ 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);
>> +    if (pmbus_debugfs_dir && simple_empty(pmbus_debugfs_dir)) {
>
> Almost. Problem is that it is racy against insertions.
> We would need a mutex to protect write accesses to pmbus_debugfs_dir.
> Wonder if we can just add module_init/module_exit to pmbus_core.c
> and initialize/remove the directory from there.

Got it, this worked well. Will send up v3 for this patch. Thanks for 
merging the others!

Eddie

>
> Guenter
>
>> +        /*
>> +         * Remove the root pmbus debugfs directory if we've removed
>> +         * all the device directories underneath already.
>> +         */
>> +        debugfs_remove(pmbus_debugfs_dir);
>> +        pmbus_debugfs_dir = NULL;
>> +    }
>> +
>>       hwmon_device_unregister(data->hwmon_dev);
>>       kfree(data->group.attrs);
>>       return 0;
>>
>