hwmon: (ucd9000) Add gpio and debugfs

PATCH Superseded Christopher Bostic 2 participants 9 messages Aug 21, 2017 – Aug 25, 2017 Hardware monitoring

Share & tools

Christopher Bostic

author

2017-08-21 22:09 UTC #0 lore raw

v4 cover hwmon: (ucd9000) Add gpio and debugfs

Add GPIO chip for user space access of the device's IO.
Add debugfs for register dump of mfr_status and individual gpi faults. Add sysfs attribute to clear logged faults.

Christopher Bostic (2):
hwmon: (ucd9000) Add gpio chip interface and clear logged faults
hwmon: (ucd9000) Add debugfs for mfr_status info

 drivers/hwmon/pmbus/ucd9000.c | 250 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 248 insertions(+), 2 deletions(-)

Signature · 1 line

1.8.2.2

Christopher Bostic

author

2017-08-21 22:10 UTC #1 lore raw

PATCH v4 1/2 hwmon: (ucd9000) Add gpio chip interface and clear logged faults

Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.

Present requirements only call for retrieving current state of pin values and their direction. No requirement at this time to switch modes between output and input within the device driver.

Add capability to clear logged faults via sysfs.

Signed-off-by Christopher Bostic <[email protected]>

---
v4 - Change status check from branch to a !! non branching method
- Remove usage comments on libgpiod for the struct gpio_chip methods.
- Clean up some text formatting.
v3 - Correct bug in gpio_chip get method. Wasn't retrieving
gpio config information correctly.
- Remove old debugfs flag from previous pmbus core changes.
- Remove all sysfs files for mfr_status command.
- Add comments on direct i2c_smbus calls to clarify that no page set is required.
v2 - Remove clear_faults file - redundant since all other sysfs
core accesses result in an automatic clear fault.
- Removed local status_word and status_vout register dumps and use the new pmbus core status facilities instead.
- Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
- Add full register dump for mfr_status.
- Add gpio chip to device structure and use gpio interfaces.

---
 drivers/hwmon/pmbus/ucd9000.c | 128 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 2 deletions(-)

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

index 3e3aa95..5cea5f6 100644
@@ -26,6 +26,9 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/i2c/pmbus.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/gpio.h>
 #include "pmbus.h"
 
 enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };

@@ -34,8 +37,16 @@
 #define UCD9000_NUM_PAGES		0xd6
 #define UCD9000_FAN_CONFIG_INDEX	0xe7
 #define UCD9000_FAN_CONFIG		0xe8
+#define UCD9000_LOGGED_FAULTS		0xea
+#define UCD9000_GPIO_SELECT		0xfa
+#define UCD9000_GPIO_CONFIG		0xfb
 #define UCD9000_DEVICE_ID		0xfd
 
+/* GPIO CONFIG bits */
+#define UCD9000_GPIO_CONFIG_ENABLE	BIT(0)
+#define UCD9000_GPIO_CONFIG_OUT_ENABLE	BIT(1)
+#define UCD9000_GPIO_CONFIG_STATUS	BIT(3)
+
 #define UCD9000_MON_TYPE(x)	(((x) >> 5) & 0x07)
 #define UCD9000_MON_PAGE(x)	((x) & 0x0f)

@@ -46,9 +57,12 @@
 
 #define UCD9000_NUM_FAN		4
 
+#define UCD90160_NUM_GPIOS	26
+
 struct ucd9000_data {
 	u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
 	struct pmbus_driver_info info;
+	struct gpio_chip gpio;
 };
 #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)

@@ -119,6 +133,87 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
 };
 MODULE_DEVICE_TABLE(i2c, ucd9000_id);
 
+static int ucd9000_gpio_read_config(struct i2c_client *client,
+				unsigned int offset)
+{
+	int ret;
+
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
+			ret);
+		return ret;
+	}
+
+	return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
+}
+
+static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct i2c_client *client  = gpiochip_get_data(gc);
+	int ret;
+
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO config\n");
+		return ret;
+	}
+
+	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
+}
+
+static int ucd9000_gpio_get_direction(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	struct i2c_client *client  = gpiochip_get_data(gc);
+	int ret;
+
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"failed to read GPIO config\n");
+		return ret;
+	}
+
+	return ret & UCD9000_GPIO_CONFIG_OUT_ENABLE ? 0 : 1;
+}
+
+static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	return 0;
+}
+
+static ssize_t ucd9000_clear_logged_faults(struct device *dev,
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_LOGGED_FAULTS, 0);
+	if (ret) {
+		dev_err(&client->dev, "Failed to clear logged faults\n");
+		return ret;
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
+		ucd9000_clear_logged_faults);
+
+static struct attribute *ucd9000_attributes[] = {
+	&dev_attr_clear_logged_faults.attr,
+	NULL
+};
+
+static const struct attribute_group ucd9000_attr_group = {
+	.attrs = ucd9000_attributes,
+};
+
 static int ucd9000_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {

@@ -227,7 +322,36 @@ static int ucd9000_probe(struct i2c_client *client,
 		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
 	}
 
-	return pmbus_do_probe(client, mid, info);
+	data->gpio.label = "gpio-ucd9000";
+	data->gpio.get_direction = ucd9000_gpio_get_direction;
+	data->gpio.direction_input = ucd9000_gpio_direction_input;
+	data->gpio.get = ucd9000_gpio_get;
+	data->gpio.can_sleep = 1;
+	data->gpio.base = -1;
+	if (mid->driver_data == ucd90160)
+		data->gpio.ngpio = UCD90160_NUM_GPIOS;
+	data->gpio.parent = &client->dev;
+	data->gpio.owner = THIS_MODULE;
+
+	ret = devm_gpiochip_add_data(&client->dev, &data->gpio, client);
+	if (ret) {
+		data->gpio.parent = NULL;
+		dev_warn(&client->dev, "Count not add gpiochip\n");
+		return ret;
+	}
+
+	ret = sysfs_create_group(&client->dev.kobj,
+				&ucd9000_attr_group);
+	if (ret < 0)
+		return ret;
+
+	return  pmbus_do_probe(client, mid, info);
+}
+
+static int ucd9000_remove(struct i2c_client *client)
+{
+	sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
+	return pmbus_do_remove(client);
 }
 
 /* This is the driver that will be inserted */

@@ -236,7 +360,7 @@ static int ucd9000_probe(struct i2c_client *client,
 		.name = "ucd9000",
 	},
 	.probe = ucd9000_probe,
-	.remove = pmbus_do_remove,
+	.remove = ucd9000_remove,
 	.id_table = ucd9000_id,
 };

Signature · 1 line

1.8.2.2

Andrew Jeffery

2017-08-22 02:01 UTC #2 lore raw

