[PATCH v4 1/1] arm64: dts: rockchip: Add initial support for Pine64 PinePhone Pro

[Tom Fitzhenry]

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.