Hi, On Fri, Nov 14, 2014 at 04:47:02PM -0600, George McCollister wrote: > Felipe, > > Thank you for your reply. no problem > >> + vbat: fixedregulator@0 { > >> + compatible = "regulator-fixed"; > >> + regulator-name = "vbat"; > >> + regulator-min-microvolt = <5000000>; > >> + regulator-max-microvolt = <5000000>; > >> + regulator-boot-on; > >> + }; > > > > I suppose this is the 5V on a power jack, or something like that ? > > It comes with one of three different power supplies (24 - 250VDC, 120 > - 240VAC, 12VDC input) all of which ultimately supply a fixed 5V and > 3.3V. Alright :-) Thanks. Do you mind adding a comment on this DTS stating that just so people don't get confused ? > > > >> + vmmcsd_fixed: fixedregulator@0 { > >> + compatible = "regulator-fixed"; > >> + regulator-name = "vmmcsd_fixed"; > >> + regulator-min-microvolt = <3300000>; > >> + regulator-max-microvolt = <3300000>; > > > > but this... I know every other board devices this as a fixed regulator, > > but is it really a fixed regulator or is supplied by one of the LDOs on > > the PMIC ? > > > > It's actually fixed (not from TP65910). Oh, so this 3.3V fixed rail is the same one derived from from the three possible power supplies you described above ? > >> +&am33xx_pinmux { > >> + mmc1_pins: pinmux_mmc1_pins { > >> + pinctrl-single,pins = < > >> + 0xf0 (PIN_INPUT_PULLUP | MUX_MODE0) /* mmc0_dat3 */ > >> + 0xf4 (PIN_INPUT_PULLUP | MUX_MODE0) /* mmc0_dat2 */ > >> + 0xf8 (PIN_INPUT_PULLUP | MUX_MODE0) /* mmc0_dat1 */ > >> + 0xfc (PIN_INPUT_PULLUP | MUX_MODE0) /* mmc0_dat0 */ > >> + 0x100 (PIN_INPUT_PULLUP | MUX_MODE0) /* mmc0_clk */ > >> + 0x104 (PIN_INPUT_PULLUP | MUX_MODE0) /* mmc0_cmd */ > >> + >; > >> + }; > >> + > >> + i2c0_pins: pinmux_i2c0_pins { > >> + pinctrl-single,pins = < > >> + 0x188 (PIN_INPUT_PULLUP | MUX_MODE0) /* i2c0_sda.i2c0_sda */ > >> + 0x18c (PIN_INPUT_PULLUP | MUX_MODE0) /* i2c0_scl.i2c0_scl */ > >> + >; > >> + }; > >> + > >> + i2c2_pins: pinmux_i2c2_pins { > >> + pinctrl-single,pins = < > >> + 0x178 (PIN_INPUT_PULLUP | MUX_MODE3) /* uart1_ctsn.i2c2_sda */ > >> + 0x17c (PIN_INPUT_PULLUP | MUX_MODE3) /* uart1_rtsn.i2c2_scl */ > > > > on thing to keep in mind, if you already have external pullups, you > > might not want to add internal pullups as you'd end up with both > > resistors in parallel. For I2C the danger is minimal (unless you have a > > ton of bus capacitance, then it changes high/low time), but it's best to > > write a more "pristine" DTS. (and sure, I know pretty much every board > > makes this mistake, but it's best if we don't proliferate the error) > > I'll make sure this is correct and include any required changes in the > next version of the patch series. cool, thanks > >> +&i2c0 { > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&i2c0_pins>; > >> + > >> + status = "okay"; > >> + clock-frequency = <400000>; > >> + > >> + serial_config1: serial_config1@20 { > >> + compatible = "nxp,pca9539"; > >> + reg = <0x20>; > >> + }; > >> + > >> + serial_config2: serial_config2@21 { > >> + compatible = "nxp,pca9539"; > >> + reg = <0x21>; > >> + }; > >> + > >> + tps: tps@2d { > >> + reg = <0x2d>; > > > > which TPS device ? no compatible ? > > > >> +/include/ "tps65910.dtsi" > > > > oh... okay. > > I'm assuming that means you're okay with this (if not please elaborate > on how to improve it). sure, i'm okay. But it's still nice to add a compatible to that tps node, then again, it's added to tps65910.dtsi so that would be a very minor nit. > >> +&tps { > >> + vcc1-supply = <&vbat>; > >> + vcc2-supply = <&vbat>; > >> + vcc3-supply = <&vbat>; > >> + vcc4-supply = <&vbat>; > >> + vcc5-supply = <&vbat>; > >> + vcc6-supply = <&vbat>; > >> + vcc7-supply = <&vbat>; > >> + vccio-supply = <&vbat>; > >> + > >> + regulators { > >> + vrtc_reg: regulator@0 { > >> + regulator-always-on; > > > > this should not be always on, you want to pass this as supply to the RTC > > module so it can manage it. It's also best to give names to all > > regulators, so people know what they're used for. > > I think we may actually be able to turn this one and possibly two > others off, I will investigate. > > I've come up with names for all of the regulators being used and will > include the changes in the next version of the patch series. Thanks, that helps reviewing the validity of your DTS binding too. > >> + }; > >> + > >> + vio_reg: regulator@1 { > >> + regulator-always-on; > >> + }; > >> + > >> + vdd1_reg: regulator@2 { > >> + regulator-name = "vdd_mpu"; > >> + regulator-min-microvolt = <600000>; > >> + regulator-max-microvolt = <1500000>; > >> + regulator-boot-on; > >> + regulator-always-on; > >> + }; > >> + > >> + vdd2_reg: regulator@3 { > >> + regulator-always-on; > >> + }; > >> + > >> + vdd3_reg: regulator@4 { > >> + regulator-always-on; > >> + }; > >> + > >> + vdig1_reg: regulator@5 { > >> + regulator-always-on; > >> + }; > >> + > >> + vdig2_reg: regulator@6 { > >> + regulator-always-on; > >> + }; > >> + > >> + vpll_reg: regulator@7 { > >> + regulator-always-on; > >> + }; > >> + > >> + vdac_reg: regulator@8 { > >> + regulator-always-on; > >> + }; > >> + > >> + vaux1_reg: regulator@9 { > >> + regulator-always-on; > >> + }; > >> + > >> + vaux2_reg: regulator@10 { > >> + regulator-always-on; > >> + }; > >> + > >> + vaux33_reg: regulator@11 { > > > > isn't this the supply to the other MMC slot ? > > no, it's actually being used for the USB PHY. As I said above I will > name all regulators being used. cool, thanks > >> + regulator-always-on; > >> + }; > >> + > >> + vmmc_reg: regulator@12 { > >> + regulator-min-microvolt = <3300000>; > >> + regulator-max-microvolt = <3300000>; > >> + regulator-always-on; > >> + }; > >> + }; > >> +}; > >> + > >> +&sham { > >> + status = "okay"; > >> +}; > >> + > >> +&aes { > >> + status = "okay"; > >> +}; > > > > just making sure, are you really using them ? > > We may need them at some point, I'd like to keep them enabled. fair enough. > >> +&mmc1 { > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&mmc1_pins>; > >> + bus-width = <4>; > >> + status = "okay"; > >> + vmmc-supply = <&vmmc_reg>; > >> + ti,vcc-aux-disable-is-sleep; > > > > this binding isn't documented anywhere. What was it supposed to do ? > > I mentioned in the commit message that there is a micro SD slot. Sure, that's fine. My comment is regarding "ti,vcc-aux-disable-is-sleep", it's not documentated or used anywhere :-) cheers -- balbi