Messages in this thread | | | Date | Mon, 22 Aug 2022 10:19:00 +1000 | Subject | Re: [PATCH v3 2/2] arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro | From | Tom Fitzhenry <> |
| |
On 18/8/22 13:05, Nícolas F. R. A. Prado wrote:
> thanks for getting the upstreaming of this DT going. Some comments below.
No worries, thank you for your review!
> You're also adding the SD controller here. Does it work as is? If so add it to > the commit description as well. I will note this in v4. >> +/* PinePhone Pro datasheet: > First comment line should be empty following the coding style [1]. Like you did > for the copyrights above. > > [1] https://www.kernel.org/doc/html/latest/process/coding-style.html#commenting
I will do this in v4.
> This signal is called vcc_sys in the datasheet, so I suggest we keep that name > here. It's not everyday that we get a device with a publicly available datasheet > :^).
Indeed! :) I will do this in v4.
> + rk818: pmic@1c { >> + compatible = "rockchip,rk818"; >> + reg = <0x1c>; >> + interrupt-parent = <&gpio1>; >> + interrupts = <RK_PC5 IRQ_TYPE_LEVEL_LOW>; >> + #clock-cells = <1>; >> + clock-output-names = "xin32k", "rk808-clkout2"; > What about keeping the datasheet names here too? clk32kout1, clk32kout2 Per Megi's response, I'll stick with the current names. >> + vcc_1v8: vcc_wl: DCDC_REG4 { > From the datasheet, vcc_wl is actually wired to vcc3v3_sys. But looks like > vcc_wl is only used for bluetooth and you're not enabling it yet anyway, so just > drop this extra label, and it can be added when bluetooth is added (or not, and > then the bluetooth supply just points directly to vcc3v3_sys). Good catch, I will remove the vcc_wl label. >> + vcc_power_on: LDO_REG4 { >> + regulator-name = "vcc_power_on"; > The name on the datasheet for this one is rk818_pwr_on. I will use the name rk818_pwr_on in v4. >> + >> +&cluster1_opp { >> + opp06 { >> + status = "disabled"; >> + }; > There's actually an opp06 node in the OPP for RK3399-T, only that the frequency > is slightly lower. Maybe you could keep it enabled but override the frequency? > > Or given the above point about the max voltages, maybe it would be best to have > a separate OPP table after all? Per Megi's response/rationale, I'll keep the existing table, but re-introduce cluster1_opp/opp06 with updated frequency/voltage, aligned with the RK3399-T datasheet. >> + >> + opp07 { >> + status = "disabled"; >> + }; >> +}; >> + >> +&io_domains { >> + status = "okay"; > Let's keep the status at the end of the node for consistency with the rest. I will do this in v4.
| |