Hello Heiko, On Fri, Jan 15, 2016 at 11:26 AM, Heiko Stuebner <heiko@xxxxxxxxx> wrote: > Am Freitag, 15. Januar 2016, 11:22:52 schrieb Javier Martinez Canillas: >> Hello Heiko, >> >> On Fri, Jan 15, 2016 at 11:03 AM, Heiko Stuebner <heiko@xxxxxxxxx> wrote: >> > Hi Javier, >> > >> > Am Freitag, 15. Januar 2016, 10:28:44 schrieb Javier Martinez Canillas: >> >> This is not a complete review but I just wanted to comment on two >> > >> >> things that I noticed: >> > same for me for now :-) >> > >> >> On Fri, Jan 15, 2016 at 10:06 AM, Matthias Brugger >> >> <matthias.bgg@xxxxxxxxx> wrote: >> >> >> >> [snip] >> >> >> >> > + }; >> >> > + >> >> > + vcc_18: vcc18-regulator { >> >> > + compatible = "regulator-fixed"; >> >> > + regulator-name = "vcc_18"; >> >> > + regulator-min-microvolt = <1800000>; >> >> > + regulator-max-microvolt = <1800000>; >> >> > + regulator-always-on; >> >> > + regulator-boot-on; >> >> > + vin-supply = <&vcc_sys>; >> >> > + }; >> >> > + >> >> > + /* supplies both host and otg */ >> >> > + vcc_host: vcc-host-regulator { >> >> > + compatible = "regulator-fixed"; >> >> > + gpio = <&gpio0 4 GPIO_ACTIVE_LOW>; >> >> > + pinctrl-names = "default"; >> >> > + pinctrl-0 = <&host_vbus_drv>; >> >> > + regulator-name = "vcc_host"; >> >> > + regulator-always-on; >> >> > + regulator-boot-on; >> >> > + vin-supply = <&vcc_sys>; >> >> > + }; >> >> > + >> >> > + vccio_sd: vcc-io-sd-regulator { >> >> > + regulator-name= "vccio_sd"; >> >> > + gpio = <&gpio0 9 GPIO_ACTIVE_LOW>; >> >> > + regulator-min-microvolt = <3300000>; >> >> > + regulator-max-microvolt = <3300000>; > > that seems to be missing essential regulator properties > like a compatible. > Oh right, I missed that when first reading the patch. >> >> > + }; >> >> > + >> >> > + vcc_sd: vcc-sd-regulator { >> >> > + compatible = "regulator-fixed"; >> >> > + regulator-name = "vcc_sd"; >> >> > + gpio = <&gpio3 11 GPIO_ACTIVE_LOW>; >> >> > + regulator-min-microvolt = <3300000>; >> >> > + regulator-max-microvolt = <3300000>; >> >> > + regulator-always-on; >> >> > + regulator-boot-on; >> >> > + vin-supply = <&vcc_io>; >> >> > + }; >> >> > + >> >> > + vcc_io: vcc-io-regulator { >> >> > + compatible = "regulator-fixed"; >> >> > + regulator-name = "vcc_io"; >> >> > + regulator-min-microvolt = <3300000>; >> >> > + regulator-max-microvolt = <3300000>; >> >> > + regulator-always-on; >> >> > + regulator-boot-on; >> >> > + vin-supply = <&vcc_sys>; >> >> > + }; >> >> > + >> >> > + vcc_lan: vcc-lan-regulator { >> >> > + compatible = "regulator-fixed"; >> >> > + regulator-name = "vcc_lan"; >> >> > + regulator-min-microvolt = <3300000>; >> >> > + regulator-max-microvolt = <3300000>; >> >> > + regulator-always-on; >> >> > + regulator-boot-on; >> >> > + vin-supply = <&vcc_io>; >> >> > + }; >> >> > + >> >> > + vcc_sys: vcc-sys-regulator { >> >> > + compatible = "regulator-fixed"; >> >> > + regulator-name = "vcc_sys"; >> >> > + regulator-min-microvolt = <5000000>; >> >> > + regulator-max-microvolt = <5000000>; >> >> > + regulator-always-on; >> >> > + regulator-boot-on; >> >> > + }; >> >> > + >> >> > + vccio_wl: vccio-wl-regulator { >> >> > + compatible = "regulator-fixed"; >> >> > + regulator-name = "vccio_wl"; >> >> > + regulator-min-microvolt = <3300000>; >> >> > + regulator-max-microvolt = <3300000>; >> >> > + regulator-always-on; >> >> > + regulator-boot-on; >> >> > + vin-supply = <&vcc_io>; >> >> > + }; >> >> > + >> >> > + vdd_10: vdd-10-regulator { >> >> > + compatible = "regulator-fixed"; >> >> > + regulator-name = "vdd_10"; >> >> > + regulator-min-microvolt = <1000000>; >> >> > + regulator-max-microvolt = <1000000>; >> >> > + regulator-always-on; >> >> > + regulator-boot-on; >> >> > + vin-supply = <&vcc_sys>; >> >> > + }; >> >> > +}; >> >> >> >> There is only one regulator that is not marked as always-on. This will >> >> prevent the regulator subsystem to disable unused regulators. Do you >> >> really need all of them to be always-on? >> > >> > I do believe the regulators should be more or less correct. Sadly there >> > are no real schematics for that specific device available, but the >> > reference design for rk3368-based tv-boxes uses a hirarchy of >> > hard-wired individual regulators without any ability to control for >> > most of them [aka always on]. >> > >> > And due to the r68 not sporting any different real pmic, I'd believe the >> > Orion will most likely also follow that scheme. >> >> I see, thanks a lot for the clarification. But then in that case I >> wonder what's the point of defining the gpio property in the fixed >> regulator device nodes? > > the ones with gpio properties actually can be turned off - here they are the > different supplies for usb and sd-card only, so leaf-regulators without > further children. > Ok, sorry I misunderstood and thought you were referring to all regulators. I of course agree that regulators that can't be turned of should be marked as always-on. I just asked this to be revisit and only mark as always-on the ones that really should be. > > Heiko Best regards, Javier -- 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