lkml.org 
[lkml]   [2022]   [Aug]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 1/1] arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro
Thanks for the review Megi.

On Mon, 22 Aug 2022, at 8:33 AM, Ondřej Jirman wrote:
>> + vdd_center: DCDC_REG2 {
>> + regulator-name = "vdd_center";
>> + regulator-always-on;
>> + regulator-boot-on;
>> + regulator-min-microvolt = <800000>;
>> + regulator-max-microvolt = <1350000>;
>
> Looks like a wrong top voltage. https://megous.com/dl/tmp/ad3dcc62bd00f41f.png

I will fix this in v5.

>> + vcca3v0_codec: LDO_REG1 {
>> + regulator-name = "vcca3v0_codec";
>> + regulator-always-on;
>> + regulator-boot-on;
>
> This should not be always on, but only enabled by the codec when needed.
> You don't have codec described in this DT.

I will fix this in v5.

>> + vcc3v0_touch: LDO_REG2 {
>> + regulator-name = "vcc3v0_touch";
>> + regulator-always-on;
>> + regulator-boot-on;
>
> This should not be always on. It should be enabled by touch controller,
> when needed. You don't have touch controller described in this DT.

I will fix this in v5.

>> + vcca1v8_codec: LDO_REG3 {
>> + regulator-name = "vcca1v8_codec";
>> + regulator-always-on;
>> + regulator-boot-on;
>
> This should not be always on, but only enabled by the codec when needed,
> I suppose. Also modem codec is supplied by vcc1v8_codec which may need
> a gpio configured as pull-down or drive low to be properly disabled,
> and it is not defined in this DT. Please make sure that regulator's input
> doesn't float and is disabled by default.

I will fix this in v5, including adding a regulator for vcc1v8_codec.

>> +&gpu_opp_table {
>> + opp00 {
>> + opp-hz = /bits/ 64 <200000000>;
>> + opp-microvolt = <825000 825000 975000>;
>> + };
>> + opp01 {
>> + opp-hz = /bits/ 64 <297000000>;
>> + opp-microvolt = <825000 825000 975000>;
>> + };
>> + opp02 {
>> + opp-hz = /bits/ 64 <400000000>;
>> + opp-microvolt = <825000 825000 975000>;
>> + };
>> + opp03 {
>> + opp-hz = /bits/ 64 <500000000>;
>> + opp-microvolt = <875000 875000 975000>;
>> + };
>> + opp04 {
>> + opp-hz = /bits/ 64 <600000000>;
>> + opp-microvolt = <925000 925000 975000>;
>> + };
>
> ^^^ Why replicate all these OPPs, when they have identical preferred voltage
> in rk3399-opp.dtsi? Also GPU is not being enabled in the DT.
>
> You don't need display output support to enable the gpu right away.

My bad, I had forgotten that I'd originally decided to leave this out of the first patch series. I will address this in the patch series when I enable the GPU.

>> + opp05 {
>> + status = "disabled";
>> + };
>> +};
>> +
>> +
>
> ^ extra space

I will fix this in v5.

\
 
 \ /
  Last update: 2022-08-29 06:29    [W:0.046 / U:0.396 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site