[PATCH v2 0/2] hwmon: (ucd9000) Add gpio and debugfs interfaces

[PATCH v2 0/2] hwmon: (ucd9000) Add gpio and debugfs interfaces

[Eddie James]

The ucd9000 series chips have gpio pins. Add a gpio chip interface to the ucd
device so that users can query and set the state of the gpio pins.

Add a debugfs interface using the existing pmbus debugfs directory to provide
MFR_STATUS and the status of the gpi faults to users.

Changes since v1:
 - dropped dev_err messages
 - made gpio chip registration conditional on having gpio pins
 - made mfr_status debugfs attribute more simple

Christopher Bostic (2):
  hwmon: (ucd9000) Add gpio chip interface
  hwmon: (ucd9000) Add debugfs attributes to provide mfr_status

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

-- 
1.8.3.1

[PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface

[Eddie James]

From: Christopher Bostic <cbostic@linux.vnet.ibm.com>

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

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/hwmon/pmbus/ucd9000.c | 201 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 201 insertions(+)

diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index b74dbec..023fb9e 100644
--- a/drivers/hwmon/pmbus/ucd9000.c
+++ b/drivers/hwmon/pmbus/ucd9000.c
@@ -27,6 +27,7 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/pmbus.h>
+#include <linux/gpio.h>
 #include "pmbus.h"
 
 enum chips { ucd9000, ucd90120, ucd90124, ucd90160, ucd9090, ucd90910 };
@@ -35,8 +36,18 @@
 #define UCD9000_NUM_PAGES		0xd6
 #define UCD9000_FAN_CONFIG_INDEX	0xe7
 #define UCD9000_FAN_CONFIG		0xe8
+#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_OUT_VALUE	BIT(2)
+#define UCD9000_GPIO_CONFIG_STATUS	BIT(3)
+#define UCD9000_GPIO_INPUT		0
+#define UCD9000_GPIO_OUTPUT		1
+
 #define UCD9000_MON_TYPE(x)	(((x) >> 5) & 0x07)
 #define UCD9000_MON_PAGE(x)	((x) & 0x0f)
 
@@ -47,9 +58,15 @@
 
 #define UCD9000_NUM_FAN		4
 
+#define UCD9000_GPIO_NAME_LEN	16
+#define UCD9090_NUM_GPIOS	23
+#define UCD901XX_NUM_GPIOS	26
+#define UCD90910_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)
 
@@ -149,6 +166,146 @@ static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
 };
 MODULE_DEVICE_TABLE(of, ucd9000_of_match);
 
+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)
+		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)
+		return ret;
+
+	return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
+}
+
+static void ucd9000_gpio_set(struct gpio_chip *gc, unsigned int offset,
+			     int value)
+{
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret;
+
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0) {
+		dev_dbg(&client->dev, "failed to read GPIO %d config: %d\n",
+			offset, ret);
+		return;
+	}
+
+	if (value) {
+		if (ret & UCD9000_GPIO_CONFIG_STATUS)
+			return;
+
+		ret |= UCD9000_GPIO_CONFIG_STATUS;
+	} else {
+		if (!(ret & UCD9000_GPIO_CONFIG_STATUS))
+			return;
+
+		ret &= ~UCD9000_GPIO_CONFIG_STATUS;
+	}
+
+	ret |= UCD9000_GPIO_CONFIG_ENABLE;
+
+	/* Page set not required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
+	if (ret < 0) {
+		dev_dbg(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
+		return;
+	}
+
+	ret &= ~UCD9000_GPIO_CONFIG_ENABLE;
+
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
+	if (ret < 0)
+		dev_dbg(&client->dev, "Failed to write GPIO %d config: %d\n",
+			offset, ret);
+}
+
+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)
+		return ret;
+
+	return !(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE);
+}
+
+static int ucd9000_gpio_set_direction(struct gpio_chip *gc,
+				      unsigned int offset, bool direction_out,
+				      int requested_out)
+{
+	struct i2c_client *client = gpiochip_get_data(gc);
+	int ret, config, out_val;
+
+	ret = ucd9000_gpio_read_config(client, offset);
+	if (ret < 0)
+		return ret;
+
+	if (direction_out) {
+		out_val = requested_out ? UCD9000_GPIO_CONFIG_OUT_VALUE : 0;
+
+		if (ret & UCD9000_GPIO_CONFIG_OUT_ENABLE) {
+			if ((ret & UCD9000_GPIO_CONFIG_OUT_VALUE) == out_val)
+				return 0;
+		} else {
+			ret |= UCD9000_GPIO_CONFIG_OUT_ENABLE;
+		}
+
+		if (out_val)
+			ret |= UCD9000_GPIO_CONFIG_OUT_VALUE;
+		else
+			ret &= ~UCD9000_GPIO_CONFIG_OUT_VALUE;
+
+	} else {
+		if (!(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE))
+			return 0;
+
+		ret &= ~UCD9000_GPIO_CONFIG_OUT_ENABLE;
+	}
+
+	ret |= UCD9000_GPIO_CONFIG_ENABLE;
+	config = ret;
+
+	/* Page set not required */
+	ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);
+	if (ret < 0)
+		return ret;
+
+	config &= ~UCD9000_GPIO_CONFIG_ENABLE;
+
+	return i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, config);
+}
+
+static int ucd9000_gpio_direction_input(struct gpio_chip *gc,
+					unsigned int offset)
+{
+	return ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_INPUT, 0);
+}
+
+static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
+					 unsigned int offset, int val)
+{
+	return ucd9000_gpio_set_direction(gc, offset, UCD9000_GPIO_OUTPUT,
+					  val);
+}
+
 static int ucd9000_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -263,6 +420,50 @@ static int ucd9000_probe(struct i2c_client *client,
 		  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
 	}
 
+	/*
+	 * Note:
+	 *
+	 * Pinmux support has not been added to the new gpio_chip.
+	 * This support should be added when possible given the mux
+	 * behavior of these IO devices.
+	 */
+	data->gpio.label = (const char *)&client->name;
+	data->gpio.get_direction = ucd9000_gpio_get_direction;
+	data->gpio.direction_input = ucd9000_gpio_direction_input;
+	data->gpio.direction_output = ucd9000_gpio_direction_output;
+	data->gpio.get = ucd9000_gpio_get;
+	data->gpio.set = ucd9000_gpio_set;
+	data->gpio.can_sleep = 1;
+	data->gpio.base = -1;
+
+	switch (mid->driver_data) {
+	case ucd9090:
+		data->gpio.ngpio = UCD9090_NUM_GPIOS;
+		break;
+	case ucd90120:
+	case ucd90124:
+	case ucd90160:
+		data->gpio.ngpio = UCD901XX_NUM_GPIOS;
+		break;
+	case ucd90910:
+		data->gpio.ngpio = UCD90910_NUM_GPIOS;
+		break;
+	default:
+		break;
+	}
+
+	data->gpio.parent = &client->dev;
+	data->gpio.owner = THIS_MODULE;
+	data->gpio.of_node = client->dev.of_node;
+
+	if (data->gpio.ngpio) {
+		ret = devm_gpiochip_add_data(&client->dev, &data->gpio,
+					     client);
+		if (ret)
+			dev_warn(&client->dev, "Could not add gpiochip: %d\n",
+				 ret);
+	}
+
 	return pmbus_do_probe(client, mid, info);
 }
 
-- 
1.8.3.1

[PATCH v2 2/2] hwmon: (ucd9000) Add debugfs attributes to provide mfr_status

[Eddie James]

From: Christopher Bostic <cbostic@linux.vnet.ibm.com>

