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.
