Re: [PATCH v2 3/3] arm64: dts: freescale: introduce rve-gateway board

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 01/11/2023 19:35, Hugo Villeneuve wrote:
> From: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> 
> The RVE gateway board is based on a Variscite VAR-SOM-NANO,
> with a NXP MX8MN nano CPU.
> 
> Signed-off-by: Hugo Villeneuve <hvilleneuve@dimonoff.com>
> ---
>  MAINTAINERS                                   |   7 +
>  arch/arm64/boot/dts/freescale/Makefile        |   1 +
>  .../boot/dts/freescale/imx8mn-rve-gateway.dts | 284 ++++++++++++++++++
>  3 files changed, 292 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/freescale/imx8mn-rve-gateway.dts
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 43c5ed3c4508..9423b1bf35fc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18778,6 +18778,13 @@ F:	samples/rust/
>  F:	scripts/*rust*
>  K:	\b(?i:rust)\b
>  
> +RVE BOARD SUPPORT
> +M:	Hugo Villeneuve <hvilleneuve@dimonoff.com>
> +L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
> +S:	Supported
> +W:	https://rve.ca
> +F:	arch/arm64/boot/dts/freescale/imx*rve-*.dts*

I still don't think individual boards should receive maintainer entry.
https://lore.kernel.org/all/20210809080204.8381-1-shawnguo@kernel.org/

> +
>  RXRPC SOCKETS (AF_RXRPC)
>  M:	David Howells <dhowells@redhat.com>
>  M:	Marc Dionne <marc.dionne@auristor.com>
> diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
> index 89aee6c92576..760a0ca7adc3 100644
> --- a/arch/arm64/boot/dts/freescale/Makefile
> +++ b/arch/arm64/boot/dts/freescale/Makefile
> @@ -89,6 +89,7 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mn-bsh-smm-s2pro.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mn-evk.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mn-ddr3l-evk.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mn-ddr4-evk.dtb
> +dtb-$(CONFIG_ARCH_MXC) += imx8mn-rve-gateway.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mn-tqma8mqnl-mba8mx.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mn-var-som-symphony.dtb
>  dtb-$(CONFIG_ARCH_MXC) += imx8mn-venice-gw7902.dtb
> diff --git a/arch/arm64/boot/dts/freescale/imx8mn-rve-gateway.dts b/arch/arm64/boot/dts/freescale/imx8mn-rve-gateway.dts
> new file mode 100644
> index 000000000000..312718982a52
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8mn-rve-gateway.dts
> @@ -0,0 +1,284 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Copyright 2023 DimOnOff
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/usb/pd.h>
> +#include "imx8mn-var-som.dtsi"
> +
> +/ {
> +	model = "RVE gateway";
> +	compatible = "rve,rve-gateway", "fsl,imx8mn";

Missing compatible of SoM.

> +
> +	crystal_duart_24m: crystal-duart-24m {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <24000000>;
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpio_keys>;
> +		status = "okay";

Drop

> +
> +		key-enter {
> +			label = "enter";
> +			gpios = <&gpio1 1 GPIO_ACTIVE_LOW>;
> +			linux,code = <KEY_ENTER>;
> +		};
> +
> +		key-exit {
> +			label = "exit";
> +			gpios = <&gpio3 23 GPIO_ACTIVE_LOW>;
> +			linux,code = <KEY_ESC>;
> +		};
> +	};
> +
> +	lcd {
> +		compatible = "hit,hd44780";
> +		display-height-chars = <2>;
> +		display-width-chars = <20>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_lcd>;
> +		data-gpios =
> +			<&gpio5  1 GPIO_ACTIVE_HIGH>,

This should be after =, with the reset of lines properly aligned.

> +			<&gpio1  6 GPIO_ACTIVE_HIGH>,
> +			<&gpio1 14 GPIO_ACTIVE_HIGH>,
> +			<&gpio4 28 GPIO_ACTIVE_HIGH>,
> +			<&gpio5 24 GPIO_ACTIVE_HIGH>,
> +			<&gpio5  2 GPIO_ACTIVE_HIGH>,
> +			<&gpio1 12 GPIO_ACTIVE_HIGH>,
> +			<&gpio5 25 GPIO_ACTIVE_HIGH>;
> +		enable-gpios = <&gpio5 23 GPIO_ACTIVE_HIGH>;
> +		rs-gpios = <&gpio1 7 GPIO_ACTIVE_HIGH>;
> +		rw-gpios = <&gpio4 27 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	reg_3p3v: regulator-3p3v {
> +		compatible = "regulator-fixed";
> +		regulator-name = "3P3V";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-always-on;
> +		};

Fix indentation.

> +
> +	rotary: rotary-encoder { /* Bourns PEC12R rotary encoder, 24 steps. */