Re: [PATCH linux dev-4.10 v4 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults

Hi Chris,

Quoted 102 lines · Christopher Bostic

Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.

Present requirements only call for retrieving current state of pin values and their direction.  No requirement at this time to switch modes between output and input within the device driver.

Add capability to clear logged faults via sysfs.
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>
---
v4 - Change status check from branch to a !! non branching method
   - Remove usage comments on libgpiod for the struct gpio_chip methods.
   - Clean up some text formatting.
v3 - Correct bug in gpio_chip get method.  Wasn't retrieving
     gpio config information correctly.
   - Remove old debugfs flag from previous pmbus core changes.
   - Remove all sysfs files for mfr_status command.
   - Add comments on direct i2c_smbus calls to clarify that no page set is required.
v2 - Remove clear_faults file - redundant since all other sysfs core accesses result in an automatic clear fault.
   - Removed local status_word and status_vout register dumps and use the new pmbus core status facilities instead.
   - Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
   - Add full register dump for mfr_status.
   - Add gpio chip to device structure and use gpio interfaces.
---
drivers/hwmon/pmbus/ucd9000.c | 128 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 2 deletions(-)
drivers/hwmon/pmbus/ucd9000.c +3 -0
index 3e3aa95..5cea5f6 100644
@@ -26,6 +26,9 @@
  #include <linux/slab.h>
  #include <linux/i2c.h>
  #include <linux/i2c/pmbus.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/gpio.h>
  #include "pmbus.h"
enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -34,8 +37,16 @@
Quoted 7 lines
#define UCD9000_NUM_PAGES 0xd6
#define UCD9000_FAN_CONFIG_INDEX 0xe7
#define UCD9000_FAN_CONFIG 0xe8
drivers/hwmon/pmbus/ucd9000.c +3 -0
+#define UCD9000_LOGGED_FAULTS		0xea
+#define UCD9000_GPIO_SELECT		0xfa
+#define UCD9000_GPIO_CONFIG		0xfb
#define UCD9000_DEVICE_ID 0xfd
+/* GPIO CONFIG bits */
drivers/hwmon/pmbus/ucd9000.c line 48
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+#define UCD9000_GPIO_CONFIG_ENABLE	BIT(0)
+#define UCD9000_GPIO_CONFIG_OUT_ENABLE	BIT(1)
+#define UCD9000_GPIO_CONFIG_STATUS	BIT(3)
+
Quoted 2 lines
#define UCD9000_MON_TYPE(x) (((x) >> 5) & 0x07)
#define UCD9000_MON_PAGE(x) ((x) & 0x0f)
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -46,9 +57,12 @@
Quoted 1 line
#define UCD9000_NUM_FAN 4
Quoted 1 line
+#define UCD90160_NUM_GPIOS 26
+
struct ucd9000_data {
drivers/hwmon/pmbus/ucd9000.c line 65
Quoted 3 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct pmbus_driver_info info;
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	struct gpio_chip gpio;
};
#define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -119,6 +133,87 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
  };
  MODULE_DEVICE_TABLE(i2c, ucd9000_id);
+static int ucd9000_gpio_read_config(struct i2c_client *client,
Review
Quoted 1 line
(quoted diff) +1 -0
+				unsigned int offset)
+{
Review
Quoted 1 line
(quoted diff) +1 -0
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 147
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c +7 -0
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
+			ret);
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
Review
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client  = gpiochip_get_data(gc);
+	int ret;
+

Quoted 7 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 161
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO config\n");
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 163
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);

I see you've fixed this case, but didn't audit the rest of the code to fix the remainder. For example the return statement in ucd9000_gpio_get_direction() still branches. Similarly there are error messages that don't report the error return value*, and unnecessary line breaks in in dev_err() calls. In the future, can you please audit your code for classes of problems that are identified in review? I'd hate to have to comment on literally every instance to get them all fixed, as that feels like a waste of my time and a bit patronising towards you.

* Where and when we do this needs some judgement with respect to taste, but it certainly frustrates me when I'm reading logs and I see vague messages like "Failed". What failed? Why?

Quoted 21 lines

drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_get_direction(struct gpio_chip *gc,
Review
Quoted 1 line
(quoted diff) +1 -0
+					unsigned int offset)
+{
Review
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client  = gpiochip_get_data(gc);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 177
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"failed to read GPIO config\n");
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return ret & UCD9000_GPIO_CONFIG_OUT_ENABLE ? 0 : 1;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
Review
Quoted 1 line
(quoted diff) +1 -0
+					unsigned int offset)
+{

Quoted 1 line

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 185
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return 0;

In your reply to my comments on v3 you said you'd implement the direction switching, but this is still unimplemented. What happened? Also I spent some time discussing documentation of assumptions in my reply to Matt, but that seems to not be addressed either. Why is that?

Did you look at a bare-bones pinctrl implementation so we can describe the GPIO ranges appropriately? If you did and it was too much effort then it would be nice to know that, and why.

Quoted 29 lines

drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static ssize_t ucd9000_clear_logged_faults(struct device *dev,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 190
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 193
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 200
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_LOGGED_FAULTS, 0);
+	if (ret) {
+		dev_err(&client->dev, "Failed to clear logged faults\n");
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return count;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+		ucd9000_clear_logged_faults);
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static struct attribute *ucd9000_attributes[] = {
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	&dev_attr_clear_logged_faults.attr,
+	NULL
drivers/hwmon/pmbus/ucd9000.c +3 -0
+};
+
+static const struct attribute_group ucd9000_attr_group = {
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	.attrs = ucd9000_attributes,

drivers/hwmon/pmbus/ucd9000.c

Quoted 2 lines

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

Joel requested the patch be sent upstream to get a decision from the maintainers on this new ABI, and I suggested to you yesterday that it should probably be split out to address ABI changes separate to the GPIO problem, but neither appear to have been done. Why was that?

Andrew

Quoted 50 lines

static int ucd9000_probe(struct i2c_client *client,
Quoted 1 line
const struct i2c_device_id *id)
{
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -227,7 +322,36 @@ static int ucd9000_probe(struct i2c_client *client,
 >  		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
 >  	}
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 334
Quoted 11 lines
drivers/hwmon/pmbus/ucd9000.c +10 -1
-	return pmbus_do_probe(client, mid, info);
+	data->gpio.label = "gpio-ucd9000";
+	data->gpio.get_direction = ucd9000_gpio_get_direction;
+	data->gpio.direction_input = ucd9000_gpio_direction_input;
+	data->gpio.get = ucd9000_gpio_get;
+	data->gpio.can_sleep = 1;
+	data->gpio.base = -1;
+	if (mid->driver_data == ucd90160)
+		data->gpio.ngpio = UCD90160_NUM_GPIOS;
+	data->gpio.parent = &client->dev;
+	data->gpio.owner = THIS_MODULE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 341
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ret = devm_gpiochip_add_data(&client->dev, &data->gpio, client);
+	if (ret) {
+		data->gpio.parent = NULL;
+		dev_warn(&client->dev, "Count not add gpiochip\n");
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 346
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = sysfs_create_group(&client->dev.kobj,
+				&ucd9000_attr_group);
+	if (ret < 0)
+		return ret;
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return  pmbus_do_probe(client, mid, info);
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_remove(struct i2c_client *client)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 354
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
+	return pmbus_do_remove(client);
}

/* This is the driver that will be inserted */
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -236,7 +360,7 @@ static int ucd9000_probe(struct i2c_client *client,
 >  		.name = "ucd9000",
 >  	},
 >  	.probe = ucd9000_probe,
 > -	.remove = pmbus_do_remove,
 > +	.remove = ucd9000_remove,
 >  	.id_table = ucd9000_id,
  };

Christopher Bostic

author

2017-08-24 18:45 UTC #3 lore raw

Re: [PATCH linux dev-4.10 v4 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults

Quoted 120 lines · Andrew Jeffery

Hi Chris,
Quoted 102 lines · Christopher Bostic
Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.
Present requirements only call for retrieving current state of pin values and their direction. No requirement at this time to switch modes between output and input within the device driver.
Add capability to clear logged faults via sysfs.
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>
---
v4 - Change status check from branch to a !! non branching method
- Remove usage comments on libgpiod for the struct gpio_chip methods.
- Clean up some text formatting.
v3 - Correct bug in gpio_chip get method. Wasn't retrieving
gpio config information correctly.
- Remove old debugfs flag from previous pmbus core changes.
- Remove all sysfs files for mfr_status command.
- Add comments on direct i2c_smbus calls to clarify that no page set is required.
v2 - Remove clear_faults file - redundant since all other sysfs
core accesses result in an automatic clear fault.
- Removed local status_word and status_vout register dumps and use the new pmbus core status facilities instead.
- Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
- Add full register dump for mfr_status.
- Add gpio chip to device structure and use gpio interfaces.
---
  drivers/hwmon/pmbus/ucd9000.c | 128 +++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 126 insertions(+), 2 deletions(-)
drivers/hwmon/pmbus/ucd9000.c +3 -0
index 3e3aa95..5cea5f6 100644
@@ -26,6 +26,9 @@
  #include <linux/slab.h>
  #include <linux/i2c.h>
  #include <linux/i2c/pmbus.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/gpio.h>
  #include "pmbus.h"
 
  enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
@@ -34,8 +37,16 @@
drivers/hwmon/pmbus/ucd9000.c line 43
Quoted 7 lines
  #define UCD9000_NUM_PAGES		0xd6
  #define UCD9000_FAN_CONFIG_INDEX	0xe7
  #define UCD9000_FAN_CONFIG		0xe8
drivers/hwmon/pmbus/ucd9000.c +3 -0
+#define UCD9000_LOGGED_FAULTS		0xea
+#define UCD9000_GPIO_SELECT		0xfa
+#define UCD9000_GPIO_CONFIG		0xfb
  #define UCD9000_DEVICE_ID		0xfd
+/* GPIO CONFIG bits */
drivers/hwmon/pmbus/ucd9000.c line 48
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+#define UCD9000_GPIO_CONFIG_ENABLE	BIT(0)
+#define UCD9000_GPIO_CONFIG_OUT_ENABLE	BIT(1)
+#define UCD9000_GPIO_CONFIG_STATUS	BIT(3)
+
Quoted 2 lines
  #define UCD9000_MON_TYPE(x)	(((x) >> 5) & 0x07)
  #define UCD9000_MON_PAGE(x)	((x) & 0x0f)
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -46,9 +57,12 @@
 
Quoted 3 lines
  #define UCD9000_NUM_FAN		4
+#define UCD90160_NUM_GPIOS 26
+
struct ucd9000_data {
drivers/hwmon/pmbus/ucd9000.c line 65
Quoted 3 lines
  	u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
  	struct pmbus_driver_info info;
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	struct gpio_chip gpio;
};
#define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
drivers/hwmon/pmbus/ucd9000.c +3 -0
@@ -119,6 +133,87 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
  };
  MODULE_DEVICE_TABLE(i2c, ucd9000_id);
 
+static int ucd9000_gpio_read_config(struct i2c_client *client,
 > +				unsigned int offset)
+{
 > +	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 147
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c +7 -0
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
+			ret);
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
Review
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client  = gpiochip_get_data(gc);
+	int ret;
+
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 161
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO config\n");
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 163
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
I see you've fixed this case, but didn't audit the rest of the code to fix the remainder. For example the return statement in ucd9000_gpio_get_direction() still branches. Similarly there are error messages that don't report the error return value*, and unnecessary line breaks in in dev_err() calls. In the future, can you please audit your code for classes of problems that are identified in review? I'd hate to have to comment on literally every instance to get them all fixed, as that feels like a waste of my time and a bit patronising towards you.

Quoted 3 lines

* Where and when we do this needs some judgement with respect to taste, but it certainly frustrates me when I'm reading logs and I see vague messages like "Failed". What failed? Why?

Hi Andrew,

OK, I'll be sure to be more thorough with respect to auditing as you suggest and will address these items for next version.

Quoted 19 lines

Quoted 19 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_get_direction(struct gpio_chip *gc,
Review
Quoted 1 line
(quoted diff) +1 -0
+					unsigned int offset)
+{
Review
Quoted 2 lines
(quoted diff) +2 -0
+	struct i2c_client *client  = gpiochip_get_data(gc);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 177
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"failed to read GPIO config\n");
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return ret & UCD9000_GPIO_CONFIG_OUT_ENABLE ? 0 : 1;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_direction_input(struct gpio_chip *gc,

Quoted 5 lines

Quoted 3 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+					unsigned int offset)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 185
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return 0;
In your reply to my comments on v3 you said you'd implement the direction switching, but this is still unimplemented. What happened?

Based on discussions in our meeting that followed my replies we had decided to only implement what was required for our implementation (non upstream OpenBMC). Direction checks weren't added as a result. For next pass I will add them.

Quoted 3 lines

Also I spent some
time discussing documentation of assumptions in my reply to Matt, but that seems to not be addressed either. Why is that?

Some basic notes on assumptions were added to the patch set cover letter. I hadn't understood this was to be commented within the code. I'll do that for next version.

Quoted 3 lines

Did you look at a bare-bones pinctrl implementation so we can describe the GPIO ranges appropriately? If you did and it was too much effort then it would be nice to know that, and why.

I haven't looked into pinctrl implementation in very much depth yet. I'd like to implement what the user requires for now and phase pinctrl in later. It might be trivial but regardless of that fact there will, I assume, be some refining of any implementation requiring further revisions/time.

Quoted 28 lines

Quoted 28 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static ssize_t ucd9000_clear_logged_faults(struct device *dev,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 190
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+				struct device_attribute *attr, const char *buf,
+				size_t count)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 193
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	struct i2c_client *client = to_i2c_client(dev);
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 200
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_LOGGED_FAULTS, 0);
+	if (ret) {
+		dev_err(&client->dev, "Failed to clear logged faults\n");
+		return ret;
+	}
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return count;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+		ucd9000_clear_logged_faults);
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static struct attribute *ucd9000_attributes[] = {
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	&dev_attr_clear_logged_faults.attr,
+	NULL
drivers/hwmon/pmbus/ucd9000.c +3 -0
+};
+
+static const struct attribute_group ucd9000_attr_group = {

Quoted 7 lines

drivers/hwmon/pmbus/ucd9000.c
Quoted 3 lines
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	.attrs = ucd9000_attributes,
drivers/hwmon/pmbus/ucd9000.c +2 -0
+};
+
Joel requested the patch be sent upstream to get a decision from the maintainers on this new ABI, and I suggested to you yesterday that it should probably be split out to address ABI changes separate to the GPIO problem, but neither appear to have been done. Why was that?

