lkml.org 
[lkml]   [2020]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/2] ARM: DTS: Add devicetree file for the Galaxy S2
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

\
 
 \ /
  Last update: 2020-03-18 16:24    [W:0.153 / U:1.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site