Expose the gpiN_fault fields of mfr_status as individual debugfs
attributes. This provides a way for users to be easily notified of gpi
faults. Also provide the whole mfr_status register in debugfs.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
---
 drivers/hwmon/pmbus/ucd9000.c | 140 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 139 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
index 023fb9e..b073d8e 100644
--- a/drivers/hwmon/pmbus/ucd9000.c
+++ b/drivers/hwmon/pmbus/ucd9000.c
@@ -19,6 +19,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -36,6 +37,7 @@
 #define UCD9000_NUM_PAGES		0xd6
 #define UCD9000_FAN_CONFIG_INDEX	0xe7
 #define UCD9000_FAN_CONFIG		0xe8
+#define UCD9000_MFR_STATUS		0xf3
 #define UCD9000_GPIO_SELECT		0xfa
 #define UCD9000_GPIO_CONFIG		0xfb
 #define UCD9000_DEVICE_ID		0xfd
@@ -63,13 +65,22 @@
 #define UCD901XX_NUM_GPIOS	26
 #define UCD90910_NUM_GPIOS	26
 
+#define UCD9000_DEBUGFS_NAME_LEN	24
+#define UCD9000_GPI_COUNT		8
+
 struct ucd9000_data {
 	u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
 	struct pmbus_driver_info info;
 	struct gpio_chip gpio;
+	struct dentry *debugfs;
 };
 #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
 
+struct ucd9000_debugfs_entry {
+	struct i2c_client *client;
+	u8 index;
+};
+
 static int ucd9000_get_fan_config(struct i2c_client *client, int fan)
 {
 	int fan_config = 0;
@@ -306,6 +317,124 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
 					  val);
 }
 
+#if IS_ENABLED(CONFIG_DEBUG_FS)
+static int ucd9000_get_mfr_status(struct i2c_client *client, u8 *buffer)
+{
+	int ret = pmbus_set_page(client, 0);
+
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * With the ucd90120 and ucd90124 devices, this command [MFR_STATUS]
+	 * is 2 bytes long (bits 0-15).  With the ucd90240 this command is 5
+	 * bytes long.  With all other devices, it is 4 bytes long.
+	 */
+	return i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS, buffer);
+}
+
+static int ucd9000_debugfs_show_mfr_status_bit(void *data, u64 *val)
+{
+	struct ucd9000_debugfs_entry *entry = data;
+	struct i2c_client *client = entry->client;
+	u8 buffer[4];
+	int ret;
+
+	/*
+	 * This attribute is only created for devices that return 4 bytes for
+	 * status_mfr, so it's safe to call with 4-byte buffer.
+	 */
+	ret = ucd9000_get_mfr_status(client, buffer);
+	if (ret < 0)
+		return ret;
+
+	/*
+	 * Attribute only created for devices with gpi fault bits at bits
+	 * 16-23, which is the second byte of the response.
+	 */
+	*val = !!(buffer[1] & BIT(entry->index));
+
+	return 0;
+}
+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_bit,
+			 ucd9000_debugfs_show_mfr_status_bit, NULL, "%1lld\n");
+
+static ssize_t ucd9000_debugfs_read_mfr_status(struct file *file,
+					       char __user *buf, size_t count,
+					       loff_t *ppos)
+{
+	struct i2c_client *client = file->private_data;
+	u8 buffer[5] = { 0 };	/* Need max 5 bytes for any ucd9000 chip. */
+	int ret;
+
+	ret = ucd9000_get_mfr_status(client, buffer);
+	if (ret < 0)
+		return ret;
+
+	return simple_read_from_buffer(buf, count, ppos, buffer, ret);
+}
+
+static const struct file_operations ucd9000_debugfs_show_mfr_status_fops = {
+	.llseek = noop_llseek,
+	.read = ucd9000_debugfs_read_mfr_status,
+	.open = simple_open,
+};
+
+static int ucd9000_init_debugfs(struct i2c_client *client,
+				const struct i2c_device_id *mid,
+				struct ucd9000_data *data)
+{
+	struct dentry *debugfs;
+	struct ucd9000_debugfs_entry *entries;
+	int i;
+	char name[UCD9000_DEBUGFS_NAME_LEN];
+
+	debugfs = pmbus_get_debugfs_dir(client);
+	if (!debugfs)
+		return -ENOENT;
+
+	data->debugfs = debugfs_create_dir(client->name, debugfs);
+	if (!data->debugfs)
+		return -ENOENT;
+
+	/*
+	 * Of the chips this driver supports, only the UCD9090, UCD90160,
+	 * and UCD90910 report GPI faults in their MFR_STATUS register, so only
+	 * create the GPI fault debugfs attributes for those chips.
+	 */
+	if (mid->driver_data == ucd9090 || mid->driver_data == ucd90160 ||
+	    mid->driver_data == ucd90910) {
+		entries = devm_kzalloc(&client->dev,
+				       sizeof(*entries) * UCD9000_GPI_COUNT,
+				       GFP_KERNEL);
+		if (!entries)
+			return -ENOMEM;
+
+		for (i = 0; i < UCD9000_GPI_COUNT; i++) {
+			entries[i].client = client;
+			entries[i].index = i;
+			scnprintf(name, UCD9000_DEBUGFS_NAME_LEN,
+				  "gpi%d_alarm", i + 1);
+			debugfs_create_file(name, 0444, data->debugfs,
+					    &entries[i],
+					    &ucd9000_debugfs_mfr_status_bit);
+		}
+	}
+
+	scnprintf(name, UCD9000_DEBUGFS_NAME_LEN, "mfr_status");
+	debugfs_create_file(name, 0444, data->debugfs, client,
+			    &ucd9000_debugfs_show_mfr_status_fops);
+
+	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 int ucd9000_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -464,7 +593,16 @@ static int ucd9000_probe(struct i2c_client *client,
 				 ret);
 	}
 
-	return pmbus_do_probe(client, mid, info);
+	ret = pmbus_do_probe(client, mid, info);
+	if (ret)
+		return ret;
+
+	ret = ucd9000_init_debugfs(client, mid, data);
+	if (ret)
+		dev_warn(&client->dev, "Failed to register debugfs: %d\n",
+			 ret);
+
+	return 0;
 }
 
 /* This is the driver that will be inserted */
-- 
1.8.3.1

Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface

[Andy Shevchenko]

On Tue, Mar 13, 2018 at 10:59 PM, Eddie James
<eajames@linux.vnet.ibm.com> wrote:
> From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>
> Add a struct gpio_chip and define some methods so that this device's
> I/O can be accessed via /sys/class/gpio.

> +       /*
> +        * Note:
> +        *
> +        * Pinmux support has not been added to the new gpio_chip.
> +        * This support should be added when possible given the mux
> +        * behavior of these IO devices.
> +        */

> +       data->gpio.label = (const char *)&client->name;

Hmm... Why do you need this casting?

> +       data->gpio.get_direction = ucd9000_gpio_get_direction;
> +       data->gpio.direction_input = ucd9000_gpio_direction_input;
> +       data->gpio.direction_output = ucd9000_gpio_direction_output;
> +       data->gpio.get = ucd9000_gpio_get;
> +       data->gpio.set = ucd9000_gpio_set;

> +       data->gpio.can_sleep = 1;

Isn't it type of boolean?

> +       data->gpio.base = -1;

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

> +       data->gpio.of_node = client->dev.of_node;

I think GPIO core does this for you.

