Re: [PATCH 2/2] ARM: DTS: Add devicetree file for the Galaxy S2

From: Krzysztof Kozlowski <krzk@kernel.org>
To: Paul Cercueil <paul@crapouillou.net>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>,
	Kukjin Kim <kgene@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Stenkin Evgeniy <stenkinevgeniy@gmail.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-samsung-soc@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] ARM: DTS: Add devicetree file for the Galaxy S2
Date: Wed, 18 Mar 2020 16:23:22 +0100	[thread overview]
Message-ID: <20200318152322.GA4112@kozik-lap> (raw)
In-Reply-To: <20200318142543.C167520772@mail.kernel.org>

On Wed, Mar 18, 2020 at 03:25:22PM +0100, Paul Cercueil wrote:
> Hi Krzysztof,
> 
> 
> Le ven. 13 mars 2020 à 10:00, Krzysztof Kozlowski <krzk@kernel.org> a écrit
> :
> > On Thu, Mar 12, 2020 at 04:34:11PM +0100, Paul Cercueil wrote:
> > >  From: Stenkin Evgeniy <stenkinevgeniy@gmail.com>
> > > 
> > >  Add devicetree file for the Exynos 4210 based Galaxy S2 (i9100
> > > version).
> > 
> > Thanks for the patch!
> > 
> > It is an GT-I9100.
> > 
> > +Cc Marek (I have few questions further)
> > 
> > 
> > > 
> > >  Signed-off-by: Stenkin Evgeniy <stenkinevgeniy@gmail.com>
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  ---
> > >   arch/arm/boot/dts/Makefile             |   1 +
> > >   arch/arm/boot/dts/exynos4210-i9100.dts | 816
> > > +++++++++++++++++++++++++
> > >   2 files changed, 817 insertions(+)
> > >   create mode 100644 arch/arm/boot/dts/exynos4210-i9100.dts
> > > 
> > >  diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > >  index d6546d2676b9..522436d30690 100644
> > >  --- a/arch/arm/boot/dts/Makefile
> > >  +++ b/arch/arm/boot/dts/Makefile
> > >  @@ -181,6 +181,7 @@ dtb-$(CONFIG_ARCH_EXYNOS3) += \
> > >   	exynos3250-monk.dtb \
> > >   	exynos3250-rinato.dtb
> > >   dtb-$(CONFIG_ARCH_EXYNOS4) += \
> > >  +	exynos4210-i9100.dtb \
> > >   	exynos4210-origen.dtb \
> > >   	exynos4210-smdkv310.dtb \
> > >   	exynos4210-trats.dtb \
> > >  diff --git a/arch/arm/boot/dts/exynos4210-i9100.dts
> > > b/arch/arm/boot/dts/exynos4210-i9100.dts
> > >  new file mode 100644
> > >  index 000000000000..c218e34959be
> > >  --- /dev/null
> > >  +++ b/arch/arm/boot/dts/exynos4210-i9100.dts
> > >  @@ -0,0 +1,816 @@
> > >  +// SPDX-License-Identifier: GPL-2.0
> > >  +/*
> > >  + * Samsung's Exynos4210 based Galaxy S2 (i9100 version) device tree
> > 
> > Ditto
> > 
> > >  + *
> > >  + * Copyright (c) 2012 Samsung Electronics Co., Ltd.
> > >  + *		http://www.samsung.com
> > >  + * Copyright (c) 2020 Stenkin Evgeniy <stenkinevgeniy@gmail.com>
> > >  + * Copyright (c) 2020 Paul Cercueil <paul@crapouillou.net>
> > >  + *
> > >  + * Device tree source file for Samsung's Galaxy S2 smartphone
> > > (i9100 version),
> > >  + * which is based on Samsung's Exynos4210 SoC.
> > 
> > Duplicated description, you mentioned it before.
> > 
> > >  + */
> > >  +
> > >  +/dts-v1/;
> > >  +#include "exynos4210.dtsi"
> > >  +#include "exynos4412-ppmu-common.dtsi"
> > >  +
> > >  +#include <dt-bindings/gpio/gpio.h>
> > >  +#include <dt-bindings/input/linux-event-codes.h>
> > >  +
> > >  +/ {
> > >  +	model = "Samsung Galaxy S2 (i9100)";
> > 
> > GT-I9100
> > 
> > >  +	compatible = "samsung,i9100", "samsung,exynos4210",
> > > "samsung,exynos4";
> > 
> > I think it shares a lot with Trats board...
> > 
> > Marek, how similar these
> > designs are? Maybe it is worth to split some parts and make common part?
> > 
> > >  +
> > >  +
> > >  +	memory@40000000 {
> > >  +		device_type = "memory";
> > >  +		reg = <0x40000000 0x40000000>;
> > >  +	};
> > >  +
> > 
> > You do not have a chosen node, at least for stdout serial selection.
> > Probably you should have one... bootargs I guess could come from
> > bootloader.
> > 
> > 
> > >  +	vemmc_reg: regulator-0 {
> > >  +		compatible = "regulator-fixed";
> > >  +		regulator-name = "VMEM_VDD_2.8V";
> > >  +		regulator-min-microvolt = <2800000>;
> > >  +		regulator-max-microvolt = <2800000>;
> > >  +		gpio = <&gpk0 2 GPIO_ACTIVE_HIGH>;
> > >  +		enable-active-high;
> > 
> > This looks like a stub/fake regulator. Probably it shoould be replaced
> > with proper regulator coming from PMIC (few seems to be missing there).
> > 
> > I don't have the schematics so I cannot judge... I see Trats has the
> > same.
> > 
> > Marek, mayybe you know, is it really a separate regulator?
> > 
> > >  +	};
> > >  +
> > >  +	tsp_reg: regulator-1 {
> > >  +		compatible = "regulator-fixed";
> > >  +		regulator-name = "TSP_FIXED_VOLTAGES";
> > >  +		regulator-min-microvolt = <3300000>;
> > >  +		regulator-max-microvolt = <3300000>;
> > >  +		gpio = <&gpl0 3 GPIO_ACTIVE_HIGH>;
> > >  +		startup-delay-us = <70000>;
> > >  +		enable-active-high;
> > >  +		regulator-boot-on;
> > >  +		regulator-always-on;
> > 
> > always-on and boot-on should not be needed. You have a consumer for this
> > regulator.
> 
> About this: the touchscreen driver does not use a regulator, so I believe
> that's why these properties were here.
> 
> I sent patches upstream to address the issue:
> https://lkml.org/lkml/2020/3/15/94
> 
> I believe this means I cannot merge the i9100 devicetree until it is acked.

This can stay because it is correct with current bindings and driver.
However you should remove avdd-supply/vdd-supply from touchscreen node
because they are not used/supported.

You can re-add them once bindings get accepted.

Best regards,
Krzysztof