[PATCH linux dev-4.10 v3 0/2] hwmon: (ucd9000) Add gpio and debugfs

[PATCH linux dev-4.10 v3 0/2] hwmon: (ucd9000) Add gpio and debugfs

[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.

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

 drivers/hwmon/pmbus/ucd9000.c | 280 ++++++++-

-- 
1.8.2.2

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

[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.

Add capability to clear logged faults via sysfs.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
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 | 136 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 134 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index 3e3aa95..af44c1e 100644
--- a/drivers/hwmon/pmbus/ucd9000.c
+++ b/drivers/hwmon/pmbus/ucd9000.c
@@ -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,92 @@ 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 read pin ID %d\n", offset);
+		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 ? 1 : 0;
+}
+
+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;
+}
+
+/*
+ * Defined so that libgpiod tool can access pin states.  No plan to actually
+ * change pin direction within this driver.
+ */
+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 +327,39 @@ 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;
+	}
+
+	dev_info(&client->dev, "gpios %i...%i\n", data->gpio.base,
+		data->gpio.base + data->gpio.ngpio - 1);
+
+	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 +368,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,
 };
 
-- 
1.8.2.2

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

[Christopher Bostic]

Add a debugfs for the mfr_status register.  Add individual
files for each gpiN_fault and a separate file for the entire
contents of the register.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/hwmon/pmbus/ucd9000.c | 143 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 142 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index af44c1e..fddb40e 100644
--- a/drivers/hwmon/pmbus/ucd9000.c
+++ b/drivers/hwmon/pmbus/ucd9000.c
@@ -19,6 +19,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/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;
@@ -190,6 +201,113 @@ 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;
+
+	if (!ucd9000_debugfs_dir)
+		return -ENODEV;
+
+	/*
+	 * Create the debugfs directory for this device.
+	 */
+	data->debugfs = debugfs_create_dir("mfr_status",
+					ucd9000_debugfs_dir);
+	if (IS_ERR_OR_NULL(data->debugfs)) {
+		data->debugfs = NULL;
+		return -ENODEV;
+	}
+
+	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, data->debugfs, &entries[i],
+					&ucd9000_debugfs_ops_mfr_status_bit);
+	}
+	entries[i].client = client;
+	scnprintf(name, UCD9000_NAME_SIZE, "mfr_status");
+	debugfs_create_file(name, 0444, data->debugfs, &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)
@@ -353,12 +471,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);
 }
 
@@ -372,7 +497,23 @@ static int ucd9000_remove(struct i2c_client *client)
 	.id_table = ucd9000_id,
 };
 
-module_i2c_driver(ucd9000_driver);
+static int __init ucd9000_init(void)
+{
+	ucd9000_debugfs_dir = debugfs_create_dir("ucd9000", NULL);
+	if (IS_ERR(ucd9000_debugfs_dir))
+		ucd9000_debugfs_dir = NULL;
+
+	return i2c_add_driver(&ucd9000_driver);
+}
+
+static void __exit ucd9000_exit(void)
+{
+	i2c_del_driver(&ucd9000_driver);
+	debugfs_remove_recursive(ucd9000_debugfs_dir);
+}
+
+module_init(ucd9000_init);
+module_exit(ucd9000_exit);
 
 MODULE_AUTHOR("Guenter Roeck");
 MODULE_DESCRIPTION("PMBus driver for TI UCD90xxx");
-- 
1.8.2.2

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

[Joel Stanley]

Hi Chris,

On Fri, Aug 18, 2017 at 11:05 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:

> +static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
> +               ucd9000_clear_logged_faults);
> +
> +static struct attribute *ucd9000_attributes[] = {
> +       &dev_attr_clear_logged_faults.attr,
> +       NULL
> +};

Given this introduces new userspace ABI that will need to be support,
we will want to run this past upstream as soon as possible.

I suggest you post the next revision of these patches to the hwmon list.

Cheers,

Joel

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

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 11759 bytes --]

Hi Chris,

On Thu, 2017-08-17 at 20:35 -0500, Christopher Bostic wrote:
> Add a struct gpio_chip and define some methods so that this device's
> I/O can be accessed via /sys/class/gpio.
> 
> Add capability to clear logged faults via sysfs.
> 
> > Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
> 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 | 136 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 134 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> index 3e3aa95..af44c1e 100644
> --- a/drivers/hwmon/pmbus/ucd9000.c
> +++ b/drivers/hwmon/pmbus/ucd9000.c
> @@ -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

This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs.
We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin
Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI
Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22
as GPOs. It would probably pay to think about how these constraints would
impact the implementation.

> +
>  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,92 @@ 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,

Why is there a linebreak here?

> > +			"Failed to read pin ID %d\n", offset);

The error message is confusing - it relates to the semantics of
ucd9000_gpio_read_config() and not the operation on which the error occurred.

I feel strongly that we should also report the error code as well: It provides
additional context on what went wrong.

As a consequence I would suggest something like:

		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");

Again, we don't need a linebreak above, but we probably should have one between
the dev_err() and the return statement.

> > +		return ret;
> > +	}
> +
> > +	return ret & UCD9000_GPIO_CONFIG_STATUS ? 1 : 0;

We can avoid the branch with:

	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);

Interestingly it's not clear from the datasheet whether the status bit contains
a relevant value when the pin is in output mode, as there's a separate bit (bit
2) to set the value. Have you tested the behaviour?

