From mboxrd@z Thu Jan 1 00:00:00 1970 From: Torsten Duwe Date: Mon, 18 Feb 2019 16:26:57 +0100 Subject: [U-Boot] [PATCH v3 6/9] regulator: Add support for ramp delay In-Reply-To: References: <20190216094548.911-1-krzk@kernel.org> <20190216094548.911-7-krzk@kernel.org> <20190218140317.GB6623@lst.de> Message-ID: <20190218152657.GC6623@lst.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Mon, Feb 18, 2019 at 03:28:46PM +0100, Krzysztof Kozlowski wrote: > On Mon, 18 Feb 2019 at 15:03, Torsten Duwe wrote: > > > > > --- a/doc/device-tree-bindings/regulator/regulator.txt > > > +++ b/doc/device-tree-bindings/regulator/regulator.txt > > > @@ -35,6 +35,7 @@ Optional properties: > > > - regulator-max-microamp: a maximum allowed Current value > > > - regulator-always-on: regulator should never be disabled > > > - regulator-boot-on: enabled by bootloader/firmware > > > +- regulator-ramp-delay: ramp delay for regulator (in uV/us) > > > > I guess you mean s/V, not V/s; at least the code suggests so. > > uV/uS. It is correct in the code: > int delay = DIV_ROUND_UP(abs(new_uV - old_uV), ramp_delay); > delay = (uV - uV) / (uV / uS) = uS You're right. _divide_ by that value; somhow this has escaped me. Sorry for the noise. nit: "s" is for seconds, "S" is for conductance. > > But my main point is: is the required delay always a linear > > function of the voltage jump? Depending on the dampening and > > load on the rail this could be an overshoot and settle, no? > > > > So I suggest to make that an array with 2 elements: a fixed part > > and a time per voltage change. Does that make sense? > > Just to make it clear - then we do not follow Linux kernel DT bindings. > The voltage change might have exponential characteristic and/or have > additional fixed delay time (see patch 7 here). We could re-use some > properties from Linux bindings for that purpose: > https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/regulator/regulator.txt#L19 > https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/regulator/regulator.txt#L24 I see. But then "static void regulator_set_value_delay(...)" should either at least have a "ramp" somewhere in its name or it should discover the device properties on its own, in order to be able to handle regulator-settling-time* and regulator-enable-ramp-delay as well in the future. (i.e. pass it uc_pdata instead of uc_pdata->ramp_delay and also let it handle the condition). Torsten