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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <538CA5C3.5050709@wwwdotorg.org>
Date:	Mon, 02 Jun 2014 10:26:43 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Marcel Ziswiler <marcel@...wiler.com>, thierry.reding@...il.com
CC:	linux@....linux.org.uk, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	linux-tegra@...r.kernel.org, stefan@...er.ch
Subject: Re: [PATCH 3/3] arm: tegra: initial support for apalis t30

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@...00000 {
> +		dc@...00000 {
> +			rgb {
> +				status = "okay";
> +				nvidia,panel = <&panel>;
> +			};
> +		};
> +		hdmi@...80000 {

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

> +	serial@...06040 {
> +		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@...0d400 {
> +		status = "okay";
> +		spi-max-frequency = <25000000>;
> +		spidev0: spidev@1 {

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

> +	sd1: sdhci@...00000 {
...
> +	mmc1: sdhci@...00400 {

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