diff mbox

[3/4] arm64: dts: exynos: make tm2 and tm2e independent from each other

Message ID CAGTfZH0tnjkqF_7DChLSvzs3C369mkb_HZmyrf3cc0T-UcBZ4A@mail.gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Chanwoo Choi Jan. 3, 2017, 3:29 p.m. UTC
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.
>
> 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.
>
> 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.

Comments

Krzysztof Kozlowski Jan. 3, 2017, 4:31 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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";
+};