> +}
> +
> +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;
> +}
> +
> +/*
> + * Defined so that libgpiod tool can access pin states.  No plan to actually
> + * change pin direction within this driver.

That's not true - the driver is general purpose. Lets not limit it too much to
*how you intend to use it* vs the actual hardware capability.

> + */
> +static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
> > +					unsigned int offset)
> +{
> > +	return 0;

I don't think we should do this for several reasons:

1) It's not that hard to implement the functionality
2) It's not true that all GPIOs are inputs with factory defaults
3) There will be uncertainty if custom defaults are programmed

Again from ucd90160.pdf, on page 34:

	7.5 Programming
	7.5.1 Device Configuration and Programming
	From the factory, the device contains the sequencing and monitoring
	firmware. It is also configured so that all GPOs are high-impedance
	(except for FPWM/GPIO pins 17-24, which are driven low), with no
	sequencing or fault-response operation. See Configuration Programming
	of UCD Devices, available from the Documentation & Help Center that can
	be selected from the Fusion GUI Help menu, for full UCD90160
	configuration details.

The FPWM pins are driven low, implying they are configured for output mode.
It's not obvious to me from this whether the other GPIOs are input or output
out-of-the-box as the chip supports active and open-drain drive types. It does
say "GPOs" in the paragraph above, but I haven't found documentation on which
pins are in what state from factory, and I don't know how we're configuring it
for production.

Regardless, the driver shouldn't make assumptions about how the device is
pre-programmed - without further information there's already ambiguity as to
whether the default settings are in play or whether the system integrator has
configured the chip. We need information from the platform about how the device
is/should be managed.

Further, the note in section 10.43 (FBh) GPIO_CONFIG (MFR_SPECIFIC_43) says:

	Configuring a pin that is also being used by another function (enable,
	fan control, and so forth) may likely result in unexpected and unwanted
	behavior.

This suggests my favourite topic: pinctrl.

Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16
of them have alternate functions. See "Table 3. GPIO Pin Configuration Options"
on page 22 of ucd90160.pdf. But a quick rundown:

1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)
3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)

Now, in terms of devicetree we'd be breaking things in the future by specifying
a GPIO controller that relies on a pinmux in hardware without also specifying
the pin controller in the devicetree. This necessitates at least *some* pinctrl
capability be baked into the driver as well, so we can point the gpiochip to
the relevant pin controller (the same device, but the bindings are general) and
map the pin spaces in the devicetree (the gpio and pin spaces are different).

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

Joel's made a fair comment here regarding upstream. Please address that.

>  static int ucd9000_probe(struct i2c_client *client,
> >  			 const struct i2c_device_id *id)
>  {
> @@ -227,7 +327,39 @@ 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;

I feel like it wouldn't be much effort to make this more complete and specify
the direction_output() and set() callbacks as well.

> > +	data->gpio.can_sleep = 1;
> > +	data->gpio.base = -1;
> > +	if (mid->driver_data == ucd90160)
> > +		data->gpio.ngpio = UCD90160_NUM_GPIOS;

This might need some rework in the future as the other ucd9000 variants want
GPIO support.

In fact, what's the behaviour of the other drivers now given we're
unconditionally registering a gpiochip?

> > +	data->gpio.parent = &client->dev;
> > +	data->gpio.owner = THIS_MODULE;

At some point we should think about pinconf as well (push-pull vs open-drain).

> +
> > +	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;
> > +	}
> +
> > +	dev_info(&client->dev, "gpios %i...%i\n", data->gpio.base,
> > +		data->gpio.base + data->gpio.ngpio - 1);

Let's drop this.

Cheers,

Andrew

> +
> > +	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 +368,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,
>  };
>  

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Matt Spinler]

Hi Andrew,

At this point, we just need a working driver so we can deliver the 
function.  This is all I need:

* read status_word

