From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wolverine01.qualcomm.com ([199.106.114.254]:33625 "EHLO wolverine01.qualcomm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753922Ab0LGWEP (ORCPT ); Tue, 7 Dec 2010 17:04:15 -0500 Message-ID: <2f8efbbd2ab7f88aa40a9c22a1fec0dc.squirrel@www.codeaurora.org> In-Reply-To: <20101207115701.GC4698@rakim.wolfsonmicro.main> References: <1291668763-15734-1-git-send-email-bleong@codeaurora.org> <20101206222907.GA2425@opensource.wolfsonmicro.com> <1291690320.4150.1.camel@m0nster> <20101207115701.GC4698@rakim.wolfsonmicro.main> Date: Tue, 7 Dec 2010 14:04:02 -0800 (PST) Subject: Re: [PATCH] regulator: debugfs: Adding debugfs functions into regulator framework From: "Brandon Leong" MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT Sender: linux-arm-msm-owner@vger.kernel.org List-ID: To: Mark Brown Cc: Daniel Walker , Brandon Leong , lrg@slimlogic.co.uk, davidb@codeaurora.org, linux-arm-msm@vger.kernel.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.