> +       if (data->gpio.ngpio) {

Hmm...

I would rather reorganize the above part to a separate helper, like

static int ..._probe_gpio()
{
...
  switch () {
   default:
     return 0; /* GPIO part is optional */
  }
  return 0;
}

ret = _probe_gpio();
if (ret)
 dev_warn();

> +               ret = devm_gpiochip_add_data(&client->dev, &data->gpio,
> +                                            client);
> +               if (ret)
> +                       dev_warn(&client->dev, "Could not add gpiochip: %d\n",
> +                                ret);
> +       }
> +
>         return pmbus_do_probe(client, mid, info);
>  }
>
> --
> 1.8.3.1
>



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface

[Guenter Roeck]

On Tue, Mar 13, 2018 at 11:13:04PM +0200, Andy Shevchenko wrote:
> On Tue, Mar 13, 2018 at 10:59 PM, Eddie James
> <eajames@linux.vnet.ibm.com> wrote:
> > From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> >
> > Add a struct gpio_chip and define some methods so that this device's
> > I/O can be accessed via /sys/class/gpio.
> 
> > +       /*
> > +        * Note:
> > +        *
> > +        * Pinmux support has not been added to the new gpio_chip.
> > +        * This support should be added when possible given the mux
> > +        * behavior of these IO devices.
> > +        */
> 
> > +       data->gpio.label = (const char *)&client->name;
> 
> Hmm... Why do you need this casting?
> 
> > +       data->gpio.get_direction = ucd9000_gpio_get_direction;
> > +       data->gpio.direction_input = ucd9000_gpio_direction_input;
> > +       data->gpio.direction_output = ucd9000_gpio_direction_output;
> > +       data->gpio.get = ucd9000_gpio_get;
> > +       data->gpio.set = ucd9000_gpio_set;
> 
> > +       data->gpio.can_sleep = 1;
> 
> Isn't it type of boolean?
> 
You are right.

> > +       data->gpio.base = -1;
> 
> > +       data->gpio.parent = &client->dev;
> > +       data->gpio.owner = THIS_MODULE;
> 
> > +       data->gpio.of_node = client->dev.of_node;
> 
> I think GPIO core does this for you.
> 
Same here. I checked and found that it also sets the owner, so maybe this can be
dropped as well.

> > +       if (data->gpio.ngpio) {
> 
> Hmm...
> 
> I would rather reorganize the above part to a separate helper, like
> 
> static int ..._probe_gpio()
> {
> ...
>   switch () {
>    default:
>      return 0; /* GPIO part is optional */
>   }
>   return 0;
> }
> 
> ret = _probe_gpio();
> if (ret)
>  dev_warn();
> 

I am neutral to positiva on that.
Might as well add the call to devm_gpiochip_add_data()
into that function as well.

> > +               ret = devm_gpiochip_add_data(&client->dev, &data->gpio,
> > +                                            client);
> > +               if (ret)
> > +                       dev_warn(&client->dev, "Could not add gpiochip: %d\n",
> > +                                ret);
> > +       }
> > +
> >         return pmbus_do_probe(client, mid, info);
> >  }
> >
> > --
> > 1.8.3.1
> >
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface

[Eddie James]

On 03/14/2018 01:55 PM, Guenter Roeck wrote:
> On Tue, Mar 13, 2018 at 11:13:04PM +0200, Andy Shevchenko wrote:
>> On Tue, Mar 13, 2018 at 10:59 PM, Eddie James
>> <eajames@linux.vnet.ibm.com> wrote:
>>> From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>>>
>>> Add a struct gpio_chip and define some methods so that this device's
>>> I/O can be accessed via /sys/class/gpio.
>>> +       /*
>>> +        * Note:
>>> +        *
>>> +        * Pinmux support has not been added to the new gpio_chip.
>>> +        * This support should be added when possible given the mux
>>> +        * behavior of these IO devices.
>>> +        */
>>> +       data->gpio.label = (const char *)&client->name;
>> Hmm... Why do you need this casting?
>>
>>> +       data->gpio.get_direction = ucd9000_gpio_get_direction;
>>> +       data->gpio.direction_input = ucd9000_gpio_direction_input;
>>> +       data->gpio.direction_output = ucd9000_gpio_direction_output;
>>> +       data->gpio.get = ucd9000_gpio_get;
>>> +       data->gpio.set = ucd9000_gpio_set;
>>> +       data->gpio.can_sleep = 1;
>> Isn't it type of boolean?
>>
> You are right.
>
>>> +       data->gpio.base = -1;
>>> +       data->gpio.parent = &client->dev;
>>> +       data->gpio.owner = THIS_MODULE;
>>> +       data->gpio.of_node = client->dev.of_node;
>> I think GPIO core does this for you.
>>
> Same here. I checked and found that it also sets the owner, so maybe this can be
> dropped as well.

Ah true. Especially since there is a TODO: remove chip->owner in the core.

>
>>> +       if (data->gpio.ngpio) {
>> Hmm...
>>
>> I would rather reorganize the above part to a separate helper, like
>>
>> static int ..._probe_gpio()
>> {
>> ...
>>    switch () {
>>     default:
>>       return 0; /* GPIO part is optional */
>>    }
>>    return 0;
>> }
>>
>> ret = _probe_gpio();
>> if (ret)
>>   dev_warn();
>>
> I am neutral to positiva on that.
> Might as well add the call to devm_gpiochip_add_data()
> into that function as well.

Sure. I can do a v4.

Thanks,
Eddie

>
>>> +               ret = devm_gpiochip_add_data(&client->dev, &data->gpio,
>>> +                                            client);
>>> +               if (ret)
>>> +                       dev_warn(&client->dev, "Could not add gpiochip: %d\n",
>>> +                                ret);
>>> +       }
>>> +
>>>          return pmbus_do_probe(client, mid, info);
>>>   }
>>>
>>> --
>>> 1.8.3.1
>>>
>>
>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko

Re: [PATCH v2 2/2] hwmon: (ucd9000) Add debugfs attributes to provide mfr_status

[Guenter Roeck]

On Tue, Mar 13, 2018 at 03:59:09PM -0500, Eddie James wrote:
> From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> 
> Expose the gpiN_fault fields of mfr_status as individual debugfs
> attributes. This provides a way for users to be easily notified of gpi
> faults. Also provide the whole mfr_status register in debugfs.
> 
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> ---
>  drivers/hwmon/pmbus/ucd9000.c | 140 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 139 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> index 023fb9e..b073d8e 100644
> --- a/drivers/hwmon/pmbus/ucd9000.c
> +++ b/drivers/hwmon/pmbus/ucd9000.c
> @@ -19,6 +19,7 @@
>   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> @@ -36,6 +37,7 @@
>  #define UCD9000_NUM_PAGES		0xd6
>  #define UCD9000_FAN_CONFIG_INDEX	0xe7
>  #define UCD9000_FAN_CONFIG		0xe8
> +#define UCD9000_MFR_STATUS		0xf3
>  #define UCD9000_GPIO_SELECT		0xfa
>  #define UCD9000_GPIO_CONFIG		0xfb
>  #define UCD9000_DEVICE_ID		0xfd
> @@ -63,13 +65,22 @@
>  #define UCD901XX_NUM_GPIOS	26
>  #define UCD90910_NUM_GPIOS	26
>  
> +#define UCD9000_DEBUGFS_NAME_LEN	24
> +#define UCD9000_GPI_COUNT		8
> +
>  struct ucd9000_data {
>  	u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
>  	struct pmbus_driver_info info;
>  	struct gpio_chip gpio;
> +	struct dentry *debugfs;
>  };
>  #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
>  
> +struct ucd9000_debugfs_entry {
> +	struct i2c_client *client;
> +	u8 index;
> +};
> +
>  static int ucd9000_get_fan_config(struct i2c_client *client, int fan)
>  {
>  	int fan_config = 0;
> @@ -306,6 +317,124 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
>  					  val);
>  }
>  
> +#if IS_ENABLED(CONFIG_DEBUG_FS)