* read mfr_status (though it isn't that useful because the pmbus core 
code that Chris had to start using sends a clear faults every second and 
clears out the edge triggered bits I'm interested in)

* read status_vout from each page

* read the realtime GPI status using regs 0xFA and 0xFB for the 6 inputs 
that are wired on this system

Chris's current code does all that, as did several of his previous 
versions, so I think we need the path of least resistance and just go 
with one of them.


On 8/18/2017 12:44 AM, Andrew Jeffery wrote:
> Hi Chris,
>
> On Thu, 2017-08-17 at 20:35 -0500, Christopher Bostic wrote:
>> Add a struct gpio_chip and define some methods so that this device's
>> I/O can be accessed via /sys/class/gpio.
>>   
>> Add capability to clear logged faults via sysfs.
>>   
>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> ---
>> 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 | 136 +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 134 insertions(+), 2 deletions(-)
>>   
>> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
>> index 3e3aa95..af44c1e 100644
>> --- a/drivers/hwmon/pmbus/ucd9000.c
>> +++ b/drivers/hwmon/pmbus/ucd9000.c
>> @@ -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
> This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs.
> We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin
> Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI
> Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22
> as GPOs. It would probably pay to think about how these constraints would
> impact the implementation.
>
>> +
>>   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,92 @@ 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,
> Why is there a linebreak here?
>
>>> +			"Failed to read pin ID %d\n", offset);
> The error message is confusing - it relates to the semantics of
> ucd9000_gpio_read_config() and not the operation on which the error occurred.
>
> I feel strongly that we should also report the error code as well: It provides
> additional context on what went wrong.
>
> As a consequence I would suggest something like:
>
> 		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");
> Again, we don't need a linebreak above, but we probably should have one between
> the dev_err() and the return statement.
>
>>> +		return ret;
>>> +	}
>> +
>>> +	return ret & UCD9000_GPIO_CONFIG_STATUS ? 1 : 0;
> We can avoid the branch with:
>
> 	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
>
> Interestingly it's not clear from the datasheet whether the status bit contains
> a relevant value when the pin is in output mode, as there's a separate bit (bit
> 2) to set the value. Have you tested the behaviour?
>
>> +}
>> +
>> +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;
>> +}
>> +
>> +/*
>> + * Defined so that libgpiod tool can access pin states.  No plan to actually
>> + * change pin direction within this driver.
> That's not true - the driver is general purpose. Lets not limit it too much to
> *how you intend to use it* vs the actual hardware capability.
>
>> + */
>> +static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
>>> +					unsigned int offset)
>> +{
>>> +	return 0;
> I don't think we should do this for several reasons:
>
> 1) It's not that hard to implement the functionality
> 2) It's not true that all GPIOs are inputs with factory defaults
> 3) There will be uncertainty if custom defaults are programmed
>
> Again from ucd90160.pdf, on page 34:
>
> 	7.5 Programming
> 	7.5.1 Device Configuration and Programming
> 	From the factory, the device contains the sequencing and monitoring
> 	firmware. It is also configured so that all GPOs are high-impedance
> 	(except for FPWM/GPIO pins 17-24, which are driven low), with no
> 	sequencing or fault-response operation. See Configuration Programming
> 	of UCD Devices, available from the Documentation & Help Center that can
> 	be selected from the Fusion GUI Help menu, for full UCD90160
> 	configuration details.
>
> The FPWM pins are driven low, implying they are configured for output mode.
> It's not obvious to me from this whether the other GPIOs are input or output
> out-of-the-box as the chip supports active and open-drain drive types. It does
> say "GPOs" in the paragraph above, but I haven't found documentation on which
> pins are in what state from factory, and I don't know how we're configuring it
> for production.
>
> Regardless, the driver shouldn't make assumptions about how the device is
> pre-programmed - without further information there's already ambiguity as to
> whether the default settings are in play or whether the system integrator has
> configured the chip. We need information from the platform about how the device
> is/should be managed.
>
> Further, the note in section 10.43 (FBh) GPIO_CONFIG (MFR_SPECIFIC_43) says:
>
> 	Configuring a pin that is also being used by another function (enable,
> 	fan control, and so forth) may likely result in unexpected and unwanted
> 	behavior.
>
> This suggests my favourite topic: pinctrl.
>
> Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16
> of them have alternate functions. See "Table 3. GPIO Pin Configuration Options"
> on page 22 of ucd90160.pdf. But a quick rundown:
>
> 1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
> 2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)
> 3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)
>
> Now, in terms of devicetree we'd be breaking things in the future by specifying
> a GPIO controller that relies on a pinmux in hardware without also specifying
> the pin controller in the devicetree. This necessitates at least *some* pinctrl
> capability be baked into the driver as well, so we can point the gpiochip to
> the relevant pin controller (the same device, but the bindings are general) and
> map the pin spaces in the devicetree (the gpio and pin spaces are different).
>
>> +}
>> +
>> +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,
>> +};
>> +
> Joel's made a fair comment here regarding upstream. Please address that.
>
>>   static int ucd9000_probe(struct i2c_client *client,
>>>   			 const struct i2c_device_id *id)
>>   {
>> @@ -227,7 +327,39 @@ 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;
> I feel like it wouldn't be much effort to make this more complete and specify
> the direction_output() and set() callbacks as well.
>
>>> +	data->gpio.can_sleep = 1;
>>> +	data->gpio.base = -1;
>>> +	if (mid->driver_data == ucd90160)
>>> +		data->gpio.ngpio = UCD90160_NUM_GPIOS;
> This might need some rework in the future as the other ucd9000 variants want
> GPIO support.
>
> In fact, what's the behaviour of the other drivers now given we're
> unconditionally registering a gpiochip?
>
>>> +	data->gpio.parent = &client->dev;
>>> +	data->gpio.owner = THIS_MODULE;
> At some point we should think about pinconf as well (push-pull vs open-drain).
>
>> +
>>> +	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;
>>> +	}
>> +
>>> +	dev_info(&client->dev, "gpios %i...%i\n", data->gpio.base,
>>> +		data->gpio.base + data->gpio.ngpio - 1);
> Let's drop this.
>
> Cheers,
>
> Andrew
>
>> +
>>> +	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 +368,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,
>>   };
>>   

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

[Christopher Bostic]

On 8/17/17 9:48 PM, Joel Stanley wrote:
> Hi Chris,
>
> On Fri, Aug 18, 2017 at 11:05 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>
>> +static DEVICE_ATTR(clear_logged_faults, 0200, NULL,
>> +               ucd9000_clear_logged_faults);
>> +
>> +static struct attribute *ucd9000_attributes[] = {
>> +       &dev_attr_clear_logged_faults.attr,
>> +       NULL
>> +};
> Given this introduces new userspace ABI that will need to be support,
> we will want to run this past upstream as soon as possible.
>
> I suggest you post the next revision of these patches to the hwmon list.

