by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 13/13] ARM: dts: exynos: Add Samsung Galaxy Tab 3 8.0 boards

On 01/05/2023 21:55, Artur Weber wrote:
> Introduce support for the Galaxy Tab 3 8.0 series of boards:
>
> - Samsung Galaxy Tab 3 8.0 WiFi (SM-T310/lt01wifi)
> - Samsung Galaxy Tab 3 8.0 3G (SM-T311/lt013g)
> - Samsung Galaxy Tab 3 8.0 LTE (SM-T315/lt01lte)
>
> What works:
>
> - Display and backlight
> - Touchscreen (without touchkeys)
> - GPIO buttons, hall sensor
> - WiFi and Bluetooth
> - USB, fuel gauge, charging
> - Accelerometer and magnetometer
> - WiFi model only: light sensor
>
> Signed-off-by: Artur Weber <[email protected]>

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

> ---
> Changed in v3:
> - Addressed review comments
> - Removed rtc node (RTC is provided by PMIC)
> - Added CPU thermal node
> - Fixed dtb_check warnings
> - Added common changes from next/dt
>
> Display panel bindings are added by a separate patchset:
> "[PATCH 0/3] Add Samsung S6D7AA0 panel controller driver"[1]
>
> LP855X node is adapted to changes from a separate patchset:
> "[PATCH 0/4] video: backlight: lp855x: modernize bindings"[2]
>
> [1] https://lore.kernel.org/all/[email protected]/
> [2] https://lore.kernel.org/all/[email protected]/

New failures:

arch/arm/boot/dts/exynos4212-tab3-3g8.dtb: rtc@10070000: clocks: [[5,
346]] is too short

