lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aac5ccf6-1b11-4ae8-857e-b344e0752212@kernel.org>
Date: Wed, 14 Aug 2024 17:08:38 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Tarang Raval <tarang.raval@...iconsignals.io>, shawnguo@...nel.org,
 krzk+dt@...nel.org, festevam@...il.com
Cc: Rob Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Sascha Hauer <s.hauer@...gutronix.de>,
 Pengutronix Kernel Team <kernel@...gutronix.de>, devicetree@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] arm64: dts: imx8mm-emtop-baseboard: Add Peripherals
 Support

On 14/08/2024 16:49, Tarang Raval wrote:
> This adds the following peripherals support for the Emtop i.MX8M Mini Baseboard
> 	* Wi-Fi
> 	* Audio
> 	* SD card
> 	* RTC
> 	* CAN bus
> 	* USB OTG
> 
> Signed-off-by: Tarang Raval <tarang.raval@...iconsignals.io>
> ---
>  .../dts/freescale/imx8mm-emtop-baseboard.dts  | 347 ++++++++++++++++++
>  .../boot/dts/freescale/imx8mm-emtop-som.dtsi  |   1 +
>  2 files changed, 348 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8mm-emtop-baseboard.dts b/arch/arm64/boot/dts/freescale/imx8mm-emtop-baseboard.dts
> index 7d2cb74c64ee..5ce8f21a0b1b 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mm-emtop-baseboard.dts
> +++ b/arch/arm64/boot/dts/freescale/imx8mm-emtop-baseboard.dts
> @@ -11,6 +11,129 @@ / {
>  	model = "Emtop Embedded Solutions i.MX8M Mini Baseboard V1";
>  	compatible = "ees,imx8mm-emtop-baseboard", "ees,imx8mm-emtop-som",
>  		"fsl,imx8mm";
> +
> +	extcon_usb: extcon_usb1otg {

Does not look like this follows DTS coding style.

Also, use generic node name - see other boards.

> +	        compatible = "linux,extcon-usb-gpio";
> +	        pinctrl-names = "default";
> +	        pinctrl-0 = <&pinctrl_extcon_usb>;
> +	        id-gpio = <&gpio1 10 GPIO_ACTIVE_HIGH>;
> +	        enable-gpio = <&gpio1 12 GPIO_ACTIVE_LOW>;
> +	};
> +
> +	modem_reset: modem-reset {
> +	        compatible = "gpio-reset";
> +	        reset-gpios = <&gpio2 6 GPIO_ACTIVE_LOW>;
> +	        reset-delay-us = <2000>;
> +	        reset-post-delay-ms = <40>;
> +	        #reset-cells = <0>;
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpio_led>;
> +
> +		beep {

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

> +		        label = "beep";
> +		        gpios = <&gpio4 29 GPIO_ACTIVE_HIGH>;
> +		        default-state = "off";
> +		};
> +
> +		canbus_reset {

Really, no underscores...

> +			label = "canbus_reset";
> +			gpios = <&gpio1 15 GPIO_ACTIVE_HIGH>;
> +			default-state = "on";
> +		};
> +	};
> +
> +	osc_can: clock-osc-can {
> +		compatible = "fixed-clock";
> +		#clock-cells = <0>;
> +		clock-frequency = <16000000>;
> +		clock-output-names = "osc-can";
> +	};
> +
> +	regulators {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <0>;

This is not a bus. No.

> +
> +		reg_audio: regulator-audio-vdd {
> +		        compatible = "regulator-fixed";
> +		        regulator-name = "wm8904_supply";
> +		        regulator-min-microvolt = <1800000>;
> +		        regulator-max-microvolt = <1800000>;
> +		        regulator-always-on;
> +		};
> +
> +		reg_wifi_vmmc: regulator@1 {

Heh? @1? What sort of bus is it?

> +			compatible = "regulator-fixed";
> +			regulator-name = "vmmc";
> +			regulator-min-microvolt = <3300000>;
> +			regulator-max-microvolt = <3300000>;
> +			gpio = <&gpio2 10 GPIO_ACTIVE_HIGH>;
> +			off-on-delay = <20000>;
> +			startup-delay-us = <100>;
> +		        enable-active-high;
> +		};
> +	};
> +
> +	sound-wm8904 {
> +	        compatible = "simple-audio-card";
> +	        simple-audio-card,bitclock-master = <&dailink_master>;
> +	        simple-audio-card,format = "i2s";
> +	        simple-audio-card,frame-master = <&dailink_master>;
> +	        simple-audio-card,name = "wm8904-audio";
> +	        simple-audio-card,mclk-fs = <256>;
> +	        simple-audio-card,routing =
> +			"Headphone Jack", "HPOUTL",
> +			"Headphone Jack", "HPOUTR",
> +			"IN2L", "Line In Jack",
> +			"IN2R", "Line In Jack",
> +			"Headphone Jack", "MICBIAS",
> +			"IN1L", "Headphone Jack";
> +
> +	        simple-audio-card,widgets =
> +	                "Microphone","Headphone Jack",
> +	                "Headphone", "Headphone Jack",
> +	                "Line", "Line In Jack";
> +
> +	        dailink_master: simple-audio-card,codec {
> +	                sound-dai = <&wm8904>;
> +	        };
> +
> +	        simple-audio-card,cpu {
> +	                sound-dai = <&sai3>;
> +	        };
> +	};
> +
> +	sound-spdif {
> +	        compatible = "fsl,imx-audio-spdif";
> +	        model = "imx-spdif";
> +	        spdif-controller = <&spdif1>;
> +	        spdif-out;
> +	        spdif-in;
> +	};
> +

Drop blank line.

> +};
> +
> +/* CAN BUS */
> +&ecspi2 {
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_ecspi2>;
> +        status = "okay";
> +
> +        canbus: mcp2515@0 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +                compatible = "microchip,mcp2515";
> +                pinctrl-names = "default";
> +                pinctrl-0 = <&pinctrl_canbus>;
> +                reg = <0>;
> +                clocks = <&osc_can>;
> +                interrupt-parent = <&gpio1>;
> +                interrupts = <14 IRQ_TYPE_EDGE_FALLING>;
> +                spi-max-frequency = <10000000>;
> +                status = "okay";

Drop.

> +        };
>  };
>  
>  &fec1 {
> @@ -40,7 +163,130 @@ vddio: vddio-regulator {
>  	};
>  };
>  
> +&i2c3 {
> +	clock-frequency = <100000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c3>;
> +	status = "okay";
> +
> +	rx8025: rtc@32 {
> +		compatible = "rx8025";
> +		reg = <0x32>;

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


> +	};
> +
> +	wm8904: wm8904@1a {
> +		compatible = "wlf,wm8904";
> +		reg = <0x1a>;
> +		#sound-dai-cells = <0>;
> +		clocks = <&clk IMX8MM_CLK_SAI3_ROOT>;
> +		clock-names = "mclk";
> +		DCVDD-supply = <&reg_audio>;
> +		DBVDD-supply = <&reg_audio>;
> +		AVDD-supply = <&reg_audio>;
> +		CPVDD-supply = <&reg_audio>;
> +		MICVDD-supply = <&reg_audio>;
> +		status = "okay";

Where is it disabled?

> +	};
> +};
> +
> +/* AUDIO */
> +&sai3 {
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_sai3>;
> +        assigned-clocks = <&clk IMX8MM_CLK_SAI3>;
> +        assigned-clock-parents = <&clk IMX8MM_AUDIO_PLL1_OUT>;
> +        assigned-clock-rates = <24576000>;
> +        status = "okay";
> +};
> +
> +&spdif1 {
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&pinctrl_spdif1>;
> +        assigned-clocks = <&clk IMX8MM_CLK_SPDIF1>;
> +        assigned-clock-parents = <&clk IMX8MM_AUDIO_PLL1_OUT>;
> +        assigned-clock-rates = <24576000>;
> +        clocks = <&clk IMX8MM_CLK_AUDIO_AHB>, <&clk IMX8MM_CLK_24M>,
> +                <&clk IMX8MM_CLK_SPDIF1>, <&clk IMX8MM_CLK_DUMMY>,
> +                <&clk IMX8MM_CLK_DUMMY>, <&clk IMX8MM_CLK_DUMMY>,
> +                <&clk IMX8MM_CLK_AUDIO_AHB>, <&clk IMX8MM_CLK_DUMMY>,
> +                <&clk IMX8MM_CLK_DUMMY>, <&clk IMX8MM_CLK_DUMMY>,
> +                <&clk IMX8MM_AUDIO_PLL1_OUT>, <&clk IMX8MM_AUDIO_PLL2_OUT>;
> +        clock-names = "core", "rxtx0", "rxtx1", "rxtx2", "rxtx3",
> +                "rxtx4", "rxtx5", "rxtx6", "rxtx7", "spba", "pll8k", "pll11k";
> +        status = "okay";
> +};
> +
> +/* Wifi */
> +&usdhc1 {
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz";
> +	pinctrl-0 = <&pinctrl_usdhc1>, <&pinctrl_usdhc1_gpio>;
> +	pinctrl-1 = <&pinctrl_usdhc1_100mhz>, <&pinctrl_usdhc1_gpio>;
> +	pinctrl-2 = <&pinctrl_usdhc1_200mhz>, <&pinctrl_usdhc1_gpio>;
> +	bus-width = <4>;
> +	vmmc-supply = <&reg_wifi_vmmc>;
> +	pm-ignore-notify;
> +	cap-power-off-card;
> +	keep-power-in-suspend;
> +	non-removable;
> +	status = "okay";
> +
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	brcmf: brcmf@1 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

> +		compatible = "brcm,bcm4329-fmac";
> +		reg = <1>;
> +		interrupt-parent = <&gpio2>;
> +		interrupts = <9 IRQ_TYPE_LEVEL_LOW>;
> +		interrupt-names = "host-wake";
> +	};
> +};
> +
> +/* SD-card */
> +&usdhc2 {
> +        pinctrl-names = "default";      /* "state_100mhz", "state_200mhz"; */
> +        pinctrl-0 = <&pinctrl_usdhc2>, <&pinctrl_usdhc2_gpio>;
> +        pinctrl-1 = <&pinctrl_usdhc2_100mhz>, <&pinctrl_usdhc2_gpio>;
> +        pinctrl-2 = <&pinctrl_usdhc2_200mhz>, <&pinctrl_usdhc2_gpio>;
> +        cd-gpios = <&gpio2 12 GPIO_ACTIVE_LOW>;
> +        bus-width = <4>;
> +        status = "okay";
> +};
> +
> +/* USBOTG */
> +&usbotg1 {

Ordering looks odd. 'b' is before 'd'.


> +	pinctrl_usdhc1_100mhz: usdhc1grp100mhz {

Do not upstream your code from vendor kernel directly. You *MUST* clean
it up from all downstream junk.


It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).



Best regards,
Krzysztof


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