diff mbox

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

Message ID 1417146874-5232-9-git-send-email-flora.fu@mediatek.com (mailing list archive)
State New, archived
Headers show

Commit Message

Flora Fu Nov. 28, 2014, 3:54 a.m. UTC 
Add device tree for MT6397 regulators in mt8135.dtsi.

Signed-off-by: Flora Fu <flora.fu@mediatek.com>
---
 arch/arm/boot/dts/mt8135.dtsi | 192 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 192 insertions(+)

Comments

Mark Brown Nov. 28, 2014, 3:30 p.m. UTC | #1 
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@mediatek.com>
> ---
>  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.
Flora Fu Dec. 1, 2014, 3:19 a.m. UTC | #2 
Hi, Mark, 

On Fri, 2014-11-28 at 15:30 +0000, Mark Brown wrote: 
> 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@mediatek.com>
> > ---
> >  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.
> 

Do you mean that we should add a mt6397.dtsi and include it from
mt8135.dtsi? For board specific, update them in mt8135.dtsi? 

Thanks,
Flora
Sascha Hauer Dec. 1, 2014, 6:40 a.m. UTC | #3 
On Mon, Dec 01, 2014 at 11:19:25AM +0800, Flora Fu wrote:
> Hi, Mark, 
> 
> On Fri, 2014-11-28 at 15:30 +0000, Mark Brown wrote: 
> > 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@mediatek.com>
> > > ---
> > >  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.
> > 
> 
> Do you mean that we should add a mt6397.dtsi and include it from
> mt8135.dtsi? For board specific, update them in mt8135.dtsi?

You can't include it from mt8135.dtsi since here you don't know if a
mt6397.dtsi is connected. You have to include both the mt8135.dtsi and
the mt6397.dtsi from the board file.

Sascha
Mark Brown Dec. 1, 2014, 11:16 a.m. UTC | #4 
On Mon, Dec 01, 2014 at 11:19:25AM +0800, Flora Fu wrote:
> On Fri, 2014-11-28 at 15:30 +0000, Mark Brown wrote: 

> > 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.

> Do you mean that we should add a mt6397.dtsi and include it from
> mt8135.dtsi? For board specific, update them in mt8135.dtsi? 

More likely you should just add any integration needed in the board
file.  There should be no need to specify anything generically for the
regulators on a PMIC except registering the device.
diff mbox

Patch

