Re: [PATCH v2 1/2] arm64: dts: rockchip: add DTs for Firefly ITX-3588J

["Heiko Stübner" <heiko@sntech.de>]

Am Freitag, 13. Dezember 2024, 19:08:54 CET schrieb Shimrra Shai:
> Main DTS for the board and Makefile addition.
> 
> Signed-off-by: Shimrra Shai <shimrrashai@gmail.com>

> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-firefly-itx-3588j.dts b/arch/arm64/boot/dts/rockchip/rk3588-firefly-itx-3588j.dts
> new file mode 100644
> index 000000000..a99c007c7
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-firefly-itx-3588j.dts
> @@ -0,0 +1,1133 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/leds/common.h>
> +#include <dt-bindings/pinctrl/rockchip.h>
> +#include <dt-bindings/pwm/pwm.h>
> +#include <dt-bindings/soc/rockchip,vop2.h>
> +#include "dt-bindings/usb/pd.h"
> +#include "rockchip-pca9555.h"
> +#include "rk3588.dtsi"

in line with my comment in the binding, please split the system-on-module
parts into a rk3588-firefly-core-3588j.dtsi and then include that file here.


> +
> +/ {
> +	model = "Firefly ITX-3588J";
> +	compatible = "firefly,itx-3588j", "rockchip,rk3588";
> +
> +	aliases {
> +		ethernet0 = &gmac0;
> +		ethernet1 = &gmac1;
> +		mmc0 = &sdhci;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial2:1500000n8";
> +	};
> +
> +	/* NB: There are also a "Reset" and "Mask ROM" button but I don't
> +	 * know the right settings for these. - Shimrra Shai
> +	 */

same comment-style as below, plus

/*
 * There are additional Reset and Maskrom keys connected, but their
 * settings are still unknown right now.
 */

> +	adc-keys-1 {

why is it adc-keys-1 ? (where is -0 ?)

> +		compatible = "adc-keys";
> +		io-channels = <&saradc 1>;
> +		io-channel-names = "buttons";
> +		keyup-threshold-microvolt = <1800000>;
> +		poll-interval = <100>;
> +
> +		button-recovery {
> +			label = "Recovery";
> +			linux,code = <KEY_VENDOR>;
> +			press-threshold-microvolt = <2000>;
> +		};
> +	};
> +
> +	analog-sound {
> +		compatible = "simple-audio-card";
> +		pinctrl-0 = <&hp_detect>;
> +		pinctrl-names = "default";
> +		simple-audio-card,aux-devs = <&amp_headphones>, <&amp_speaker>;
> +		simple-audio-card,format = "i2s";
> +		simple-audio-card,hp-det-gpios = <&gpio1 RK_PC4 GPIO_ACTIVE_LOW>;
> +		simple-audio-card,mclk-fs = <384>;
> +		simple-audio-card,name = "rockchip_es8323";
> +		simple-audio-card,pin-switches = "Headphones", "Speaker";
> +		simple-audio-card,routing =
> +			"Speaker Amplifier INL", "LOUT2",
> +			"Speaker Amplifier INR", "ROUT2",
> +			"Speaker", "Speaker Amplifier OUTL",
> +			"Speaker", "Speaker Amplifier OUTR",
> +			"Headphones Amplifier INL", "LOUT1",
> +			"Headphones Amplifier INR", "ROUT1",
> +			"Headphones", "Headphones Amplifier OUTL",
> +			"Headphones", "Headphones Amplifier OUTR",
> +			"LINPUT1", "Microphone Jack",
> +			"RINPUT1", "Microphone Jack",
> +			"LINPUT2", "Onboard Microphone",
> +			"RINPUT2", "Onboard Microphone";
> +		simple-audio-card,widgets =
> +			"Microphone", "Microphone Jack",
> +			"Microphone", "Onboard Microphone",
> +			"Headphone", "Headphones",
> +			"Speaker", "Speaker";
> +
> +		simple-audio-card,cpu {
> +			sound-dai = <&i2s0_8ch>;
> +		};
> +
> +		simple-audio-card,codec {
> +			sound-dai = <&es8323>;
> +			system-clock-frequency = <12288000>;
> +		};
> +	};
> +
> +	/* note: this does not seem to be a proper "amplifier" but is just

	/*
	 * note: this does not seem to be a proper "amplifier" but is just

comment formatting please

> +	 * a way to control the GPIO pins to switch on or off the given
> +	 * sound output device
> +	 */
> +	amp_headphones: headphones-audio-amplifier {
> +		compatible = "simple-audio-amplifier";
> +		enable-gpios = <&gpio4 RK_PB0 GPIO_ACTIVE_HIGH>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&headphone_amplifier_en>;
> +		sound-name-prefix = "Headphones Amplifier";
> +	};

> +	leds {
> +		compatible = "gpio-leds";
> +
> +		/* NB: This Power LED control does not seem to work for
> +		 * some reason. - Shimrra Shai
> +		 */
> +#if 0
> +		power_led: led-0 {
> +			gpios = <&gpio1 RK_PB3 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "default-on";
> +		};
> +#endif

please don't add dead code

> +
> +		user_led: led-1 {
> +			gpios = <&pca9555 PCA_IO0_3 GPIO_ACTIVE_HIGH>;
> +			linux,default-trigger = "disk-activity";
> +		};
> +	};
> +
> +	vcc_sata_pwr_en: vcc-sata-pwr-en-regulator {

vcc_sata_pwr_en: regulator-vcc-sata-pwr-en

Applied to all regulator nodes

> +		compatible = "regulator-fixed";
> +		regulator-name = "vcc_sata_pwr_en";
> +		regulator-boot-on;
> +		regulator-always-on;
> +		enable-active-high;
> +		gpio = <&pca9555 PCA_IO1_2 GPIO_ACTIVE_HIGH>;  //PCA_IO 12

please sort properties (compatible, regs, [alphabetically], status) and please
drop those comments at the end.

Applied to all regulator nodes

> +	};

[...]

> +&cpu_l0 {
> +	cpu-supply = <&vdd_cpu_lit_s0>;
> +	mem-supply = <&vdd_cpu_lit_mem_s0>;

that mem-supply property is not specified and also is not necessary here.
Same for the other cases above.

> +};

[...]

> +	usbc0: usb-typec@22 {
> +		compatible = "fcs,fusb302";
> +		reg = <0x22>;
> +		interrupt-parent = <&gpio0>;
> +		interrupts = <RK_PD3 IRQ_TYPE_LEVEL_LOW>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&usbc0_int>;
> +		vbus-supply = <&vbus5v0_typec_pwr_en>;
> +		status = "okay";

default status is always "okay", so no need to add it for new nodes.
Same for possible other places in this file.


> diff --git a/arch/arm64/boot/dts/rockchip/rockchip-pca9555.h b/arch/arm64/boot/dts/rockchip/rockchip-pca9555.h
> new file mode 100644
> index 000000000..c4c9a2471
> --- /dev/null
> +++ b/arch/arm64/boot/dts/rockchip/rockchip-pca9555.h

if anything, that include needs to live in include/dt-bindings/something
and needs to be a separate patch, if you really need that

> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR MIT */
> +/*
> + * Bindings for the PCA9555 GPIO extender used on some Rockchip devices, e.g.
> + * Firefly.
> + *
> + * Copyright (c) 2013 MundoReader S.L.
> + * Authors: Heiko Stuebner <heiko@sntech.de>

where does this copyright come from? I don't remember being involved
in a binding header for that expander?


> + *          Shimrra Shai <shimrrashai@gmail.com>
> + */
> +
> +#ifndef __RK_PCA9555_H__
> +#define __RK_PCA9555_H__
> +
> +#define PCA_IO0_0          0
> +#define PCA_IO0_1          1
> +#define PCA_IO0_2          2
> +#define PCA_IO0_3          3
> +#define PCA_IO0_4          4
> +#define PCA_IO0_5          5
> +#define PCA_IO0_6          6
> +#define PCA_IO0_7          7
> +#define PCA_IO1_0          8
> +#define PCA_IO1_1          9
> +#define PCA_IO1_2          10
> +#define PCA_IO1_3          11
> +#define PCA_IO1_4          12
> +#define PCA_IO1_5          13
> +#define PCA_IO1_6          14
> +#define PCA_IO1_7          15
> +
> +#endif
> 

Heiko