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

Krzysztof Kozlowski krzysztof.kozlowski at linaro.org
Mon May 1 10:31:05 PDT 2023


On 01/05/2023 18:39, Himanshu Bhavani wrote:
> From 33b96ec2602598f2a29e876c1f83b101d88faf2e Mon Sep 17 00:00:00 2001
> From: Himanshu Bhavani <himanshu.bhavani at siliconsignals.io>

Use subject prefixes matching the subsystem (which you can get for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching). It is still wrong.

> Date: Mon, 1 May 2023 21:50:44 +0530
> Subject: [PATCH] Added dts for describing the Emtop SOM-IMX8MM
> 

This is still not applicable patch. This is not a patch at all.


> Changes in v2:
> 	- Update dtb add order in Makefile
> 	- Update bindings
> 	- Update proper prefix/name in dts
> 	- Removed stray blank line
> 	- Add pinctrl-names in pmic node

This is not commit msg, but changelog so under ---.

> 
> Signed-off-by: Himanshu Bhavani <himanshu.bhavani at siliconsignals.io>
> 
> diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> index 198fff3731ae..36590515fbc1 100644
> --- a/arch/arm64/boot/dts/freescale/Makefile
> +++ b/arch/arm64/boot/dts/freescale/Makefile
> @@ -54,6 +54,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mm-beacon-kit.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mm-data-modul-edm-sbc.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mm-ddr4-evk.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mm-emcon-avari.dtb
> +dtb-$(CONFIG_ARCH_MXC) += imx8mm-emtop.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mm-evk.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mm-icore-mx8mm-ctouch2.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mm-icore-mx8mm-edimm2.2.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..5c569bbedc69
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-emtop.dts
> @@ -0,0 +1,276 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2023 Emtop
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/usb/pd.h>
> +
> +#include "imx8mm.dtsi"
> +
> +/ {
> +	model = "Emtop SOM i.MX8MM";
> +	compatible = "emtop,imx8mm-emtop", "fsl,imx8mm";

You still miss bindings. Don't send them separately, but as part of
patchset. There is extensive guide how to do it.
https://elixir.bootlin.com/linux/v5.19-rc5/source/Documentation/process/submitting-patches.rst

> +
> +	chosen {
> +		stdout-path = &uart2;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpio_led>;
> +		
> +		led-0 {
> +			function = LED_FUNCTION_POWER;
> +			gpios = <&gpio3 16 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "heartbeat";
> +		};
> +	};
> +/*	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpio_led>;
> +
> +		sys {
> +			label = "sys";
> +			gpios = <&gpio3 16 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "heartbeat";
> +		};
> +	};*/

Don't add dead code to upstream.

> +};
> +
> +&A53_0 {
> +	cpu-supply = <&buck2>;

Run checkpatch. Fix indentation to tabs,

> +};
> +
> +&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 */

Keep one style of comments.

