Re: [PATCH v2 6/6] arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree

[Johan Hovold]

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