Re: [PATCH 2/4] arm64: dts: qcom: sc7180: Add pompom rev3

[Matthias Kaehlcke <mka@chromium.org>]

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.