On Wed, Jun 22, 2022 at 02:39:43PM +0200, Konrad Dybcio wrote:
> On 22.06.2022 06:12, Bjorn Andersson wrote:
> > From: Johan Hovold <johan+linaro@xxxxxxxxxx>
> >
> > Add an initial Lenovo Thinkpad X13s devicetree.
> >
> > Signed-off-by: Johan Hovold <johan+linaro@xxxxxxxxxx>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > ---
> >
> > Changs since v1:
> > - New patch
> >
> > arch/arm64/boot/dts/qcom/Makefile | 1 +
> > .../qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 393 ++++++++++++++++++
> > 2 files changed, 394 insertions(+)
> > create mode 100644 arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> >
> > diff --git a/arch/arm64/boot/dts/qcom/Makefile b/arch/arm64/boot/dts/qcom/Makefile
> > index 2f416b84b71c..43053f1f6be9 100644
> > --- a/arch/arm64/boot/dts/qcom/Makefile
> > +++ b/arch/arm64/boot/dts/qcom/Makefile
> > @@ -91,6 +91,7 @@ dtb-$(CONFIG_ARCH_QCOM) += sc7280-idp.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += sc7280-idp2.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += sc7280-crd-r3.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += sc8280xp-crd.dtb
> > +dtb-$(CONFIG_ARCH_QCOM) += sc8280xp-lenovo-thinkpad-x13s.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += sdm630-sony-xperia-ganges-kirin.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += sdm630-sony-xperia-nile-discovery.dtb
> > dtb-$(CONFIG_ARCH_QCOM) += sdm630-sony-xperia-nile-pioneer.dtb
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > new file mode 100644
> > index 000000000000..58d3d8035751
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> > @@ -0,0 +1,393 @@
> > +// SPDX-License-Identifier: BSD-3-Clause
> > +/*
> > + * Copyright (c) 2021, The Linux Foundation. All rights reserved.
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> Can be moved to SoC dtsi.
Again, no, the SoC dtsi does not need that header and we include files
where they are used.
> > +&qup0_i2c4 {
> > + clock-frequency = <400000>;
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&qup0_i2c4_default>, <&ts0_default>;
> > +
> > + status = "okay";
> Again, the ordering of status= question from one of the previous patches.
This is the right order, but the ADP dts hasn't been updated yet.
>
> > +
> > + /* FIXME: verify, missing on some x13s */
> Won't this fail gracefully-ish if it's absent?
Sure, but that's not the point. I don't access to a X13s with a
touchscreen yet so the i2c address and hid descriptor address has not
been verified yet.
> > + touchscreen@10 {
> > + compatible = "hid-over-i2c";
> > + reg = <0x10>;
> > + hid-descr-addr = <0x1>;
> > + interrupts-extended = <&tlmm 175 IRQ_TYPE_LEVEL_LOW>;
> > + vdd-supply = <&vreg_misc_3p3>;
> > + };
> > +};
> > +/* PINCTRL - additions to nodes defined in sc8280xp.dtsi */
> Seems redundant.
Again, no. But I've dropped the reference to sc8280xp.dtsi.
> > + tpad_default: tpad-default-state {
> > + int-n {
> Same comment as in the reference device dt.
I think it's better to keep the child node as documentation.
Johan
