On Tue, Jun 10, 2014 at 8:59 AM, Florian Vaussard
<florian.vaussard@xxxxxxx> wrote:
> It is not very common to add oneself as the maintainer of a single .dts
> file. I could only found the am335x-nano.dts with such a way of doing.
> Usually get_maintainer.pl will also take into account the commit author
> and properly handle this case.
Okay--I'll remove this. Ironically, I was looking at the nano to make
sure I'd done everything I needed to ;-).
...
> For all the boards that I have seen, all the properties of a new node
> are declared at the same place, where you put some of them at the end. I
> have not a strong opinion on this, but I find it a bit more difficult to
> read.
...
> Splitting the pinmux is also unusual, but in this case I found it more
> readable.
My goal both by moving the properties and the pinmux was to separate
out chunks of functionality into discrete sections. It makes it easy
to resolve an issue with a specific board feature (e.g. LEDs) or adapt
for a new board version as the pinmux and any node properties are
grouped together.
I recognize this is something of a personal bias though so I can
certainly re-organize if this is stylistically bad.
...
>> +&am33xx_pinmux {
>> + accel_pins: pinmux_accel {
>> + pinctrl-single,pins = <
>> + 0x98 (PIN_INPUT_PULLUP | MUX_MODE7) /* gpmc_wen.gpio2_4 */
>
> The INT pin of the LIS33 chip seems to be push-pull, thus enabling the
> pullup on the SoC side will increase the current consumption when held down.
Good catch.
...
>> +&am33xx_pinmux {
>> + audio_pins: pinmux_audio {
>> + pinctrl-single,pins = <
>> + 0x1AC (PIN_INPUT_PULLDOWN | MUX_MODE0) /* mcasp0_ahcklx.mcasp0_ahclkx */
>> + 0x194 (PIN_INPUT_PULLDOWN | MUX_MODE0) /* mcasp0_fsx.mcasp0_fsx */
>> + 0x190 (PIN_INPUT_PULLDOWN | MUX_MODE0) /* mcasp0_aclkx.mcasp0_aclkx */
>> + 0x198 (PIN_INPUT_PULLDOWN | MUX_MODE0) /* mcasp0_axr0.mcasp0_axr0 */
>> + 0x1A8 (PIN_INPUT_PULLDOWN | MUX_MODE0) /* mcasp0_axr1.mcasp0_axr1 */
>> + 0x40 (PIN_OUTPUT_PULLUP | MUX_MODE7) /* gpmc_a0.gpio1_16 */
>
> You already have an external pullup in your design, so this one seems
> superfluous.
You're correct. Changed.
...
>> +/* Power */
>> +&vbat {
>> + regulator-name = "vbat";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <5000000>;
>> + regulator-always-on;
removed regulator-always-on
>> +};
>> +
>> +&v3v3c_reg {
>> + regulator-name = "v3v3c_reg";
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + vin-supply = <&vbat>;
>> + regulator-always-on;
removed regulator-always-on
>> +};
>> +
>> +&vdd5_reg {
>> + regulator-name = "vdd5_reg";
>> + regulator-min-microvolt = <5000000>;
>> + regulator-max-microvolt = <5000000>;
>> + regulator-always-on;
removed regulator-always-on
>> + vin-supply = <&vbat>;
>> +};
>> +
>> +/include/ "tps65217.dtsi"
>> +
>> +&tps {
>> + backlight {
>> + isel = <1>; /* ISET1 */
>> + fdim = <200>; /* TPS65217_BL_FDIM_200HZ */
>> + default-brightness = <80>;
>> + };
>> +
>> + regulators {
>> + dcdc1_reg: regulator@0 {
>> + /* VDD_1V8 system supply */
>> + regulator-always-on;
removed regulator-always-on
>> + };
>> +
>> + dcdc2_reg: regulator@1 {
>> + /* VDD_CORE voltage limits 0.95V - 1.26V with +/-4% tolerance */
>> + regulator-name = "vdd_core";
>> + regulator-min-microvolt = <925000>;
>> + regulator-max-microvolt = <1325000>;
>> + regulator-boot-on;
>> + regulator-always-on;
removed regulator-always-on
>> + };
>> +
>> + dcdc3_reg: regulator@2 {
>> + /* VDD_MPU voltage limits 0.95V - 1.1V with +/-4% tolerance */
>> + regulator-name = "vdd_mpu";
>> + regulator-min-microvolt = <925000>;
>> + regulator-max-microvolt = <1150000>;
>> + regulator-boot-on;
>> + regulator-always-on;
removed regulator-always-on
>> + };
>> +
>> + ldo1_reg: regulator@3 {
>> + /* VRTC 1.8V always-on supply */
>> + regulator-always-on;
This is the backup supply shouldn't be switched off.
>> + };
>> +
>> + ldo2_reg: regulator@4 {
>> + /* 3.3V rail */
>> + regulator-always-on;
removed regulator-always-on
>> + };
>> +
>> + ldo3_reg: regulator@5 {
>> + /* VDD_3V3A 3.3V rail */
>> + regulator-always-on;
removed regulator-always-on
>> + regulator-min-microvolt = <3300000>;
>> + regulator-max-microvolt = <3300000>;
>> + };
>> +
>> + ldo4_reg: regulator@6 {
>> + /* VDD_3V3B 3.3V rail */
>> + regulator-always-on;
removed regulator-always-on
>> + };
>
> Why are all the regulators always on? Most of them should be managed
> automatically if you have correct dependencies, shouldn't they?
A little cargo-cult I fear. I've removed as noted above.
...
> You can use the key codes defined in <dt-bindings/input/input.h>.
Ah--cool. Will do.
>
>> + gpios = <&gpio1 22 GPIO_ACTIVE_LOW>;
>> + gpio-key,wakeup;
>> + };
>> +
>> + button@1 {
>> + label = "menu";
>> + linux,code = <139>;
>
> KEY_MENU
>
>> + gpios = <&gpio1 23 GPIO_ACTIVE_LOW>;
>> + gpio-key,wakeup;
>> + };
>> +
>> + buttons@2 {
>> + label = "power";
>> + linux,code = <116>;
>
> KEY_POWER
>
>> + gpios = <&gpio0 7 GPIO_ACTIVE_LOW>;
>> + gpio-key,wakeup;
>> + };
>> +};
>> +
>> +&am33xx_pinmux {
>> + user_leds_pins: pinmux_user_leds {
>> + pinctrl-single,pins = <
>> + 0x50 (PIN_OUTPUT_PULLDOWN | MUX_MODE7) /* gpmc_a4.gpio1_20 */
>> + 0x54 (PIN_OUTPUT_PULLDOWN | MUX_MODE7) /* gpmc_a5.gpio1_21 */
>
> Why enabling the pulldown on an output gpio? You will increase the power
> consumption when driving high.
These work fine as simple outputs.
Thanks for your careful review Florian. Much appreciated.
--Ash
--
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