I have done the separate ABI changes but was interrupted by other unrelated high priority work before I could send out to maintainers. It is on its way, I have not forgotten.

Chris

Quoted 53 lines

Andrew
Quoted 50 lines
static int ucd9000_probe(struct i2c_client *client,
Quoted 1 line
const struct i2c_device_id *id)
{
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -227,7 +322,36 @@ static int ucd9000_probe(struct i2c_client *client,
 >   		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
 >   	}
 
 > -	return pmbus_do_probe(client, mid, info);
 > +	data->gpio.label = "gpio-ucd9000";
 > +	data->gpio.get_direction = ucd9000_gpio_get_direction;
 > +	data->gpio.direction_input = ucd9000_gpio_direction_input;
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 334
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c +7 -0
+	data->gpio.get = ucd9000_gpio_get;
+	data->gpio.can_sleep = 1;
+	data->gpio.base = -1;
+	if (mid->driver_data == ucd90160)
+		data->gpio.ngpio = UCD90160_NUM_GPIOS;
+	data->gpio.parent = &client->dev;
+	data->gpio.owner = THIS_MODULE;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 341
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ret = devm_gpiochip_add_data(&client->dev, &data->gpio, client);
+	if (ret) {
+		data->gpio.parent = NULL;
+		dev_warn(&client->dev, "Count not add gpiochip\n");
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 346
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = sysfs_create_group(&client->dev.kobj,
+				&ucd9000_attr_group);
+	if (ret < 0)
+		return ret;
+
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return  pmbus_do_probe(client, mid, info);
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_remove(struct i2c_client *client)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 354
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
+	return pmbus_do_remove(client);
}
/* This is the driver that will be inserted */
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -236,7 +360,7 @@ static int ucd9000_probe(struct i2c_client *client,
 >   		.name = "ucd9000",
 >   	},
 >   	.probe = ucd9000_probe,
 > -	.remove = pmbus_do_remove,
 > +	.remove = ucd9000_remove,
 >   	.id_table = ucd9000_id,
  };

Andrew Jeffery

2017-08-25 11:40 UTC #4 lore raw

Re: [PATCH linux dev-4.10 v4 1/2] hwmon: (ucd9000) Add gpio chip interface and clear logged faults

Quoted 139 lines · Christopher Bostic

Quoted 124 lines · Andrew Jeffery
Hi Chris,
Quoted 121 lines · Christopher Bostic
Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.

Present requirements only call for retrieving current state of pin values and their direction.  No requirement at this time to switch modes between output and input within the device driver.

Add capability to clear logged faults via sysfs.
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>

---
v4 - Change status check from branch to a !! non branching method
    - Remove usage comments on libgpiod for the struct gpio_chip methods.
    - Clean up some text formatting.
v3 - Correct bug in gpio_chip get method.  Wasn't retrieving
      gpio config information correctly.
    - Remove old debugfs flag from previous pmbus core changes.
    - Remove all sysfs files for mfr_status command.
    - Add comments on direct i2c_smbus calls to clarify that no page set is required.
v2 - Remove clear_faults file - redundant since all other sysfs core accesses result in an automatic clear fault.
    - Removed local status_word and status_vout register dumps and use the new pmbus core status facilities instead.
    - Rename gpi<x>_fault to gpi<x>_alarm to better match core naming conventions.
    - Add full register dump for mfr_status.
    - Add gpio chip to device structure and use gpio interfaces.
---
drivers/hwmon/pmbus/ucd9000.c | 128 +++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 126 insertions(+), 2 deletions(-)
drivers/hwmon/pmbus/ucd9000.c +3 -0
index 3e3aa95..5cea5f6 100644
@@ -26,6 +26,9 @@
   #include <linux/slab.h>
   #include <linux/i2c.h>
   #include <linux/i2c/pmbus.h>
+#include <linux/sysfs.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/gpio.h>
   #include "pmbus.h"
enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -34,8 +37,16 @@
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c line 42
Quoted 6 lines
#define UCD9000_NUM_PAGES 0xd6
#define UCD9000_FAN_CONFIG_INDEX 0xe7
#define UCD9000_FAN_CONFIG 0xe8
drivers/hwmon/pmbus/ucd9000.c +3 -0
+#define UCD9000_LOGGED_FAULTS		0xea
+#define UCD9000_GPIO_SELECT		0xfa
+#define UCD9000_GPIO_CONFIG		0xfb
#define UCD9000_DEVICE_ID 0xfd
+/* GPIO CONFIG bits */
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c line 47
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+#define UCD9000_GPIO_CONFIG_ENABLE	BIT(0)
+#define UCD9000_GPIO_CONFIG_OUT_ENABLE	BIT(1)
+#define UCD9000_GPIO_CONFIG_STATUS BIT(3)
+
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
#define UCD9000_MON_TYPE(x) (((x) >> 5) & 0x07)
#define UCD9000_MON_PAGE(x) ((x) & 0x0f)
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -46,9 +57,12 @@
Quoted 1 line
#define UCD9000_NUM_FAN 4

Quoted 1 line
+#define UCD90160_NUM_GPIOS 26

+
struct ucd9000_data {
drivers/hwmon/pmbus/ucd9000.c line 65
Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct pmbus_driver_info info;
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	struct gpio_chip gpio;
};
#define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -119,6 +133,87 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
   };
   MODULE_DEVICE_TABLE(i2c, ucd9000_id);
+static int ucd9000_gpio_read_config(struct i2c_client *client,
Review
Quoted 1 line
(quoted diff) +1 -0
+				unsigned int offset)
+{
Review
Quoted 1 line
(quoted diff) +1 -0
+	int ret;
+
Review
Quoted 7 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
+			ret);
+		return ret;
(quoted diff) +1 -0
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 149
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+	struct i2c_client *client  = gpiochip_get_data(gc);
(quoted diff) +1 -0
+	int ret;
+
Review
Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read GPIO config\n");
+		return ret;
(quoted diff) +1 -0
+	}
+
Quoted 13 lines
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 163
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
I see you've fixed this case, but didn't audit the rest of the code to fix the remainder. For example the return statement in ucd9000_gpio_get_direction() still branches. Similarly there are error messages that don't report the error return value*, and unnecessary line breaks in in dev_err() calls. In the future, can you please audit your code for classes of problems that are identified in review? I'd hate to have to comment on literally every instance to get them all fixed, as that feels like a waste of my time and a bit patronising towards you.
* Where and when we do this needs some judgement with respect to taste, but it certainly frustrates me when I'm reading logs and I see vague messages like "Failed". What failed? Why?

Quoted 5 lines

Hi Andrew,

OK, I'll be sure to be more thorough with respect to auditing as you suggest and will address these items for next version.

Thanks.

Quoted 31 lines

