| Message ID | 1453185124-30809-6-git-send-email-puck.chen@hisilicon.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jan 19, 2016 at 02:32:04PM +0800, Chen Feng wrote: > index 82d2488..6de9881 100644 > --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > + regulators { > + ldo2: ldo2@a21 { > + regulator-compatible = "LDO2"; Why are you using the legacy regulator-compatible property? No new bindings should use this. > + regulator-min-microvolt = <2500000>; > + regulator-max-microvolt = <3200000>; This is broken as it misunderstands the purpose of specifying constraints. The constraints are there to say what the safe and supported configuration is on a given board, it is not possible to provide this information safely in a general include that is used by all systems using the PMIC. Specifying the maximum voltage range for the regulators is almost guaranteed to result in at least some configurations being enabled which will not work, in the worst case this may include configurations which could physically damage the system. In general it is very unusual to include the regulators in a .dtsi since essentially all the configuration for them should be board specific.
Mark, On 2016/1/20 21:08, Mark Brown wrote: > On Tue, Jan 19, 2016 at 02:32:04PM +0800, Chen Feng wrote: > >> index 82d2488..6de9881 100644 >> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi >> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi > >> + regulators { >> + ldo2: ldo2@a21 { >> + regulator-compatible = "LDO2"; > I will change it like this: regulators { ldo2: LDO2@a21 { regulator-name = "ldo2"; regulator-min-microvolt = <2500000>; regulator-max-microvolt = <3200000>; regulator-valid-modes-mask = <0x02>; regulator-enable-ramp-delay = <120>; }; > Why are you using the legacy regulator-compatible property? No new > bindings should use this. > >> + regulator-min-microvolt = <2500000>; >> + regulator-max-microvolt = <3200000>; > > This is broken as it misunderstands the purpose of specifying > constraints. The constraints are there to say what the safe and > supported configuration is on a given board, it is not possible to > provide this information safely in a general include that is used by all > systems using the PMIC. Specifying the maximum voltage range for the > regulators is almost guaranteed to result in at least some > configurations being enabled which will not work, in the worst case this > may include configurations which could physically damage the system. > > In general it is very unusual to include the regulators in a .dtsi since > essentially all the configuration for them should be board specific. > Do you mean that I should move this into dts and enable it by default?
On Thu, Jan 21, 2016 at 06:32:32PM +0800, chenfeng wrote: > On 2016/1/20 21:08, Mark Brown wrote: > > On Tue, Jan 19, 2016 at 02:32:04PM +0800, Chen Feng wrote: > I will change it like this: > regulators { > ldo2: LDO2@a21 { > regulator-name = "ldo2"; > regulator-min-microvolt = <2500000>; > regulator-max-microvolt = <3200000>; > regulator-valid-modes-mask = <0x02>; > regulator-enable-ramp-delay = <120>; > }; No, do not include the voltage constraints, see the second half of my reply. > >> + regulator-min-microvolt = <2500000>; > >> + regulator-max-microvolt = <3200000>; > > This is broken as it misunderstands the purpose of specifying > > constraints. The constraints are there to say what the safe and > > supported configuration is on a given board, it is not possible to > > provide this information safely in a general include that is used by all > > systems using the PMIC. Specifying the maximum voltage range for the > > regulators is almost guaranteed to result in at least some > > configurations being enabled which will not work, in the worst case this > > may include configurations which could physically damage the system. > > In general it is very unusual to include the regulators in a .dtsi since > > essentially all the configuration for them should be board specific. > Do you mean that I should move this into dts and enable it by default? It would be more normal to put everything to do with the regulators into the board DTS. It is unlikely that a straight move would be the right thing, you would need to understand what all the voltage ranges on the board are and set them appropriately and should normally also be naming the regulators as per the schematic so users can tie the DT and the schematic together.
diff --git a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts index 8d43a0f..f714ac7 100644 --- a/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts +++ b/arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts @@ -31,4 +31,9 @@ device_type = "memory"; reg = <0x0 0x0 0x0 0x40000000>; }; + +}; + +&pmic { + status = "okay"; }; diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi index 82d2488..6de9881 100644 --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi @@ -208,5 +208,92 @@ clock-names = "uartclk", "apb_pclk"; status = "disabled"; }; + + }; + + pmic: pmic@f8000000 { + compatible = "hisilicon,hi655x-pmic"; + reg = <0x0 0xf8000000 0x0 0x1000>; + #interrupt-cells = <2>; + interrupt-controller; + pmic-gpios = <&gpio1 2 0>; + status = "disabled"; + + regulators { + ldo2: ldo2@a21 { + regulator-compatible = "LDO2"; + regulator-min-microvolt = <2500000>; + regulator-max-microvolt = <3200000>; + regulator-enable-ramp-delay = <120>; + }; + + ldo7: ldo7@a26 { + regulator-compatible = "LDO7"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + regulator-enable-ramp-delay = <120>; + }; + + ldo10: ldo10@a29 { + regulator-compatible = "LDO10"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3000000>; + regulator-enable-ramp-delay = <360>; + }; + + ldo13: ldo13@a32 { + regulator-compatible = "LDO13"; + regulator-min-microvolt = <1600000>; + regulator-max-microvolt = <1950000>; + regulator-enable-ramp-delay = <120>; + }; + + ldo14: ldo14@a33 { + regulator-compatible = "LDO14"; + regulator-min-microvolt = <2500000>; + regulator-max-microvolt = <3200000>; + regulator-enable-ramp-delay = <120>; + }; + + ldo15: ldo15@a34 { + regulator-compatible = "LDO15"; + regulator-min-microvolt = <1600000>; + regulator-max-microvolt = <1950000>; + regulator-boot-on; + regulator-always-on; + regulator-enable-ramp-delay = <120>; + }; + + ldo17: ldo17@a36 { + regulator-compatible = "LDO17"; + regulator-min-microvolt = <2500000>; + regulator-max-microvolt = <3200000>; + regulator-enable-ramp-delay = <120>; + }; + + ldo19: ldo19@a38 { + regulator-compatible = "LDO19"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3000000>; + regulator-enable-ramp-delay = <360>; + }; + + ldo21: ldo21@a40 { + regulator-compatible = "LDO21"; + regulator-min-microvolt = <1650000>; + regulator-max-microvolt = <2000000>; + regulator-always-on; + regulator-enable-ramp-delay = <120>; + }; + + ldo22: ldo22@a41 { + regulator-compatible = "LDO22"; + regulator-min-microvolt = <900000>; + regulator-max-microvolt = <1200000>; + regulator-boot-on; + regulator-always-on; + regulator-enable-ramp-delay = <120>; + }; + }; }; };