Re: [PATCH 1/1] Add PPD device tree

Path	csiph.com!eternal-september.org!feeder.eternal-september.org!aioe.org!bofh.it!news.nic.it!robomod
From	Mark Rutland <mark.rutland@arm.com>
Newsgroups	linux.kernel
Subject	Re: [PATCH 1/1] Add PPD device tree
Date	Fri, 05 Aug 2016 15:50:01 +0200
Message-ID	<s2NFD-8dj-23@gated-at.bofh.it> (permalink)
References	<s2NvX-89z-11@gated-at.bofh.it>
MIME-Version	1.0
Content-Type	text/plain; charset=us-ascii
Content-Disposition	inline
User-Agent	Mutt/1.5.21 (2010-09-15)
Sender	robomod@news.nic.it
List-ID	<linux-kernel.vger.kernel.org>
X-Mailing-List	linux-kernel@vger.kernel.org
Approved	robomod@news.nic.it
Lines	124
Organization	linux.* mail to news gateway
X-Original-Cc	Rob Herring <robh+dt@kernel.org>, Russell King <linux@armlinux.org.uk>, Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <kernel@pengutronix.de>, Fabio Estevam <fabio.estevam@nxp.com>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, "moderated list:ARM PORT" <linux-arm-kernel@lists.infradead.org>, open list <linux-kernel@vger.kernel.org>
X-Original-Date	Fri, 5 Aug 2016 14:39:55 +0100
X-Original-Message-ID	<20160805133954.GC25152@leverpostej>
X-Original-References	<1470403887-15190-1-git-send-email-fabien.lahoudere@collabora.co.uk>
X-Original-Sender	linux-kernel-owner@vger.kernel.org
Xref	csiph.com linux.kernel:1457108

Show key headers only | View raw

On Fri, Aug 05, 2016 at 03:31:25PM +0200, Fabien Lahoudere wrote:
> +/*
> + * Hardware uses CKO2 at 24MHz at several places. Set the parent clock of
> + * CKO2 to OSC.
> + */
> +#define CKO2_CONFIGURATION \
> +	clock-frequency = <24000000>; \
> +	clocks = <&clks IMX5_CLK_CKO2>; \
> +	assigned-clocks = <&clks IMX5_CLK_CKO2_SEL>, <&clks IMX5_CLK_OSC>; \
> +	assigned-clock-parents = <&clks IMX5_CLK_OSC>

This makes the DT harder to read.

Please place these explicitly where used.

> +
> +/ {
> +	model = "Freescale i.MX53 CPUV0 PPD rev6";
> +	compatible = "fsl,imx53-ppd", "fsl,imx53";
> +
> +	aliases {
> +		spi0 = &cspi;
> +		spi1 = &ecspi1;
> +		spi2 = &ecspi2;
> +	};
> +
> +	chosen {
> +		stdout-path = &uart1;
> +	};

It would be good if you could also define the rate, using an alias if
necessary.

> +
> +	memory {
> +		reg = <0x70000000 0x20000000>,
> +		      <0xb0000000 0x20000000>;
> +	};
> +
> +	clocks {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		cko2_11M: sgtl_clock_cko2 {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <11289600>;
> +		};
> +	};

Get rid of the clocks node, and place the clock directly under the root
node.

> +
> +	regulators {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		reg_sgtl5k: regulator@0 {
> +			compatible = "regulator-fixed";
> +			reg = <0>;
> +			regulator-name = "reg-sgtl5k";
> +			regulator-min-microvolt = <3300000>;
> +			regulator-max-microvolt = <3300000>;
> +			regulator-always-on;
> +		};

Likewise for these. Get rid of the regulators container node and place
these directly under the root node.

Please don't make fake up reg and unit-address values. Just suffix the
node name, e.g. regulator-sgtl5k.

> +&ecspi1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_ecspi1>;
> +	fsl,spi-num-chipselects = <4>;
> +	/* Fake this GPIO, this has special handling in userspace. */

NAK. The DT should reflect reality, and should not be specific to a
given userspace.

> +	cs-gpios = <&gpio5 1 GPIO_ACTIVE_LOW
> +		    &gpio4 10 GPIO_ACTIVE_LOW
> +		    &gpio4 11 GPIO_ACTIVE_LOW
> +		    &gpio4 12 GPIO_ACTIVE_LOW>;
> +	status = "okay";
> +
> +	spidev0: spi@0 {
> +		compatible = "spidev";
> +		reg = <0>;
> +		spi-max-frequency = <1000000>;
> +		status = "okay";
> +	};
> +
> +	spidev1: spi@1 {
> +		compatible = "spidev";
> +		reg = <1>;
> +		spi-max-frequency = <1000000>;
> +		status = "okay";
> +	};
> +
> +	spidev2: spi@2 {
> +		compatible = "spidev";
> +		reg = <2>;
> +		spi-max-frequency = <1000000>;
> +		status = "okay";
> +	};
> +
> +	spidev3: spi@3 {
> +		compatible = "spidev";
> +		reg = <3>;
> +		spi-max-frequency = <1000000>;
> +		status = "okay";
> +	};
> +};

What are these SPI devices?

Please fully describe them, with a complete compatible list.

Thanks,
Mark.

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

[PATCH 1/1] Add PPD device tree Fabien Lahoudere <fabien.lahoudere@collabora.co.uk> - 2016-08-05 15:40 +0200
  Re: [PATCH 1/1] Add PPD device tree Mark Rutland <mark.rutland@arm.com> - 2016-08-05 15:50 +0200

csiph-web