DEBUG_FS is bool, so #ifdef CONFIG_DEBUG_FS is fine here.

> +static int ucd9000_get_mfr_status(struct i2c_client *client, u8 *buffer)
> +{
> +	int ret = pmbus_set_page(client, 0);
> +
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * With the ucd90120 and ucd90124 devices, this command [MFR_STATUS]
> +	 * is 2 bytes long (bits 0-15).  With the ucd90240 this command is 5
> +	 * bytes long.  With all other devices, it is 4 bytes long.
> +	 */
> +	return i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS, buffer);
> +}
> +
> +static int ucd9000_debugfs_show_mfr_status_bit(void *data, u64 *val)
> +{
> +	struct ucd9000_debugfs_entry *entry = data;
> +	struct i2c_client *client = entry->client;
> +	u8 buffer[4];
> +	int ret;
> +
> +	/*
> +	 * This attribute is only created for devices that return 4 bytes for
> +	 * status_mfr, so it's safe to call with 4-byte buffer.
> +	 */
> +	ret = ucd9000_get_mfr_status(client, buffer);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Attribute only created for devices with gpi fault bits at bits
> +	 * 16-23, which is the second byte of the response.
> +	 */
> +	*val = !!(buffer[1] & BIT(entry->index));
> +
> +	return 0;
> +}
> +DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_bit,
> +			 ucd9000_debugfs_show_mfr_status_bit, NULL, "%1lld\n");
> +
> +static ssize_t ucd9000_debugfs_read_mfr_status(struct file *file,
> +					       char __user *buf, size_t count,
> +					       loff_t *ppos)
> +{
> +	struct i2c_client *client = file->private_data;
> +	u8 buffer[5] = { 0 };	/* Need max 5 bytes for any ucd9000 chip. */
> +	int ret;
> +
> +	ret = ucd9000_get_mfr_status(client, buffer);
> +	if (ret < 0)
> +		return ret;
> +
> +	return simple_read_from_buffer(buf, count, ppos, buffer, ret);

Doesn't this report the raw binary data to userspace ? The output
should be human-readable.

> +}
> +
> +static const struct file_operations ucd9000_debugfs_show_mfr_status_fops = {
> +	.llseek = noop_llseek,
> +	.read = ucd9000_debugfs_read_mfr_status,
> +	.open = simple_open,
> +};
> +
> +static int ucd9000_init_debugfs(struct i2c_client *client,
> +				const struct i2c_device_id *mid,
> +				struct ucd9000_data *data)
> +{
> +	struct dentry *debugfs;
> +	struct ucd9000_debugfs_entry *entries;
> +	int i;
> +	char name[UCD9000_DEBUGFS_NAME_LEN];
> +
> +	debugfs = pmbus_get_debugfs_dir(client);
> +	if (!debugfs)
> +		return -ENOENT;
> +
> +	data->debugfs = debugfs_create_dir(client->name, debugfs);
> +	if (!data->debugfs)
> +		return -ENOENT;
> +
> +	/*
> +	 * Of the chips this driver supports, only the UCD9090, UCD90160,
> +	 * and UCD90910 report GPI faults in their MFR_STATUS register, so only
> +	 * create the GPI fault debugfs attributes for those chips.
> +	 */
> +	if (mid->driver_data == ucd9090 || mid->driver_data == ucd90160 ||
> +	    mid->driver_data == ucd90910) {
> +		entries = devm_kzalloc(&client->dev,
> +				       sizeof(*entries) * UCD9000_GPI_COUNT,
> +				       GFP_KERNEL);
> +		if (!entries)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < UCD9000_GPI_COUNT; i++) {
> +			entries[i].client = client;
> +			entries[i].index = i;
> +			scnprintf(name, UCD9000_DEBUGFS_NAME_LEN,
> +				  "gpi%d_alarm", i + 1);
> +			debugfs_create_file(name, 0444, data->debugfs,
> +					    &entries[i],
> +					    &ucd9000_debugfs_mfr_status_bit);
> +		}
> +	}
> +
> +	scnprintf(name, UCD9000_DEBUGFS_NAME_LEN, "mfr_status");
> +	debugfs_create_file(name, 0444, data->debugfs, client,
> +			    &ucd9000_debugfs_show_mfr_status_fops);
> +
> +	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 int ucd9000_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -464,7 +593,16 @@ static int ucd9000_probe(struct i2c_client *client,
>  				 ret);
>  	}
>  
> -	return pmbus_do_probe(client, mid, info);
> +	ret = pmbus_do_probe(client, mid, info);
> +	if (ret)
> +		return ret;
> +
> +	ret = ucd9000_init_debugfs(client, mid, data);
> +	if (ret)
> +		dev_warn(&client->dev, "Failed to register debugfs: %d\n",
> +			 ret);
> +
> +	return 0;
>  }
>  
>  /* This is the driver that will be inserted */
> -- 
> 1.8.3.1
> 

Re: [PATCH v2 2/2] hwmon: (ucd9000) Add debugfs attributes to provide mfr_status

[Eddie James]

On 03/14/2018 02:19 PM, Guenter Roeck wrote:
> On Tue, Mar 13, 2018 at 03:59:09PM -0500, Eddie James wrote:
>> From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>>
>> Expose the gpiN_fault fields of mfr_status as individual debugfs
>> attributes. This provides a way for users to be easily notified of gpi
>> faults. Also provide the whole mfr_status register in debugfs.
>>
>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>> Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
>> ---
>>   drivers/hwmon/pmbus/ucd9000.c | 140 +++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 139 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
>> index 023fb9e..b073d8e 100644
>> --- a/drivers/hwmon/pmbus/ucd9000.c
>> +++ b/drivers/hwmon/pmbus/ucd9000.c
>> @@ -19,6 +19,7 @@
>>    * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>>    */
>>   
>> +#include <linux/debugfs.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/of_device.h>
>> @@ -36,6 +37,7 @@
>>   #define UCD9000_NUM_PAGES		0xd6
>>   #define UCD9000_FAN_CONFIG_INDEX	0xe7
>>   #define UCD9000_FAN_CONFIG		0xe8
>> +#define UCD9000_MFR_STATUS		0xf3
>>   #define UCD9000_GPIO_SELECT		0xfa
>>   #define UCD9000_GPIO_CONFIG		0xfb
>>   #define UCD9000_DEVICE_ID		0xfd
>> @@ -63,13 +65,22 @@
>>   #define UCD901XX_NUM_GPIOS	26
>>   #define UCD90910_NUM_GPIOS	26
>>   
>> +#define UCD9000_DEBUGFS_NAME_LEN	24
>> +#define UCD9000_GPI_COUNT		8
>> +
>>   struct ucd9000_data {
>>   	u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
>>   	struct pmbus_driver_info info;
>>   	struct gpio_chip gpio;
>> +	struct dentry *debugfs;
>>   };
>>   #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
>>   
>> +struct ucd9000_debugfs_entry {
>> +	struct i2c_client *client;
>> +	u8 index;
>> +};
>> +
>>   static int ucd9000_get_fan_config(struct i2c_client *client, int fan)
>>   {
>>   	int fan_config = 0;
>> @@ -306,6 +317,124 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
>>   					  val);
>>   }
>>   
>> +#if IS_ENABLED(CONFIG_DEBUG_FS)
> DEBUG_FS is bool, so #ifdef CONFIG_DEBUG_FS is fine here.

