Re: [PATCH 3/3] arm: tegra: initial support for apalis t30

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

On 06/01/2014 05:37 PM, Marcel Ziswiler wrote:
> This patch adds the device tree to support Toradex Apalis T30, a
> computer on module which can be used on different carrier boards.
> 
> The module consists of a Tegra 3 SoC, two PMICs, 1 or 2 GB of DDR3L
> RAM, eMMC, an LM95245 temperature sensor chip, an i210 resp. i211
> gigabit Ethernet controller, an STMPE811 ADC/touch controller as well
> as two MCP2515 CAN controllers. Furthermore, there is an SGTL5000 audio
> codec which is not yet supported. Anything that is not self contained
> on the module is disabled by default.
> 
> The device tree for the Evaluation Board includes the modules device
> tree and enables the supported peripherals of the carrier board (the
> Evaluation Board supports almost all of them).
> 
> While at it also add the device tree binding documentation for Apalis
> T30 as well as the previously missing one for the recently added
> Colibri T30.

> diff --git a/Documentation/devicetree/bindings/arm/tegra.txt b/Documentation/devicetree/bindings/arm/tegra.txt

> +  toradex,colibri_t30
> +  toradex,colibri_t30-eval-v3

Those don't seem to be related to Apalis support.

> diff --git a/arch/arm/boot/dts/tegra30-apalis-eval.dts b/arch/arm/boot/dts/tegra30-apalis-eval.dts
> +	host1x@50000000 {
> +		dc@54200000 {
> +			rgb {
> +				status = "okay";
> +				nvidia,panel = <&panel>;
> +			};
> +		};
> +		hdmi@54280000 {

Nit: Add a blank line between the nodes. Check elsewhere too.

> +	serial@70006040 {
> +		compatible = "nvidia,tegra30-hsuart";
> +		status = "okay";
> +	};

Nit: Put the status property first followed by new/overridden
properties, to be consistent with other Tegra DTs. Check elsewhere too.

> +	/* SPI1: Apalis SPI1 */
> +	spi@7000d400 {
> +		status = "okay";
> +		spi-max-frequency = <25000000>;
> +		spidev0: spidev@1 {

Nit: Add a blank line between properties and nodes. Check elsewhere too.

> +	sd1: sdhci@78000000 {
...
> +	mmc1: sdhci@78000400 {

Do those nodes really need labels? Nothing appears to reference them,
and I can't see why anything would.

Should the mmc1 node be non-removable? It seems a bit odd for a
removable device to have an 8-bit data bus.

> +	backlight: backlight {
> +		compatible = "pwm-backlight";
> +
> +		/* PWM0 */

Nit: No need for a blank line between a bunch of related properties.
Check elsewhere too.

> +	pwmleds {
> +		compatible = "pwm-leds";
> +
> +		pwm3 {
> +			label = "PWM3";
> +			pwms = <&pwm 1 19600>;
> +			max-brightness = <255>;
> +		};
> +		pwm2 {
> +			label = "PWM2";
> +			pwms = <&pwm 2 19600>;
> +			max-brightness = <255>;
> +		};
> +		pwm1 {
> +			label = "PWM1";
> +			pwms = <&pwm 3 19600>;
> +			max-brightness = <255>;
> +		};

Nit: Why not sort those nodes in numerical order?

> +	regulators {
> +		sys_5v0_reg: regulator@1 {

Nit: Why not start the numbering at 0?

> diff --git a/arch/arm/boot/dts/tegra30-apalis.dtsi b/arch/arm/boot/dts/tegra30-apalis.dtsi

> +	pinmux@70000868 {
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&state_default>;
> +
> +		state_default: pinmux {

It might make sense to add all the pinmux data to
https://github.com/NVIDIA/tegra-pinmux-scripts, so that both the kernel
and U-Boot pinmux initialization tables can be auto-generated from a
single data-structure. I think that'll get a small amount of
error-/consistency-checking of the data too.