Re: [PATCH v2 8/8] ARM: dts: mt8135: Add support for MT6397 regulator

[Mark Brown]

On Fri, Nov 28, 2014 at 11:54:34AM +0800, Flora Fu wrote:

> Add device tree for MT6397 regulators in mt8135.dtsi.
> 
> Signed-off-by: Flora Fu <flora.fu@xxxxxxxxxxxx>
> ---
>  arch/arm/boot/dts/mt8135.dtsi | 192 ++++++++++++++++++++++++++++++++++++++++++

This appears to be the DT fragment for a SoC but you are defining the
system integration for the PMIC.  That's bad, the PMIC is a separate
device so should be hooked up by the board using it.  If there's common
elements from a reference design they should be in their own .dtsi.

> +					mt6397_vsramca15_reg: buck_vsramca15 {
> +						regulator-name = "vsramca15";
> +						regulator-min-microvolt = < 700000>;
> +						regulator-max-microvolt = <1493750>;
> +						regulator-ramp-delay = <12500>;
> +						regulator-enable-ramp-delay = <115>;
> +						regulator-always-on;
> +						regulator-boot-on;

Why do these regulators have both a board specific ramp delay specified
and -always-on?  If they are always on presumably they never ramp at
runtime; if this is a generic parameter for the device it should be in
the driver.

Similarly why is boot_on being specified - we can read the startup state
for all these regulators?

> +					};
> +
> +					mt6397_vsramca7_reg: buck_vsramca7 {
> +						regulator-name = "vsramca7";
> +						regulator-min-microvolt = < 700000>;
> +						regulator-max-microvolt = <1493750>;

All these regulators seem to have exactly the same range specified which
looks awfully like it might be the maximum variability the regulator has
rather than the board specific range that's supported; the fact that
they appear to have no consumers that might vary the voltage is another
warning sign.  This is probably wrong, the constraints should be
whatever is verified good for the board.

Attachment: signature.asc
Description: Digital signature