Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Feedback-ID: iefc945ae:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-841-g7899e99a45-fm-20220811.002-g7899e99a
Mime-Version: 1.0
Message-Id: <7782b803-b331-4b19-84f5-86d1f06b0259@www.fastmail.com>
In-Reply-To: <20220822083339.o6bqlyirrm5hvjbc@core>
References: <20220822012449.21005-1-tom@tom-fitzhenry.me.uk>
 <20220822012449.21005-2-tom@tom-fitzhenry.me.uk>
 <20220822083339.o6bqlyirrm5hvjbc@core>
Date:   Mon, 29 Aug 2022 04:28:03 +0000
From:   "Tom Fitzhenry" <tom@tom-fitzhenry.me.uk>
To:     =?UTF-8?Q?Ond=C5=99ej_Jirman?= <megi@xff.cz>
Cc:     "Rob Herring" <robh+dt@kernel.org>,
        "Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
        "Heiko Stuebner" <heiko@sntech.de>, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
        phone-devel@vger.kernel.org, "Martijn Braam" <martijn@brixit.nl>,
        =?UTF-8?Q?Kamil_Trzci=C5=84ski?= <ayufan@ayufan.eu>,
        "Caleb Connolly" <kc@postmarketos.org>,
        =?UTF-8?Q?N=C3=ADcolas_F_=2E_R_=2E_A_=2E_Prado?= <n@nfraprado.net>
Subject: Re: [PATCH v4 1/1] arm64: dts: rockchip: Add initial support for Pine64
 PinePhone Pro
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Thanks for the review Megi.

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

I will fix this in v5.

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

I will fix this in v5.

>> +			vcc3v0_touch: LDO_REG2 {
>> +				regulator-name =3D "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 =3D "vcca1v8_codec";
>> +				regulator-always-on;
>> +				regulator-boot-on;
>
> This should not be always on, but only enabled by the codec when neede=
d,
> 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 in=
put
> 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 =3D /bits/ 64 <200000000>;
>> +		opp-microvolt =3D <825000 825000 975000>;
>> +	};
>> +	opp01 {
>> +		opp-hz =3D /bits/ 64 <297000000>;
>> +		opp-microvolt =3D <825000 825000 975000>;
>> +	};
>> +	opp02 {
>> +		opp-hz =3D /bits/ 64 <400000000>;
>> +		opp-microvolt =3D <825000 825000 975000>;
>> +	};
>> +	opp03 {
>> +		opp-hz =3D /bits/ 64 <500000000>;
>> +		opp-microvolt =3D <875000 875000 975000>;
>> +	};
>> +	opp04 {
>> +		opp-hz =3D /bits/ 64 <600000000>;
>> +		opp-microvolt =3D <925000 925000 975000>;
>> +	};
>
> ^^^ Why replicate all these OPPs, when they have identical preferred v=
oltage
> 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 =3D "disabled";
>> +	};
>> +};
>> +
>> +
>
> ^ extra space

I will fix this in v5.