From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753815AbcHQHeb (ORCPT <rfc822;w@1wt.eu>);
	Wed, 17 Aug 2016 03:34:31 -0400
Received: from bhuna.collabora.co.uk ([46.235.227.227]:53964 "EHLO
	bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750903AbcHQHe3 (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 17 Aug 2016 03:34:29 -0400
From: Fabien Lahoudere <fabien.lahoudere@collabora.co.uk>
Subject: Re: [PATCH 1/1] Add PPD device tree
To: Mark Rutland <mark.rutland@arm.com>
References: <1470403887-15190-1-git-send-email-fabien.lahoudere@collabora.co.uk>
 <20160805133954.GC25152@leverpostej>
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>
Message-ID: <f01a11ec-c5fe-8bde-ef3a-bdf240c5cc5b@collabora.co.uk>
Date: Wed, 17 Aug 2016 09:34:25 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.2.0
MIME-Version: 1.0
In-Reply-To: <20160805133954.GC25152@leverpostej>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello,

Thanks for your comments

On 05/08/2016 15:39, Mark Rutland wrote:
> 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.
>

Ok for all above.

>> +&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.
>
I understand what you mean and I agree but spidev0 and spidev1 
communicate with another micro with an OS (cortex M4) so it is ok if I 
add 'compatible = "fsl,mk20fn1m0vmd12"' in the dts and in spidev.c ?

The 2 other devices are EXAR xra1403 (gpio controller).

-- 
Fabien Lahoudere
Collabora Ltd.