Ok.

>
>> +static int ucd9000_get_mfr_status(struct i2c_client *client, u8 *buffer)
>> +{
>> +	int ret = pmbus_set_page(client, 0);
>> +
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/*
>> +	 * With the ucd90120 and ucd90124 devices, this command [MFR_STATUS]
>> +	 * is 2 bytes long (bits 0-15).  With the ucd90240 this command is 5
>> +	 * bytes long.  With all other devices, it is 4 bytes long.
>> +	 */
>> +	return i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS, buffer);
>> +}
>> +
>> +static int ucd9000_debugfs_show_mfr_status_bit(void *data, u64 *val)
>> +{
>> +	struct ucd9000_debugfs_entry *entry = data;
>> +	struct i2c_client *client = entry->client;
>> +	u8 buffer[4];
>> +	int ret;
>> +
>> +	/*
>> +	 * This attribute is only created for devices that return 4 bytes for
>> +	 * status_mfr, so it's safe to call with 4-byte buffer.
>> +	 */
>> +	ret = ucd9000_get_mfr_status(client, buffer);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/*
>> +	 * Attribute only created for devices with gpi fault bits at bits
>> +	 * 16-23, which is the second byte of the response.
>> +	 */
>> +	*val = !!(buffer[1] & BIT(entry->index));
>> +
>> +	return 0;
>> +}
>> +DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_bit,
>> +			 ucd9000_debugfs_show_mfr_status_bit, NULL, "%1lld\n");
>> +
>> +static ssize_t ucd9000_debugfs_read_mfr_status(struct file *file,
>> +					       char __user *buf, size_t count,
>> +					       loff_t *ppos)
>> +{
>> +	struct i2c_client *client = file->private_data;
>> +	u8 buffer[5] = { 0 };	/* Need max 5 bytes for any ucd9000 chip. */
>> +	int ret;
>> +
>> +	ret = ucd9000_get_mfr_status(client, buffer);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return simple_read_from_buffer(buf, count, ppos, buffer, ret);
> Doesn't this report the raw binary data to userspace ? The output
> should be human-readable.

Yes, sorry I thought that was your suggestion when you mentioned hexdump 
earlier... Will fix.

Thanks,
Eddie

>
>> +}
>> +
>> +static const struct file_operations ucd9000_debugfs_show_mfr_status_fops = {
>> +	.llseek = noop_llseek,
>> +	.read = ucd9000_debugfs_read_mfr_status,
>> +	.open = simple_open,
>> +};
>> +
>> +static int ucd9000_init_debugfs(struct i2c_client *client,
>> +				const struct i2c_device_id *mid,
>> +				struct ucd9000_data *data)
>> +{
>> +	struct dentry *debugfs;
>> +	struct ucd9000_debugfs_entry *entries;
>> +	int i;
>> +	char name[UCD9000_DEBUGFS_NAME_LEN];
>> +
>> +	debugfs = pmbus_get_debugfs_dir(client);
>> +	if (!debugfs)
>> +		return -ENOENT;
>> +
>> +	data->debugfs = debugfs_create_dir(client->name, debugfs);
>> +	if (!data->debugfs)
>> +		return -ENOENT;
>> +
>> +	/*
>> +	 * Of the chips this driver supports, only the UCD9090, UCD90160,
>> +	 * and UCD90910 report GPI faults in their MFR_STATUS register, so only
>> +	 * create the GPI fault debugfs attributes for those chips.
>> +	 */
>> +	if (mid->driver_data == ucd9090 || mid->driver_data == ucd90160 ||
>> +	    mid->driver_data == ucd90910) {
>> +		entries = devm_kzalloc(&client->dev,
>> +				       sizeof(*entries) * UCD9000_GPI_COUNT,
>> +				       GFP_KERNEL);
>> +		if (!entries)
>> +			return -ENOMEM;
>> +
>> +		for (i = 0; i < UCD9000_GPI_COUNT; i++) {
>> +			entries[i].client = client;
>> +			entries[i].index = i;
>> +			scnprintf(name, UCD9000_DEBUGFS_NAME_LEN,
>> +				  "gpi%d_alarm", i + 1);
>> +			debugfs_create_file(name, 0444, data->debugfs,
>> +					    &entries[i],
>> +					    &ucd9000_debugfs_mfr_status_bit);
>> +		}
>> +	}
>> +
>> +	scnprintf(name, UCD9000_DEBUGFS_NAME_LEN, "mfr_status");
>> +	debugfs_create_file(name, 0444, data->debugfs, client,
>> +			    &ucd9000_debugfs_show_mfr_status_fops);
>> +
>> +	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 int ucd9000_probe(struct i2c_client *client,
>>   			 const struct i2c_device_id *id)
>>   {
>> @@ -464,7 +593,16 @@ static int ucd9000_probe(struct i2c_client *client,
>>   				 ret);
>>   	}
>>   
>> -	return pmbus_do_probe(client, mid, info);
>> +	ret = pmbus_do_probe(client, mid, info);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = ucd9000_init_debugfs(client, mid, data);
>> +	if (ret)
>> +		dev_warn(&client->dev, "Failed to register debugfs: %d\n",
>> +			 ret);
>> +
>> +	return 0;
>>   }
>>   
>>   /* This is the driver that will be inserted */
>> -- 
>> 1.8.3.1
>>

Re: [PATCH v2 2/2] hwmon: (ucd9000) Add debugfs attributes to provide mfr_status

[Guenter Roeck]

