* [PATCH v4 0/2] Add support for the Purism Librem5 devkit
@ 2019-04-15 23:53 Angus Ainslie (Purism)
2019-04-15 23:53 ` [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the " Angus Ainslie (Purism)
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Angus Ainslie (Purism) @ 2019-04-15 23:53 UTC (permalink / raw)
Cc: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team,
Angus Ainslie (Purism), devicetree, linux-arm-kernel,
linux-kernel
The Librem5 devkit is based on the imx8mq from NXP. This is a default
devicetree to boot the board to a command prompt.
Changes since v3:
Freshly sorted and pressed nodes.
Change the backlight to an interpolated scale.
Dropped i2c2.
Dropped devkit version number to match debian MR.
Changes since v2:
Fixed incorrect phy-supply for the fsl-fec.
Dropped unused regulator property.
Fixup Makefile for linux-next.
Changes since v1:
Dropped config file.
Updated the board compatible label.
Changed node names to follow naming conventions.
Added a more complete regulator hierachy.
Removed unused nodes.
Removed unknown devices.
Fixed comment style.
Dropped undocumented properties.
Angus Ainslie (Purism) (2):
arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit
dt-bindings: Add an entry for Purism SPC
.../devicetree/bindings/vendor-prefixes.txt | 1 +
arch/arm64/boot/dts/freescale/Makefile | 1 +
.../dts/freescale/imx8mq-librem5-devkit.dts | 822 ++++++++++++++++++
3 files changed, 824 insertions(+)
create mode 100644 arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit 2019-04-15 23:53 [PATCH v4 0/2] Add support for the Purism Librem5 devkit Angus Ainslie (Purism) @ 2019-04-15 23:53 ` Angus Ainslie (Purism) 2019-04-16 7:54 ` Marco Felsch 2019-04-15 23:53 ` [PATCH v4 2/2] dt-bindings: Add an entry for Purism SPC Angus Ainslie (Purism) 2019-04-16 7:58 ` [PATCH v4 0/2] Add support for the Purism Librem5 devkit Marco Felsch 2 siblings, 1 reply; 11+ messages in thread From: Angus Ainslie (Purism) @ 2019-04-15 23:53 UTC (permalink / raw) Cc: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Angus Ainslie (Purism), devicetree, linux-arm-kernel, linux-kernel This is the development kit board for the Librem 5. The current level of support yields a working console and is able to boot userspace from the Network or eMMC. Additional subsystems that are active : - Both USB ports - SD card socket - WiFi usdhc - WWAN modem - GNSS - GPIO keys - LEDs - gyro - magnetometer - touchscreen - pwm - backlight - haptic motor Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> --- arch/arm64/boot/dts/freescale/Makefile | 1 + .../dts/freescale/imx8mq-librem5-devkit.dts | 822 ++++++++++++++++++ 2 files changed, 823 insertions(+) create mode 100644 arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile index 0bd122f60549..c043aca66572 100644 --- a/arch/arm64/boot/dts/freescale/Makefile +++ b/arch/arm64/boot/dts/freescale/Makefile @@ -22,6 +22,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb +dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts new file mode 100644 index 000000000000..dd705b6b94a4 --- /dev/null +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts @@ -0,0 +1,822 @@ +/* SPDX-License-Identifier: GPL-2.0+ + * + * Copyright 2018-2019 Purism SPC + */ + +/dts-v1/; + +#include "imx8mq.dtsi" +#include "dt-bindings/usb/pd.h" +#include "dt-bindings/input/input.h" + +/ { + model = "Purism Librem 5 devkit"; + compatible = "purism,librem5-devkit", "fsl,imx8mq"; + + backlight_dsi: backlight-dsi { + compatible = "pwm-backlight"; + /* 200 Hz for the PAM2841 */ + pwms = <&pwm1 0 5000000>; + brightness-levels = <0 100>; + num-interpolated-steps = <100>; + /* Default brightness level (index into the array defined by */ + /* the "brightness-levels" property) */ + default-brightness-level = <0>; + power-supply = <®_22V4_P>; + }; + + chosen { + stdout-path = &uart1; + }; + + gpio-keys { + compatible = "gpio-keys"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_gpio_keys>; + + btn1 { + label = "VOL_UP"; + gpios = <&gpio4 21 GPIO_ACTIVE_LOW>; + gpio-key,wakeup; + linux,code = <KEY_VOLUMEUP>; + }; + + btn2 { + label = "VOL_DOWN"; + gpios = <&gpio4 22 GPIO_ACTIVE_LOW>; + gpio-key,wakeup; + linux,code = <KEY_VOLUMEDOWN>; + }; + + hp_det { + label = "HP_DET"; + gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; + gpio-key,wakeup; + linux,code = <KEY_HP>; + }; + }; + + leds { + compatible = "gpio-leds"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_gpio_leds>; + status = "okay"; + + led1 { + label = "LED 1"; + gpios = <&gpio1 13 GPIO_ACTIVE_HIGH>; + default-state = "off"; + }; + }; + + pmic_osc: pmic-osc { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <32768>; + clock-output-names = "pmic_osc"; + }; + + pwmleds { + compatible = "pwm-leds"; + haptic { + label = "librem5::haptic"; + pwms = <&pwm2 0 200000>; + active-low; + max-brightness = <255>; + power-supply = <®_3V3_P>; + }; + }; + + reg_1V8_P: regulator-1V8-P { + compatible = "regulator-fixed"; + regulator-name = "1V8_P"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <1800000>; + vin-supply = <®_vbat>; + en-supply = <®_pwr_en>; + }; + + reg_2V8_P: regulator-2V8-P { + compatible = "regulator-fixed"; + regulator-name = "2V8_P"; + regulator-min-microvolt = <2800000>; + regulator-max-microvolt = <2800000>; + vin-supply = <®_3V3_P>; + }; + + reg_3V3_P: regulator-3V3-P { + compatible = "regulator-fixed"; + regulator-name = "3V3_P"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + vin-supply = <®_vbat>; + en-supply = <®_pwr_en>; + }; + + reg_5V_P: regulator-5V-P { + /* Supplies HDMI, USB hub and smartcard */ + compatible = "regulator-fixed"; + regulator-name = "5V_P"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + vin-supply = <®_vbat>; + en-supply = <®_pwr_en>; + }; + + reg_5V_SOM: regulator-5V-SOM { + compatible = "regulator-fixed"; + regulator-name = "5V_SOM"; + regulator-min-microvolt = <5000000>; + regulator-max-microvolt = <5000000>; + vin-supply = <®_vbat>; + regulator-always-on; + }; + + reg_22V4_P: regulator-22V4-P { + compatible = "regulator-fixed"; + regulator-name = "22V4_P"; + regulator-min-microvolt = <22400000>; + regulator-max-microvolt = <22400000>; + vin-supply = <®_vbat>; + }; + + reg_pwr_en: regulator-pwr-en { + compatible = "regulator-fixed"; + regulator-name = "PWR_EN"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + gpio = <&gpio1 8 GPIO_ACTIVE_HIGH>; + enable-active-high; + regulator-always-on; + }; + + reg_usdhc2_vmmc: regulator-usdhc2-vmmc { + compatible = "regulator-fixed"; + regulator-name = "VSD_3V3"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>; + enable-active-high; + regulator-always-on; + }; + + reg_vbat: regulator-vbat { + compatible = "regulator-fixed"; + regulator-name = "VBAT_REG"; + regulator-always-on; + }; + + wifi_pwr_en: wifi-en { + compatible = "regulator-fixed"; + regulator-name = "WIFI_EN"; + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + gpio = <&gpio3 5 GPIO_ACTIVE_HIGH>; + enable-active-high; + regulator-always-on; + }; +}; + + +&clk { + assigned-clocks = <&clk IMX8MQ_AUDIO_PLL1>, <&clk IMX8MQ_AUDIO_PLL2>; + assigned-clock-rates = <786432000>, <722534400>; +}; + +&fec1 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_fec1>; + phy-mode = "rgmii-id"; + phy-handle = <ðphy0>; + fsl,magic-packet; + status = "okay"; + phy-supply = <®_3V3_P>; + + mdio { + #address-cells = <1>; + #size-cells = <0>; + + ethphy0: ethernet-phy@1 { + compatible = "ethernet-phy-ieee802.3-c22"; + reg = <1>; + }; + }; +}; + +&i2c1 { + clock-frequency = <400000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c1>; + status = "okay"; + + pmic: pmic@4b { + reg = <0x4b>; + compatible = "rohm,bd71837"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_pmic>; + clocks = <&pmic_osc>; + clock-names = "osc"; + clock-output-names = "pmic_clk"; + interrupt-parent = <&gpio1>; + interrupts = <3 GPIO_ACTIVE_LOW>; + interrupt-names = "irq"; + rohm,reset-snvs-powered; + + regulators { + #address-cells = <1>; + #size-cells = <0>; + + buck1_reg: BUCK1@0 { + reg = <0>; + regulator-name = "buck1"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1300000>; + regulator-boot-on; + regulator-always-on; + regulator-ramp-delay = <1250>; + rohm,dvs-run-voltage = <900000>; + rohm,dvs-idle-voltage = <850000>; + rohm,dvs-suspend-voltage = <800000>; + }; + + buck2_reg: BUCK2@1 { + reg = <1>; + regulator-name = "buck2"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1300000>; + regulator-boot-on; + regulator-always-on; + regulator-ramp-delay = <1250>; + rohm,dvs-run-voltage = <1000000>; + rohm,dvs-idle-voltage = <900000>; + }; + + buck3_reg: BUCK3@2 { + reg = <2>; + regulator-name = "buck3"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1300000>; + rohm,dvs-run-voltage = <1000000>; + }; + + buck4_reg: BUCK4@3 { + reg = <3>; + regulator-name = "buck4"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1300000>; + rohm,dvs-run-voltage = <1000000>; + }; + + buck5_reg: BUCK5@4 { + reg = <4>; + regulator-name = "buck5"; + regulator-min-microvolt = <700000>; + regulator-max-microvolt = <1350000>; + regulator-boot-on; + regulator-always-on; + }; + + buck6_reg: BUCK6@5 { + reg = <5>; + regulator-name = "buck6"; + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3300000>; + regulator-boot-on; + regulator-always-on; + }; + + buck7_reg: BUCK7@6 { + reg = <6>; + regulator-name = "buck7"; + regulator-min-microvolt = <1605000>; + regulator-max-microvolt = <1995000>; + regulator-boot-on; + regulator-always-on; + }; + + buck8_reg: BUCK8@7 { + reg = <7>; + regulator-name = "buck8"; + regulator-min-microvolt = <800000>; + regulator-max-microvolt = <1400000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo1_reg: LDO1@8 { + reg = <8>; + regulator-name = "ldo1"; + regulator-min-microvolt = <3000000>; + regulator-max-microvolt = <3300000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo2_reg: LDO2@9 { + reg = <9>; + regulator-name = "ldo2"; + regulator-min-microvolt = <900000>; + regulator-max-microvolt = <900000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo3_reg: LDO3@10 { + reg = <10>; + regulator-name = "ldo3"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo4_reg: LDO4@11 { + reg = <11>; + regulator-name = "ldo4"; + regulator-min-microvolt = <900000>; + regulator-max-microvolt = <1800000>; + regulator-boot-on; + regulator-always-on; + }; + + ldo5_reg: LDO5@12 { + reg = <12>; + regulator-name = "ldo5"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + }; + + ldo6_reg: LDO6@13 { + reg = <13>; + regulator-name = "ldo6"; + regulator-min-microvolt = <900000>; + regulator-max-microvolt = <1800000>; + }; + + ldo7_reg: LDO7@14 { + reg = <14>; + regulator-name = "ldo7"; + regulator-min-microvolt = <1800000>; + regulator-max-microvolt = <3300000>; + }; + }; + }; + + typec_ptn5100: usb_typec@52 { + compatible = "nxp,ptn5110"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_typec>; + reg = <0x52>; + interrupt-parent = <&gpio3>; + interrupts = <1 IRQ_TYPE_LEVEL_LOW>; + vconn-supply = <®_5V_P>; + usb_con: connector { + compatible = "usb-c-connector"; + label = "USB-C"; + data-role = "dual"; + power-role = "dual"; + try-power-role = "sink"; + source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP )>; + sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP ) + PDO_VAR(5000, 12000, 2000)>; + op-sink-microwatt = <10000000>; + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + usb_con_hs: endpoint { + remote-endpoint = <&typec_hs>; + }; + }; + port@1 { + reg = <1>; + usb_con_ss: endpoint { + remote-endpoint = <&typec_ss>; + }; + }; + }; + }; + + }; + + rtc@68 { + pinctrl-names = "default"; + compatible = "microcrystal,rv4162"; + reg = <0x68>; + pinctrl-0 = <&pinctrl_rtc>; + interrupt-parent = <&gpio4>; + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; + }; + + charger: charger@6b { /* bq25896 */ + compatible = "ti,bq25890"; + reg = <0x6b>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_charger>; + interrupt-parent = <&gpio3>; + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; + ti,battery-regulation-voltage = <4192000>; /* 4.192V */ + ti,charge-current = <1600000>; /* 1.6 A */ + ti,termination-current = <66000>; /* 66mA */ + ti,precharge-current = <1300000>; /* 1.3A */ + ti,minimum-sys-voltage = <2750000>; /* 2.75V */ + ti,boost-voltage = <5000000>; /* 5V */ + ti,boost-max-current = <50000>; /* 50mA */ + }; +}; + +&i2c3 { + clock-frequency = <100000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c3>, <&pinctrl_imu>; + status = "okay"; + + lsm9d_magn: magnetometer@1e { + compatible = "st,lsm9ds1-magn"; + reg = <0x1e>; + interrupt-parent = <&gpio3>; + interrupts = <19 IRQ_TYPE_LEVEL_LOW>; + vdd-supply = <®_3V3_P>; + vddio-supply = <®_3V3_P>; + }; + + goodix_ts: touchscreen@5d { + compatible = "goodix,gt5688"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_goodix_ts>; + reg = <0x5d>; + interrupt-parent = <&gpio3>; + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; + reset-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>; + irq-gpios = <&gpio3 0 GPIO_ACTIVE_HIGH>; + touchscreen-size-x = <720>; + touchscreen-size-y = <1440>; + AVDD28-supply = <®_2V8_P>; + VDDIO-supply = <®_1V8_P>; + }; +}; + +&iomuxc { + pinctrl_bt: btgrp { + fsl,pins = < + /* nBT_DISABLE */ + MX8MQ_IOMUXC_NAND_DATA05_GPIO3_IO11 0x16 + /* BT_HOST_WAKE */ + MX8MQ_IOMUXC_NAND_DATA01_GPIO3_IO7 0x10 + >; + }; + + pinctrl_charger: chargernirq { + fsl,pins = < + /* CHRG_nINT */ + MX8MQ_IOMUXC_SAI5_MCLK_GPIO3_IO25 0x80 + >; + }; + + pinctrl_fec1: fec1grp { + fsl,pins = < + MX8MQ_IOMUXC_ENET_MDC_ENET1_MDC 0x3 + MX8MQ_IOMUXC_ENET_MDIO_ENET1_MDIO 0x3 + MX8MQ_IOMUXC_ENET_TD3_ENET1_RGMII_TD3 0x1f + MX8MQ_IOMUXC_ENET_TD2_ENET1_RGMII_TD2 0x1f + MX8MQ_IOMUXC_ENET_TD1_ENET1_RGMII_TD1 0x1f + MX8MQ_IOMUXC_ENET_TD0_ENET1_RGMII_TD0 0x1f + MX8MQ_IOMUXC_ENET_RD3_ENET1_RGMII_RD3 0x91 + MX8MQ_IOMUXC_ENET_RD2_ENET1_RGMII_RD2 0x91 + MX8MQ_IOMUXC_ENET_RD1_ENET1_RGMII_RD1 0x91 + MX8MQ_IOMUXC_ENET_RD0_ENET1_RGMII_RD0 0x91 + MX8MQ_IOMUXC_ENET_TXC_ENET1_RGMII_TXC 0x1f + MX8MQ_IOMUXC_ENET_RXC_ENET1_RGMII_RXC 0x91 + MX8MQ_IOMUXC_ENET_RX_CTL_ENET1_RGMII_RX_CTL 0x91 + MX8MQ_IOMUXC_ENET_TX_CTL_ENET1_RGMII_TX_CTL 0x1f + MX8MQ_IOMUXC_GPIO1_IO09_GPIO1_IO9 0x19 + MX8MQ_IOMUXC_GPIO1_IO15_CCMSRCGPCMIX_CLKO2 0x1f + >; + }; + + pinctrl_goodix_ts: gt5688 { + fsl,pins = < + /* TOUCH INT */ + MX8MQ_IOMUXC_NAND_ALE_GPIO3_IO0 0x16 + /* TOUCH RST */ + MX8MQ_IOMUXC_GPIO1_IO05_GPIO1_IO5 0x19 + >; + }; + + pinctrl_gpio_leds: gpioleds { + fsl,pins = < + MX8MQ_IOMUXC_GPIO1_IO13_GPIO1_IO13 0x16 + >; + }; + + pinctrl_gpio_keys: gpiokeys { + fsl,pins = < + MX8MQ_IOMUXC_SAI2_RXFS_GPIO4_IO21 0x16 + MX8MQ_IOMUXC_SAI2_RXC_GPIO4_IO22 0x16 + /* HP_DET */ + MX8MQ_IOMUXC_SAI5_RXC_GPIO3_IO20 0x180 + >; + }; + + pinctrl_haptic: hapticgrp { + fsl,pins = < + MX8MQ_IOMUXC_SPDIF_RX_PWM2_OUT 0xc6 /* nHAPTIC */ + >; + }; + + pinctrl_i2c1: i2c1grp { + fsl,pins = < + MX8MQ_IOMUXC_I2C1_SCL_I2C1_SCL 0x4000001f + MX8MQ_IOMUXC_I2C1_SDA_I2C1_SDA 0x4000001f + >; + }; + + pinctrl_i2c3: i2c3grp { + fsl,pins = < + MX8MQ_IOMUXC_I2C3_SCL_I2C3_SCL 0x4000001f + MX8MQ_IOMUXC_I2C3_SDA_I2C3_SDA 0x4000001f + >; + }; + + pinctrl_imu: imugrp { + fsl,pins = < + /* IMU_INT */ + MX8MQ_IOMUXC_SAI5_RXFS_GPIO3_IO19 0x8 + >; + }; + + pinctrl_pmic: pmicint { + fsl,pins = < + MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3 0x80 + >; + }; + + pinctrl_pwr_en: pwrengrp { + fsl,pins = < + MX8MQ_IOMUXC_GPIO1_IO08_GPIO1_IO8 0x06 + >; + }; + + pinctrl_pwm1: pwm1 { + fsl,pins = < + MX8MQ_IOMUXC_GPIO1_IO01_PWM1_OUT 0x6 /* DSI_BL_PWM */ + >; + }; + + pinctrl_rtc: rtcirq { + fsl,pins = < + MX8MQ_IOMUXC_SAI3_RXC_GPIO4_IO29 0x80 + >; + }; + + pinctrl_typec: typecgrp { + fsl,pins = < + MX8MQ_IOMUXC_NAND_DATA06_GPIO3_IO12 0x16 + MX8MQ_IOMUXC_NAND_CE0_B_GPIO3_IO1 0x80 + >; + }; + + pinctrl_uart1: uart1grp { + fsl,pins = < + MX8MQ_IOMUXC_UART1_RXD_UART1_DCE_RX 0x49 + MX8MQ_IOMUXC_UART1_TXD_UART1_DCE_TX 0x49 + >; + }; + + pinctrl_uart2: uart2grp { + fsl,pins = < + MX8MQ_IOMUXC_UART2_TXD_UART2_DCE_TX 0x49 + MX8MQ_IOMUXC_UART2_RXD_UART2_DCE_RX 0x49 + MX8MQ_IOMUXC_UART4_RXD_UART2_DCE_CTS_B 0x49 + MX8MQ_IOMUXC_UART4_TXD_UART2_DCE_RTS_B 0x49 + >; + }; + + pinctrl_uart3: uart3grp { + fsl,pins = < + MX8MQ_IOMUXC_UART3_RXD_UART3_DCE_RX 0x49 + MX8MQ_IOMUXC_UART3_TXD_UART3_DCE_TX 0x49 + >; + }; + + pinctrl_uart4: uart4grp { + fsl,pins = < + MX8MQ_IOMUXC_ECSPI2_SCLK_UART4_DCE_RX 0x49 + MX8MQ_IOMUXC_ECSPI2_MOSI_UART4_DCE_TX 0x49 + MX8MQ_IOMUXC_ECSPI2_MISO_UART4_DCE_CTS_B 0x49 + MX8MQ_IOMUXC_ECSPI2_SS0_UART4_DCE_RTS_B 0x49 + MX8MQ_IOMUXC_GPIO1_IO00_ANAMIX_REF_CLK_32K 0x49 + >; + }; + + pinctrl_usdhc1: usdhc1grp { + fsl,pins = < + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x83 + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc3 + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xc3 + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xc3 + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xc3 + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xc3 + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xc3 + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xc3 + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xc3 + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xc3 + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x83 + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 + >; + }; + + pinctrl_usdhc1_100mhz: usdhc1grp100mhz { + fsl,pins = < + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x8d + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xcd + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xcd + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xcd + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xcd + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xcd + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xcd + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xcd + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xcd + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xcd + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x8d + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 + >; + }; + + pinctrl_usdhc1_200mhz: usdhc1grp200mhz { + fsl,pins = < + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x9f + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xdf + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xdf + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xdf + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xdf + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xdf + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xdf + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xdf + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xdf + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xdf + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x9f + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 + >; + }; + + pinctrl_usdhc2_gpio: usdhc2grpgpio { + fsl,pins = < + MX8MQ_IOMUXC_SD2_RESET_B_GPIO2_IO19 0x41 + /* WIFI_WAKE */ + MX8MQ_IOMUXC_SD2_WP_GPIO2_IO20 0x80 + >; + }; + + pinctrl_usdhc2: usdhc2grp { + fsl,pins = < + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x83 + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc3 + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xc3 + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xc3 + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xc3 + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xc3 + >; + }; + + pinctrl_usdhc2_100mhz: usdhc2grp100mhz { + fsl,pins = < + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x8d + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcd + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcd + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcd + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcd + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcd + >; + }; + + pinctrl_usdhc2_200mhz: usdhc2grp200mhz { + fsl,pins = < + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x9f + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcf + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcf + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcf + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcf + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcf + >; + }; + + pinctrl_wdog: wdoggrp { + fsl,pins = < + MX8MQ_IOMUXC_GPIO1_IO02_WDOG1_WDOG_B 0xc6 + >; + }; + + pinctrl_wwan: wwangrp { + fsl,pins = < + /* nWWAN_DISABLE */ + MX8MQ_IOMUXC_NAND_CE3_B_GPIO3_IO4 0x09 + /* nWoWWAN */ + MX8MQ_IOMUXC_NAND_DATA02_GPIO3_IO8 0x80 + /* WWAN_RESET */ + MX8MQ_IOMUXC_NAND_DATA03_GPIO3_IO9 0x19 + >; + }; + +}; + +&pwm1 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_pwm1>; + status = "okay"; +}; + +&pwm2 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_haptic>; + status = "okay"; +}; + +&uart1 { /* console */ + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_uart1>; + status = "okay"; +}; + +&uart3 { /* GNSS */ + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_uart3>; + status = "okay"; +}; + +&uart4 { /* BT */ + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_uart4>, <&pinctrl_bt>; + uart-has-rtscts; + status = "okay"; +}; + +&usb3_phy0 { + status = "okay"; + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + typec_hs: endpoint { + remote-endpoint = <&usb_con_hs>; + }; + }; + port@1 { + reg = <1>; + typec_ss: endpoint { + remote-endpoint = <&usb_con_ss>; + }; + }; +}; + +&usb3_phy1 { + status = "okay"; + #address-cells = <1>; + #size-cells = <0>; +}; + +&usb_dwc3_0 { + status = "okay"; + extcon = <&typec_ptn5100>; + dr_mode = "otg"; +}; + +&usb_dwc3_1 { + status = "okay"; + dr_mode = "host"; +}; + +&usdhc1 { + pinctrl-names = "default", "state_100mhz", "state_200mhz"; + pinctrl-0 = <&pinctrl_usdhc1>; + pinctrl-1 = <&pinctrl_usdhc1_100mhz>; + pinctrl-2 = <&pinctrl_usdhc1_200mhz>; + bus-width = <8>; + non-removable; + status = "okay"; +}; + +&usdhc2 { + pinctrl-names = "default", "state_100mhz", "state_200mhz"; + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; + pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>; + pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>; + bus-width = <4>; + vmmc-supply = <®_usdhc2_vmmc>; + power-supply = <&wifi_pwr_en>; + + non-removable; + disable-wp; + cap-sdio-irq; + keep-power-in-suspend; + wakeup-source; + status = "okay"; +}; + +&wdog1 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_wdog>; + fsl,ext-reset-output; + status = "okay"; +}; -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit 2019-04-15 23:53 ` [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the " Angus Ainslie (Purism) @ 2019-04-16 7:54 ` Marco Felsch 2019-04-16 18:02 ` Angus Ainslie 0 siblings, 1 reply; 11+ messages in thread From: Marco Felsch @ 2019-04-16 7:54 UTC (permalink / raw) To: Angus Ainslie (Purism) Cc: Mark Rutland, devicetree, Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel Hi Agnus, On 19-04-15 16:53, Angus Ainslie (Purism) wrote: > This is the development kit board for the Librem 5. The current level of > support yields a working console and is able to boot userspace from the > Network or eMMC. > > Additional subsystems that are active : > > - Both USB ports > - SD card socket > - WiFi usdhc > - WWAN modem > - GNSS > - GPIO keys > - LEDs > - gyro > - magnetometer > - touchscreen > - pwm > - backlight > - haptic motor > > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> > --- > arch/arm64/boot/dts/freescale/Makefile | 1 + > .../dts/freescale/imx8mq-librem5-devkit.dts | 822 ++++++++++++++++++ > 2 files changed, 823 insertions(+) > create mode 100644 arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile > index 0bd122f60549..c043aca66572 100644 > --- a/arch/arm64/boot/dts/freescale/Makefile > +++ b/arch/arm64/boot/dts/freescale/Makefile > @@ -22,6 +22,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb > > dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb > dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb > +dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb > dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb > dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb > dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > new file mode 100644 > index 000000000000..dd705b6b94a4 > --- /dev/null > +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > @@ -0,0 +1,822 @@ > +/* SPDX-License-Identifier: GPL-2.0+ > + * > + * Copyright 2018-2019 Purism SPC > + */ > + > +/dts-v1/; > + > +#include "imx8mq.dtsi" > +#include "dt-bindings/usb/pd.h" > +#include "dt-bindings/input/input.h" Just a nitpick but this should be alphabetical too. > + > +/ { > + model = "Purism Librem 5 devkit"; > + compatible = "purism,librem5-devkit", "fsl,imx8mq"; > + > + backlight_dsi: backlight-dsi { > + compatible = "pwm-backlight"; > + /* 200 Hz for the PAM2841 */ > + pwms = <&pwm1 0 5000000>; > + brightness-levels = <0 100>; > + num-interpolated-steps = <100>; > + /* Default brightness level (index into the array defined by */ > + /* the "brightness-levels" property) */ > + default-brightness-level = <0>; > + power-supply = <®_22V4_P>; > + }; > + > + chosen { > + stdout-path = &uart1; > + }; > + > + gpio-keys { > + compatible = "gpio-keys"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_gpio_keys>; > + > + btn1 { > + label = "VOL_UP"; > + gpios = <&gpio4 21 GPIO_ACTIVE_LOW>; > + gpio-key,wakeup; > + linux,code = <KEY_VOLUMEUP>; > + }; > + > + btn2 { > + label = "VOL_DOWN"; > + gpios = <&gpio4 22 GPIO_ACTIVE_LOW>; > + gpio-key,wakeup; > + linux,code = <KEY_VOLUMEDOWN>; > + }; > + > + hp_det { > + label = "HP_DET"; > + gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; > + gpio-key,wakeup; > + linux,code = <KEY_HP>; > + }; > + }; > + > + leds { > + compatible = "gpio-leds"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_gpio_leds>; > + status = "okay"; You can drop the status since the default is "okay". > + > + led1 { > + label = "LED 1"; > + gpios = <&gpio1 13 GPIO_ACTIVE_HIGH>; > + default-state = "off"; > + }; > + }; > + > + pmic_osc: pmic-osc { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <32768>; > + clock-output-names = "pmic_osc"; > + }; > + > + pwmleds { > + compatible = "pwm-leds"; Can you add a blank line? It's just a nitpick. > + haptic { > + label = "librem5::haptic"; > + pwms = <&pwm2 0 200000>; > + active-low; > + max-brightness = <255>; > + power-supply = <®_3V3_P>; > + }; > + }; > + > + reg_1V8_P: regulator-1V8-P { I don't know the policy but I would name the nodes and phandles always in lower case. > + compatible = "regulator-fixed"; > + regulator-name = "1V8_P"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <1800000>; > + vin-supply = <®_vbat>; > + en-supply = <®_pwr_en>; What is 'en-supply'? Just checked the v5.1-rc5 and didn't found that property at all. This applies to the other regulator nodes too. > + }; > + > + reg_2V8_P: regulator-2V8-P { > + compatible = "regulator-fixed"; > + regulator-name = "2V8_P"; > + regulator-min-microvolt = <2800000>; > + regulator-max-microvolt = <2800000>; > + vin-supply = <®_3V3_P>; > + }; > + > + reg_3V3_P: regulator-3V3-P { > + compatible = "regulator-fixed"; > + regulator-name = "3V3_P"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + vin-supply = <®_vbat>; > + en-supply = <®_pwr_en>; > + }; > + > + reg_5V_P: regulator-5V-P { > + /* Supplies HDMI, USB hub and smartcard */ > + compatible = "regulator-fixed"; > + regulator-name = "5V_P"; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + vin-supply = <®_vbat>; > + en-supply = <®_pwr_en>; > + }; > + > + reg_5V_SOM: regulator-5V-SOM { > + compatible = "regulator-fixed"; > + regulator-name = "5V_SOM"; > + regulator-min-microvolt = <5000000>; > + regulator-max-microvolt = <5000000>; > + vin-supply = <®_vbat>; > + regulator-always-on; > + }; > + > + reg_22V4_P: regulator-22V4-P { > + compatible = "regulator-fixed"; > + regulator-name = "22V4_P"; > + regulator-min-microvolt = <22400000>; > + regulator-max-microvolt = <22400000>; > + vin-supply = <®_vbat>; > + }; > + > + reg_pwr_en: regulator-pwr-en { > + compatible = "regulator-fixed"; > + regulator-name = "PWR_EN"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + gpio = <&gpio1 8 GPIO_ACTIVE_HIGH>; You missed the pinctrl-0, pinctrl-names property since you already have a pinctrl_pwr_en phandle. > + enable-active-high; > + regulator-always-on; > + }; > + > + reg_usdhc2_vmmc: regulator-usdhc2-vmmc { > + compatible = "regulator-fixed"; > + regulator-name = "VSD_3V3"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>; IMHO you should mux (pinctrl) a gpio where you use them but that's only my opinion. > + enable-active-high; > + regulator-always-on; > + }; > + > + reg_vbat: regulator-vbat { > + compatible = "regulator-fixed"; > + regulator-name = "VBAT_REG"; > + regulator-always-on; > + }; A fixed regulator without a regulator-min-microvolt regulator-max-microvolt? > + > + wifi_pwr_en: wifi-en { > + compatible = "regulator-fixed"; > + regulator-name = "WIFI_EN"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + gpio = <&gpio3 5 GPIO_ACTIVE_HIGH>; Where do you mux that gpio? It seems that you use the pinctrl_usdhc2_gpio for muxing but there is no GPIO3 mux. > + enable-active-high; > + regulator-always-on; > + }; > +}; > + > + > +&clk { > + assigned-clocks = <&clk IMX8MQ_AUDIO_PLL1>, <&clk IMX8MQ_AUDIO_PLL2>; > + assigned-clock-rates = <786432000>, <722534400>; > +}; > + > +&fec1 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_fec1>; > + phy-mode = "rgmii-id"; > + phy-handle = <ðphy0>; > + fsl,magic-packet; > + status = "okay"; status should be the last property in this list. > + phy-supply = <®_3V3_P>; > + > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ethphy0: ethernet-phy@1 { Is it necessary to name it ethphy0? Just a nitpick. > + compatible = "ethernet-phy-ieee802.3-c22"; > + reg = <1>; > + }; > + }; > +}; > + > +&i2c1 { > + clock-frequency = <400000>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_i2c1>; > + status = "okay"; > + > + pmic: pmic@4b { > + reg = <0x4b>; > + compatible = "rohm,bd71837"; Please swap the reg and compatible property. > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_pmic>; > + clocks = <&pmic_osc>; > + clock-names = "osc"; > + clock-output-names = "pmic_clk"; > + interrupt-parent = <&gpio1>; > + interrupts = <3 GPIO_ACTIVE_LOW>; > + interrupt-names = "irq"; > + rohm,reset-snvs-powered; > + > + regulators { > + #address-cells = <1>; > + #size-cells = <0>; Please drop the #address-cells, #size-cells and the @<reg> suffix including the reg = <> property. So it would look like: buck1_reg: BUCK1 { regulator-name = "buck1"; regulator-min-microvolt = <700000>; regulator-max-microvolt = <1300000>; regulator-boot-on; regulator-always-on; regulator-ramp-delay = <1250>; rohm,dvs-run-voltage = <900000>; rohm,dvs-idle-voltage = <850000>; rohm,dvs-suspend-voltage = <800000>; }; Please look at Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt. > + > + buck1_reg: BUCK1@0 { > + reg = <0>; > + regulator-name = "buck1"; > + regulator-min-microvolt = <700000>; > + regulator-max-microvolt = <1300000>; > + regulator-boot-on; > + regulator-always-on; > + regulator-ramp-delay = <1250>; > + rohm,dvs-run-voltage = <900000>; > + rohm,dvs-idle-voltage = <850000>; > + rohm,dvs-suspend-voltage = <800000>; > + }; > + > + buck2_reg: BUCK2@1 { > + reg = <1>; > + regulator-name = "buck2"; > + regulator-min-microvolt = <700000>; > + regulator-max-microvolt = <1300000>; > + regulator-boot-on; > + regulator-always-on; > + regulator-ramp-delay = <1250>; > + rohm,dvs-run-voltage = <1000000>; > + rohm,dvs-idle-voltage = <900000>; > + }; > + > + buck3_reg: BUCK3@2 { > + reg = <2>; > + regulator-name = "buck3"; > + regulator-min-microvolt = <700000>; > + regulator-max-microvolt = <1300000>; > + rohm,dvs-run-voltage = <1000000>; > + }; > + > + buck4_reg: BUCK4@3 { > + reg = <3>; > + regulator-name = "buck4"; > + regulator-min-microvolt = <700000>; > + regulator-max-microvolt = <1300000>; > + rohm,dvs-run-voltage = <1000000>; > + }; > + > + buck5_reg: BUCK5@4 { > + reg = <4>; > + regulator-name = "buck5"; > + regulator-min-microvolt = <700000>; > + regulator-max-microvolt = <1350000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + buck6_reg: BUCK6@5 { > + reg = <5>; > + regulator-name = "buck6"; > + regulator-min-microvolt = <3000000>; > + regulator-max-microvolt = <3300000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + buck7_reg: BUCK7@6 { > + reg = <6>; > + regulator-name = "buck7"; > + regulator-min-microvolt = <1605000>; > + regulator-max-microvolt = <1995000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + buck8_reg: BUCK8@7 { > + reg = <7>; > + regulator-name = "buck8"; > + regulator-min-microvolt = <800000>; > + regulator-max-microvolt = <1400000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + ldo1_reg: LDO1@8 { > + reg = <8>; > + regulator-name = "ldo1"; > + regulator-min-microvolt = <3000000>; > + regulator-max-microvolt = <3300000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + ldo2_reg: LDO2@9 { > + reg = <9>; > + regulator-name = "ldo2"; > + regulator-min-microvolt = <900000>; > + regulator-max-microvolt = <900000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + ldo3_reg: LDO3@10 { > + reg = <10>; > + regulator-name = "ldo3"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <3300000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + ldo4_reg: LDO4@11 { > + reg = <11>; > + regulator-name = "ldo4"; > + regulator-min-microvolt = <900000>; > + regulator-max-microvolt = <1800000>; > + regulator-boot-on; > + regulator-always-on; > + }; > + > + ldo5_reg: LDO5@12 { > + reg = <12>; > + regulator-name = "ldo5"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <3300000>; > + }; > + > + ldo6_reg: LDO6@13 { > + reg = <13>; > + regulator-name = "ldo6"; > + regulator-min-microvolt = <900000>; > + regulator-max-microvolt = <1800000>; > + }; > + > + ldo7_reg: LDO7@14 { > + reg = <14>; > + regulator-name = "ldo7"; > + regulator-min-microvolt = <1800000>; > + regulator-max-microvolt = <3300000>; > + }; > + }; > + }; > + > + typec_ptn5100: usb_typec@52 { > + compatible = "nxp,ptn5110"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_typec>; > + reg = <0x52>; The reg = <> should be followed after the compatible. > + interrupt-parent = <&gpio3>; > + interrupts = <1 IRQ_TYPE_LEVEL_LOW>; > + vconn-supply = <®_5V_P>; The vconn-supply isn't documented elsewhere. > + usb_con: connector { Is that phandle nedded? > + compatible = "usb-c-connector"; > + label = "USB-C"; > + data-role = "dual"; > + power-role = "dual"; > + try-power-role = "sink"; > + source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP )>; > + sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP ) > + PDO_VAR(5000, 12000, 2000)>; Just a nitpick but I would break the lines, e.g. source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP )>; sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP ) PDO_VAR(5000, 12000, 2000)>; > + op-sink-microwatt = <10000000>; > + ports { > + #address-cells = <1>; > + #size-cells = <0>; Blank line please. > + port@0 { > + reg = <0>; > + usb_con_hs: endpoint { > + remote-endpoint = <&typec_hs>; > + }; > + }; Here too. > + port@1 { > + reg = <1>; > + usb_con_ss: endpoint { > + remote-endpoint = <&typec_ss>; > + }; > + }; > + }; > + }; > + > + }; > + > + rtc@68 { > + pinctrl-names = "default"; > + compatible = "microcrystal,rv4162"; > + reg = <0x68>; > + pinctrl-0 = <&pinctrl_rtc>; > + interrupt-parent = <&gpio4>; > + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; > + }; > + > + charger: charger@6b { /* bq25896 */ Is that phandle necessary? > + compatible = "ti,bq25890"; > + reg = <0x6b>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_charger>; > + interrupt-parent = <&gpio3>; > + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; > + ti,battery-regulation-voltage = <4192000>; /* 4.192V */ > + ti,charge-current = <1600000>; /* 1.6 A */ > + ti,termination-current = <66000>; /* 66mA */ > + ti,precharge-current = <1300000>; /* 1.3A */ > + ti,minimum-sys-voltage = <2750000>; /* 2.75V */ > + ti,boost-voltage = <5000000>; /* 5V */ > + ti,boost-max-current = <50000>; /* 50mA */ IMHO I would comment only the measurments. > + }; > +}; > + > +&i2c3 { > + clock-frequency = <100000>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_i2c3>, <&pinctrl_imu>; Why do you don't mux the pinctrl_imu within the magnetometer@1e node? > + status = "okay"; > + > + lsm9d_magn: magnetometer@1e { Is that phandle necessary? > + compatible = "st,lsm9ds1-magn"; > + reg = <0x1e>; > + interrupt-parent = <&gpio3>; > + interrupts = <19 IRQ_TYPE_LEVEL_LOW>; > + vdd-supply = <®_3V3_P>; > + vddio-supply = <®_3V3_P>; > + }; > + > + goodix_ts: touchscreen@5d { This here too. > + compatible = "goodix,gt5688"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_goodix_ts>; > + reg = <0x5d>; The reg = <> should be followed the compatible string. > + interrupt-parent = <&gpio3>; > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > + reset-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>; > + irq-gpios = <&gpio3 0 GPIO_ACTIVE_HIGH>; > + touchscreen-size-x = <720>; > + touchscreen-size-y = <1440>; > + AVDD28-supply = <®_2V8_P>; > + VDDIO-supply = <®_1V8_P>; Didn't found any of these supplies. Can you drop them? > + }; > +}; > + > +&iomuxc { It seems a bit inconsistent to add a comment behind the mux or above. I would add comments just above. > + pinctrl_bt: btgrp { > + fsl,pins = < > + /* nBT_DISABLE */ > + MX8MQ_IOMUXC_NAND_DATA05_GPIO3_IO11 0x16 As you do it here. > + /* BT_HOST_WAKE */ > + MX8MQ_IOMUXC_NAND_DATA01_GPIO3_IO7 0x10 > + >; > + }; > + > + pinctrl_charger: chargernirq { > + fsl,pins = < > + /* CHRG_nINT */ > + MX8MQ_IOMUXC_SAI5_MCLK_GPIO3_IO25 0x80 > + >; > + }; > + > + pinctrl_fec1: fec1grp { > + fsl,pins = < > + MX8MQ_IOMUXC_ENET_MDC_ENET1_MDC 0x3 > + MX8MQ_IOMUXC_ENET_MDIO_ENET1_MDIO 0x3 > + MX8MQ_IOMUXC_ENET_TD3_ENET1_RGMII_TD3 0x1f > + MX8MQ_IOMUXC_ENET_TD2_ENET1_RGMII_TD2 0x1f > + MX8MQ_IOMUXC_ENET_TD1_ENET1_RGMII_TD1 0x1f > + MX8MQ_IOMUXC_ENET_TD0_ENET1_RGMII_TD0 0x1f > + MX8MQ_IOMUXC_ENET_RD3_ENET1_RGMII_RD3 0x91 > + MX8MQ_IOMUXC_ENET_RD2_ENET1_RGMII_RD2 0x91 > + MX8MQ_IOMUXC_ENET_RD1_ENET1_RGMII_RD1 0x91 > + MX8MQ_IOMUXC_ENET_RD0_ENET1_RGMII_RD0 0x91 > + MX8MQ_IOMUXC_ENET_TXC_ENET1_RGMII_TXC 0x1f > + MX8MQ_IOMUXC_ENET_RXC_ENET1_RGMII_RXC 0x91 > + MX8MQ_IOMUXC_ENET_RX_CTL_ENET1_RGMII_RX_CTL 0x91 > + MX8MQ_IOMUXC_ENET_TX_CTL_ENET1_RGMII_TX_CTL 0x1f > + MX8MQ_IOMUXC_GPIO1_IO09_GPIO1_IO9 0x19 > + MX8MQ_IOMUXC_GPIO1_IO15_CCMSRCGPCMIX_CLKO2 0x1f > + >; > + }; > + > + pinctrl_goodix_ts: gt5688 { > + fsl,pins = < > + /* TOUCH INT */ > + MX8MQ_IOMUXC_NAND_ALE_GPIO3_IO0 0x16 > + /* TOUCH RST */ > + MX8MQ_IOMUXC_GPIO1_IO05_GPIO1_IO5 0x19 > + >; > + }; > + > + pinctrl_gpio_leds: gpioleds { > + fsl,pins = < > + MX8MQ_IOMUXC_GPIO1_IO13_GPIO1_IO13 0x16 > + >; > + }; > + > + pinctrl_gpio_keys: gpiokeys { > + fsl,pins = < > + MX8MQ_IOMUXC_SAI2_RXFS_GPIO4_IO21 0x16 > + MX8MQ_IOMUXC_SAI2_RXC_GPIO4_IO22 0x16 > + /* HP_DET */ > + MX8MQ_IOMUXC_SAI5_RXC_GPIO3_IO20 0x180 > + >; > + }; > + > + pinctrl_haptic: hapticgrp { > + fsl,pins = < > + MX8MQ_IOMUXC_SPDIF_RX_PWM2_OUT 0xc6 /* nHAPTIC */ > + >; > + }; > + > + pinctrl_i2c1: i2c1grp { > + fsl,pins = < > + MX8MQ_IOMUXC_I2C1_SCL_I2C1_SCL 0x4000001f > + MX8MQ_IOMUXC_I2C1_SDA_I2C1_SDA 0x4000001f > + >; > + }; > + > + pinctrl_i2c3: i2c3grp { > + fsl,pins = < > + MX8MQ_IOMUXC_I2C3_SCL_I2C3_SCL 0x4000001f > + MX8MQ_IOMUXC_I2C3_SDA_I2C3_SDA 0x4000001f > + >; > + }; > + > + pinctrl_imu: imugrp { > + fsl,pins = < > + /* IMU_INT */ > + MX8MQ_IOMUXC_SAI5_RXFS_GPIO3_IO19 0x8 > + >; > + }; > + > + pinctrl_pmic: pmicint { > + fsl,pins = < > + MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3 0x80 > + >; > + }; > + > + pinctrl_pwr_en: pwrengrp { > + fsl,pins = < > + MX8MQ_IOMUXC_GPIO1_IO08_GPIO1_IO8 0x06 > + >; > + }; > + > + pinctrl_pwm1: pwm1 { > + fsl,pins = < > + MX8MQ_IOMUXC_GPIO1_IO01_PWM1_OUT 0x6 /* DSI_BL_PWM */ > + >; > + }; > + > + pinctrl_rtc: rtcirq { > + fsl,pins = < > + MX8MQ_IOMUXC_SAI3_RXC_GPIO4_IO29 0x80 > + >; > + }; > + > + pinctrl_typec: typecgrp { > + fsl,pins = < > + MX8MQ_IOMUXC_NAND_DATA06_GPIO3_IO12 0x16 > + MX8MQ_IOMUXC_NAND_CE0_B_GPIO3_IO1 0x80 > + >; > + }; > + > + pinctrl_uart1: uart1grp { > + fsl,pins = < > + MX8MQ_IOMUXC_UART1_RXD_UART1_DCE_RX 0x49 > + MX8MQ_IOMUXC_UART1_TXD_UART1_DCE_TX 0x49 > + >; > + }; > + > + pinctrl_uart2: uart2grp { > + fsl,pins = < > + MX8MQ_IOMUXC_UART2_TXD_UART2_DCE_TX 0x49 > + MX8MQ_IOMUXC_UART2_RXD_UART2_DCE_RX 0x49 > + MX8MQ_IOMUXC_UART4_RXD_UART2_DCE_CTS_B 0x49 > + MX8MQ_IOMUXC_UART4_TXD_UART2_DCE_RTS_B 0x49 > + >; > + }; > + > + pinctrl_uart3: uart3grp { > + fsl,pins = < > + MX8MQ_IOMUXC_UART3_RXD_UART3_DCE_RX 0x49 > + MX8MQ_IOMUXC_UART3_TXD_UART3_DCE_TX 0x49 > + >; > + }; > + > + pinctrl_uart4: uart4grp { > + fsl,pins = < > + MX8MQ_IOMUXC_ECSPI2_SCLK_UART4_DCE_RX 0x49 > + MX8MQ_IOMUXC_ECSPI2_MOSI_UART4_DCE_TX 0x49 > + MX8MQ_IOMUXC_ECSPI2_MISO_UART4_DCE_CTS_B 0x49 > + MX8MQ_IOMUXC_ECSPI2_SS0_UART4_DCE_RTS_B 0x49 > + MX8MQ_IOMUXC_GPIO1_IO00_ANAMIX_REF_CLK_32K 0x49 > + >; > + }; > + > + pinctrl_usdhc1: usdhc1grp { > + fsl,pins = < > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x83 > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc3 > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xc3 > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xc3 > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xc3 > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xc3 > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xc3 > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xc3 > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xc3 > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xc3 > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x83 > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 > + >; > + }; > + > + pinctrl_usdhc1_100mhz: usdhc1grp100mhz { > + fsl,pins = < > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x8d > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xcd > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xcd > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xcd > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xcd > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xcd > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xcd > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xcd > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xcd > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xcd > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x8d > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 > + >; > + }; > + > + pinctrl_usdhc1_200mhz: usdhc1grp200mhz { > + fsl,pins = < > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x9f > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xdf > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xdf > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xdf > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xdf > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xdf > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xdf > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xdf > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xdf > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xdf > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x9f > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 > + >; > + }; > + > + pinctrl_usdhc2_gpio: usdhc2grpgpio { > + fsl,pins = < > + MX8MQ_IOMUXC_SD2_RESET_B_GPIO2_IO19 0x41 > + /* WIFI_WAKE */ > + MX8MQ_IOMUXC_SD2_WP_GPIO2_IO20 0x80 > + >; > + }; > + > + pinctrl_usdhc2: usdhc2grp { > + fsl,pins = < > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x83 > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc3 > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xc3 > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xc3 > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xc3 > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xc3 > + >; > + }; > + > + pinctrl_usdhc2_100mhz: usdhc2grp100mhz { > + fsl,pins = < > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x8d > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcd > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcd > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcd > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcd > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcd > + >; > + }; > + > + pinctrl_usdhc2_200mhz: usdhc2grp200mhz { > + fsl,pins = < > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x9f > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcf > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcf > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcf > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcf > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcf > + >; > + }; > + > + pinctrl_wdog: wdoggrp { > + fsl,pins = < > + MX8MQ_IOMUXC_GPIO1_IO02_WDOG1_WDOG_B 0xc6 > + >; > + }; > + > + pinctrl_wwan: wwangrp { > + fsl,pins = < > + /* nWWAN_DISABLE */ > + MX8MQ_IOMUXC_NAND_CE3_B_GPIO3_IO4 0x09 > + /* nWoWWAN */ > + MX8MQ_IOMUXC_NAND_DATA02_GPIO3_IO8 0x80 > + /* WWAN_RESET */ > + MX8MQ_IOMUXC_NAND_DATA03_GPIO3_IO9 0x19 > + >; > + }; > + > +}; > + > +&pwm1 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_pwm1>; > + status = "okay"; > +}; > + > +&pwm2 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_haptic>; Just a nitpick but I would name it pinctrl_pwm2 as you did it for pwm1. > + status = "okay"; > +}; > + > +&uart1 { /* console */ > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_uart1>; > + status = "okay"; > +}; > + > +&uart3 { /* GNSS */ > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_uart3>; > + status = "okay"; > +}; > + > +&uart4 { /* BT */ > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_uart4>, <&pinctrl_bt>; > + uart-has-rtscts; > + status = "okay"; > +}; > + > +&usb3_phy0 { > + status = "okay"; Should be the last property. > + #address-cells = <1>; > + #size-cells = <0>; Blank line. > + port@0 { > + reg = <0>; and here > + typec_hs: endpoint { > + remote-endpoint = <&usb_con_hs>; > + }; > + }; here > + port@1 { > + reg = <1>; and here. > + typec_ss: endpoint { > + remote-endpoint = <&usb_con_ss>; > + }; > + }; > +}; > + > +&usb3_phy1 { > + status = "okay"; > + #address-cells = <1>; > + #size-cells = <0>; Why did you assign the #address-cells and #size-cells without specify any port? Status should be the last property. > +}; > + > +&usb_dwc3_0 { > + status = "okay"; Here also. > + extcon = <&typec_ptn5100>; > + dr_mode = "otg"; > +}; > + > +&usb_dwc3_1 { > + status = "okay"; And here. > + dr_mode = "host"; > +}; > + > +&usdhc1 { > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > + pinctrl-0 = <&pinctrl_usdhc1>; > + pinctrl-1 = <&pinctrl_usdhc1_100mhz>; > + pinctrl-2 = <&pinctrl_usdhc1_200mhz>; > + bus-width = <8>; > + non-removable; > + status = "okay"; > +}; > + > +&usdhc2 { > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; You can drop the pinctrl_usdhc2_gpio handle if you mux them within the regulator node. > + pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>; > + pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>; > + bus-width = <4>; > + vmmc-supply = <®_usdhc2_vmmc>; > + power-supply = <&wifi_pwr_en>; > + > + non-removable; > + disable-wp; > + cap-sdio-irq; > + keep-power-in-suspend; > + wakeup-source; > + status = "okay"; > +}; > + > +&wdog1 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_wdog>; > + fsl,ext-reset-output; > + status = "okay"; > +}; > -- > 2.17.1 > So now I finished, sorry for don't cover that in your v3. Regards, Marco -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit 2019-04-16 7:54 ` Marco Felsch @ 2019-04-16 18:02 ` Angus Ainslie 2019-04-16 21:20 ` Marco Felsch 0 siblings, 1 reply; 11+ messages in thread From: Angus Ainslie @ 2019-04-16 18:02 UTC (permalink / raw) To: Marco Felsch Cc: Mark Rutland, devicetree, Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel Hi Marco, On 2019-04-16 00:54, Marco Felsch wrote: > Hi Agnus, > Just a nitpick but could you spell my name correctly :) > On 19-04-15 16:53, Angus Ainslie (Purism) wrote: >> This is the development kit board for the Librem 5. The current level >> of >> support yields a working console and is able to boot userspace from >> the >> Network or eMMC. >> >> Additional subsystems that are active : >> >> - Both USB ports >> - SD card socket >> - WiFi usdhc >> - WWAN modem >> - GNSS >> - GPIO keys >> - LEDs >> - gyro >> - magnetometer >> - touchscreen >> - pwm >> - backlight >> - haptic motor >> >> Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> >> --- >> arch/arm64/boot/dts/freescale/Makefile | 1 + >> .../dts/freescale/imx8mq-librem5-devkit.dts | 822 >> ++++++++++++++++++ >> 2 files changed, 823 insertions(+) >> create mode 100644 >> arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts >> >> diff --git a/arch/arm64/boot/dts/freescale/Makefile >> b/arch/arm64/boot/dts/freescale/Makefile >> index 0bd122f60549..c043aca66572 100644 >> --- a/arch/arm64/boot/dts/freescale/Makefile >> +++ b/arch/arm64/boot/dts/freescale/Makefile >> @@ -22,6 +22,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb >> >> dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb >> dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb >> +dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb >> dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb >> dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb >> dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb >> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts >> b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts >> new file mode 100644 >> index 000000000000..dd705b6b94a4 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts >> @@ -0,0 +1,822 @@ >> +/* SPDX-License-Identifier: GPL-2.0+ >> + * >> + * Copyright 2018-2019 Purism SPC >> + */ >> + >> +/dts-v1/; >> + >> +#include "imx8mq.dtsi" >> +#include "dt-bindings/usb/pd.h" >> +#include "dt-bindings/input/input.h" > > Just a nitpick but this should be alphabetical too. > Ok. >> + >> +/ { >> + model = "Purism Librem 5 devkit"; >> + compatible = "purism,librem5-devkit", "fsl,imx8mq"; >> + >> + backlight_dsi: backlight-dsi { >> + compatible = "pwm-backlight"; >> + /* 200 Hz for the PAM2841 */ >> + pwms = <&pwm1 0 5000000>; >> + brightness-levels = <0 100>; >> + num-interpolated-steps = <100>; >> + /* Default brightness level (index into the array defined by */ >> + /* the "brightness-levels" property) */ >> + default-brightness-level = <0>; >> + power-supply = <®_22V4_P>; >> + }; >> + >> + chosen { >> + stdout-path = &uart1; >> + }; >> + >> + gpio-keys { >> + compatible = "gpio-keys"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_gpio_keys>; >> + >> + btn1 { >> + label = "VOL_UP"; >> + gpios = <&gpio4 21 GPIO_ACTIVE_LOW>; >> + gpio-key,wakeup; >> + linux,code = <KEY_VOLUMEUP>; >> + }; >> + >> + btn2 { >> + label = "VOL_DOWN"; >> + gpios = <&gpio4 22 GPIO_ACTIVE_LOW>; >> + gpio-key,wakeup; >> + linux,code = <KEY_VOLUMEDOWN>; >> + }; >> + >> + hp_det { >> + label = "HP_DET"; >> + gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; >> + gpio-key,wakeup; >> + linux,code = <KEY_HP>; >> + }; >> + }; >> + >> + leds { >> + compatible = "gpio-leds"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_gpio_leds>; >> + status = "okay"; > > You can drop the status since the default is "okay". > Ok. >> + >> + led1 { >> + label = "LED 1"; >> + gpios = <&gpio1 13 GPIO_ACTIVE_HIGH>; >> + default-state = "off"; >> + }; >> + }; >> + >> + pmic_osc: pmic-osc { >> + compatible = "fixed-clock"; >> + #clock-cells = <0>; >> + clock-frequency = <32768>; >> + clock-output-names = "pmic_osc"; >> + }; >> + >> + pwmleds { >> + compatible = "pwm-leds"; > > Can you add a blank line? It's just a nitpick. > Sure. >> + haptic { >> + label = "librem5::haptic"; >> + pwms = <&pwm2 0 200000>; >> + active-low; >> + max-brightness = <255>; >> + power-supply = <®_3V3_P>; >> + }; >> + }; >> + >> + reg_1V8_P: regulator-1V8-P { > > I don't know the policy but I would name the nodes and phandles always > in lower case. > Ok. >> + compatible = "regulator-fixed"; >> + regulator-name = "1V8_P"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <1800000>; >> + vin-supply = <®_vbat>; >> + en-supply = <®_pwr_en>; > > What is 'en-supply'? Just checked the v5.1-rc5 and didn't found that > property at all. This applies to the other regulator nodes too. > I was attempting to show a dependency on both supplies but I'll change the way it is structured. >> + }; >> + >> + reg_2V8_P: regulator-2V8-P { >> + compatible = "regulator-fixed"; >> + regulator-name = "2V8_P"; >> + regulator-min-microvolt = <2800000>; >> + regulator-max-microvolt = <2800000>; >> + vin-supply = <®_3V3_P>; >> + }; >> + >> + reg_3V3_P: regulator-3V3-P { >> + compatible = "regulator-fixed"; >> + regulator-name = "3V3_P"; >> + regulator-min-microvolt = <3300000>; >> + regulator-max-microvolt = <3300000>; >> + vin-supply = <®_vbat>; >> + en-supply = <®_pwr_en>; >> + }; >> + >> + reg_5V_P: regulator-5V-P { >> + /* Supplies HDMI, USB hub and smartcard */ >> + compatible = "regulator-fixed"; >> + regulator-name = "5V_P"; >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + vin-supply = <®_vbat>; >> + en-supply = <®_pwr_en>; >> + }; >> + >> + reg_5V_SOM: regulator-5V-SOM { >> + compatible = "regulator-fixed"; >> + regulator-name = "5V_SOM"; >> + regulator-min-microvolt = <5000000>; >> + regulator-max-microvolt = <5000000>; >> + vin-supply = <®_vbat>; >> + regulator-always-on; >> + }; >> + >> + reg_22V4_P: regulator-22V4-P { >> + compatible = "regulator-fixed"; >> + regulator-name = "22V4_P"; >> + regulator-min-microvolt = <22400000>; >> + regulator-max-microvolt = <22400000>; >> + vin-supply = <®_vbat>; >> + }; >> + >> + reg_pwr_en: regulator-pwr-en { >> + compatible = "regulator-fixed"; >> + regulator-name = "PWR_EN"; >> + regulator-min-microvolt = <3300000>; >> + regulator-max-microvolt = <3300000>; >> + gpio = <&gpio1 8 GPIO_ACTIVE_HIGH>; > > You missed the pinctrl-0, pinctrl-names property since you already have > a pinctrl_pwr_en phandle. > >> + enable-active-high; >> + regulator-always-on; >> + }; >> + >> + reg_usdhc2_vmmc: regulator-usdhc2-vmmc { >> + compatible = "regulator-fixed"; >> + regulator-name = "VSD_3V3"; >> + regulator-min-microvolt = <3300000>; >> + regulator-max-microvolt = <3300000>; >> + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>; > > IMHO you should mux (pinctrl) a gpio where you use them but that's only > my opinion. > >> + enable-active-high; >> + regulator-always-on; >> + }; >> + >> + reg_vbat: regulator-vbat { >> + compatible = "regulator-fixed"; >> + regulator-name = "VBAT_REG"; >> + regulator-always-on; >> + }; > > A fixed regulator without a regulator-min-microvolt > regulator-max-microvolt? > That will get fixed with the issue above. >> + >> + wifi_pwr_en: wifi-en { >> + compatible = "regulator-fixed"; >> + regulator-name = "WIFI_EN"; >> + regulator-min-microvolt = <3300000>; >> + regulator-max-microvolt = <3300000>; >> + gpio = <&gpio3 5 GPIO_ACTIVE_HIGH>; > > Where do you mux that gpio? It seems that you use the > pinctrl_usdhc2_gpio for muxing but there is no GPIO3 mux. > I'll fix it with the regulators. >> + enable-active-high; >> + regulator-always-on; >> + }; >> +}; >> + >> + >> +&clk { >> + assigned-clocks = <&clk IMX8MQ_AUDIO_PLL1>, <&clk >> IMX8MQ_AUDIO_PLL2>; >> + assigned-clock-rates = <786432000>, <722534400>; >> +}; >> + >> +&fec1 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_fec1>; >> + phy-mode = "rgmii-id"; >> + phy-handle = <ðphy0>; >> + fsl,magic-packet; >> + status = "okay"; > > status should be the last property in this list. > Ok. >> + phy-supply = <®_3V3_P>; >> + >> + mdio { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + ethphy0: ethernet-phy@1 { > > Is it necessary to name it ethphy0? Just a nitpick. > >> + compatible = "ethernet-phy-ieee802.3-c22"; >> + reg = <1>; >> + }; >> + }; >> +}; >> + >> +&i2c1 { >> + clock-frequency = <400000>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_i2c1>; >> + status = "okay"; >> + >> + pmic: pmic@4b { >> + reg = <0x4b>; >> + compatible = "rohm,bd71837"; > > Please swap the reg and compatible property. > >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_pmic>; >> + clocks = <&pmic_osc>; >> + clock-names = "osc"; >> + clock-output-names = "pmic_clk"; >> + interrupt-parent = <&gpio1>; >> + interrupts = <3 GPIO_ACTIVE_LOW>; >> + interrupt-names = "irq"; >> + rohm,reset-snvs-powered; >> + >> + regulators { >> + #address-cells = <1>; >> + #size-cells = <0>; > > Please drop the #address-cells, #size-cells and the @<reg> suffix > including the reg = <> property. So it would look like: > > buck1_reg: BUCK1 { > regulator-name = "buck1"; > regulator-min-microvolt = <700000>; > regulator-max-microvolt = <1300000>; > regulator-boot-on; > regulator-always-on; > regulator-ramp-delay = <1250>; > rohm,dvs-run-voltage = <900000>; > rohm,dvs-idle-voltage = <850000>; > rohm,dvs-suspend-voltage = <800000>; > }; > > Please look at > Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt. > Thanks hadn't noticed the documentation change. >> + >> + buck1_reg: BUCK1@0 { >> + reg = <0>; >> + regulator-name = "buck1"; >> + regulator-min-microvolt = <700000>; >> + regulator-max-microvolt = <1300000>; >> + regulator-boot-on; >> + regulator-always-on; >> + regulator-ramp-delay = <1250>; >> + rohm,dvs-run-voltage = <900000>; >> + rohm,dvs-idle-voltage = <850000>; >> + rohm,dvs-suspend-voltage = <800000>; >> + }; >> + >> + buck2_reg: BUCK2@1 { >> + reg = <1>; >> + regulator-name = "buck2"; >> + regulator-min-microvolt = <700000>; >> + regulator-max-microvolt = <1300000>; >> + regulator-boot-on; >> + regulator-always-on; >> + regulator-ramp-delay = <1250>; >> + rohm,dvs-run-voltage = <1000000>; >> + rohm,dvs-idle-voltage = <900000>; >> + }; >> + >> + buck3_reg: BUCK3@2 { >> + reg = <2>; >> + regulator-name = "buck3"; >> + regulator-min-microvolt = <700000>; >> + regulator-max-microvolt = <1300000>; >> + rohm,dvs-run-voltage = <1000000>; >> + }; >> + >> + buck4_reg: BUCK4@3 { >> + reg = <3>; >> + regulator-name = "buck4"; >> + regulator-min-microvolt = <700000>; >> + regulator-max-microvolt = <1300000>; >> + rohm,dvs-run-voltage = <1000000>; >> + }; >> + >> + buck5_reg: BUCK5@4 { >> + reg = <4>; >> + regulator-name = "buck5"; >> + regulator-min-microvolt = <700000>; >> + regulator-max-microvolt = <1350000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + buck6_reg: BUCK6@5 { >> + reg = <5>; >> + regulator-name = "buck6"; >> + regulator-min-microvolt = <3000000>; >> + regulator-max-microvolt = <3300000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + buck7_reg: BUCK7@6 { >> + reg = <6>; >> + regulator-name = "buck7"; >> + regulator-min-microvolt = <1605000>; >> + regulator-max-microvolt = <1995000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + buck8_reg: BUCK8@7 { >> + reg = <7>; >> + regulator-name = "buck8"; >> + regulator-min-microvolt = <800000>; >> + regulator-max-microvolt = <1400000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo1_reg: LDO1@8 { >> + reg = <8>; >> + regulator-name = "ldo1"; >> + regulator-min-microvolt = <3000000>; >> + regulator-max-microvolt = <3300000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo2_reg: LDO2@9 { >> + reg = <9>; >> + regulator-name = "ldo2"; >> + regulator-min-microvolt = <900000>; >> + regulator-max-microvolt = <900000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo3_reg: LDO3@10 { >> + reg = <10>; >> + regulator-name = "ldo3"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <3300000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo4_reg: LDO4@11 { >> + reg = <11>; >> + regulator-name = "ldo4"; >> + regulator-min-microvolt = <900000>; >> + regulator-max-microvolt = <1800000>; >> + regulator-boot-on; >> + regulator-always-on; >> + }; >> + >> + ldo5_reg: LDO5@12 { >> + reg = <12>; >> + regulator-name = "ldo5"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <3300000>; >> + }; >> + >> + ldo6_reg: LDO6@13 { >> + reg = <13>; >> + regulator-name = "ldo6"; >> + regulator-min-microvolt = <900000>; >> + regulator-max-microvolt = <1800000>; >> + }; >> + >> + ldo7_reg: LDO7@14 { >> + reg = <14>; >> + regulator-name = "ldo7"; >> + regulator-min-microvolt = <1800000>; >> + regulator-max-microvolt = <3300000>; >> + }; >> + }; >> + }; >> + >> + typec_ptn5100: usb_typec@52 { >> + compatible = "nxp,ptn5110"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_typec>; >> + reg = <0x52>; > > The reg = <> should be followed after the compatible. > >> + interrupt-parent = <&gpio3>; >> + interrupts = <1 IRQ_TYPE_LEVEL_LOW>; >> + vconn-supply = <®_5V_P>; > > The vconn-supply isn't documented elsewhere. > Yeah that came from the schematic not the code or documentation. >> + usb_con: connector { > > Is that phandle nedded? > >> + compatible = "usb-c-connector"; >> + label = "USB-C"; >> + data-role = "dual"; >> + power-role = "dual"; >> + try-power-role = "sink"; >> + source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | >> PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP )>; >> + sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | >> PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP ) >> + PDO_VAR(5000, 12000, 2000)>; > > Just a nitpick but I would break the lines, e.g. > > source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | > PDO_FIXED_DUAL_ROLE | > PDO_FIXED_DATA_SWAP )>; > sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | > PDO_FIXED_DUAL_ROLE | > PDO_FIXED_DATA_SWAP ) > PDO_VAR(5000, 12000, 2000)>; > > Ok they are longer than 80 cols anyway >> + op-sink-microwatt = <10000000>; >> + ports { >> + #address-cells = <1>; >> + #size-cells = <0>; > > Blank line please. > >> + port@0 { >> + reg = <0>; >> + usb_con_hs: endpoint { >> + remote-endpoint = <&typec_hs>; >> + }; >> + }; > > Here too. > >> + port@1 { >> + reg = <1>; >> + usb_con_ss: endpoint { >> + remote-endpoint = <&typec_ss>; >> + }; >> + }; >> + }; >> + }; >> + >> + }; >> + >> + rtc@68 { >> + pinctrl-names = "default"; >> + compatible = "microcrystal,rv4162"; >> + reg = <0x68>; >> + pinctrl-0 = <&pinctrl_rtc>; >> + interrupt-parent = <&gpio4>; >> + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; >> + }; >> + >> + charger: charger@6b { /* bq25896 */ > > Is that phandle necessary? > >> + compatible = "ti,bq25890"; >> + reg = <0x6b>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_charger>; >> + interrupt-parent = <&gpio3>; >> + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; >> + ti,battery-regulation-voltage = <4192000>; /* 4.192V */ >> + ti,charge-current = <1600000>; /* 1.6 A */ >> + ti,termination-current = <66000>; /* 66mA */ >> + ti,precharge-current = <1300000>; /* 1.3A */ >> + ti,minimum-sys-voltage = <2750000>; /* 2.75V */ >> + ti,boost-voltage = <5000000>; /* 5V */ >> + ti,boost-max-current = <50000>; /* 50mA */ > > IMHO I would comment only the measurments. > Sorry do you mean the units ? I liked the additional clarity of not having to count zero's. >> + }; >> +}; >> + >> +&i2c3 { >> + clock-frequency = <100000>; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_i2c3>, <&pinctrl_imu>; > > Why do you don't mux the pinctrl_imu within the magnetometer@1e node? > >> + status = "okay"; >> + >> + lsm9d_magn: magnetometer@1e { > > Is that phandle necessary? > >> + compatible = "st,lsm9ds1-magn"; >> + reg = <0x1e>; >> + interrupt-parent = <&gpio3>; >> + interrupts = <19 IRQ_TYPE_LEVEL_LOW>; >> + vdd-supply = <®_3V3_P>; >> + vddio-supply = <®_3V3_P>; >> + }; >> + >> + goodix_ts: touchscreen@5d { > > This here too. > >> + compatible = "goodix,gt5688"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_goodix_ts>; >> + reg = <0x5d>; > > The reg = <> should be followed the compatible string. > >> + interrupt-parent = <&gpio3>; >> + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; >> + reset-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>; >> + irq-gpios = <&gpio3 0 GPIO_ACTIVE_HIGH>; >> + touchscreen-size-x = <720>; >> + touchscreen-size-y = <1440>; >> + AVDD28-supply = <®_2V8_P>; >> + VDDIO-supply = <®_1V8_P>; > > Didn't found any of these supplies. Can you drop them? > AVDD28 line 539 in drivers/input/touchscreen/goodix.c VDDIO line 548 in drivers/input/touchscreen/goodix.c >> + }; >> +}; >> + >> +&iomuxc { > > It seems a bit inconsistent to add a comment behind the > mux or above. I would add comments just above. > My preference was for commenting after but that made some lines longer than 80 cols so I reduced the offending lines. >> + pinctrl_bt: btgrp { >> + fsl,pins = < >> + /* nBT_DISABLE */ >> + MX8MQ_IOMUXC_NAND_DATA05_GPIO3_IO11 0x16 > > As you do it here. > >> + /* BT_HOST_WAKE */ >> + MX8MQ_IOMUXC_NAND_DATA01_GPIO3_IO7 0x10 >> + >; >> + }; >> + >> + pinctrl_charger: chargernirq { >> + fsl,pins = < >> + /* CHRG_nINT */ >> + MX8MQ_IOMUXC_SAI5_MCLK_GPIO3_IO25 0x80 >> + >; >> + }; >> + >> + pinctrl_fec1: fec1grp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_ENET_MDC_ENET1_MDC 0x3 >> + MX8MQ_IOMUXC_ENET_MDIO_ENET1_MDIO 0x3 >> + MX8MQ_IOMUXC_ENET_TD3_ENET1_RGMII_TD3 0x1f >> + MX8MQ_IOMUXC_ENET_TD2_ENET1_RGMII_TD2 0x1f >> + MX8MQ_IOMUXC_ENET_TD1_ENET1_RGMII_TD1 0x1f >> + MX8MQ_IOMUXC_ENET_TD0_ENET1_RGMII_TD0 0x1f >> + MX8MQ_IOMUXC_ENET_RD3_ENET1_RGMII_RD3 0x91 >> + MX8MQ_IOMUXC_ENET_RD2_ENET1_RGMII_RD2 0x91 >> + MX8MQ_IOMUXC_ENET_RD1_ENET1_RGMII_RD1 0x91 >> + MX8MQ_IOMUXC_ENET_RD0_ENET1_RGMII_RD0 0x91 >> + MX8MQ_IOMUXC_ENET_TXC_ENET1_RGMII_TXC 0x1f >> + MX8MQ_IOMUXC_ENET_RXC_ENET1_RGMII_RXC 0x91 >> + MX8MQ_IOMUXC_ENET_RX_CTL_ENET1_RGMII_RX_CTL 0x91 >> + MX8MQ_IOMUXC_ENET_TX_CTL_ENET1_RGMII_TX_CTL 0x1f >> + MX8MQ_IOMUXC_GPIO1_IO09_GPIO1_IO9 0x19 >> + MX8MQ_IOMUXC_GPIO1_IO15_CCMSRCGPCMIX_CLKO2 0x1f >> + >; >> + }; >> + >> + pinctrl_goodix_ts: gt5688 { >> + fsl,pins = < >> + /* TOUCH INT */ >> + MX8MQ_IOMUXC_NAND_ALE_GPIO3_IO0 0x16 >> + /* TOUCH RST */ >> + MX8MQ_IOMUXC_GPIO1_IO05_GPIO1_IO5 0x19 >> + >; >> + }; >> + >> + pinctrl_gpio_leds: gpioleds { >> + fsl,pins = < >> + MX8MQ_IOMUXC_GPIO1_IO13_GPIO1_IO13 0x16 >> + >; >> + }; >> + >> + pinctrl_gpio_keys: gpiokeys { >> + fsl,pins = < >> + MX8MQ_IOMUXC_SAI2_RXFS_GPIO4_IO21 0x16 >> + MX8MQ_IOMUXC_SAI2_RXC_GPIO4_IO22 0x16 >> + /* HP_DET */ >> + MX8MQ_IOMUXC_SAI5_RXC_GPIO3_IO20 0x180 >> + >; >> + }; >> + >> + pinctrl_haptic: hapticgrp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_SPDIF_RX_PWM2_OUT 0xc6 /* nHAPTIC */ >> + >; >> + }; >> + >> + pinctrl_i2c1: i2c1grp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_I2C1_SCL_I2C1_SCL 0x4000001f >> + MX8MQ_IOMUXC_I2C1_SDA_I2C1_SDA 0x4000001f >> + >; >> + }; >> + >> + pinctrl_i2c3: i2c3grp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_I2C3_SCL_I2C3_SCL 0x4000001f >> + MX8MQ_IOMUXC_I2C3_SDA_I2C3_SDA 0x4000001f >> + >; >> + }; >> + >> + pinctrl_imu: imugrp { >> + fsl,pins = < >> + /* IMU_INT */ >> + MX8MQ_IOMUXC_SAI5_RXFS_GPIO3_IO19 0x8 >> + >; >> + }; >> + >> + pinctrl_pmic: pmicint { >> + fsl,pins = < >> + MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3 0x80 >> + >; >> + }; >> + >> + pinctrl_pwr_en: pwrengrp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_GPIO1_IO08_GPIO1_IO8 0x06 >> + >; >> + }; >> + >> + pinctrl_pwm1: pwm1 { >> + fsl,pins = < >> + MX8MQ_IOMUXC_GPIO1_IO01_PWM1_OUT 0x6 /* DSI_BL_PWM */ >> + >; >> + }; >> + >> + pinctrl_rtc: rtcirq { >> + fsl,pins = < >> + MX8MQ_IOMUXC_SAI3_RXC_GPIO4_IO29 0x80 >> + >; >> + }; >> + >> + pinctrl_typec: typecgrp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_NAND_DATA06_GPIO3_IO12 0x16 >> + MX8MQ_IOMUXC_NAND_CE0_B_GPIO3_IO1 0x80 >> + >; >> + }; >> + >> + pinctrl_uart1: uart1grp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_UART1_RXD_UART1_DCE_RX 0x49 >> + MX8MQ_IOMUXC_UART1_TXD_UART1_DCE_TX 0x49 >> + >; >> + }; >> + >> + pinctrl_uart2: uart2grp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_UART2_TXD_UART2_DCE_TX 0x49 >> + MX8MQ_IOMUXC_UART2_RXD_UART2_DCE_RX 0x49 >> + MX8MQ_IOMUXC_UART4_RXD_UART2_DCE_CTS_B 0x49 >> + MX8MQ_IOMUXC_UART4_TXD_UART2_DCE_RTS_B 0x49 >> + >; >> + }; >> + >> + pinctrl_uart3: uart3grp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_UART3_RXD_UART3_DCE_RX 0x49 >> + MX8MQ_IOMUXC_UART3_TXD_UART3_DCE_TX 0x49 >> + >; >> + }; >> + >> + pinctrl_uart4: uart4grp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_ECSPI2_SCLK_UART4_DCE_RX 0x49 >> + MX8MQ_IOMUXC_ECSPI2_MOSI_UART4_DCE_TX 0x49 >> + MX8MQ_IOMUXC_ECSPI2_MISO_UART4_DCE_CTS_B 0x49 >> + MX8MQ_IOMUXC_ECSPI2_SS0_UART4_DCE_RTS_B 0x49 >> + MX8MQ_IOMUXC_GPIO1_IO00_ANAMIX_REF_CLK_32K 0x49 >> + >; >> + }; >> + >> + pinctrl_usdhc1: usdhc1grp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x83 >> + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc3 >> + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xc3 >> + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xc3 >> + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xc3 >> + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xc3 >> + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xc3 >> + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xc3 >> + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xc3 >> + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xc3 >> + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x83 >> + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 >> + >; >> + }; >> + >> + pinctrl_usdhc1_100mhz: usdhc1grp100mhz { >> + fsl,pins = < >> + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x8d >> + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xcd >> + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xcd >> + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xcd >> + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xcd >> + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xcd >> + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xcd >> + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xcd >> + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xcd >> + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xcd >> + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x8d >> + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 >> + >; >> + }; >> + >> + pinctrl_usdhc1_200mhz: usdhc1grp200mhz { >> + fsl,pins = < >> + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x9f >> + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xdf >> + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xdf >> + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xdf >> + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xdf >> + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xdf >> + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xdf >> + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xdf >> + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xdf >> + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xdf >> + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x9f >> + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 >> + >; >> + }; >> + >> + pinctrl_usdhc2_gpio: usdhc2grpgpio { >> + fsl,pins = < >> + MX8MQ_IOMUXC_SD2_RESET_B_GPIO2_IO19 0x41 >> + /* WIFI_WAKE */ >> + MX8MQ_IOMUXC_SD2_WP_GPIO2_IO20 0x80 >> + >; >> + }; >> + >> + pinctrl_usdhc2: usdhc2grp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x83 >> + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc3 >> + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xc3 >> + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xc3 >> + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xc3 >> + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xc3 >> + >; >> + }; >> + >> + pinctrl_usdhc2_100mhz: usdhc2grp100mhz { >> + fsl,pins = < >> + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x8d >> + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcd >> + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcd >> + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcd >> + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcd >> + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcd >> + >; >> + }; >> + >> + pinctrl_usdhc2_200mhz: usdhc2grp200mhz { >> + fsl,pins = < >> + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x9f >> + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcf >> + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcf >> + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcf >> + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcf >> + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcf >> + >; >> + }; >> + >> + pinctrl_wdog: wdoggrp { >> + fsl,pins = < >> + MX8MQ_IOMUXC_GPIO1_IO02_WDOG1_WDOG_B 0xc6 >> + >; >> + }; >> + >> + pinctrl_wwan: wwangrp { >> + fsl,pins = < >> + /* nWWAN_DISABLE */ >> + MX8MQ_IOMUXC_NAND_CE3_B_GPIO3_IO4 0x09 >> + /* nWoWWAN */ >> + MX8MQ_IOMUXC_NAND_DATA02_GPIO3_IO8 0x80 >> + /* WWAN_RESET */ >> + MX8MQ_IOMUXC_NAND_DATA03_GPIO3_IO9 0x19 >> + >; >> + }; >> + >> +}; >> + >> +&pwm1 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_pwm1>; >> + status = "okay"; >> +}; >> + >> +&pwm2 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_haptic>; > > Just a nitpick but I would name it pinctrl_pwm2 as you did it for pwm1. > I figured haptic was more descriptive. >> + status = "okay"; >> +}; >> + >> +&uart1 { /* console */ >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_uart1>; >> + status = "okay"; >> +}; >> + >> +&uart3 { /* GNSS */ >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_uart3>; >> + status = "okay"; >> +}; >> + >> +&uart4 { /* BT */ >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_uart4>, <&pinctrl_bt>; >> + uart-has-rtscts; >> + status = "okay"; >> +}; >> + >> +&usb3_phy0 { >> + status = "okay"; > > Should be the last property. > >> + #address-cells = <1>; >> + #size-cells = <0>; > > Blank line. >> + port@0 { >> + reg = <0>; > > and here >> + typec_hs: endpoint { >> + remote-endpoint = <&usb_con_hs>; >> + }; >> + }; > > here > >> + port@1 { >> + reg = <1>; > > and here. > >> + typec_ss: endpoint { >> + remote-endpoint = <&usb_con_ss>; >> + }; >> + }; >> +}; >> + >> +&usb3_phy1 { >> + status = "okay"; >> + #address-cells = <1>; >> + #size-cells = <0>; > > Why did you assign the #address-cells and #size-cells without specify > any port? Status should be the last property. > Cut and paste from phy0. >> +}; >> + >> +&usb_dwc3_0 { >> + status = "okay"; > > Here also. > >> + extcon = <&typec_ptn5100>; >> + dr_mode = "otg"; >> +}; >> + >> +&usb_dwc3_1 { >> + status = "okay"; > > And here. > >> + dr_mode = "host"; >> +}; >> + >> +&usdhc1 { >> + pinctrl-names = "default", "state_100mhz", "state_200mhz"; >> + pinctrl-0 = <&pinctrl_usdhc1>; >> + pinctrl-1 = <&pinctrl_usdhc1_100mhz>; >> + pinctrl-2 = <&pinctrl_usdhc1_200mhz>; >> + bus-width = <8>; >> + non-removable; >> + status = "okay"; >> +}; >> + >> +&usdhc2 { >> + pinctrl-names = "default", "state_100mhz", "state_200mhz"; >> + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; > > You can drop the pinctrl_usdhc2_gpio handle if you mux them within the > regulator node. Ok > >> + pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>; >> + pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>; >> + bus-width = <4>; >> + vmmc-supply = <®_usdhc2_vmmc>; >> + power-supply = <&wifi_pwr_en>; >> + >> + non-removable; >> + disable-wp; >> + cap-sdio-irq; >> + keep-power-in-suspend; >> + wakeup-source; >> + status = "okay"; >> +}; >> + >> +&wdog1 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&pinctrl_wdog>; >> + fsl,ext-reset-output; >> + status = "okay"; >> +}; >> -- >> 2.17.1 >> > > So now I finished, sorry for don't cover that in your v3. > > Regards, > Marco Thanks Angus ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit 2019-04-16 18:02 ` Angus Ainslie @ 2019-04-16 21:20 ` Marco Felsch 2019-04-16 22:36 ` Angus Ainslie 0 siblings, 1 reply; 11+ messages in thread From: Marco Felsch @ 2019-04-16 21:20 UTC (permalink / raw) To: Angus Ainslie Cc: Mark Rutland, devicetree, Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel Hi Angus, On 19-04-16 11:02, Angus Ainslie wrote: > Hi Marco, > > On 2019-04-16 00:54, Marco Felsch wrote: > > Hi Agnus, > > > > Just a nitpick but could you spell my name correctly :) Sorry it was to early that morning and I haven't had my first coffee. > > On 19-04-15 16:53, Angus Ainslie (Purism) wrote: > > > This is the development kit board for the Librem 5. The current > > > level of > > > support yields a working console and is able to boot userspace from > > > the > > > Network or eMMC. > > > > > > Additional subsystems that are active : > > > > > > - Both USB ports > > > - SD card socket > > > - WiFi usdhc > > > - WWAN modem > > > - GNSS > > > - GPIO keys > > > - LEDs > > > - gyro > > > - magnetometer > > > - touchscreen > > > - pwm > > > - backlight > > > - haptic motor > > > > > > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> > > > --- > > > arch/arm64/boot/dts/freescale/Makefile | 1 + > > > .../dts/freescale/imx8mq-librem5-devkit.dts | 822 > > > ++++++++++++++++++ > > > 2 files changed, 823 insertions(+) > > > create mode 100644 > > > arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > > > > > diff --git a/arch/arm64/boot/dts/freescale/Makefile > > > b/arch/arm64/boot/dts/freescale/Makefile > > > index 0bd122f60549..c043aca66572 100644 > > > --- a/arch/arm64/boot/dts/freescale/Makefile > > > +++ b/arch/arm64/boot/dts/freescale/Makefile > > > @@ -22,6 +22,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb > > > > > > dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb > > > dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb > > > +dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb > > > dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb > > > dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb > > > dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > > b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > > new file mode 100644 > > > index 000000000000..dd705b6b94a4 > > > --- /dev/null > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > > @@ -0,0 +1,822 @@ > > > +/* SPDX-License-Identifier: GPL-2.0+ > > > + * > > > + * Copyright 2018-2019 Purism SPC > > > + */ > > > + > > > +/dts-v1/; > > > + > > > +#include "imx8mq.dtsi" > > > +#include "dt-bindings/usb/pd.h" > > > +#include "dt-bindings/input/input.h" > > > > Just a nitpick but this should be alphabetical too. > > > > Ok. > > > > + > > > +/ { > > > + model = "Purism Librem 5 devkit"; > > > + compatible = "purism,librem5-devkit", "fsl,imx8mq"; > > > + > > > + backlight_dsi: backlight-dsi { > > > + compatible = "pwm-backlight"; > > > + /* 200 Hz for the PAM2841 */ > > > + pwms = <&pwm1 0 5000000>; > > > + brightness-levels = <0 100>; > > > + num-interpolated-steps = <100>; > > > + /* Default brightness level (index into the array defined by */ > > > + /* the "brightness-levels" property) */ > > > + default-brightness-level = <0>; > > > + power-supply = <®_22V4_P>; > > > + }; > > > + > > > + chosen { > > > + stdout-path = &uart1; > > > + }; > > > + > > > + gpio-keys { > > > + compatible = "gpio-keys"; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_gpio_keys>; > > > + > > > + btn1 { > > > + label = "VOL_UP"; > > > + gpios = <&gpio4 21 GPIO_ACTIVE_LOW>; > > > + gpio-key,wakeup; > > > + linux,code = <KEY_VOLUMEUP>; > > > + }; > > > + > > > + btn2 { > > > + label = "VOL_DOWN"; > > > + gpios = <&gpio4 22 GPIO_ACTIVE_LOW>; > > > + gpio-key,wakeup; > > > + linux,code = <KEY_VOLUMEDOWN>; > > > + }; > > > + > > > + hp_det { > > > + label = "HP_DET"; > > > + gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; > > > + gpio-key,wakeup; > > > + linux,code = <KEY_HP>; > > > + }; > > > + }; > > > + > > > + leds { > > > + compatible = "gpio-leds"; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_gpio_leds>; > > > + status = "okay"; > > > > You can drop the status since the default is "okay". > > > > Ok. > > > > + > > > + led1 { > > > + label = "LED 1"; > > > + gpios = <&gpio1 13 GPIO_ACTIVE_HIGH>; > > > + default-state = "off"; > > > + }; > > > + }; > > > + > > > + pmic_osc: pmic-osc { > > > + compatible = "fixed-clock"; > > > + #clock-cells = <0>; > > > + clock-frequency = <32768>; > > > + clock-output-names = "pmic_osc"; > > > + }; > > > + > > > + pwmleds { > > > + compatible = "pwm-leds"; > > > > Can you add a blank line? It's just a nitpick. > > > > Sure. > > > > + haptic { > > > + label = "librem5::haptic"; > > > + pwms = <&pwm2 0 200000>; > > > + active-low; > > > + max-brightness = <255>; > > > + power-supply = <®_3V3_P>; > > > + }; > > > + }; > > > + > > > + reg_1V8_P: regulator-1V8-P { > > > > I don't know the policy but I would name the nodes and phandles always > > in lower case. > > > > Ok. > > > > + compatible = "regulator-fixed"; > > > + regulator-name = "1V8_P"; > > > + regulator-min-microvolt = <1800000>; > > > + regulator-max-microvolt = <1800000>; > > > + vin-supply = <®_vbat>; > > > + en-supply = <®_pwr_en>; > > > > What is 'en-supply'? Just checked the v5.1-rc5 and didn't found that > > property at all. This applies to the other regulator nodes too. > > > > I was attempting to show a dependency on both supplies but I'll change the > way it is structured. > > > > + }; > > > + > > > + reg_2V8_P: regulator-2V8-P { > > > + compatible = "regulator-fixed"; > > > + regulator-name = "2V8_P"; > > > + regulator-min-microvolt = <2800000>; > > > + regulator-max-microvolt = <2800000>; > > > + vin-supply = <®_3V3_P>; > > > + }; > > > + > > > + reg_3V3_P: regulator-3V3-P { > > > + compatible = "regulator-fixed"; > > > + regulator-name = "3V3_P"; > > > + regulator-min-microvolt = <3300000>; > > > + regulator-max-microvolt = <3300000>; > > > + vin-supply = <®_vbat>; > > > + en-supply = <®_pwr_en>; > > > + }; > > > + > > > + reg_5V_P: regulator-5V-P { > > > + /* Supplies HDMI, USB hub and smartcard */ > > > + compatible = "regulator-fixed"; > > > + regulator-name = "5V_P"; > > > + regulator-min-microvolt = <5000000>; > > > + regulator-max-microvolt = <5000000>; > > > + vin-supply = <®_vbat>; > > > + en-supply = <®_pwr_en>; > > > + }; > > > + > > > + reg_5V_SOM: regulator-5V-SOM { > > > + compatible = "regulator-fixed"; > > > + regulator-name = "5V_SOM"; > > > + regulator-min-microvolt = <5000000>; > > > + regulator-max-microvolt = <5000000>; > > > + vin-supply = <®_vbat>; > > > + regulator-always-on; > > > + }; > > > + > > > + reg_22V4_P: regulator-22V4-P { > > > + compatible = "regulator-fixed"; > > > + regulator-name = "22V4_P"; > > > + regulator-min-microvolt = <22400000>; > > > + regulator-max-microvolt = <22400000>; > > > + vin-supply = <®_vbat>; > > > + }; > > > + > > > + reg_pwr_en: regulator-pwr-en { > > > + compatible = "regulator-fixed"; > > > + regulator-name = "PWR_EN"; > > > + regulator-min-microvolt = <3300000>; > > > + regulator-max-microvolt = <3300000>; > > > + gpio = <&gpio1 8 GPIO_ACTIVE_HIGH>; > > > > You missed the pinctrl-0, pinctrl-names property since you already have > > a pinctrl_pwr_en phandle. > > > > > + enable-active-high; > > > + regulator-always-on; > > > + }; > > > + > > > + reg_usdhc2_vmmc: regulator-usdhc2-vmmc { > > > + compatible = "regulator-fixed"; > > > + regulator-name = "VSD_3V3"; > > > + regulator-min-microvolt = <3300000>; > > > + regulator-max-microvolt = <3300000>; > > > + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>; > > > > IMHO you should mux (pinctrl) a gpio where you use them but that's only > > my opinion. > > > > > + enable-active-high; > > > + regulator-always-on; > > > + }; > > > + > > > + reg_vbat: regulator-vbat { > > > + compatible = "regulator-fixed"; > > > + regulator-name = "VBAT_REG"; > > > + regulator-always-on; > > > + }; > > > > A fixed regulator without a regulator-min-microvolt > > regulator-max-microvolt? > > > > That will get fixed with the issue above. > > > > + > > > + wifi_pwr_en: wifi-en { > > > + compatible = "regulator-fixed"; > > > + regulator-name = "WIFI_EN"; > > > + regulator-min-microvolt = <3300000>; > > > + regulator-max-microvolt = <3300000>; > > > + gpio = <&gpio3 5 GPIO_ACTIVE_HIGH>; > > > > Where do you mux that gpio? It seems that you use the > > pinctrl_usdhc2_gpio for muxing but there is no GPIO3 mux. > > > > I'll fix it with the regulators. > > > > + enable-active-high; > > > + regulator-always-on; > > > + }; > > > +}; > > > + > > > + > > > +&clk { > > > + assigned-clocks = <&clk IMX8MQ_AUDIO_PLL1>, <&clk > > > IMX8MQ_AUDIO_PLL2>; > > > + assigned-clock-rates = <786432000>, <722534400>; > > > +}; > > > + > > > +&fec1 { > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_fec1>; > > > + phy-mode = "rgmii-id"; > > > + phy-handle = <ðphy0>; > > > + fsl,magic-packet; > > > + status = "okay"; > > > > status should be the last property in this list. > > > > Ok. > > > > + phy-supply = <®_3V3_P>; > > > + > > > + mdio { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + ethphy0: ethernet-phy@1 { > > > > Is it necessary to name it ethphy0? Just a nitpick. > > > > > + compatible = "ethernet-phy-ieee802.3-c22"; > > > + reg = <1>; > > > + }; > > > + }; > > > +}; > > > + > > > +&i2c1 { > > > + clock-frequency = <400000>; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_i2c1>; > > > + status = "okay"; > > > + > > > + pmic: pmic@4b { > > > + reg = <0x4b>; > > > + compatible = "rohm,bd71837"; > > > > Please swap the reg and compatible property. > > > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_pmic>; > > > + clocks = <&pmic_osc>; > > > + clock-names = "osc"; > > > + clock-output-names = "pmic_clk"; > > > + interrupt-parent = <&gpio1>; > > > + interrupts = <3 GPIO_ACTIVE_LOW>; > > > + interrupt-names = "irq"; > > > + rohm,reset-snvs-powered; > > > + > > > + regulators { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > > Please drop the #address-cells, #size-cells and the @<reg> suffix > > including the reg = <> property. So it would look like: > > > > buck1_reg: BUCK1 { > > regulator-name = "buck1"; > > regulator-min-microvolt = <700000>; > > regulator-max-microvolt = <1300000>; > > regulator-boot-on; > > regulator-always-on; > > regulator-ramp-delay = <1250>; > > rohm,dvs-run-voltage = <900000>; > > rohm,dvs-idle-voltage = <850000>; > > rohm,dvs-suspend-voltage = <800000>; > > }; > > > > Please look at > > Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt. > > > > Thanks hadn't noticed the documentation change. > > > > + > > > + buck1_reg: BUCK1@0 { > > > + reg = <0>; > > > + regulator-name = "buck1"; > > > + regulator-min-microvolt = <700000>; > > > + regulator-max-microvolt = <1300000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + regulator-ramp-delay = <1250>; > > > + rohm,dvs-run-voltage = <900000>; > > > + rohm,dvs-idle-voltage = <850000>; > > > + rohm,dvs-suspend-voltage = <800000>; > > > + }; > > > + > > > + buck2_reg: BUCK2@1 { > > > + reg = <1>; > > > + regulator-name = "buck2"; > > > + regulator-min-microvolt = <700000>; > > > + regulator-max-microvolt = <1300000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + regulator-ramp-delay = <1250>; > > > + rohm,dvs-run-voltage = <1000000>; > > > + rohm,dvs-idle-voltage = <900000>; > > > + }; > > > + > > > + buck3_reg: BUCK3@2 { > > > + reg = <2>; > > > + regulator-name = "buck3"; > > > + regulator-min-microvolt = <700000>; > > > + regulator-max-microvolt = <1300000>; > > > + rohm,dvs-run-voltage = <1000000>; > > > + }; > > > + > > > + buck4_reg: BUCK4@3 { > > > + reg = <3>; > > > + regulator-name = "buck4"; > > > + regulator-min-microvolt = <700000>; > > > + regulator-max-microvolt = <1300000>; > > > + rohm,dvs-run-voltage = <1000000>; > > > + }; > > > + > > > + buck5_reg: BUCK5@4 { > > > + reg = <4>; > > > + regulator-name = "buck5"; > > > + regulator-min-microvolt = <700000>; > > > + regulator-max-microvolt = <1350000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + }; > > > + > > > + buck6_reg: BUCK6@5 { > > > + reg = <5>; > > > + regulator-name = "buck6"; > > > + regulator-min-microvolt = <3000000>; > > > + regulator-max-microvolt = <3300000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + }; > > > + > > > + buck7_reg: BUCK7@6 { > > > + reg = <6>; > > > + regulator-name = "buck7"; > > > + regulator-min-microvolt = <1605000>; > > > + regulator-max-microvolt = <1995000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + }; > > > + > > > + buck8_reg: BUCK8@7 { > > > + reg = <7>; > > > + regulator-name = "buck8"; > > > + regulator-min-microvolt = <800000>; > > > + regulator-max-microvolt = <1400000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + }; > > > + > > > + ldo1_reg: LDO1@8 { > > > + reg = <8>; > > > + regulator-name = "ldo1"; > > > + regulator-min-microvolt = <3000000>; > > > + regulator-max-microvolt = <3300000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + }; > > > + > > > + ldo2_reg: LDO2@9 { > > > + reg = <9>; > > > + regulator-name = "ldo2"; > > > + regulator-min-microvolt = <900000>; > > > + regulator-max-microvolt = <900000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + }; > > > + > > > + ldo3_reg: LDO3@10 { > > > + reg = <10>; > > > + regulator-name = "ldo3"; > > > + regulator-min-microvolt = <1800000>; > > > + regulator-max-microvolt = <3300000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + }; > > > + > > > + ldo4_reg: LDO4@11 { > > > + reg = <11>; > > > + regulator-name = "ldo4"; > > > + regulator-min-microvolt = <900000>; > > > + regulator-max-microvolt = <1800000>; > > > + regulator-boot-on; > > > + regulator-always-on; > > > + }; > > > + > > > + ldo5_reg: LDO5@12 { > > > + reg = <12>; > > > + regulator-name = "ldo5"; > > > + regulator-min-microvolt = <1800000>; > > > + regulator-max-microvolt = <3300000>; > > > + }; > > > + > > > + ldo6_reg: LDO6@13 { > > > + reg = <13>; > > > + regulator-name = "ldo6"; > > > + regulator-min-microvolt = <900000>; > > > + regulator-max-microvolt = <1800000>; > > > + }; > > > + > > > + ldo7_reg: LDO7@14 { > > > + reg = <14>; > > > + regulator-name = "ldo7"; > > > + regulator-min-microvolt = <1800000>; > > > + regulator-max-microvolt = <3300000>; > > > + }; > > > + }; > > > + }; > > > + > > > + typec_ptn5100: usb_typec@52 { > > > + compatible = "nxp,ptn5110"; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_typec>; > > > + reg = <0x52>; > > > > The reg = <> should be followed after the compatible. > > > > > + interrupt-parent = <&gpio3>; > > > + interrupts = <1 IRQ_TYPE_LEVEL_LOW>; > > > + vconn-supply = <®_5V_P>; > > > > The vconn-supply isn't documented elsewhere. > > > > Yeah that came from the schematic not the code or documentation. I wouldn't add properties which aren't supported. > > > + usb_con: connector { > > > > Is that phandle nedded? > > > > > + compatible = "usb-c-connector"; > > > + label = "USB-C"; > > > + data-role = "dual"; > > > + power-role = "dual"; > > > + try-power-role = "sink"; > > > + source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | > > > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP )>; > > > + sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | > > > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP ) > > > + PDO_VAR(5000, 12000, 2000)>; > > > > Just a nitpick but I would break the lines, e.g. > > > > source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | > > PDO_FIXED_DUAL_ROLE | > > PDO_FIXED_DATA_SWAP )>; > > sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | > > PDO_FIXED_DUAL_ROLE | > > PDO_FIXED_DATA_SWAP ) > > PDO_VAR(5000, 12000, 2000)>; > > > > > > Ok they are longer than 80 cols anyway That's true it was just a idea. You can break the lines where you want :) > > > > + op-sink-microwatt = <10000000>; > > > + ports { > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > > Blank line please. > > > > > + port@0 { > > > + reg = <0>; > > > + usb_con_hs: endpoint { > > > + remote-endpoint = <&typec_hs>; > > > + }; > > > + }; > > > > Here too. > > > > > + port@1 { > > > + reg = <1>; > > > + usb_con_ss: endpoint { > > > + remote-endpoint = <&typec_ss>; > > > + }; > > > + }; > > > + }; > > > + }; > > > + > > > + }; > > > + > > > + rtc@68 { > > > + pinctrl-names = "default"; > > > + compatible = "microcrystal,rv4162"; > > > + reg = <0x68>; > > > + pinctrl-0 = <&pinctrl_rtc>; > > > + interrupt-parent = <&gpio4>; > > > + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; > > > + }; > > > + > > > + charger: charger@6b { /* bq25896 */ > > > > Is that phandle necessary? > > > > > + compatible = "ti,bq25890"; > > > + reg = <0x6b>; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_charger>; > > > + interrupt-parent = <&gpio3>; > > > + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; > > > + ti,battery-regulation-voltage = <4192000>; /* 4.192V */ > > > + ti,charge-current = <1600000>; /* 1.6 A */ > > > + ti,termination-current = <66000>; /* 66mA */ > > > + ti,precharge-current = <1300000>; /* 1.3A */ > > > + ti,minimum-sys-voltage = <2750000>; /* 2.75V */ > > > + ti,boost-voltage = <5000000>; /* 5V */ > > > + ti,boost-max-current = <50000>; /* 50mA */ > > > > IMHO I would comment only the measurments. > > > > Sorry do you mean the units ? Yes and sorry my english wasn't the best this morning. > > I liked the additional clarity of not having to count zero's. Like you prefer. I knew cases where the comments and the values didn't matched anymore. Then it gets harder to decide which value is correct. > > > + }; > > > +}; > > > + > > > +&i2c3 { > > > + clock-frequency = <100000>; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_i2c3>, <&pinctrl_imu>; > > > > Why do you don't mux the pinctrl_imu within the magnetometer@1e node? > > > > > + status = "okay"; > > > + > > > + lsm9d_magn: magnetometer@1e { > > > > Is that phandle necessary? > > > > > + compatible = "st,lsm9ds1-magn"; > > > + reg = <0x1e>; > > > + interrupt-parent = <&gpio3>; > > > + interrupts = <19 IRQ_TYPE_LEVEL_LOW>; > > > + vdd-supply = <®_3V3_P>; > > > + vddio-supply = <®_3V3_P>; > > > + }; > > > + > > > + goodix_ts: touchscreen@5d { > > > > This here too. > > > > > + compatible = "goodix,gt5688"; > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_goodix_ts>; > > > + reg = <0x5d>; > > > > The reg = <> should be followed the compatible string. > > > > > + interrupt-parent = <&gpio3>; > > > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > > > + reset-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>; > > > + irq-gpios = <&gpio3 0 GPIO_ACTIVE_HIGH>; > > > + touchscreen-size-x = <720>; > > > + touchscreen-size-y = <1440>; > > > + AVDD28-supply = <®_2V8_P>; > > > + VDDIO-supply = <®_1V8_P>; > > > > Didn't found any of these supplies. Can you drop them? > > > > AVDD28 line 539 in drivers/input/touchscreen/goodix.c > > VDDIO line 548 in drivers/input/touchscreen/goodix.c Nope, https://elixir.bootlin.com/linux/v5.1-rc5/source/drivers/input/touchscreen/goodix.c#L539 Did you have any patches applied or are there some ongoing patches? > > > > + }; > > > +}; > > > + > > > +&iomuxc { > > > > It seems a bit inconsistent to add a comment behind the > > mux or above. I would add comments just above. > > > > My preference was for commenting after but that made some lines longer than > 80 cols so I reduced the offending lines. > > > > + pinctrl_bt: btgrp { > > > + fsl,pins = < > > > + /* nBT_DISABLE */ > > > + MX8MQ_IOMUXC_NAND_DATA05_GPIO3_IO11 0x16 > > > > As you do it here. > > > > > + /* BT_HOST_WAKE */ > > > + MX8MQ_IOMUXC_NAND_DATA01_GPIO3_IO7 0x10 > > > + >; > > > + }; > > > + > > > + pinctrl_charger: chargernirq { > > > + fsl,pins = < > > > + /* CHRG_nINT */ > > > + MX8MQ_IOMUXC_SAI5_MCLK_GPIO3_IO25 0x80 > > > + >; > > > + }; > > > + > > > + pinctrl_fec1: fec1grp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_ENET_MDC_ENET1_MDC 0x3 > > > + MX8MQ_IOMUXC_ENET_MDIO_ENET1_MDIO 0x3 > > > + MX8MQ_IOMUXC_ENET_TD3_ENET1_RGMII_TD3 0x1f > > > + MX8MQ_IOMUXC_ENET_TD2_ENET1_RGMII_TD2 0x1f > > > + MX8MQ_IOMUXC_ENET_TD1_ENET1_RGMII_TD1 0x1f > > > + MX8MQ_IOMUXC_ENET_TD0_ENET1_RGMII_TD0 0x1f > > > + MX8MQ_IOMUXC_ENET_RD3_ENET1_RGMII_RD3 0x91 > > > + MX8MQ_IOMUXC_ENET_RD2_ENET1_RGMII_RD2 0x91 > > > + MX8MQ_IOMUXC_ENET_RD1_ENET1_RGMII_RD1 0x91 > > > + MX8MQ_IOMUXC_ENET_RD0_ENET1_RGMII_RD0 0x91 > > > + MX8MQ_IOMUXC_ENET_TXC_ENET1_RGMII_TXC 0x1f > > > + MX8MQ_IOMUXC_ENET_RXC_ENET1_RGMII_RXC 0x91 > > > + MX8MQ_IOMUXC_ENET_RX_CTL_ENET1_RGMII_RX_CTL 0x91 > > > + MX8MQ_IOMUXC_ENET_TX_CTL_ENET1_RGMII_TX_CTL 0x1f > > > + MX8MQ_IOMUXC_GPIO1_IO09_GPIO1_IO9 0x19 > > > + MX8MQ_IOMUXC_GPIO1_IO15_CCMSRCGPCMIX_CLKO2 0x1f > > > + >; > > > + }; > > > + > > > + pinctrl_goodix_ts: gt5688 { > > > + fsl,pins = < > > > + /* TOUCH INT */ > > > + MX8MQ_IOMUXC_NAND_ALE_GPIO3_IO0 0x16 > > > + /* TOUCH RST */ > > > + MX8MQ_IOMUXC_GPIO1_IO05_GPIO1_IO5 0x19 > > > + >; > > > + }; > > > + > > > + pinctrl_gpio_leds: gpioleds { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_GPIO1_IO13_GPIO1_IO13 0x16 > > > + >; > > > + }; > > > + > > > + pinctrl_gpio_keys: gpiokeys { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_SAI2_RXFS_GPIO4_IO21 0x16 > > > + MX8MQ_IOMUXC_SAI2_RXC_GPIO4_IO22 0x16 > > > + /* HP_DET */ > > > + MX8MQ_IOMUXC_SAI5_RXC_GPIO3_IO20 0x180 > > > + >; > > > + }; > > > + > > > + pinctrl_haptic: hapticgrp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_SPDIF_RX_PWM2_OUT 0xc6 /* nHAPTIC */ > > > + >; > > > + }; > > > + > > > + pinctrl_i2c1: i2c1grp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_I2C1_SCL_I2C1_SCL 0x4000001f > > > + MX8MQ_IOMUXC_I2C1_SDA_I2C1_SDA 0x4000001f > > > + >; > > > + }; > > > + > > > + pinctrl_i2c3: i2c3grp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_I2C3_SCL_I2C3_SCL 0x4000001f > > > + MX8MQ_IOMUXC_I2C3_SDA_I2C3_SDA 0x4000001f > > > + >; > > > + }; > > > + > > > + pinctrl_imu: imugrp { > > > + fsl,pins = < > > > + /* IMU_INT */ > > > + MX8MQ_IOMUXC_SAI5_RXFS_GPIO3_IO19 0x8 > > > + >; > > > + }; > > > + > > > + pinctrl_pmic: pmicint { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3 0x80 > > > + >; > > > + }; > > > + > > > + pinctrl_pwr_en: pwrengrp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_GPIO1_IO08_GPIO1_IO8 0x06 > > > + >; > > > + }; > > > + > > > + pinctrl_pwm1: pwm1 { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_GPIO1_IO01_PWM1_OUT 0x6 /* DSI_BL_PWM */ > > > + >; > > > + }; > > > + > > > + pinctrl_rtc: rtcirq { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_SAI3_RXC_GPIO4_IO29 0x80 > > > + >; > > > + }; > > > + > > > + pinctrl_typec: typecgrp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_NAND_DATA06_GPIO3_IO12 0x16 > > > + MX8MQ_IOMUXC_NAND_CE0_B_GPIO3_IO1 0x80 > > > + >; > > > + }; > > > + > > > + pinctrl_uart1: uart1grp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_UART1_RXD_UART1_DCE_RX 0x49 > > > + MX8MQ_IOMUXC_UART1_TXD_UART1_DCE_TX 0x49 > > > + >; > > > + }; > > > + > > > + pinctrl_uart2: uart2grp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_UART2_TXD_UART2_DCE_TX 0x49 > > > + MX8MQ_IOMUXC_UART2_RXD_UART2_DCE_RX 0x49 > > > + MX8MQ_IOMUXC_UART4_RXD_UART2_DCE_CTS_B 0x49 > > > + MX8MQ_IOMUXC_UART4_TXD_UART2_DCE_RTS_B 0x49 > > > + >; > > > + }; > > > + > > > + pinctrl_uart3: uart3grp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_UART3_RXD_UART3_DCE_RX 0x49 > > > + MX8MQ_IOMUXC_UART3_TXD_UART3_DCE_TX 0x49 > > > + >; > > > + }; > > > + > > > + pinctrl_uart4: uart4grp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_ECSPI2_SCLK_UART4_DCE_RX 0x49 > > > + MX8MQ_IOMUXC_ECSPI2_MOSI_UART4_DCE_TX 0x49 > > > + MX8MQ_IOMUXC_ECSPI2_MISO_UART4_DCE_CTS_B 0x49 > > > + MX8MQ_IOMUXC_ECSPI2_SS0_UART4_DCE_RTS_B 0x49 > > > + MX8MQ_IOMUXC_GPIO1_IO00_ANAMIX_REF_CLK_32K 0x49 > > > + >; > > > + }; > > > + > > > + pinctrl_usdhc1: usdhc1grp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x83 > > > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc3 > > > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xc3 > > > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xc3 > > > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xc3 > > > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xc3 > > > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xc3 > > > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xc3 > > > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xc3 > > > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xc3 > > > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x83 > > > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 > > > + >; > > > + }; > > > + > > > + pinctrl_usdhc1_100mhz: usdhc1grp100mhz { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x8d > > > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xcd > > > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xcd > > > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xcd > > > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xcd > > > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xcd > > > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xcd > > > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xcd > > > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xcd > > > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xcd > > > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x8d > > > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 > > > + >; > > > + }; > > > + > > > + pinctrl_usdhc1_200mhz: usdhc1grp200mhz { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x9f > > > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xdf > > > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xdf > > > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xdf > > > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xdf > > > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xdf > > > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xdf > > > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xdf > > > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xdf > > > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xdf > > > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x9f > > > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 > > > + >; > > > + }; > > > + > > > + pinctrl_usdhc2_gpio: usdhc2grpgpio { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_SD2_RESET_B_GPIO2_IO19 0x41 > > > + /* WIFI_WAKE */ > > > + MX8MQ_IOMUXC_SD2_WP_GPIO2_IO20 0x80 > > > + >; > > > + }; > > > + > > > + pinctrl_usdhc2: usdhc2grp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x83 > > > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc3 > > > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xc3 > > > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xc3 > > > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xc3 > > > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xc3 > > > + >; > > > + }; > > > + > > > + pinctrl_usdhc2_100mhz: usdhc2grp100mhz { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x8d > > > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcd > > > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcd > > > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcd > > > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcd > > > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcd > > > + >; > > > + }; > > > + > > > + pinctrl_usdhc2_200mhz: usdhc2grp200mhz { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x9f > > > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcf > > > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcf > > > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcf > > > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcf > > > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcf > > > + >; > > > + }; > > > + > > > + pinctrl_wdog: wdoggrp { > > > + fsl,pins = < > > > + MX8MQ_IOMUXC_GPIO1_IO02_WDOG1_WDOG_B 0xc6 > > > + >; > > > + }; > > > + > > > + pinctrl_wwan: wwangrp { > > > + fsl,pins = < > > > + /* nWWAN_DISABLE */ > > > + MX8MQ_IOMUXC_NAND_CE3_B_GPIO3_IO4 0x09 > > > + /* nWoWWAN */ > > > + MX8MQ_IOMUXC_NAND_DATA02_GPIO3_IO8 0x80 > > > + /* WWAN_RESET */ > > > + MX8MQ_IOMUXC_NAND_DATA03_GPIO3_IO9 0x19 > > > + >; > > > + }; > > > + > > > +}; > > > + > > > +&pwm1 { > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_pwm1>; > > > + status = "okay"; > > > +}; > > > + > > > +&pwm2 { > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_haptic>; > > > > Just a nitpick but I would name it pinctrl_pwm2 as you did it for pwm1. > > > > I figured haptic was more descriptive. Descriptive for the pwm node? If that is the case the pwm1 node should be called backlight. Anyway as I said it's just a nitpick. > > > > + status = "okay"; > > > +}; > > > + > > > +&uart1 { /* console */ > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_uart1>; > > > + status = "okay"; > > > +}; > > > + > > > +&uart3 { /* GNSS */ > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_uart3>; > > > + status = "okay"; > > > +}; > > > + > > > +&uart4 { /* BT */ > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_uart4>, <&pinctrl_bt>; > > > + uart-has-rtscts; > > > + status = "okay"; > > > +}; > > > + > > > +&usb3_phy0 { > > > + status = "okay"; > > > > Should be the last property. > > > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > > Blank line. > > > + port@0 { > > > + reg = <0>; > > > > and here > > > + typec_hs: endpoint { > > > + remote-endpoint = <&usb_con_hs>; > > > + }; > > > + }; > > > > here > > > > > + port@1 { > > > + reg = <1>; > > > > and here. > > > > > + typec_ss: endpoint { > > > + remote-endpoint = <&usb_con_ss>; > > > + }; > > > + }; > > > +}; > > > + > > > +&usb3_phy1 { > > > + status = "okay"; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > > Why did you assign the #address-cells and #size-cells without specify > > any port? Status should be the last property. > > > > Cut and paste from phy0. > > > > +}; > > > + > > > +&usb_dwc3_0 { > > > + status = "okay"; > > > > Here also. > > > > > + extcon = <&typec_ptn5100>; > > > + dr_mode = "otg"; > > > +}; > > > + > > > +&usb_dwc3_1 { > > > + status = "okay"; > > > > And here. > > > > > + dr_mode = "host"; > > > +}; > > > + > > > +&usdhc1 { > > > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > > > + pinctrl-0 = <&pinctrl_usdhc1>; > > > + pinctrl-1 = <&pinctrl_usdhc1_100mhz>; > > > + pinctrl-2 = <&pinctrl_usdhc1_200mhz>; > > > + bus-width = <8>; > > > + non-removable; > > > + status = "okay"; > > > +}; > > > + > > > +&usdhc2 { > > > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > > > + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; > > > > You can drop the pinctrl_usdhc2_gpio handle if you mux them within the > > regulator node. > > Ok > > > > > > + pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>; > > > + pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>; > > > + bus-width = <4>; > > > + vmmc-supply = <®_usdhc2_vmmc>; > > > + power-supply = <&wifi_pwr_en>; > > > + > > > + non-removable; > > > + disable-wp; > > > + cap-sdio-irq; > > > + keep-power-in-suspend; > > > + wakeup-source; > > > + status = "okay"; > > > +}; > > > + > > > +&wdog1 { > > > + pinctrl-names = "default"; > > > + pinctrl-0 = <&pinctrl_wdog>; > > > + fsl,ext-reset-output; > > > + status = "okay"; > > > +}; > > > -- > > > 2.17.1 > > > > > > > So now I finished, sorry for don't cover that in your v3. > > > > Regards, > > Marco > > Thanks > Angus Regards, Marco > > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit 2019-04-16 21:20 ` Marco Felsch @ 2019-04-16 22:36 ` Angus Ainslie 2019-04-17 6:54 ` Marco Felsch 0 siblings, 1 reply; 11+ messages in thread From: Angus Ainslie @ 2019-04-16 22:36 UTC (permalink / raw) To: Marco Felsch Cc: Mark Rutland, devicetree, Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel Hi Marco, On 2019-04-16 15:20, Marco Felsch wrote: > Hi Angus, > > On 19-04-16 11:02, Angus Ainslie wrote: >> Hi Marco, >> >> On 2019-04-16 00:54, Marco Felsch wrote: >> > Hi Agnus, >> > >> >> Just a nitpick but could you spell my name correctly :) > > Sorry it was to early that morning and I haven't had my first coffee. > >> > On 19-04-15 16:53, Angus Ainslie (Purism) wrote: >> > > This is the development kit board for the Librem 5. The current >> > > level of >> > > support yields a working console and is able to boot userspace from >> > > the >> > > Network or eMMC. >> > > >> > > Additional subsystems that are active : >> > > >> > > - Both USB ports >> > > - SD card socket >> > > - WiFi usdhc >> > > - WWAN modem >> > > - GNSS >> > > - GPIO keys >> > > - LEDs >> > > - gyro >> > > - magnetometer >> > > - touchscreen >> > > - pwm >> > > - backlight >> > > - haptic motor >> > > >> > > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> >> > > --- >> > > arch/arm64/boot/dts/freescale/Makefile | 1 + >> > > .../dts/freescale/imx8mq-librem5-devkit.dts | 822 >> > > ++++++++++++++++++ >> > > 2 files changed, 823 insertions(+) >> > > create mode 100644 >> > > arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts >> > > >> > > diff --git a/arch/arm64/boot/dts/freescale/Makefile >> > > b/arch/arm64/boot/dts/freescale/Makefile >> > > index 0bd122f60549..c043aca66572 100644 >> > > --- a/arch/arm64/boot/dts/freescale/Makefile >> > > +++ b/arch/arm64/boot/dts/freescale/Makefile >> > > @@ -22,6 +22,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb >> > > >> > > dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb >> > > dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb >> > > +dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb >> > > dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb >> > > dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb >> > > dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb >> > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts >> > > b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts >> > > new file mode 100644 >> > > index 000000000000..dd705b6b94a4 >> > > --- /dev/null >> > > +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts >> > > @@ -0,0 +1,822 @@ >> > > +/* SPDX-License-Identifier: GPL-2.0+ >> > > + * >> > > + * Copyright 2018-2019 Purism SPC >> > > + */ >> > > + >> > > +/dts-v1/; >> > > + >> > > +#include "imx8mq.dtsi" >> > > +#include "dt-bindings/usb/pd.h" >> > > +#include "dt-bindings/input/input.h" >> > >> > Just a nitpick but this should be alphabetical too. >> > >> >> Ok. >> >> > > + >> > > +/ { >> > > + model = "Purism Librem 5 devkit"; >> > > + compatible = "purism,librem5-devkit", "fsl,imx8mq"; >> > > + >> > > + backlight_dsi: backlight-dsi { >> > > + compatible = "pwm-backlight"; >> > > + /* 200 Hz for the PAM2841 */ >> > > + pwms = <&pwm1 0 5000000>; >> > > + brightness-levels = <0 100>; >> > > + num-interpolated-steps = <100>; >> > > + /* Default brightness level (index into the array defined by */ >> > > + /* the "brightness-levels" property) */ >> > > + default-brightness-level = <0>; >> > > + power-supply = <®_22V4_P>; >> > > + }; >> > > + >> > > + chosen { >> > > + stdout-path = &uart1; >> > > + }; >> > > + >> > > + gpio-keys { >> > > + compatible = "gpio-keys"; >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_gpio_keys>; >> > > + >> > > + btn1 { >> > > + label = "VOL_UP"; >> > > + gpios = <&gpio4 21 GPIO_ACTIVE_LOW>; >> > > + gpio-key,wakeup; >> > > + linux,code = <KEY_VOLUMEUP>; >> > > + }; >> > > + >> > > + btn2 { >> > > + label = "VOL_DOWN"; >> > > + gpios = <&gpio4 22 GPIO_ACTIVE_LOW>; >> > > + gpio-key,wakeup; >> > > + linux,code = <KEY_VOLUMEDOWN>; >> > > + }; >> > > + >> > > + hp_det { >> > > + label = "HP_DET"; >> > > + gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; >> > > + gpio-key,wakeup; >> > > + linux,code = <KEY_HP>; >> > > + }; >> > > + }; >> > > + >> > > + leds { >> > > + compatible = "gpio-leds"; >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_gpio_leds>; >> > > + status = "okay"; >> > >> > You can drop the status since the default is "okay". >> > >> >> Ok. >> >> > > + >> > > + led1 { >> > > + label = "LED 1"; >> > > + gpios = <&gpio1 13 GPIO_ACTIVE_HIGH>; >> > > + default-state = "off"; >> > > + }; >> > > + }; >> > > + >> > > + pmic_osc: pmic-osc { >> > > + compatible = "fixed-clock"; >> > > + #clock-cells = <0>; >> > > + clock-frequency = <32768>; >> > > + clock-output-names = "pmic_osc"; >> > > + }; >> > > + >> > > + pwmleds { >> > > + compatible = "pwm-leds"; >> > >> > Can you add a blank line? It's just a nitpick. >> > >> >> Sure. >> >> > > + haptic { >> > > + label = "librem5::haptic"; >> > > + pwms = <&pwm2 0 200000>; >> > > + active-low; >> > > + max-brightness = <255>; >> > > + power-supply = <®_3V3_P>; >> > > + }; >> > > + }; >> > > + >> > > + reg_1V8_P: regulator-1V8-P { >> > >> > I don't know the policy but I would name the nodes and phandles always >> > in lower case. >> > >> >> Ok. >> >> > > + compatible = "regulator-fixed"; >> > > + regulator-name = "1V8_P"; >> > > + regulator-min-microvolt = <1800000>; >> > > + regulator-max-microvolt = <1800000>; >> > > + vin-supply = <®_vbat>; >> > > + en-supply = <®_pwr_en>; >> > >> > What is 'en-supply'? Just checked the v5.1-rc5 and didn't found that >> > property at all. This applies to the other regulator nodes too. >> > >> >> I was attempting to show a dependency on both supplies but I'll change >> the >> way it is structured. >> >> > > + }; >> > > + >> > > + reg_2V8_P: regulator-2V8-P { >> > > + compatible = "regulator-fixed"; >> > > + regulator-name = "2V8_P"; >> > > + regulator-min-microvolt = <2800000>; >> > > + regulator-max-microvolt = <2800000>; >> > > + vin-supply = <®_3V3_P>; >> > > + }; >> > > + >> > > + reg_3V3_P: regulator-3V3-P { >> > > + compatible = "regulator-fixed"; >> > > + regulator-name = "3V3_P"; >> > > + regulator-min-microvolt = <3300000>; >> > > + regulator-max-microvolt = <3300000>; >> > > + vin-supply = <®_vbat>; >> > > + en-supply = <®_pwr_en>; >> > > + }; >> > > + >> > > + reg_5V_P: regulator-5V-P { >> > > + /* Supplies HDMI, USB hub and smartcard */ >> > > + compatible = "regulator-fixed"; >> > > + regulator-name = "5V_P"; >> > > + regulator-min-microvolt = <5000000>; >> > > + regulator-max-microvolt = <5000000>; >> > > + vin-supply = <®_vbat>; >> > > + en-supply = <®_pwr_en>; >> > > + }; >> > > + >> > > + reg_5V_SOM: regulator-5V-SOM { >> > > + compatible = "regulator-fixed"; >> > > + regulator-name = "5V_SOM"; >> > > + regulator-min-microvolt = <5000000>; >> > > + regulator-max-microvolt = <5000000>; >> > > + vin-supply = <®_vbat>; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + reg_22V4_P: regulator-22V4-P { >> > > + compatible = "regulator-fixed"; >> > > + regulator-name = "22V4_P"; >> > > + regulator-min-microvolt = <22400000>; >> > > + regulator-max-microvolt = <22400000>; >> > > + vin-supply = <®_vbat>; >> > > + }; >> > > + >> > > + reg_pwr_en: regulator-pwr-en { >> > > + compatible = "regulator-fixed"; >> > > + regulator-name = "PWR_EN"; >> > > + regulator-min-microvolt = <3300000>; >> > > + regulator-max-microvolt = <3300000>; >> > > + gpio = <&gpio1 8 GPIO_ACTIVE_HIGH>; >> > >> > You missed the pinctrl-0, pinctrl-names property since you already have >> > a pinctrl_pwr_en phandle. >> > >> > > + enable-active-high; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + reg_usdhc2_vmmc: regulator-usdhc2-vmmc { >> > > + compatible = "regulator-fixed"; >> > > + regulator-name = "VSD_3V3"; >> > > + regulator-min-microvolt = <3300000>; >> > > + regulator-max-microvolt = <3300000>; >> > > + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>; >> > >> > IMHO you should mux (pinctrl) a gpio where you use them but that's only >> > my opinion. >> > >> > > + enable-active-high; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + reg_vbat: regulator-vbat { >> > > + compatible = "regulator-fixed"; >> > > + regulator-name = "VBAT_REG"; >> > > + regulator-always-on; >> > > + }; >> > >> > A fixed regulator without a regulator-min-microvolt >> > regulator-max-microvolt? >> > >> >> That will get fixed with the issue above. >> >> > > + >> > > + wifi_pwr_en: wifi-en { >> > > + compatible = "regulator-fixed"; >> > > + regulator-name = "WIFI_EN"; >> > > + regulator-min-microvolt = <3300000>; >> > > + regulator-max-microvolt = <3300000>; >> > > + gpio = <&gpio3 5 GPIO_ACTIVE_HIGH>; >> > >> > Where do you mux that gpio? It seems that you use the >> > pinctrl_usdhc2_gpio for muxing but there is no GPIO3 mux. >> > >> >> I'll fix it with the regulators. >> >> > > + enable-active-high; >> > > + regulator-always-on; >> > > + }; >> > > +}; >> > > + >> > > + >> > > +&clk { >> > > + assigned-clocks = <&clk IMX8MQ_AUDIO_PLL1>, <&clk >> > > IMX8MQ_AUDIO_PLL2>; >> > > + assigned-clock-rates = <786432000>, <722534400>; >> > > +}; >> > > + >> > > +&fec1 { >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_fec1>; >> > > + phy-mode = "rgmii-id"; >> > > + phy-handle = <ðphy0>; >> > > + fsl,magic-packet; >> > > + status = "okay"; >> > >> > status should be the last property in this list. >> > >> >> Ok. >> >> > > + phy-supply = <®_3V3_P>; >> > > + >> > > + mdio { >> > > + #address-cells = <1>; >> > > + #size-cells = <0>; >> > > + >> > > + ethphy0: ethernet-phy@1 { >> > >> > Is it necessary to name it ethphy0? Just a nitpick. >> > >> > > + compatible = "ethernet-phy-ieee802.3-c22"; >> > > + reg = <1>; >> > > + }; >> > > + }; >> > > +}; >> > > + >> > > +&i2c1 { >> > > + clock-frequency = <400000>; >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_i2c1>; >> > > + status = "okay"; >> > > + >> > > + pmic: pmic@4b { >> > > + reg = <0x4b>; >> > > + compatible = "rohm,bd71837"; >> > >> > Please swap the reg and compatible property. >> > >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_pmic>; >> > > + clocks = <&pmic_osc>; >> > > + clock-names = "osc"; >> > > + clock-output-names = "pmic_clk"; >> > > + interrupt-parent = <&gpio1>; >> > > + interrupts = <3 GPIO_ACTIVE_LOW>; >> > > + interrupt-names = "irq"; >> > > + rohm,reset-snvs-powered; >> > > + >> > > + regulators { >> > > + #address-cells = <1>; >> > > + #size-cells = <0>; >> > >> > Please drop the #address-cells, #size-cells and the @<reg> suffix >> > including the reg = <> property. So it would look like: >> > >> > buck1_reg: BUCK1 { >> > regulator-name = "buck1"; >> > regulator-min-microvolt = <700000>; >> > regulator-max-microvolt = <1300000>; >> > regulator-boot-on; >> > regulator-always-on; >> > regulator-ramp-delay = <1250>; >> > rohm,dvs-run-voltage = <900000>; >> > rohm,dvs-idle-voltage = <850000>; >> > rohm,dvs-suspend-voltage = <800000>; >> > }; >> > >> > Please look at >> > Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt. >> > >> >> Thanks hadn't noticed the documentation change. >> >> > > + >> > > + buck1_reg: BUCK1@0 { >> > > + reg = <0>; >> > > + regulator-name = "buck1"; >> > > + regulator-min-microvolt = <700000>; >> > > + regulator-max-microvolt = <1300000>; >> > > + regulator-boot-on; >> > > + regulator-always-on; >> > > + regulator-ramp-delay = <1250>; >> > > + rohm,dvs-run-voltage = <900000>; >> > > + rohm,dvs-idle-voltage = <850000>; >> > > + rohm,dvs-suspend-voltage = <800000>; >> > > + }; >> > > + >> > > + buck2_reg: BUCK2@1 { >> > > + reg = <1>; >> > > + regulator-name = "buck2"; >> > > + regulator-min-microvolt = <700000>; >> > > + regulator-max-microvolt = <1300000>; >> > > + regulator-boot-on; >> > > + regulator-always-on; >> > > + regulator-ramp-delay = <1250>; >> > > + rohm,dvs-run-voltage = <1000000>; >> > > + rohm,dvs-idle-voltage = <900000>; >> > > + }; >> > > + >> > > + buck3_reg: BUCK3@2 { >> > > + reg = <2>; >> > > + regulator-name = "buck3"; >> > > + regulator-min-microvolt = <700000>; >> > > + regulator-max-microvolt = <1300000>; >> > > + rohm,dvs-run-voltage = <1000000>; >> > > + }; >> > > + >> > > + buck4_reg: BUCK4@3 { >> > > + reg = <3>; >> > > + regulator-name = "buck4"; >> > > + regulator-min-microvolt = <700000>; >> > > + regulator-max-microvolt = <1300000>; >> > > + rohm,dvs-run-voltage = <1000000>; >> > > + }; >> > > + >> > > + buck5_reg: BUCK5@4 { >> > > + reg = <4>; >> > > + regulator-name = "buck5"; >> > > + regulator-min-microvolt = <700000>; >> > > + regulator-max-microvolt = <1350000>; >> > > + regulator-boot-on; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + buck6_reg: BUCK6@5 { >> > > + reg = <5>; >> > > + regulator-name = "buck6"; >> > > + regulator-min-microvolt = <3000000>; >> > > + regulator-max-microvolt = <3300000>; >> > > + regulator-boot-on; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + buck7_reg: BUCK7@6 { >> > > + reg = <6>; >> > > + regulator-name = "buck7"; >> > > + regulator-min-microvolt = <1605000>; >> > > + regulator-max-microvolt = <1995000>; >> > > + regulator-boot-on; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + buck8_reg: BUCK8@7 { >> > > + reg = <7>; >> > > + regulator-name = "buck8"; >> > > + regulator-min-microvolt = <800000>; >> > > + regulator-max-microvolt = <1400000>; >> > > + regulator-boot-on; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + ldo1_reg: LDO1@8 { >> > > + reg = <8>; >> > > + regulator-name = "ldo1"; >> > > + regulator-min-microvolt = <3000000>; >> > > + regulator-max-microvolt = <3300000>; >> > > + regulator-boot-on; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + ldo2_reg: LDO2@9 { >> > > + reg = <9>; >> > > + regulator-name = "ldo2"; >> > > + regulator-min-microvolt = <900000>; >> > > + regulator-max-microvolt = <900000>; >> > > + regulator-boot-on; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + ldo3_reg: LDO3@10 { >> > > + reg = <10>; >> > > + regulator-name = "ldo3"; >> > > + regulator-min-microvolt = <1800000>; >> > > + regulator-max-microvolt = <3300000>; >> > > + regulator-boot-on; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + ldo4_reg: LDO4@11 { >> > > + reg = <11>; >> > > + regulator-name = "ldo4"; >> > > + regulator-min-microvolt = <900000>; >> > > + regulator-max-microvolt = <1800000>; >> > > + regulator-boot-on; >> > > + regulator-always-on; >> > > + }; >> > > + >> > > + ldo5_reg: LDO5@12 { >> > > + reg = <12>; >> > > + regulator-name = "ldo5"; >> > > + regulator-min-microvolt = <1800000>; >> > > + regulator-max-microvolt = <3300000>; >> > > + }; >> > > + >> > > + ldo6_reg: LDO6@13 { >> > > + reg = <13>; >> > > + regulator-name = "ldo6"; >> > > + regulator-min-microvolt = <900000>; >> > > + regulator-max-microvolt = <1800000>; >> > > + }; >> > > + >> > > + ldo7_reg: LDO7@14 { >> > > + reg = <14>; >> > > + regulator-name = "ldo7"; >> > > + regulator-min-microvolt = <1800000>; >> > > + regulator-max-microvolt = <3300000>; >> > > + }; >> > > + }; >> > > + }; >> > > + >> > > + typec_ptn5100: usb_typec@52 { >> > > + compatible = "nxp,ptn5110"; >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_typec>; >> > > + reg = <0x52>; >> > >> > The reg = <> should be followed after the compatible. >> > >> > > + interrupt-parent = <&gpio3>; >> > > + interrupts = <1 IRQ_TYPE_LEVEL_LOW>; >> > > + vconn-supply = <®_5V_P>; >> > >> > The vconn-supply isn't documented elsewhere. >> > >> >> Yeah that came from the schematic not the code or documentation. > > I wouldn't add properties which aren't supported. > Agreed >> > > + usb_con: connector { >> > >> > Is that phandle nedded? >> > >> > > + compatible = "usb-c-connector"; >> > > + label = "USB-C"; >> > > + data-role = "dual"; >> > > + power-role = "dual"; >> > > + try-power-role = "sink"; >> > > + source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | >> > > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP )>; >> > > + sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | >> > > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP ) >> > > + PDO_VAR(5000, 12000, 2000)>; >> > >> > Just a nitpick but I would break the lines, e.g. >> > >> > source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | >> > PDO_FIXED_DUAL_ROLE | >> > PDO_FIXED_DATA_SWAP )>; >> > sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | >> > PDO_FIXED_DUAL_ROLE | >> > PDO_FIXED_DATA_SWAP ) >> > PDO_VAR(5000, 12000, 2000)>; >> > >> > >> >> Ok they are longer than 80 cols anyway > > That's true it was just a idea. You can break the lines where you want > :) > >> >> > > + op-sink-microwatt = <10000000>; >> > > + ports { >> > > + #address-cells = <1>; >> > > + #size-cells = <0>; >> > >> > Blank line please. >> > >> > > + port@0 { >> > > + reg = <0>; >> > > + usb_con_hs: endpoint { >> > > + remote-endpoint = <&typec_hs>; >> > > + }; >> > > + }; >> > >> > Here too. >> > >> > > + port@1 { >> > > + reg = <1>; >> > > + usb_con_ss: endpoint { >> > > + remote-endpoint = <&typec_ss>; >> > > + }; >> > > + }; >> > > + }; >> > > + }; >> > > + >> > > + }; >> > > + >> > > + rtc@68 { >> > > + pinctrl-names = "default"; >> > > + compatible = "microcrystal,rv4162"; >> > > + reg = <0x68>; >> > > + pinctrl-0 = <&pinctrl_rtc>; >> > > + interrupt-parent = <&gpio4>; >> > > + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; >> > > + }; >> > > + >> > > + charger: charger@6b { /* bq25896 */ >> > >> > Is that phandle necessary? >> > >> > > + compatible = "ti,bq25890"; >> > > + reg = <0x6b>; >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_charger>; >> > > + interrupt-parent = <&gpio3>; >> > > + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; >> > > + ti,battery-regulation-voltage = <4192000>; /* 4.192V */ >> > > + ti,charge-current = <1600000>; /* 1.6 A */ >> > > + ti,termination-current = <66000>; /* 66mA */ >> > > + ti,precharge-current = <1300000>; /* 1.3A */ >> > > + ti,minimum-sys-voltage = <2750000>; /* 2.75V */ >> > > + ti,boost-voltage = <5000000>; /* 5V */ >> > > + ti,boost-max-current = <50000>; /* 50mA */ >> > >> > IMHO I would comment only the measurments. >> > >> >> Sorry do you mean the units ? > > Yes and sorry my english wasn't the best this morning. > Sure no problem, I just wanted to be sure I understood. >> >> I liked the additional clarity of not having to count zero's. > > Like you prefer. I knew cases where the comments and the values didn't > matched anymore. Then it gets harder to decide which value is correct. > Yes they should be kept in sync. >> > > + }; >> > > +}; >> > > + >> > > +&i2c3 { >> > > + clock-frequency = <100000>; >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_i2c3>, <&pinctrl_imu>; >> > >> > Why do you don't mux the pinctrl_imu within the magnetometer@1e node? >> > >> > > + status = "okay"; >> > > + >> > > + lsm9d_magn: magnetometer@1e { >> > >> > Is that phandle necessary? >> > >> > > + compatible = "st,lsm9ds1-magn"; >> > > + reg = <0x1e>; >> > > + interrupt-parent = <&gpio3>; >> > > + interrupts = <19 IRQ_TYPE_LEVEL_LOW>; >> > > + vdd-supply = <®_3V3_P>; >> > > + vddio-supply = <®_3V3_P>; >> > > + }; >> > > + >> > > + goodix_ts: touchscreen@5d { >> > >> > This here too. >> > >> > > + compatible = "goodix,gt5688"; >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_goodix_ts>; >> > > + reg = <0x5d>; >> > >> > The reg = <> should be followed the compatible string. >> > >> > > + interrupt-parent = <&gpio3>; >> > > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; >> > > + reset-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>; >> > > + irq-gpios = <&gpio3 0 GPIO_ACTIVE_HIGH>; >> > > + touchscreen-size-x = <720>; >> > > + touchscreen-size-y = <1440>; >> > > + AVDD28-supply = <®_2V8_P>; >> > > + VDDIO-supply = <®_1V8_P>; >> > >> > Didn't found any of these supplies. Can you drop them? >> > >> >> AVDD28 line 539 in drivers/input/touchscreen/goodix.c >> >> VDDIO line 548 in drivers/input/touchscreen/goodix.c > > Nope, > https://elixir.bootlin.com/linux/v5.1-rc5/source/drivers/input/touchscreen/goodix.c#L539 > > Did you have any patches applied or are there some ongoing patches? > This is based on linux-next next-20190415 https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/touchscreen/goodix.c?h=next-20190412#n539 https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/touchscreen/goodix.c?h=next-20190412#n548 >> >> > > + }; >> > > +}; >> > > + >> > > +&iomuxc { >> > >> > It seems a bit inconsistent to add a comment behind the >> > mux or above. I would add comments just above. >> > >> >> My preference was for commenting after but that made some lines longer >> than >> 80 cols so I reduced the offending lines. >> >> > > + pinctrl_bt: btgrp { >> > > + fsl,pins = < >> > > + /* nBT_DISABLE */ >> > > + MX8MQ_IOMUXC_NAND_DATA05_GPIO3_IO11 0x16 >> > >> > As you do it here. >> > >> > > + /* BT_HOST_WAKE */ >> > > + MX8MQ_IOMUXC_NAND_DATA01_GPIO3_IO7 0x10 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_charger: chargernirq { >> > > + fsl,pins = < >> > > + /* CHRG_nINT */ >> > > + MX8MQ_IOMUXC_SAI5_MCLK_GPIO3_IO25 0x80 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_fec1: fec1grp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_ENET_MDC_ENET1_MDC 0x3 >> > > + MX8MQ_IOMUXC_ENET_MDIO_ENET1_MDIO 0x3 >> > > + MX8MQ_IOMUXC_ENET_TD3_ENET1_RGMII_TD3 0x1f >> > > + MX8MQ_IOMUXC_ENET_TD2_ENET1_RGMII_TD2 0x1f >> > > + MX8MQ_IOMUXC_ENET_TD1_ENET1_RGMII_TD1 0x1f >> > > + MX8MQ_IOMUXC_ENET_TD0_ENET1_RGMII_TD0 0x1f >> > > + MX8MQ_IOMUXC_ENET_RD3_ENET1_RGMII_RD3 0x91 >> > > + MX8MQ_IOMUXC_ENET_RD2_ENET1_RGMII_RD2 0x91 >> > > + MX8MQ_IOMUXC_ENET_RD1_ENET1_RGMII_RD1 0x91 >> > > + MX8MQ_IOMUXC_ENET_RD0_ENET1_RGMII_RD0 0x91 >> > > + MX8MQ_IOMUXC_ENET_TXC_ENET1_RGMII_TXC 0x1f >> > > + MX8MQ_IOMUXC_ENET_RXC_ENET1_RGMII_RXC 0x91 >> > > + MX8MQ_IOMUXC_ENET_RX_CTL_ENET1_RGMII_RX_CTL 0x91 >> > > + MX8MQ_IOMUXC_ENET_TX_CTL_ENET1_RGMII_TX_CTL 0x1f >> > > + MX8MQ_IOMUXC_GPIO1_IO09_GPIO1_IO9 0x19 >> > > + MX8MQ_IOMUXC_GPIO1_IO15_CCMSRCGPCMIX_CLKO2 0x1f >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_goodix_ts: gt5688 { >> > > + fsl,pins = < >> > > + /* TOUCH INT */ >> > > + MX8MQ_IOMUXC_NAND_ALE_GPIO3_IO0 0x16 >> > > + /* TOUCH RST */ >> > > + MX8MQ_IOMUXC_GPIO1_IO05_GPIO1_IO5 0x19 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_gpio_leds: gpioleds { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_GPIO1_IO13_GPIO1_IO13 0x16 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_gpio_keys: gpiokeys { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_SAI2_RXFS_GPIO4_IO21 0x16 >> > > + MX8MQ_IOMUXC_SAI2_RXC_GPIO4_IO22 0x16 >> > > + /* HP_DET */ >> > > + MX8MQ_IOMUXC_SAI5_RXC_GPIO3_IO20 0x180 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_haptic: hapticgrp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_SPDIF_RX_PWM2_OUT 0xc6 /* nHAPTIC */ >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_i2c1: i2c1grp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_I2C1_SCL_I2C1_SCL 0x4000001f >> > > + MX8MQ_IOMUXC_I2C1_SDA_I2C1_SDA 0x4000001f >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_i2c3: i2c3grp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_I2C3_SCL_I2C3_SCL 0x4000001f >> > > + MX8MQ_IOMUXC_I2C3_SDA_I2C3_SDA 0x4000001f >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_imu: imugrp { >> > > + fsl,pins = < >> > > + /* IMU_INT */ >> > > + MX8MQ_IOMUXC_SAI5_RXFS_GPIO3_IO19 0x8 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_pmic: pmicint { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3 0x80 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_pwr_en: pwrengrp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_GPIO1_IO08_GPIO1_IO8 0x06 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_pwm1: pwm1 { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_GPIO1_IO01_PWM1_OUT 0x6 /* DSI_BL_PWM */ >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_rtc: rtcirq { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_SAI3_RXC_GPIO4_IO29 0x80 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_typec: typecgrp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_NAND_DATA06_GPIO3_IO12 0x16 >> > > + MX8MQ_IOMUXC_NAND_CE0_B_GPIO3_IO1 0x80 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_uart1: uart1grp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_UART1_RXD_UART1_DCE_RX 0x49 >> > > + MX8MQ_IOMUXC_UART1_TXD_UART1_DCE_TX 0x49 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_uart2: uart2grp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_UART2_TXD_UART2_DCE_TX 0x49 >> > > + MX8MQ_IOMUXC_UART2_RXD_UART2_DCE_RX 0x49 >> > > + MX8MQ_IOMUXC_UART4_RXD_UART2_DCE_CTS_B 0x49 >> > > + MX8MQ_IOMUXC_UART4_TXD_UART2_DCE_RTS_B 0x49 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_uart3: uart3grp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_UART3_RXD_UART3_DCE_RX 0x49 >> > > + MX8MQ_IOMUXC_UART3_TXD_UART3_DCE_TX 0x49 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_uart4: uart4grp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_ECSPI2_SCLK_UART4_DCE_RX 0x49 >> > > + MX8MQ_IOMUXC_ECSPI2_MOSI_UART4_DCE_TX 0x49 >> > > + MX8MQ_IOMUXC_ECSPI2_MISO_UART4_DCE_CTS_B 0x49 >> > > + MX8MQ_IOMUXC_ECSPI2_SS0_UART4_DCE_RTS_B 0x49 >> > > + MX8MQ_IOMUXC_GPIO1_IO00_ANAMIX_REF_CLK_32K 0x49 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_usdhc1: usdhc1grp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x83 >> > > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc3 >> > > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xc3 >> > > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xc3 >> > > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xc3 >> > > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xc3 >> > > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xc3 >> > > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xc3 >> > > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xc3 >> > > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xc3 >> > > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x83 >> > > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_usdhc1_100mhz: usdhc1grp100mhz { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x8d >> > > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xcd >> > > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xcd >> > > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xcd >> > > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xcd >> > > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xcd >> > > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xcd >> > > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xcd >> > > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xcd >> > > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xcd >> > > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x8d >> > > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_usdhc1_200mhz: usdhc1grp200mhz { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x9f >> > > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xdf >> > > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xdf >> > > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xdf >> > > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xdf >> > > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xdf >> > > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xdf >> > > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xdf >> > > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xdf >> > > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xdf >> > > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x9f >> > > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_usdhc2_gpio: usdhc2grpgpio { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_SD2_RESET_B_GPIO2_IO19 0x41 >> > > + /* WIFI_WAKE */ >> > > + MX8MQ_IOMUXC_SD2_WP_GPIO2_IO20 0x80 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_usdhc2: usdhc2grp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x83 >> > > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc3 >> > > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xc3 >> > > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xc3 >> > > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xc3 >> > > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xc3 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_usdhc2_100mhz: usdhc2grp100mhz { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x8d >> > > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcd >> > > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcd >> > > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcd >> > > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcd >> > > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcd >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_usdhc2_200mhz: usdhc2grp200mhz { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x9f >> > > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcf >> > > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcf >> > > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcf >> > > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcf >> > > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcf >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_wdog: wdoggrp { >> > > + fsl,pins = < >> > > + MX8MQ_IOMUXC_GPIO1_IO02_WDOG1_WDOG_B 0xc6 >> > > + >; >> > > + }; >> > > + >> > > + pinctrl_wwan: wwangrp { >> > > + fsl,pins = < >> > > + /* nWWAN_DISABLE */ >> > > + MX8MQ_IOMUXC_NAND_CE3_B_GPIO3_IO4 0x09 >> > > + /* nWoWWAN */ >> > > + MX8MQ_IOMUXC_NAND_DATA02_GPIO3_IO8 0x80 >> > > + /* WWAN_RESET */ >> > > + MX8MQ_IOMUXC_NAND_DATA03_GPIO3_IO9 0x19 >> > > + >; >> > > + }; >> > > + >> > > +}; >> > > + >> > > +&pwm1 { >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_pwm1>; >> > > + status = "okay"; >> > > +}; >> > > + >> > > +&pwm2 { >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_haptic>; >> > >> > Just a nitpick but I would name it pinctrl_pwm2 as you did it for pwm1. >> > >> >> I figured haptic was more descriptive. > > Descriptive for the pwm node? If that is the case the pwm1 node should > be > called backlight. Anyway as I said it's just a nitpick. > >> >> > > + status = "okay"; >> > > +}; >> > > + >> > > +&uart1 { /* console */ >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_uart1>; >> > > + status = "okay"; >> > > +}; >> > > + >> > > +&uart3 { /* GNSS */ >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_uart3>; >> > > + status = "okay"; >> > > +}; >> > > + >> > > +&uart4 { /* BT */ >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_uart4>, <&pinctrl_bt>; >> > > + uart-has-rtscts; >> > > + status = "okay"; >> > > +}; >> > > + >> > > +&usb3_phy0 { >> > > + status = "okay"; >> > >> > Should be the last property. >> > >> > > + #address-cells = <1>; >> > > + #size-cells = <0>; >> > >> > Blank line. >> > > + port@0 { >> > > + reg = <0>; >> > >> > and here >> > > + typec_hs: endpoint { >> > > + remote-endpoint = <&usb_con_hs>; >> > > + }; >> > > + }; >> > >> > here >> > >> > > + port@1 { >> > > + reg = <1>; >> > >> > and here. >> > >> > > + typec_ss: endpoint { >> > > + remote-endpoint = <&usb_con_ss>; >> > > + }; >> > > + }; >> > > +}; >> > > + >> > > +&usb3_phy1 { >> > > + status = "okay"; >> > > + #address-cells = <1>; >> > > + #size-cells = <0>; >> > >> > Why did you assign the #address-cells and #size-cells without specify >> > any port? Status should be the last property. >> > >> >> Cut and paste from phy0. >> >> > > +}; >> > > + >> > > +&usb_dwc3_0 { >> > > + status = "okay"; >> > >> > Here also. >> > >> > > + extcon = <&typec_ptn5100>; >> > > + dr_mode = "otg"; >> > > +}; >> > > + >> > > +&usb_dwc3_1 { >> > > + status = "okay"; >> > >> > And here. >> > >> > > + dr_mode = "host"; >> > > +}; >> > > + >> > > +&usdhc1 { >> > > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; >> > > + pinctrl-0 = <&pinctrl_usdhc1>; >> > > + pinctrl-1 = <&pinctrl_usdhc1_100mhz>; >> > > + pinctrl-2 = <&pinctrl_usdhc1_200mhz>; >> > > + bus-width = <8>; >> > > + non-removable; >> > > + status = "okay"; >> > > +}; >> > > + >> > > +&usdhc2 { >> > > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; >> > > + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; >> > >> > You can drop the pinctrl_usdhc2_gpio handle if you mux them within the >> > regulator node. >> >> Ok >> >> > >> > > + pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>; >> > > + pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>; >> > > + bus-width = <4>; >> > > + vmmc-supply = <®_usdhc2_vmmc>; >> > > + power-supply = <&wifi_pwr_en>; >> > > + >> > > + non-removable; >> > > + disable-wp; >> > > + cap-sdio-irq; >> > > + keep-power-in-suspend; >> > > + wakeup-source; >> > > + status = "okay"; >> > > +}; >> > > + >> > > +&wdog1 { >> > > + pinctrl-names = "default"; >> > > + pinctrl-0 = <&pinctrl_wdog>; >> > > + fsl,ext-reset-output; >> > > + status = "okay"; >> > > +}; >> > > -- >> > > 2.17.1 >> > > >> > >> > So now I finished, sorry for don't cover that in your v3. >> > And you're sure there aren't any more :) Thanks Angus >> > Regards, >> > Marco >> >> Thanks >> Angus > > Regards, > Marco > >> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit 2019-04-16 22:36 ` Angus Ainslie @ 2019-04-17 6:54 ` Marco Felsch 0 siblings, 0 replies; 11+ messages in thread From: Marco Felsch @ 2019-04-17 6:54 UTC (permalink / raw) To: Angus Ainslie Cc: Mark Rutland, devicetree, Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel Hi Angus, On 19-04-16 16:36, Angus Ainslie wrote: > Hi Marco, > > On 2019-04-16 15:20, Marco Felsch wrote: > > Hi Angus, > > > > On 19-04-16 11:02, Angus Ainslie wrote: > > > Hi Marco, > > > > > > On 2019-04-16 00:54, Marco Felsch wrote: > > > > Hi Agnus, > > > > > > > > > > Just a nitpick but could you spell my name correctly :) > > > > Sorry it was to early that morning and I haven't had my first coffee. > > > > > > On 19-04-15 16:53, Angus Ainslie (Purism) wrote: > > > > > This is the development kit board for the Librem 5. The current > > > > > level of > > > > > support yields a working console and is able to boot userspace from > > > > > the > > > > > Network or eMMC. > > > > > > > > > > Additional subsystems that are active : > > > > > > > > > > - Both USB ports > > > > > - SD card socket > > > > > - WiFi usdhc > > > > > - WWAN modem > > > > > - GNSS > > > > > - GPIO keys > > > > > - LEDs > > > > > - gyro > > > > > - magnetometer > > > > > - touchscreen > > > > > - pwm > > > > > - backlight > > > > > - haptic motor > > > > > > > > > > Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> > > > > > --- > > > > > arch/arm64/boot/dts/freescale/Makefile | 1 + > > > > > .../dts/freescale/imx8mq-librem5-devkit.dts | 822 > > > > > ++++++++++++++++++ > > > > > 2 files changed, 823 insertions(+) > > > > > create mode 100644 > > > > > arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > > > > > > > > > diff --git a/arch/arm64/boot/dts/freescale/Makefile > > > > > b/arch/arm64/boot/dts/freescale/Makefile > > > > > index 0bd122f60549..c043aca66572 100644 > > > > > --- a/arch/arm64/boot/dts/freescale/Makefile > > > > > +++ b/arch/arm64/boot/dts/freescale/Makefile > > > > > @@ -22,6 +22,7 @@ dtb-$(CONFIG_ARCH_LAYERSCAPE) += fsl-lx2160a-rdb.dtb > > > > > > > > > > dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb > > > > > dtb-$(CONFIG_ARCH_MXC) += imx8mq-evk.dtb > > > > > +dtb-$(CONFIG_ARCH_MXC) += imx8mq-librem5-devkit.dtb > > > > > dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-rmb3.dtb > > > > > dtb-$(CONFIG_ARCH_MXC) += imx8mq-zii-ultra-zest.dtb > > > > > dtb-$(CONFIG_ARCH_MXC) += imx8qxp-mek.dtb > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > > > > b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > > > > new file mode 100644 > > > > > index 000000000000..dd705b6b94a4 > > > > > --- /dev/null > > > > > +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > > > > @@ -0,0 +1,822 @@ > > > > > +/* SPDX-License-Identifier: GPL-2.0+ > > > > > + * > > > > > + * Copyright 2018-2019 Purism SPC > > > > > + */ > > > > > + > > > > > +/dts-v1/; > > > > > + > > > > > +#include "imx8mq.dtsi" > > > > > +#include "dt-bindings/usb/pd.h" > > > > > +#include "dt-bindings/input/input.h" > > > > > > > > Just a nitpick but this should be alphabetical too. > > > > > > > > > > Ok. > > > > > > > > + > > > > > +/ { > > > > > + model = "Purism Librem 5 devkit"; > > > > > + compatible = "purism,librem5-devkit", "fsl,imx8mq"; > > > > > + > > > > > + backlight_dsi: backlight-dsi { > > > > > + compatible = "pwm-backlight"; > > > > > + /* 200 Hz for the PAM2841 */ > > > > > + pwms = <&pwm1 0 5000000>; > > > > > + brightness-levels = <0 100>; > > > > > + num-interpolated-steps = <100>; > > > > > + /* Default brightness level (index into the array defined by */ > > > > > + /* the "brightness-levels" property) */ > > > > > + default-brightness-level = <0>; > > > > > + power-supply = <®_22V4_P>; > > > > > + }; > > > > > + > > > > > + chosen { > > > > > + stdout-path = &uart1; > > > > > + }; > > > > > + > > > > > + gpio-keys { > > > > > + compatible = "gpio-keys"; > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_gpio_keys>; > > > > > + > > > > > + btn1 { > > > > > + label = "VOL_UP"; > > > > > + gpios = <&gpio4 21 GPIO_ACTIVE_LOW>; > > > > > + gpio-key,wakeup; > > > > > + linux,code = <KEY_VOLUMEUP>; > > > > > + }; > > > > > + > > > > > + btn2 { > > > > > + label = "VOL_DOWN"; > > > > > + gpios = <&gpio4 22 GPIO_ACTIVE_LOW>; > > > > > + gpio-key,wakeup; > > > > > + linux,code = <KEY_VOLUMEDOWN>; > > > > > + }; > > > > > + > > > > > + hp_det { > > > > > + label = "HP_DET"; > > > > > + gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; > > > > > + gpio-key,wakeup; > > > > > + linux,code = <KEY_HP>; > > > > > + }; > > > > > + }; > > > > > + > > > > > + leds { > > > > > + compatible = "gpio-leds"; > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_gpio_leds>; > > > > > + status = "okay"; > > > > > > > > You can drop the status since the default is "okay". > > > > > > > > > > Ok. > > > > > > > > + > > > > > + led1 { > > > > > + label = "LED 1"; > > > > > + gpios = <&gpio1 13 GPIO_ACTIVE_HIGH>; > > > > > + default-state = "off"; > > > > > + }; > > > > > + }; > > > > > + > > > > > + pmic_osc: pmic-osc { > > > > > + compatible = "fixed-clock"; > > > > > + #clock-cells = <0>; > > > > > + clock-frequency = <32768>; > > > > > + clock-output-names = "pmic_osc"; > > > > > + }; > > > > > + > > > > > + pwmleds { > > > > > + compatible = "pwm-leds"; > > > > > > > > Can you add a blank line? It's just a nitpick. > > > > > > > > > > Sure. > > > > > > > > + haptic { > > > > > + label = "librem5::haptic"; > > > > > + pwms = <&pwm2 0 200000>; > > > > > + active-low; > > > > > + max-brightness = <255>; > > > > > + power-supply = <®_3V3_P>; > > > > > + }; > > > > > + }; > > > > > + > > > > > + reg_1V8_P: regulator-1V8-P { > > > > > > > > I don't know the policy but I would name the nodes and phandles always > > > > in lower case. > > > > > > > > > > Ok. > > > > > > > > + compatible = "regulator-fixed"; > > > > > + regulator-name = "1V8_P"; > > > > > + regulator-min-microvolt = <1800000>; > > > > > + regulator-max-microvolt = <1800000>; > > > > > + vin-supply = <®_vbat>; > > > > > + en-supply = <®_pwr_en>; > > > > > > > > What is 'en-supply'? Just checked the v5.1-rc5 and didn't found that > > > > property at all. This applies to the other regulator nodes too. > > > > > > > > > > I was attempting to show a dependency on both supplies but I'll > > > change the > > > way it is structured. > > > > > > > > + }; > > > > > + > > > > > + reg_2V8_P: regulator-2V8-P { > > > > > + compatible = "regulator-fixed"; > > > > > + regulator-name = "2V8_P"; > > > > > + regulator-min-microvolt = <2800000>; > > > > > + regulator-max-microvolt = <2800000>; > > > > > + vin-supply = <®_3V3_P>; > > > > > + }; > > > > > + > > > > > + reg_3V3_P: regulator-3V3-P { > > > > > + compatible = "regulator-fixed"; > > > > > + regulator-name = "3V3_P"; > > > > > + regulator-min-microvolt = <3300000>; > > > > > + regulator-max-microvolt = <3300000>; > > > > > + vin-supply = <®_vbat>; > > > > > + en-supply = <®_pwr_en>; > > > > > + }; > > > > > + > > > > > + reg_5V_P: regulator-5V-P { > > > > > + /* Supplies HDMI, USB hub and smartcard */ > > > > > + compatible = "regulator-fixed"; > > > > > + regulator-name = "5V_P"; > > > > > + regulator-min-microvolt = <5000000>; > > > > > + regulator-max-microvolt = <5000000>; > > > > > + vin-supply = <®_vbat>; > > > > > + en-supply = <®_pwr_en>; > > > > > + }; > > > > > + > > > > > + reg_5V_SOM: regulator-5V-SOM { > > > > > + compatible = "regulator-fixed"; > > > > > + regulator-name = "5V_SOM"; > > > > > + regulator-min-microvolt = <5000000>; > > > > > + regulator-max-microvolt = <5000000>; > > > > > + vin-supply = <®_vbat>; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + reg_22V4_P: regulator-22V4-P { > > > > > + compatible = "regulator-fixed"; > > > > > + regulator-name = "22V4_P"; > > > > > + regulator-min-microvolt = <22400000>; > > > > > + regulator-max-microvolt = <22400000>; > > > > > + vin-supply = <®_vbat>; > > > > > + }; > > > > > + > > > > > + reg_pwr_en: regulator-pwr-en { > > > > > + compatible = "regulator-fixed"; > > > > > + regulator-name = "PWR_EN"; > > > > > + regulator-min-microvolt = <3300000>; > > > > > + regulator-max-microvolt = <3300000>; > > > > > + gpio = <&gpio1 8 GPIO_ACTIVE_HIGH>; > > > > > > > > You missed the pinctrl-0, pinctrl-names property since you already have > > > > a pinctrl_pwr_en phandle. > > > > > > > > > + enable-active-high; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + reg_usdhc2_vmmc: regulator-usdhc2-vmmc { > > > > > + compatible = "regulator-fixed"; > > > > > + regulator-name = "VSD_3V3"; > > > > > + regulator-min-microvolt = <3300000>; > > > > > + regulator-max-microvolt = <3300000>; > > > > > + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>; > > > > > > > > IMHO you should mux (pinctrl) a gpio where you use them but that's only > > > > my opinion. > > > > > > > > > + enable-active-high; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + reg_vbat: regulator-vbat { > > > > > + compatible = "regulator-fixed"; > > > > > + regulator-name = "VBAT_REG"; > > > > > + regulator-always-on; > > > > > + }; > > > > > > > > A fixed regulator without a regulator-min-microvolt > > > > regulator-max-microvolt? > > > > > > > > > > That will get fixed with the issue above. > > > > > > > > + > > > > > + wifi_pwr_en: wifi-en { > > > > > + compatible = "regulator-fixed"; > > > > > + regulator-name = "WIFI_EN"; > > > > > + regulator-min-microvolt = <3300000>; > > > > > + regulator-max-microvolt = <3300000>; > > > > > + gpio = <&gpio3 5 GPIO_ACTIVE_HIGH>; > > > > > > > > Where do you mux that gpio? It seems that you use the > > > > pinctrl_usdhc2_gpio for muxing but there is no GPIO3 mux. > > > > > > > > > > I'll fix it with the regulators. > > > > > > > > + enable-active-high; > > > > > + regulator-always-on; > > > > > + }; > > > > > +}; > > > > > + > > > > > + > > > > > +&clk { > > > > > + assigned-clocks = <&clk IMX8MQ_AUDIO_PLL1>, <&clk > > > > > IMX8MQ_AUDIO_PLL2>; > > > > > + assigned-clock-rates = <786432000>, <722534400>; > > > > > +}; > > > > > + > > > > > +&fec1 { > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_fec1>; > > > > > + phy-mode = "rgmii-id"; > > > > > + phy-handle = <ðphy0>; > > > > > + fsl,magic-packet; > > > > > + status = "okay"; > > > > > > > > status should be the last property in this list. > > > > > > > > > > Ok. > > > > > > > > + phy-supply = <®_3V3_P>; > > > > > + > > > > > + mdio { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > + > > > > > + ethphy0: ethernet-phy@1 { > > > > > > > > Is it necessary to name it ethphy0? Just a nitpick. > > > > > > > > > + compatible = "ethernet-phy-ieee802.3-c22"; > > > > > + reg = <1>; > > > > > + }; > > > > > + }; > > > > > +}; > > > > > + > > > > > +&i2c1 { > > > > > + clock-frequency = <400000>; > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_i2c1>; > > > > > + status = "okay"; > > > > > + > > > > > + pmic: pmic@4b { > > > > > + reg = <0x4b>; > > > > > + compatible = "rohm,bd71837"; > > > > > > > > Please swap the reg and compatible property. > > > > > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_pmic>; > > > > > + clocks = <&pmic_osc>; > > > > > + clock-names = "osc"; > > > > > + clock-output-names = "pmic_clk"; > > > > > + interrupt-parent = <&gpio1>; > > > > > + interrupts = <3 GPIO_ACTIVE_LOW>; > > > > > + interrupt-names = "irq"; > > > > > + rohm,reset-snvs-powered; > > > > > + > > > > > + regulators { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > > > > Please drop the #address-cells, #size-cells and the @<reg> suffix > > > > including the reg = <> property. So it would look like: > > > > > > > > buck1_reg: BUCK1 { > > > > regulator-name = "buck1"; > > > > regulator-min-microvolt = <700000>; > > > > regulator-max-microvolt = <1300000>; > > > > regulator-boot-on; > > > > regulator-always-on; > > > > regulator-ramp-delay = <1250>; > > > > rohm,dvs-run-voltage = <900000>; > > > > rohm,dvs-idle-voltage = <850000>; > > > > rohm,dvs-suspend-voltage = <800000>; > > > > }; > > > > > > > > Please look at > > > > Documentation/devicetree/bindings/regulator/rohm,bd71837-regulator.txt. > > > > > > > > > > Thanks hadn't noticed the documentation change. > > > > > > > > + > > > > > + buck1_reg: BUCK1@0 { > > > > > + reg = <0>; > > > > > + regulator-name = "buck1"; > > > > > + regulator-min-microvolt = <700000>; > > > > > + regulator-max-microvolt = <1300000>; > > > > > + regulator-boot-on; > > > > > + regulator-always-on; > > > > > + regulator-ramp-delay = <1250>; > > > > > + rohm,dvs-run-voltage = <900000>; > > > > > + rohm,dvs-idle-voltage = <850000>; > > > > > + rohm,dvs-suspend-voltage = <800000>; > > > > > + }; > > > > > + > > > > > + buck2_reg: BUCK2@1 { > > > > > + reg = <1>; > > > > > + regulator-name = "buck2"; > > > > > + regulator-min-microvolt = <700000>; > > > > > + regulator-max-microvolt = <1300000>; > > > > > + regulator-boot-on; > > > > > + regulator-always-on; > > > > > + regulator-ramp-delay = <1250>; > > > > > + rohm,dvs-run-voltage = <1000000>; > > > > > + rohm,dvs-idle-voltage = <900000>; > > > > > + }; > > > > > + > > > > > + buck3_reg: BUCK3@2 { > > > > > + reg = <2>; > > > > > + regulator-name = "buck3"; > > > > > + regulator-min-microvolt = <700000>; > > > > > + regulator-max-microvolt = <1300000>; > > > > > + rohm,dvs-run-voltage = <1000000>; > > > > > + }; > > > > > + > > > > > + buck4_reg: BUCK4@3 { > > > > > + reg = <3>; > > > > > + regulator-name = "buck4"; > > > > > + regulator-min-microvolt = <700000>; > > > > > + regulator-max-microvolt = <1300000>; > > > > > + rohm,dvs-run-voltage = <1000000>; > > > > > + }; > > > > > + > > > > > + buck5_reg: BUCK5@4 { > > > > > + reg = <4>; > > > > > + regulator-name = "buck5"; > > > > > + regulator-min-microvolt = <700000>; > > > > > + regulator-max-microvolt = <1350000>; > > > > > + regulator-boot-on; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + buck6_reg: BUCK6@5 { > > > > > + reg = <5>; > > > > > + regulator-name = "buck6"; > > > > > + regulator-min-microvolt = <3000000>; > > > > > + regulator-max-microvolt = <3300000>; > > > > > + regulator-boot-on; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + buck7_reg: BUCK7@6 { > > > > > + reg = <6>; > > > > > + regulator-name = "buck7"; > > > > > + regulator-min-microvolt = <1605000>; > > > > > + regulator-max-microvolt = <1995000>; > > > > > + regulator-boot-on; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + buck8_reg: BUCK8@7 { > > > > > + reg = <7>; > > > > > + regulator-name = "buck8"; > > > > > + regulator-min-microvolt = <800000>; > > > > > + regulator-max-microvolt = <1400000>; > > > > > + regulator-boot-on; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + ldo1_reg: LDO1@8 { > > > > > + reg = <8>; > > > > > + regulator-name = "ldo1"; > > > > > + regulator-min-microvolt = <3000000>; > > > > > + regulator-max-microvolt = <3300000>; > > > > > + regulator-boot-on; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + ldo2_reg: LDO2@9 { > > > > > + reg = <9>; > > > > > + regulator-name = "ldo2"; > > > > > + regulator-min-microvolt = <900000>; > > > > > + regulator-max-microvolt = <900000>; > > > > > + regulator-boot-on; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + ldo3_reg: LDO3@10 { > > > > > + reg = <10>; > > > > > + regulator-name = "ldo3"; > > > > > + regulator-min-microvolt = <1800000>; > > > > > + regulator-max-microvolt = <3300000>; > > > > > + regulator-boot-on; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + ldo4_reg: LDO4@11 { > > > > > + reg = <11>; > > > > > + regulator-name = "ldo4"; > > > > > + regulator-min-microvolt = <900000>; > > > > > + regulator-max-microvolt = <1800000>; > > > > > + regulator-boot-on; > > > > > + regulator-always-on; > > > > > + }; > > > > > + > > > > > + ldo5_reg: LDO5@12 { > > > > > + reg = <12>; > > > > > + regulator-name = "ldo5"; > > > > > + regulator-min-microvolt = <1800000>; > > > > > + regulator-max-microvolt = <3300000>; > > > > > + }; > > > > > + > > > > > + ldo6_reg: LDO6@13 { > > > > > + reg = <13>; > > > > > + regulator-name = "ldo6"; > > > > > + regulator-min-microvolt = <900000>; > > > > > + regulator-max-microvolt = <1800000>; > > > > > + }; > > > > > + > > > > > + ldo7_reg: LDO7@14 { > > > > > + reg = <14>; > > > > > + regulator-name = "ldo7"; > > > > > + regulator-min-microvolt = <1800000>; > > > > > + regulator-max-microvolt = <3300000>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + > > > > > + typec_ptn5100: usb_typec@52 { > > > > > + compatible = "nxp,ptn5110"; > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_typec>; > > > > > + reg = <0x52>; > > > > > > > > The reg = <> should be followed after the compatible. > > > > > > > > > + interrupt-parent = <&gpio3>; > > > > > + interrupts = <1 IRQ_TYPE_LEVEL_LOW>; > > > > > + vconn-supply = <®_5V_P>; > > > > > > > > The vconn-supply isn't documented elsewhere. > > > > > > > > > > Yeah that came from the schematic not the code or documentation. > > > > I wouldn't add properties which aren't supported. > > > > Agreed > > > > > > + usb_con: connector { > > > > > > > > Is that phandle nedded? > > > > > > > > > + compatible = "usb-c-connector"; > > > > > + label = "USB-C"; > > > > > + data-role = "dual"; > > > > > + power-role = "dual"; > > > > > + try-power-role = "sink"; > > > > > + source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | > > > > > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP )>; > > > > > + sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | > > > > > PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP ) > > > > > + PDO_VAR(5000, 12000, 2000)>; > > > > > > > > Just a nitpick but I would break the lines, e.g. > > > > > > > > source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | > > > > PDO_FIXED_DUAL_ROLE | > > > > PDO_FIXED_DATA_SWAP )>; > > > > sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM | > > > > PDO_FIXED_DUAL_ROLE | > > > > PDO_FIXED_DATA_SWAP ) > > > > PDO_VAR(5000, 12000, 2000)>; > > > > > > > > > > > > > > Ok they are longer than 80 cols anyway > > > > That's true it was just a idea. You can break the lines where you want > > :) > > > > > > > > > > + op-sink-microwatt = <10000000>; > > > > > + ports { > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > > > > Blank line please. > > > > > > > > > + port@0 { > > > > > + reg = <0>; > > > > > + usb_con_hs: endpoint { > > > > > + remote-endpoint = <&typec_hs>; > > > > > + }; > > > > > + }; > > > > > > > > Here too. > > > > > > > > > + port@1 { > > > > > + reg = <1>; > > > > > + usb_con_ss: endpoint { > > > > > + remote-endpoint = <&typec_ss>; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + }; > > > > > + > > > > > + }; > > > > > + > > > > > + rtc@68 { > > > > > + pinctrl-names = "default"; > > > > > + compatible = "microcrystal,rv4162"; > > > > > + reg = <0x68>; > > > > > + pinctrl-0 = <&pinctrl_rtc>; > > > > > + interrupt-parent = <&gpio4>; > > > > > + interrupts = <29 IRQ_TYPE_LEVEL_LOW>; > > > > > + }; > > > > > + > > > > > + charger: charger@6b { /* bq25896 */ > > > > > > > > Is that phandle necessary? > > > > > > > > > + compatible = "ti,bq25890"; > > > > > + reg = <0x6b>; > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_charger>; > > > > > + interrupt-parent = <&gpio3>; > > > > > + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; > > > > > + ti,battery-regulation-voltage = <4192000>; /* 4.192V */ > > > > > + ti,charge-current = <1600000>; /* 1.6 A */ > > > > > + ti,termination-current = <66000>; /* 66mA */ > > > > > + ti,precharge-current = <1300000>; /* 1.3A */ > > > > > + ti,minimum-sys-voltage = <2750000>; /* 2.75V */ > > > > > + ti,boost-voltage = <5000000>; /* 5V */ > > > > > + ti,boost-max-current = <50000>; /* 50mA */ > > > > > > > > IMHO I would comment only the measurments. > > > > > > > > > > Sorry do you mean the units ? > > > > Yes and sorry my english wasn't the best this morning. > > > > Sure no problem, I just wanted to be sure I understood. > > > > > > > I liked the additional clarity of not having to count zero's. > > > > Like you prefer. I knew cases where the comments and the values didn't > > matched anymore. Then it gets harder to decide which value is correct. > > > > Yes they should be kept in sync. > > > > > > + }; > > > > > +}; > > > > > + > > > > > +&i2c3 { > > > > > + clock-frequency = <100000>; > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_i2c3>, <&pinctrl_imu>; > > > > > > > > Why do you don't mux the pinctrl_imu within the magnetometer@1e node? > > > > > > > > > + status = "okay"; > > > > > + > > > > > + lsm9d_magn: magnetometer@1e { > > > > > > > > Is that phandle necessary? > > > > > > > > > + compatible = "st,lsm9ds1-magn"; > > > > > + reg = <0x1e>; > > > > > + interrupt-parent = <&gpio3>; > > > > > + interrupts = <19 IRQ_TYPE_LEVEL_LOW>; > > > > > + vdd-supply = <®_3V3_P>; > > > > > + vddio-supply = <®_3V3_P>; > > > > > + }; > > > > > + > > > > > + goodix_ts: touchscreen@5d { > > > > > > > > This here too. > > > > > > > > > + compatible = "goodix,gt5688"; > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_goodix_ts>; > > > > > + reg = <0x5d>; > > > > > > > > The reg = <> should be followed the compatible string. > > > > > > > > > + interrupt-parent = <&gpio3>; > > > > > + interrupts = <0 IRQ_TYPE_LEVEL_LOW>; > > > > > + reset-gpios = <&gpio1 5 GPIO_ACTIVE_HIGH>; > > > > > + irq-gpios = <&gpio3 0 GPIO_ACTIVE_HIGH>; > > > > > + touchscreen-size-x = <720>; > > > > > + touchscreen-size-y = <1440>; > > > > > + AVDD28-supply = <®_2V8_P>; > > > > > + VDDIO-supply = <®_1V8_P>; > > > > > > > > Didn't found any of these supplies. Can you drop them? > > > > > > > > > > AVDD28 line 539 in drivers/input/touchscreen/goodix.c > > > > > > VDDIO line 548 in drivers/input/touchscreen/goodix.c > > > > Nope, > > https://elixir.bootlin.com/linux/v5.1-rc5/source/drivers/input/touchscreen/goodix.c#L539 > > > > Did you have any patches applied or are there some ongoing patches? > > > > This is based on linux-next next-20190415 > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/touchscreen/goodix.c?h=next-20190412#n539 > > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/input/touchscreen/goodix.c?h=next-20190412#n548 Ah okay. Can I mention this in the commit notes (after the three '---' followed). Regrads, Marco > > > > > > > > + }; > > > > > +}; > > > > > + > > > > > +&iomuxc { > > > > > > > > It seems a bit inconsistent to add a comment behind the > > > > mux or above. I would add comments just above. > > > > > > > > > > My preference was for commenting after but that made some lines > > > longer than > > > 80 cols so I reduced the offending lines. > > > > > > > > + pinctrl_bt: btgrp { > > > > > + fsl,pins = < > > > > > + /* nBT_DISABLE */ > > > > > + MX8MQ_IOMUXC_NAND_DATA05_GPIO3_IO11 0x16 > > > > > > > > As you do it here. > > > > > > > > > + /* BT_HOST_WAKE */ > > > > > + MX8MQ_IOMUXC_NAND_DATA01_GPIO3_IO7 0x10 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_charger: chargernirq { > > > > > + fsl,pins = < > > > > > + /* CHRG_nINT */ > > > > > + MX8MQ_IOMUXC_SAI5_MCLK_GPIO3_IO25 0x80 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_fec1: fec1grp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_ENET_MDC_ENET1_MDC 0x3 > > > > > + MX8MQ_IOMUXC_ENET_MDIO_ENET1_MDIO 0x3 > > > > > + MX8MQ_IOMUXC_ENET_TD3_ENET1_RGMII_TD3 0x1f > > > > > + MX8MQ_IOMUXC_ENET_TD2_ENET1_RGMII_TD2 0x1f > > > > > + MX8MQ_IOMUXC_ENET_TD1_ENET1_RGMII_TD1 0x1f > > > > > + MX8MQ_IOMUXC_ENET_TD0_ENET1_RGMII_TD0 0x1f > > > > > + MX8MQ_IOMUXC_ENET_RD3_ENET1_RGMII_RD3 0x91 > > > > > + MX8MQ_IOMUXC_ENET_RD2_ENET1_RGMII_RD2 0x91 > > > > > + MX8MQ_IOMUXC_ENET_RD1_ENET1_RGMII_RD1 0x91 > > > > > + MX8MQ_IOMUXC_ENET_RD0_ENET1_RGMII_RD0 0x91 > > > > > + MX8MQ_IOMUXC_ENET_TXC_ENET1_RGMII_TXC 0x1f > > > > > + MX8MQ_IOMUXC_ENET_RXC_ENET1_RGMII_RXC 0x91 > > > > > + MX8MQ_IOMUXC_ENET_RX_CTL_ENET1_RGMII_RX_CTL 0x91 > > > > > + MX8MQ_IOMUXC_ENET_TX_CTL_ENET1_RGMII_TX_CTL 0x1f > > > > > + MX8MQ_IOMUXC_GPIO1_IO09_GPIO1_IO9 0x19 > > > > > + MX8MQ_IOMUXC_GPIO1_IO15_CCMSRCGPCMIX_CLKO2 0x1f > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_goodix_ts: gt5688 { > > > > > + fsl,pins = < > > > > > + /* TOUCH INT */ > > > > > + MX8MQ_IOMUXC_NAND_ALE_GPIO3_IO0 0x16 > > > > > + /* TOUCH RST */ > > > > > + MX8MQ_IOMUXC_GPIO1_IO05_GPIO1_IO5 0x19 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_gpio_leds: gpioleds { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_GPIO1_IO13_GPIO1_IO13 0x16 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_gpio_keys: gpiokeys { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_SAI2_RXFS_GPIO4_IO21 0x16 > > > > > + MX8MQ_IOMUXC_SAI2_RXC_GPIO4_IO22 0x16 > > > > > + /* HP_DET */ > > > > > + MX8MQ_IOMUXC_SAI5_RXC_GPIO3_IO20 0x180 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_haptic: hapticgrp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_SPDIF_RX_PWM2_OUT 0xc6 /* nHAPTIC */ > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_i2c1: i2c1grp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_I2C1_SCL_I2C1_SCL 0x4000001f > > > > > + MX8MQ_IOMUXC_I2C1_SDA_I2C1_SDA 0x4000001f > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_i2c3: i2c3grp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_I2C3_SCL_I2C3_SCL 0x4000001f > > > > > + MX8MQ_IOMUXC_I2C3_SDA_I2C3_SDA 0x4000001f > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_imu: imugrp { > > > > > + fsl,pins = < > > > > > + /* IMU_INT */ > > > > > + MX8MQ_IOMUXC_SAI5_RXFS_GPIO3_IO19 0x8 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_pmic: pmicint { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_GPIO1_IO03_GPIO1_IO3 0x80 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_pwr_en: pwrengrp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_GPIO1_IO08_GPIO1_IO8 0x06 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_pwm1: pwm1 { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_GPIO1_IO01_PWM1_OUT 0x6 /* DSI_BL_PWM */ > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_rtc: rtcirq { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_SAI3_RXC_GPIO4_IO29 0x80 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_typec: typecgrp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_NAND_DATA06_GPIO3_IO12 0x16 > > > > > + MX8MQ_IOMUXC_NAND_CE0_B_GPIO3_IO1 0x80 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_uart1: uart1grp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_UART1_RXD_UART1_DCE_RX 0x49 > > > > > + MX8MQ_IOMUXC_UART1_TXD_UART1_DCE_TX 0x49 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_uart2: uart2grp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_UART2_TXD_UART2_DCE_TX 0x49 > > > > > + MX8MQ_IOMUXC_UART2_RXD_UART2_DCE_RX 0x49 > > > > > + MX8MQ_IOMUXC_UART4_RXD_UART2_DCE_CTS_B 0x49 > > > > > + MX8MQ_IOMUXC_UART4_TXD_UART2_DCE_RTS_B 0x49 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_uart3: uart3grp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_UART3_RXD_UART3_DCE_RX 0x49 > > > > > + MX8MQ_IOMUXC_UART3_TXD_UART3_DCE_TX 0x49 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_uart4: uart4grp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_ECSPI2_SCLK_UART4_DCE_RX 0x49 > > > > > + MX8MQ_IOMUXC_ECSPI2_MOSI_UART4_DCE_TX 0x49 > > > > > + MX8MQ_IOMUXC_ECSPI2_MISO_UART4_DCE_CTS_B 0x49 > > > > > + MX8MQ_IOMUXC_ECSPI2_SS0_UART4_DCE_RTS_B 0x49 > > > > > + MX8MQ_IOMUXC_GPIO1_IO00_ANAMIX_REF_CLK_32K 0x49 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_usdhc1: usdhc1grp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x83 > > > > > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xc3 > > > > > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xc3 > > > > > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xc3 > > > > > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xc3 > > > > > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xc3 > > > > > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xc3 > > > > > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xc3 > > > > > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xc3 > > > > > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xc3 > > > > > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x83 > > > > > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_usdhc1_100mhz: usdhc1grp100mhz { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x8d > > > > > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xcd > > > > > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xcd > > > > > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xcd > > > > > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xcd > > > > > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xcd > > > > > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xcd > > > > > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xcd > > > > > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xcd > > > > > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xcd > > > > > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x8d > > > > > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_usdhc1_200mhz: usdhc1grp200mhz { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_SD1_CLK_USDHC1_CLK 0x9f > > > > > + MX8MQ_IOMUXC_SD1_CMD_USDHC1_CMD 0xdf > > > > > + MX8MQ_IOMUXC_SD1_DATA0_USDHC1_DATA0 0xdf > > > > > + MX8MQ_IOMUXC_SD1_DATA1_USDHC1_DATA1 0xdf > > > > > + MX8MQ_IOMUXC_SD1_DATA2_USDHC1_DATA2 0xdf > > > > > + MX8MQ_IOMUXC_SD1_DATA3_USDHC1_DATA3 0xdf > > > > > + MX8MQ_IOMUXC_SD1_DATA4_USDHC1_DATA4 0xdf > > > > > + MX8MQ_IOMUXC_SD1_DATA5_USDHC1_DATA5 0xdf > > > > > + MX8MQ_IOMUXC_SD1_DATA6_USDHC1_DATA6 0xdf > > > > > + MX8MQ_IOMUXC_SD1_DATA7_USDHC1_DATA7 0xdf > > > > > + MX8MQ_IOMUXC_SD1_STROBE_USDHC1_STROBE 0x9f > > > > > + MX8MQ_IOMUXC_SD1_RESET_B_USDHC1_RESET_B 0xc1 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_usdhc2_gpio: usdhc2grpgpio { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_SD2_RESET_B_GPIO2_IO19 0x41 > > > > > + /* WIFI_WAKE */ > > > > > + MX8MQ_IOMUXC_SD2_WP_GPIO2_IO20 0x80 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_usdhc2: usdhc2grp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x83 > > > > > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xc3 > > > > > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xc3 > > > > > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xc3 > > > > > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xc3 > > > > > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xc3 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_usdhc2_100mhz: usdhc2grp100mhz { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x8d > > > > > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcd > > > > > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcd > > > > > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcd > > > > > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcd > > > > > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcd > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_usdhc2_200mhz: usdhc2grp200mhz { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_SD2_CLK_USDHC2_CLK 0x9f > > > > > + MX8MQ_IOMUXC_SD2_CMD_USDHC2_CMD 0xcf > > > > > + MX8MQ_IOMUXC_SD2_DATA0_USDHC2_DATA0 0xcf > > > > > + MX8MQ_IOMUXC_SD2_DATA1_USDHC2_DATA1 0xcf > > > > > + MX8MQ_IOMUXC_SD2_DATA2_USDHC2_DATA2 0xcf > > > > > + MX8MQ_IOMUXC_SD2_DATA3_USDHC2_DATA3 0xcf > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_wdog: wdoggrp { > > > > > + fsl,pins = < > > > > > + MX8MQ_IOMUXC_GPIO1_IO02_WDOG1_WDOG_B 0xc6 > > > > > + >; > > > > > + }; > > > > > + > > > > > + pinctrl_wwan: wwangrp { > > > > > + fsl,pins = < > > > > > + /* nWWAN_DISABLE */ > > > > > + MX8MQ_IOMUXC_NAND_CE3_B_GPIO3_IO4 0x09 > > > > > + /* nWoWWAN */ > > > > > + MX8MQ_IOMUXC_NAND_DATA02_GPIO3_IO8 0x80 > > > > > + /* WWAN_RESET */ > > > > > + MX8MQ_IOMUXC_NAND_DATA03_GPIO3_IO9 0x19 > > > > > + >; > > > > > + }; > > > > > + > > > > > +}; > > > > > + > > > > > +&pwm1 { > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_pwm1>; > > > > > + status = "okay"; > > > > > +}; > > > > > + > > > > > +&pwm2 { > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_haptic>; > > > > > > > > Just a nitpick but I would name it pinctrl_pwm2 as you did it for pwm1. > > > > > > > > > > I figured haptic was more descriptive. > > > > Descriptive for the pwm node? If that is the case the pwm1 node should > > be > > called backlight. Anyway as I said it's just a nitpick. > > > > > > > > > > + status = "okay"; > > > > > +}; > > > > > + > > > > > +&uart1 { /* console */ > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_uart1>; > > > > > + status = "okay"; > > > > > +}; > > > > > + > > > > > +&uart3 { /* GNSS */ > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_uart3>; > > > > > + status = "okay"; > > > > > +}; > > > > > + > > > > > +&uart4 { /* BT */ > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_uart4>, <&pinctrl_bt>; > > > > > + uart-has-rtscts; > > > > > + status = "okay"; > > > > > +}; > > > > > + > > > > > +&usb3_phy0 { > > > > > + status = "okay"; > > > > > > > > Should be the last property. > > > > > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > > > > Blank line. > > > > > + port@0 { > > > > > + reg = <0>; > > > > > > > > and here > > > > > + typec_hs: endpoint { > > > > > + remote-endpoint = <&usb_con_hs>; > > > > > + }; > > > > > + }; > > > > > > > > here > > > > > > > > > + port@1 { > > > > > + reg = <1>; > > > > > > > > and here. > > > > > > > > > + typec_ss: endpoint { > > > > > + remote-endpoint = <&usb_con_ss>; > > > > > + }; > > > > > + }; > > > > > +}; > > > > > + > > > > > +&usb3_phy1 { > > > > > + status = "okay"; > > > > > + #address-cells = <1>; > > > > > + #size-cells = <0>; > > > > > > > > Why did you assign the #address-cells and #size-cells without specify > > > > any port? Status should be the last property. > > > > > > > > > > Cut and paste from phy0. > > > > > > > > +}; > > > > > + > > > > > +&usb_dwc3_0 { > > > > > + status = "okay"; > > > > > > > > Here also. > > > > > > > > > + extcon = <&typec_ptn5100>; > > > > > + dr_mode = "otg"; > > > > > +}; > > > > > + > > > > > +&usb_dwc3_1 { > > > > > + status = "okay"; > > > > > > > > And here. > > > > > > > > > + dr_mode = "host"; > > > > > +}; > > > > > + > > > > > +&usdhc1 { > > > > > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > > > > > + pinctrl-0 = <&pinctrl_usdhc1>; > > > > > + pinctrl-1 = <&pinctrl_usdhc1_100mhz>; > > > > > + pinctrl-2 = <&pinctrl_usdhc1_200mhz>; > > > > > + bus-width = <8>; > > > > > + non-removable; > > > > > + status = "okay"; > > > > > +}; > > > > > + > > > > > +&usdhc2 { > > > > > + pinctrl-names = "default", "state_100mhz", "state_200mhz"; > > > > > + pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>; > > > > > > > > You can drop the pinctrl_usdhc2_gpio handle if you mux them within the > > > > regulator node. > > > > > > Ok > > > > > > > > > > > > + pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>; > > > > > + pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>; > > > > > + bus-width = <4>; > > > > > + vmmc-supply = <®_usdhc2_vmmc>; > > > > > + power-supply = <&wifi_pwr_en>; > > > > > + > > > > > + non-removable; > > > > > + disable-wp; > > > > > + cap-sdio-irq; > > > > > + keep-power-in-suspend; > > > > > + wakeup-source; > > > > > + status = "okay"; > > > > > +}; > > > > > + > > > > > +&wdog1 { > > > > > + pinctrl-names = "default"; > > > > > + pinctrl-0 = <&pinctrl_wdog>; > > > > > + fsl,ext-reset-output; > > > > > + status = "okay"; > > > > > +}; > > > > > -- > > > > > 2.17.1 > > > > > > > > > > > > > So now I finished, sorry for don't cover that in your v3. > > > > > > And you're sure there aren't any more :) > > Thanks > Angus > > > > > Regards, > > > > Marco > > > > > > Thanks > > > Angus > > > > Regards, > > Marco > > > > > > > > > > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4 2/2] dt-bindings: Add an entry for Purism SPC 2019-04-15 23:53 [PATCH v4 0/2] Add support for the Purism Librem5 devkit Angus Ainslie (Purism) 2019-04-15 23:53 ` [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the " Angus Ainslie (Purism) @ 2019-04-15 23:53 ` Angus Ainslie (Purism) 2019-04-16 7:58 ` [PATCH v4 0/2] Add support for the Purism Librem5 devkit Marco Felsch 2 siblings, 0 replies; 11+ messages in thread From: Angus Ainslie (Purism) @ 2019-04-15 23:53 UTC (permalink / raw) Cc: Rob Herring, Mark Rutland, Shawn Guo, Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam, NXP Linux Team, Angus Ainslie (Purism), devicetree, linux-arm-kernel, linux-kernel Add an entry for Purism, SPC Signed-off-by: Angus Ainslie (Purism) <angus@akkea.ca> --- Documentation/devicetree/bindings/vendor-prefixes.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 40f632db7bc2..44ba42e48007 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -331,6 +331,7 @@ poslab Poslab Technology Co., Ltd. powervr PowerVR (deprecated, use img) probox2 PROBOX2 (by W2COMP Co., Ltd.) pulsedlight PulsedLight, Inc +purism Purism, SPC qca Qualcomm Atheros, Inc. qcom Qualcomm Technologies, Inc qemu QEMU, a generic and open source machine emulator and virtualizer -- 2.17.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v4 0/2] Add support for the Purism Librem5 devkit 2019-04-15 23:53 [PATCH v4 0/2] Add support for the Purism Librem5 devkit Angus Ainslie (Purism) 2019-04-15 23:53 ` [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the " Angus Ainslie (Purism) 2019-04-15 23:53 ` [PATCH v4 2/2] dt-bindings: Add an entry for Purism SPC Angus Ainslie (Purism) @ 2019-04-16 7:58 ` Marco Felsch 2019-04-16 16:38 ` Angus Ainslie 2 siblings, 1 reply; 11+ messages in thread From: Marco Felsch @ 2019-04-16 7:58 UTC (permalink / raw) To: Angus Ainslie (Purism) Cc: Mark Rutland, devicetree, Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel Hi Agnus, On 19-04-15 16:53, Angus Ainslie (Purism) wrote: > The Librem5 devkit is based on the imx8mq from NXP. This is a default > devicetree to boot the board to a command prompt. You should also provide a patch which adds the devicetree to the yaml file, see: Documentation/devicetree/bindings/arm/fsl.yaml. Thanks, Marco > > Changes since v3: > > Freshly sorted and pressed nodes. > Change the backlight to an interpolated scale. > Dropped i2c2. > Dropped devkit version number to match debian MR. > > Changes since v2: > > Fixed incorrect phy-supply for the fsl-fec. > Dropped unused regulator property. > Fixup Makefile for linux-next. > > Changes since v1: > > Dropped config file. > Updated the board compatible label. > Changed node names to follow naming conventions. > Added a more complete regulator hierachy. > Removed unused nodes. > Removed unknown devices. > Fixed comment style. > Dropped undocumented properties. > > Angus Ainslie (Purism) (2): > arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit > dt-bindings: Add an entry for Purism SPC > > .../devicetree/bindings/vendor-prefixes.txt | 1 + > arch/arm64/boot/dts/freescale/Makefile | 1 + > .../dts/freescale/imx8mq-librem5-devkit.dts | 822 ++++++++++++++++++ > 3 files changed, 824 insertions(+) > create mode 100644 arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > -- > 2.17.1 > > > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 0/2] Add support for the Purism Librem5 devkit 2019-04-16 7:58 ` [PATCH v4 0/2] Add support for the Purism Librem5 devkit Marco Felsch @ 2019-04-16 16:38 ` Angus Ainslie 2019-04-16 16:55 ` Marco Felsch 0 siblings, 1 reply; 11+ messages in thread From: Angus Ainslie @ 2019-04-16 16:38 UTC (permalink / raw) To: Marco Felsch Cc: Mark Rutland, devicetree, Shawn Guo, Sascha Hauer, linux-kernel, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel On 2019-04-16 00:58, Marco Felsch wrote: > Hi Agnus, > > On 19-04-15 16:53, Angus Ainslie (Purism) wrote: >> The Librem5 devkit is based on the imx8mq from NXP. This is a default >> devicetree to boot the board to a command prompt. > > You should also provide a patch which adds the devicetree to the yaml > file, see: Documentation/devicetree/bindings/arm/fsl.yaml. > Sure, what is that yaml file for ? Angus > Thanks, > Marco > >> >> Changes since v3: >> >> Freshly sorted and pressed nodes. >> Change the backlight to an interpolated scale. >> Dropped i2c2. >> Dropped devkit version number to match debian MR. >> >> Changes since v2: >> >> Fixed incorrect phy-supply for the fsl-fec. >> Dropped unused regulator property. >> Fixup Makefile for linux-next. >> >> Changes since v1: >> >> Dropped config file. >> Updated the board compatible label. >> Changed node names to follow naming conventions. >> Added a more complete regulator hierachy. >> Removed unused nodes. >> Removed unknown devices. >> Fixed comment style. >> Dropped undocumented properties. >> >> Angus Ainslie (Purism) (2): >> arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit >> dt-bindings: Add an entry for Purism SPC >> >> .../devicetree/bindings/vendor-prefixes.txt | 1 + >> arch/arm64/boot/dts/freescale/Makefile | 1 + >> .../dts/freescale/imx8mq-librem5-devkit.dts | 822 >> ++++++++++++++++++ >> 3 files changed, 824 insertions(+) >> create mode 100644 >> arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts >> >> -- >> 2.17.1 >> >> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v4 0/2] Add support for the Purism Librem5 devkit 2019-04-16 16:38 ` Angus Ainslie @ 2019-04-16 16:55 ` Marco Felsch 0 siblings, 0 replies; 11+ messages in thread From: Marco Felsch @ 2019-04-16 16:55 UTC (permalink / raw) To: Angus Ainslie Cc: Mark Rutland, devicetree, Fabio Estevam, Sascha Hauer, linux-kernel, Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Shawn Guo, linux-arm-kernel On 19-04-16 09:38, Angus Ainslie wrote: > On 2019-04-16 00:58, Marco Felsch wrote: > > Hi Agnus, > > > > On 19-04-15 16:53, Angus Ainslie (Purism) wrote: > > > The Librem5 devkit is based on the imx8mq from NXP. This is a default > > > devicetree to boot the board to a command prompt. > > > > You should also provide a patch which adds the devicetree to the yaml > > file, see: Documentation/devicetree/bindings/arm/fsl.yaml. > > > > Sure, what is that yaml file for ? Thats for Rob's the validation: https://lwn.net/Articles/771621/ Regards, Marco > > Angus > > > Thanks, > > Marco > > > > > > > > Changes since v3: > > > > > > Freshly sorted and pressed nodes. > > > Change the backlight to an interpolated scale. > > > Dropped i2c2. > > > Dropped devkit version number to match debian MR. > > > > > > Changes since v2: > > > > > > Fixed incorrect phy-supply for the fsl-fec. > > > Dropped unused regulator property. > > > Fixup Makefile for linux-next. > > > > > > Changes since v1: > > > > > > Dropped config file. > > > Updated the board compatible label. > > > Changed node names to follow naming conventions. > > > Added a more complete regulator hierachy. > > > Removed unused nodes. > > > Removed unknown devices. > > > Fixed comment style. > > > Dropped undocumented properties. > > > > > > Angus Ainslie (Purism) (2): > > > arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit > > > dt-bindings: Add an entry for Purism SPC > > > > > > .../devicetree/bindings/vendor-prefixes.txt | 1 + > > > arch/arm64/boot/dts/freescale/Makefile | 1 + > > > .../dts/freescale/imx8mq-librem5-devkit.dts | 822 > > > ++++++++++++++++++ > > > 3 files changed, 824 insertions(+) > > > create mode 100644 > > > arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts > > > > > > -- > > > 2.17.1 > > > > > > > > > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-04-17 6:54 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-04-15 23:53 [PATCH v4 0/2] Add support for the Purism Librem5 devkit Angus Ainslie (Purism) 2019-04-15 23:53 ` [PATCH v4 1/2] arm64: dts: fsl: librem5: Add a device tree for the " Angus Ainslie (Purism) 2019-04-16 7:54 ` Marco Felsch 2019-04-16 18:02 ` Angus Ainslie 2019-04-16 21:20 ` Marco Felsch 2019-04-16 22:36 ` Angus Ainslie 2019-04-17 6:54 ` Marco Felsch 2019-04-15 23:53 ` [PATCH v4 2/2] dt-bindings: Add an entry for Purism SPC Angus Ainslie (Purism) 2019-04-16 7:58 ` [PATCH v4 0/2] Add support for the Purism Librem5 devkit Marco Felsch 2019-04-16 16:38 ` Angus Ainslie 2019-04-16 16:55 ` Marco Felsch
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox