[linux,dev-4.10,2/2] hwmon: (pmbus): cffps: Add input_history debugfs file

From: "Edward A. James" <eajames@us.ibm.com>

Provide binary data of the powers supply input history to debugfs.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/hwmon/pmbus/ibm-cffps.c | 97 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)

Reviewed-by: Matt Spinler mspinler@linux.vnet.ibm.com


On 11/2/2017 3:59 PM, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Provide binary data of the powers supply input history to debugfs.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>   drivers/hwmon/pmbus/ibm-cffps.c | 97 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 96 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
> index 1a6294c..0570c12 100644
> --- a/drivers/hwmon/pmbus/ibm-cffps.c
> +++ b/drivers/hwmon/pmbus/ibm-cffps.c
> @@ -7,13 +7,19 @@
>    * (at your option) any later version.
>    */
>
> +#include <linux/debugfs.h>
>   #include <linux/device.h>
> +#include <linux/fs.h>
>   #include <linux/i2c.h>
>   #include <linux/jiffies.h>
>   #include <linux/module.h>
> +#include <linux/mutex.h>
>
>   #include "pmbus.h"
>
> +#define CFFPS_INPUT_HISTORY_CMD			0xD6
> +#define CFFPS_INPUT_HISTORY_SIZE		100
> +
>   /* STATUS_MFR_SPECIFIC bits */
>   #define CFFPS_MFR_FAN_FAULT			BIT(0)
>   #define CFFPS_MFR_THERMAL_FAULT			BIT(1)
> @@ -24,6 +30,70 @@
>   #define CFFPS_MFR_VAUX_FAULT			BIT(6)
>   #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
>
> +struct ibm_cffps {
> +	struct i2c_client *client;
> +
> +	struct mutex update_lock;
> +	unsigned long last_updated;
> +
> +	u8 byte_count;
> +	u8 input_history[CFFPS_INPUT_HISTORY_SIZE];
> +};
> +
> +static ssize_t ibm_cffps_read_input_history(struct file *file,
> +					    char __user *buf, size_t count,
> +					    loff_t *ppos)
> +{
> +	int rc;
> +	struct ibm_cffps *psu = file->private_data;
> +	u8 msgbuf0[1] = { CFFPS_INPUT_HISTORY_CMD };
> +	u8 msgbuf1[CFFPS_INPUT_HISTORY_SIZE + 1] = { 0 };
> +	struct i2c_msg msg[2] = {
> +		{
> +			.addr = psu->client->addr,
> +			.flags = psu->client->flags,
> +			.len = 1,
> +			.buf = msgbuf0,
> +		}, {
> +			.addr = psu->client->addr,
> +			.flags = psu->client->flags | I2C_M_RD,
> +			.len = CFFPS_INPUT_HISTORY_SIZE + 1,
> +			.buf = msgbuf1,
> +		},
> +	};
> +
> +	if (!*ppos) {
> +		mutex_lock(&psu->update_lock);
> +		if (time_after(jiffies, psu->last_updated + HZ)) {
> +			/*
> +			 * Use a raw i2c transfer, since we need more bytes
> +			 * than Linux I2C supports through smbus xfr (only 32).
> +			 */
> +			rc = i2c_transfer(psu->client->adapter, msg, 2);
> +			if (rc < 0) {
> +				mutex_unlock(&psu->update_lock);
> +				return rc;
> +			}
> +
> +			psu->byte_count = msgbuf1[0];
> +			memcpy(psu->input_history, &msgbuf1[1],
> +			       CFFPS_INPUT_HISTORY_SIZE);
> +			psu->last_updated = jiffies;
> +		}
> +
> +		mutex_unlock(&psu->update_lock);
> +	}
> +
> +	return simple_read_from_buffer(buf, count, ppos, psu->input_history,
> +				       psu->byte_count);
> +}
> +
> +static const struct file_operations ibm_cffps_fops = {
> +	.llseek = noop_llseek,
> +	.read = ibm_cffps_read_input_history,
> +	.open = simple_open,
> +};
> +
>   static int ibm_cffps_read_byte_data(struct i2c_client *client, int page,
>   				    int reg)
>   {
> @@ -119,7 +189,32 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
>   static int ibm_cffps_probe(struct i2c_client *client,
>   			   const struct i2c_device_id *id)
>   {
> -	return pmbus_do_probe(client, id, &ibm_cffps_info);
> +	int rc;
> +	struct dentry *debugfs;
> +	struct ibm_cffps *psu;
> +
> +	rc = pmbus_do_probe(client, id, &ibm_cffps_info);
> +	if (rc)
> +		return rc;
> +
> +	/* Don't fail the probe if we can't create debugfs */
> +	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
> +	if (!psu)
> +		return 0;
> +
> +	psu->client = client;
> +	mutex_init(&psu->update_lock);
> +	if (jiffies > HZ)
> +		psu->last_updated = jiffies - HZ;
> +
> +	debugfs = pmbus_get_debugfs_dir(client);
> +	if (!debugfs)
> +		return 0;
> +
> +	debugfs_create_file("ibm_cffps_input_history", 0444, debugfs, psu,
> +			    &ibm_cffps_fops);
> +
> +	return 0;
>   }
>
>   static const struct i2c_device_id ibm_cffps_id[] = {
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    <p>Reviewed-by: Matt Spinler <a class="moz-txt-link-abbreviated"
        href="mailto:mspinler@linux.vnet.ibm.com">mspinler@linux.vnet.ibm.com</a></p>
    <br>
    <div class="moz-cite-prefix">On 11/2/2017 3:59 PM, Eddie James
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:1509656381-22755-3-git-send-email-eajames@linux.vnet.ibm.com">
      <pre wrap="">From: "Edward A. James" <a class="moz-txt-link-rfc2396E" href="mailto:eajames@us.ibm.com">&lt;eajames@us.ibm.com&gt;</a>

Provide binary data of the powers supply input history to debugfs.

Signed-off-by: Edward A. James <a class="moz-txt-link-rfc2396E" href="mailto:eajames@us.ibm.com">&lt;eajames@us.ibm.com&gt;</a>
---
 drivers/hwmon/pmbus/ibm-cffps.c | 97 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 96 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
index 1a6294c..0570c12 100644
--- a/drivers/hwmon/pmbus/ibm-cffps.c
+++ b/drivers/hwmon/pmbus/ibm-cffps.c
@@ -7,13 +7,19 @@
  * (at your option) any later version.
  */

+#include &lt;linux/debugfs.h&gt;
 #include &lt;linux/device.h&gt;
+#include &lt;linux/fs.h&gt;
 #include &lt;linux/i2c.h&gt;
 #include &lt;linux/jiffies.h&gt;
 #include &lt;linux/module.h&gt;
+#include &lt;linux/mutex.h&gt;

 #include "pmbus.h"

+#define CFFPS_INPUT_HISTORY_CMD			0xD6
+#define CFFPS_INPUT_HISTORY_SIZE		100
+
 /* STATUS_MFR_SPECIFIC bits */
 #define CFFPS_MFR_FAN_FAULT			BIT(0)
 #define CFFPS_MFR_THERMAL_FAULT			BIT(1)
@@ -24,6 +30,70 @@
 #define CFFPS_MFR_VAUX_FAULT			BIT(6)
 #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)

+struct ibm_cffps {
+	struct i2c_client *client;
+
+	struct mutex update_lock;
+	unsigned long last_updated;
+
+	u8 byte_count;
+	u8 input_history[CFFPS_INPUT_HISTORY_SIZE];
+};
+
+static ssize_t ibm_cffps_read_input_history(struct file *file,
+					    char __user *buf, size_t count,
+					    loff_t *ppos)
+{
+	int rc;
+	struct ibm_cffps *psu = file-&gt;private_data;
+	u8 msgbuf0[1] = { CFFPS_INPUT_HISTORY_CMD };
+	u8 msgbuf1[CFFPS_INPUT_HISTORY_SIZE + 1] = { 0 };
+	struct i2c_msg msg[2] = {
+		{
+			.addr = psu-&gt;client-&gt;addr,
+			.flags = psu-&gt;client-&gt;flags,
+			.len = 1,
+			.buf = msgbuf0,
+		}, {
+			.addr = psu-&gt;client-&gt;addr,
+			.flags = psu-&gt;client-&gt;flags | I2C_M_RD,
+			.len = CFFPS_INPUT_HISTORY_SIZE + 1,
+			.buf = msgbuf1,
+		},
+	};
+
+	if (!*ppos) {
+		mutex_lock(&amp;psu-&gt;update_lock);
+		if (time_after(jiffies, psu-&gt;last_updated + HZ)) {
+			/*
+			 * Use a raw i2c transfer, since we need more bytes
+			 * than Linux I2C supports through smbus xfr (only 32).
+			 */
+			rc = i2c_transfer(psu-&gt;client-&gt;adapter, msg, 2);
+			if (rc &lt; 0) {
+				mutex_unlock(&amp;psu-&gt;update_lock);
+				return rc;
+			}
+
+			psu-&gt;byte_count = msgbuf1[0];
+			memcpy(psu-&gt;input_history, &amp;msgbuf1[1],
+			       CFFPS_INPUT_HISTORY_SIZE);
+			psu-&gt;last_updated = jiffies;
+		}
+
+		mutex_unlock(&amp;psu-&gt;update_lock);
+	}
+
+	return simple_read_from_buffer(buf, count, ppos, psu-&gt;input_history,
+				       psu-&gt;byte_count);
+}
+
+static const struct file_operations ibm_cffps_fops = {
+	.llseek = noop_llseek,
+	.read = ibm_cffps_read_input_history,
+	.open = simple_open,
+};
+
 static int ibm_cffps_read_byte_data(struct i2c_client *client, int page,
 				    int reg)
 {
@@ -119,7 +189,32 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
 static int ibm_cffps_probe(struct i2c_client *client,
 			   const struct i2c_device_id *id)
 {
-	return pmbus_do_probe(client, id, &amp;ibm_cffps_info);
+	int rc;
+	struct dentry *debugfs;
+	struct ibm_cffps *psu;
+
+	rc = pmbus_do_probe(client, id, &amp;ibm_cffps_info);
+	if (rc)
+		return rc;
+
+	/* Don't fail the probe if we can't create debugfs */
+	psu = devm_kzalloc(&amp;client-&gt;dev, sizeof(*psu), GFP_KERNEL);
+	if (!psu)
+		return 0;
+
+	psu-&gt;client = client;
+	mutex_init(&amp;psu-&gt;update_lock);
+	if (jiffies &gt; HZ)
+		psu-&gt;last_updated = jiffies - HZ;
+
+	debugfs = pmbus_get_debugfs_dir(client);
+	if (!debugfs)
+		return 0;
+
+	debugfs_create_file("ibm_cffps_input_history", 0444, debugfs, psu,
+			    &amp;ibm_cffps_fops);
+
+	return 0;
 }

 static const struct i2c_device_id ibm_cffps_id[] = {
</pre>
    </blockquote>
    <br>
  </body>
</html>

On Thu, 2017-11-02 at 15:59 -0500, Eddie James wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
> 
> Provide binary data of the powers supply input history to debugfs.

Please improve this commit message to discuss the finer points of the
approach you took. For instance, getting the history off of the PSU
involves some annoying quasi-duplication of core functions with the
ibm_cffps_read_input_history() implementation.

> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  drivers/hwmon/pmbus/ibm-cffps.c | 97 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 96 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/pmbus/ibm-cffps.c b/drivers/hwmon/pmbus/ibm-cffps.c
> index 1a6294c..0570c12 100644
> --- a/drivers/hwmon/pmbus/ibm-cffps.c
> +++ b/drivers/hwmon/pmbus/ibm-cffps.c
> @@ -7,13 +7,19 @@
>   * (at your option) any later version.
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/device.h>
> +#include <linux/fs.h>
>  #include <linux/i2c.h>
>  #include <linux/jiffies.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  
>  #include "pmbus.h"
>  
> +#define CFFPS_INPUT_HISTORY_CMD			0xD6
> +#define CFFPS_INPUT_HISTORY_SIZE		100
> +
>  /* STATUS_MFR_SPECIFIC bits */
>  #define CFFPS_MFR_FAN_FAULT			BIT(0)
>  #define CFFPS_MFR_THERMAL_FAULT			BIT(1)
> @@ -24,6 +30,70 @@
>  #define CFFPS_MFR_VAUX_FAULT			BIT(6)
>  #define CFFPS_MFR_CURRENT_SHARE_WARNING		BIT(7)
>  
> +struct ibm_cffps {
> +	struct i2c_client *client;
> +
> +	struct mutex update_lock;
> +	unsigned long last_updated;
> +
> +	u8 byte_count;
> +	u8 input_history[CFFPS_INPUT_HISTORY_SIZE];
> +};
> +
> +static ssize_t ibm_cffps_read_input_history(struct file *file,
> +					    char __user *buf, size_t count,
> +					    loff_t *ppos)
> +{
> +	int rc;
> +	struct ibm_cffps *psu = file->private_data;
> +	u8 msgbuf0[1] = { CFFPS_INPUT_HISTORY_CMD };
> +	u8 msgbuf1[CFFPS_INPUT_HISTORY_SIZE + 1] = { 0 };
> +	struct i2c_msg msg[2] = {
> +		{
> +			.addr = psu->client->addr,
> +			.flags = psu->client->flags,
> +			.len = 1,
> +			.buf = msgbuf0,
> +		}, {
> +			.addr = psu->client->addr,
> +			.flags = psu->client->flags | I2C_M_RD,
> +			.len = CFFPS_INPUT_HISTORY_SIZE + 1,
> +			.buf = msgbuf1,
> +		},
> +	};
> +
> +	if (!*ppos) {
> +		mutex_lock(&psu->update_lock);
> +		if (time_after(jiffies, psu->last_updated + HZ)) {
> +			/*
> +			 * Use a raw i2c transfer, since we need more bytes
> +			 * than Linux I2C supports through smbus xfr (only 32).
> +			 */
> +			rc = i2c_transfer(psu->client->adapter, msg, 2);
> +			if (rc < 0) {
> +				mutex_unlock(&psu->update_lock);
> +				return rc;
> +			}
> +
> +			psu->byte_count = msgbuf1[0];
> +			memcpy(psu->input_history, &msgbuf1[1],
> +			       CFFPS_INPUT_HISTORY_SIZE);
> +			psu->last_updated = jiffies;
> +		}
> +
> +		mutex_unlock(&psu->update_lock);
> +	}
> +
> +	return simple_read_from_buffer(buf, count, ppos, psu->input_history,
> +				       psu->byte_count);
> +}
> +
> +static const struct file_operations ibm_cffps_fops = {
> +	.llseek = noop_llseek,
> +	.read = ibm_cffps_read_input_history,
> +	.open = simple_open,
> +};
> +
>  static int ibm_cffps_read_byte_data(struct i2c_client *client, int page,
>  				    int reg)
>  {
> @@ -119,7 +189,32 @@ static int ibm_cffps_read_word_data(struct i2c_client *client, int page,
>  static int ibm_cffps_probe(struct i2c_client *client,
>  			   const struct i2c_device_id *id)
>  {
> -	return pmbus_do_probe(client, id, &ibm_cffps_info);
> +	int rc;
> +	struct dentry *debugfs;
> +	struct ibm_cffps *psu;
> +
> +	rc = pmbus_do_probe(client, id, &ibm_cffps_info);
> +	if (rc)
> +		return rc;
> +
> +	/* Don't fail the probe if we can't create debugfs */
> +	psu = devm_kzalloc(&client->dev, sizeof(*psu), GFP_KERNEL);
> +	if (!psu)
> +		return 0;
> +
> +	psu->client = client;
> +	mutex_init(&psu->update_lock);
> +	if (jiffies > HZ)
> +		psu->last_updated = jiffies - HZ;

I think we can ditch the test: jiffies is an unsigned long and unsigned
underflow is defined as wrapping. If jiffies <= HZ, then jiffies - HZ
becomes (jiffies - HZ) + ULONG_MAX + 1, which can be rewritten as
ULONG_MAX - HZ + jiffies + 1 to keep your sanity in an unsigned world.
Then because you're using time_after() in the read() function, the
wrapping is taken care of in the jiffies <= HZ case (and naturally the
jiffies > HZ case is catered for), and we're home.

> +
> +	debugfs = pmbus_get_debugfs_dir(client);
> +	if (!debugfs)
> +		return 0;
> +
> +	debugfs_create_file("ibm_cffps_input_history", 0444, debugfs, psu,
> +			    &ibm_cffps_fops);
> +
> +	return 0;
>  }
>  
>  static const struct i2c_device_id ibm_cffps_id[] = {

Please send this patch upstream as well. We'll cherry-pick it back
after it's applied.

Cheers,

Andrew