lkml.org 
[lkml]   [2022]   [Aug]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH v3 2/2] arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro
From
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.

\
 
 \ /
  Last update: 2022-08-22 02:20    [W:0.618 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site