Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754917AbbDXMzg (ORCPT ); Fri, 24 Apr 2015 08:55:36 -0400 Received: from metis.ext.pengutronix.de ([92.198.50.35]:41538 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750858AbbDXMzc (ORCPT ); Fri, 24 Apr 2015 08:55:32 -0400 Date: Fri, 24 Apr 2015 14:55:21 +0200 From: Sascha Hauer To: Pi-Cheng Chen Cc: Viresh Kumar , Mike Turquette , Matthias Brugger , "Joe.C" , Eddie Huang , Howard Chen , Chen Fan , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Linux Kernel Mailing List , "linux-pm@vger.kernel.org" , Linaro Kernel Mailman List , linux-mediatek@lists.infradead.org Subject: Re: [PATCH 1/2] cpufreq: mediatek: Add MT8173 cpufreq driver Message-ID: <20150424125521.GU6325@pengutronix.de> References: <1429522047-16675-1-git-send-email-pi-cheng.chen@linaro.org> <1429522047-16675-2-git-send-email-pi-cheng.chen@linaro.org> <20150423120152.GM6325@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 14:34:43 up 39 days, 26 min, 82 users, load average: 0.06, 0.09, 0.13 User-Agent: Mutt/1.5.21 (2010-09-15) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: sha@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3679 Lines: 88 On Fri, Apr 24, 2015 at 02:46:25PM +0800, Pi-Cheng Chen wrote: > Hi Sascha, > > Thanks for reviewing. > > On Thu, Apr 23, 2015 at 8:01 PM, Sascha Hauer wrote: > > On Mon, Apr 20, 2015 at 05:27:26PM +0800, pi-cheng.chen wrote: > >> This patch implements MT8173 specific cpufreq driver with OPP table defined > >> in the driver code. > >> > >> Signed-off-by: pi-cheng.chen > >> --- > >> drivers/cpufreq/Kconfig.arm | 6 + > >> drivers/cpufreq/Makefile | 1 + > >> drivers/cpufreq/mt8173-cpufreq.c | 509 +++++++++++++++++++++++++++++++++++++++ > >> 3 files changed, 516 insertions(+) > >> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c > >> > >> +static int mtk_cpufreq_voltage_trace(struct cpu_dvfs_info *info, > >> + struct mtk_cpu_opp *opp) > >> +{ > >> + struct regulator *proc_reg = info->proc_reg; > >> + struct regulator *sram_reg = info->sram_reg; > >> + int old_vproc, new_vproc, old_vsram, new_vsram, vsram, vproc, ret; > >> + > >> + old_vproc = regulator_get_voltage(proc_reg); > >> + old_vsram = regulator_get_voltage(sram_reg); > >> + > >> + new_vproc = opp->vproc; > >> + new_vsram = opp->vsram; > >> + > >> + /* > >> + * In the case the voltage is going to be scaled up, Vsram and Vproc > >> + * need to be scaled up step by step. In each step, Vsram needs to be > >> + * set to (Vproc + 200mV) first, then Vproc is set to (Vsram - 100mV). > >> + * Repeat the step until Vsram and Vproc are set to target voltage. > >> + */ > >> + if (old_vproc < new_vproc) { > >> +next_up_step: > >> + old_vsram = regulator_get_voltage(sram_reg); > >> + > >> + vsram = (new_vsram - old_vproc < MAX_VOLT_SHIFT) ? > >> + new_vsram : old_vproc + MAX_VOLT_SHIFT; > >> + vsram = get_regulator_voltage_floor(sram_reg, vsram); > >> + > >> + ret = regulator_set_voltage(sram_reg, vsram, vsram); > >> + if (ret) > >> + return ret; > > > > This introspecting the regulators for possible voltages looks hacky and > > unnecessary. regulator_set_voltage() can be passed minimum and maximum > > values, why don't you use it to increase the voltage within sensible > > limit, like > > > > regulator_set_voltage(sram_reg, old_vsram + 100000, old_vsram + 200000); > > > > or similar? > > I am sorry I don't understand how could I do it. Would you elaborate? You try to set the OPPs to the exact voltages, then next use functions to determine the next exact higher voltage and set the regulator voltage to an exact value. Instead of doing this you can let the ability to specify a voltage range work for you, something like: int tolerance = 50000; while (vproc < new_vproc) { int next = min(new_vproc - vproc, 200000); int next_sram = next + 100000; regulator_set_voltage(sram_reg, next_sram - tolerance, next_sram + tolerance); regulator_set_voltage(vproc_reg, next - tolerance, next + tolerance); vproc = regulator_get_voltage(vproc_reg); } Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/