Re: [PATCH 1/3] arm64: dts: iesy: add support for iesy PX30 SoM OSM-S

[Krzysztof Kozlowski]

On 19/03/2024 10:54, Dominik Poggel wrote:
> This adds support for the iesy SoM px30-iesy-osm-sf and the matching
> evalboard px30-iesy-eva-mi V2.XX.
> 
> Signed-off-by: Dominik Poggel <pog@xxxxxxxx>
> ---
>  arch/arm64/boot/dts/rockchip/Makefile         |   1 +
>  .../boot/dts/rockchip/px30-iesy-eva-mi-v2.dts | 624 ++++++++++++++++++
>  .../boot/dts/rockchip/px30-iesy-osm-sf.dtsi   | 346 ++++++++++
>  3 files changed, 971 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/rockchip/px30-iesy-eva-mi-v2.dts
>  create mode 100644 arch/arm64/boot/dts/rockchip/px30-iesy-osm-sf.dtsi
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
> index f906a868b71a..a46234ccbe15 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -3,6 +3,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-evb.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-ctouch2.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-ctouch2-of10.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-engicam-px30-core-edimm2.2.dtb
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-iesy-eva-mi-v2.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += px30-ringneck-haikou.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-evb.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3308-roc-cc.dtb
> diff --git a/arch/arm64/boot/dts/rockchip/px30-iesy-eva-mi-v2.dts b/arch/arm64/boot/dts/rockchip/px30-iesy-eva-mi-v2.dts
> new file mode 100644
> index 000000000000..be1d709bbab0
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/px30-iesy-eva-mi-v2.dts
> @@ -0,0 +1,624 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Device Tree for iesy RPX30 EVA-MI V2.xx (Eval Kit)
> + *
> + * Copyright (c) 2022 iesy GmbH
> + */
> +
> +/dts-v1/;
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/net/mscc-phy-vsc8531.h>
> +#include "px30-iesy-osm-sf.dtsi"
> +
> +/ {
> +	model = "iesy RPX30 EVA-MI V2.xx (Eval Kit)";
> +	compatible = "iesy,rpx30-eva-mi-v2", "rockchip,px30";
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_leds_bb138>;
> +
> +		/* BB138a: green user led (LD4) */
> +		led@0 {

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 = "USER_LED_00";
> +			gpios = <&gpio3 RK_PA0 GPIO_ACTIVE_LOW>;
> +			linux,default-trigger = "heartbeat";
> +		};
> +
> +		/* BB138a: yellow user led (LD9)) */
> +		led@1 {
> +			label = "USER_LED_01";
> +			gpios = <&gpio3 RK_PA1 GPIO_ACTIVE_LOW>;
> +			default-state = "off";
> +		};
> +	};
> +
> +	/* BB138a: MAX9867ETJ+ audio codec */
> +	max9867-sound {

sound {

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 = "simple-audio-card";
> +		simple-audio-card,name = "rockchip,max9867-codec";
> +		simple-audio-card,format = "i2s";
> +
> +		simple-audio-card,widgets =
> +			"Speaker", "Jack",
> +			"Microphone", "Mic";
> +		simple-audio-card,routing =
> +			"Jack", "LOUT",
> +			"Jack", "ROUT",
> +			"Mic", "DMICL",
> +			"Mic", "DMICR";
> +
> +		simple-audio-card,frame-master = <&cpudai>;
> +		simple-audio-card,bitclock-master = <&cpudai>;
> +
> +		status = "okay";
> +
> +		cpudai: simple-audio-card,cpu {
> +			sound-dai = <&i2s1_2ch>;
> +			dai-tdm-slot-num = <1>;
> +			dai-tdm-slot-width = <16>;
> +		};
> +
> +		simple-audio-card,codec {
> +			sound-dai = <&max9867>;
> +			clocks = <&cru SCLK_I2S1_OUT>;
> +		};
> +	};
> +
> +	/* regulator for USB OTG port */
> +	usb_a_vbus_regulator: regulator@1 {

Not a bus.

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).

> +		compatible = "regulator-fixed";
> +		regulator-name = "usb_a_vbus_regulator";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		gpio = <&gpio3 RK_PC0 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +	};
> +
> +	/* regulator for USB host port */
> +	usb_b_vbus_regulator: regulator@2 {

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).

> +		compatible = "regulator-fixed";
> +		regulator-name = "usb_b_vbus_regulator";
> +		regulator-min-microvolt = <5000000>;
> +		regulator-max-microvolt = <5000000>;
> +		gpio = <&gpio3 RK_PC3 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +		regulator-always-on;
> +		regulator-boot-on;
> +	};
> +
> +	user-buttons {
> +		compatible = "gpio-keys";
> +		#address-cells = <1>;
> +		#size-cells = <0>;

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).

You are repeating the same mistakes we fixed long time ago. This means
you started work from some downstream or old DTS. Don't. It's a waste of
time, also mine to tell you that we fixed all this.

Please start from scratch from upstream DTS and test your code before
sending.

..

..

> +/ {
> +	aliases {
> +		mmc0 = &emmc;
> +		mmc1 = &sdmmc;
> +		mmc2 = &sdio;
> +	};
> +
> +	chosen {
> +		bootargs = "earlycon=uart8250,mmio32,0xff160000 console=ttyFIQ0 rw root=PARTUUID=614e0000-0000 rootwait";

That's not correct bootargs. earlyocon is debugging, so drop from
mainline code. console goes to stdout property.
root/PARTUIID is obviously not correct. My PARTUIID is different.

Drop entire chosen.

> +	};
> +
> +	fiq-debugger {
> +		compatible = "rockchip,fiq-debugger";

Undocumented. Please run scripts/checkpatch.pl and fix reported
warnings. Some warnings can be ignored, but the code here looks like it
needs a fix. Feel free to get in touch if the warning is not clear.

You MUST run checkpatch on your submissions. All of them.

> +		rockchip,serial-id = <2>;
> +		rockchip,wake-irq = <0>;
> +		/* If enable uart uses irq instead of fiq */
> +		rockchip,irq-mode-enable = <1>;
> +		rockchip,baudrate = <115200>;  /* Only 115200 and 1500000 */
> +		interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_LOW>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&uart2m1_xfer>;
> +		status = "okay";

Why? Was it disabled anywhere? Anyway, that's not even hardware.

I am not going to review the rest. Please read submitting patches and
carefully follow it. Run all standard tests on your code before sending
patches. There is no point in reviewer telling you something which
automated tools tell.

Best regards,
Krzysztof