> +&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 at 25 {
> +		compatible = "nxp,pca9450";
> +		reg = <0x25>;
> +		pinctrl-names = "default";
> +		/* PMIC PCA9450 PMIC_nINT GPIO1_IO3 */
> +		pinctrl-0 = <&pinctrl_pmic>;
> +		interrupt-parent = <&gpio1>;
> +		interrupts = <3 IRQ_TYPE_EDGE_RISING>;
> +
> +		regulators {
> +			buck1: BUCK1 {
> +				regulator-compatible = "BUCK1";
> +				regulator-min-microvolt = <800000>;
> +				regulator-max-microvolt = <1000000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +				regulator-ramp-delay = <3125>;
> +			};
> +
> +			buck2: BUCK2 {
> +				regulator-compatible = "BUCK2";
> +				regulator-min-microvolt = <800000>;
> +				regulator-max-microvolt = <900000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +				regulator-ramp-delay = <3125>;
> +			};
> +
> +			buck3: BUCK3 {
> +				regulator-compatible = "BUCK3";
> +				regulator-min-microvolt = <800000>;
> +				regulator-max-microvolt = <1000000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			buck4: BUCK4 {
> +				regulator-compatible = "BUCK4";
> +				regulator-min-microvolt = <3000000>;
> +				regulator-max-microvolt = <3600000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			buck5: BUCK5 {
> +				regulator-compatible = "BUCK5";
> +				regulator-min-microvolt = <1650000>;
> +				regulator-max-microvolt = <1950000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			buck6: BUCK6 {
> +				regulator-compatible = "BUCK6";
> +				regulator-min-microvolt = <1100000>;
> +				regulator-max-microvolt = <1200000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo1: LDO1 {
> +				regulator-compatible = "LDO1";
> +				regulator-min-microvolt = <1650000>;
> +				regulator-max-microvolt = <1950000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo2: LDO2 {
> +				regulator-compatible = "LDO2";
> +				regulator-min-microvolt = <800000>;
> +				regulator-max-microvolt = <945000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo3: LDO3 {
> +				regulator-compatible = "LDO3";
> +				regulator-min-microvolt = <1710000>;
> +				regulator-max-microvolt = <1890000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo4: LDO4 {
> +				regulator-compatible = "LDO4";
> +				regulator-min-microvolt = <810000>;
> +				regulator-max-microvolt = <945000>;
> +				regulator-boot-on;
> +				regulator-always-on;
> +			};
> +
> +			ldo5: LDO5 {
> +				regulator-compatible = "LDO5";
> +				regulator-min-microvolt = <1650000>;
> +				regulator-max-microvolt = <3600000>;
> +			};
> +		};
> +	};
> +};
> +
> +&iomuxc {
> +	pinctrl-names = "default";
> +
> +	pinctrl_gpio_led: gpioledgrp {
> +		fsl,pins = <
> +			MX8MM_IOMUXC_NAND_READY_B_GPIO3_IO16			0x19
> +			MX8MM_IOMUXC_SAI3_RXC_GPIO4_IO29			0x19
> +		>;
> +	};
> +
> +	pinctrl_i2c1: i2c1grp {
> +		fsl,pins = <
> +			MX8MM_IOMUXC_I2C1_SCL_I2C1_SCL				0x400001c3
> +			MX8MM_IOMUXC_I2C1_SDA_I2C1_SDA				0x400001c3
> +		>;
> +	};
> +
> +	pinctrl_pmic: pmicirq {
> +		fsl,pins = <
> +			MX8MM_IOMUXC_GPIO1_IO03_GPIO1_IO3			0x41
> +		>;
> +	};
> +
> +	pinctrl_uart2: uart2grp {
> +		fsl,pins = <
> +			MX8MM_IOMUXC_UART2_RXD_UART2_DCE_RX			0x140
> +			MX8MM_IOMUXC_UART2_TXD_UART2_DCE_TX			0x140
> +		>;
> +	};
> +
> +	pinctrl_usdhc3: usdhc3grp {
> +		fsl,pins = <
> +			MX8MM_IOMUXC_NAND_WE_B_USDHC3_CLK			0x190
> +			MX8MM_IOMUXC_NAND_WP_B_USDHC3_CMD			0x1d0
> +			MX8MM_IOMUXC_NAND_DATA04_USDHC3_DATA0			0x1d0
> +			MX8MM_IOMUXC_NAND_DATA05_USDHC3_DATA1			0x1d0
> +			MX8MM_IOMUXC_NAND_DATA06_USDHC3_DATA2			0x1d0
> +			MX8MM_IOMUXC_NAND_DATA07_USDHC3_DATA3			0x1d0
> +			MX8MM_IOMUXC_NAND_RE_B_USDHC3_DATA4			0x1d0
> +			MX8MM_IOMUXC_NAND_CE2_B_USDHC3_DATA5			0x1d0
> +			MX8MM_IOMUXC_NAND_CE3_B_USDHC3_DATA6			0x1d0
> +			MX8MM_IOMUXC_NAND_CLE_USDHC3_DATA7			0x1d0
> +			MX8MM_IOMUXC_NAND_CE1_B_USDHC3_STROBE			0x190
> +		>;
> +	};
> +
> +	pinctrl_usdhc3_100mhz: usdhc3grp100mhz {

NAK, nothing improved here.

This is a friendly reminder during the review process.

It seems my previous comments were not fully addressed. Maybe my
feedback got lost between the quotes, maybe you just forgot to apply it.
Please go back to the previous discussion and either implement all
requested changes or keep discussing them.

Thank you.
Best regards,
Krzysztof




More information about the linux-arm-kernel mailing list