Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201223110343.126638-1-jagan@amarulasolutions.com>
 <20201223110343.126638-5-jagan@amarulasolutions.com> <20201223115933.GC21091@kozik-lap>
 <CAMty3ZA0zrHS_wVKm8gdqU1q5ONokoT7aG_MBcWtXTJK59Bozg@mail.gmail.com>
 <CAJKOXPfNP+6uv6PXmyTCimRd1hrQUbosnY--55yUQh4G4jfZWw@mail.gmail.com>
 <CAMty3ZBWteW2OZMNwFT5hSRegy-AARqqkR7SAz53fDoM3j5Ong@mail.gmail.com> <CAJKOXPey8+k7_uT4LLTwo8-UY51+TwUgWhtkPDRW3AD-BCi1Bw@mail.gmail.com>
In-Reply-To: <CAJKOXPey8+k7_uT4LLTwo8-UY51+TwUgWhtkPDRW3AD-BCi1Bw@mail.gmail.com>
From:   Jagan Teki <jagan@amarulasolutions.com>
Date:   Mon, 28 Dec 2020 13:50:54 +0530
Message-ID: <CAMty3ZAo+9L4rtkJgBkimJu4SPv=gycVUFaUYtL_RSAhs7yGWw@mail.gmail.com>
Subject: Re: [PATCH v3 4/6] arm64: dts: imx8mm: Add Engicam i.Core MX8M Mini
 C.TOUCH 2.0
To:     Krzysztof Kozlowski <krzk@kernel.org>
Cc:     Rob Herring <robh+dt@kernel.org>, Shawn Guo <shawnguo@kernel.org>,
        Li Yang <leoyang.li@nxp.com>,
        Fabio Estevam <festevam@gmail.com>,
        devicetree <devicetree@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        NXP Linux Team <linux-imx@nxp.com>,
        linux-amarula <linux-amarula@amarulasolutions.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Will Deacon <will@kernel.org>,
        Matteo Lisi <matteo.lisi@engicam.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Dec 24, 2020 at 3:51 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Thu, 24 Dec 2020 at 11:08, Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > On Thu, Dec 24, 2020 at 2:48 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > >
> > > On Wed, 23 Dec 2020 at 13:07, Jagan Teki <jagan@amarulasolutions.com> wrote:
> > > >
> > > > On Wed, Dec 23, 2020 at 5:29 PM Krzysztof Kozlowski <krzk@kernel.org> 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@engicam.com>
> > > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > > ---
> > > > > > 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.