diff --git a/arch/arm/boot/dts/mt8135.dtsi b/arch/arm/boot/dts/mt8135.dtsi
index 14a8aad..436b72f 100644
--- a/arch/arm/boot/dts/mt8135.dtsi
+++ b/arch/arm/boot/dts/mt8135.dtsi
@@ -118,6 +118,198 @@ 
 
 			pmic {
 				compatible = "mediatek,mt6397";
+
+				regulators {
+					mt6397_vpca15_reg: buck_vpca15 {
+						regulator-name = "vpca15";
+						regulator-min-microvolt = < 700000>;
+						regulator-max-microvolt = <1493750>;
+						regulator-ramp-delay = <12500>;
+						regulator-enable-ramp-delay = <200>;
+						regulator-always-on;
+						regulator-boot-on;
+					};
+
+					mt6397_vpca7_reg: buck_vpca7 {
+						regulator-name = "vpca7";
+						regulator-min-microvolt = < 700000>;
+						regulator-max-microvolt = <1493750>;
+						regulator-ramp-delay = <12500>;
+						regulator-enable-ramp-delay = <115>;
+						regulator-always-on;
+						regulator-boot-on;
+					};
+
+					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;
+					};
+
+					mt6397_vsramca7_reg: buck_vsramca7 {
+						regulator-name = "vsramca7";
+						regulator-min-microvolt = < 700000>;
+						regulator-max-microvolt = <1493750>;
+						regulator-ramp-delay = <12500>;
+						regulator-enable-ramp-delay = <115>;
+						regulator-always-on;
+						regulator-boot-on;
+					};
+
+					mt6397_vcore_reg: buck_vcore {
+						regulator-name = "vcore";
+						regulator-min-microvolt = < 700000>;
+						regulator-max-microvolt = <1493750>;
+						regulator-ramp-delay = <12500>;
+						regulator-enable-ramp-delay = <115>;
+						regulator-always-on;
+						regulator-boot-on;
+					};
+
+					mt6397_vgpu_reg: buck_vgpu {
+						regulator-name = "vgpu";
+						regulator-min-microvolt = < 700000>;
+						regulator-max-microvolt = <1493750>;
+						regulator-ramp-delay = <12500>;
+						regulator-enable-ramp-delay = <115>;
+					};
+
+					mt6397_vdrm_reg: buck_vdrm {
+						regulator-name = "vdrm";
+						regulator-min-microvolt = < 800000>;
+						regulator-max-microvolt = <1593750>;
+						regulator-ramp-delay = <12500>;
+						regulator-enable-ramp-delay = <500>;
+						regulator-always-on;
+						regulator-boot-on;
+					};
+
+					mt6397_vio18_reg: buck_vio18 {
+						regulator-name = "vio18";
+						regulator-min-microvolt = <1500000>;
+						regulator-max-microvolt = <2120000>;
+						regulator-ramp-delay = <12500>;
+						regulator-enable-ramp-delay = <500>;
+						regulator-always-on;
+						regulator-boot-on;
+					};
+
+					mt6397_vtcxo_reg: ldo_vtcxo {
+						regulator-name = "vtcxo";
+						regulator-min-microvolt = <2800000>;
+						regulator-max-microvolt = <2800000>;
+						regulator-enable-ramp-delay = <90>;
+						regulator-always-on;
+						regulator-boot-on;
+					};
+
+					mt6397_va28_reg: ldo_va28 {
+						regulator-name = "va28";
+						regulator-min-microvolt = <2800000>;
+						regulator-max-microvolt = <2800000>;
+						regulator-enable-ramp-delay = <218>;
+						regulator-always-on;
+						regulator-boot-on;
+					};
+
+					mt6397_vcama_reg: ldo_vcama {
+						regulator-name = "vcama";
+						regulator-min-microvolt = <1500000>;
+						regulator-max-microvolt = <2800000>;
+						regulator-enable-ramp-delay = <218>;
+					};
+
+					mt6397_vio28_reg: ldo_vio28 {
+						regulator-name = "vio28";
+						regulator-min-microvolt = <2800000>;
+						regulator-max-microvolt = <2800000>;
+						regulator-enable-ramp-delay = <240>;
+						regulator-always-on;
+						regulator-boot-on;
+					};
+
+					mt6397_usb_reg: ldo_usb {
+						regulator-name = "usb";
+						regulator-min-microvolt = <3300000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-enable-ramp-delay = <218>;
+					};
+
+					mt6397_vmc_reg: ldo_vmc {
+						regulator-name = "vmc";
+						regulator-min-microvolt = <1800000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-enable-ramp-delay = <218>;
+					};
+
+					mt6397_vmch_reg: ldo_vmch {
+						regulator-name = "vmch";
+						regulator-min-microvolt = <3000000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-enable-ramp-delay = <218>;
+					};
+
+					mt6397_vemc_3v3_reg: ldo_vemc3v3 {
+						regulator-name = "vemc_3v3";
+						regulator-min-microvolt = <3000000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-enable-ramp-delay = <218>;
+						regulator-boot-on;
+					};
+
+					mt6397_vgp1_reg: ldo_vcamd {
+						regulator-name = "vgp1";
+						regulator-min-microvolt = <1220000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-enable-ramp-delay = <240>;
+					};
+
+					mt6397_vgp2_reg: ldo_vcamio {
+						regulator-name = "vgp2";
+						regulator-min-microvolt = <1200000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-enable-ramp-delay = <218>;
+					};
+
+					mt6397_vgp3_reg: ldo_vcamaf {
+						regulator-name = "vgp3";
+						regulator-min-microvolt = <1200000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-enable-ramp-delay = <218>;
+					};
+
+					mt6397_vgp4_reg: ldo_vgp4 {
+						regulator-name = "vgp4";
+						regulator-min-microvolt = <1200000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-enable-ramp-delay = <218>;
+					};
+
+					mt6397_vgp5_reg: ldo_vgp5 {
+						regulator-name = "vgp5";
+						regulator-min-microvolt = <1200000>;
+						regulator-max-microvolt = <3000000>;
+						regulator-enable-ramp-delay = <218>;
+					};
+
+					mt6397_vgp6_reg: ldo_vgp6 {
+						regulator-name = "vgp6";
+						regulator-min-microvolt = <1200000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-enable-ramp-delay = <218>;
+					};
+
+					mt6397_vibr_reg: ldo_vibr {
+						regulator-name = "vibr";
+						regulator-min-microvolt = <1200000>;
+						regulator-max-microvolt = <3300000>;
+						regulator-enable-ramp-delay = <218>;
+					};
+				};
 			};
 		};