Hi Joel,

Sure, will do that.

Thanks,
Chris
>
> Cheers,
>
> Joel
>

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

[Christopher Bostic]

On 8/18/17 12:44 AM, Andrew Jeffery wrote:
> Hi Chris,
>
> On Thu, 2017-08-17 at 20:35 -0500, Christopher Bostic wrote:
>> Add a struct gpio_chip and define some methods so that this device's
>> I/O can be accessed via /sys/class/gpio.
>>   
>> Add capability to clear logged faults via sysfs.
>>   
>>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> ---
>> 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 | 136 +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 134 insertions(+), 2 deletions(-)
>>   
>> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
>> index 3e3aa95..af44c1e 100644
>> --- a/drivers/hwmon/pmbus/ucd9000.c
>> +++ b/drivers/hwmon/pmbus/ucd9000.c
>> @@ -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
> This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs.
> We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin
> Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI
> Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22
> as GPOs. It would probably pay to think about how these constraints would
> impact the implementation.

Hi Andrew,

Will go through and review the data for the various ucd9000 devs. I 
understand the need to cover all these device types but want to also be 
able to deliver internally what is needed which is just the gpi faults.  
Maybe we can discuss further on Sunday's call.

>> +
>>   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,92 @@ 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,
> Why is there a linebreak here?
No good reason, will correct.

>
>>> +			"Failed to read pin ID %d\n", offset);
> The error message is confusing - it relates to the semantics of
> ucd9000_gpio_read_config() and not the operation on which the error occurred.
>
> I feel strongly that we should also report the error code as well: It provides
> additional context on what went wrong.
>
> As a consequence I would suggest something like:
>
> 		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
> 			ret);
OK, will go with your suggestion.

>>> +		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");
> Again, we don't need a linebreak above, but we probably should have one between
> the dev_err() and the return statement.

Will take care of that.


>>> +		return ret;
>>> +	}
>> +
>>> +	return ret & UCD9000_GPIO_CONFIG_STATUS ? 1 : 0;
> We can avoid the branch with:
>
> 	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
Will update.

> Interestingly it's not clear from the datasheet whether the status bit contains
> a relevant value when the pin is in output mode, as there's a separate bit (bit
> 2) to set the value. Have you tested the behaviour?

I've not tested this in output mode.  Up to this point there isn't any 
means to set direction so, at least for how we're using it currently 
these are all set to inputs.
Will see how this behaves in output mode.
>> +}
>> +
>> +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;
>> +}
>> +
>> +/*
>> + * Defined so that libgpiod tool can access pin states.  No plan to actually
>> + * change pin direction within this driver.
> That's not true - the driver is general purpose. Lets not limit it too much to
> *how you intend to use it* vs the actual hardware capability.
Understand.  Will implement direction switch for next revision in any case.

>> + */
>> +static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
>>> +					unsigned int offset)
>> +{
>>> +	return 0;
> I don't think we should do this for several reasons:
>
> 1) It's not that hard to implement the functionality
> 2) It's not true that all GPIOs are inputs with factory defaults
> 3) There will be uncertainty if custom defaults are programmed
>
> Again from ucd90160.pdf, on page 34:
>
> 	7.5 Programming
> 	7.5.1 Device Configuration and Programming
> 	From the factory, the device contains the sequencing and monitoring
> 	firmware. It is also configured so that all GPOs are high-impedance
> 	(except for FPWM/GPIO pins 17-24, which are driven low), with no
> 	sequencing or fault-response operation. See Configuration Programming
> 	of UCD Devices, available from the Documentation & Help Center that can
> 	be selected from the Fusion GUI Help menu, for full UCD90160
> 	configuration details.
>
> The FPWM pins are driven low, implying they are configured for output mode.
> It's not obvious to me from this whether the other GPIOs are input or output
> out-of-the-box as the chip supports active and open-drain drive types. It does
> say "GPOs" in the paragraph above, but I haven't found documentation on which
> pins are in what state from factory, and I don't know how we're configuring it
> for production.
>
> Regardless, the driver shouldn't make assumptions about how the device is
> pre-programmed - without further information there's already ambiguity as to
> whether the default settings are in play or whether the system integrator has
> configured the chip. We need information from the platform about how the device
> is/should be managed.
>
> Further, the note in section 10.43 (FBh) GPIO_CONFIG (MFR_SPECIFIC_43) says:
>
> 	Configuring a pin that is also being used by another function (enable,
> 	fan control, and so forth) may likely result in unexpected and unwanted
> 	behavior.
All good points.  Will implement direction switching for next submit.

> This suggests my favourite topic: pinctrl.
>
> Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16
> of them have alternate functions. See "Table 3. GPIO Pin Configuration Options"
> on page 22 of ucd90160.pdf. But a quick rundown:
>
> 1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
> 2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)
> 3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)
>
> Now, in terms of devicetree we'd be breaking things in the future by specifying
> a GPIO controller that relies on a pinmux in hardware without also specifying
> the pin controller in the devicetree. This necessitates at least *some* pinctrl
> capability be baked into the driver as well, so we can point the gpiochip to
> the relevant pin controller (the same device, but the bindings are general) and
> map the pin spaces in the devicetree (the gpio and pin spaces are different).

