hwmon: (ucd9000) Add gpio and debugfs interfaces

PATCH Superseded Eddie James 4 participants 11 messages Mar 13, 2018 – Mar 16, 2018 Hardware monitoring

Share & tools

Eddie James

author

2018-03-13 20:59 UTC #0 lore raw

v2 cover hwmon: (ucd9000) Add gpio and debugfs interfaces

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(-)

Signature · 1 line

1.8.3.1

Eddie James

author

2018-03-13 20:59 UTC #1 lore raw

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

From: Christopher Bostic <[email protected]>

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 <[email protected]>

Signed-off-by Andrew Jeffery <[email protected]>

Signed-off-by Eddie James <[email protected]>

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

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

index b74dbec..023fb9e 100644
@@ -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);
 }

Signature · 1 line

1.8.3.1

Andy Shevchenko

2018-03-13 21:13 UTC #2 lore raw

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

Quoted 5 lines · Eddie James

From: Christopher Bostic <[email protected]>
Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client,

Quoted 9 lines

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

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

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client,

Quoted 7 lines

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

+       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?

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client,

Quoted 6 lines

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

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

Quoted 1 line

+ 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();

Quoted 13 lines

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

+               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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Guenter Roeck

maintainer

2018-03-14 18:55 UTC #3 lore raw

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

Quoted 25 lines · Andy Shevchenko

Quoted 5 lines · Eddie James
From: Christopher Bostic <[email protected]>
Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client,
Quoted 9 lines
drivers/hwmon/pmbus/ucd9000.c +8 -0
+       /*
+        * 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?
Quoted 5 lines
drivers/hwmon/pmbus/ucd9000.c +5 -0
+       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;

Quoted 4 lines

Quoted 1 line
+ data->gpio.can_sleep = 1;
Isn't it type of boolean?

You are right.

Quoted 9 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client,
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+       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.

Quoted 16 lines

Quoted 1 line
+ 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;
}

Quoted 4 lines

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.

Quoted 19 lines

Quoted 13 lines

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

+               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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Eddie James

author

2018-03-14 19:01 UTC #4 lore raw

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

Quoted 27 lines · Guenter Roeck

Quoted 16 lines · Andy Shevchenko
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 430
Quoted 12 lines · Eddie James
From: Christopher Bostic <[email protected]>
Add a struct gpio_chip and define some methods so that this device's I/O can be accessed via /sys/class/gpio.
drivers/hwmon/pmbus/ucd9000.c +8 -0
+       /*
+        * 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?
Quoted 8 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 436
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+       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.

Quoted 8 lines

Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c static int ucd9000_probe(struct i2c_client *client, · line 457
Quoted 4 lines
drivers/hwmon/pmbus/ucd9000.c +4 -0
+       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.

Quoted 16 lines

Quoted 16 lines
Quoted 2 lines
+ 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;
}

Quoted 7 lines

Quoted 4 lines
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

Quoted 19 lines

Quoted 14 lines

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

+               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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

kbuild test robot

build bot

2018-03-16 05:26 UTC #5 lore raw

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

In reply to Eddie James

Thank you for the patch! Yet something to improve:

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

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

Eddie James

author

2018-03-13 20:59 UTC #7 lore raw

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

In reply to Eddie James

From: Christopher Bostic <[email protected]>

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 <[email protected]>

Signed-off-by Andrew Jeffery <[email protected]>

Signed-off-by Eddie James <[email protected]>

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

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

index 023fb9e..b073d8e 100644
@@ -19,6 +19,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/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 */

Signature · 1 line

1.8.3.1

Guenter Roeck

maintainer

2018-03-14 19:19 UTC #8 lore raw

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

Quoted 57 lines · Eddie James

From: Christopher Bostic <[email protected]>

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

Signed-off-by Andrew Jeffery <[email protected]>

Signed-off-by Eddie James <[email protected]>

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

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

index 023fb9e..b073d8e 100644
@@ -19,6 +19,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
+#include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/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;

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

Quoted 4 lines

drivers/hwmon/pmbus/ucd9000.c +1 -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.

Quoted 44 lines

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

+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)

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

Quoted 10 lines

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

+{
+	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.

Quoted 87 lines

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

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

Signature · 1 line

1.8.3.1

Eddie James

author

2018-03-14 19:47 UTC #9 lore raw

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

Quoted 61 lines · Guenter Roeck

Quoted 60 lines · Eddie James

From: Christopher Bostic <[email protected]>

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

Signed-off-by Andrew Jeffery <[email protected]>

Signed-off-by Eddie James <[email protected]>

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

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

index 023fb9e..b073d8e 100644
@@ -19,6 +19,7 @@
   * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
   */
 
+#include <linux/debugfs.h>
  #include <linux/kernel.h>
  #include <linux/module.h>
  #include <linux/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);
  }

Quoted 2 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 320
Quoted 1 line
drivers/hwmon/pmbus/ucd9000.c +1 -0
+#if IS_ENABLED(CONFIG_DEBUG_FS)
DEBUG_FS is bool, so #ifdef CONFIG_DEBUG_FS is fine here.

Ok.

Quoted 49 lines

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

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

Quoted 8 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 374
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+
+	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

Quoted 88 lines

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

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

Signature · 1 line

1.8.3.1

Guenter Roeck

maintainer

2018-03-14 21:16 UTC #10 lore raw

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

Quoted 123 lines · Eddie James

Quoted 51 lines · Guenter Roeck

Quoted 50 lines · Eddie James

From: Christopher Bostic <[email protected]>

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

Signed-off-by Andrew Jeffery <[email protected]>

Signed-off-by Eddie James <[email protected]>

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

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

index 023fb9e..b073d8e 100644
@@ -19,6 +19,7 @@
  * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
+#include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/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;
+};
+

Quoted 8 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 319
Quoted 7 lines
drivers/hwmon/pmbus/ucd9000.c +1 -0
 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.

Quoted 49 lines

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

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

Quoted 8 lines

drivers/hwmon/pmbus/ucd9000.c static int ucd9000_gpio_direction_output(struct gpio_chip *gc, · line 374
Quoted 6 lines
drivers/hwmon/pmbus/ucd9000.c +6 -0
+
+	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.

Quoted 3 lines

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

Quoted 90 lines

Thanks,
Eddie

Quoted 86 lines

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

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

Signature · 1 line

1.8.3.1