On Thu, Dec 24, 2020 at 3:51 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > > On Thu, 24 Dec 2020 at 11:08, Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > On Thu, Dec 24, 2020 at 2:48 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > > > > > > On Wed, 23 Dec 2020 at 13:07, Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > > > > > On Wed, Dec 23, 2020 at 5:29 PM Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > > > > > > > > > > On Wed, Dec 23, 2020 at 04:33:41PM +0530, Jagan Teki wrote: > > > > > > Engicam C.TOUCH 2.0 is an EDIMM compliant general purpose Carrier > > > > > > board. > > > > > > > > > > > > Genaral features: > > > > > > - Ethernet 10/100 > > > > > > - Wifi/BT > > > > > > - USB Type A/OTG > > > > > > - Audio Out > > > > > > - CAN > > > > > > - LVDS panel connector > > > > > > > > > > > > i.Core MX8M Mini is an EDIMM SoM based on NXP i.MX8M Mini from Engicam. > > > > > > > > > > > > i.Core MX8M Mini needs to mount on top of this Carrier board for > > > > > > creating complete i.Core MX8M Mini C.TOUCH 2.0 board. > > > > > > > > > > > > Add support for it. > > > > > > > > > > > > Signed-off-by: Matteo Lisi <matteo.lisi@xxxxxxxxxxx> > > > > > > Signed-off-by: Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> > > > > > > --- > > > > > > Changes for v3: > > > > > > - don't maintain common nodes and include it, if no feature diff > > > > > > Changes for v2: > > > > > > - enabled fec1 node > > > > > > - updated commit message > > > > > > - dropped engicam from filename since it aligned with imx6 engicam > > > > > > dts files naming conventions. > > > > > > - add i2c nodes > > > > > > - fixed v1 comments > > > > > > > > > > > > arch/arm64/boot/dts/freescale/Makefile | 1 + > > > > > > .../dts/freescale/imx8mm-engicam-ctouch2.dtsi | 82 +++++++++++++++++++ > > > > > > .../freescale/imx8mm-icore-mx8mm-ctouch2.dts | 21 +++++ > > > > > > 3 files changed, 104 insertions(+) > > > > > > create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-engicam-ctouch2.dtsi > > > > > > > > > > You split some common part to ctouch2.dtsi so it can be reused in > > > > > multiple places. I saw so far only one usage, where are the others? > > > > > > > > To be clear, ctouch2.dtsi not mean for common it is C.TOUCH2 carrier > > > > board dtsi. The other carrier is C.TOUCH2 10.1" Open Frame(display), > > > > since DSI is not yet mainlined, I didn't add this yet. > > > > > > If I understand correctly: it is a DTSI which is included only by one > > > DTS... and DTS does not have any other nodes. This as well is not the > > > > This is not mandatory as per my understanding, including exiting DTS > > topologies in Mainline. > > > > There are several places where more than one dtsi has been included, > > Simple example of imx8mm tree is > > It's not the problem of including more than one DTSI. It's the problem > of creating fake DTS or DTSI files whose purpose is only to include > others. Keep it simple. Don't create unnecessary files. "Entities > should not be multiplied without necessity." > > > > > arch/arm64/boot/dts/freescale/imx8mm-beacon-kit.dts > > Which was wrong as well. Don't create unnecessary files. > > > > > /dts-v1/; > > > > #include "imx8mm.dtsi" > > #include "imx8mm-beacon-som.dtsi" > > #include "imx8mm-beacon-baseboard.dtsi" > > > > (SoC dtsi, SoM dtsi, Carrier board dtsi) > > > > > design which makes any sense. We do not create empty DTS files which > > > only include one more DTSI. The contents of > > > imx8mm-engicam-ctouch2.dtsi should be directly in > > > imx8mm-icore-mx8mm-ctouch2.dts. That's the same problem as with v1 - > > > you overcomplicate simple stuff. It really looks like you ignored the > > > comments from v1 in multiple places. > > > > As explained above, the design is pretty much the same as the existing SoM's. > > > > imx8mm-engicam-ctouch2.dtsi is not just a dtsi file where nodes are > > enabled. It has nodes enabled for Carrier board, so keeping nodes > > separately will > > The files represent real devices or their components. So you have a > SOM - a DTSI file. You have a carrier board - a DTS file. That's > simple design which is mostly followed, unless something over > complicated passes the review. > > > 1. More verbose for which IP's are available in the carrier board > > No difference when carrier DTSI is the DTS. Exactly the same. > > > 2. Easy to extend if someone can create another SoM with a similar Carrier. > > Not really, if they include carrier DTSI they need to override a lot. > So usually (including practice - I did it) they *copy* the carrier to > create their own design. But what if the new board has slite change to use exiting carrier like what ctouch2 10" OF. Can we add ctouch2 dtsi as a separate file for this case? > > > > > Ie is the whole idea to keep carrier board dtsi and includes them in dts. > > > > As I suggest, if you can look into px30 you can understand more easily. > > NAK from my side. I explained my reasoning. You created a fake, empty > DTSI which included only other DTSI. After review, you agreed to fix > it. However you still create a fake DTS which includes only a DTSI. Not sure. I have updated the series according to comments by dropping -common.dtsi ie what I was thought of "empty DTSI inclusion" you have pointed at previous versions. Jagan.
