Re: [PATCH] ARM: dts: add support for Ka-Ro TX51

From: Lothar WaÃmann
Date: Mon Jun 23 2014 - 06:20:52 EST

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