Re: [PATCH] arm64: dts: Add a device tree file for Emtop SOM IMX8MM

[Krzysztof Kozlowski]

On 29/04/2023 15:44, Himanshu Bhavani wrote:
> From 9b2f563e1286c960ad81bc78ec811029ffb2f57a Mon Sep 17 00:00:00 2001
> From: Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>
> Date: Sat, 29 Apr 2023 19:03:53 +0530
> Subject: [PATCH] Add a device tree file for Emtop SOM IMX8MM

Fix your email setup.

> 
> Added dts for describing the Emtop SOM-IMX8MM
> 
> Signed-off-by: Himanshu Bhavani <himanshu.bhavani@xxxxxxxxxxxxxxxxx>
> 
> diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> index 198fff3731ae..8f281c753e5a 100644
> --- a/arch/arm64/boot/dts/freescale/Makefile
> +++ b/arch/arm64/boot/dts/freescale/Makefile
> @@ -79,6 +79,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mm-verdin-nonwifi-yavia.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mm-verdin-wifi-dahlia.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mm-verdin-wifi-dev.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mm-verdin-wifi-yavia.dtb
> +dtb-$(CONFIG_ARCH_MXC) += imx8mm-emtop.dtb

Keep order.

>  dtb-$(CONFIG_ARCH_MXC) += imx8mn-beacon-kit.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mn-bsh-smm-s2.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mn-bsh-smm-s2pro.dtb
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-emtop.dts b/arch/arm64/boot/dts/freescale/imx8mm-emtop.dts
> new file mode 100644
> index 000000000000..5238a3b5f4cf
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-emtop.dts
> @@ -0,0 +1,262 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2019 NXP
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/usb/pd.h>
> +#include "imx8mm.dtsi"
> +
> +/ {
> +	model = "FSL i.MX8MM Emtop SOM";
> +	compatible = "fsl,imx8mm-emtop", "fsl,imx8mm-evk", "fsl,imx8mm";

I commented on bindings - this does not look correct at all.

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

> +
> +	chosen {
> +		stdout-path = &uart2;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpio_led>;
> +
> +		sys {

missing proper prefix/name.
Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

> +			label = "sys";
> +			gpios = <&gpio3 16 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "heartbeat";
> +		};
> +	};
> +};
> +
> +&A53_0 {
> +	cpu-supply = <&buck2>;
> +};
> +
> +&A53_1 {
> +	cpu-supply = <&buck2>;
> +};
> +
> +&A53_2 {
> +	cpu-supply = <&buck2>;
> +};
> +
> +&A53_3 {
> +	cpu-supply = <&buck2>;
> +};
> +
> +&uart2 { /* console */
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_uart2>;
> +	status = "okay";
> +};
> +
> +/* eMMC */
> +&usdhc3 {
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
> +	pinctrl-0 = <&pinctrl_usdhc3>;
> +	pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
> +	pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
> +	bus-width = <8>;
> +	non-removable;
> +	status = "okay";
> +};
> +
> +&wdog1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_wdog>;
> +	fsl,ext-reset-output;
> +	status = "okay";
> +};
> +
> +&i2c1 {
> +	clock-frequency = <400000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c1>;
> +	status = "okay";
> +
> +	pmic@25 {
> +		reg = <0x25>;
> +		compatible = "nxp,pca9450";
> +		/* PMIC PCA9450 PMIC_nINT GPIO1_IO3 */
> +		pinctrl-0 = <&pinctrl_pmic>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <3 IRQ_TYPE_EDGE_RISING>;
> +
> +		regulators {
> +

Stray blank line.

...

> +	pinctrl_usdhc3_100mhz: usdhc3grp100mhz {

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions).

> +		fsl,pins = <
> +			MX8MM_IOMUXC_NAND_WE_B_USDHC3_CLK			0x194
> +			MX8MM_IOMUXC_NAND_WP_B_USDHC3_CMD			0x1d4
> +			MX8MM_IOMUXC_NAND_DATA04_USDHC3_DATA0			0x1d4
> +			MX8MM_IOMUXC_NAND_DATA05_USDHC3_DATA1			0x1d4
> +			MX8MM_IOMUXC_NAND_DATA06_USDHC3_DATA2			0x1d4
> +			MX8MM_IOMUXC_NAND_DATA07_USDHC3_DATA3			0x1d4
> +			MX8MM_IOMUXC_NAND_RE_B_USDHC3_DATA4			0x1d4
> +			MX8MM_IOMUXC_NAND_CE2_B_USDHC3_DATA5			0x1d4
> +			MX8MM_IOMUXC_NAND_CE3_B_USDHC3_DATA6			0x1d4
> +			MX8MM_IOMUXC_NAND_CLE_USDHC3_DATA7			0x1d4
> +			MX8MM_IOMUXC_NAND_CE1_B_USDHC3_STROBE			0x194
> +		>;
> +	};
> +
> +	pinctrl_usdhc3_200mhz: usdhc3grp200mhz {

Here as well.


Best regards,
Krzysztof