Quoted 23 lines
Quoted 23 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_get_direction(struct gpio_chip *gc,
Review
Quoted 1 line
(quoted diff) +1 -0
+					unsigned int offset)
+{
Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
(quoted diff) +1 -0
+	struct i2c_client *client  = gpiochip_get_data(gc);
(quoted diff) +1 -0
+	int ret;
+
Review
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"failed to read GPIO config\n");
+		return ret;
(quoted diff) +1 -0
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 179
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return ret & UCD9000_GPIO_CONFIG_OUT_ENABLE ? 0 : 1;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
Quoted 7 lines
Quoted 4 lines
Review
Quoted 1 line
(quoted diff) +1 -0
+					unsigned int offset)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 185
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return 0;
In your reply to my comments on v3 you said you'd implement the direction switching, but this is still unimplemented. What happened?

Quoted 5 lines

Based on discussions in our meeting that followed my replies we had decided to only implement what was required for our implementation (non upstream OpenBMC). Direction checks weren't added as a result. For next pass I will add them.

Right; that wasn't my understanding, or at least not what I intended to convey. We've spent more time discussing whether to implement it than it should take to do. Implementing it removes a pretty large assumption in the code so I think it is worthwhile.

Quoted 8 lines

Quoted 3 lines
Also I spent some
time discussing documentation of assumptions in my reply to Matt, but that seems to not be addressed either. Why is that?

Some basic notes on assumptions were added to the patch set cover letter. I hadn't understood this was to be commented within the code. I'll do that for next version.

Thanks. Explicitly documenting the assumptions in the code will help when we get around to improving it to the point we can send it upstream.

Quoted 9 lines

Quoted 3 lines
Did you look at a bare-bones pinctrl implementation so we can describe the GPIO ranges appropriately? If you did and it was too much effort then it would be nice to know that, and why.

I haven't looked into pinctrl implementation in very much depth yet. I'd like to implement what the user requires for now and phase pinctrl in later. It might be trivial but regardless of that fact there will, I assume, be some refining of any implementation requiring further revisions/time.

Yeah, this was always a stretch request. However, like the direction issues above, it would eliminate some pretty hefty assumptions from the code.

Quoted 44 lines

Quoted 34 lines
Quoted 34 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static ssize_t ucd9000_clear_logged_faults(struct device *dev,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 190
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+				struct device_attribute *attr, const char *buf,
drivers/hwmon/pmbus/ucd9000.c +1 -0
+				size_t count)
+{
Review
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	struct i2c_client *client = to_i2c_client(dev);
(quoted diff) +1 -0
+	int ret;
+
Review
Quoted 6 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	/* No page set required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_LOGGED_FAULTS, 0);
+	if (ret) {
+		dev_err(&client->dev, "Failed to clear logged faults\n");
+		return ret;
(quoted diff) +1 -0
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 202
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	return count;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+
+static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 206
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+		ucd9000_clear_logged_faults);
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static struct attribute *ucd9000_attributes[] = {
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 210
Quoted 2 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	&dev_attr_clear_logged_faults.attr,
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	NULL
drivers/hwmon/pmbus/ucd9000.c +3 -0
+};
+
+static const struct attribute_group ucd9000_attr_group = {
Quoted 9 lines
drivers/hwmon/pmbus/ucd9000.c
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg) · line 214
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	.attrs = ucd9000_attributes,
drivers/hwmon/pmbus/ucd9000.c +2 -0
+};
+
Joel requested the patch be sent upstream to get a decision from the maintainers on this new ABI, and I suggested to you yesterday that it should probably be split out to address ABI changes separate to the GPIO problem, but neither appear to have been done. Why was that?

Quoted 4 lines

I have done the separate ABI changes but was interrupted by other unrelated high priority work before I could send out to maintainers. It is on its way, I have not forgotten.

Great. Thanks.

Andrew

Christopher Bostic

author

2017-08-21 22:10 UTC #5 lore raw

PATCH v4 2/2 hwmon: (ucd9000) Add debugfs for mfr_status info

In reply to Christopher Bostic

Add debugfs to list the mfr_status register and its gpiN_fault fields.

Signed-off-by Christopher Bostic <[email protected]>

---
v2 - Remove mfr_status directory in debugfs and place the files
up in the parent ucd9000 directory.

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

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

index 5cea5f6..f906d8e 100644
@@ -19,6 +19,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/init.h>

@@ -38,6 +39,7 @@
 #define UCD9000_FAN_CONFIG_INDEX	0xe7
 #define UCD9000_FAN_CONFIG		0xe8
 #define UCD9000_LOGGED_FAULTS		0xea
+#define UCD9000_MFR_STATUS		0xf3
 #define UCD9000_GPIO_SELECT		0xfa
 #define UCD9000_GPIO_CONFIG		0xfb
 #define UCD9000_DEVICE_ID		0xfd

@@ -56,16 +58,25 @@
 #define UCD9000_MON_VOLTAGE_HW	4
 
 #define UCD9000_NUM_FAN		4
+#define UCD9000_NAME_SIZE	24
 
 #define UCD90160_NUM_GPIOS	26
+#define UCD90160_GPI_COUNT	8
+#define UCD90160_GPI_FAULT_BASE	16
 
 struct ucd9000_data {
 	u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
 	struct pmbus_driver_info info;
 	struct gpio_chip gpio;
+	struct dentry *debugfs;	/* debugfs device directory */
 };
 #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
 
+struct ucd9000_debugfs_entry {
+	struct i2c_client *client;
+	u8 index;
+};
+
 static int ucd9000_get_fan_config(struct i2c_client *client, int fan)
 {
 	int fan_config = 0;

@@ -143,6 +154,7 @@ static int ucd9000_gpio_read_config(struct i2c_client *client,
 	if (ret < 0) {
 		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
 			ret);
+
 		return ret;
 	}

@@ -157,6 +169,7 @@ static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
 	ret = ucd9000_gpio_read_config(client, offset);
 	if (ret < 0) {
 		dev_err(&client->dev, "failed to read GPIO config\n");
+
 		return ret;
 	}

@@ -185,6 +198,108 @@ static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
 	return 0;
 }
 
