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

[Konrad Dybcio <konrad.dybcio@somainline.org>]

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.


> +
> +/*
> + * This pin goes to the display panel but then doesn't actually do anything
> + * on the panel itself (it doesn't connect to the touchscreen controller).
> + * We'll set a pullup here just to park the line.
> + */
> +&ts_rst_conn {
> +	bias-pull-up;
> +};
> +
>   /* PINCTRL - BOARD-SPECIFIC */
>   
>   /*
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts
> index b69ca09d9bfb..c1647a85a371 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-herobrine-r1.dts
> @@ -128,6 +128,17 @@ &sdhc_2 {
>   	status = "okay";
>   };
>   
> +/* PINCTRL - ADDITIONS TO NODES IN PARENT DEVICE TREE FILES */

Ditto


> +
> +/*
> + * This pin goes to the display panel but then doesn't actually do anything
> + * on the panel itself (it doesn't connect to the touchscreen controller).
> + * We'll set a pullup here just to park the line.
> + */
> +&ts_rst_conn {
> +	bias-pull-up;
> +};
> +
>   /* PINCTRL - BOARD-SPECIFIC */
>   
>   /*
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts
> index d3d6ffad4eff..950b69448109 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-herobrine-villager-r0.dts
> @@ -46,6 +46,25 @@ trackpad: trackpad@2c {
>   	};
>   };
>   
> +ts_i2c: &i2c13 {
> +	status = "okay";
> +	clock-frequency = <400000>;
> +
> +	ap_ts: touchscreen@10 {
> +		compatible = "elan,ekth6915";
> +		reg = <0x10>;
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&ts_int_conn>, <&ts_rst_conn>;
> +
> +		interrupt-parent = <&tlmm>;
> +		interrupts = <55 IRQ_TYPE_LEVEL_LOW>;
> +
> +		reset-gpios = <&tlmm 54 GPIO_ACTIVE_LOW>;
> +
> +		vcc33-supply = <&ts_avdd>;
> +	};
> +};
> +
>   &ap_sar_sensor_i2c {
>   	status = "okay";
>   };
> @@ -81,6 +100,12 @@ &sdhc_1 {
>   	status = "okay";
>   };
>   
> +/* PINCTRL - ADDITIONS TO NODES IN PARENT DEVICE TREE FILES */

Ditto


> +
> +&ts_rst_conn {
> +	bias-disable;
> +};
> +
>   /* PINCTRL - BOARD-SPECIFIC */
>   
>   /*
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi
> index d59002d4492e..404936c6bf20 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-qcard.dtsi
> @@ -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?


Konrad

>   		drive-strength = <2>;
>   	};
>   };
>