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@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
>
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@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
