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

[Nishanth Menon]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/