I'd like to avoid implementing pinctrl for the time being given the 
limited function we need internally.   I do see the advantages as you 
point out so maybe we can phase this in?

>> +}
>> +
>> +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,
>> +};
>> +
> Joel's made a fair comment here regarding upstream. Please address that.
Next patch set will be sent upstream.

>>   static int ucd9000_probe(struct i2c_client *client,
>>>   			 const struct i2c_device_id *id)
>>   {
>> @@ -227,7 +327,39 @@ 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;
> I feel like it wouldn't be much effort to make this more complete and specify
> the direction_output() and set() callbacks as well.

Right, will do that.

>
>>> +	data->gpio.can_sleep = 1;
>>> +	data->gpio.base = -1;
>>> +	if (mid->driver_data == ucd90160)
>>> +		data->gpio.ngpio = UCD90160_NUM_GPIOS;
> This might need some rework in the future as the other ucd9000 variants want
> GPIO support.
>
> In fact, what's the behaviour of the other drivers now given we're
> unconditionally registering a gpiochip?
The other drivers would have a gpio_chip that is available but if they 
don't use it I'm not thinking this would break them in any way.  Would 
it be better to only register a gpiochip if its a ucd90160 in particular?

>>> +	data->gpio.parent = &client->dev;
>>> +	data->gpio.owner = THIS_MODULE;
> At some point we should think about pinconf as well (push-pull vs open-drain).
Ok, will make a note of that.


>> +
>>> +	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;
>>> +	}
>> +
>>> +	dev_info(&client->dev, "gpios %i...%i\n", data->gpio.base,
>>> +		data->gpio.base + data->gpio.ngpio - 1);
> Let's drop this.
Will remove.

Thanks,
Chris


>
> Cheers,
>
> Andrew
>
>> +
>>> +	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 +368,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,
>>   };
>>   

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

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 16957 bytes --]

Hi Matt,

Thanks for clarifying your requirements. I have some comments inline below.

