| Message ID | CAGTfZH0tnjkqF_7DChLSvzs3C369mkb_HZmyrf3cc0T-UcBZ4A@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@etezian.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
diff --git a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts index 1db4e7f..09b6935 100644 --- a/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts +++ b/arch/arm64/boot/dts/exynos/exynos5433-tm2e.dts @@ -18,6 +18,17 @@ compatible = "samsung,tm2e", "samsung,exynos5433"; }; +&display_timings { + clock-frequency = <16523724>; + hactive = <1600>; +}; + +&hsi2c_9 { + /* TM2E don't use the separate touchkey device. Instead, touchscreen + * device support the touchkey device.*/ + status = "disabled"; +}; + &ldo23_reg { regulator-name = "CAM_SEN_CORE_1.025V_AP"; regulator-max-microvolt = <1050000>; @@ -39,3 +50,7 @@ regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; }; + +&touchscreen { + x-size = "1599"; +};