On Wed, Mar 14, 2018 at 02:46:55PM -0500, Eddie James wrote:
> 
> 
> On 03/14/2018 02:19 PM, Guenter Roeck wrote:
> >On Tue, Mar 13, 2018 at 03:59:09PM -0500, Eddie James wrote:
> >>From: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> >>
> >>Expose the gpiN_fault fields of mfr_status as individual debugfs
> >>attributes. This provides a way for users to be easily notified of gpi
> >>faults. Also provide the whole mfr_status register in debugfs.
> >>
> >>Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> >>Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> >>Signed-off-by: Eddie James <eajames@linux.vnet.ibm.com>
> >>---
> >>  drivers/hwmon/pmbus/ucd9000.c | 140 +++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 139 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/drivers/hwmon/pmbus/ucd9000.c b/drivers/hwmon/pmbus/ucd9000.c
> >>index 023fb9e..b073d8e 100644
> >>--- a/drivers/hwmon/pmbus/ucd9000.c
> >>+++ b/drivers/hwmon/pmbus/ucd9000.c
> >>@@ -19,6 +19,7 @@
> >>   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> >>   */
> >>+#include <linux/debugfs.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/module.h>
> >>  #include <linux/of_device.h>
> >>@@ -36,6 +37,7 @@
> >>  #define UCD9000_NUM_PAGES		0xd6
> >>  #define UCD9000_FAN_CONFIG_INDEX	0xe7
> >>  #define UCD9000_FAN_CONFIG		0xe8
> >>+#define UCD9000_MFR_STATUS		0xf3
> >>  #define UCD9000_GPIO_SELECT		0xfa
> >>  #define UCD9000_GPIO_CONFIG		0xfb
> >>  #define UCD9000_DEVICE_ID		0xfd
> >>@@ -63,13 +65,22 @@
> >>  #define UCD901XX_NUM_GPIOS	26
> >>  #define UCD90910_NUM_GPIOS	26
> >>+#define UCD9000_DEBUGFS_NAME_LEN	24
> >>+#define UCD9000_GPI_COUNT		8
> >>+
> >>  struct ucd9000_data {
> >>  	u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
> >>  	struct pmbus_driver_info info;
> >>  	struct gpio_chip gpio;
> >>+	struct dentry *debugfs;
> >>  };
> >>  #define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
> >>+struct ucd9000_debugfs_entry {
> >>+	struct i2c_client *client;
> >>+	u8 index;
> >>+};
> >>+
> >>  static int ucd9000_get_fan_config(struct i2c_client *client, int fan)
> >>  {
> >>  	int fan_config = 0;
> >>@@ -306,6 +317,124 @@ static int ucd9000_gpio_direction_output(struct gpio_chip *gc,
> >>  					  val);
> >>  }
> >>+#if IS_ENABLED(CONFIG_DEBUG_FS)
> >DEBUG_FS is bool, so #ifdef CONFIG_DEBUG_FS is fine here.
> 
> Ok.
> 
> >
> >>+static int ucd9000_get_mfr_status(struct i2c_client *client, u8 *buffer)
> >>+{
> >>+	int ret = pmbus_set_page(client, 0);
> >>+
> >>+	if (ret < 0)
> >>+		return ret;
> >>+
> >>+	/*
> >>+	 * With the ucd90120 and ucd90124 devices, this command [MFR_STATUS]
> >>+	 * is 2 bytes long (bits 0-15).  With the ucd90240 this command is 5
> >>+	 * bytes long.  With all other devices, it is 4 bytes long.
> >>+	 */
> >>+	return i2c_smbus_read_block_data(client, UCD9000_MFR_STATUS, buffer);
> >>+}
> >>+
> >>+static int ucd9000_debugfs_show_mfr_status_bit(void *data, u64 *val)
> >>+{
> >>+	struct ucd9000_debugfs_entry *entry = data;
> >>+	struct i2c_client *client = entry->client;
> >>+	u8 buffer[4];
> >>+	int ret;
> >>+
> >>+	/*
> >>+	 * This attribute is only created for devices that return 4 bytes for
> >>+	 * status_mfr, so it's safe to call with 4-byte buffer.
> >>+	 */
> >>+	ret = ucd9000_get_mfr_status(client, buffer);
> >>+	if (ret < 0)
> >>+		return ret;
> >>+
> >>+	/*
> >>+	 * Attribute only created for devices with gpi fault bits at bits
> >>+	 * 16-23, which is the second byte of the response.
> >>+	 */
> >>+	*val = !!(buffer[1] & BIT(entry->index));
> >>+
> >>+	return 0;
> >>+}
> >>+DEFINE_DEBUGFS_ATTRIBUTE(ucd9000_debugfs_mfr_status_bit,
> >>+			 ucd9000_debugfs_show_mfr_status_bit, NULL, "%1lld\n");
> >>+
> >>+static ssize_t ucd9000_debugfs_read_mfr_status(struct file *file,
> >>+					       char __user *buf, size_t count,
> >>+					       loff_t *ppos)
> >>+{
> >>+	struct i2c_client *client = file->private_data;
> >>+	u8 buffer[5] = { 0 };	/* Need max 5 bytes for any ucd9000 chip. */
> >>+	int ret;
> >>+
> >>+	ret = ucd9000_get_mfr_status(client, buffer);
> >>+	if (ret < 0)
> >>+		return ret;
> >>+
> >>+	return simple_read_from_buffer(buf, count, ppos, buffer, ret);
> >Doesn't this report the raw binary data to userspace ? The output
> >should be human-readable.
> 
> Yes, sorry I thought that was your suggestion when you mentioned hexdump
> earlier... Will fix.
> 
Sorry for the confusion. I meant a user-readable hexdump.

Guenter

> Thanks,
> Eddie
> 
> >
> >>+}
> >>+
> >>+static const struct file_operations ucd9000_debugfs_show_mfr_status_fops = {
> >>+	.llseek = noop_llseek,
> >>+	.read = ucd9000_debugfs_read_mfr_status,
> >>+	.open = simple_open,
> >>+};
> >>+
> >>+static int ucd9000_init_debugfs(struct i2c_client *client,
> >>+				const struct i2c_device_id *mid,
> >>+				struct ucd9000_data *data)
> >>+{
> >>+	struct dentry *debugfs;
> >>+	struct ucd9000_debugfs_entry *entries;
> >>+	int i;
> >>+	char name[UCD9000_DEBUGFS_NAME_LEN];
> >>+
> >>+	debugfs = pmbus_get_debugfs_dir(client);
> >>+	if (!debugfs)
> >>+		return -ENOENT;
> >>+
> >>+	data->debugfs = debugfs_create_dir(client->name, debugfs);
> >>+	if (!data->debugfs)
> >>+		return -ENOENT;
> >>+
> >>+	/*
> >>+	 * Of the chips this driver supports, only the UCD9090, UCD90160,
> >>+	 * and UCD90910 report GPI faults in their MFR_STATUS register, so only
> >>+	 * create the GPI fault debugfs attributes for those chips.
> >>+	 */
> >>+	if (mid->driver_data == ucd9090 || mid->driver_data == ucd90160 ||
> >>+	    mid->driver_data == ucd90910) {
> >>+		entries = devm_kzalloc(&client->dev,
> >>+				       sizeof(*entries) * UCD9000_GPI_COUNT,
> >>+				       GFP_KERNEL);
> >>+		if (!entries)
> >>+			return -ENOMEM;
> >>+
> >>+		for (i = 0; i < UCD9000_GPI_COUNT; i++) {
> >>+			entries[i].client = client;
> >>+			entries[i].index = i;
> >>+			scnprintf(name, UCD9000_DEBUGFS_NAME_LEN,
> >>+				  "gpi%d_alarm", i + 1);
> >>+			debugfs_create_file(name, 0444, data->debugfs,
> >>+					    &entries[i],
> >>+					    &ucd9000_debugfs_mfr_status_bit);
> >>+		}
> >>+	}
> >>+
> >>+	scnprintf(name, UCD9000_DEBUGFS_NAME_LEN, "mfr_status");
> >>+	debugfs_create_file(name, 0444, data->debugfs, client,
> >>+			    &ucd9000_debugfs_show_mfr_status_fops);
> >>+
> >>+	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 int ucd9000_probe(struct i2c_client *client,
> >>  			 const struct i2c_device_id *id)
> >>  {
> >>@@ -464,7 +593,16 @@ static int ucd9000_probe(struct i2c_client *client,
> >>  				 ret);
> >>  	}
> >>-	return pmbus_do_probe(client, mid, info);
> >>+	ret = pmbus_do_probe(client, mid, info);
> >>+	if (ret)
> >>+		return ret;
> >>+
> >>+	ret = ucd9000_init_debugfs(client, mid, data);
> >>+	if (ret)
> >>+		dev_warn(&client->dev, "Failed to register debugfs: %d\n",
> >>+			 ret);
> >>+
> >>+	return 0;
> >>  }
> >>  /* This is the driver that will be inserted */
> >>-- 
> >>1.8.3.1
> >>
> 

Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface

[kbuild test robot]

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

Hi Christopher,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc5 next-20180315]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eddie-James/hwmon-ucd9000-Add-gpio-and-debugfs-interfaces/20180316-125048
config: x86_64-randconfig-x019-201810 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All error/warnings (new ones prefixed by >>):

>> drivers/hwmon/pmbus/ucd9000.c:69:19: error: field 'gpio' has incomplete type
     struct gpio_chip gpio;
                      ^~~~
   drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_gpio_get':