On Fri, 2017-08-18 at 09:35 -0500, Matt Spinler wrote:
> Hi Andrew,
> 
> At this point, we just need a working driver so we can deliver the 
> function.  This is all I need:
> 
> * read status_word
> 
> * read mfr_status (though it isn't that useful because the pmbus core 
> code that Chris had to start using sends a clear faults every second and 
> clears out the edge triggered bits I'm interested in)

That's a valid complaint. At the moment pmbus_clear_faults() is driven by calls
to pmbus_update_device() if the sensor value needs to be updated (values are
cached for a time-period by the core).

There's no timer that clears the faults, it's purely the accesses from
userspace. So if the accesses are ordered such that MFR_STATUS is read before
any sensors then you should not miss any edge-triggered faults.

However that's not to say that this behaviour is sane. Some work should be done
to figure out if we can avoid sending a CLEAR_FAULTS on every update.

> 
> * read status_vout from each page
> 
> * read the realtime GPI status using regs 0xFA and 0xFB for the 6 inputs 
> that are wired on this system
> 
> Chris's current code does all that, as did several of his previous 
> versions, so I think we need the path of least resistance and just go 
> with one of them.

I disagree on several fronts:

1. We've seen how frustrating userspace-breaking changes are with FSI and it
follows (in my mind) that we'd want to minimise such problems going forward.
This driver has evolved from defining many new, non-standard ABIs in v1 and v2
down to one in v3, which we're looking to get a blessing on from upstream. I
would be *very* surprised if v1 or v2 were accepted with no complaints by
upstream, and thus v3 is a big improvement over what we had (through minimising
new ABIs).

2. The implementation of v2 would have seriously disrupted the behaviour of the
device or given incorrect values for queries of the GPIO state.

3. v3 makes many assumptions about how the device is programmed but documents
none of them. In light of that I can only review for the general case of
needing to put the device in the state required to read off the values you
need. Prior to your email the exact requirements on the implementation were not
clear to me either. The flexibility and complexity of the UCD90160 means this
is non-trivial, so either we implement the required functionality to achieve
generality OR we heavily document the assumptions about how the device is
programmed so it's *obvious* to the reader why the implementation is hamstrung
the way it is. The latter approach makes it unsuitable for sending upstream,
but at least it documents the path that should be followed before doing so.

Cheers,

Andrew

> 
> 
> On 8/18/2017 12:44 AM, Andrew Jeffery wrote:
> > Hi Chris,
> > 
> > On Thu, 2017-08-17 at 20:35 -0500, Christopher Bostic wrote:
> > > Add a struct gpio_chip and define some methods so that this device's
> > > I/O can be accessed via /sys/class/gpio.
> > >   
> > > Add capability to clear logged faults via sysfs.
> > >   
> > > > Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> > > 
> > > ---
> > > 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 | 136 +++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 134 insertions(+), 2 deletions(-)
> > >   
> > > diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> > > index 3e3aa95..af44c1e 100644
> > > --- a/drivers/hwmon/pmbus/ucd9000.c
> > > +++ b/drivers/hwmon/pmbus/ucd9000.c
> > > @@ -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
> > 
> > This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs.
> > We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin
> > Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI
> > Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22
> > as GPOs. It would probably pay to think about how these constraints would
> > impact the implementation.
> > 
> > > +
> > >   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,92 @@ 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,
> > 
> > Why is there a linebreak here?
> > 
> > > > +			"Failed to read pin ID %d\n", offset);
> > 
> > The error message is confusing - it relates to the semantics of
> > ucd9000_gpio_read_config() and not the operation on which the error occurred.
> > 
> > I feel strongly that we should also report the error code as well: It provides
> > additional context on what went wrong.
> > 
> > As a consequence I would suggest something like:
> > 
> > > > 		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");
> > 
> > Again, we don't need a linebreak above, but we probably should have one between
> > the dev_err() and the return statement.
> > 
> > > > > > > > +		return ret;
> > > > +	}
> > > 
> > > +
> > > > +	return ret & UCD9000_GPIO_CONFIG_STATUS ? 1 : 0;
> > 
> > We can avoid the branch with:
> > 
> > 	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
> > 
> > Interestingly it's not clear from the datasheet whether the status bit contains
> > a relevant value when the pin is in output mode, as there's a separate bit (bit
> > 2) to set the value. Have you tested the behaviour?
> > 
> > > +}
> > > +
> > > +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;
> > > 
> > > +}
> > > +
> > > +/*
> > > + * Defined so that libgpiod tool can access pin states.  No plan to actually
> > > + * change pin direction within this driver.
> > 
> > That's not true - the driver is general purpose. Lets not limit it too much to
> > *how you intend to use it* vs the actual hardware capability.
> > 
> > > + */
> > > +static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
> > > > +					unsigned int offset)
> > > 
> > > +{
> > > > +	return 0;
> > 
> > I don't think we should do this for several reasons:
> > 
> > 1) It's not that hard to implement the functionality
> > 2) It's not true that all GPIOs are inputs with factory defaults
> > 3) There will be uncertainty if custom defaults are programmed
> > 
> > Again from ucd90160.pdf, on page 34:
> > 
> > 	7.5 Programming
> > 	7.5.1 Device Configuration and Programming
> > 	From the factory, the device contains the sequencing and monitoring
> > 	firmware. It is also configured so that all GPOs are high-impedance
> > 	(except for FPWM/GPIO pins 17-24, which are driven low), with no
> > 	sequencing or fault-response operation. See Configuration Programming
> > 	of UCD Devices, available from the Documentation & Help Center that can
> > 	be selected from the Fusion GUI Help menu, for full UCD90160
> > 	configuration details.
> > 
> > The FPWM pins are driven low, implying they are configured for output mode.
> > It's not obvious to me from this whether the other GPIOs are input or output
> > out-of-the-box as the chip supports active and open-drain drive types. It does
> > say "GPOs" in the paragraph above, but I haven't found documentation on which
> > pins are in what state from factory, and I don't know how we're configuring it
> > for production.
> > 
> > Regardless, the driver shouldn't make assumptions about how the device is
> > pre-programmed - without further information there's already ambiguity as to
> > whether the default settings are in play or whether the system integrator has
> > configured the chip. We need information from the platform about how the device
> > is/should be managed.
> > 
> > Further, the note in section 10.43 (FBh) GPIO_CONFIG (MFR_SPECIFIC_43) says:
> > 
> > 	Configuring a pin that is also being used by another function (enable,
> > 	fan control, and so forth) may likely result in unexpected and unwanted
> > 	behavior.
> > 
> > This suggests my favourite topic: pinctrl.
> > 
> > Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16
> > of them have alternate functions. See "Table 3. GPIO Pin Configuration Options"
> > on page 22 of ucd90160.pdf. But a quick rundown:
> > 
> > 1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
> > 2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)
> > 3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)
> > 
> > Now, in terms of devicetree we'd be breaking things in the future by specifying
> > a GPIO controller that relies on a pinmux in hardware without also specifying
> > the pin controller in the devicetree. This necessitates at least *some* pinctrl
> > capability be baked into the driver as well, so we can point the gpiochip to
> > the relevant pin controller (the same device, but the bindings are general) and
> > map the pin spaces in the devicetree (the gpio and pin spaces are different).
> > 
> > > +}
> > > +
> > > +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,
> > > 
> > > +};
> > > +
> > 
> > Joel's made a fair comment here regarding upstream. Please address that.
> > 
> > >   static int ucd9000_probe(struct i2c_client *client,
> > > >   			 const struct i2c_device_id *id)
> > > 
> > >   {
> > > @@ -227,7 +327,39 @@ 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;
> > 
> > I feel like it wouldn't be much effort to make this more complete and specify
> > the direction_output() and set() callbacks as well.
> > 
> > > > > > > > +	data->gpio.can_sleep = 1;
> > > > > > > > +	data->gpio.base = -1;
> > > > > > > > +	if (mid->driver_data == ucd90160)
> > > > +		data->gpio.ngpio = UCD90160_NUM_GPIOS;
> > 
> > This might need some rework in the future as the other ucd9000 variants want
> > GPIO support.
> > 
> > In fact, what's the behaviour of the other drivers now given we're
> > unconditionally registering a gpiochip?
> > 
> > > > > > > > +	data->gpio.parent = &client->dev;
> > > > +	data->gpio.owner = THIS_MODULE;
> > 
> > At some point we should think about pinconf as well (push-pull vs open-drain).
> > 
> > > +
> > > > > > > > +	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;
> > > > +	}
> > > 
> > > +
> > > > > > > > +	dev_info(&client->dev, "gpios %i...%i\n", data->gpio.base,
> > > > +		data->gpio.base + data->gpio.ngpio - 1);
> > 
> > Let's drop this.
> > 
> > Cheers,
> > 
> > Andrew
> > 
> > > +
> > > > > > > > +	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 +368,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,
> > > 
> > >   };
> > >   
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 16808 bytes --]

