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

[Dominik Poggel <pog@iesy.com>]

On 19.03.24 13:40, Krzysztof Kozlowski wrote:
> Achtung! Diese Mail kommt von extern. Bitte besonders vorsichtig mit den Inhalten und den Anhängen umgehen.
>
> 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@iesy.com>
>> ---
>>   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.

Thanks for your input, I will use it and resubmit it afterwards.

Unfortunately 'make dtbs_check' isn't working, as it can't resolve
http://devicetree.org/meta-schemas/core.yaml. Probably just a problem on 
my end,
I will investigate further.

Best regards,
Dominik

>
> Best regards,
> Krzysztof
>