Re: [PATCH v2 2/2] arm: dts: add support for AM437x StarterKit

Nishanth Menon
2014-06-18T21:54:12Z

On 06/18/2014 02:31 PM, Felipe Balbi wrote:
> On Wed, Jun 18, 2014 at 11:14:21AM -0500, Nishanth Menon wrote:
>> On 06/18/2014 10:43 AM, Felipe Balbi wrote:
>>> Add support for TI's AM437x StarterKit Evaluation
>>> Module.
>>
>> is there a link for this platform?
>
> internal only

but will eventually be sold externally? I assume this is not an TI
internal only board.
[...]
>>> +
>>> + matrix_keypad: matrix_keypad@0 {
>>> + compatible = "gpio-matrix-keypad";
>>
>> no pinctrl needed?
>
> pins are gpio by default

Might be good to explicitly configure it - no strong opinions though ->
GPIOs are always good to pinctrl up esp if bootloader screws up at a
later date.

[...]
>>> +&i2c0 {
>>> + status = "okay";
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&i2c0_pins>;
>>
>> what speed are you running this on? -> also can you align these to 1
>
> 100kHz ?

Rule of thumb is to do the following:
MIN(MAX_FREQ(D1), MAX_FREQ(D2).... MAX_FREQ(Dn));
where D1..n are all the peripherals on this i2c bus.

>>> + tps@24 {
>>> + compatible = "ti,tps65218";
>>> + reg = <0x24>;
>>> + interrupt-parent = <&gic>;
>>> + interrupts = <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
>>
>> is this muxed?
>
> there's no configuration for this. This pin is a single function.
>
>>> + interrupt-controller;
>>> + #interrupt-cells = <2>;
>>> +
>>> + dcdc1: regulator-dcdc1 {
>>> + compatible = "ti,tps65218-dcdc1";
>>> + /* VDD_CORE limits min of OPP50 and max of OPP100 */
>>> + regulator-name = "vdd_core";
>>> + regulator-min-microvolt = <912000>;
>>> + regulator-max-microvolt = <1144000>;
>>> + regulator-boot-on;
>>> + regulator-always-on;
>>> + };
>>> +
>>> + dcdc2: regulator-dcdc2 {
>>> + compatible = "ti,tps65218-dcdc2";
>>> + /* VDD_MPU limits min of OPP50 and max of OPP_NITRO */
>>> + regulator-name = "vdd_mpu";
>>> + regulator-min-microvolt = <912000>;
>>> + regulator-max-microvolt = <1378000>;
>>> + regulator-boot-on;
>>> + regulator-always-on;
>>> + };
>>> +
>>> + dcdc3: regulator-dcdc3 {
>>> + compatible = "ti,tps65218-dcdc3";
>>> + regulator-name = "vdds_ddr";
>> no voltage ?
>
> has no users in kernel. Also, it comes out with default, and correct,
> voltage.

Device tree is description of hardware, not just who uses what in OS of
interest.

you might consider u-boot to use the same device tree at a later date
and having complete details about the hardware is always the norm.

I suggest setting the voltage here to be complete even if there are no
current users.

>>> + edt-ft5306@38 {
>>> + status = "okay";
>>> + compatible = "edt,edt-ft5306", "edt,edt-ft5x06";
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&edt_ft5306_ts_pins>;
>>> + reg = <0x38>;
>>> + interrupt-parent = <&gpio0>;
>>> + interrupts = <31 0>;
>>> +
>>> + wake-gpios = <&gpio1 28 GPIO_ACTIVE_HIGH>;
>>
>> why wake-gpios? we should be using pinctrl with interrupt-extended to
>> do wakeup sequence, no?
>
> sure, can you patch the edt driver ? I'll fix the DTS after that gets
> merged

If you really want to go down that road, so you could probably help
review the pinctrl patches I posted to enable pinctrl wakeup[1]?

Come on, as of today, there is no ability to suspend AM437x without
doing [1], let alone talk about wakeup gpio vs interrupt-extended. and
do we really want to wakeup from suspend when touch screen is touched?

Do you expect wake-gpio to work even after doing interrupt based
solution? I am no edt driver expert... maybe you can help me here.

>>> +&mmc1 {
>>> + status = "okay";
>>> + vmmc-supply = <&dcdc4>;
>>> + bus-width = <4>;
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&mmc1_pins>;
>>
>> just for style, wonder if moving the pinctrl just after status is better?
>
> why ? makes no difference.

it does not - I agree, except, when you look at all other nodes:
status="okay"
pinctrl
other things..

it is just a symmetry thing, I guess..

>
>>> + cd-gpios = <&gpio0 6 GPIO_ACTIVE_HIGH>;
>>> +};
>>> +
>>> +&usb2_phy1 {
>>> + status = "okay";
>>> +};
>>> +
>>> +&usb1 {
>>> + dr_mode = "peripheral";
>>> + status = "okay";
>>> +};
>>> +
>>> +&usb2_phy2 {
>>> + status = "okay";
>>> +};
>>> +
>>> +&usb2 {
>>> + dr_mode = "host";
>>> + status = "okay";
>>> +};
>> none of the above need pinctrl? no regulator supplies?
>
> pins in default states, drivers don't use regulators.

USB works without a supply? even a fixed voltage supply? that is weird.

[..]

[1] http://marc.info/?l=devicetree&m=140301966510748&w=2