Hi Chris,

On Fri, 2017-08-18 at 15:38 -0500, Christopher Bostic wrote:
> 
> On 8/18/17 12:44 AM, Andrew Jeffery wrote:
> > Hi Chris,
> > 
> > On Thu, 2017-08-17 at 20:35 -0500, Christopher Bostic wrote:
> > > Add a struct gpio_chip and define some methods so that this device's
> > > I/O can be accessed via /sys/class/gpio.
> > >   
> > > Add capability to clear logged faults via sysfs.
> > >   
> > > > Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> > > 
> > > ---
> > > 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 | 136 +++++++++++++++++++++++++++++++++++++++++-
> > >   1 file changed, 134 insertions(+), 2 deletions(-)
> > >   
> > > diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> > > index 3e3aa95..af44c1e 100644
> > > --- a/drivers/hwmon/pmbus/ucd9000.c
> > > +++ b/drivers/hwmon/pmbus/ucd9000.c
> > > @@ -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
> > 
> > This is mostly true, though there are only 22 physical GPIOs, plus 4 GPIs.
> > We're additionally constrained on the UCD90160 by "Table 3. GPIO Pin
> > Configuration Options" in "16-Rail Power Supply Sequencer and Monitor with ACPI
> > Support datasheet (Rev. C)" (ucd90160.pdf, page 22) to 8/26 as GPIs and 16/22
> > as GPOs. It would probably pay to think about how these constraints would
> > impact the implementation.
> 
> Hi Andrew,
> 
> Will go through and review the data for the various ucd9000 devs. I 
> understand the need to cover all these device types but want to also be 
> able to deliver internally what is needed which is just the gpi faults.  
> Maybe we can discuss further on Sunday's call.

Yeah, understood, it was a thinking-out-loud comment. However I would hate to
get too deep on an implementation only to find that it's not appropriate for
the device's behaviour.

> 
> > > +
> > >   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,92 @@ 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,
> > 
> > Why is there a linebreak here?
> 
> No good reason, will correct.
> 
> > 
> > > > +			"Failed to read pin ID %d\n", offset);
> > 
> > The error message is confusing - it relates to the semantics of
> > ucd9000_gpio_read_config() and not the operation on which the error occurred.
> > 
> > I feel strongly that we should also report the error code as well: It provides
> > additional context on what went wrong.
> > 
> > As a consequence I would suggest something like:
> > 
> > > > 		dev_err(&client->dev, "Failed to select GPIO %d: %d\n", offset,
> > 			ret);
> 
> OK, will go with your suggestion.
> 
> > > > > > > > +		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");
> > 
> > Again, we don't need a linebreak above, but we probably should have one between
> > the dev_err() and the return statement.
> 
> Will take care of that.
> 
> 
> > > > > > > > +		return ret;
> > > > +	}
> > > 
> > > +
> > > > +	return ret & UCD9000_GPIO_CONFIG_STATUS ? 1 : 0;
> > 
> > We can avoid the branch with:
> > 
> > 	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
> 
> Will update.
> 
> > Interestingly it's not clear from the datasheet whether the status bit contains
> > a relevant value when the pin is in output mode, as there's a separate bit (bit
> > 2) to set the value. Have you tested the behaviour?
> 
> I've not tested this in output mode.  Up to this point there isn't any 
> means to set direction so, at least for how we're using it currently 
> these are all set to inputs.
> Will see how this behaves in output mode.

Do it as you have time - as you point out there's not support for output
currently so it's not the end of the world. I just wanted to note the behaviour
so we didn't forget about it.

> > > +}
> > > +
> > > +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;
> > > 
> > > +}
> > > +
> > > +/*
> > > + * Defined so that libgpiod tool can access pin states.  No plan to actually
> > > + * change pin direction within this driver.
> > 
> > That's not true - the driver is general purpose. Lets not limit it too much to
> > *how you intend to use it* vs the actual hardware capability.
> 
> Understand.  Will implement direction switch for next revision in any case.

Great.

> 
> > > + */
> > > +static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
> > > > +					unsigned int offset)
> > > 
> > > +{
> > > > +	return 0;
> > 
> > I don't think we should do this for several reasons:
> > 
> > 1) It's not that hard to implement the functionality
> > 2) It's not true that all GPIOs are inputs with factory defaults
> > 3) There will be uncertainty if custom defaults are programmed
> > 
> > Again from ucd90160.pdf, on page 34:
> > 
> > 	7.5 Programming
> > 	7.5.1 Device Configuration and Programming
> > 	From the factory, the device contains the sequencing and monitoring
> > 	firmware. It is also configured so that all GPOs are high-impedance
> > 	(except for FPWM/GPIO pins 17-24, which are driven low), with no
> > 	sequencing or fault-response operation. See Configuration Programming
> > 	of UCD Devices, available from the Documentation & Help Center that can
> > 	be selected from the Fusion GUI Help menu, for full UCD90160
> > 	configuration details.
> > 
> > The FPWM pins are driven low, implying they are configured for output mode.
> > It's not obvious to me from this whether the other GPIOs are input or output
> > out-of-the-box as the chip supports active and open-drain drive types. It does
> > say "GPOs" in the paragraph above, but I haven't found documentation on which
> > pins are in what state from factory, and I don't know how we're configuring it
> > for production.
> > 
> > Regardless, the driver shouldn't make assumptions about how the device is
> > pre-programmed - without further information there's already ambiguity as to
> > whether the default settings are in play or whether the system integrator has
> > configured the chip. We need information from the platform about how the device
> > is/should be managed.
> > 
> > Further, the note in section 10.43 (FBh) GPIO_CONFIG (MFR_SPECIFIC_43) says:
> > 
> > 	Configuring a pin that is also being used by another function (enable,
> > 	fan control, and so forth) may likely result in unexpected and unwanted
> > 	behavior.
> 
> All good points.  Will implement direction switching for next submit.

