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

["Brandon Leong" <bleong@codeaurora.org>]

So is it decided that we should use BUG_ON() now?

Also, regarding this issue:

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

----

Could you clarify the issue with this? All I am doing here is if the user
enters a "1", then enable, if the user enters a "0" then disable.

Thanks,
-Brandon

> On Mon, Dec 06, 2010 at 06:52:00PM -0800, Daniel Walker wrote:
>> On Mon, 2010-12-06 at 22:29 +0000, Mark Brown wrote:
>
>> > > +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...
>
>> Could we do WARN_ON() here? Unless this is a really serious problem.
>
> It means we've managed to let the user open a file without setting up
> the private data for the file correctly which is a pretty bad bug which
> we'd never expect to see at runtime unless there was a clear kernel bug.
>


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