linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Doug Anderson <dianders@chromium.org>
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>
Subject: Re: [PATCH 1/2] arm64: dts: sc7180: Support Lazor/Limozeen rev9
Date: Thu, 28 Oct 2021 15:52:58 -0700	[thread overview]
Message-ID: <CAD=FV=U+rzQcZaihDCZuNd54nmRHTgr7TMThESECZRfxaBNkrg@mail.gmail.com> (raw)
In-Reply-To: <20211028151022.1.Ie56f55924f5c7706fe3194e710bbef6fdb8b5bc6@changeid>

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

  parent reply	other threads:[~2021-10-28 22:53 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-28 22:11 [PATCH 1/2] arm64: dts: sc7180: Support Lazor/Limozeen rev9 Philip Chen
2021-10-28 22:11 ` [PATCH 2/2] arm64: dts: sc7180: Support Homestar rev4 Philip Chen
2021-10-28 22:55   ` Doug Anderson
2021-10-29  0:42     ` Philip Chen
2021-10-28 22:52 ` Doug Anderson [this message]
2021-10-29  0:46   ` [PATCH 1/2] arm64: dts: sc7180: Support Lazor/Limozeen rev9 Philip Chen
2021-10-28 23:00 ` Stephen Boyd
2021-10-29  0:48   ` Philip Chen
2021-10-29  6:34     ` Stephen Boyd

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAD=FV=U+rzQcZaihDCZuNd54nmRHTgr7TMThESECZRfxaBNkrg@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=agross@kernel.org \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=philipchen@chromium.org \
    --cc=robh+dt@kernel.org \
    --cc=swboyd@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).