Hi,

Shawn Guo wrote:
> On Thu, Jun 12, 2014 at 03:09:44PM +0200, Lothar Waßmann wrote:
> > Add support for Ka-Ro electronics i.MX51 based TX51 modules
> > 
> > Signed-off-by: Lothar Waßmann <l...@karo-electronics.de>
> > ---
> >  arch/arm/boot/dts/Makefile       |    1 +
> >  arch/arm/boot/dts/imx51-tx51.dts |  620 
> > ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 621 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/imx51-tx51.dts
> > 
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index 0f1e8be..8dd4dbc 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -177,6 +177,7 @@ dtb-$(CONFIG_ARCH_MXC) += \
> >     imx51-babbage.dtb \
> >     imx51-digi-connectcore-jsk.dtb \
> >     imx51-eukrea-mbimxsd51-baseboard.dtb \
> > +   imx51-tx51.dtb \
> >     imx53-ard.dtb \
> >     imx53-m53evk.dtb \
> >     imx53-mba53.dtb \
> > diff --git a/arch/arm/boot/dts/imx51-tx51.dts 
> > b/arch/arm/boot/dts/imx51-tx51.dts
> > new file mode 100644
> > index 0000000..9ae7758
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/imx51-tx51.dts
> > @@ -0,0 +1,620 @@
> > +/*
> > + * Copyright 2012-2014 Lothar Waßmann <l...@karo-electronics.de>
> > + *
> > + * The code contained herein is licensed under the GNU General Public
> > + * License. You may obtain a copy of the GNU General Public License
> > + * Version 2 at the following locations:
> > + *
> > + * http://www.opensource.org/licenses/gpl-license.html
> > + * http://www.gnu.org/copyleft/gpl.html
> > + */
> > +
> > +/dts-v1/;
> > +#include "imx51.dtsi"
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/input/input.h>
> 
> imx51.dtsi already includes them.
> 
OK.

