mbox series

[0/4] arm64: dts: qcom: sc7180: Disable the charger thermal zone on more trogdor boards

Message ID 20210304180415.1531430-1-mka@chromium.org
Headers show
Series arm64: dts: qcom: sc7180: Disable the charger thermal zone on more trogdor boards | expand

Message

Matthias Kaehlcke March 4, 2021, 6:04 p.m. UTC
We already disabled the charger thermal zone for lazor to avoid
bogus temperature readings from an unsupported thermistor. Some
revisions of other trogdor boards that are added by Doug's
'arm64: dts: qcom: Update sc7180-trogdor variants from downstream'
series have the same problem. Disable the charger thermal zone for
them too.

This series is based on v2 of the 'arm64: dts: qcom: Update
sc7180-trogdor variants from downstream' series
(https://patchwork.kernel.org/project/linux-arm-msm/list/?series=440315)


Matthias Kaehlcke (4):
  arm64: dts: qcom: sc7180: lazor: Simplify disabling of charger thermal
    zone
  arm64: dts: qcom: sc7180: Add pompom rev3
  arm64: dts: qcom: sc7180: pompom: Disable charger thermal zone for
    rev1 and rev2
  arm64: dts: qcom: sc7180: Disable charger thermal zone for coachz rev1
    and rev2

 .../dts/qcom/sc7180-trogdor-coachz-r1.dts     |  9 ++++
 .../dts/qcom/sc7180-trogdor-coachz-r2.dts     |  9 ++++
 .../boot/dts/qcom/sc7180-trogdor-lazor-r0.dts |  9 ----
 .../boot/dts/qcom/sc7180-trogdor-lazor-r1.dts |  9 ----
 .../boot/dts/qcom/sc7180-trogdor-lazor-r3.dts |  9 ----
 .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi   |  9 ++++
 .../dts/qcom/sc7180-trogdor-pompom-r1.dts     |  9 ++++
 .../dts/qcom/sc7180-trogdor-pompom-r2-lte.dts |  4 +-
 .../dts/qcom/sc7180-trogdor-pompom-r2.dts     | 13 +++++-
 .../dts/qcom/sc7180-trogdor-pompom-r3-lte.dts | 14 ++++++
 .../dts/qcom/sc7180-trogdor-pompom-r3.dts     | 46 +++++++++++++++++++
 11 files changed, 109 insertions(+), 31 deletions(-)
 create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts
 create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts

Comments

Doug Anderson March 5, 2021, 6:36 p.m. UTC | #1
Hi,

On Thu, Mar 4, 2021 at 10:04 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> Commit f73558cc83d1 ("arm64: dts: qcom: sc7180: Disable charger
> thermal zone for lazor") disables the charger thermal zone for
> specific lazor revisions due to an unsupported thermistor type.
> The initial idea was to disable the thermal zone for older
> revisions and leave it enabled for newer ones that use a
> supported thermistor. Finally the thermistor won't be changed
> on newer revisions, hence the thermal zone should be disabled
> for all lazor (and limozeen) revisions. Instead of disabling
> it per revision do it once in the shared .dtsi for lazor.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 9 ---------
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 9 ---------
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts | 9 ---------
>  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi   | 9 +++++++++
>  4 files changed, 9 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
> index 5c997cd90069..30e3e769d2b4 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts
> @@ -14,15 +14,6 @@ / {
>         compatible = "google,lazor-rev0", "qcom,sc7180";
>  };
>
> -/*
> - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> - * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> - * to avoid using bogus temperature values.
> - */
> -&charger_thermal {
> -       status = "disabled";
> -};
> -
>  &pp3300_hub {
>         /* pp3300_l7c is used to power the USB hub */
>         /delete-property/regulator-always-on;
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
> index d9fbcc7bc5bd..c2ef06367baf 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts
> @@ -14,15 +14,6 @@ / {
>         compatible = "google,lazor-rev1", "google,lazor-rev2", "qcom,sc7180";
>  };
>
> -/*
> - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> - * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> - * to avoid using bogus temperature values.
> - */
> -&charger_thermal {
> -       status = "disabled";
> -};
> -
>  &pp3300_hub {
>         /* pp3300_l7c is used to power the USB hub */
>         /delete-property/regulator-always-on;
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts
> index 19e69adb9e04..1b9d2f46359e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts
> @@ -13,12 +13,3 @@ / {
>         model = "Google Lazor (rev3+)";
>         compatible = "google,lazor", "qcom,sc7180";
>  };
> -
> -/*
> - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> - * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> - * to avoid using bogus temperature values.
> - */
> -&charger_thermal {
> -       status = "disabled";
> -};
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
> index 89e5cd29ec09..aa2c4a9098db 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
> @@ -58,6 +58,15 @@ ap_ts: touchscreen@10 {
>         };
>  };
>
> +/*
> + * Lazor is stuffed with a 47k NTC as charger thermistor which currently is
> + * not supported by the PM6150 ADC driver. Disable the charger thermal zone
> + * to avoid using bogus temperature values.
> + */
> +&charger_thermal {
> +       status = "disabled";
> +};
> +
>  /* PINCTRL - modifications to sc7180-trogdor.dtsi */
>
>  &ts_reset_l {

The idea is right, but I'm having a hard time figuring out what tree
you posted your patch against. You said you did it atop my "v2" series
[1], right?  ...but the "sc7180-trogdor-lazor.dtsi" really doesn't
match. In my tree, for instance, right above the PINCTRL comment
should be:

&wifi {
  qcom,ath10k-calibration-variant = "GO_LAZOR";
};

...but that's definitely not what's there in whatever your patch was
written against... It seems like you're also missing the panel and
trackpad nodes...

[1] https://patchwork.kernel.org/project/linux-arm-msm/list/?series=440315


-Doug
Doug Anderson March 5, 2021, 6:48 p.m. UTC | #2
Hi,

On Thu, Mar 4, 2021 at 10:04 AM Matthias Kaehlcke <mka@chromium.org> wrote:
>

> The only kernel visible change with respect to rev2 is that pompom

> rev3 changed the charger thermistor from a 47k to a 100k NTC to use

> a thermistor which is supported by the PM6150 ADC driver.

>

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

> ---

>

>  .../dts/qcom/sc7180-trogdor-pompom-r2-lte.dts |  4 +-

>  .../dts/qcom/sc7180-trogdor-pompom-r2.dts     |  4 +-

>  .../dts/qcom/sc7180-trogdor-pompom-r3-lte.dts | 14 ++++++

>  .../dts/qcom/sc7180-trogdor-pompom-r3.dts     | 46 +++++++++++++++++++

>  4 files changed, 64 insertions(+), 4 deletions(-)

>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts

>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts

>

> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts

> index 791d496ad046..00e187c08eb9 100644

> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts

> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts

> @@ -9,6 +9,6 @@

>  #include "sc7180-trogdor-lte-sku.dtsi"

>

>  / {

> -       model = "Google Pompom (rev2+) with LTE";

> -       compatible = "google,pompom-sku0", "qcom,sc7180";

> +       model = "Google Pompom (rev2) with LTE";

> +       compatible = "google,pompom-rev2-sku0", "qcom,sc7180";

>  };

> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts

> index 984d7337da78..2b2bd906321d 100644

> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts

> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts

> @@ -10,8 +10,8 @@

>  #include "sc7180-trogdor-pompom.dtsi"

>

>  / {

> -       model = "Google Pompom (rev2+)";

> -       compatible = "google,pompom", "qcom,sc7180";

> +       model = "Google Pompom (rev2)";

> +       compatible = "google,pompom-rev2", "qcom,sc7180";

>  };

>

>  &keyboard_controller {

> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts

> new file mode 100644

> index 000000000000..067cb75a011e

> --- /dev/null

> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts

> @@ -0,0 +1,14 @@

> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

> +/*

> + * Google Pompom board device tree source

> + *

> + * Copyright 2020 Google LLC.

> + */

> +

> +#include "sc7180-trogdor-pompom-r3.dts"

> +#include "sc7180-trogdor-lte-sku.dtsi"

> +

> +/ {

> +       model = "Google Pompom (rev3+) with LTE";

> +       compatible = "google,pompom-sku0", "qcom,sc7180";

> +};

> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts

> new file mode 100644

> index 000000000000..12d2d1e8e9e1

> --- /dev/null

> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts

> @@ -0,0 +1,46 @@

> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

> +/*

> + * Google Pompom board device tree source

> + *

> + * Copyright 2021 Google LLC.

> + */

> +

> +/dts-v1/;

> +

> +#include "sc7180-trogdor-pompom.dtsi"

> +

> +/ {

> +       model = "Google Pompom (rev3+)";

> +       compatible = "google,pompom", "qcom,sc7180";

> +};

> +

> +&keyboard_controller {

> +       function-row-physmap = <

> +               MATRIX_KEY(0x00, 0x02, 0)       /* T1 */

> +               MATRIX_KEY(0x03, 0x02, 0)       /* T2 */

> +               MATRIX_KEY(0x02, 0x02, 0)       /* T3 */

> +               MATRIX_KEY(0x01, 0x02, 0)       /* T4 */

> +               MATRIX_KEY(0x03, 0x04, 0)       /* T5 */

> +               MATRIX_KEY(0x02, 0x04, 0)       /* T6 */

> +               MATRIX_KEY(0x01, 0x04, 0)       /* T7 */

> +               MATRIX_KEY(0x02, 0x09, 0)       /* T8 */

> +               MATRIX_KEY(0x01, 0x09, 0)       /* T9 */

> +               MATRIX_KEY(0x00, 0x04, 0)       /* T10 */

> +       >;

> +       linux,keymap = <

> +               MATRIX_KEY(0x00, 0x02, KEY_BACK)

> +               MATRIX_KEY(0x03, 0x02, KEY_REFRESH)

> +               MATRIX_KEY(0x02, 0x02, KEY_ZOOM)

> +               MATRIX_KEY(0x01, 0x02, KEY_SCALE)

> +               MATRIX_KEY(0x03, 0x04, KEY_SYSRQ)

> +               MATRIX_KEY(0x02, 0x04, KEY_BRIGHTNESSDOWN)

> +               MATRIX_KEY(0x01, 0x04, KEY_BRIGHTNESSUP)

> +               MATRIX_KEY(0x02, 0x09, KEY_MUTE)

> +               MATRIX_KEY(0x01, 0x09, KEY_VOLUMEDOWN)

> +               MATRIX_KEY(0x00, 0x04, KEY_VOLUMEUP)

> +

> +               MATRIX_KEY(0x03, 0x09, KEY_SLEEP)       /* LOCK key */

> +

> +               CROS_STD_MAIN_KEYMAP

> +       >;

> +};


I don't love copying all this keymap stuff.  Options I can think of:

1. Just put it in "-rev3".  Have the "-rev2" dts just include the
"-rev3" dts and then override the model/compatible and disable the
charger_thermal.

2. Put the keyboard stuff in the "dtsi" file and then "-rev1" can have
something like:

/delete-node/ keyboard_controller;
#include <arm/cros-ec-keyboard.dtsi>

In general the preference is that the ugly device trees should get
pushed down to earlier revs since (eventually) they can just be
dropped.

I'll also mention that I don't see a huge benefit in this being a
separate patch from the next one--I'd just squash them together...

-Doug
Matthias Kaehlcke March 5, 2021, 8:11 p.m. UTC | #3
On Fri, Mar 05, 2021 at 10:36:49AM -0800, Doug Anderson wrote:
> Hi,

> 

> On Thu, Mar 4, 2021 at 10:04 AM Matthias Kaehlcke <mka@chromium.org> wrote:

> >

> > Commit f73558cc83d1 ("arm64: dts: qcom: sc7180: Disable charger

> > thermal zone for lazor") disables the charger thermal zone for

> > specific lazor revisions due to an unsupported thermistor type.

> > The initial idea was to disable the thermal zone for older

> > revisions and leave it enabled for newer ones that use a

> > supported thermistor. Finally the thermistor won't be changed

> > on newer revisions, hence the thermal zone should be disabled

> > for all lazor (and limozeen) revisions. Instead of disabling

> > it per revision do it once in the shared .dtsi for lazor.

> >

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

> > ---

> >

> >  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts | 9 ---------

> >  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts | 9 ---------

> >  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts | 9 ---------

> >  arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi   | 9 +++++++++

> >  4 files changed, 9 insertions(+), 27 deletions(-)

> >

> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts

> > index 5c997cd90069..30e3e769d2b4 100644

> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts

> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r0.dts

> > @@ -14,15 +14,6 @@ / {

> >         compatible = "google,lazor-rev0", "qcom,sc7180";

> >  };

> >

> > -/*

> > - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is

> > - * not supported by the PM6150 ADC driver. Disable the charger thermal zone

> > - * to avoid using bogus temperature values.

> > - */

> > -&charger_thermal {

> > -       status = "disabled";

> > -};

> > -

> >  &pp3300_hub {

> >         /* pp3300_l7c is used to power the USB hub */

> >         /delete-property/regulator-always-on;

> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts

> > index d9fbcc7bc5bd..c2ef06367baf 100644

> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts

> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r1.dts

> > @@ -14,15 +14,6 @@ / {

> >         compatible = "google,lazor-rev1", "google,lazor-rev2", "qcom,sc7180";

> >  };

> >

> > -/*

> > - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is

> > - * not supported by the PM6150 ADC driver. Disable the charger thermal zone

> > - * to avoid using bogus temperature values.

> > - */

> > -&charger_thermal {

> > -       status = "disabled";

> > -};

> > -

> >  &pp3300_hub {

> >         /* pp3300_l7c is used to power the USB hub */

> >         /delete-property/regulator-always-on;

> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts

> > index 19e69adb9e04..1b9d2f46359e 100644

> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts

> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor-r3.dts

> > @@ -13,12 +13,3 @@ / {

> >         model = "Google Lazor (rev3+)";

> >         compatible = "google,lazor", "qcom,sc7180";

> >  };

> > -

> > -/*

> > - * Lazor is stuffed with a 47k NTC as charger thermistor which currently is

> > - * not supported by the PM6150 ADC driver. Disable the charger thermal zone

> > - * to avoid using bogus temperature values.

> > - */

> > -&charger_thermal {

> > -       status = "disabled";

> > -};

> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi

> > index 89e5cd29ec09..aa2c4a9098db 100644

> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi

> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi

> > @@ -58,6 +58,15 @@ ap_ts: touchscreen@10 {

> >         };

> >  };

> >

> > +/*

> > + * Lazor is stuffed with a 47k NTC as charger thermistor which currently is

> > + * not supported by the PM6150 ADC driver. Disable the charger thermal zone

> > + * to avoid using bogus temperature values.

> > + */

> > +&charger_thermal {

> > +       status = "disabled";

> > +};

> > +

> >  /* PINCTRL - modifications to sc7180-trogdor.dtsi */

> >

> >  &ts_reset_l {

> 

> The idea is right, but I'm having a hard time figuring out what tree

> you posted your patch against. You said you did it atop my "v2" series

> [1], right?  ...but the "sc7180-trogdor-lazor.dtsi" really doesn't

> match. In my tree, for instance, right above the PINCTRL comment

> should be:

> 

> &wifi {

>   qcom,ath10k-calibration-variant = "GO_LAZOR";

> };

> 

> ...but that's definitely not what's there in whatever your patch was

> written against... It seems like you're also missing the panel and

> trackpad nodes...

> 

> [1] https://patchwork.kernel.org/project/linux-arm-msm/list/?series=440315


You got me! I was too lazy to pick all 13 patches, since most of them are
irrelevant for this series, but apparently I missed some that are. I guess
I'll pick them all for v2 ...
Matthias Kaehlcke March 5, 2021, 8:17 p.m. UTC | #4
On Fri, Mar 05, 2021 at 10:48:16AM -0800, Doug Anderson wrote:
> Hi,

> 

> On Thu, Mar 4, 2021 at 10:04 AM Matthias Kaehlcke <mka@chromium.org> wrote:

> >

> > The only kernel visible change with respect to rev2 is that pompom

> > rev3 changed the charger thermistor from a 47k to a 100k NTC to use

> > a thermistor which is supported by the PM6150 ADC driver.

> >

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

> > ---

> >

> >  .../dts/qcom/sc7180-trogdor-pompom-r2-lte.dts |  4 +-

> >  .../dts/qcom/sc7180-trogdor-pompom-r2.dts     |  4 +-

> >  .../dts/qcom/sc7180-trogdor-pompom-r3-lte.dts | 14 ++++++

> >  .../dts/qcom/sc7180-trogdor-pompom-r3.dts     | 46 +++++++++++++++++++

> >  4 files changed, 64 insertions(+), 4 deletions(-)

> >  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts

> >  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts

> >

> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts

> > index 791d496ad046..00e187c08eb9 100644

> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts

> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2-lte.dts

> > @@ -9,6 +9,6 @@

> >  #include "sc7180-trogdor-lte-sku.dtsi"

> >

> >  / {

> > -       model = "Google Pompom (rev2+) with LTE";

> > -       compatible = "google,pompom-sku0", "qcom,sc7180";

> > +       model = "Google Pompom (rev2) with LTE";

> > +       compatible = "google,pompom-rev2-sku0", "qcom,sc7180";

> >  };

> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts

> > index 984d7337da78..2b2bd906321d 100644

> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts

> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r2.dts

> > @@ -10,8 +10,8 @@

> >  #include "sc7180-trogdor-pompom.dtsi"

> >

> >  / {

> > -       model = "Google Pompom (rev2+)";

> > -       compatible = "google,pompom", "qcom,sc7180";

> > +       model = "Google Pompom (rev2)";

> > +       compatible = "google,pompom-rev2", "qcom,sc7180";

> >  };

> >

> >  &keyboard_controller {

> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts

> > new file mode 100644

> > index 000000000000..067cb75a011e

> > --- /dev/null

> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3-lte.dts

> > @@ -0,0 +1,14 @@

> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

> > +/*

> > + * Google Pompom board device tree source

> > + *

> > + * Copyright 2020 Google LLC.

> > + */

> > +

> > +#include "sc7180-trogdor-pompom-r3.dts"

> > +#include "sc7180-trogdor-lte-sku.dtsi"

> > +

> > +/ {

> > +       model = "Google Pompom (rev3+) with LTE";

> > +       compatible = "google,pompom-sku0", "qcom,sc7180";

> > +};

> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts

> > new file mode 100644

> > index 000000000000..12d2d1e8e9e1

> > --- /dev/null

> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom-r3.dts

> > @@ -0,0 +1,46 @@

> > +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)

> > +/*

> > + * Google Pompom board device tree source

> > + *

> > + * Copyright 2021 Google LLC.

> > + */

> > +

> > +/dts-v1/;

> > +

> > +#include "sc7180-trogdor-pompom.dtsi"

> > +

> > +/ {

> > +       model = "Google Pompom (rev3+)";

> > +       compatible = "google,pompom", "qcom,sc7180";

> > +};

> > +

> > +&keyboard_controller {

> > +       function-row-physmap = <

> > +               MATRIX_KEY(0x00, 0x02, 0)       /* T1 */

> > +               MATRIX_KEY(0x03, 0x02, 0)       /* T2 */

> > +               MATRIX_KEY(0x02, 0x02, 0)       /* T3 */

> > +               MATRIX_KEY(0x01, 0x02, 0)       /* T4 */

> > +               MATRIX_KEY(0x03, 0x04, 0)       /* T5 */

> > +               MATRIX_KEY(0x02, 0x04, 0)       /* T6 */

> > +               MATRIX_KEY(0x01, 0x04, 0)       /* T7 */

> > +               MATRIX_KEY(0x02, 0x09, 0)       /* T8 */

> > +               MATRIX_KEY(0x01, 0x09, 0)       /* T9 */

> > +               MATRIX_KEY(0x00, 0x04, 0)       /* T10 */

> > +       >;

> > +       linux,keymap = <

> > +               MATRIX_KEY(0x00, 0x02, KEY_BACK)

> > +               MATRIX_KEY(0x03, 0x02, KEY_REFRESH)

> > +               MATRIX_KEY(0x02, 0x02, KEY_ZOOM)

> > +               MATRIX_KEY(0x01, 0x02, KEY_SCALE)

> > +               MATRIX_KEY(0x03, 0x04, KEY_SYSRQ)

> > +               MATRIX_KEY(0x02, 0x04, KEY_BRIGHTNESSDOWN)

> > +               MATRIX_KEY(0x01, 0x04, KEY_BRIGHTNESSUP)

> > +               MATRIX_KEY(0x02, 0x09, KEY_MUTE)

> > +               MATRIX_KEY(0x01, 0x09, KEY_VOLUMEDOWN)

> > +               MATRIX_KEY(0x00, 0x04, KEY_VOLUMEUP)

> > +

> > +               MATRIX_KEY(0x03, 0x09, KEY_SLEEP)       /* LOCK key */

> > +

> > +               CROS_STD_MAIN_KEYMAP

> > +       >;

> > +};

> 

> I don't love copying all this keymap stuff.  Options I can think of:


agreed

> 1. Just put it in "-rev3".  Have the "-rev2" dts just include the

> "-rev3" dts and then override the model/compatible and disable the

> charger_thermal.

> 

> 2. Put the keyboard stuff in the "dtsi" file and then "-rev1" can have

> something like:

> 

> /delete-node/ keyboard_controller;

> #include <arm/cros-ec-keyboard.dtsi>

> 

> In general the preference is that the ugly device trees should get

> pushed down to earlier revs since (eventually) they can just be

> dropped.


Thanks, I'll evaluate the options you suggested.

> I'll also mention that I don't see a huge benefit in this being a

> separate patch from the next one--I'd just squash them together...


It keeps the individual patches simpler by doing one thing at a time,
but you can certainly go either way.