Re: [PATCH v3 2/2] arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro

[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.