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
Message-ID: <460cd42b-4192-6761-7313-268a684e1e28@tom-fitzhenry.me.uk>
Date:   Mon, 22 Aug 2022 10:19:00 +1000
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux aarch64; rv:102.0) Gecko/20100101
 Thunderbird/102.0.3
Subject: Re: [PATCH v3 2/2] arm64: dts: rockchip: Add initial support for
 Pine64 PinePhone Pro
Content-Language: en-US
To:     =?UTF-8?B?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= <n@nfraprado.net>
Cc:     robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        heiko@sntech.de, martijn@brixit.nl, ayufan@ayufan.eu, megi@xff.cz,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-rockchip@lists.infradead.org, phone-devel@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220815123004.252014-1-tom@tom-fitzhenry.me.uk>
 <20220815123004.252014-3-tom@tom-fitzhenry.me.uk>
 <20220818030547.eblbmchutmnn6jih@notapiano>
From:   Tom Fitzhenry <tom@tom-fitzhenry.me.uk>
In-Reply-To: <20220818030547.eblbmchutmnn6jih@notapiano>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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.