| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 23 Jul 2014 17:45:57 +0200
From: Sebastian Hesselbarth <sebastian.hesselbarth@...il.com>
To: Jason Cooper <jason@...edaemon.net>,
benoitm974 <yahoo@...enite.com>
CC: benoitm@...enite.com, Rob Herring <robh+dt@...nel.org>,
Pawel Moll <pawel.moll@....com>,
Ian Campbell <ijc+devicetree@...lion.org.uk>,
Kumar Gala <galak@...eaurora.org>,
Russell King <linux@....linux.org.uk>,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, Andrew Lunn <andrew@...n.ch>,
Gregory CLEMENT <gregory.clement@...e-electrons.com>,
Sebastian Hesselbarth <sebastian.hesselbarth@...glemail.com>
Subject: Re: [PATCH 1/2] Added dts defintion for Lenovo ix4-300d nas
On 07/23/2014 03:45 PM, Jason Cooper wrote:
> Thanks for the patch! A few minor things:
Benoit,
I also have some minor remarks just because I stumble over them
all the time.
>> ---
>> arch/arm/boot/dts/Makefile | 1 +
>> arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts | 267 +++++++++++++++++++++++++
I looked it up and Lenovo names it "ix4-300d", maybe you should
rename the dts "armada-xp-lenovo-ix4-300d.dts" then.
[...]
>> diff --git a/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts b/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts
>> new file mode 100644
>> index 0000000..e04e7a6
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/armada-xp-lenovo-ix4300d.dts
>> @@ -0,0 +1,267 @@
>> +/*
>> + * Device Tree file for LENOVO IX4-300d
It is spelled "Lenovo" and the full name is "Lenovo Iomega ix4-300d".
As it is the initial commit, we should be more sensitive on the case
here.
>> + * Copyright (C) 2014, Benoit Masson <yahoo@...enite.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + */
>> +
>> +/dts-v1/;
>> +
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include "armada-xp-mv78230.dtsi"
>> +
>> +/ {
>> + model = "LENOVO IX4-300d";
Same comment about "Lenovo Iomega ix4-300d"
>> + compatible = "lenovo,ix4-300d", "marvell,armadaxp-mv78230", "marvell,armadaxp", "marvell,armada-370-xp";
>> +
>> + chosen {
>> + bootargs = "console=ttyS0,115200 earlyprintk";
Consider to add
stdout-path = "/soc/internal-regs/serial@...00";
That way you would not have to fix it up for e.g. Barebox.
Unfortunately, neither Armada 370 nor XP dtsi define node
labels yet, so you have to put the full node path.
>> + };
>> +
[...]
>> + soc {
>> + ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xd0000000 0x100000
>> + MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000>;
>> +
>> + pcie-controller {
>> + status = "okay";
>> +
>> + pcie@1,0 {
>> + /* Port 0, Lane 0 */
>> + status = "okay";
>> + };
>> + pcie@5,0 {
>> + /* Port 1, Lane 0 */
>> + status = "okay";
>> + };
>> +
Any chance you know what is connected to above PCIe ports?
If so, put a comment above the node itself naming the attached
device, e.g.
/* USB 3.0 xHCI controller */
pcie@1,0 {...}
Also, I saw you have a good impression about the GPIOs used on
the ix4-300d. If you also know the PERST# GPIOs, add them with
reset-gpios = <&gpioN M GPIO_ACTIVE_LOW>;
where N, M follow gpio# = (32 * N) + M, i.e. N=1, M=22 for gpio54.
>> + };
>> +
>> + internal-regs {
>> + pinctrl {
>> + poweroff: poweroff {
nit: poweroff_pin: poweroff-pin {...}
like the ones below?
>> + marvell,pins = "mpp24";
>> + marvell,function = "gpio";
>> + };
>> +
>> + power_button_pin: power-button-pin {
>> + marvell,pins = "mpp44";
>> + marvell,function = "gpio";
>> + };
>> +
>> + reset_button_pin: reset-button-pin {
>> + marvell,pins = "mpp45";
>> + marvell,function = "gpio";
>> + };
>> + select_button_pin: select-button-pin {
>> + marvell,pins = "mpp41";
>> + marvell,function = "gpio";
>> + };
>> +
>> + scroll_button_pin: scroll-button-pin {
>> + marvell,pins = "mpp42";
>> + marvell,function = "gpio";
>> + };
>> + hdd_led_pin: hdd-led-pin {
>> + marvell,pins = "mpp26";
>> + marvell,function = "gpio";
>> + };
[...]
>> + };
>> +
>> + serial@...00 {
>> + clocks = <&coreclk 0>;
As Andrew already said, no need to duplicate the clocks property.
>> + status = "okay";
>> + };
[...]
>> + spi3 {
>> + compatible = "spi-gpio";
>> + status = "okay";
>> + gpio-sck = <&gpio0 25 0>;
>> + gpio-mosi = <&gpio1 15 0>; /*gpio 47*/
>> + cs-gpios = <&gpio0 27 0 >;
[...]
>> + num-chipselects = <1>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + gpio2: gpio2@0 {
>> + compatible = "fairchild,74hc595";
Freaky we have a driver for a 74x595 8-bit shift register ;)
>> + gpio-controller;
>> + #gpio-cells = <2>;
>> + reg = <0>;
>> + registers-number = <2>;
>> + spi-max-frequency = <100000>;
>> + };
>> + };
>> +
>> + gpio-leds {
>> + compatible = "gpio-leds";
>> + pinctrl-0 = <&hdd_led_pin>;
>> + pinctrl-names = "default";
>> +
>> + hdd-led {
>> + label = "ix4300d:blue:hdd";
Can you get rid of "ix4300d:" or rename it to "ix4-300d:" without
breaking any userspace stuff? If so, please update all LEDs.
>> + gpios = <&gpio0 26 GPIO_ACTIVE_HIGH>;
>> + default-state = "off";
>> + };
[...]
>> + /* warning: you need both eth1 & 0 to be initialize for poweroff to shutdown otherwise it reboots */
I don't get the comment.
> This is a great first version
Yup, Thanks!
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists