Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211028151022.1.Ie56f55924f5c7706fe3194e710bbef6fdb8b5bc6@changeid>
In-Reply-To: <20211028151022.1.Ie56f55924f5c7706fe3194e710bbef6fdb8b5bc6@changeid>
From:   Doug Anderson <dianders@chromium.org>
Date:   Thu, 28 Oct 2021 15:52:58 -0700
Message-ID: <CAD=FV=U+rzQcZaihDCZuNd54nmRHTgr7TMThESECZRfxaBNkrg@mail.gmail.com>
Subject: Re: [PATCH 1/2] arm64: dts: sc7180: Support Lazor/Limozeen rev9
To:     Philip Chen <philipchen@chromium.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Stephen Boyd <swboyd@chromium.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Thu, Oct 28, 2021 at 3:11 PM Philip Chen <philipchen@chromium.org> wrote:
>
> Support Lazor/Limozeen rev9 board where Parade ps8640 is added as the
> second source edp bridge.

It probably wouldn't hurt to at least mention in the commit message
why the include of "sc7180.dtsi" was moved out of
"sc7180-trogdor-lazor.dtsi" so that people reviewing the change aren't
confused. Otherwise it just looks arbitrary.


> Signed-off-by: Philip Chen <philipchen@chromium.org>
> ...
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots-r5.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots-r5.dts
> new file mode 100644
> index 000000000000..a2a6e2515ee6
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots-r5.dts
> @@ -0,0 +1,31 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Lazor Limozeen board device tree source
> + *
> + * Copyright 2021 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sc7180.dtsi"
> +#include "sc7180-trogdor-ti-sn65dsi86.dtsi"
> +#include "sc7180-trogdor-lazor.dtsi"
> +#include "sc7180-trogdor-lte-sku.dtsi"
> +
> +/ {
> +       model = "Google Lazor Limozeen without Touchscreen (rev5 - rev8)";
> +       /* No sku5 post-rev5 */
> +       compatible = "google,lazor-rev5-sku6", "google,lazor-rev6-sku6",
> +               "google,lazor-rev7-sku6", "google,lazor-rev8-sku6",
> +               "google,rev5-sku5", "qcom,sc7180";

Two issues.

1. "google,rev5-sku5" should probably be "google,lazor-rev5-sku5"

2. rev5-sku5 should sort to the start of the list, not to the end.


> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-nots-r9.dts
> @@ -2,16 +2,18 @@
>  /*
>   * Google Lazor Limozeen board device tree source
>   *
> - * Copyright 2020 Google LLC.
> + * Copyright 2021 Google LLC.
>   */
>
>  /dts-v1/;
>
> +#include "sc7180.dtsi"
> +#include "sc7180-trogdor-parade-ps8640.dtsi"
>  #include "sc7180-trogdor-lazor.dtsi"
>  #include "sc7180-trogdor-lte-sku.dtsi"
>
>  / {
> -       model = "Google Lazor Limozeen without Touchscreen";
> +       model = "Google Lazor Limozeen without Touchscreen (rev9+)";
>         compatible = "google,lazor-sku6", "google,lazor-sku5", "qcom,sc7180";

Earlier you said "sku5" doesn't exist past -rev5. So it doesn't need
to be here, right?


> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-r4.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-r4.dts
> new file mode 100644
> index 000000000000..10f48f4b429a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-limozeen-r4.dts
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Lazor Limozeen board device tree source
> + *
> + * Copyright 2021 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sc7180.dtsi"
> +#include "sc7180-trogdor-ti-sn65dsi86.dtsi"
> +#include "sc7180-trogdor-lazor.dtsi"
> +#include "sc7180-trogdor-lte-sku.dtsi"
> +
> +/ {
> +       model = "Google Lazor Limozeen (rev4 - rev8)";
> +       compatible = "google,lazor-rev4-sku4", "google,lazor-rev5-sku4",
> +               "google,lazor-rev6-sku4", "google,lazor-rev6-sku4",

-rev6 is listed twice and -rev7 is missing.


> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9-kb.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9-kb.dts
> new file mode 100644
> index 000000000000..326ba9f88394
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9-kb.dts
> @@ -0,0 +1,26 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Lazor board device tree source
> + *
> + * Copyright 2020 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sc7180.dtsi"
> +#include "sc7180-trogdor-parade-ps8640.dtsi"
> +#include "sc7180-trogdor-lazor.dtsi"
> +#include "sc7180-lite.dtsi"
> +
> +/ {
> +       model = "Google Lazor (rev9+) with KB Backlight";
> +       compatible = "google,lazor-sku2", "qcom,sc7180";
> +};
> +
> +&keyboard_backlight {
> +       status = "okay";
> +};
> +
> +&panel {
> +       compatible = "boe,nv133fhm-n62";
> +};

Why do you need the "panel" here?


> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9-lte.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9-lte.dts
> new file mode 100644
> index 000000000000..a705c956f62b
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9-lte.dts
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Lazor board device tree source
> + *
> + * Copyright 2021 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sc7180.dtsi"
> +#include "sc7180-trogdor-parade-ps8640.dtsi"
> +#include "sc7180-trogdor-lazor.dtsi"
> +#include "sc7180-trogdor-lte-sku.dtsi"
> +
> +/ {
> +       model = "Google Lazor (rev9+) with LTE";
> +       compatible = "google,lazor-sku0", "qcom,sc7180";
> +};
> +
> +&ap_sar_sensor {
> +       status = "okay";
> +};
> +
> +&ap_sar_sensor_i2c {
> +       status = "okay";
> +};
> +
> +&keyboard_backlight {
> +       status = "okay";
> +};
> +
> +&panel {
> +       compatible = "boe,nv133fhm-n62";
> +};

Why do you need the "panel" hre?


> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9.dts
> new file mode 100644
> index 000000000000..13ff9a851613
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r9.dts
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Lazor board device tree source
> + *
> + * Copyright 2021 Google LLC.
> + */
> +
> +/dts-v1/;
> +
> +#include "sc7180.dtsi"
> +#include "sc7180-trogdor-parade-ps8640.dtsi"
> +#include "sc7180-trogdor-lazor.dtsi"
> +#include "sc7180-lite.dtsi"
> +
> +/ {
> +       model = "Google Lazor (rev9+)";
> +       compatible = "google,lazor", "qcom,sc7180";
> +};
> +
> +&panel {
> +       compatible = "boe,nv133fhm-n62";
> +};

Why do you need the "panel" here?


> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index faf8b807d0ff..286d3594b6ae 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -11,6 +11,7 @@
>  #include <dt-bindings/clock/qcom,lpasscorecc-sc7180.h>
>  #include <dt-bindings/clock/qcom,rpmh.h>
>  #include <dt-bindings/clock/qcom,videocc-sc7180.h>
> +#include <dt-bindings/gpio/gpio.h>

I don't think you should be adding this to sc7180.dtsi. If you need it
in the bridge chip "fragments" you should add the includes to those.

-Doug