From patchwork Wed Sep 19 16:58:25 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pawel Moll X-Patchwork-Id: 1478631 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) by patchwork2.kernel.org (Postfix) with ESMTP id 0E606DF280 for ; Wed, 19 Sep 2012 17:00:21 +0000 (UTC) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TENbd-0006YC-BW; Wed, 19 Sep 2012 16:58:33 +0000 Received: from service87.mimecast.com ([91.220.42.44]) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TENba-0006Vv-1I for linux-arm-kernel@lists.infradead.org; Wed, 19 Sep 2012 16:58:30 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.21]) by service87.mimecast.com; Wed, 19 Sep 2012 17:58:25 +0100 Received: from [10.1.205.42] ([10.1.255.212]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.0); Wed, 19 Sep 2012 17:58:25 +0100 Message-ID: <1348073905.11116.80.camel@hornet> Subject: Re: [PATCH v2 04/13] regulators: Versatile Express regulator driver From: Pawel Moll To: Mark Brown Date: Wed, 19 Sep 2012 17:58:25 +0100 In-Reply-To: <20120919022141.GA8832@opensource.wolfsonmicro.com> References: <1347977875-16855-1-git-send-email-pawel.moll@arm.com> <1347977875-16855-5-git-send-email-pawel.moll@arm.com> <20120918150212.GA12543@opensource.wolfsonmicro.com> <1347983056.11116.11.camel@hornet> <20120918160909.GA15587@opensource.wolfsonmicro.com> <1347987819.11116.38.camel@hornet> <20120919022141.GA8832@opensource.wolfsonmicro.com> X-Mailer: Evolution 3.2.3-0ubuntu6 Mime-Version: 1.0 X-OriginalArrivalTime: 19 Sep 2012 16:58:25.0739 (UTC) FILETIME=[FC11D1B0:01CD9687] X-MC-Unique: 112091917582506201 X-Spam-Note: CRM114 invocation failed X-Spam-Score: -2.6 (--) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-2.6 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.7 RCVD_IN_DNSWL_LOW RBL: Sender listed at http://www.dnswl.org/, low trust [91.220.42.44 listed in list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Liam Girdwood , "linux-arm-kernel@lists.infradead.org" X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org On Wed, 2012-09-19 at 03:21 +0100, Mark Brown wrote: > No, I mean discovering which regulators are present and what they can > do. Nope. This driver is supposed to work only on Device Tree "enabled" platforms. Having said that, I should have added the bindings documentation in the patch. Will do. > > > So this is going to break interoperation with a bunch of consumer > > > drivers that rely on being able to tell what voltages are supported. > > > The key thing for them would be that regulator_is_supported_voltage() > > > works, currently it relies on list_voltage() as that's the only way to > > > do that right now. > > > Ok, I guess I should use regulator_list_voltage_linear() and > > regulator_map_voltage_linear() then? I'll just have to carefully think > > what step to choose. > > No, we should provide a way to describe this situation in the API - it's > not unreasonable and having to pick step sizes is obviously suboptimal. Actually before I finally got this mail (our mail system was playing stupid today), I came up with idea of using the power supply specified tolerance as a base to chose the step sizes. This comes down to such code (with the regulator_*_voltage_linear in the ops): 8<--- int range = init_data->constraints.max_uV - init_data->constraints.min_uV; u32 tolerance; int avg_error; err = of_property_read_u32(pdev->dev.of_node, "arm,vexpress-volt,tolerance", &tolerance); if (err) goto error_property_read; reg->desc.min_uV = init_data->constraints.min_uV; avg_error = (range / 2 + reg->desc.min_uV) * tolerance / 100; reg->desc.n_voltages = range / avg_error + 1; reg->desc.uV_step = range / (reg->desc.n_voltages - 1); 8<--- so it doesn't look that bad to me. Now, if you are considering changing the API, I would propose something like this: 8<--- 8<--- I originally assumed that if I provide no operating points (in the form of list_voltage function) any voltage within the constraints range will be allowed. Both options are perfectly fine with me. Thanks! Pawel diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c index 4838531..a091719 100644 --- a/drivers/regulator/core.c +++ b/drivers/regulator/core.c @@ -1948,8 +1948,14 @@ int regulator_is_supported_voltage(struct regulator *regulator, } ret = regulator_count_voltages(regulator); - if (ret < 0) - return ret; + if (ret < 0) { + /* No operating points defined, allow any value within range */ + struct regulation_constraints *constraints = + regulator->rdev->constraints; + + return min_uV >= constraints->min_uV && + max_uV <= constraints->max_uV; + } voltages = ret; for (i = 0; i < voltages; i++) {