| Message ID | 20221217234340.819752-1-aford173@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [V2,1/2] dt-bindings: arm: Add Beacon EmbeddedWorks i.MX8M Plus kit | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 18/12/2022 00:43, Adam Ford wrote: > Beacon Embedded has an i.MX8M Plus development kit which consists > of a SOM + baseboard. The SOM includes Bluetooth, WiFi, QSPI, eMMC, > and one Ethernet PHY. The baseboard includes audio, HDMI, USB-C Dual > Role port, USB Hub with five ports, a PCIe slot, and a second Ethernet > PHY. > > Signed-off-by: Adam Ford <aford173@gmail.com> > --- > V2: Fix whitespace, remove dead nodes, and fix some node and > parameter names to address errors in 'make dt_binding_check' You still cannot check/test/build this DTS without Makefile. > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts > new file mode 100644 > index 000000000000..70399f2419cf > --- /dev/null > +++ b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts > @@ -0,0 +1,549 @@ > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > +/* > + * Copyright 2022 Logic PD, Inc dba Beacon EmbeddedWorks > + */ > + > +/dts-v1/; > + > +#include <dt-bindings/usb/pd.h> > +#include <dt-bindings/phy/phy-imx8-pcie.h> > +#include "imx8mp.dtsi" > +#include "imx8mp-beacon-som.dtsi" > + > +/ { > + model = "Beacon EmbeddedWorks i.MX8MPlus Development kit"; > + compatible = "beacon,imx8mp-beacon-kit", "fsl,imx8mp"; > + > + aliases { > + ethernet0 = &eqos; > + ethernet1 = &fec; > + }; > + > + chosen { > + stdout-path = &uart2; > + }; > + > + connector-1 { > + compatible = "usb-c-connector"; > + label = "USB-C"; > + data-role = "dual"; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + hs_ep: endpoint { > + remote-endpoint = <&usb3_hs_ep>; > + }; > + }; > + port@1 { > + reg = <1>; > + ss_ep: endpoint { > + remote-endpoint = <&hd3ss3220_in_ep>; > + }; > + }; > + }; > + }; > + > + gpio-keys { > + compatible = "gpio-keys"; > + autorepeat; > + > + button-0 { > + label = "btn0"; > + linux,code = <BTN_0>; > + gpios = <&pca6416_1 12 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; > + wakeup-source; > + }; > + > + button-1 { > + label = "btn1"; > + linux,code = <BTN_1>; > + gpios = <&pca6416_1 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; > + wakeup-source; > + }; > + > + button-2 { > + label = "btn2"; > + linux,code = <BTN_2>; > + gpios = <&pca6416_1 14 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; > + wakeup-source; > + }; > + > + button-3 { > + label = "btn3"; > + linux,code = <BTN_3>; > + gpios = <&pca6416_1 15 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; > + wakeup-source; > + }; > + }; > + > + leds { > + compatible = "gpio-leds"; > + > + led0 { No improvements here. > + label = "gen_led0"; > + gpios = <&pca6416_1 4 GPIO_ACTIVE_HIGH>; > + default-state = "off"; > + }; > + > + led1 { > + label = "gen_led1"; > + gpios = <&pca6416_1 5 GPIO_ACTIVE_HIGH>; > + default-state = "off"; > + }; > + > + led2 { > + label = "gen_led2"; > + gpios = <&pca6416_1 6 GPIO_ACTIVE_HIGH>; > + default-state = "off"; > + }; > + > + led3 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_led3>; > + label = "heartbeat"; > + gpios = <&gpio4 28 GPIO_ACTIVE_HIGH>; > + linux,default-trigger = "heartbeat"; > + }; > + }; > + > + pcie0_refclk: pcie0-refclk { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <100000000>; > + }; > + > + reg_usdhc2_vmmc: regulator-usdhc2 { > + compatible = "regulator-fixed"; > + regulator-name = "VSD_3V3"; > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + startup-delay-us = <100>; > + off-on-delay-us = <20000>; > + }; > + > + reg_usb1_host_vbus: regulator-usb1-vbus { > + compatible = "regulator-fixed"; > + regulator-name = "usb1_host_vbus"; > + regulator-max-microvolt = <5000000>; > + regulator-min-microvolt = <5000000>; > + gpio = <&pca6416_1 0 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + }; > +}; > + > +&ecspi2 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_ecspi2>; > + cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>; > + status = "okay"; > + > + tpm: tpm@0 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_tpm>; > + compatible = "infineon,slb9670"; > + reg = <0>; > + reset-gpios = <&gpio4 0 GPIO_ACTIVE_LOW>; > + spi-max-frequency = <18500000>; > + }; > +}; > + > +&fec { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_fec>; > + phy-mode = "rgmii-id"; > + phy-handle = <ðphy1>; > + phy-reset-gpios = <&gpio4 18 GPIO_ACTIVE_LOW>; > + phy-reset-post-delay = <150>; > + phy-reset-duration = <10>; > + fsl,magic-packet; > + status = "okay"; > + > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + ethphy1: ethernet1-phy@3 { > + compatible = "ethernet-phy-ieee802.3-c22"; > + reg = <3>; > + }; > + }; > +}; > + > +&flexcan1 { > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_flexcan1>; > + status = "okay"; > +}; > + > + > +&i2c2 { > + clock-frequency = <384000>; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_i2c2>; > + status = "okay"; > + > + pca6416_3: gpio-i2c2@20 { No improvements. > + compatible = "nxp,pcal6416"; > + reg = <0x20>; > + gpio-controller; > + #gpio-cells = <2>; > + interrupt-parent = <&gpio4>; > + interrupts = <27 IRQ_TYPE_EDGE_FALLING>; > + interrupt-controller; > + #interrupt-cells = <2>; > + }; > +}; > + > +&i2c3 { > + /* Connected to USB Hub */ > + ptn5110: ptn5110@52 { No improvements ... I'll stop reviewing. No point if you do not intend to apply the comments. Best regards, Krzysztof
On Sun, Dec 18, 2022 at 12:42 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 18/12/2022 00:43, Adam Ford wrote: > > Beacon Embedded has an i.MX8M Plus development kit which consists > > of a SOM + baseboard. The SOM includes Bluetooth, WiFi, QSPI, eMMC, > > and one Ethernet PHY. The baseboard includes audio, HDMI, USB-C Dual > > Role port, USB Hub with five ports, a PCIe slot, and a second Ethernet > > PHY. > > > > Signed-off-by: Adam Ford <aford173@gmail.com> > > --- > > V2: Fix whitespace, remove dead nodes, and fix some node and > > parameter names to address errors in 'make dt_binding_check' > > You still cannot check/test/build this DTS without Makefile. The lack of makefile was an honest mistake as I forgot to add it to my patch. I ran the make dt_binding_check and addressed many of the errors, but some of them are not unique to my board, because they were errors in the imx8mp.dtsi file. I addressed some of them by sending patches to fix some types in imx8mp.dtsi. I will make sure V3 has the makefile updated. > > > > > diff --git a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts > > new file mode 100644 > > index 000000000000..70399f2419cf > > --- /dev/null > > +++ b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts > > @@ -0,0 +1,549 @@ > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > > +/* > > + * Copyright 2022 Logic PD, Inc dba Beacon EmbeddedWorks > > + */ > > + > > +/dts-v1/; > > + > > +#include <dt-bindings/usb/pd.h> > > +#include <dt-bindings/phy/phy-imx8-pcie.h> > > +#include "imx8mp.dtsi" > > +#include "imx8mp-beacon-som.dtsi" > > + > > +/ { > > + model = "Beacon EmbeddedWorks i.MX8MPlus Development kit"; > > + compatible = "beacon,imx8mp-beacon-kit", "fsl,imx8mp"; > > + > > + aliases { > > + ethernet0 = &eqos; > > + ethernet1 = &fec; > > + }; > > + > > + chosen { > > + stdout-path = &uart2; > > + }; > > + > > + connector-1 { > > + compatible = "usb-c-connector"; > > + label = "USB-C"; > > + data-role = "dual"; > > + > > + ports { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + port@0 { > > + reg = <0>; > > + hs_ep: endpoint { > > + remote-endpoint = <&usb3_hs_ep>; > > + }; > > + }; > > + port@1 { > > + reg = <1>; > > + ss_ep: endpoint { > > + remote-endpoint = <&hd3ss3220_in_ep>; > > + }; > > + }; > > + }; > > + }; > > + > > + gpio-keys { > > + compatible = "gpio-keys"; > > + autorepeat; > > + > > + button-0 { > > + label = "btn0"; > > + linux,code = <BTN_0>; > > + gpios = <&pca6416_1 12 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; > > + wakeup-source; > > + }; > > + > > + button-1 { > > + label = "btn1"; > > + linux,code = <BTN_1>; > > + gpios = <&pca6416_1 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; > > + wakeup-source; > > + }; > > + > > + button-2 { > > + label = "btn2"; > > + linux,code = <BTN_2>; > > + gpios = <&pca6416_1 14 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; > > + wakeup-source; > > + }; > > + > > + button-3 { > > + label = "btn3"; > > + linux,code = <BTN_3>; > > + gpios = <&pca6416_1 15 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; > > + wakeup-source; > > + }; > > + }; > > + > > + leds { > > + compatible = "gpio-leds"; > > + > > + led0 { > > No improvements here. I forgot the git commit --amend so my patch was missing the changes, but my machine has the fix. Sorry about that. > > > + label = "gen_led0"; > > + gpios = <&pca6416_1 4 GPIO_ACTIVE_HIGH>; > > + default-state = "off"; > > + }; > > + > > + led1 { > > + label = "gen_led1"; > > + gpios = <&pca6416_1 5 GPIO_ACTIVE_HIGH>; > > + default-state = "off"; > > + }; > > + > > + led2 { > > + label = "gen_led2"; > > + gpios = <&pca6416_1 6 GPIO_ACTIVE_HIGH>; > > + default-state = "off"; > > + }; > > + > > + led3 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_led3>; > > + label = "heartbeat"; > > + gpios = <&gpio4 28 GPIO_ACTIVE_HIGH>; > > + linux,default-trigger = "heartbeat"; > > + }; > > + }; > > + > > + pcie0_refclk: pcie0-refclk { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <100000000>; > > + }; > > + > > + reg_usdhc2_vmmc: regulator-usdhc2 { > > + compatible = "regulator-fixed"; > > + regulator-name = "VSD_3V3"; > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > + startup-delay-us = <100>; > > + off-on-delay-us = <20000>; > > + }; > > + > > + reg_usb1_host_vbus: regulator-usb1-vbus { > > + compatible = "regulator-fixed"; > > + regulator-name = "usb1_host_vbus"; > > + regulator-max-microvolt = <5000000>; > > + regulator-min-microvolt = <5000000>; > > + gpio = <&pca6416_1 0 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > + }; > > +}; > > + > > +&ecspi2 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_ecspi2>; > > + cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>; > > + status = "okay"; > > + > > + tpm: tpm@0 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_tpm>; > > + compatible = "infineon,slb9670"; > > + reg = <0>; > > + reset-gpios = <&gpio4 0 GPIO_ACTIVE_LOW>; > > + spi-max-frequency = <18500000>; > > + }; > > +}; > > + > > +&fec { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_fec>; > > + phy-mode = "rgmii-id"; > > + phy-handle = <ðphy1>; > > + phy-reset-gpios = <&gpio4 18 GPIO_ACTIVE_LOW>; > > + phy-reset-post-delay = <150>; > > + phy-reset-duration = <10>; > > + fsl,magic-packet; > > + status = "okay"; > > + > > + mdio { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + ethphy1: ethernet1-phy@3 { > > + compatible = "ethernet-phy-ieee802.3-c22"; > > + reg = <3>; > > + }; > > + }; > > +}; > > + > > +&flexcan1 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_flexcan1>; > > + status = "okay"; > > +}; > > + > > + > > +&i2c2 { > > + clock-frequency = <384000>; > > + pinctrl-names = "default"; > > + pinctrl-0 = <&pinctrl_i2c2>; > > + status = "okay"; > > + > > + pca6416_3: gpio-i2c2@20 { > > No improvements. I am not sure what I am supposed to do here. I already have one node called "gpio@20" and as far as I know, I can't have duplicate nodes with the same name. I have the same GPIO expander with the same on a different i2c bus. Do you have a recommendation as to what name I should use so the names are unique? > > > + compatible = "nxp,pcal6416"; > > + reg = <0x20>; > > + gpio-controller; > > + #gpio-cells = <2>; > > + interrupt-parent = <&gpio4>; > > + interrupts = <27 IRQ_TYPE_EDGE_FALLING>; > > + interrupt-controller; > > + #interrupt-cells = <2>; > > + }; > > +}; > > + > > +&i2c3 { > > + /* Connected to USB Hub */ > > + ptn5110: ptn5110@52 { > > No improvements ... I'll stop reviewing. No point if you do not intend > to apply the comments. I changed this node from tcpc to ptn5110 per your last comment, and it matches other boards that use this and Documentation/devicetree/bindings/connector/usb-connector.yaml example uses ptn5110. Documentation/devicetree/bindings/usb/typec-tcpci.txt just called it "ptn5110@50" . imx8mm-evk.dtsi calls it "ptn5110: tcpc@50" so that's the example I followed. I am guessing it fails the build test because there isn't a yaml file for this driver. I wasn't intentionally ignoring your comments, and I honestly tried to catch them. If you let me know how you want me to proceed, I will do a V3. thank you, adam > > > Best regards, > Krzysztof >
On 18/12/2022 20:00, Adam Ford wrote: > On Sun, Dec 18, 2022 at 12:42 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 18/12/2022 00:43, Adam Ford wrote: >>> Beacon Embedded has an i.MX8M Plus development kit which consists >>> of a SOM + baseboard. The SOM includes Bluetooth, WiFi, QSPI, eMMC, >>> and one Ethernet PHY. The baseboard includes audio, HDMI, USB-C Dual >>> Role port, USB Hub with five ports, a PCIe slot, and a second Ethernet >>> PHY. >>> >>> Signed-off-by: Adam Ford <aford173@gmail.com> >>> --- >>> V2: Fix whitespace, remove dead nodes, and fix some node and >>> parameter names to address errors in 'make dt_binding_check' >> >> You still cannot check/test/build this DTS without Makefile. > > The lack of makefile was an honest mistake as I forgot to add it to my > patch. I ran the make dt_binding_check and addressed many of the > errors, but some of them are not unique to my board, because they were > errors in the imx8mp.dtsi file. I addressed some of them by sending > patches to fix some types in imx8mp.dtsi. I will make sure V3 has the > makefile updated. > >> >>> >>> diff --git a/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts >>> new file mode 100644 >>> index 000000000000..70399f2419cf >>> --- /dev/null >>> +++ b/arch/arm64/boot/dts/freescale/imx8mp-beacon-kit.dts >>> @@ -0,0 +1,549 @@ >>> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) >>> +/* >>> + * Copyright 2022 Logic PD, Inc dba Beacon EmbeddedWorks >>> + */ >>> + >>> +/dts-v1/; >>> + >>> +#include <dt-bindings/usb/pd.h> >>> +#include <dt-bindings/phy/phy-imx8-pcie.h> >>> +#include "imx8mp.dtsi" >>> +#include "imx8mp-beacon-som.dtsi" >>> + >>> +/ { >>> + model = "Beacon EmbeddedWorks i.MX8MPlus Development kit"; >>> + compatible = "beacon,imx8mp-beacon-kit", "fsl,imx8mp"; >>> + >>> + aliases { >>> + ethernet0 = &eqos; >>> + ethernet1 = &fec; >>> + }; >>> + >>> + chosen { >>> + stdout-path = &uart2; >>> + }; >>> + >>> + connector-1 { >>> + compatible = "usb-c-connector"; >>> + label = "USB-C"; >>> + data-role = "dual"; >>> + >>> + ports { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + port@0 { >>> + reg = <0>; >>> + hs_ep: endpoint { >>> + remote-endpoint = <&usb3_hs_ep>; >>> + }; >>> + }; >>> + port@1 { >>> + reg = <1>; >>> + ss_ep: endpoint { >>> + remote-endpoint = <&hd3ss3220_in_ep>; >>> + }; >>> + }; >>> + }; >>> + }; >>> + >>> + gpio-keys { >>> + compatible = "gpio-keys"; >>> + autorepeat; >>> + >>> + button-0 { >>> + label = "btn0"; >>> + linux,code = <BTN_0>; >>> + gpios = <&pca6416_1 12 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; >>> + wakeup-source; >>> + }; >>> + >>> + button-1 { >>> + label = "btn1"; >>> + linux,code = <BTN_1>; >>> + gpios = <&pca6416_1 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; >>> + wakeup-source; >>> + }; >>> + >>> + button-2 { >>> + label = "btn2"; >>> + linux,code = <BTN_2>; >>> + gpios = <&pca6416_1 14 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; >>> + wakeup-source; >>> + }; >>> + >>> + button-3 { >>> + label = "btn3"; >>> + linux,code = <BTN_3>; >>> + gpios = <&pca6416_1 15 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>; >>> + wakeup-source; >>> + }; >>> + }; >>> + >>> + leds { >>> + compatible = "gpio-leds"; >>> + >>> + led0 { >> >> No improvements here. > I forgot the git commit --amend so my patch was missing the changes, > but my machine has the fix. Sorry about that. > >> >>> + label = "gen_led0"; >>> + gpios = <&pca6416_1 4 GPIO_ACTIVE_HIGH>; >>> + default-state = "off"; >>> + }; >>> + >>> + led1 { >>> + label = "gen_led1"; >>> + gpios = <&pca6416_1 5 GPIO_ACTIVE_HIGH>; >>> + default-state = "off"; >>> + }; >>> + >>> + led2 { >>> + label = "gen_led2"; >>> + gpios = <&pca6416_1 6 GPIO_ACTIVE_HIGH>; >>> + default-state = "off"; >>> + }; >>> + >>> + led3 { >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&pinctrl_led3>; >>> + label = "heartbeat"; >>> + gpios = <&gpio4 28 GPIO_ACTIVE_HIGH>; >>> + linux,default-trigger = "heartbeat"; >>> + }; >>> + }; >>> + >>> + pcie0_refclk: pcie0-refclk { >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-frequency = <100000000>; >>> + }; >>> + >>> + reg_usdhc2_vmmc: regulator-usdhc2 { >>> + compatible = "regulator-fixed"; >>> + regulator-name = "VSD_3V3"; >>> + regulator-min-microvolt = <3300000>; >>> + regulator-max-microvolt = <3300000>; >>> + gpio = <&gpio2 19 GPIO_ACTIVE_HIGH>; >>> + enable-active-high; >>> + startup-delay-us = <100>; >>> + off-on-delay-us = <20000>; >>> + }; >>> + >>> + reg_usb1_host_vbus: regulator-usb1-vbus { >>> + compatible = "regulator-fixed"; >>> + regulator-name = "usb1_host_vbus"; >>> + regulator-max-microvolt = <5000000>; >>> + regulator-min-microvolt = <5000000>; >>> + gpio = <&pca6416_1 0 GPIO_ACTIVE_HIGH>; >>> + enable-active-high; >>> + }; >>> +}; >>> + >>> +&ecspi2 { >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&pinctrl_ecspi2>; >>> + cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>; >>> + status = "okay"; >>> + >>> + tpm: tpm@0 { >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&pinctrl_tpm>; >>> + compatible = "infineon,slb9670"; >>> + reg = <0>; >>> + reset-gpios = <&gpio4 0 GPIO_ACTIVE_LOW>; >>> + spi-max-frequency = <18500000>; >>> + }; >>> +}; >>> + >>> +&fec { >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&pinctrl_fec>; >>> + phy-mode = "rgmii-id"; >>> + phy-handle = <ðphy1>; >>> + phy-reset-gpios = <&gpio4 18 GPIO_ACTIVE_LOW>; >>> + phy-reset-post-delay = <150>; >>> + phy-reset-duration = <10>; >>> + fsl,magic-packet; >>> + status = "okay"; >>> + >>> + mdio { >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + >>> + ethphy1: ethernet1-phy@3 { >>> + compatible = "ethernet-phy-ieee802.3-c22"; >>> + reg = <3>; >>> + }; >>> + }; >>> +}; >>> + >>> +&flexcan1 { >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&pinctrl_flexcan1>; >>> + status = "okay"; >>> +}; >>> + >>> + >>> +&i2c2 { >>> + clock-frequency = <384000>; >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&pinctrl_i2c2>; >>> + status = "okay"; >>> + >>> + pca6416_3: gpio-i2c2@20 { >> >> No improvements. > > I am not sure what I am supposed to do here. I already have one node > called "gpio@20" and as far as I know, I can't have duplicate nodes > with the same name. I have the same GPIO expander with the same on a > different i2c bus. Do you have a recommendation as to what name I > should use so the names are unique? > >> >>> + compatible = "nxp,pcal6416"; >>> + reg = <0x20>; >>> + gpio-controller; >>> + #gpio-cells = <2>; >>> + interrupt-parent = <&gpio4>; >>> + interrupts = <27 IRQ_TYPE_EDGE_FALLING>; >>> + interrupt-controller; >>> + #interrupt-cells = <2>; >>> + }; >>> +}; >>> + >>> +&i2c3 { >>> + /* Connected to USB Hub */ >>> + ptn5110: ptn5110@52 { >> >> No improvements ... I'll stop reviewing. No point if you do not intend >> to apply the comments. > > I changed this node from tcpc to ptn5110 per your last comment, and it > matches other boards that use this and > Documentation/devicetree/bindings/connector/usb-connector.yaml example > uses ptn5110. > Documentation/devicetree/bindings/usb/typec-tcpci.txt just called it > "ptn5110@50" . My comment was that node name has to be generic. ptn5110 is not generic but specific. I gave you reference to examples of generic names but in other boards there are more examples how this could be named. > > imx8mm-evk.dtsi calls it "ptn5110: tcpc@50" so that's the example I > followed. I am guessing it fails the build test because there isn't a > yaml file for this driver. > > I wasn't intentionally ignoring your comments, and I honestly tried to > catch them. > > If you let me know how you want me to proceed, I will do a V3. I stopped reviewing. Implement all the comments but if something is not clear, just ask for clarification. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml index 05b5276a0e14..4a92e0b2b890 100644 --- a/Documentation/devicetree/bindings/arm/fsl.yaml +++ b/Documentation/devicetree/bindings/arm/fsl.yaml @@ -931,6 +931,7 @@ properties: - description: i.MX8MP based Boards items: - enum: + - beacon,imx8mp-beacon-kit # i.MX8MP Beacon Development Kit - dh,imx8mp-dhcom-som # i.MX8MP DHCOM SoM - dh,imx8mp-dhcom-pdk2 # i.MX8MP DHCOM SoM on PDK2 board - fsl,imx8mp-evk # i.MX8MP EVK Board