Re: [PATCH 3/3] ARM: dts: qcom: Add support for Samsung Galaxy Tab 4 10.1 (SM-T530)

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 17/07/2022 23:34, Matti Lehtimäki wrote:
> Add a device tree for the Samsung Galaxy Tab 4 10.1 (SM-T530) wifi tablet
> based on the apq8026 platform.
> 
> Currently supported are accelerometer sensor, hall sensor, internal storage, physical
> buttons (power & volume), screen (based on simple-framebuffer set up by
> the bootloader) sdcard, touchscreen and USB.
> 
> Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com>

Thank you for your patch. There is something to discuss/improve.

> ---
>  arch/arm/boot/dts/Makefile                    |   1 +
>  .../dts/qcom-apq8026-samsung-matissewifi.dts  | 475 ++++++++++++++++++
>  2 files changed, 476 insertions(+)
>  create mode 100644 arch/arm/boot/dts/qcom-apq8026-samsung-matissewifi.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 5112f493f494..4d02a1740079 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -1010,6 +1010,7 @@ dtb-$(CONFIG_ARCH_QCOM) += \
>  	qcom-apq8016-sbc.dtb \
>  	qcom-apq8026-asus-sparrow.dtb \
>  	qcom-apq8026-lg-lenok.dtb \
> +	qcom-apq8026-samsung-matissewifi.dtb \
>  	qcom-apq8060-dragonboard.dtb \
>  	qcom-apq8064-cm-qs600.dtb \
>  	qcom-apq8064-ifc6410.dtb \
> diff --git a/arch/arm/boot/dts/qcom-apq8026-samsung-matissewifi.dts b/arch/arm/boot/dts/qcom-apq8026-samsung-matissewifi.dts
> new file mode 100644
> index 000000000000..f4c5eb9db11c
> --- /dev/null
> +++ b/arch/arm/boot/dts/qcom-apq8026-samsung-matissewifi.dts
> @@ -0,0 +1,475 @@
> +// SPDX-License-Identifier: BSD-3-Clause
> +/*
> + * Copyright (c) 2022, Matti Lehtimäki <matti.lehtimaki@gmail.com>
> + */
> +
> +/dts-v1/;
> +
> +#include "qcom-msm8226.dtsi"
> +#include "qcom-pm8226.dtsi"
> +#include <dt-bindings/input/input.h>
> +
> +/delete-node/ &smem_region;
> +
> +/ {
> +	model = "Samsung Galaxy Tab 4 10.1";
> +	compatible = "samsung,matissewifi", "qcom,apq8026";
> +	chassis-type = "tablet";
> +
> +	qcom,msm-id = <0xC708FF01 0 0x20000>,
> +		      <0xC708FF01 1 0x20000>,
> +		      <0xC708FF01 2 0x20000>,
> +		      <0xC708FF01 3 0x20000>;

Lower case hex and does not match bindings.
https://lore.kernel.org/all/20220705130300.100882-2-krzysztof.kozlowski@linaro.org/

This would need detailed explanation because it really does not look
correct.

> +
> +	aliases {
> +		mmc0 = &sdhc_1; /* SDC1 eMMC slot */
> +		mmc1 = &sdhc_2; /* SDC2 SD card slot */
> +		display0 = &framebuffer0;
> +	};
> +
> +	chosen {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		stdout-path = "display0";
> +
> +		framebuffer0: framebuffer@3200000 {
> +			compatible = "simple-framebuffer";
> +			reg = <0x3200000 0x800000>;
> +			width = <1280>;
> +			height = <800>;
> +			stride = <(1280 * 3)>;
> +			format = "r8g8b8";
> +			status = "okay";
> +		};
> +	};
> +
> +	gpio-hall-sensor {
> +		compatible = "gpio-keys";
> +
> +		event-hall-sensor {
> +			label = "Hall Effect Sensor";
> +			gpios = <&tlmm 110 GPIO_ACTIVE_HIGH>;
> +			interrupts = <&tlmm 110 IRQ_TYPE_EDGE_FALLING>;
> +			linux,input-type = <EV_SW>;
> +			linux,code = <SW_LID>;
> +			debounce-interval = <15>;
> +			wakeup-source;
> +		};
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		autorepeat;
> +
> +		key-home {
> +			label = "Home";
> +			gpios = <&tlmm 108 GPIO_ACTIVE_LOW>;
> +			linux,code = <KEY_HOMEPAGE>;
> +			debounce-interval = <15>;
> +		};
> +
> +		key-volume-down {
> +			label = "Volume Down";
> +			gpios = <&tlmm 107 GPIO_ACTIVE_LOW>;
> +			linux,code = <KEY_VOLUMEDOWN>;
> +			debounce-interval = <15>;
> +		};
> +
> +		key-volume-up {
> +			label = "Volume Up";
> +			gpios = <&tlmm 106 GPIO_ACTIVE_LOW>;
> +			linux,code = <KEY_VOLUMEUP>;
> +			debounce-interval = <15>;
> +		};
> +	};
> +
> +	i2c-muic {
> +		compatible = "i2c-gpio";
> +		sda-gpios = <&tlmm 14 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +		scl-gpios = <&tlmm 15 (GPIO_ACTIVE_HIGH|GPIO_OPEN_DRAIN)>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&muic_i2c_default_state>;
> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		muic: extcon@25 {

extcon is Linux specific, need some generic name instead.

> +			compatible = "siliconmitus,sm5502-muic";
> +			reg = <0x25>;
> +
> +			interrupt-parent = <&tlmm>;
> +			interrupts = <67 IRQ_TYPE_EDGE_FALLING>;
> +
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&muic_int_default_state>;
> +		};
> +	};
> +
> +	reg_tsp_1p8v: regulator-tsp-1p8v {
> +		compatible = "regulator-fixed";
> +		regulator-name = "tsp_1p8v";
> +		regulator-min-microvolt = <1800000>;
> +		regulator-max-microvolt = <1800000>;
> +
> +		gpio = <&tlmm 31 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&tsp_en_default_state>;
> +	};
> +
> +	reg_tsp_3p3v: regulator-tsp-3p3v {
> +		compatible = "regulator-fixed";
> +		regulator-name = "tsp_3p3v";
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +
> +		gpio = <&tlmm 73 GPIO_ACTIVE_HIGH>;
> +		enable-active-high;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&tsp_en1_default_state>;
> +	};
> +
> +	reserved-memory {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +
> +		framebuffer@3200000 {

Generic node names, so memory@

> +			reg = <0x3200000 0x800000>;
> +			no-map;
> +		};
> +
> +		mpss_region: mpss@8400000 {

The same and so on...

> +			reg = <0x08400000 0x1f00000>;
> +			no-map;
> +		};
> +
> +		mba_region: mba@a300000 {

Best regards,
Krzysztof