Thanks.

> 
> > This suggests my favourite topic: pinctrl.
> > 
> > Twenty-two of the pins on the UCD90160 are GPIO-capable. Of these 22 pins, 16
> > of them have alternate functions. See "Table 3. GPIO Pin Configuration Options"
> > on page 22 of ucd90160.pdf. But a quick rundown:
> > 
> > 1. 4 PWM pins (GPIOs 1-4, pins 31, 32, 42, 41)
> > 2. 8 FPWM (Fast PWM) pins (GPIOs 5-12, pins 17-24)
> > 3. 4 pins shared with JTAG (GPIOs 19-22, pins 36-39)
> > 
> > Now, in terms of devicetree we'd be breaking things in the future by specifying
> > a GPIO controller that relies on a pinmux in hardware without also specifying
> > the pin controller in the devicetree. This necessitates at least *some* pinctrl
> > capability be baked into the driver as well, so we can point the gpiochip to
> > the relevant pin controller (the same device, but the bindings are general) and
> > map the pin spaces in the devicetree (the gpio and pin spaces are different).
> 
> I'd like to avoid implementing pinctrl for the time being given the 
> limited function we need internally.   I do see the advantages as you 
> point out so maybe we can phase this in?

I think it should be possible to do a bare-bones pinctrl implementation that
describes no pinmux functions but allows allocation of GPIOs. That way the
devicetree won't have to change in a breaking way. See how the GPIO controller
defers to the pinctrl node to allocate pins in the Aspeed DTSIs:

	https://github.com/openbmc/linux/blob/dev-4.10/Documentation/devicetree/bindings/gpio/gpio.txt#L230
	https://github.com/openbmc/linux/blob/dev-4.10/arch/arm/boot/dts/aspeed-g5.dtsi#L271
	https://github.com/openbmc/linux/blob/dev-4.10/drivers/pinctrl/aspeed/pinctrl-aspeed-g5.c#L2439
	https://github.com/openbmc/linux/blob/dev-4.10/drivers/pinctrl/aspeed/pinctrl-aspeed.c#L453

The implementation should be no-where near as complex as the Aspeed pinmux, and
hopefully we only need the GPIO portions of it.

Cheers,

Andrew

> 
> > > +}
> > > +
> > > +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,
> > > 
> > > +};
> > > +
> > 
> > Joel's made a fair comment here regarding upstream. Please address that.
> 
> Next patch set will be sent upstream.
> 
> > >   static int ucd9000_probe(struct i2c_client *client,
> > > >   			 const struct i2c_device_id *id)
> > > 
> > >   {
> > > @@ -227,7 +327,39 @@ 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;
> > 
> > I feel like it wouldn't be much effort to make this more complete and specify
> > the direction_output() and set() callbacks as well.
> 
> Right, will do that.
> 
> > 
> > > > > > > > +	data->gpio.can_sleep = 1;
> > > > > > > > +	data->gpio.base = -1;
> > > > > > > > +	if (mid->driver_data == ucd90160)
> > > > +		data->gpio.ngpio = UCD90160_NUM_GPIOS;
> > 
> > This might need some rework in the future as the other ucd9000 variants want
> > GPIO support.
> > 
> > In fact, what's the behaviour of the other drivers now given we're
> > unconditionally registering a gpiochip?
> 
> The other drivers would have a gpio_chip that is available but if they 
> don't use it I'm not thinking this would break them in any way.  Would 
> it be better to only register a gpiochip if its a ucd90160 in particular?
> 
> > > > > > > > +	data->gpio.parent = &client->dev;
> > > > +	data->gpio.owner = THIS_MODULE;
> > 
> > At some point we should think about pinconf as well (push-pull vs open-drain).
> 
> Ok, will make a note of that.
> 
> 
> > > +
> > > > > > > > +	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;
> > > > +	}
> > > 
> > > +
> > > > > > > > +	dev_info(&client->dev, "gpios %i...%i\n", data->gpio.base,
> > > > +		data->gpio.base + data->gpio.ngpio - 1);
> > 
> > Let's drop this.
> 
> Will remove.
> 
> Thanks,
> Chris
> 
> 
> > 
> > Cheers,
> > 
> > Andrew
> > 
> > > +
> > > > > > > > +	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 +368,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,
> > > 
> > >   };
> > >   
> 
> 

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]