> > +#include <dt-bindings/pwm/pwm.h>
> > +
> > +/ {
> > +   model = "Ka-Ro electronics TX51 module";
> > +   compatible = "karo,tx51", "fsl,imx51";
> > +
> > +   aliases {
> > +           backlight = &backlight;
> > +           display = &display;
> > +           i2c1 = &i2c_gpio;
> > +           usbotg = &usbotg;
> > +   };
> > +
> > +   chosen {
> > +           stdout-path = &uart1;
> > +   };
> > +
> > +   backlight: pwm-backlight {
> > +           compatible = "pwm-backlight";
> > +
> 
> Drop this new line.
> 
OK.

> > +           pwms = <&pwm1 0 500000 PWM_POLARITY_INVERTED>;
> > +           power-supply = <&reg_3v3>;
> > +           brightness-levels = <
> > +                     0  1  2  3  4  5  6  7  8  9
> > +                    10 11 12 13 14 15 16 17 18 19
> > +                    20 21 22 23 24 25 26 27 28 29
> > +                    30 31 32 33 34 35 36 37 38 39
> > +                    40 41 42 43 44 45 46 47 48 49
> > +                    50 51 52 53 54 55 56 57 58 59
> > +                    60 61 62 63 64 65 66 67 68 69
> > +                    70 71 72 73 74 75 76 77 78 79
> > +                    80 81 82 83 84 85 86 87 88 89
> > +                    90 91 92 93 94 95 96 97 98 99
> > +                   100
> > +           >;
> > +           default-brightness-level = <50>;
> > +   };
> > +
> > +   clocks {
> > +           ckih1 {
> > +                   clock-frequency = <0>;
> > +           };
> > +
> > +           mclk: clock@0 {
> > +                   compatible = "fixed-clock";
> > +                   reg = <0>;
> > +                   #clock-cells = <0>;
> > +                   clock-frequency = <27000000>;
> > +           };
> > +
> > +           clk_26M: clock@1 {
> 
> When using generic name, you will need clock-output-names property.
> Otherwise, the clocks will fail in registration except the first one,
> because of clock name collision.
> 
Didn't recognize that yet.

[...]
> > +   usbphy {
> > +           #address-cells = <1>;
> > +           #size-cells = <0>;
> > +           compatible = "simple-bus";
> > +
> > +           usbh1phy: usbh1phy@0 {
> 
> The node name should be something generic like usbphy?
> 
OK.

> > +                   compatible = "usb-nop-xceiv";
> > +                   reg = <0>;
> > +                   clocks = <&clk_26M>;
> > +                   clock-names = "main_clk";
> > +           };
> > +   };
> > +};
> > +
> > +&audmux {
> > +   status = "okay";
> > +};
> > +
> > +&fec {
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <&pinctrl_fec>;
> > +   phy-mode = "mii";
> > +// phy-reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>;
> 
> Drop it?
> 
Leftover from debugging...

> > +   phy-handle = <&phy0>;
> > +   mac-address = [000000000000]; /* will be set by U-Boot */
> 
> Shouldn't it be local-mac-address?
> 
probably yes, but both 'mac-address' and 'local-mac-address' are being
set up by U-Boot anyway.

[...]
> > +&ipu_di0_disp0 {
> > +   remote-endpoint = <&display0_in>;
> > +};
> > +
> > +&kpp {
> > +   status = "okay";
> 
> Put 'status' at the bottom of property list.
>
OK.

> > +
> > +   linux,keymap = <        /* sample keymap */
> > +           MATRIX_KEY(0, 0, KEY_POWER)
> > +           MATRIX_KEY(0, 1, KEY_KP0)
> > +           MATRIX_KEY(0, 2, KEY_KP1)
> > +           MATRIX_KEY(0, 3, KEY_KP2)
> > +           MATRIX_KEY(1, 0, KEY_KP3)
> > +           MATRIX_KEY(1, 1, KEY_KP4)
> > +           MATRIX_KEY(1, 2, KEY_KP5)
> > +           MATRIX_KEY(1, 3, KEY_KP6)
> > +           MATRIX_KEY(2, 0, KEY_KP7)
> > +           MATRIX_KEY(2, 1, KEY_KP8)
> > +           MATRIX_KEY(2, 2, KEY_KP9)
> > +   >;
> > +};
> > +
> > +&esdhc1 {
> > +   cd-gpios = <&gpio3 8 GPIO_ACTIVE_LOW>;
> > +   fsl,wp-controller;
> 
> Does it work for you, since the driver does not support it as of today?
> 
What driver doesn't support what?
AFAICT the sdhci-esdhc-imx.c driver supports both of these properties.

> > +   status = "okay";
> > +};
> > +
> > +&esdhc2 {
> > +   cd-gpios = <&gpio3 6 GPIO_ACTIVE_LOW>;
> > +   fsl,wp-controller;
> > +   status = "okay";
> > +};
> > +
> > +&pwm1 {
> > +   pinctrl-names = "default";
> > +   pinctrl-0 = <&pinctrl_pwm1>;
> > +};
> > +
> > +&ecspi1 {
> > +   fsl,spi-num-chipselects = <2>;
> > +   cs-gpios = <&gpio4 24 GPIO_ACTIVE_LOW &gpio4 25 GPIO_ACTIVE_LOW>;
> 
> More readable to write it like below?
> 
>       cs-gpios = <&gpio4 24 GPIO_ACTIVE_LOW>,
>                  <&gpio4 25 GPIO_ACTIVE_LOW>;
> 
OK.

> > +   status = "okay";
> > +
> > +   spidev0: spi@0 {
> > +           compatible = "spidev";
> > +           reg = <0>;
> > +           spi-max-frequency = <54000000>;
> > +   };
> > +
> > +   spidev1: spi@1 {
> > +           compatible = "spidev";
> > +           reg = <1>;
> > +           spi-max-frequency = <54000000>;
> > +   };
> 
> I'm not sure we should have these two devices.
> 
Why not? With this the SPI bus can readily be used with the spidev
driver from Documentation/spi/spidev_test.c (which is what most of our
customers are asking for)!

> > +};
> > +
> > +&ssi1 {
> > +   fsl,mode = "i2s-slave";
> > +   codec-handle = <&sgtl5000>;
> > +   status = "okay";
> > +};
> > +
> > +&ssi2 {
> > +   status = "disabled";
> > +};
> 
> Why is this needed, since the device is disabled in imx51.dtsi?
> 
That's included here as a placeholder in case someone wants to enable
the second SSI interface so that they...

> > +&nfc {
> 
> Please sort the nodes alphabetically.
> 
...won't have this problem. ;)


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | i...@karo-electronics.de
___________________________________________________________
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Reply via email to