> ---
> arch/arm/boot/dts/Makefile | 3 +
> arch/arm/boot/dts/exynos4212-tab3-3g8.dts | 29 +
> arch/arm/boot/dts/exynos4212-tab3-lte8.dts | 44 +
> arch/arm/boot/dts/exynos4212-tab3-wifi8.dts | 26 +
> arch/arm/boot/dts/exynos4212-tab3.dtsi | 1171 +++++++++++++++++++
> 5 files changed, 1273 insertions(+)
> create mode 100644 arch/arm/boot/dts/exynos4212-tab3-3g8.dts
> create mode 100644 arch/arm/boot/dts/exynos4212-tab3-lte8.dts
> create mode 100644 arch/arm/boot/dts/exynos4212-tab3-wifi8.dts
> create mode 100644 arch/arm/boot/dts/exynos4212-tab3.dtsi
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index efe4152e5846..e5f63b636637 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -217,6 +217,9 @@ dtb-$(CONFIG_ARCH_EXYNOS4) += \
> exynos4210-smdkv310.dtb \
> exynos4210-trats.dtb \
> exynos4210-universal_c210.dtb \
> + exynos4212-tab3-3g8.dtb \
> + exynos4212-tab3-lte8.dtb \
> + exynos4212-tab3-wifi8.dtb \
> exynos4412-i9300.dtb \
> exynos4412-i9305.dtb \
> exynos4412-itop-elite.dtb \
> diff --git a/arch/arm/boot/dts/exynos4212-tab3-3g8.dts b/arch/arm/boot/dts/exynos4212-tab3-3g8.dts
> new file mode 100644
> index 000000000000..6d890353ae76
> --- /dev/null
> +++ b/arch/arm/boot/dts/exynos4212-tab3-3g8.dts
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Samsung's Exynos4212 based Galaxy Tab 3 8.0 3G board device tree
> + * source
> + *
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + */
> +
> +/dts-v1/;
> +#include "exynos4212-tab3.dtsi"
> +
> +/ {
> + model = "Samsung Galaxy Tab 3 8.0 3G (SM-T311) based on Exynos4212";
> + compatible = "samsung,t311", "samsung,tab3", "samsung,exynos4212", "samsung,exynos4";
> + chassis-type = "tablet";
> +};
> +
> +/* Pin control sleep state overrides */
> +&sleep0 {
> + PIN_SLP(gpb-5, INPUT, UP);

Too much indentent.

> +};
> +
> +&sleep1 {
> + PIN_SLP(gpl0-0, OUT0, NONE);

Same here.

> + PIN_SLP(gpl1-0, OUT0, NONE);
> + PIN_SLP(gpl2-4, OUT0, NONE);
> + PIN_SLP(gpm3-3, OUT1, NONE);
> +};
> diff --git a/arch/arm/boot/dts/exynos4212-tab3-lte8.dts b/arch/arm/boot/dts/exynos4212-tab3-lte8.dts
> new file mode 100644
> index 000000000000..c5ec68c292b0
> --- /dev/null
> +++ b/arch/arm/boot/dts/exynos4212-tab3-lte8.dts
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Samsung's Exynos4212 based Galaxy Tab 3 8.0 LTE board device tree
> + * source
> + *
> + * Copyright (c) 2013 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + */
> +
> +/dts-v1/;
> +#include "exynos4212-tab3.dtsi"
> +
> +/ {
> + model = "Samsung Galaxy Tab 3 8.0 LTE (SM-T315) based on Exynos4212";
> + compatible = "samsung,t315", "samsung,tab3", "samsung,exynos4212", "samsung,exynos4";
> + chassis-type = "tablet";
> +};
> +
> +/* Pin control sleep state overrides */
> +&sleep0 {
> + PIN_SLP(gpa0-4, INPUT, UP);
> + PIN_SLP(gpa0-5, INPUT, UP);

Same.

> +
> + PIN_SLP(gpb-5, INPUT, UP);
> +
> + PIN_SLP(gpc0-0, PREV, NONE);
> + PIN_SLP(gpc1-3, INPUT, NONE);
> +
> + PIN_SLP(gpf1-6, INPUT, NONE);
> + PIN_SLP(gpf2-2, PREV, NONE);
> +};
> +
> +&sleep1 {
> + PIN_SLP(gpl0-0, PREV, NONE);
> +
> + PIN_SLP(gpl1-0, PREV, NONE);
> +
> + PIN_SLP(gpl2-1, INPUT, DOWN);
> + PIN_SLP(gpl2-2, INPUT, DOWN);
> + PIN_SLP(gpl2-4, OUT0, NONE);
> + PIN_SLP(gpl2-5, PREV, NONE);
> +
> + PIN_SLP(gpm3-3, OUT1, NONE);

...

> + buck5_reg: BUCK5 {
> + regulator-name = "VMEM_1.2V_AP";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + regulator-always-on;
> + regulator-boot-on;
> + op_mode = <1>;
> + };
> +
> + buck6_reg: BUCK6 {
> + regulator-name = "CAM_ISP_CORE_1.2V";
> + regulator-min-microvolt = <1200000>;
> + regulator-max-microvolt = <1200000>;
> + op_mode = <1>;
> +
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> + };
> +
> + s5m8767_osc: clocks {
> + compatible = "samsung,s5m8767-clk";
> + #clock-cells = <1>;
> + clock-output-names = "en32khz_ap",
> + "en32khz_cp",
> + "en32khz_bt";

Are these aligned with opening "?

> + };
> + };
> +};

Best regards,
Krzysztof

2023-05-09 18:27:20

by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v3 00/13] Re-introduce Exynos4212 support and add Samsung Galaxy Tab 3 8.0 boards

On 01/05/2023 21:55, Artur Weber wrote:
> This patches re-introduces the Exynos4212 platform and adds support
> for the Samsung Galaxy Tab 3 8.0 series of tablets that uses it:
>
> - Samsung Galaxy Tab 3 8.0 WiFi (SM-T310/lt01wifi)
> - Samsung Galaxy Tab 3 8.0 3G (SM-T311/lt013g)
> - Samsung Galaxy Tab 3 8.0 LTE (SM-T315/lt01lte)
>
> What works:
>
> - Display and backlight
> - Touchscreen (without touchkeys)
> - GPIO buttons, hall sensor
> - WiFi and Bluetooth
> - USB, fuel gauge, charging
> - Accelerometer and magnetometer
> - WiFi model only: light sensor

Thanks for the patches, nice work.

I applied parts of the set. Please:
1. Split the patchset per subsystems, so separate to media, phy and me.
2. Check reviews and resend bindings for display and LP855X.
3. Rebase and check/fix DTS and send a new version. I cannot apply DTS
without bindings being reviewed.

Best regards,
Krzysztof