On 17/08/2022 23:49, Chris Morgan wrote:
> From: Chris Morgan <macromorgan@xxxxxxxxxxx>
>
> 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
> + 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
> + 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.
> + 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
> + 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@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.
(...)
> +
> +&pinctrl {
> +
No need for blank line
> + audio-amplifier {
> + spk_amp_enable_h: spk-amp-enable-h {
> + rockchip,pins =
> + <4 RK_PC2 RK_FUNC_GPIO &pcfg_pull_none>;
> + };
> + };
> +
Best regards,
Krzysztof
