Date:   Tue, 15 Jan 2019 12:16:49 +0000
From:   Vokáč Michal <Michal.Vokac@...ft.com>
To:     Shawn Guo <shawnguo@...nel.org>
CC:     Rob Herring <robh+dt@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Andrew Lunn <andrew@...n.ch>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Fabio Estevam <fabio.estevam@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH shawn/for-next v3 2/2] ARM: dts: imx: Add Y Soft IOTA
 Draco, Hydra and Ursa boards

On 13. 01. 19 10:34, Shawn Guo wrote:
> On Thu, Jan 10, 2019 at 04:20:51PM +0000, Vokáč Michal wrote:
>> These are i.MX6S/DL based SBCs embedded in various Y Soft products.
>> All share the same board design but have slightly different HW
>> configuration.
>>
>> Ursa
>> - i.MX6S SoC, 512MB RAM DDR3, 4GB eMMC, microSD
>> - parallel WVGA 7" LCD with touch panel
>> - 1x Eth (QCA8334 switch)
>> - USB OTG
>> - USB host (micro-B)
>>
>> Draco
>> - i.MX6S SoC, 512MB RAM DDR3, 4GB eMMC, microSD
>> - parallel WVGA 7" LCD with touch panel
>> - 2x Eth (QCA8334 switch)
>> - USB OTG
>> - USB host (micro-B)
>> - RGB LED (I2C LP5562)
>> - 3.5mm audio jack + codec (LM49350)
>>
>> Hydra
>> - i.MX6DL SoC, 2GB RAM DDR3, 4GB eMMC, microSD
>> - I2C OLED display, capacitive matrix keys
>> - 2x Eth (QCA8334 switch)
>> - USB OTG
>> - RGB LED (I2C LP5562)
>> - 3.5mm audio jack + codec (LM49350)
>> - HDMI
>> - miniPCIe slot
>>
>> Cc: Andrew Lunn <andrew@...n.ch>
>> Signed-off-by: Michal Vokáč <michal.vokac@...ft.com>
>> ---
>> Changes since v2:
>>   - Remove cpus {/delete-node/ cpu@1;}; node from i.MX6Solo based board. (Fabio)
>>   - Use "device_type = "memory";" in memory nodes. (Fabio)
>>   - Use GPIO_ACTIVE_LOW for fec phy-reset-gpios. (Fabio)
>>   - Use "-" in node names. (Rob)
>>   - Remove label from led-controller node. (Rob)
>>
>> Changes since v1:
>>   - Enable HDMI on Hydra board.
>>   - Move regulators to the root node and remove simple-bus property. (Rob)
>>   - Remove reg and unit-address property from regulators. (Rob)
>>   - Use correct names for led-controller and pmic node. (Rob)
>>   - Use wakeup-source instead of deprecated enable-sdio-wakeup. (Shawn)
>>
>>   arch/arm/boot/dts/Makefile                 |   3 +
>>   arch/arm/boot/dts/imx6dl-yapp4-common.dtsi | 594 +++++++++++++++++++++++++++++
>>   arch/arm/boot/dts/imx6dl-yapp4-draco.dts   |  58 +++
>>   arch/arm/boot/dts/imx6dl-yapp4-hydra.dts   |  50 +++
>>   arch/arm/boot/dts/imx6dl-yapp4-ursa.dts    |  54 +++
>>   5 files changed, 759 insertions(+)
>>   create mode 100644 arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
>>   create mode 100644 arch/arm/boot/dts/imx6dl-yapp4-draco.dts
>>   create mode 100644 arch/arm/boot/dts/imx6dl-yapp4-hydra.dts
>>   create mode 100644 arch/arm/boot/dts/imx6dl-yapp4-ursa.dts
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index bd40148..c0a9b89 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -445,6 +445,9 @@ dtb-$(CONFIG_SOC_IMX6Q) += \
>>   	imx6dl-wandboard.dtb \
>>   	imx6dl-wandboard-revb1.dtb \
>>   	imx6dl-wandboard-revd1.dtb \
>> +	imx6dl-yapp4-draco.dtb \
>> +	imx6dl-yapp4-hydra.dtb \
>> +	imx6dl-yapp4-ursa.dtb \
>>   	imx6q-apalis-eval.dtb \
>>   	imx6q-apalis-ixora.dtb \
>>   	imx6q-apalis-ixora-v1.1.dtb \
>> diff --git a/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
>> new file mode 100644
>> index 0000000..48c2847
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/imx6dl-yapp4-common.dtsi
>> @@ -0,0 +1,594 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +//
>> +// Copyright (C) 2015-2018 Y Soft Corporation, a.s.
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/interrupt-controller/irq.h>
>> +#include <dt-bindings/pwm/pwm.h>
>> +
>> +/ {
>> +	backlight: backlight {
>> +		compatible = "pwm-backlight";
>> +		pwms = <&pwm1 0 500000 PWM_POLARITY_INVERTED>;
>> +		brightness-levels = <0 32 64 128 255>;
>> +		default-brightness-level = <32>;
>> +		num-interpolated-steps = <8>;
>> +		power-supply = <&sw2_reg>;
>> +		status = "disabled";
>> +	};
>> +
>> +	lcd_display: display {
>> +		compatible = "fsl,imx-parallel-display";
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +		interface-pix-fmt = "rgb24";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_ipu1>;
>> +		status = "disabled";
>> +
>> +		port@0 {
>> +			reg = <0>;
>> +
>> +			lcd_display_in: endpoint {
>> +				remote-endpoint = <&ipu1_di0_disp0>;
>> +			};
>> +		};
>> +
>> +		port@1 {
>> +			reg = <1>;
>> +
>> +			lcd_display_out: endpoint {
>> +				remote-endpoint = <&lcd_panel_in>;
>> +			};
>> +		};
>> +	};
>> +
>> +	panel: panel {
>> +		compatible = "dataimage,scf0700c48ggu18";
>> +		power-supply = <&sw2_reg>;
>> +		status = "disabled";
>> +
>> +		port {
>> +			lcd_panel_in: endpoint {
>> +				remote-endpoint = <&lcd_display_out>;
>> +			};
>> +		};
>> +	};
>> +
>> +	reg_usb_h1_vbus: reg-usb-h1-vbus {
> 
> Please spell out regulator in node name, and abbr. 'reg-' for label is
> all right.
> 
>> +		compatible = "regulator-fixed";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_usbh1_vbus>;
>> +		regulator-name = "usb_h1_vbus";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +		gpio = <&gpio1 29 GPIO_ACTIVE_HIGH>;
>> +		enable-active-high;
>> +		status = "disabled";
>> +	};
>> +
>> +	reg_usb_otg_vbus: reg-usb-otg-vbus {
>> +		compatible = "regulator-fixed";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_usbotg_vbus>;
>> +		regulator-name = "usb_otg_vbus";
>> +		regulator-min-microvolt = <5000000>;
>> +		regulator-max-microvolt = <5000000>;
>> +		gpio = <&gpio3 22 GPIO_ACTIVE_HIGH>;
>> +		enable-active-high;
>> +		status = "okay";
>> +	};
>> +
>> +	reg_pcie: reg-pcie {
>> +		compatible = "regulator-fixed";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_pcie_reg>;
>> +		regulator-name = "MPCIE_3V3";
>> +		regulator-min-microvolt = <3300000>;
>> +		regulator-max-microvolt = <3300000>;
>> +		gpio = <&gpio3 19 GPIO_ACTIVE_HIGH>;
>> +		enable-active-high;
>> +		status = "disabled";
>> +	};
>> +};
>> +
>> +&fec {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_enet>;
>> +	phy-mode = "rgmii-id";
>> +	phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
>> +	phy-reset-duration = <20>;
>> +	phy-supply = <&sw2_reg>;
>> +	phy-handle = <&ethphy0>;
>> +	status = "okay";
>> +
>> +	mdio {
>> +		#address-cells = <1>;
>> +		#size-cells = <0>;
>> +
>> +		phy_port2: phy@1 {
>> +			reg = <1>;
>> +		};
>> +
>> +		phy_port3: phy@2 {
>> +			reg = <2>;
>> +		};
>> +
>> +		switch@0 {
>> +			compatible = "qca,qca8334";
>> +			reg = <0>;
>> +
>> +			switch_ports: ports {
>> +				#address-cells = <1>;
>> +				#size-cells = <0>;
>> +
>> +				ethphy0: port@0 {
>> +					reg = <0>;
>> +					label = "cpu";
>> +					phy-mode = "rgmii";
>> +					ethernet = <&fec>;
> 
> Please have a newline between properties and child node.
> 
>> +					fixed-link {
>> +						speed = <1000>;
>> +						full-duplex;
>> +					};
>> +				};
>> +
>> +				port@2 {
>> +					reg = <2>;
>> +					label = "eth2";
>> +					phy-handle = <&phy_port2>;
>> +				};
>> +
>> +				port@3 {
>> +					reg = <3>;
>> +					label = "eth1";
>> +					phy-handle = <&phy_port3>;
>> +				};
>> +			};
>> +		};
>> +	};
>> +};
>> +
>> +&hdmi {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_hdmi_cec>;
>> +	ddc-i2c-bus = <&i2c2>;
>> +	status = "disabled";
>> +};
>> +
>> +&i2c2 {
>> +	clock-frequency = <100000>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c2>;
>> +	status = "okay";
>> +
>> +	eeprom@57 {
>> +		compatible = "atmel,24c128";
>> +		reg = <0x57>;
>> +		pagesize = <64>;
>> +		status = "okay";
>> +	};
>> +
>> +	leds: led-controller@30 {
> 
> Please sort child nodes of i2c bus in order of unit-address.
> 
>> +		compatible = "ti,lp5562";
>> +		reg = <0x30>;
>> +		clock-mode = /bits/ 8 <1>;
>> +		status = "disabled";
>> +
>> +		chan0 {
>> +			chan-name = "R";
>> +			led-cur = /bits/ 8 <0x20>;
>> +			max-cur = /bits/ 8 <0x60>;
>> +		};
>> +
>> +		chan1 {
>> +			chan-name = "G";
>> +			led-cur = /bits/ 8 <0x20>;
>> +			max-cur = /bits/ 8 <0x60>;
>> +		};
>> +
>> +		chan2 {
>> +			chan-name = "B";
>> +			led-cur = /bits/ 8 <0x20>;
>> +			max-cur = /bits/ 8 <0x60>;
>> +		};
>> +
>> +		chan3 {
>> +			chan-name = "W";
>> +			led-cur = /bits/ 8 <0x0>;
>> +			max-cur = /bits/ 8 <0x0>;
>> +		};
>> +	};
>> +
>> +	pmic@8 {
>> +		compatible = "fsl,pfuze200";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&pinctrl_pmic>;
>> +		reg = <0x8>;
>> +
>> +		regulators {
>> +			sw1a_reg: sw1ab {
>> +				regulator-min-microvolt = <300000>;
>> +				regulator-max-microvolt = <1875000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +				regulator-ramp-delay = <6250>;
>> +			};
>> +
>> +			sw2_reg: sw2 {
>> +				regulator-min-microvolt = <800000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			sw3a_reg: sw3a {
>> +				regulator-min-microvolt = <400000>;
>> +				regulator-max-microvolt = <1975000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			sw3b_reg: sw3b {
>> +				regulator-min-microvolt = <400000>;
>> +				regulator-max-microvolt = <1975000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			swbst_reg: swbst {
>> +				regulator-min-microvolt = <5000000>;
>> +				regulator-max-microvolt = <5150000>;
>> +			};
>> +
>> +			snvs_reg: vsnvs {
>> +				regulator-min-microvolt = <1000000>;
>> +				regulator-max-microvolt = <3000000>;
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			vref_reg: vrefddr {
>> +				regulator-boot-on;
>> +				regulator-always-on;
>> +			};
>> +
>> +			vgen1_reg: vgen1 {
>> +				regulator-min-microvolt = <800000>;
>> +				regulator-max-microvolt = <1550000>;
>> +			};
>> +
>> +			vgen2_reg: vgen2 {
>> +				regulator-min-microvolt = <800000>;
>> +				regulator-max-microvolt = <1550000>;
>> +			};
>> +
>> +			vgen3_reg: vgen3 {
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			vgen4_reg: vgen4 {
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			vgen5_reg: vgen5 {
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-always-on;
>> +			};
>> +
>> +			vgen6_reg: vgen6 {
>> +				regulator-min-microvolt = <1800000>;
>> +				regulator-max-microvolt = <3300000>;
>> +				regulator-always-on;
>> +			};
>> +		};
>> +	};
>> +
>> +	touchscreen: touchscreen@5c {
>> +		compatible = "pixcir,pixcir_tangoc";
>> +		reg = <0x5c>;
>> +		pinctrl-0 = <&pinctrl_touch>;
>> +		interrupt-parent = <&gpio4>;
>> +		interrupts = <5 IRQ_TYPE_EDGE_FALLING>;
>> +		attb-gpio = <&gpio4 5 GPIO_ACTIVE_HIGH>;
>> +		reset-gpio = <&gpio1 2 GPIO_ACTIVE_HIGH>;
>> +		touchscreen-size-x = <800>;
>> +		touchscreen-size-y = <480>;
>> +		status = "disabled";
>> +	};
>> +};
>> +
>> +&i2c3 {
>> +	clock-frequency = <100000>;
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&pinctrl_i2c3>;
>> +	status = "disabled";
>> +
>> +	gpio_oled: gpio@41 {
>> +		compatible = "nxp,pca9536";
>> +		gpio-controller;
>> +		#gpio-cells = <2>;
>> +		reg = <0x41>;
>> +		vcc-supply = <&sw2_reg>;
>> +		status = "disabled";
>> +	};
>> +
>> +	oled: oled@3d {
> 
> Sort child nodes in unit-address.
> 
>> +		compatible = "solomon,ssd1305fb-i2c";
>> +		reg = <0x3d>;
>> +		solomon,height = <64>;
>> +		solomon,width = <128>;
>> +		solomon,page-offset = <0>;
>> +		solomon,prechargep2 = <15>;
>> +		reset-gpios = <&gpio_oled 1 GPIO_ACTIVE_LOW>;
>> +		vbat-supply = <&sw2_reg>;
>> +		status = "disabled";
>> +	};
>> +};
>> +
>> +&iomuxc {
>> +	pinctrl_enet: enetgrp {
>> +		fsl,pins = <
>> +			MX6QDL_PAD_ENET_MDIO__ENET_MDIO		0x1b020
>> +			MX6QDL_PAD_ENET_MDC__ENET_MDC		0x1b020
>> +			MX6QDL_PAD_RGMII_TXC__RGMII_TXC		0x1b020
>> +			MX6QDL_PAD_RGMII_TD0__RGMII_TD0		0x1b020
>> +			MX6QDL_PAD_RGMII_TD1__RGMII_TD1		0x1b020
>> +			MX6QDL_PAD_RGMII_TD2__RGMII_TD2		0x1b020
>> +			MX6QDL_PAD_RGMII_TD3__RGMII_TD3		0x1b020
>> +			MX6QDL_PAD_RGMII_TX_CTL__RGMII_TX_CTL	0x1b020
>> +			MX6QDL_PAD_RGMII_RXC__RGMII_RXC		0x1b020
>> +			MX6QDL_PAD_RGMII_RD0__RGMII_RD0		0x1b020
>> +			MX6QDL_PAD_RGMII_RD1__RGMII_RD1		0x1b020
>> +			MX6QDL_PAD_RGMII_RD2__RGMII_RD2		0x1b020
>> +			MX6QDL_PAD_RGMII_RD3__RGMII_RD3		0x1b020
>> +			MX6QDL_PAD_RGMII_RX_CTL__RGMII_RX_CTL	0x1b020
>> +			MX6QDL_PAD_ENET_REF_CLK__ENET_TX_CLK	0x1b010
>> +			MX6QDL_PAD_GPIO_16__ENET_REF_CLK	0x1b010
>> +			MX6QDL_PAD_ENET_CRS_DV__GPIO1_IO25	0x1b098
>> +		>;
>> +	};
>> +
>> +	pinctrl_hdmi_cec: hdmicecgrp {
>> +		fsl,pins = <
>> +			MX6QDL_PAD_EIM_A25__HDMI_TX_CEC_LINE	0x1b898
>> +		>;
>> +	};
>> +
>> +	pinctrl_i2c2: i2c2grp {
>> +		fsl,pins = <
>> +			MX6QDL_PAD_KEY_COL3__I2C2_SCL	0x4001b899
>> +			MX6QDL_PAD_KEY_ROW3__I2C2_SDA	0x4001b899
>> +		>;
>> +	};
>> +
>> +	pinctrl_i2c3: i2c3grp {
>> +		fsl,pins = <
>> +			MX6QDL_PAD_GPIO_3__I2C3_SCL	0x4001b899
>> +			MX6QDL_PAD_GPIO_6__I2C3_SDA	0x4001b899
>> +		>;
>> +	};
>> +
>> +	pinctrl_ipu1: ipu1grp {
>> +		fsl,pins = <
>> +			MX6QDL_PAD_DI0_DISP_CLK__IPU1_DI0_DISP_CLK	0x10
>> +			MX6QDL_PAD_DI0_PIN2__IPU1_DI0_PIN02		0x10
>> +			MX6QDL_PAD_DI0_PIN3__IPU1_DI0_PIN03		0x10
>> +			MX6QDL_PAD_DISP0_DAT0__IPU1_DISP0_DATA00	0x10
>> +			MX6QDL_PAD_DISP0_DAT1__IPU1_DISP0_DATA01	0x10
>> +			MX6QDL_PAD_DISP0_DAT2__IPU1_DISP0_DATA02	0x10
>> +			MX6QDL_PAD_DISP0_DAT3__IPU1_DISP0_DATA03	0x10
>> +			MX6QDL_PAD_DISP0_DAT4__IPU1_DISP0_DATA04	0x10
>> +			MX6QDL_PAD_DISP0_DAT5__IPU1_DISP0_DATA05	0x10
>> +			MX6QDL_PAD_DISP0_DAT6__IPU1_DISP0_DATA06	0x10
>> +			MX6QDL_PAD_DISP0_DAT7__IPU1_DISP0_DATA07	0x10
>> +			MX6QDL_PAD_DISP0_DAT8__IPU1_DISP0_DATA08	0x10
>> +			MX6QDL_PAD_DISP0_DAT9__IPU1_DISP0_DATA09	0x10
>> +			MX6QDL_PAD_DISP0_DAT10__IPU1_DISP0_DATA10	0x10
>> +			MX6QDL_PAD_DISP0_DAT11__IPU1_DISP0_DATA11	0x10
>> +			MX6QDL_PAD_DISP0_DAT12__IPU1_DISP0_DATA12	0x10
>> +			MX6QDL_PAD_DISP0_DAT13__IPU1_DISP0_DATA13	0x10
>> +			MX6QDL_PAD_DISP0_DAT14__IPU1_DISP0_DATA14	0x10
>> +			MX6QDL_PAD_DISP0_DAT15__IPU1_DISP0_DATA15	0x10
>> +			MX6QDL_PAD_DISP0_DAT16__IPU1_DISP0_DATA16	0x10
>> +			MX6QDL_PAD_DISP0_DAT17__IPU1_DISP0_DATA17	0x10
>> +			MX6QDL_PAD_DISP0_DAT18__IPU1_DISP0_DATA18	0x10
>> +			MX6QDL_PAD_DISP0_DAT19__IPU1_DISP0_DATA19	0x10
>> +			MX6QDL_PAD_DISP0_DAT20__IPU1_DISP0_DATA20	0x10
>> +			MX6QDL_PAD_DISP0_DAT21__IPU1_DISP0_DATA21	0x10
>> +			MX6QDL_PAD_DISP0_DAT22__IPU1_DISP0_DATA22	0x10
>> +			MX6QDL_PAD_DISP0_DAT23__IPU1_DISP0_DATA23	0x10
>> +		>;
>> +	};
>> +
>> +	pinctrl_pcie: pciegrp {
>> +		fsl,pins = <
>> +			MX6QDL_PAD_GPIO_17__GPIO7_IO12		0x1b098
>> +			MX6QDL_PAD_KEY_COL4__GPIO4_IO14		0x1b098
>> +			MX6QDL_PAD_CSI0_DATA_EN__GPIO5_IO20	0x1b098
>> +		>;
>> +	};
>> +
>> +	pinctrl_pcie_reg: pciereggrp {
>> +		fsl,pins = <
>> +			MX6QDL_PAD_EIM_D19__GPIO3_IO19	0x1b098
>> +		>;
>> +	};
>> +
>> +	pinctrl_pmic: pmicgrp {
>> +		fsl,pins = <
>> +			MX6QDL_PAD_GPIO_18__GPIO7_IO13	0x1b098
>> +		>;
>> +	};
>> +
>> +	pinctrl_backlight_pwm: pwm1grp {
> 
> Name the label in a way consistent with node name, i.e.
> pinctrl_pwm_xxx, and sort it alphabetically.

Thank you Shawn. I will fix all the issues.
I am amazed how you, reviewers/maintainers, can always find something to improve!

Michal

Powered by blists - more mailing lists