Rob, thanks for your comments!
El Tue, Feb 21, 2017 at 06:22:14PM -0600 Rob Herring ha dit:
> On Fri, Feb 10, 2017 at 12:43:48PM -0800, Matthias Kaehlcke wrote:
> > The output voltage of a voltage controlled regulator can be controlled
> > through the voltage of another regulator. The current version of this
> > driver assumes that the output voltage is a linear function of the control
> > voltage.
> >
> > ...
> >
> > .../devicetree/bindings/regulator/vctrl.txt | 56 +++
>
> Please split bindings to separate patch.
The driver is not functional without the bindings, is a separate patch
preferred nevertheless? I saw other recentish regulator drivers
(ltc3676, pv88080) added the driver and the bindings together.
> > +Optional properties:
> > +--------------------
> > ...
> > +- min-slew-down-rate : Describes how slowly the regulator voltage will decay
> > + down in the worst case (lightest expected load).
> > + Specified in uV / us (like main regulator ramp rate).
> > + This value is required when ovp-threshold-percent is
> > + specified.
>
> Don't we have a standard prop for this or that's just for ramp?
regulator-ramp-delay is related, but not exactly the same. The
ramp-delay is applied at the end of an up- or downward transition,
while this prop only specifies the downward rate and is applied in
between partial transitions towards the final voltage.
We possibly could use ramp-delay and add a set_voltage_time() op to
vctrl to prevent the core code from adding the "normal" ramp-delay at
the end of the transition. However it could be confusing that vctrl
handles the ramp-delay differently than other drivers, especially we
don't want a delay in the upward transition for vctrl. But maybe
nobody would care about the different behavior, as long as the
regulator does its job ...
> Perhaps this should be common?
I agree this could be a property other regulators might have. I think
it could be common as long as it is a descriptive property which
regulator drivers can use or not, without adding functionality to the
core code (except for DT parsing).
Mark, what do you think?
> > +
> > +Example:
> > +
> > + vctrl_reg {
>
> Don't use '_' in node names.
Will fix in next revision.
> > + compatible = "vctrl-regulator";
> > + regulator-name = "vctrl_reg";
> > +
> > + ctrl-supply = <&ctrl_supply>;
> > +
>
> > + regulator-min-microvolt = <800000>;
> > + regulator-max-microvolt = <1500000>;
> > +
> > + output-voltage-range = <800000 1500000>;
>
> Why do you need both?
We don't necessarily need both. The constraints are not available yet
when the driver parses and validates the DT node, but we can read
regulator-min/max-microvolt manually in the driver instead of
duplicating the values.
> > + ctrl-voltage-range = <200000 500000>;
> > +
> > + slew-rate = <225>;
>
> Not documented.
Should be min-slew-down-rate. Will fix in next revision.
Thanks
Matthias
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
