Message-ID: <20170103163136.rzoswklqevoeugn4@kozik-lap>
Date:   Tue, 3 Jan 2017 18:31:36 +0200
From:   Krzysztof Kozlowski <krzk@...nel.org>
To:     Chanwoo Choi <cwchoi00@...il.com>
Cc:     Andi Shyti <andi@...zian.org>,
        Javier Martinez Canillas <javier@....samsung.com>,
        Andi Shyti <andi.shyti@...sung.com>,
        Mark Rutland <mark.rutland@....com>,
        devicetree <devicetree@...r.kernel.org>,
        linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Jaechul Lee <jcsing.lee@...sung.com>,
        Will Deacon <will.deacon@....com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Chanwoo Choi <cw00.choi@...sung.com>,
        Rob Herring <robh+dt@...nel.org>,
        Kukjin Kim <kgene@...nel.org>, linux-input@...r.kernel.org,
        galaxyra@...il.com, beomho.seo@...sung.com,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 3/4] arm64: dts: exynos: make tm2 and tm2e independent
 from each other

On Wed, Jan 04, 2017 at 12:29:23AM +0900, Chanwoo Choi wrote:
> Hi Andi,
> 
> 2017-01-03 23:40 GMT+09:00 Andi Shyti <andi@...zian.org>:
> > Hi,
> >
> >> FWIW, I also agree with Chanwoo that the difference is too small to
> >> need a common .dtsi file.
> >
> > in principle I don't like "switching on and off" properties by
> > overwriting them with "status = disable", unless it's really
> > necessary (and this case is not). Even for small differences. It
> > makes the DTS harder to read and duplicates nodes with different
> > values throughout the DTS include chain.

I agree.

> >
> > In my opinion this approach should be discouraged.
> >
> > Besides, there are other overwritten differences in tm2e.dts that
> > I think should be separated as well. The "common" file approach is
> > widely used in arm/boot/dts/exynos* files.

I agree. Mostly we use:
1. Common DTSI and final addons/customizations in DTS.
2. Several common DTSI for specific parts (like sound).

> > The "status = disable" looks to me more like a temporary hack
> > rather than a permanent solution.
> >
> > In any case, still up to you :)
> >
> > Andi
> 
> I think that "status=disabled" of hsi2c_9 is not hack. The overwrite
> is possible for Device-tree. But, there is just difference how to
> support them with some method.
> 
> Except for touchkey, all peripheral device are same on both tm2 and
> tm2e. There are only small difference for a few property value.
> 
> To understand the difference between tm2 and tm2e, I made the patch
> (it is not complete version). If we implement the following patch, we
> support both tm2 and tm2e. So, I think that it is not complex to
> understand the h/w difference between tm2 and tm2e.

The difference below (I removed it from the quote) is indeed very small,
thanks Chanwoo for pointing this. However having common DTSI should not
end with much bigger final diff between files. I mean that it should be
the same amount of lines in total...

Actually, after applying this patch, the final exynos5433-tm2e.dts is
exactly the same as before. To me, this is a indication that a common
file is a good approach.


>From a separate Javier's mail:
> On 01/03/2017 11:40 AM, Andi Shyti wrote:
> > Hi,
> > 
> >> FWIW, I also agree with Chanwoo that the difference is too small to
> >> need a common .dtsi file.
> > 
> > in principle I don't like "switching on and off" properties by
> > overwriting them with "status = disable", unless it's really
> 
> This is a very good point. It would had been different if it was the
> opposite and tm2e had to enable the device node, but disabling it is
> indeed more confusing.
> On 01/03/2017 11:40 AM, Andi Shyti wrote:
> > Hi,
> > 
> >> FWIW, I also agree with Chanwoo that the difference is too small to
> >> need a common .dtsi file.
> > 
> > in principle I don't like "switching on and off" properties by
> > overwriting them with "status = disable", unless it's really
> 
> This is a very good point. It would had been different if it was the
> opposite and tm2e had to enable the device node, but disabling it is
> indeed more confusing.

I agree, 'status = disable' in final DTS is not a common pattern and
to me when reading, it makes me wonder what was the author's intention.

Also including a DTS in DTS is not a common pattern, neither. It appears
(git grep include arch/arm/boot/dts | grep 'dts"') in few places but it
is not obvious. At least to me.

Overall, I prefer the common approach.

Best regards,
Krzysztof

Powered by blists - more mailing lists