+static struct dentry *ucd9000_debugfs_dir;
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static int ucd9000_get_mfr_status(struct i2c_client *client, u32 *buffer)
+{
+	int ret;
+
+	ret = pmbus_set_page(client, 0);
+	if (ret < 0) {
+		dev_err(&client->dev, "pmbus_set_page failed. rc:%d\n", ret);
+		return ret;
+	}
+	return i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS,
+					(u8 *)buffer);
+}
+
+static int ucd9000_debugfs_get_mfr_status_bit(void *data, u64 *val)
+{
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	int nr = entry->index;
+	u32 buffer;
+	int ret;
+
+	ret = ucd9000_get_mfr_status(client, &buffer);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to read mfr status. rc:%d\n", ret);
+		return ret;
+	}
+
+	*val = ret & BIT(nr) ? 1 : 0;
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_bit,
+			ucd9000_debugfs_get_mfr_status_bit, NULL, "%1lld\n");
+
+static int ucd9000_debugfs_get_mfr_status_word(void *data, u64 *val)
+{
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	u32 buffer;
+	int ret;
+
+	ret = ucd9000_get_mfr_status(client, &buffer);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to read mfr status. rc:%d\n", ret);
+		return ret;
+	}
+
+	*val = ret;
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_word,
+			ucd9000_debugfs_get_mfr_status_word, NULL, "%08llx\n");
+
+static int ucd9000_init_debugfs(struct i2c_client *client,
+				struct ucd9000_data *data)
+{
+	struct ucd9000_debugfs_entry *entries;
+	char name[UCD9000_NAME_SIZE];
+	int i;
+
+	ucd9000_debugfs_dir = debugfs_create_dir("ucd9000", NULL);
+	if (IS_ERR(ucd9000_debugfs_dir)) {
+		ucd9000_debugfs_dir = NULL;
+		dev_warn(&client->dev, "Failed to create debugfs dir\n");
+		return 0;
+	}
+
+	entries = devm_kzalloc(&client->dev, sizeof(*entries) *
+				(UCD90160_GPI_COUNT + 1) * 10,
+				GFP_KERNEL);
+	if (!entries)
+		return -ENOMEM;
+
+	for (i = 0; i < UCD90160_GPI_COUNT; i++) {
+		entries[i].client = client;
+		entries[i].index = UCD90160_GPI_FAULT_BASE + i;
+		scnprintf(name, UCD9000_NAME_SIZE, "gpi%d_alarm", i+1);
+		debugfs_create_file(name, 0444, ucd9000_debugfs_dir,
+				&entries[i],
+				&ucd9000_debugfs_ops_mfr_status_bit);
+	}
+	entries[i].client = client;
+	scnprintf(name, UCD9000_NAME_SIZE, "mfr_status");
+	debugfs_create_file(name, 0444, ucd9000_debugfs_dir, &entries[i],
+				&ucd9000_debugfs_ops_mfr_status_word);
+
+	return 0;
+}
+#else
+static int ucd9000_init_debugfs(struct i2c_client *client,
+				struct ucd9000_data *data)
+{
+	return 0;
+}
+#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
+
 static ssize_t ucd9000_clear_logged_faults(struct device *dev,
 				struct device_attribute *attr, const char *buf,
 				size_t count)

@@ -345,12 +460,19 @@ static int ucd9000_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
+	if (mid->driver_data == ucd90160) {
+		ret = ucd9000_init_debugfs(client, data);
+		if (ret < 0)
+			dev_warn(&client->dev, "Failed to register debugfs\n");
+	}
+
 	return  pmbus_do_probe(client, mid, info);
 }
 
 static int ucd9000_remove(struct i2c_client *client)
 {
 	sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
+	debugfs_remove_recursive(ucd9000_debugfs_dir);
 	return pmbus_do_remove(client);
 }

Signature · 1 line

1.8.2.2

Andrew Jeffery

2017-08-22 02:01 UTC #6 lore raw

Re: [PATCH linux dev-4.10 v4 2/2] hwmon: (ucd9000) Add debugfs for mfr_status info

Quoted 58 lines · Christopher Bostic

Add debugfs to list the mfr_status register and its gpiN_fault fields.
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>
---
v2 - Remove mfr_status directory in debugfs and place the files
up in the parent ucd9000 directory.
---
drivers/hwmon/pmbus/ucd9000.c | 122 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 122 insertions(+)
drivers/hwmon/pmbus/ucd9000.c +0 -0
index 5cea5f6..f906d8e 100644
@@ -19,6 +19,7 @@
   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
   */
