[PATCHv2] regulator: debugfs: Adding debugfs functions into regulator framework

[PATCHv2] regulator: debugfs: Adding debugfs functions into regulator framework

[Brandon Leong]

Allow the user to read and edit regulator information in user space
through the debugfs file system.

Signed-off-by: Brandon Leong <bleong@codeaurora.org>
---
Dynamic allocation of buffer added for debugfs.
Cleaned up error messages and stylistic errors.
Clean-up function created.

 drivers/regulator/core.c         |  403 ++++++++++++++++++++++++++++++++++++++
 include/linux/regulator/driver.h |    5 +
 2 files changed, 408 insertions(+), 0 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index ba521f0..50137c3 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -13,6 +13,8 @@
  *
  */
 
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
 #include <linux/kernel.h>
 #include <linux/init.h>
 #include <linux/device.h>
@@ -21,6 +23,8 @@
 #include <linux/mutex.h>
 #include <linux/suspend.h>
 #include <linux/delay.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
 #include <linux/regulator/consumer.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
@@ -48,6 +52,20 @@ struct regulator_map {
 };
 
 /*
+ * struct regulator_debug
+ *
+ * Created for each regulator. Used specifically for debugfs purposes.
+ */
+struct regulator_debug {
+	struct regulator *reg;
+	struct regulator_dev *rdev;
+	struct dentry *reg_subdir;
+	int enabled;
+	char *debug_buffer;
+	int debug_buffer_len;
+};
+
+/*
  * struct regulator
  *
  * One for each consumer device.
@@ -2271,6 +2289,390 @@ static int add_regulator_attributes(struct regulator_dev *rdev)
 	return status;
 }
 
+#ifdef CONFIG_DEBUG_FS
+
+#define INIT_DEBUG_BUF_SIZE 20
+
+/* debug_buf_resize: dynamically resizes debug_buffer as needed */
+static char *debug_buf_resize(struct regulator_debug *reg_debug, int bufsize)
+{
+	if (bufsize <= reg_debug->debug_buffer_len)
+		return reg_debug->debug_buffer;
+
+	kfree(reg_debug->debug_buffer);
+
+	reg_debug->debug_buffer = kmalloc(bufsize, GFP_KERNEL);
+	reg_debug->debug_buffer_len = bufsize;
+
+	return reg_debug->debug_buffer;
+}
+
+static int reg_debug_enable_set(void *data, u64 val)
+{
+	int err_info = 0;
+	struct regulator_debug *reg_debug = data;
+
+	BUG_ON(data == NULL);
+
+	if (val && !(reg_debug->enabled))
+		err_info = regulator_enable(reg_debug->reg);
+	else if (!val && (reg_debug->enabled))
+		err_info = regulator_disable(reg_debug->reg);
+	else if (val && (reg_debug->enabled))
+		pr_err("regulator_enable has already been called for this regulator\n");
+	else
+		pr_err("regulator_disable has already been called for this regulator\n");
+
+	if (err_info < 0)
+		pr_err("regulator_enable/disable returned error: %d\n",
+			err_info);
+	else
+		reg_debug->enabled = val;
+
+	return 0;
+}
+
+static int reg_debug_enable_get(void *data, u64 *val)
+{
+	struct regulator_debug *reg_debug = data;
+
+	BUG_ON(data == NULL);
+
+	*val = regulator_is_enabled(reg_debug->reg);
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(reg_enable_fops, reg_debug_enable_get,
+			reg_debug_enable_set, "%llu\n");
+
+static int reg_debug_fdisable_set(void *data, u64 val)
+{
+	int err_info;
+	struct regulator_debug *reg_debug = data;
+
+	BUG_ON(data == NULL);
+
+	if (val > 0) {
+		err_info = regulator_force_disable(reg_debug->reg);
+		reg_debug->enabled = 0;
+	} else
+		err_info = 0;
+
+	if (err_info < 0)
+		pr_err("regulator_force_disable returned error: %d\n",
+			err_info);
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(reg_fdisable_fops, reg_debug_enable_get,
+			reg_debug_fdisable_set, "%llu\n");
+
+static ssize_t reg_debug_voltage_set(struct file *file, const char __user *buf,
+					size_t count, loff_t *ppos)
+{
+	int err_info, filled;
+	char *voltage_buffer;
+	struct regulator_debug *reg_debug = file->private_data;
+	int min, max = -1;
+
+	BUG_ON(file == NULL);
+
+	voltage_buffer = debug_buf_resize(reg_debug, count);
+
+	if (voltage_buffer == NULL) {
+		pr_err("debug_buf_resize returned NULL\n");
+		return -ENOMEM;
+	}
+
+	if (copy_from_user(voltage_buffer, (void __user *) buf, count))
+		return -EFAULT;
+
+	filled = sscanf(voltage_buffer, "%d %d", &min, &max);
+
+	/* check that user entered two integers */
+	if (filled < 2 || min < 0 || max < min) {
+		pr_info("Error, correct format: 'echo \"min max\" > voltage\n");
+		return -ENOMEM;
+	} else
+		err_info = regulator_set_voltage(reg_debug->reg, min, max);
+
+	if (err_info < 0)
+		pr_err("regulator_set_voltage returned error: %d\n", err_info);
+
+	return count;
+}
+
+static ssize_t reg_debug_voltage_get(struct file *file, char __user *buf,
+					size_t count, loff_t *ppos)
+{
+	int voltage, output;
+	char *voltage_buffer;
+	int rc = -1;
+	struct regulator_debug *reg_debug = file->private_data;
+
+	BUG_ON(file == NULL);
+
+	voltage = regulator_get_voltage(reg_debug->reg);
+
+	if (voltage < 0) {
+		pr_err("regulator_get_voltage returned error: %d", voltage);
+		return -ENOMEM;
+	} else {
+		voltage_buffer = debug_buf_resize(reg_debug,
+							INIT_DEBUG_BUF_SIZE);
+
+		output = snprintf(voltage_buffer, reg_debug->debug_buffer_len,
+					"%d\n", voltage);
+		rc = simple_read_from_buffer((void __user *) buf, output, ppos,
+				(void *) voltage_buffer, output);
+
+		return rc;
+	}
+}
+
+static int reg_debug_voltage_open(struct inode *inode, struct file *file)
+{
+	BUG_ON(file == NULL);
+
+	file->private_data = inode->i_private;
+	return 0;
+}
+
+static const struct file_operations reg_voltage_fops = {
+	.write	= reg_debug_voltage_set,
+	.open   = reg_debug_voltage_open,
+	.read	= reg_debug_voltage_get,
+};
+
+static int reg_debug_mode_set(void *data, u64 val)
+{
+	int err_info;
+	struct regulator_debug *reg_debug = data;
+
+	BUG_ON(data == NULL);
+
+	err_info = regulator_set_mode(reg_debug->reg, (unsigned int)val);
+
+	if (err_info < 0)
+		pr_err("regulator_set_mode returned error: %d\n", err_info);
+
+	return 0;
+}
+
+static int reg_debug_mode_get(void *data, u64 *val)
+{
+	int err_info;
+	struct regulator_debug *reg_debug = data;
+
+	BUG_ON(data == NULL);
+
+	err_info = regulator_get_mode(reg_debug->reg);
+
+	if (err_info < 0) {
+		pr_err("Regulator_get_mode returned error: %d\n", err_info);
+	} else {
+		*val = err_info;
+	}
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(reg_mode_fops, reg_debug_mode_get,
+			reg_debug_mode_set, "%llu\n");
+
+static int reg_debug_optimum_mode_set(void *data, u64 val)
+{
+	int err_info;
+	struct regulator_debug *reg_debug = data;
+
+	BUG_ON(data == NULL);
+
+	err_info = regulator_set_optimum_mode(reg_debug->reg,
+						(unsigned int)val);
+
+	if (err_info < 0) {
+		pr_err("Regulator_set_optimum_mode returned error: %d\n",
+			err_info);
+	}
+
+	return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(reg_optimum_mode_fops, reg_debug_mode_get,
+			reg_debug_optimum_mode_set, "%llu\n");
+
+static struct dentry *debugfs_base;
+
+static int reg_debug_init(void)
+{
+	debugfs_base = debugfs_create_dir("regulator", NULL);
+	if (IS_ERR(debugfs_base) || debugfs_base == NULL) {
+		pr_err("Debugfs_create_dir PTR_ERR: %ld\n",
+			PTR_ERR(debugfs_base));
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static void regulator_debugfs_cleanup(struct regulator_dev *rdev)
+{
+	BUG_ON(rdev == NULL);
+
+	regulator_put(rdev->debug_data->reg);
+	debugfs_remove_recursive(rdev->debug_data->reg_subdir);
+	kfree(rdev->debug_data->debug_buffer);
+	kfree(rdev->debug_data);
+}
+
+/**
+ * regulator_debug_create_directory - Creates debugfs directories and files
+ *					for each regulator
+ *
+ * @param regulator_dev Voltage/Current regulator class device.
+ *
+ * Description: This function is run everytime "regulator_register" is run.
+ *		It is called for every regulator and initilaizes both the
+ *		regulator directory and the data files inside (ex. enable,
+ *		get/set voltage etc).
+ */
+static int regulator_debug_create_directory(struct regulator_dev *regulator_dev)
+{
+	struct dentry *reg_subdir;
+	struct dentry *err_ptr;
+	struct regulator *reg;
+	struct regulator_ops *reg_ops;
+	struct regulator_debug *debugfs_info;
+	mode_t mode;
+
+	if (IS_ERR(regulator_dev) || regulator_dev == NULL) {
+		pr_err("regulator_dev PTR_ERR: %ld\n", PTR_ERR(regulator_dev));
+		goto error;
+	}
+
+	if (IS_ERR(debugfs_base) || debugfs_base == NULL) {
+		pr_err("debugfs_base PTR_ERR: %ld\n", PTR_ERR(debugfs_base));
+		goto error;
+	}
+
+	reg_subdir = debugfs_create_dir(regulator_dev->desc->name,
+					debugfs_base);
+
+	reg = regulator_get(NULL, regulator_dev->desc->name);
+	if (IS_ERR(reg) || reg == NULL) {
+		pr_err("regulator_get PTR_ERR: %ld\n", PTR_ERR(reg));
+		goto error;
+	}
+
+	debugfs_info = kzalloc(sizeof(struct regulator_debug), GFP_KERNEL);
+
+	if (debugfs_info == NULL) {
+		pr_err("debugfs_info is NULL after kzalloc\n");
+		goto error;
+	}
+
+	/* Populate debugfs_info */
+	debugfs_info->reg = reg;
+	debugfs_info->reg_subdir = reg_subdir;
+	debugfs_info->debug_buffer = NULL;
+
+	regulator_dev->debug_data = debugfs_info;
+
+	reg_ops = regulator_dev->desc->ops;
+
+	/* Enabled File */
+	mode = 0;
+	if (reg_ops->is_enabled)
+		mode |= S_IRUGO;
+	if (reg_ops->enable || reg_ops->disable)
+		mode |= S_IWUSR;
+	if (mode)
+		err_ptr = debugfs_create_file("enable", mode, reg_subdir,
+						debugfs_info, &reg_enable_fops);
+	if (IS_ERR(err_ptr)) {
+		pr_err("Failed to create enable file: %ld\n", PTR_ERR(err_ptr));
+		debugfs_remove_recursive(reg_subdir);
+		goto error;
+	}
+
+	/* Force-Disable File */
+	mode = 0;
+	if (reg_ops->is_enabled)
+		mode |= S_IRUGO;
+	if (reg_ops->enable || reg_ops->disable)
+		mode |= S_IWUSR;
+	if (mode)
+		err_ptr = debugfs_create_file("force_disable", mode, reg_subdir,
+					debugfs_info, &reg_fdisable_fops);
+	if (IS_ERR(err_ptr)) {
+		pr_err("Failed to create force_disable file: %ld\n",
+			PTR_ERR(err_ptr));
+		debugfs_remove_recursive(reg_subdir);
+		goto error;
+	}
+
+	/* Voltage File */
+	mode = 0;
+	if (reg_ops->get_voltage)
+		mode |= S_IRUGO;
+	if (reg_ops->set_voltage)
+		mode |= S_IWUSR;
+	if (mode)
+		err_ptr = debugfs_create_file("voltage", mode, reg_subdir,
+					debugfs_info, &reg_voltage_fops);
+	if (IS_ERR(err_ptr)) {
+		pr_err("Failed to create voltage file: %ld\n",
+			PTR_ERR(err_ptr));
+		debugfs_remove_recursive(reg_subdir);
+		goto error;
+	}
+
+	/* Mode File */
+	mode = 0;
+	if (reg_ops->get_mode)
+		mode |= S_IRUGO;
+	if (reg_ops->set_mode)
+		mode |= S_IWUSR;
+	if (mode)
+		err_ptr = debugfs_create_file("mode", mode, reg_subdir,
+						debugfs_info, &reg_mode_fops);
+	if (IS_ERR(err_ptr)) {
+		pr_err("Failed to create mode file: %ld\n", PTR_ERR(err_ptr));
+		debugfs_remove_recursive(reg_subdir);
+		goto error;
+	}
+
+	/* Optimum Mode File */
+	mode = 0;
+	if (reg_ops->get_mode)
+		mode |= S_IRUGO;
+	if (reg_ops->set_mode)
+		mode |= S_IWUSR;
+	if (mode)
+		err_ptr = debugfs_create_file("optimum_mode", mode, reg_subdir,
+					debugfs_info, &reg_optimum_mode_fops);
+	if (IS_ERR(err_ptr)) {
+		pr_err("Failed to create optimum_mode file: %ld\n",
+			PTR_ERR(err_ptr));
+		debugfs_remove_recursive(reg_subdir);
+		goto error;
+	}
+
+	return 0;
+
+error:
+	return -ENOMEM;
+}
+#else
+static inline void regulator_debug_create_directory(struct regulator_dev
+						*regulator_dev) { }
+
+static inline void reg_debug_init(void) { }
+
+static inline void regulator_debugfs_cleanup(struct regulator_dev *rdev) { }
+#endif
+
 /**
  * regulator_register - register regulator
  * @regulator_desc: regulator to register
@@ -2432,6 +2834,7 @@ void regulator_unregister(struct regulator_dev *rdev)
 
 	mutex_lock(&regulator_list_mutex);
 	WARN_ON(rdev->open_count);
+	regulator_debugfs_cleanup(rdev);
 	unset_regulator_supplies(rdev);
 	list_del(&rdev->list);
 	if (rdev->supply)
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index 592cd7c..ce63090 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -20,6 +20,7 @@
 
 struct regulator_dev;
 struct regulator_init_data;
+struct regulator_debug;
 
 enum regulator_status {
 	REGULATOR_STATUS_OFF,
@@ -188,6 +189,10 @@ struct regulator_dev {
 	struct regulator_dev *supply;	/* for tree */
 
 	void *reg_data;		/* regulator_dev data */
+
+#ifdef CONFIG_DEBUG_FS
+	struct regulator_debug *debug_data;
+#endif
 };
 
 struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
-- 
1.7.3.1

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

Re: [PATCHv2] regulator: debugfs: Adding debugfs functions into regulator framework

[Mark Brown]

On Wed, Dec 15, 2010 at 05:49:05PM -0800, Brandon Leong wrote:

I just noticed that you only posted this to the MSM kernel list -
please always post generic patches to the relevant generic list, lkml in
this case.  I've CCed lkml.

> Allow the user to read and edit regulator information in user space
> through the debugfs file system.

> Signed-off-by: Brandon Leong <bleong@codeaurora.org>
> ---
> Dynamic allocation of buffer added for debugfs.
> Cleaned up error messages and stylistic errors.
> Clean-up function created.

Quoting my reply to your previous version:

| Basically what you're doing here is providing a default consumer via
| debugfs along with the consumers that have been defined by the board.
| On the one hand it's good that this means that we won't let the user
| change anything unless it has been explicitly enabled, on the other hand
| it means that we've not really added anything that you can't do already
| with the virtual consumer except made it slightly more convenient to set
| up and give it a different interface.

| This makes me a bit nervous, especially given that the regulator API is
| mostly used in embedded systems where things like debugfs not being a
| production API don't entirely count for much.  It feels like it
| encourages people to write random userspace code which prods and pokes
| at the regulators directly without any form of mediation and without
| suggesting that perhaps they ought to be doing something a bit nicer.

| I can see some useful debug facilities we could add using debugfs.  For
| example, it'd be good to expose more of the dynamic structures the API
| has like the consumers, their reference counts and the voltages they
| have requested.  Showing the voltage lists could also be nice.  This is
| all peering inside the API to provide additional insight to the user,
| though, rather than something layered on top of the API as this is.

| If we do want to go this way it'd be good to have more discussion in the
| changelog about what we're adding here, what it gives us, how it should
| be used and how it compares with the existing approaches.  Given the
| similarities with the virtual consumer I am inclined to wonder why we'd
| want to reimplement the actual user visible API bit.

Please engage with the above - it feels rather like you've ignoring it.
I'm not completely against a debugfs interface but we do need to
consider how this fits in with both the existing debug facilities and
the need to avoid having random userspace code just going in and
fiddling with stuff directly.

Some more specific comments on the code, but please don't post any
updated patches without discussing the above:

>   *
>   */
>  
> +#define pr_fmt(fmt) "%s: " fmt, __func__
> +

Hrm?  If this is needed we shouldn't be adding it in the regulator API
with a name like that, and we probably ought to be doing something with
the rdev_ variants.

> +#ifdef CONFIG_DEBUG_FS
> +
> +#define INIT_DEBUG_BUF_SIZE 20
> +
> +/* debug_buf_resize: dynamically resizes debug_buffer as needed */
> +static char *debug_buf_resize(struct regulator_debug *reg_debug, int bufsize)
> +{
> +	if (bufsize <= reg_debug->debug_buffer_len)
> +		return reg_debug->debug_buffer;
> +
> +	kfree(reg_debug->debug_buffer);
> +
> +	reg_debug->debug_buffer = kmalloc(bufsize, GFP_KERNEL);
> +	reg_debug->debug_buffer_len = bufsize;
> +
> +	return reg_debug->debug_buffer;
> +}

This seems more trouble than it's worth - why not just allocate and free
the buffer as needed?  If we're doing enough of this for it to be worth
avoding the allocations it seems we're doing something wrong.

> +static int reg_debug_enable_set(void *data, u64 val)
> +{
> +	int err_info = 0;
> +	struct regulator_debug *reg_debug = data;
> +
> +	BUG_ON(data == NULL);
> +
> +	if (val && !(reg_debug->enabled))
> +		err_info = regulator_enable(reg_debug->reg);
> +	else if (!val && (reg_debug->enabled))
> +		err_info = regulator_disable(reg_debug->reg);
> +	else if (val && (reg_debug->enabled))
> +		pr_err("regulator_enable has already been called for this regulator\n");
> +	else
> +		pr_err("regulator_disable has already been called for this regulator\n");

This feels like an awkward halfway house between a debug facility and a
proper consumer - it interacts with the refcounting real consumers do
but tries to work directly with the actual hardware state.  You end up
not being able to do multiple enables if you want to but with disable
only decremeting the refcount so possibly needing to be iterated
multiple times.

Either this should be a real consumer and make sure it balances its own
enables and disables or it should be a diagnostic facility for the
reference counts and the hardware state should be reported separately.
This all ties in with the overall comments at the top...

> +	/* check that user entered two integers */
> +	if (filled < 2 || min < 0 || max < min) {
> +		pr_info("Error, correct format: 'echo \"min max\" > voltage\n");
> +		return -ENOMEM;
> +	} else
> +		err_info = regulator_set_voltage(reg_debug->reg, min, max);

It'd be more friendly to accept a single voltage and DTRT with it as
well.

> +static int reg_debug_mode_set(void *data, u64 val)
> +{
> +	int err_info;
> +	struct regulator_debug *reg_debug = data;
> +
> +	BUG_ON(data == NULL);
> +
> +	err_info = regulator_set_mode(reg_debug->reg, (unsigned int)val);
> +
> +	if (err_info < 0)
> +		pr_err("regulator_set_mode returned error: %d\n", err_info);

Taking strings would be much nicer here, and...

> +static int reg_debug_mode_get(void *data, u64 *val)
> +{
> +	int err_info;
> +	struct regulator_debug *reg_debug = data;
> +
> +	BUG_ON(data == NULL);
> +
> +	err_info = regulator_get_mode(reg_debug->reg);
> +
> +	if (err_info < 0) {
> +		pr_err("Regulator_get_mode returned error: %d\n", err_info);
> +	} else {
> +		*val = err_info;

...displaying them here also.  Even more friendly would be showing the
set of available modes.

> +/**
> + * regulator_debug_create_directory - Creates debugfs directories and files
> + *					for each regulator

This doesn't need kerneldoc, it's not an exported API so shouldn't end
up in the generated documentation (so no /** only /* at the start of the
comment).

> +       if (reg_ops->get_voltage)
> +               mode |= S_IRUGO;
> +       if (reg_ops->set_voltage)
> +               mode |= S_IWUSR;

This needs refreshing against -next at a minimum.