>> drivers/hwmon/pmbus/ucd9000.c:184:31: error: implicit declaration of function 'gpiochip_get_data'; did you mean 'gpio_get_value'? [-Werror=implicit-function-declaration]
     struct i2c_client *client  = gpiochip_get_data(gc);
                                  ^~~~~~~~~~~~~~~~~
                                  gpio_get_value
>> drivers/hwmon/pmbus/ucd9000.c:184:31: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
   drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_gpio_set':
   drivers/hwmon/pmbus/ucd9000.c:197:30: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
     struct i2c_client *client = gpiochip_get_data(gc);
                                 ^~~~~~~~~~~~~~~~~
   drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_gpio_get_direction':
   drivers/hwmon/pmbus/ucd9000.c:240:30: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
     struct i2c_client *client = gpiochip_get_data(gc);
                                 ^~~~~~~~~~~~~~~~~
   drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_gpio_set_direction':
   drivers/hwmon/pmbus/ucd9000.c:254:30: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
     struct i2c_client *client = gpiochip_get_data(gc);
                                 ^~~~~~~~~~~~~~~~~
   drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_probe':
>> drivers/hwmon/pmbus/ucd9000.c:460:9: error: implicit declaration of function 'devm_gpiochip_add_data'; did you mean 'devm_gpio_request'? [-Werror=implicit-function-declaration]
      ret = devm_gpiochip_add_data(&client->dev, &data->gpio,
            ^~~~~~~~~~~~~~~~~~~~~~
            devm_gpio_request
   cc1: some warnings being treated as errors

vim +/gpio +69 drivers/hwmon/pmbus/ucd9000.c

    65	
    66	struct ucd9000_data {
    67		u8 fan_data[UCD9000_NUM_FAN][I2C_SMBUS_BLOCK_MAX];
    68		struct pmbus_driver_info info;
  > 69		struct gpio_chip gpio;
    70	};
    71	#define to_ucd9000_data(_info) container_of(_info, struct ucd9000_data, info)
    72	
    73	static int ucd9000_get_fan_config(struct i2c_client *client, int fan)
    74	{
    75		int fan_config = 0;
    76		struct ucd9000_data *data
    77		  = to_ucd9000_data(pmbus_get_driver_info(client));
    78	
    79		if (data->fan_data[fan][3] & 1)
    80			fan_config |= PB_FAN_2_INSTALLED;   /* Use lower bit position */
    81	
    82		/* Pulses/revolution */
    83		fan_config |= (data->fan_data[fan][3] & 0x06) >> 1;
    84	
    85		return fan_config;
    86	}
    87	
    88	static int ucd9000_read_byte_data(struct i2c_client *client, int page, int reg)
    89	{
    90		int ret = 0;
    91		int fan_config;
    92	
    93		switch (reg) {
    94		case PMBUS_FAN_CONFIG_12:
    95			if (page > 0)
    96				return -ENXIO;
    97	
    98			ret = ucd9000_get_fan_config(client, 0);
    99			if (ret < 0)
   100				return ret;
   101			fan_config = ret << 4;
   102			ret = ucd9000_get_fan_config(client, 1);
   103			if (ret < 0)
   104				return ret;
   105			fan_config |= ret;
   106			ret = fan_config;
   107			break;
   108		case PMBUS_FAN_CONFIG_34:
   109			if (page > 0)
   110				return -ENXIO;
   111	
   112			ret = ucd9000_get_fan_config(client, 2);
   113			if (ret < 0)
   114				return ret;
   115			fan_config = ret << 4;
   116			ret = ucd9000_get_fan_config(client, 3);
   117			if (ret < 0)
   118				return ret;
   119			fan_config |= ret;
   120			ret = fan_config;
   121			break;
   122		default:
   123			ret = -ENODATA;
   124			break;
   125		}
   126		return ret;
   127	}
   128	
   129	static const struct i2c_device_id ucd9000_id[] = {
   130		{"ucd9000", ucd9000},
   131		{"ucd90120", ucd90120},
   132		{"ucd90124", ucd90124},
   133		{"ucd90160", ucd90160},
   134		{"ucd9090", ucd9090},
   135		{"ucd90910", ucd90910},
   136		{}
   137	};
   138	MODULE_DEVICE_TABLE(i2c, ucd9000_id);
   139	
   140	static const struct of_device_id ucd9000_of_match[] = {
   141		{
   142			.compatible = "ti,ucd9000",
   143			.data = (void *)ucd9000
   144		},
   145		{
   146			.compatible = "ti,ucd90120",
   147			.data = (void *)ucd90120
   148		},
   149		{
   150			.compatible = "ti,ucd90124",
   151			.data = (void *)ucd90124
   152		},
   153		{
   154			.compatible = "ti,ucd90160",
   155			.data = (void *)ucd90160
   156		},
   157		{
   158			.compatible = "ti,ucd9090",
   159			.data = (void *)ucd9090
   160		},
   161		{
   162			.compatible = "ti,ucd90910",
   163			.data = (void *)ucd90910
   164		},
   165		{ },
   166	};
   167	MODULE_DEVICE_TABLE(of, ucd9000_of_match);
   168	
   169	static int ucd9000_gpio_read_config(struct i2c_client *client,
   170					    unsigned int offset)
   171	{
   172		int ret;
   173	
   174		/* No page set required */
   175		ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_SELECT, offset);
   176		if (ret < 0)
   177			return ret;
   178	
   179		return i2c_smbus_read_byte_data(client, UCD9000_GPIO_CONFIG);
   180	}
   181	
   182	static int ucd9000_gpio_get(struct gpio_chip *gc, unsigned int offset)
   183	{
 > 184		struct i2c_client *client  = gpiochip_get_data(gc);
   185		int ret;
   186	
   187		ret = ucd9000_gpio_read_config(client, offset);
   188		if (ret < 0)
   189			return ret;
   190	
   191		return !!(ret & UCD9000_GPIO_CONFIG_STATUS);
   192	}
   193	
   194	static void ucd9000_gpio_set(struct gpio_chip *gc, unsigned int offset,
   195				     int value)
   196	{
   197		struct i2c_client *client = gpiochip_get_data(gc);
   198		int ret;
   199	
   200		ret = ucd9000_gpio_read_config(client, offset);
   201		if (ret < 0) {
   202			dev_dbg(&client->dev, "failed to read GPIO %d config: %d\n",
   203				offset, ret);
   204			return;
   205		}
   206	
   207		if (value) {
   208			if (ret & UCD9000_GPIO_CONFIG_STATUS)
   209				return;
   210	
   211			ret |= UCD9000_GPIO_CONFIG_STATUS;
   212		} else {
   213			if (!(ret & UCD9000_GPIO_CONFIG_STATUS))
   214				return;
   215	
   216			ret &= ~UCD9000_GPIO_CONFIG_STATUS;
   217		}
   218	
   219		ret |= UCD9000_GPIO_CONFIG_ENABLE;
   220	
   221		/* Page set not required */
   222		ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
   223		if (ret < 0) {
   224			dev_dbg(&client->dev, "Failed to write GPIO %d config: %d\n",
   225				offset, ret);
   226			return;
   227		}
   228	
   229		ret &= ~UCD9000_GPIO_CONFIG_ENABLE;
   230	
   231		ret = i2c_smbus_write_byte_data(client, UCD9000_GPIO_CONFIG, ret);
   232		if (ret < 0)
   233			dev_dbg(&client->dev, "Failed to write GPIO %d config: %d\n",
   234				offset, ret);
   235	}
   236	
   237	static int ucd9000_gpio_get_direction(struct gpio_chip *gc,
   238					      unsigned int offset)
   239	{
 > 240		struct i2c_client *client = gpiochip_get_data(gc);
   241		int ret;
   242	
   243		ret = ucd9000_gpio_read_config(client, offset);
   244		if (ret < 0)
   245			return ret;
   246	
   247		return !(ret & UCD9000_GPIO_CONFIG_OUT_ENABLE);
   248	}
   249	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31317 bytes --]

