On Mon, Dec 06, 2010 at 12:52:43PM -0800, Brandon Leong wrote:
> Allow the user to read and edit regulator information in user space
> through the debugfs file system.
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.
Some more specific comments on the code:
> +#ifdef CONFIG_DEBUG_FS
> +
> +#define MAX_DEBUG_BUF_LEN 50
Seems low...
> +static DEFINE_MUTEX(debug_buf_mutex);
> +static char debug_buf[MAX_DEBUG_BUF_LEN];
May as well just allocate buffers dynamically? Saves locking and means
less worry about allocating a larger buffer.
> +static int reg_debug_enable_set(void *data, u64 val)
> +{
> + int err_info;
> + if (IS_ERR(data) || data == NULL) {
> + pr_err("Function Input Error %ld\n", PTR_ERR(data));
Please Try To Make Your Log Messages A Bit More Descriptive And
Typographically Correct - I'd not expect a user to have a hope of
figuring out what's gone wrong here. That said, I suspect you're
looking for BUG_ON() here...
> + if (val)
> + err_info = regulator_enable(data);
> + else
> + err_info = regulator_disable(data);
This isn't going to do what people expect - the refcounting really is
going to surprise people, especially as you read back the physical
enable/disable state through the same file. Abuse of this file is
likely to confuse any actual consumers we have too.
> +static ssize_t reg_debug_volt_set(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
Don't think we ever use volt as an abberviation anywhere else in the
API?
> + mutex_unlock(&debug_buf_mutex);
> + /* check that user entered two numbers */
Blank line between these two.
> + pr_err("Error-Input voltage pair"
> + "string exceeds maximum buffer length");
> +
Don't split text like this, it just makes the source hard to read and
grep (and note that you're missing a space in there the absence of which
is hidden by the line break).
> + voltage = regulator_get_voltage(file->private_data);
> + mutex_lock(&debug_buf_mutex);
Blank line here too.
> + if (err_info < 0) {
> + pr_err("regulator_get_mode returned an error!\n");
> + return -ENOMEM;
What was the error, and why not pass it back as the return value as well
as logging it?
> + if (IS_ERR(err_ptr)) {
> + pr_err("Error-Could not create voltage file\n");
Throughout the patch you're using this sort of non-idomatic format for
log messages and not logging error codes - I'd expect to see something
like:
pr_err("Failed to create voltage file: %d\n", PTR_ERR(err_ptr));
though since this is per-regulator you should be able to say dev_err()
instead.
> + mode = 0;
> + /* Mode File */
> + if (reg_ops->get_mode)
Odd formatting.
> +static inline void regulator_debug_create_directory(struct regulator_dev
> + *regulator_dev)
> +{
> + return;
> +}
No need for return statement here.
> @@ -2400,6 +2732,7 @@ struct regulator_dev *regulator_register(struct regulator_desc *regulator_desc,
> list_add(&rdev->list, ®ulator_list);
> out:
> mutex_unlock(®ulator_list_mutex);
> + regulator_debug_create_directory(rdev);
> return rdev;
>
> unset_supplies:
We never clean up if the regulator is unregistered.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
