| 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
| ||
|
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 = <®_audio>;
> + DBVDD-supply = <®_audio>;
> + AVDD-supply = <®_audio>;
> + CPVDD-supply = <®_audio>;
> + MICVDD-supply = <®_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 = <®_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