Re: [PATCH v2 1/2] hwmon: (ucd9000) Add gpio chip interface

[kbuild test robot]

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

Hi Christopher,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc5 next-20180315]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eddie-James/hwmon-ucd9000-Add-gpio-and-debugfs-interfaces/20180316-125048
config: x86_64-randconfig-x017-201810 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/hwmon/pmbus/ucd9000.c: In function 'ucd9000_probe':
>> drivers/hwmon/pmbus/ucd9000.c:457:12: error: 'struct gpio_chip' has no member named 'of_node'
     data->gpio.of_node = client->dev.of_node;
               ^

vim +457 drivers/hwmon/pmbus/ucd9000.c

   308	
   309	static int ucd9000_probe(struct i2c_client *client,
   310				 const struct i2c_device_id *id)
   311	{
   312		u8 block_buffer[I2C_SMBUS_BLOCK_MAX + 1];
   313		struct ucd9000_data *data;
   314		struct pmbus_driver_info *info;
   315		const struct i2c_device_id *mid;
   316		enum chips chip;
   317		int i, ret;
   318	
   319		if (!i2c_check_functionality(client->adapter,
   320					     I2C_FUNC_SMBUS_BYTE_DATA |
   321					     I2C_FUNC_SMBUS_BLOCK_DATA))
   322			return -ENODEV;
   323	
   324		ret = i2c_smbus_read_block_data(client, UCD9000_DEVICE_ID,
   325						block_buffer);
   326		if (ret < 0) {
   327			dev_err(&client->dev, "Failed to read device ID\n");
   328			return ret;
   329		}
   330		block_buffer[ret] = '\0';
   331		dev_info(&client->dev, "Device ID %s\n", block_buffer);
   332	
   333		for (mid = ucd9000_id; mid->name[0]; mid++) {
   334			if (!strncasecmp(mid->name, block_buffer, strlen(mid->name)))
   335				break;
   336		}
   337		if (!mid->name[0]) {
   338			dev_err(&client->dev, "Unsupported device\n");
   339			return -ENODEV;
   340		}
   341	
   342		if (client->dev.of_node)
   343			chip = (enum chips)of_device_get_match_data(&client->dev);
   344		else
   345			chip = id->driver_data;
   346	
   347		if (chip != ucd9000 && chip != mid->driver_data)
   348			dev_notice(&client->dev,
   349				   "Device mismatch: Configured %s, detected %s\n",
   350				   id->name, mid->name);
   351	
   352		data = devm_kzalloc(&client->dev, sizeof(struct ucd9000_data),
   353				    GFP_KERNEL);
   354		if (!data)
   355			return -ENOMEM;
   356		info = &data->info;
   357	
   358		ret = i2c_smbus_read_byte_data(client, UCD9000_NUM_PAGES);
   359		if (ret < 0) {
   360			dev_err(&client->dev,
   361				"Failed to read number of active pages\n");
   362			return ret;
   363		}
   364		info->pages = ret;
   365		if (!info->pages) {
   366			dev_err(&client->dev, "No pages configured\n");
   367			return -ENODEV;
   368		}
   369	
   370		/* The internal temperature sensor is always active */
   371		info->func[0] = PMBUS_HAVE_TEMP;
   372	
   373		/* Everything else is configurable */
   374		ret = i2c_smbus_read_block_data(client, UCD9000_MONITOR_CONFIG,
   375						block_buffer);
   376		if (ret <= 0) {
   377			dev_err(&client->dev, "Failed to read configuration data\n");
   378			return -ENODEV;
   379		}
   380		for (i = 0; i < ret; i++) {
   381			int page = UCD9000_MON_PAGE(block_buffer[i]);
   382	
   383			if (page >= info->pages)
   384				continue;
   385	
   386			switch (UCD9000_MON_TYPE(block_buffer[i])) {
   387			case UCD9000_MON_VOLTAGE:
   388			case UCD9000_MON_VOLTAGE_HW:
   389				info->func[page] |= PMBUS_HAVE_VOUT
   390				  | PMBUS_HAVE_STATUS_VOUT;
   391				break;
   392			case UCD9000_MON_TEMPERATURE:
   393				info->func[page] |= PMBUS_HAVE_TEMP2
   394				  | PMBUS_HAVE_STATUS_TEMP;
   395				break;
   396			case UCD9000_MON_CURRENT:
   397				info->func[page] |= PMBUS_HAVE_IOUT
   398				  | PMBUS_HAVE_STATUS_IOUT;
   399				break;
   400			default:
   401				break;
   402			}
   403		}
   404	
   405		/* Fan configuration */
   406		if (mid->driver_data == ucd90124) {
   407			for (i = 0; i < UCD9000_NUM_FAN; i++) {
   408				i2c_smbus_write_byte_data(client,
   409							  UCD9000_FAN_CONFIG_INDEX, i);
   410				ret = i2c_smbus_read_block_data(client,
   411								UCD9000_FAN_CONFIG,
   412								data->fan_data[i]);
   413				if (ret < 0)
   414					return ret;
   415			}
   416			i2c_smbus_write_byte_data(client, UCD9000_FAN_CONFIG_INDEX, 0);
   417	
   418			info->read_byte_data = ucd9000_read_byte_data;
   419			info->func[0] |= PMBUS_HAVE_FAN12 | PMBUS_HAVE_STATUS_FAN12
   420			  | PMBUS_HAVE_FAN34 | PMBUS_HAVE_STATUS_FAN34;
   421		}
   422	
   423		/*
   424		 * Note:
   425		 *
   426		 * Pinmux support has not been added to the new gpio_chip.
   427		 * This support should be added when possible given the mux
   428		 * behavior of these IO devices.
   429		 */
   430		data->gpio.label = (const char *)&client->name;
   431		data->gpio.get_direction = ucd9000_gpio_get_direction;
   432		data->gpio.direction_input = ucd9000_gpio_direction_input;
   433		data->gpio.direction_output = ucd9000_gpio_direction_output;
   434		data->gpio.get = ucd9000_gpio_get;
   435		data->gpio.set = ucd9000_gpio_set;
   436		data->gpio.can_sleep = 1;
   437		data->gpio.base = -1;
   438	
   439		switch (mid->driver_data) {
   440		case ucd9090:
   441			data->gpio.ngpio = UCD9090_NUM_GPIOS;
   442			break;
   443		case ucd90120:
   444		case ucd90124:
   445		case ucd90160:
   446			data->gpio.ngpio = UCD901XX_NUM_GPIOS;
   447			break;
   448		case ucd90910:
   449			data->gpio.ngpio = UCD90910_NUM_GPIOS;
   450			break;
   451		default:
   452			break;
   453		}
   454	
   455		data->gpio.parent = &client->dev;
   456		data->gpio.owner = THIS_MODULE;
 > 457		data->gpio.of_node = client->dev.of_node;
   458	
   459		if (data->gpio.ngpio) {
   460			ret = devm_gpiochip_add_data(&client->dev, &data->gpio,
   461						     client);
   462			if (ret)
   463				dev_warn(&client->dev, "Could not add gpiochip: %d\n",
   464					 ret);
   465		}
   466	
   467		return pmbus_do_probe(client, mid, info);
   468	}
   469	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26033 bytes --]