Re: [PATCH 12/13] arm64: dts: qcom: Add sc7180-lazor-pompom skus

From: Matthias Kaehlcke <mka@chromium.org>
To: Douglas Anderson <dianders@chromium.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Alexandru M Stan <amstan@chromium.org>,
	Rob Clark <robdclark@chromium.org>,
	Philip Chen <philipchen@google.com>,
	Tzung-Bi Shih <tzungbi@chromium.org>,
	Judy Hsiao <judyhsiao@chromium.org>,
	Andy Gross <agross@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	devicetree@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 12/13] arm64: dts: qcom: Add sc7180-lazor-pompom skus
Date: Thu, 25 Feb 2021 18:13:28 -0800	[thread overview]
Message-ID: <YDhZSAd0XLgdHEEm@google.com> (raw)
In-Reply-To: <20210225141022.12.If93a01b30d20dccacbad4be8ddc519dc20a51a1e@changeid>

On Thu, Feb 25, 2021 at 02:13:09PM -0800, Douglas Anderson wrote:
> This is a trogdor variant.  This is mostly a grab from the downstream
> tree with notable exceptions:
> - I skip -rev0.  This was a super early build and there's no advantage
>   of long term support.
> - In -rev1 I translate the handling of the USB hub like is done for
>   similar boards.  See the difference between the downstream and
>   upstream 'sc7180-trogdor-lazor-r0.dts' for an example.  This will
>   need to be resolved when proper support for the USB hub is figured
>   out upstream.
> - I remove sound node since sound hasn't landed upstream yet.
> - In incorporate the pending <https://crrev.com/c/2719075> for the
>   keyboard.
> 
> Cc: Philip Chen <philipchen@google.com>
> Cc: Matthias Kaehlcke <mka@chromium.org>
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: Tzung-Bi Shih <tzungbi@chromium.org>
> Cc: Judy Hsiao <judyhsiao@chromium.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

<snip>

> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
> new file mode 100644
> index 000000000000..8f1596b8e90a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Pompom board device tree source
> + *
> + * Copyright 2020 Google LLC.
> + */
> +
> +#include "sc7180.dtsi"
> +
> +ap_ec_spi: &spi6 {};
> +ap_h1_spi: &spi0 {};
> +
> +#include "sc7180-trogdor.dtsi"
> +
> +/ {
> +	thermal-zones {
> +		5v-choke-thermal {
> +			polling-delay-passive = <0>;
> +			polling-delay = <250>;
> +
> +			thermal-sensors = <&pm6150_adc_tm 0>;

This is fine with how things are currently configured for trogdor, however be
aware that in the ADC thermal monitor config your patch is racing with 'arm64:
dts: qcom: sc7180: trogdor: Use ADC TM channel 0 instead of 1 for charger
temperature' (https://lore.kernel.org/patchwork/patch/1384514/). That patch
changes the charger thermistor for all trogdor boards to ADC TM channel 0,
so the 5v-choke thermistor would have to move to another channel (most
likely 1).

<snip>

> +&pm6150_adc_tm {
> +	status = "okay";
> +
> +	5v-choke-thermistor@0 {
> +		reg = <0>;

s/0/1/ in the two lines above if 'arm64: dts: qcom: sc7180: trogdor: Use ADC
TM channel 0 instead of 1 for charger temperature' lands before this patch.

The other deltas with downstream are mentioned in the commit message, as long
as we keep the change of the ADC TM channel in mind this looks good to me.

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>