+#include <linux/debugfs.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -38,6 +39,7 @@
Quoted 7 lines
#define UCD9000_FAN_CONFIG_INDEX 0xe7
#define UCD9000_FAN_CONFIG 0xe8
#define UCD9000_LOGGED_FAULTS 0xea
+#define UCD9000_MFR_STATUS 0xf3
#define UCD9000_GPIO_SELECT 0xfa
#define UCD9000_GPIO_CONFIG 0xfb
#define UCD9000_DEVICE_ID 0xfd
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -56,16 +58,25 @@
Quoted 1 line
#define UCD9000_MON_VOLTAGE_HW 4
Quoted 2 lines
#define UCD9000_NUM_FAN 4
+#define UCD9000_NAME_SIZE 24
drivers/hwmon/pmbus/ucd9000.c line 65
Quoted 3 lines
#define UCD90160_NUM_GPIOS 26
drivers/hwmon/pmbus/ucd9000.c +2 -0
+#define UCD90160_GPI_COUNT	8
+#define UCD90160_GPI_FAULT_BASE	16
struct ucd9000_data {
Quoted 4 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct pmbus_driver_info info;
struct gpio_chip gpio;
drivers/hwmon/pmbus/ucd9000.c line 71
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	struct dentry *debugfs;	/* debugfs device directory */
};
#define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
+struct ucd9000_debugfs_entry {
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	struct i2c_client *client;
+	u8 index;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+};
+
static int ucd9000_get_fan_config(struct i2c_client *client, int fan) {
Quoted 1 line
int fan_config = 0;

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_read_config(struct i2c_client *client, · line 157

Quoted 4 lines

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

@@ -143,6 +154,7 @@ static int ucd9000_gpio_read_config(struct i2c_client *client,
 >  	if (ret < 0) {
 >  		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
 >  			ret);
+

Can we fold the whitespace cleanups into the appropriate patch? Please audit the rest of the patch for similar issues.

Quoted 22 lines

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

 >  		return ret;
 >  	}

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

@@ -157,6 +169,7 @@ static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
 >  	ret = ucd9000_gpio_read_config(client, offset);
 >  	if (ret < 0) {
 >  		dev_err(&client->dev, "failed to read GPIO config\n");
+
 >  		return ret;
 >  	}

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

@@ -185,6 +198,108 @@ static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
 >  	return 0;
  }

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

+static struct dentry *ucd9000_debugfs_dir;
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static int ucd9000_get_mfr_status(struct i2c_client *client, u32 *buffer)
+{

Review

Quoted 1 line

(quoted diff) +1 -0
+	int ret;

Quoted 7 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 214
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c +7 -0
+	ret = pmbus_set_page(client, 0);
+	if (ret < 0) {
+		dev_err(&client->dev, "pmbus_set_page failed. rc:%d\n", ret);
+		return ret;
+	}
+	return i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS,
+					(u8 *)buffer);

I know you don't intend to support the device but this will cause stack corruption on the ucd90240. Maybe it's not worth fixing at the moment, but it's probably worth a comment. From the command reference, page 81:

With the UCD90120 and UCD90124 devices, this command [MFR_STATUS] is 2 bytes long (bits 0-15). With the UCD90240, this command is 5 bytes long. With all other devices, it is 4 bytes long.

Quoted 10 lines

drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_debugfs_get_mfr_status_bit(void *data, u64 *val)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 223
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	int nr = entry->index;
+	u32 buffer;
+	int ret;
+

Quoted 8 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 230
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ret = ucd9000_get_mfr_status(client, &buffer);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to read mfr status. rc:%d\n", ret);
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 232
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	*val = ret & BIT(nr) ? 1 : 0;

We should switch this to the non-branchy solution.

Quoted 47 lines

+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_bit,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+			ucd9000_debugfs_get_mfr_status_bit, NULL, "%1lld\n");
drivers/hwmon/pmbus/ucd9000.c +3 -0
+
+static int ucd9000_debugfs_get_mfr_status_word(void *data, u64 *val)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 223
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	u32 buffer;
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 230
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ret = ucd9000_get_mfr_status(client, &buffer);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to read mfr status. rc:%d\n", ret);
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 253
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	*val = ret;
+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_word,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+			ucd9000_debugfs_get_mfr_status_word, NULL, "%08llx\n");
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static int ucd9000_init_debugfs(struct i2c_client *client,
Review
Quoted 1 line
(quoted diff) +1 -0
+				struct ucd9000_data *data)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 265
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+	struct ucd9000_debugfs_entry *entries;
+	char name[UCD9000_NAME_SIZE];
+	int i;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 272
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ucd9000_debugfs_dir = debugfs_create_dir("ucd9000", NULL);
+	if (IS_ERR(ucd9000_debugfs_dir)) {
+		ucd9000_debugfs_dir = NULL;
+		dev_warn(&client->dev, "Failed to create debugfs dir\n");
+		return 0;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 278
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	entries = devm_kzalloc(&client->dev, sizeof(*entries) *
+				(UCD90160_GPI_COUNT + 1) * 10,
+				GFP_KERNEL);
+	if (!entries)
+		return -ENOMEM;
+

Quoted 12 lines

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

Quoted 12 lines

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

+	for (i = 0; i < UCD90160_GPI_COUNT; i++) {
+		entries[i].client = client;
+		entries[i].index = UCD90160_GPI_FAULT_BASE + i;
+		scnprintf(name, UCD9000_NAME_SIZE, "gpi%d_alarm", i+1);
+		debugfs_create_file(name, 0444, ucd9000_debugfs_dir,
+				&entries[i],
+				&ucd9000_debugfs_ops_mfr_status_bit);
+	}
+	entries[i].client = client;
+	scnprintf(name, UCD9000_NAME_SIZE, "mfr_status");
+	debugfs_create_file(name, 0444, ucd9000_debugfs_dir, &entries[i],
+				&ucd9000_debugfs_ops_mfr_status_word);

Given the static ucd9000_debugfs_dir, how does this work if there are multiple devices? It's a naive question - I haven't looked into it at all, but I'm curious.

Cheers,

Andrew

Quoted 34 lines

+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+#else
+static int ucd9000_init_debugfs(struct i2c_client *client,
Review
Quoted 1 line
(quoted diff) +1 -0
+				struct ucd9000_data *data)
+{
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
+
static ssize_t ucd9000_clear_logged_faults(struct device *dev,
Quoted 2 lines
struct device_attribute *attr, const char *buf, size_t count)
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -345,12 +460,19 @@ static int ucd9000_probe(struct i2c_client *client,
 >  	if (ret < 0)
 >  		return ret;
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 467
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	if (mid->driver_data == ucd90160) {
+		ret = ucd9000_init_debugfs(client, data);
+		if (ret < 0)
+			dev_warn(&client->dev, "Failed to register debugfs\n");
+	}
+
Quoted 1 line
return pmbus_do_probe(client, mid, info);
}

static int ucd9000_remove(struct i2c_client *client) {
Quoted 3 lines
sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	debugfs_remove_recursive(ucd9000_debugfs_dir);
return pmbus_do_remove(client);
}

Christopher Bostic

author

2017-08-24 18:53 UTC #7 lore raw

Re: [PATCH linux dev-4.10 v4 2/2] hwmon: (ucd9000) Add debugfs for mfr_status info

Quoted 57 lines · Andrew Jeffery

Quoted 56 lines · Christopher Bostic
Add debugfs to list the mfr_status register and its gpiN_fault fields.
Quoted 1 line
Signed-off-by Christopher Bostic <[email protected]>
---
v2 - Remove mfr_status directory in debugfs and place the files
up in the parent ucd9000 directory.
---
  drivers/hwmon/pmbus/ucd9000.c | 122 ++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 122 insertions(+)
drivers/hwmon/pmbus/ucd9000.c +1 -0
index 5cea5f6..f906d8e 100644
@@ -19,6 +19,7 @@
   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
   */
 
+#include <linux/debugfs.h>
  #include <linux/kernel.h>
  #include <linux/module.h>
  #include <linux/init.h>
@@ -38,6 +39,7 @@
Quoted 7 lines
#define UCD9000_FAN_CONFIG_INDEX 0xe7
#define UCD9000_FAN_CONFIG 0xe8
#define UCD9000_LOGGED_FAULTS 0xea
+#define UCD9000_MFR_STATUS 0xf3
#define UCD9000_GPIO_SELECT 0xfa
#define UCD9000_GPIO_CONFIG 0xfb
#define UCD9000_DEVICE_ID 0xfd
drivers/hwmon/pmbus/ucd9000.c +0 -0
@@ -56,16 +58,25 @@
drivers/hwmon/pmbus/ucd9000.c line 65
Quoted 8 lines
  #define UCD9000_MON_VOLTAGE_HW	4
  
  #define UCD9000_NUM_FAN		4
drivers/hwmon/pmbus/ucd9000.c +3 -0
+#define UCD9000_NAME_SIZE	24
 
  #define UCD90160_NUM_GPIOS	26
+#define UCD90160_GPI_COUNT	8
+#define UCD90160_GPI_FAULT_BASE	16
struct ucd9000_data {
Quoted 4 lines
u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX]; struct pmbus_driver_info info;
struct gpio_chip gpio;
drivers/hwmon/pmbus/ucd9000.c line 71
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	struct dentry *debugfs;	/* debugfs device directory */
};
#define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
+struct ucd9000_debugfs_entry {
Quoted 2 lines
drivers/hwmon/pmbus/ucd9000.c +2 -0
+	struct i2c_client *client;
+	u8 index;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+};
+
  static int ucd9000_get_fan_config(struct i2c_client *client, int fan)
  {

Quoted 8 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_read_config(struct i2c_client *client, · line 157
Quoted 6 lines
Quoted 1 line
  	int fan_config = 0;
drivers/hwmon/pmbus/ucd9000.c +1 -0
@@ -143,6 +154,7 @@ static int ucd9000_gpio_read_config(struct i2c_client *client,
 >   	if (ret < 0) {
 >   		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
 >   			ret);
+
Can we fold the whitespace cleanups into the appropriate patch? Please audit the rest of the patch for similar issues.

Yes, will do that.

Quoted 33 lines

Quoted 24 lines
drivers/hwmon/pmbus/ucd9000.c +7 -0
 >   		return ret;
 >   	}
 
@@ -157,6 +169,7 @@ static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
 >   	ret = ucd9000_gpio_read_config(client, offset);
 >   	if (ret < 0) {
 >   		dev_err(&client->dev, "failed to read GPIO config\n");
+
 >   		return ret;
 >   	}
 
@@ -185,6 +198,108 @@ static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
 >   	return 0;
  }
 
+static struct dentry *ucd9000_debugfs_dir;
+
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static int ucd9000_get_mfr_status(struct i2c_client *client, u32 *buffer)
+{
 > +	int ret;
+
 > +	ret = pmbus_set_page(client, 0);
 > +	if (ret < 0) {
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 214
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+		dev_err(&client->dev, "pmbus_set_page failed. rc:%d\n", ret);
+		return ret;
+	}
+	return i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS,
+					(u8 *)buffer);
I know you don't intend to support the device but this will cause stack corruption on the ucd90240. Maybe it's not worth fixing at the moment, but it's probably worth a comment. From the command reference, page 81:

Quoted 3 lines

With the UCD90120 and UCD90124 devices, this command [MFR_STATUS] is 2 bytes long (bits 0-15). With the UCD90240, this command is 5 bytes long. With all other devices, it is 4 bytes long.

Will add a comment.

Quoted 18 lines

Quoted 18 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+}
+
+static int ucd9000_debugfs_get_mfr_status_bit(void *data, u64 *val)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 223
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	int nr = entry->index;
+	u32 buffer;
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 230
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ret = ucd9000_get_mfr_status(client, &buffer);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to read mfr status. rc:%d\n", ret);
+		return ret;
+	}
+

