Re: [PATCH] arm64: dts: qcom: sc7280: Add touchscreen to villager

[Doug Anderson <dianders@chromium.org>]

Hi,

On Tue, May 24, 2022 at 2:13 PM Konrad Dybcio
<konrad.dybcio@somainline.org> wrote:
>
>
> On 24/05/2022 22:48, Douglas Anderson wrote:
> > This adds the touchscreen to the sc7280-herobrine-villager device
> > tree. Note that the touchscreen on villager actually uses the reset
> > line and thus we use the more specific "elan,ekth6915" compatible
> > which allows us to specify the reset.
> >
> > The fact that villager's touchscreen uses the reset line can be
> > contrasted against the touchscreen for CRD/herobrine-r1. On those
> > boards, even though the touchscreen goes to the display, it's not
> > hooked up to anything there.
> >
> > In order to keep the line parked on herobrine/CRD, we'll move the
> > pullup from the qcard.dtsi file to the specific boards. This allows us
> > to disable the pullup in the villager device tree since the pin is an
> > output.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > This uses bindings introduced in the patch ("dt-bindings: HID:
> > i2c-hid: elan: Introduce bindings for Elan eKTH6915") [1].
> >
> > [1] https://lore.kernel.org/r/20220523142257.v2.1.Iedc61f9ef220a89af6a031200a7850a27a440134@changeid
> >
> >   .../boot/dts/qcom/sc7280-herobrine-crd.dts    | 11 ++++++++
> >   .../qcom/sc7280-herobrine-herobrine-r1.dts    | 11 ++++++++
> >   .../dts/qcom/sc7280-herobrine-villager-r0.dts | 25 +++++++++++++++++++
> >   arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi    |  1 -
> >   4 files changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > index a4ac33c4fd59..b79d84d7870a 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-crd.dts
> > @@ -134,6 +134,17 @@ &sdhc_2 {
> >       status = "okay";
> >   };
> >
> > +/* PINCTRL - ADDITIONS TO NODES IN PARENT DEVICE TREE FILES */
>
> Please drop this line, this isn't msm-3.4. It's immediately obvious that
> if a pin is referenced by a label and it is not defined in this file
> (because otherwise it wouldn't be both defined and referenced here..),
> it comes from a previously included device tree.

In general these headings specify a change in sort ordering. Without
them then either we intersperse pinctrl overrides with other stuff,
which IMO is overall worse or people have no idea why the sort
ordering changes.


> > @@ -604,7 +604,6 @@ ts_int_conn: ts-int-conn {
> >       ts_rst_conn: ts-rst-conn {
> >               pins = "gpio54";
> >               function = "gpio";
> > -             bias-pull-up;
>
> If you overwrite it where it should be overwritten, wouldn't it make
> more sense to leave bias-pull-up here as a default configuration for
> boards that don't have a peculiar routed-but-NC line?

Yeah, it'd be nice.  ...but because of the way "bias" is specified in
the device tree it means an ugly "/delete-property" in places that
actually route the line. I believe that, style wise, the preference is
to avoid delete-property and move the bias toward board files in cases
like this. Bjorn can feel free to override me if he disagrees.

-Doug