[PATCH 3/3] arm64: dts: rockchip: add Anbernic RG353 and RG503

[Chris Morgan]

On Thu, Aug 18, 2022 at 11:14:17AM +0300, Krzysztof Kozlowski wrote:
> On 17/08/2022 23:49, Chris Morgan wrote:
> > From: Chris Morgan <macromorgan at hotmail.com>
> > 
> > Anbernic RG353 and RG503 are both RK3566 based handheld gaming devices
> > from Anbernic.
> > 
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +		red_led: led-2 {
> > +			color = <LED_COLOR_ID_RED>;
> > +			default-state = "off";
> > +			function = LED_FUNCTION_STATUS;
> > +			gpios = <&gpio0 RK_PC7 GPIO_ACTIVE_HIGH>;
> > +		};
> > +	};
> > +
> > +	rk817-sound {
> 
> just sound
> 
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 

Acknowledged. I just cut and pasted from a different tree, but I'll make this change.

> > +		compatible = "simple-audio-card";
> > +		simple-audio-card,name = "anbernic_rk817";
> > +		simple-audio-card,aux-devs = <&spk_amp>;
> > +		simple-audio-card,format = "i2s";
> > +		simple-audio-card,hp-det-gpio = <&gpio4 RK_PC6 GPIO_ACTIVE_HIGH>;
> > +		simple-audio-card,mclk-fs = <256>;
> > +		simple-audio-card,widgets =
> > +			"Microphone", "Mic Jack",
> > +			"Headphone", "Headphones",
> > +			"Speaker", "Internal Speakers";
> > +		simple-audio-card,routing =
> > +			"MICL", "Mic Jack",
> > +			"Headphones", "HPOL",
> > +			"Headphones", "HPOR",
> > +			"Internal Speakers", "Speaker Amp OUTL",
> > +			"Internal Speakers", "Speaker Amp OUTR",
> > +			"Speaker Amp INL", "HPOL",
> > +			"Speaker Amp INR", "HPOR";
> > +		simple-audio-card,pin-switches = "Internal Speakers";
> > +
> > +		simple-audio-card,codec {
> > +			sound-dai = <&rk817>;
> > +		};
> > +
> > +		simple-audio-card,cpu {
> > +			sound-dai = <&i2s1_8ch>;
> > +		};
> > +	};
> > +
> > +	sdio_pwrseq: sdio-pwrseq {
> > +		compatible = "mmc-pwrseq-simple";
> > +		clocks = <&rk817 1>;
> > +		clock-names = "ext_clock";
> > +		pinctrl-0 = <&wifi_enable_h>;
> > +		pinctrl-names = "default";
> > +		post-power-on-delay-ms = <200>;
> > +		reset-gpios = <&gpio4 RK_PA2 GPIO_ACTIVE_LOW>;
> > +	};
> > +
> > +	spk_amp: audio-amplifier {
> > +		compatible = "simple-audio-amplifier";
> > +		enable-gpios = <&gpio4 RK_PC2 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-0 = <&spk_amp_enable_h>;
> > +		pinctrl-names = "default";
> > +		sound-name-prefix = "Speaker Amp";
> > +	};
> > +
> > +	vcc3v3_lcd0_n: vcc3v3-lcd0-n {
> 
> Node name:
> regulator-vcc3v3-lcd0-n
> vcc3v3-lcd0-n-regulator
> or just regulator-0

Does this restriction only apply to node names for regulators, or all
node names? The docs I looked at suggested that it was okay to use an
underscore, but I'll defer to you.

> 
> > +		compatible = "regulator-fixed";
> > +		gpio = <&gpio0 RK_PC2 GPIO_ACTIVE_HIGH>;
> > +		enable-active-high;
> > +		pinctrl-0 = <&vcc_lcd_h>;
> > +		pinctrl-names = "default";
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		regulator-name = "vcc3v3_lcd0_n";
> > +		vin-supply = <&vcc_3v3>;
> > +		regulator-state-mem {
> > +			regulator-off-in-suspend;
> > +		};
> > +	};
> > +
> > +	vcc_sys: vcc_sys {
> 
> No underscores in node names. Same comment as above.
> 

Ack.

> > +		compatible = "regulator-fixed";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <3800000>;
> > +		regulator-max-microvolt = <3800000>;
> > +		regulator-name = "vcc_sys";
> > +	};
> > +
> > +	vcc_wifi: vcc-wifi {
> 
> Same comment as above
> 

Ack.

> > +		compatible = "regulator-fixed";
> > +		enable-active-high;
> > +		gpio = <&gpio0 RK_PA0 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-0 = <&vcc_wifi_h>;
> > +		pinctrl-names = "default";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		regulator-name = "vcc_wifi";
> > +	};
> > +
> > +	vibrator: pwm-vibrator {
> > +		compatible = "pwm-vibrator";
> > +		pwm-names = "enable";
> > +		pwms = <&pwm5 0 1000000000 0>;
> > +	};
> > +};
> > +
> > +&combphy1 {
> > +	status = "okay";
> > +};
> > +
> > +&cpu0 {
> > +	cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&cpu1 {
> > +	cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&cpu2 {
> > +	cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&cpu3 {
> > +	cpu-supply = <&vdd_cpu>;
> > +};
> > +
> > +&gpu {
> > +	mali-supply = <&vdd_gpu>;
> > +	status = "okay";
> > +};
> > +
> > +&hdmi {
> > +	status = "okay";
> > +};
> > +
> > +&hdmi_in {
> > +	hdmi_in_vp0: endpoint {
> > +		remote-endpoint = <&vp0_out_hdmi>;
> > +	};
> > +};
> > +
> > +&hdmi_out {
> > +	hdmi_out_con: endpoint {
> > +		remote-endpoint = <&hdmi_con_in>;
> > +	};
> > +};
> > +
> > +&hdmi_sound {
> > +	status = "okay";
> > +};
> > +
> > +&i2c0 {
> > +	status = "okay";
> > +
> > +	rk817: pmic at 20 {
> > +		compatible = "rockchip,rk817";
> > +		reg = <0x20>;
> > +		interrupt-parent = <&gpio0>;
> > +		interrupts = <RK_PA3 IRQ_TYPE_LEVEL_LOW>;
> > +		clock-output-names = "rk808-clkout1", "rk808-clkout2";
> > +		clock-names = "mclk";
> > +		clocks = <&cru I2S1_MCLKOUT_TX>;
> > +		assigned-clocks = <&cru I2S1_MCLKOUT_TX>;
> > +		assigned-clock-parents = <&cru CLK_I2S1_8CH_TX>;
> > +		#clock-cells = <1>;
> > +		#sound-dai-cells = <0>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&i2s1m0_mclk>, <&pmic_int_l>;
> > +		wakeup-source;
> > +
> > +		vcc1-supply = <&vcc_sys>;
> > +		vcc2-supply = <&vcc_sys>;
> > +		vcc3-supply = <&vcc_sys>;
> > +		vcc4-supply = <&vcc_sys>;
> > +		vcc5-supply = <&vcc_sys>;
> > +		vcc6-supply = <&vcc_sys>;
> > +		vcc7-supply = <&vcc_sys>;
> > +		vcc8-supply = <&vcc_sys>;
> > +		vcc9-supply = <&dcdc_boost>;
> > +
> > +		regulators {
> > +			vdd_logic: DCDC_REG1 {
> 
> No underscores in node names, unless the PMIC requires it.
> 

Ack.

> (...)
> 
> > +
> > +&pinctrl {
> > +
> 
> No need for blank line

Ack.

> 
> > +	audio-amplifier {
> > +		spk_amp_enable_h: spk-amp-enable-h {
> > +			rockchip,pins =
> > +				<4 RK_PC2 RK_FUNC_GPIO &pcfg_pull_none>;
> > +		};
> > +	};
> > +
> 
> Best regards,
> Krzysztof