lkml.org 
[lkml]   [2016]   [Aug]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 1/1] Add PPD device tree
Date
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.

\
 
 \ /
  Last update: 2016-09-17 09:57    [W:0.042 / U:0.100 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site