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

From: Krzysztof Kozlowski
Date: Mon May 01 2023 - 13:31:21 EST

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

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@xxxxxxxxxxxxxxxxx>
>
> 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@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