Re: [PATCH v3 4/6] arm64: dts: imx8mm: Add Engicam i.Core MX8M Mini C.TOUCH 2.0

From: Krzysztof Kozlowski <krzk@kernel.org>
To: Jagan Teki <jagan@amarulasolutions.com>
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>
Subject: Re: [PATCH v3 4/6] arm64: dts: imx8mm: Add Engicam i.Core MX8M Mini C.TOUCH 2.0
Date: Thu, 24 Dec 2020 10:18:36 +0100	[thread overview]
Message-ID: <CAJKOXPfNP+6uv6PXmyTCimRd1hrQUbosnY--55yUQh4G4jfZWw@mail.gmail.com> (raw)
In-Reply-To: <CAMty3ZA0zrHS_wVKm8gdqU1q5ONokoT7aG_MBcWtXTJK59Bozg@mail.gmail.com>

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
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.

The same applies to imx8mm-engicam-edimm2.2.dtsi.

Best regards,
Krzysztof