No comments after {. Put them in line before or after. This applies to
other places as well.

> +		compatible = "rotary-encoder";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_rotary>;
> +		gpios =
> +			<&gpio1  5 GPIO_ACTIVE_LOW>, /* A */

This should be after =, with the reset of lines properly aligned.


> +			<&gpio3 21 GPIO_ACTIVE_LOW>; /* B */
> +		linux,axis = <0>; /* REL_X */
> +		rotary-encoder,encoding = "gray";
> +		rotary-encoder,relative-axis;
> +	};
> +};
> +
> +&ecspi1 {
> +	cs-gpios = <&gpio5 9 GPIO_ACTIVE_LOW>;
> +
> +	duart1: serial@0 {
> +		compatible = "nxp,sc16is752";
> +		reg = <0>;
> +		spi-rx-bus-width = <1>;
> +		spi-tx-bus-width = <1>;
> +		spi-max-frequency = <4000000>;
> +		clocks = <&crystal_duart_24m>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <22 IRQ_TYPE_EDGE_FALLING>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-line-names = "RADIO0", "RADIO1", "RADIO2", "RADIO3",
> +		"RADIO4", "RADIO_RESET", "TP12", "TP11";
> +		linux,rs485-enabled-at-boot-time;
> +		rs485-rts-active-low;
> +	};
> +
> +	/delete-node/ touchscreen@0;
> +};
> +
> +&ecspi2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_ecspi2>;
> +	cs-gpios = <&gpio5 13 GPIO_ACTIVE_LOW>;
> +	/delete-property/ dmas;
> +	/delete-property/ dma-names;
> +	status = "okay";
> +
> +	duart2: serial@0 {
> +		compatible = "nxp,sc16is752";
> +		reg = <0>;
> +		spi-rx-bus-width = <1>;
> +		spi-tx-bus-width = <1>;
> +		spi-max-frequency = <4000000>;
> +		clocks = <&crystal_duart_24m>;
> +		interrupt-parent = <&gpio3>;
> +		interrupts = <20 IRQ_TYPE_EDGE_FALLING>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-line-names = "LED_B_USER", "LED_R_USER", "LED_G_USER",
> +		"GPIO_EXT3", "GPIO_EXT2", "GPIO_EXT1", "GPIO_EXT0", "TP13";
> +		linux,rs485-enabled-at-boot-time;
> +		rs485-rts-active-low;
> +	};
> +};
> +
> +/* Configure PWM pins in GPIO mode: */
> +&gpio5 {
> +	gpio-line-names = "", "", "", "PWM3", "PWM2", "PWM1";
> +};
> +
> +&gpu {
> +	status = "disabled";
> +};
> +
> +&i2c2 {
> +	clock-frequency = <400000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c2>;
> +	status = "okay";
> +
> +	eeprom_cb: eeprom@56 { /* Carrier board EEPROM */
> +		compatible = "atmel,24c04";
> +		reg = <0x56>;
> +		pagesize = <16>;
> +		vcc-supply = <&reg_3p3v>;
> +	};
> +
> +	lm75: sensor@48 {
> +		compatible = "st,stlm75";
> +		reg = <0x48>;
> +		vs-supply = <&reg_3p3v>;
> +	};
> +
> +	mcp7940: rtc@6f {
> +		compatible = "microchip,mcp7940x";
> +		reg = <0x6f>;
> +	};
> +};
> +
> +&i2c3 {
> +	codec@1a {
> +		status = "disabled";
> +	};
> +};
> +
> +&i2c4 {
> +	clock-frequency = <400000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c4>;
> +	status = "okay";
> +
> +	pcf8574_1: gpio@38 {
> +		status = "okay";

Drop

> +		compatible = "nxp,pcf8574";
> +		reg = <0x38>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-line-names = "LED_B_COMM3", "LED_R_COMM3", "LED_G_COMM3",
> +		"TP14", "TP15", "LED_G_COMM4", "LED_R_COMM4", "LED_B_COMM4";
> +	};
> +
> +	pcf8574_2: gpio@39 {
> +		status = "okay";

Drop


> +		compatible = "nxp,pcf8574";
> +		reg = <0x39>;
> +		gpio-controller;
> +		#gpio-cells = <2>;
> +		gpio-line-names = "LED_B_COMM2", "LED_G_COMM2", "LED_B_COMM1",
> +		"LED_R_COMM2", "LED_R_COMM1", "LED_G_COMM1", "TP16", "TP17";
> +	};
> +};
> +
> +&uart2 {
> +	/delete-node/ bluetooth;

Why? If the hardware does not exist, why it is even in the DTSI? It does
exist, so removing is not correct.


> +};
> +
> +&usbotg1 {
> +	dr_mode = "host";
> +	disable-over-current;
> +	status = "okay";
> +};
> +
> +/* SD interface on expansion connector. */
> +&usdhc2 {
> +	/delete-property/vmmc-supply;

Why? The supply is needed.


Best regards,
Krzysztof