Hi, thanks for the input! I've addressed these things in a v4 that I just sent out: * Added chassis-type * Removed unused nodes * Sorted alphabetically The brightness setting is in-line with what MNT are shipping currently for their other modules. I can send a follow-up patch to update the settings for both i.MX8MQ and A311D. Cheers, Patrick On Wed, Feb 12, 2025 at 10:28:59PM -0600, Jimmy Hon wrote: > Hi, > > A few questions below. > > [ snip ] > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-firefly-icore-3588q.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-firefly-icore-3588q.dtsi > > new file mode 100644 > > index 000000000000..898a7b29692f > > --- /dev/null > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-firefly-icore-3588q.dtsi > > That's interesting, the Firefly Core 3588Q SoM's dtsi is more than 95% > the same as the Core 3588J SoM's dtsi. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588-firefly-core-3588j.dtsi?h=v6.14-rc1 > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-mnt-reform2.dts b/arch/arm64/boot/dts/rockchip/rk3588-mnt-reform2.dts > > new file mode 100644 > > index 000000000000..cd7a82a3b0de > > --- /dev/null > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-mnt-reform2.dts > > @@ -0,0 +1,348 @@ > > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT) > > +/* > > + * Copyright (c) 2021 Rockchip Electronics Co., Ltd. > > + * Copyright (c) 2024 MNT Research GmbH > > + * > > + */ > > + > > +/dts-v1/; > > + > > +#include <dt-bindings/gpio/gpio.h> > > +#include <dt-bindings/input/input.h> > > +#include <dt-bindings/pinctrl/rockchip.h> > > +#include <dt-bindings/soc/rockchip,vop2.h> > > +#include <dt-bindings/usb/pd.h> > > + > > +#include "rk3588-firefly-icore-3588q.dtsi" > > + > > +/ { > > + model = "MNT Reform 2 with RCORE RK3588 Module"; > > + compatible = "mntre,reform2-rcore", "firefly,icore-3588q", "rockchip,rk3588"; > Will you be adding "chassis-type"? > > > + > > + aliases { > > + ethernet0 = &gmac0; > > + mmc1 = &sdmmc; > > + }; > > + > > + chosen { > > + stdout-path = "serial2:1500000n8"; > > + }; > > + > > + backlight: backlight { > > + compatible = "pwm-backlight"; > > + pwms = <&pwm8 0 10000 0>; > > + enable-gpios = <&gpio2 RK_PB5 GPIO_ACTIVE_HIGH>; > > + brightness-levels = <0 8 16 32 64 128 160 200 255>; > > + default-brightness-level = <128>; > Interestingly, the brightness curve is more fine-grained than one used > for the IMX8MQ SoM and the A311D SoM. Should those be updated > eventually? > > brightness-levels = <0 32 64 128 160 200 255>; > default-brightness-level = <6>; > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mq-mnt-reform2.dts?h=v6.14-rc1#n24 > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/amlogic/meson-g12b-bananapi-cm4-mnt-reform2.dts?h=v6.14-rc1#n198 > > [ snip ] > > + > > +&combphy1_ps { > > + status = "okay"; > > +}; > Should this be left out? It doesn't look like pcie2x1l0 or sata1 is being used. > > > + > > +&combphy2_psu { > > + status = "okay"; > > +}; > Should this be left out? It doesn't look like pcie2x1l1, sata2, or > usb_host2_xhci is being used. > > > + > > +&saradc { > > + vref-supply = <&avcc_1v8_s0>; > > + status = "okay"; > > +}; > > + > > +&tsadc { > > + status = "okay"; > > +}; > These nodes should be sorted alphabetically > > > + > > +&sdmmc { > > + bus-width = <4>; > > + max-frequency = <40000000>; > > + no-sdio; > > + no-mmc; > > + no-1-8-v; > > + cap-sd-highspeed; > > + vqmmc-supply = <&vcc3v3_pcie30>; > > + vmmc-supply = <&vcc3v3_pcie30>; > > + cd-gpios = <&gpio0 RK_PA4 GPIO_ACTIVE_LOW>; > > + disable-wp; > > + status = "okay"; > > +}; > > + > > +&mdio0 { > > + rgmii_phy: ethernet-phy@0 { > > + compatible = "ethernet-phy-ieee802.3-c22"; > > + reg = <0x0>; > > + }; > > +}; > > Jimmy