Quoted 2 lines

Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 232
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	*val = ret & BIT(nr) ? 1 : 0;
We should switch this to the non-branchy solution.

Will fix.

Quoted 48 lines

Quoted 48 lines
+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_bit,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+			ucd9000_debugfs_get_mfr_status_bit, NULL, "%1lld\n");
drivers/hwmon/pmbus/ucd9000.c +3 -0
+
+static int ucd9000_debugfs_get_mfr_status_word(void *data, u64 *val)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 223
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	u32 buffer;
+	int ret;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 230
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ret = ucd9000_get_mfr_status(client, &buffer);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to read mfr status. rc:%d\n", ret);
+		return ret;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 253
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	*val = ret;
+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +2 -0
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_ops_mfr_status_word,
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+			ucd9000_debugfs_get_mfr_status_word, NULL, "%08llx\n");
drivers/hwmon/pmbus/ucd9000.c +2 -0
+
+static int ucd9000_init_debugfs(struct i2c_client *client,
Review
Quoted 1 line
(quoted diff) +1 -0
+				struct ucd9000_data *data)
+{
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 265
Quoted 3 lines
drivers/hwmon/pmbus/ucd9000.c +3 -0
+	struct ucd9000_debugfs_entry *entries;
+	char name[UCD9000_NAME_SIZE];
+	int i;
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 272
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+	ucd9000_debugfs_dir = debugfs_create_dir("ucd9000", NULL);
+	if (IS_ERR(ucd9000_debugfs_dir)) {
+		ucd9000_debugfs_dir = NULL;
+		dev_warn(&client->dev, "Failed to create debugfs dir\n");
+		return 0;
+	}
+
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 278
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+	entries = devm_kzalloc(&client->dev, sizeof(*entries) *
+				(UCD90160_GPI_COUNT + 1) * 10,
+				GFP_KERNEL);
+	if (!entries)
+		return -ENOMEM;
+

Quoted 15 lines

Quoted 4 lines
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+	for (i = 0; i < UCD90160_GPI_COUNT; i++) {
+		entries[i].client = client;
+		entries[i].index = UCD90160_GPI_FAULT_BASE + i;
+		scnprintf(name, UCD9000_NAME_SIZE, "gpi%d_alarm", i+1);
Quoted 8 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_input(struct gpio_chip *gc, · line 291
Quoted 8 lines
drivers/hwmon/pmbus/ucd9000.c +8 -0
+		debugfs_create_file(name, 0444, ucd9000_debugfs_dir,
+				&entries[i],
+				&ucd9000_debugfs_ops_mfr_status_bit);
+	}
+	entries[i].client = client;
+	scnprintf(name, UCD9000_NAME_SIZE, "mfr_status");
+	debugfs_create_file(name, 0444, ucd9000_debugfs_dir, &entries[i],
+				&ucd9000_debugfs_ops_mfr_status_word);
Given the static ucd9000_debugfs_dir, how does this work if there are multiple devices? It's a naive question - I haven't looked into it at all, but I'm curious.

Right, this won't work well given that. Will add some unique identifier for each device.

Thanks,
Chris

Quoted 38 lines

Cheers,
Andrew
Quoted 34 lines
+
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+#else
+static int ucd9000_init_debugfs(struct i2c_client *client,
Review
Quoted 1 line
(quoted diff) +1 -0
+				struct ucd9000_data *data)
+{
Quoted 1 line
(quoted diff) +1 -0
+	return 0;
drivers/hwmon/pmbus/ucd9000.c +3 -0
+}
+#endif /* IS_ENABLED(CONFIG_DEBUG_FS) */
+
  static ssize_t ucd9000_clear_logged_faults(struct device *dev,
Quoted 2 lines
struct device_attribute *attr, const char *buf, size_t count)
drivers/hwmon/pmbus/ucd9000.c +1 -0
@@ -345,12 +460,19 @@ static int ucd9000_probe(struct i2c_client *client,
 >   	if (ret < 0)
 >   		return ret;
 
 > +	if (mid->driver_data == ucd90160) {
 > +		ret = ucd9000_init_debugfs(client, data);
 > +		if (ret < 0)
 > +			dev_warn(&client->dev, "Failed to register debugfs\n");
 > +	}
+
 >   	return  pmbus_do_probe(client, mid, info);
  }
 
  static int ucd9000_remove(struct i2c_client *client)
  {
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 476
Quoted 3 lines
  	sysfs_remove_group(&client->dev.kobj, &ucd9000_attr_group);
drivers/hwmon/pmbus/ucd9000.c +1 -0
+	debugfs_remove_recursive(ucd9000_debugfs_dir);
  	return pmbus_do_remove(client);
}

Andrew Jeffery

2017-08-22 02:00 UTC #8 lore raw

In reply to Christopher Bostic

Hi Chris,

Quoted 4 lines · Christopher Bostic

Add GPIO chip for user space access of the device's IO. Add debugfs for register dump of mfr_status and individual gpi faults.
Add sysfs attribute to clear logged faults.

In general my preference for cover letters, similar to commit messages, is they should describe the why and not the what. However, unlike the individual commit messages, the cover letter should address the motivation for the series as a whole, synthesising the information in the commit messages and discussing issues that don't make sense for any one commit message.

Again ideally, and in general, the cover letter should call out different classes of patches if necessary (e.g. "Patches 1-3 fix bugs, patch 4 adds feature X, patch 5 integrates X into Y"). If features been implemented in ways that might be controversial it helps to point that out too (and discuss design choices).

Essentially the cover letter should be your guide for reviewers on how to most effectively review your patches. This is why I don't find just listing the "what" ultimately that helpful - your cover letter is effectively only serving as a header under which to group the threaded patches. I feel like this is a missed opportunity.

Cheers,

Andrew

Quoted 9 lines

Christopher Bostic (2):
hwmon: (ucd9000) Add gpio chip interface and clear logged faults
hwmon: (ucd9000) Add debugfs for mfr_status info

drivers/hwmon/pmbus/ucd9000.c | 250
+++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 248 insertions(+), 2 deletions(-)